Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core: compile kernel_init and core_panic conditionally #13238

Closed
wants to merge 1 commit into from
Closed

core: compile kernel_init and core_panic conditionally #13238

wants to merge 1 commit into from

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Jan 30, 2020

Contribution description

riotboot provides its own implementation of these functions in bootloaders/riotboot/main.c. Compiling them unconditionally causes two symbols with the same name to be present for core_panic and kernel_init, the linker will discard one of them.

Testing procedure

This was discovered during the implementation of link time reordering in #13176. Either test this using the aforementioned PR or read the code and check with tests/riotboot that both bootloaders/riotboot/main.c and core/kernel_init.c as well as core/panic.c are compiled and contain symbols with the same name.

Issues/PRs references

@nmeum
core: compile kernel_init and core_panic conditionally
ba2c314 
RIOTBOOT provides its own implementation of these functions in
`bootloaders/riotboot/*`. Compiling them unconditionally causes two
symbols with the same name to be present, the linker will discard one of
them.
@benpicco benpicco added Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jan 30, 2020
@nmeum
Copy link
Member Author

nmeum commented Jan 30, 2020

From Murdock:

--- result: BUILD FAILED!

--- static tests: failed!
dwqw: command timed out

What does this mean? What is dwqw?

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 30, 2020
@kaspar030
Copy link
Contributor

What does this mean? What is dwqw?

dwqw is the tool that executes jobs on the worker. The message means that the job timed out (ran for more than 5 minutes).

benpicco
benpicco approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Makes sense and Murdock is happy.

@kaspar030
Copy link
Contributor

I'm not happy with the RIOTBOOT defines.
I suggest the following:

  • rename "kernel_init" "core_init"

  • make it a default (sub-)module

  • disable it for RIOTBOOT

  • move the minimal "panic()" to panic.c

  • make it selectable as module

I'll try how it looks and report back.

@kaspar030 kaspar030 mentioned this pull request Jan 31, 2020
Merged
kaspar030
kaspar030 requested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

I think turning both kernel_init and core panic into submodules is cleaner.

@nmeum
Copy link
Member Author

nmeum commented Jan 31, 2020

I prefer @kaspar030's solution closing this.

@nmeum nmeum closed this Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 requested changes

@benpicco benpicco benpicco approved these changes

Assignees
No one assigned
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nmeum @kaspar030 @benpicco