Skip to content

treewide: compile fixes for native#14663

Merged
maribu merged 6 commits intoRIOT-OS:masterfrom
benpicco:native-fixes
Jul 31, 2020
Merged

treewide: compile fixes for native#14663
maribu merged 6 commits intoRIOT-OS:masterfrom
benpicco:native-fixes

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Jul 30, 2020 •
edited
Loading

Contribution description

char is unsigned on ARM but signed on x86.
This lead to a few compile errors when adding the periph_gpio feature to native, because now code that was previously never build for x86 got hit by this difference.
The host compiler is also usually newer than the embedded one, which usually means it is stricter.

This makes offending chars explicitly unsigned and works around some pedantry.

Testing procedure

No logic changes. In fact, the binaries should stay the same.

Issues/PRs references

split off & needed for #12451
fixes #14667

@benpicco benpicco added 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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Jul 30, 2020
@benpicco benpicco changed the title Native fixes treewide: compile fixes for native Jul 30, 2020
@benpicco benpicco mentioned this pull request Jul 30, 2020
Merged
maribu
maribu reviewed Jul 31, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

See inline comments

@maribu
Copy link
Copy Markdown
Member

maribu commented Jul 31, 2020

Feel free to squash

@maribu maribu mentioned this pull request Jul 31, 2020
Closed
maribu
maribu reviewed Jul 31, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

See inline. You can directly squash in the style nitpick. Regarding the -Wno-pedantic: Does this only affect code out of our control?

@maribu
Copy link
Copy Markdown
Member

maribu commented Jul 31, 2020

Is it only me, or are the inline comments gone?

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jul 31, 2020

I don't see it

@maribu
Copy link
Copy Markdown
Member

maribu commented Jul 31, 2020

OK, the comments was a another (void*) to (void *) that I initially overlooked, but you already fixed it. And the -Wno-pedantic indeed does only apply to the package and we cannot fix code outside of RIOTs tree.

So, both comments are addressed already.

maribu
maribu approved these changes Jul 31, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. This PR does three things:

  • Use a different type name (uint8_t instead of char), which refers to the same type for all previously supported targets
  • Use hex instead of binary representation (0x01 instead of 0xb00000001) of the same numbers
  • Store a function pointer into an uint32_t instead of an void *

The first two changes do obviously not change anything in the generated binaries. The third change resulted in identical binaries being build of tests/pkg_semtech-loramac for the nucleo-f767zi according to elf_diff. Thus, no regressions are expected.

@maribu maribu merged commit f48147b into RIOT-OS:master Jul 31, 2020
@benpicco benpicco deleted the native-fixes branch July 31, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees

No one assigned

Labels

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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Undefined behavior in pkg/semtech-loramac/contrib/

2 participants

@benpicco @maribu