cpu/arm7_common: Moved compiler flags here by maribu · Pull Request #11882 · RIOT-OS/RIOT

maribu · 2019-07-22T13:29:39Z

Contribution description

Moved compiler & linker flags from boards/common/msba2 to cpu/arm7_common
Moved dependency to newlib nano to cpu/arm7_common
Moved config to link in cpu/startup.o to cpu/arm7_common

Testing procedure

Select a few random example applications and check if the still build, flash and run as expected

Issues/PRs references

This PR is a response to issue #11861, the actual fix for issue #11861 was done in #11883

maribu · 2019-07-22T13:30:32Z

Maybe @cladmi can check, if the changes to the Makefiles make sense? Maybe @miri64 can confirm, if this really solves issue #11861?

maribu · 2019-07-22T13:30:59Z

Let's check if Murdock can find any issues.

cladmi · 2019-07-22T14:46:58Z

For me this is mixing refactoring and fixing #11861 as from what I understand, the fix would only be to define USE_NANO_SPECS = 1.
Which I prefer in two different PRs as it took some time to understand what made it work.

I even think that it would be better to replace the check against USE_NANO_SPECS with newlib_nano

diff --git a/sys/Makefile.include b/sys/Makefile.include

diff --git a/sys/Makefile.include b/sys/Makefile.include
index a9ab8095e..bdc8147ff 100644
--- a/sys/Makefile.include
+++ b/sys/Makefile.include
@@ -72,13 +72,13 @@ ifneq (,$(filter arduino,$(USEMODULE)))
 endif
 
 ifneq (,$(filter printf_float,$(USEMODULE)))
-  ifeq (1,$(USE_NANO_SPECS))
+  ifneq (,$(filter newlib_nano,$(USEMODULE)))
     export LINKFLAGS += -u _printf_float
   endif
 endif
 
 ifneq (,$(filter scanf_float,$(USEMODULE)))
-  ifeq (1,$(USE_NANO_SPECS))
+  ifneq (,$(filter newlib_nano,$(USEMODULE)))
     export LINKFLAGS += -u _scanf_float
   endif
 endif

For me it fixes the test:

BOARD=msba2 make --no-print-directory -C tests/unittests/ tests-scanf_float flash-only test

BOARD=msba2 make --no-print-directory -C tests/unittests/ tests-scanf_float flash-only test

Booting (hardware reset)...

Reset CPU (into user code)
Programming done.
ssh -t vaduz.imp.fu-berlin.de 'source /srv/ilab-builds/workspace/workspace.rc && BOARD=msba2 QUIET=0 make --no-print-directory -C /srv/ilab-builds/boards term'
/srv/ilab-builds/boards/RIOT/dist/tools/pyterm/pyterm -tg -p "/dev/riot/ttyMSBA2"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2019-07-22 16:37:43,181 - INFO # Connect to serial port /dev/riot/ttyMSBA2
Welcome to pyterm!
Type '/exit' to exit.
2019-07-22 16:37:44,187 - INFO # main(): This is RIOT! (Version: 2019.10-devel-104-g294fb-HEAD)
2019-07-22 16:37:44,187 - INFO # ......
2019-07-22 16:37:44,188 - INFO # OK (6 tests)

I do not see a reason for this USE_NANO_SPECS to still exist. (I did not check the history yet).
I can check the history, impact and provide a change to remove this USE_NANO_SPECS in RIOT if that is indeed what fixed your general case.

Not changing to `makefiles/arch`

As I planned to change this for cortexm_common and the others, I think that moving to makefiles/arch is a bad thing.
I do not understand why there is this indirection in the first place for cortexm.

I would prefer simply moving the definitions to cpu/arm7_common as indeed for example the CFLAGS and LINKFLAGS setting ares more a cpu than a board thing.

Reasoning

There is still a dependency from makefiles/arch to cpu/ARCH_NAME_common. So it is not more generic than when being in cpu.
The inclusion result in being (for cortexm boars) board -> board/common -> cpu/ cpu/common -> arch -> cpu/arch_common which does not make much sense.

Also the arch directory currently prevents having a .dep, and a .features, which is blocking to solve #9913 as dependencies are still declared in the .include equivalent file.

General remark

For a refactoring, better only move required things first without adding everything that is done in cortexm.inc.mk, there are things that should be cleaned there

Setting CPU_MODEL if it is not needed
setting the default goal as it is already handled (I will remove it for cortexm).
is the .lst rule is useful? if yes, should it be set globally?

miri64 · 2019-07-22T21:11:27Z

With #11883 merged: can this be closed?

maribu · 2019-07-23T02:50:07Z

I'll rebase update the cleanup according to @cladmi's comments. Moving the ARM7 compiler and linker flags from boards to cpu/arm7_common seems to still make sense.

