sys/byteorder: clean up implementation #20313
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benpicco
added
the
CI: ready for build
mguetschow
reviewed
Jan 30, 2024
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
43f844d
to
4a7097a
Compare
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
0384d19
to
83a0c79
Compare
github-actions
bot
added
Area: tests
|
This has revealed some bugs in |
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
83a0c79
to
1e8337e
Compare
Teufelchen1
requested changes
Jan 31, 2024
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
2 times, most recently
from
8fdc667
to
16ebc02
Compare
The script to fix the vendor header files has been renamed to `fix_headers.sh` and now does two things: 1. Strip bogus type qualifiers in front of padding (as before) 2. Strip bogus `LITTLE_ENDIAN` defines.
Ran the `fix_headers.sh` to fix the vendor header files and removed the no longer needed work around for them.
The constants BIG_ENDIAN etc. were not consistently defined. This bug was not caught by the unit tests, as the preprocessor would treat undefined macros as being `0` in numeric comparisons, hence the following test did select the correct implementation: ```C #if BYTE_ORDER == LITTLE_ENDIAN /* correct implementation for all currently supported boards */ #endif ``` This adds now an explicit test to the unit tests to ensure that the magic numbers are consistently the correct values. Hence, this bug should no longer be able to sneak in. Co-authored-by: Teufelchen <9516484+Teufelchen1@users.noreply.github.com>
There is no need to use `__uint16_t` instead of `uint16_t` etc., so let's just go with `uint16_t` and have AVR GCC happy.
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
16ebc02
to
2328a32
Compare
github-actions
bot
added
Platform: ARM
Older versions of newlib already provide the magic endian numbers via `machine/endian.h`, which may be indirectly included. This changes the header to only provide the macros if the are not provided otherwise. For sanity, it checks if the values are indeed the expected magic numbers, even if provided from other sources.
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
2328a32
to
ad67d24
Compare
|
@benpicco Could you take a look at the first two commits of this PR? I dropped the work around for the |
Move the <endian.h> header from `sys/libc/include` to `sys/include`.
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
a307f15
to
944ce26
Compare
|
OK, this has been quite a journey, but everything is green now. I reorganized the commits into a logically sensible order. Everything should be ready for another round of review. |
Teufelchen1
approved these changes
Feb 1, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This changes the implementation to be solely build upon `endian.h` and `unaligned.h`. This turns `byteorder.h` basically in syntactic sugar on top of the `<endian.h>` API, reducing the complexity of the implementation and, hence, the maintenance effort. Note that yields a small ROM reduction as well *yeah!* ``` make BOARD=nrf52840dk RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 -C tests/unittests ``` Yields before this commit: ``` text data bss dec hex filename 417788 2200 28640 448628 6d874 /data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf ``` And with this commit: ``` text data bss dec hex filename 417756 2200 28640 448596 6d854 /data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf ```
maribu
force-pushed
the
sys/byteorder/cleanup-implementation
branch
from
944ce26
to
ebfaa36
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
This time it was just a CI glitch... |
|
Woohooo! 🎉 Thanks for the review! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This changes the implementation to be solely build upon
endian.handunaligned.h.This turns
byteorder.hbasically in syntactic sugar on top of the<endian.h>API, reducing the complexity of the implementation and, hence, the maintenance effort.Note that yields a small ROM reduction as well yeah!
Yields before this commit:
And with this commit:
Update
This uncovered some bugs in
<endian.h>that were not detected by the unit tests, as the unit test did not cover the magic constants but only that the right implementation was chosen. Due to the preprocessor handling undifined macros as0in#ifdirectives, the correct implementation on little endian system was chosen anyway.In addition to fixing the bug, the unit test was extended, so that the same bug would be detected now.
Testing procedure
The unit tests in core cover
sys/byteorder(historically,byteorder.hwas once incore):make BOARD=nrf52840dk BUILD_IN_DOCKER=1 -C tests/unittests tests-core flash test -jtl;dr
Issues/PRs references
None