Skip to content

tree-wide: fix compilation with GCC 15.1 and picolibc 1.8.10#21443

Merged
maribu merged 6 commits intoRIOT-OS:masterfrom
maribu:tree-wide/gcc-15-1-fixes
Apr 28, 2025
Merged

tree-wide: fix compilation with GCC 15.1 and picolibc 1.8.10#21443
maribu merged 6 commits intoRIOT-OS:masterfrom
maribu:tree-wide/gcc-15-1-fixes

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Apr 26, 2025

Contribution description

This contains fixes to get RIOT building with a recent toolchain, namely:

  • Relaxing warnings for external packages. (Those issues need to be fixed upstream, rather than causing issues updating them by needlessly large downstream patches.)
  • Adding the __attribute__((nonstring)) to char arrays that do not need a terminating zero byte via the NONSTRING macro, which is defined to be an empty string for older GCC or clang
  • Updating the picolibc syscall integration to test for both the old and the new spelling of a macro used as falg
  • Fixing tests/drivers/hm330x to not use a vendored in implementation

Testing procedure

Other than tests/drivers/hm330x, this should not cause changes in the generated binaries. But compiling with GCC 15.1.0 and with picolibc 1.8.10 should now work

Issues/PRs references

None

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 26, 2025
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: build system Area: Build system Area: pkg Area: External package ports Area: sys Area: System labels Apr 26, 2025
@riot-ci
Copy link
Copy Markdown

riot-ci commented Apr 26, 2025 •
edited
Loading

Murdock results

✔️ PASSED

0be58f6 sys/picolibc_syscalls_default: fix linking with 1.8.10+

Success Failures Total Runtime
10320 0 10320 12m:34s

Artifacts

crasbe
crasbe reviewed Apr 26, 2025
View reviewed changes
crasbe
crasbe reviewed Apr 26, 2025
View reviewed changes
@crasbe crasbe added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Apr 26, 2025
crasbe
crasbe reviewed Apr 27, 2025
View reviewed changes
@crasbe
Copy link
Copy Markdown
Contributor

crasbe commented Apr 27, 2025 •
edited
Loading

Unfortunately I can't test this locally, but the changes look reasonable.

Do you have any test traces of the HM330x? Just to see that the vendored-in function worked the same as the one in the standard library?
I don't have one of these sensors for testing unfortunately.

Edit: Also, you can squash :D

@maribu maribu force-pushed the tree-wide/gcc-15-1-fixes branch 2 times, most recently from 13d95c3 to b4a34e5 Compare April 27, 2025 16:23
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Apr 27, 2025

Do you have any test traces of the HM330x? Just to see that the vendored-in function worked the same as the one in the standard library?

Sadly not, I do not have the hardware. But I realized that the reason for me appearing in the copyright is that the function was copied from a test that I did author before this function was moved to fmt_table. While over time the print_pattern() has been split out to also print table lines, the behavior of that function has not changed when moved to fmt_table. So we can be sure this is fine :)

@maribu maribu enabled auto-merge April 27, 2025 16:27
@maribu maribu added this pull request to the merge queue Apr 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 27, 2025
maribu
maribu commented Apr 27, 2025
View reviewed changes
maribu added 4 commits April 27, 2025 22:49
@maribu
tests/drivers/hm330x: use fmt_table
85233b3 
This replaces the vendored in `print_col_u32_dec()` with the one provided
by the `fmt_table` module.
@maribu
core/lib/include/compiler_hints.h: fix style
914429c 
This adds indent to make reading preprocessor conditionals easier, as
is recommended by our coding convention.
@maribu
core/compiler_hints: Add NONSTRING attribute
0b01fa0 
This adds the `NONSTRING` attribute that is defined as either
`__attribute__((nonstring))` or as empty, depending on whether the
toolchain understands the `nonstring` attribute.