maribu · 2019-07-23T04:53:10Z

OK, I updated to only contain the cleanup. The compiler & linker flags have now been moved to cpu/arm7_common/Makefile.include.

cladmi · 2019-08-26T18:12:26Z

The changes are good, they remove setting compiler options in the board when it belongs more to the cpu (or arch) part.
It is the case for all boards in RIOT except native currently.

In master it was:

git grep -e CFLAGS -e LINKFLAGS boards ':!**/Makefile' ':!**/doc.txt'

git grep -e CFLAGS -e LINKFLAGS boards ':!**/Makefile' ':!**/doc.txt'
boards/common/msba2/Makefile.include:export CFLAGS_CPU   = -mcpu=arm7tdmi-s
boards/common/msba2/Makefile.include:export CFLAGS_LINK  = -ffunction-sections -fdata-sections -fno-builtin -fshort-enums
boards/common/msba2/Makefile.include:export CFLAGS_DBG  ?= -ggdb -g3
boards/common/msba2/Makefile.include:export CFLAGS_OPT  ?= -Os
boards/common/msba2/Makefile.include:export CFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT)
boards/common/msba2/Makefile.include:export ASFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG)
boards/common/msba2/Makefile.include:export LINKFLAGS += -T$(RIOTCPU)/$(CPU)/ldscripts/$(CPU).ld
boards/common/msba2/Makefile.include:export LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) -static -lgcc -nostartfiles
boards/common/msba2/Makefile.include:export LINKFLAGS += -Wl,--gc-sections
boards/common/nrf51/Makefile.include:  CFLAGS += -DBOARD_NRF51_XTIMER_ALT
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DENC28J60_PARAM_SPI=$(ENC28J60_PARAM_SPI)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DENC28J60_PARAM_CS=$(ENC28J60_PARAM_CS)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DENC28J60_PARAM_INT=$(ENC28J60_PARAM_INT)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DENC28J60_PARAM_RESET=$(ENC28J60_PARAM_RESET)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DMRF24J40_PARAM_SPI=$(MRF24J40_PARAM_SPI)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DMRF24J40_PARAM_SPI_CLK=$(MRF24J40_PARAM_SPI_CLK)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DMRF24J40_PARAM_CS=$(MRF24J40_PARAM_CS)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DMRF24J40_PARAM_INT=$(MRF24J40_PARAM_INT)
boards/esp32-wroom-32/Makefile.dep:        CFLAGS += -DMRF24J40_PARAM_RESET=$(MRF24J40_PARAM_RESET)
boards/mulle/Makefile.include:  CFLAGS += -DMULLE_SERIAL=$(MULLE_SERIAL)
boards/native/Makefile.dep:    CFLAGS += -DCAN_DLL_NUMOF=2
boards/native/Makefile.include:export CFLAGS += -Wall -Wextra -pedantic
boards/native/Makefile.include:ifeq (,$(filter -std=%, $(CFLAGS)))
boards/native/Makefile.include:  export CFLAGS += -std=gnu99
boards/native/Makefile.include:  export CFLAGS += -m32
boards/native/Makefile.include:ifneq (,$(filter -DDEVELHELP,$(CFLAGS)))
boards/native/Makefile.include:  export CFLAGS += -fstack-protector-all
boards/native/Makefile.include:    export CFLAGS += -m32 -DCOMPAT_32BIT -B/usr/lib32
boards/native/Makefile.include:  export CFLAGS += -Wno-deprecated-declarations
boards/native/Makefile.include:  export LINKFLAGS += -m32
boards/native/Makefile.include:    export LINKFLAGS += -m32 -DCOMPAT_32BIT -L/usr/lib32 -B/usr/lib32
boards/native/Makefile.include:  export LINKFLAGS += -L $(BINDIR)
boards/native/Makefile.include:  export LINKFLAGS += -ldl
boards/native/Makefile.include:export CFLAGS += -ffunction-sections -fdata-sections
boards/native/Makefile.include:  export LINKFLAGS += -Wl,-dead_strip
boards/native/Makefile.include:  export LINKFLAGS += -Wl,--gc-sections
boards/native/Makefile.include:export LINKFLAGS += -ffunction-sections
boards/native/Makefile.include:all-valgrind: export CFLAGS += -DHAVE_VALGRIND_H -g3
boards/native/Makefile.include:all-debug: export CFLAGS += -g3
boards/native/Makefile.include:all-cachegrind: export CFLAGS += -g3
boards/native/Makefile.include:all-gprof: export CFLAGS += -pg
boards/native/Makefile.include:all-gprof: export LINKFLAGS += -pg
boards/native/Makefile.include:all-asan: export CFLAGS += -fsanitize=address -fno-omit-frame-pointer -g3
boards/native/Makefile.include:all-asan: export CFLAGS += -DNATIVE_IN_CALLOC
boards/native/Makefile.include:all-asan: export LINKFLAGS += -fsanitize=address -fno-omit-frame-pointer -g3
boards/native/Makefile.include:export CFLAGS += -DDEBUG_ASSERT_VERBOSE
boards/native/Makefile.include:  export CFLAGS += -DHAVE_NO_BUILTIN_BSWAP16
boards/native/Makefile.include:   LINKFLAGS += -lrt
boards/saml10-xpro/Makefile.include:export CFLAGS += -D__SAML10E16A__
boards/saml11-xpro/Makefile.include:export CFLAGS += -D__SAML11E16A__
boards/saml21-xpro/Makefile.include:export CFLAGS += -D__SAML21J18A__
boards/teensy31/Makefile.include:       CC= CFLAGS= make -C $(RIOTTOOLS)/teensy-loader-cli

