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: turn kernel_init.c and panic.c into submodules of core #13241

Merged
merged 2 commits into from
Jan 31, 2020
Merged

core: turn kernel_init.c and panic.c into submodules of core #13241

merged 2 commits into from
Jan 31, 2020

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Jan 31, 2020 •
edited
Loading

Contribution description

This PR changes the default kernel_init to be a submodule called "core_init".
That makes it possible to completely bypass it, as done by riotboot.
Previously, riotboot depended on a specific link order to properly override kernel_init()
core_init is made a default module, riotboot simply disables it.

Same with panic.c turned into the core_panic module.

Testing procedure

  • basic compilation and execution should still work
  • riotboot should be tested
  • actual code sizes should be unchanged

CI can do that.

Issues/PRs references

#13238

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation 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 Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Jan 31, 2020
nmeum
nmeum reviewed Jan 31, 2020
View reviewed changes
bootloaders/riotboot/Makefile Outdated
@@ -14,6 +14,7 @@ CFLAGS += -DRIOTBOOT

# Disable unused modules
CFLAGS += -DNDEBUG -DLOG_LEVEL=LOG_NONE
DISABLE_MODULE += core_init core_msg
Copy link
Member

Choose a reason for hiding this comment

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

What about core_panic, shouldn't it be disabled as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about core_panic, shouldn't it be disabled as well?

Yes, that'll be another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that'll be another PR.

actually, that doesn't make sense. They both require exactly the same testing.

@kaspar030 kaspar030 added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer and removed Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Jan 31, 2020
@kaspar030 kaspar030 force-pushed the introduce_core_init branch from 2408bf4 to b6703f4 Compare January 31, 2020 12:53
@kaspar030 kaspar030 changed the title core: turn kernel_init.c into core_init submodule core: turn kernel_init.c and panic.c into core_init submodules Jan 31, 2020
@kaspar030
Copy link
Contributor Author

  • rebased, changed panic.c similar to kernel_init.c

@nmeum Does this fix the issues with linktime re-ordering?

@kaspar030 kaspar030 changed the title core: turn kernel_init.c and panic.c into core_init submodules core: turn kernel_init.c and panic.c into submodules of core Jan 31, 2020
@nmeum
Copy link
Member

nmeum commented Jan 31, 2020

@nmeum Does this fix the issues with linktime re-ordering?

Yes, it does seem to fix it, did a few tests locally. I also just pushed your two commits on the #13176 link time reordering branch. Since I can just rebase my branch against master as soon as this PR is merged. If you want, you can wait to see if it passes on the CI.

nmeum
nmeum approved these changes Jan 31, 2020
View reviewed changes
Copy link
Member

@nmeum nmeum left a comment

Choose a reason for hiding this comment

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

Tested with link time reordering (see #13176), seems to solve the problem at hand.

haukepetersen
haukepetersen approved these changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Change makes a lot of sense! Tested with native and arduino-due, everything still behaves as expected.

ACK.

@kaspar030 kaspar030 merged commit a45abf2 into RIOT-OS:master Jan 31, 2020
@kaspar030
Copy link
Contributor Author

Thanks @nmeum @haukepetersen!

@kaspar030 kaspar030 deleted the introduce_core_init branch January 31, 2020 14:20
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haukepetersen haukepetersen haukepetersen approved these changes

@nmeum nmeum nmeum approved these changes

@miri64 miri64 Awaiting requested review from miri64

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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
4 participants
@kaspar030 @nmeum @haukepetersen @leandrolanzieri