This allows declaring char arrays as not being a zero-terminated
c-string without cluttering the code with preprocessor conditinational
to ensure backward compatibility with compilers that do not support
`-Wunterminated-string-initialization` yet.
@maribu
tree-wide: add the NONSTRING attribute where needed
1b70a59 
This declares all char arrays that intentionally are lacking the
terminated zero byte as `NONSTRING`.
@maribu maribu force-pushed the tree-wide/gcc-15-1-fixes branch from b4a34e5 to 9b7a50b Compare April 27, 2025 20:49
@github-actions github-actions bot added the Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … label Apr 27, 2025
crasbe
crasbe reviewed Apr 27, 2025
View reviewed changes
@maribu maribu force-pushed the tree-wide/gcc-15-1-fixes branch from 9b7a50b to d3bf379 Compare April 27, 2025 20:55
@github-actions github-actions bot removed the Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … label Apr 27, 2025
maribu and others added 2 commits April 27, 2025 22:59
@maribu
pkg: Disable more warnings
59b1774 
This adds `-Wno-unterminated-string-initialization` to the `CFLAGS` if
(and only if) the compiler supports this.

This also adds `-Wno-maybe-uninitialized`. This warning has been
supported for quite some time, but the diagnostics triggers more often
with newer GCC releases.
@maribu @crasbe
sys/picolibc_syscalls_default: fix linking with 1.8.10+
0be58f6 
This adapts to an API change for providing stdin/stdout/stderr: The
macro to test whether globals are to be used is now prefixed with two
underscores.

Co-authored-by: crasbe <[email protected]>
@maribu maribu force-pushed the tree-wide/gcc-15-1-fixes branch from 1470773 to 0be58f6 Compare April 27, 2025 20:59
maribu
maribu commented Apr 27, 2025
View reviewed changes
pkg/pkg.mk
CFLAGS += -Wno-documentation
endif

include $(RIOTBASE)/makefiles/utils/strings.mk
Copy link
Copy Markdown
Member Author

@maribu maribu Apr 27, 2025

Choose a reason for hiding this comment

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

Interesting: When run with make -j, $(RIOTMAKE) (shorthand for $(RIOTBASE)/makefiles) is not defined for building the tools such as edbg. When build with make it is defined.

These kinds of race conditions really are no fun.

Copy link
Copy Markdown
Contributor

@crasbe crasbe Apr 27, 2025

Choose a reason for hiding this comment

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

I didn't look into the CI trace, but often packages are not fully defined and some assume certain variables are set which is not always the case. For example some packages don't build when you're in the folder of the package etc.

pkg/pkg.mk could use some love and the packages as well.

@maribu maribu added this pull request to the merge queue Apr 28, 2025
Merged via the queue into RIOT-OS:master with commit 385f06e Apr 28, 2025
27 checks passed
@maribu maribu deleted the tree-wide/gcc-15-1-fixes branch April 28, 2025 09:13
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Apr 28, 2025

Thanks a lot :)

kfessel
kfessel reviewed Apr 28, 2025
View reviewed changes
tests/sys/psa_crypto/main.c
Comment on lines +77 to 80
NONSTRING
const uint8_t in1[1] = "a";
NONSTRING
const uint8_t in2[1] = "b";
Copy link
Copy Markdown
Contributor

@kfessel kfessel Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

looks ( first glace) like these should be

in1[1] = 'a';
in2[1] = 'b';

Copy link
Copy Markdown
Member Author

@maribu maribu Apr 28, 2025

Choose a reason for hiding this comment

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

No, that is invalid syntax.

You could do

const uint8_t in1[1] = { 'a' };

instead. But that would be the same result as before.

@Teufelchen1 Teufelchen1 added this to the Release 2025.07 milestone Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfessel kfessel kfessel left review comments

@crasbe crasbe crasbe approved these changes

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

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

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

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

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

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: pkg Area: External package ports Area: sys Area: 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2025.07

Development

Successfully merging this pull request may close these issues.

5 participants

@maribu @riot-ci @crasbe @kfessel @Teufelchen1