benpicco · 2019-09-10T22:09:16Z

Another rebase is needed.

- Moved compiler & linker flags from boards/common/msba2 to cpu/arm7_common - Moved dependency to newlib nano to cpu/arm7_common - Moved config to link in cpu/startup.o to cpu/arm7_common

maribu · 2019-09-10T23:04:52Z

Rebased and manually applied the changes from #12098 (the dropping of the export) to the new location of the compiler flags in cpu/arm7_common/Makefile.include

benpicco

The flags are still the same, they just moved to a different place now.

maribu · 2019-09-11T08:17:09Z

Thanks for the review. I was fearing that no one has the interest to review my ARM7 PRs.

maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system labels Jul 22, 2019

maribu requested a review from miri64 July 22, 2019 13:29

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 22, 2019

maribu mentioned this pull request Jul 22, 2019

msba2: floating point tests fail #11861

Closed

cladmi mentioned this pull request Jul 22, 2019

sys/Makefile.include: check for newlib_nano instead of USE_NANO_SPECS #11883

Merged

3 tasks

maribu force-pushed the arm7_buildsystem branch from 0ee76da to 1299daa Compare July 23, 2019 04:48

maribu changed the title ~~build system: Restructured ARM7 Makefiles~~ cpu/arm7_common: Moved compiler flags here Jul 23, 2019

maribu added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation and removed Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jul 23, 2019

cladmi mentioned this pull request Jul 23, 2019

sys/Makefile.include: check for newlib_nano instead of USE_NANO_SPECS [backport 2019.07] #11890

Closed

3 tasks

cladmi mentioned this pull request Aug 28, 2019

make: do not locally export compilation variables #12098

Merged

cpu/arm7_common: Moved compiler flags here

7092566

- Moved compiler & linker flags from boards/common/msba2 to cpu/arm7_common - Moved dependency to newlib nano to cpu/arm7_common - Moved config to link in cpu/startup.o to cpu/arm7_common

maribu force-pushed the arm7_buildsystem branch from 1299daa to 7092566 Compare September 10, 2019 23:03

benpicco approved these changes Sep 11, 2019

View reviewed changes

benpicco merged commit 410e6ed into RIOT-OS:master Sep 11, 2019

maribu deleted the arm7_buildsystem branch September 11, 2019 08:17

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/arm7_common: Moved compiler flags here#11882

cpu/arm7_common: Moved compiler flags here#11882
benpicco merged 1 commit intoRIOT-OS:masterfrom
maribu:arm7_buildsystem

maribu commented Jul 22, 2019 •

edited

Loading

Uh oh!

maribu commented Jul 22, 2019

Uh oh!

maribu commented Jul 22, 2019

Uh oh!

cladmi commented Jul 22, 2019

Uh oh!

miri64 commented Jul 22, 2019

Uh oh!

maribu commented Jul 23, 2019

Uh oh!

maribu commented Jul 23, 2019

Uh oh!

cladmi commented Aug 26, 2019

Uh oh!

benpicco commented Sep 10, 2019

Uh oh!

maribu commented Sep 10, 2019

Uh oh!

benpicco left a comment

Uh oh!

maribu commented Sep 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

maribu commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu commented Jul 22, 2019

Uh oh!

maribu commented Jul 22, 2019

Uh oh!

cladmi commented Jul 22, 2019

Not changing to makefiles/arch

Reasoning

General remark

Uh oh!

miri64 commented Jul 22, 2019

Uh oh!

maribu commented Jul 23, 2019

Uh oh!

maribu commented Jul 23, 2019

Uh oh!

cladmi commented Aug 26, 2019

Uh oh!

benpicco commented Sep 10, 2019

Uh oh!

maribu commented Sep 10, 2019

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Sep 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maribu commented Jul 22, 2019 •

edited

Loading

Not changing to `makefiles/arch`