Skip to content

Makefile.include: RIOTNOLINK ensure linking fails#11168

Closed
cladmi wants to merge 1 commit intoRIOT-OS:masterfrom
cladmi:pr/make/ensure_link_fails
Closed

Makefile.include: RIOTNOLINK ensure linking fails#11168
cladmi wants to merge 1 commit intoRIOT-OS:masterfrom
cladmi:pr/make/ensure_link_fails

Conversation

@cladmi
Copy link
Copy Markdown
Contributor

@cladmi cladmi commented Mar 12, 2019

Contribution description

This checks that linking correctly fails when it is supposed to.
This should help keeping the BOARD_INSUFFICIENT_MEMORY list up to date.

Testing procedure

This verifies the board is indeed not linking:

RIOT_CI_BUILD=1 BOARD=arduino-uno make  -C examples/default
make: Entering directory '/home/harter/work/git/RIOT/examples/default'
CI-build: skipping link step
Building application "default" for "arduino-uno" with MCU "atmega328p".

/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x800a56 of /home/harter/work/git/RIOT/examples/default/bin/arduino-uno/default.elf.nolink section `.data' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x800e1b of /home/harter/work/git/RIOT/examples/default/bin/arduino-uno/default.elf.nolink section `.bss' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x800e1d of /home/harter/work/git/RIOT/examples/default/bin/arduino-uno/default.elf.nolink section `.noinit' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x800a56 of /home/harter/work/git/RIOT/examples/default/bin/arduino-uno/default.elf.nolink section `.data' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x800e1b of /home/harter/work/git/RIOT/examples/default/bin/arduino-uno/default.elf.nolink section `.bss' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x800e1d of /home/harter/work/git/RIOT/examples/default/bin/arduino-uno/default.elf.nolink section `.noinit' is not within region `data'
collect2: error: ld returned 1 exit status
make: Leaving directory '/home/harter/work/git/RIOT/examples/default'

If the board should not link (here overwritten) but does, it shows an error:

RIOT_CI_BUILD=1 BOARD=samr21-xpro make -C examples/default/  BOARD_INSUFFICIENT_MEMORY='$(BOARD)'
make: Entering directory '/home/harter/work/git/RIOT/examples/default'
CI-build: skipping link step
Building application "default" for "samr21-xpro" with MCU "samd21".

Error, linking should have failed because of not enough memory but did not
/home/harter/work/git/RIOT/examples/default/../../Makefile.include:466: recipe for target '/home/harter/work/git/RIOT/examples/default/bin/samr21-xpro/default.elf.nolink' failed
make: *** [/home/harter/work/git/RIOT/examples/default/bin/samr21-xpro/default.elf.nolink] Error 1
make: Leaving directory '/home/harter/work/git/RIOT/examples/default'

Issues/PRs references

#11128

@cladmi cladmi added Area: tests Area: tests and testing framework Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 12, 2019
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Mar 12, 2019

Nice!

I suggest only diverting the error output to the file. Then we can add LINKFLAGS += -Wl,--print-memory-usage, which would show nice stats for most platforms (not native though).

jcarrano
jcarrano reviewed Mar 13, 2019
View reviewed changes
Makefile.include
$(ELFFILE).nolink: FORCE
$(ELFFILE).nolink: $(BASELIBS)
$(Q)\
$(_LINK) -o $(ELFFILE) 2>&1 > $@; \
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$(_LINK) -o $(ELFFILE) 2>&1 > $@; \
$(_LINK) -o /dev/null 2>&1 > $@; \

This way we don't have to worry about cleanup if linking succeeds.

Copy link
Copy Markdown
Contributor Author

@cladmi cladmi Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left ELFFILE on purpose, because you could check the elffile if it succeeds and you would like to analyze it. All the files from BASELIBS are also still there.

But I can change it if wanted.

@kaspar030 kaspar030 mentioned this pull request Apr 11, 2019
Merged
@cladmi
Makefile.include: RIOTNOLINK ensure linking fails
3b17337 
This checks that linking correctly fails when it is supposed to.
This should help keeping the `BOARD_INSUFFICIENT_MEMORY` list up to date.
@cladmi cladmi force-pushed the pr/make/ensure_link_fails branch from 50c605f to 3b17337 Compare May 28, 2019 17:17
@cladmi cladmi mentioned this pull request May 28, 2019
Merged
@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Jun 3, 2019

The solution I would like to do here, would be to have a global or per architecture file of not enough memory patterns and if the output of the linker is not only not enough memory, trigger an error. (it may require forcing LC=C)

However, this requires communicating with the tests to know if the test must be run or not in dist/tools/compile_and_test_for_board.py and murdock.

@cladmi cladmi added this to the Release 2019.10 milestone Jun 14, 2019
@kb2ma kb2ma modified the milestones: Release 2019.10, Release 2020.01 Oct 9, 2019
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jan 1, 2020

I suggest only diverting the error output to the file. Then we can add LINKFLAGS += -Wl,--print-memory-usage, which would show nice stats for most platforms (not native though).

@kaspar030 would you want to push your suggested change? I can finish the review afterwards.

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jan 15, 2020

This one seemed almost ready, but I would rather not have it in since soft feature freeze is already here. So I'm moving the milestone.

@leandrolanzieri leandrolanzieri removed this from the Release 2020.04 milestone Apr 8, 2020
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 25, 2020

There is still one day left to get this into 2020.07 ;-)

@stale
Copy link
Copy Markdown

stale bot commented Dec 28, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Dec 28, 2020
@stale stale bot closed this Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smlng smlng Awaiting requested review from smlng

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

1 more reviewer

@jcarrano jcarrano jcarrano left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: stale State: The issue / PR has no activity for >185 days

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cladmi @kaspar030 @fjmolinas @miri64 @jcarrano @kb2ma @leandrolanzieri