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.

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

toolchain: Add toolchain compiler warning macro indirection #84944

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thughes
Copy link
Contributor

@thughes thughes commented Jan 31, 2025

In order to support new toolchains that are not compatible with gcc/clang (e.g., IAR), we need to add a level of indirection for the name of warnings.

Depends on #84063

@thughes thughes marked this pull request as ready for review January 31, 2025 00:21
@zephyrbot zephyrbot added the area: Toolchains Toolchains label Jan 31, 2025
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Thanks for opening this.

I think it's very good that we have a look at this and clean up / generalize the usage so that it becomes cleaner and easier to improve toolchain support.

I looked up some of the warnings to see where they are actually being used in code.
Some warnings are disabled in production code, such as nonnull for vector table relocation.
Other warnings are disabled for test-purposes only.
I marked some of then in comments.

I think it's important that we keep the test-only warning disabling separated from the production code, perhaps by creating a dedicated include header which can extend the normal toolchain header(s).

This will achieve two purposes

  • Make it clear which warnings are disabled for test purposes (more freedom), and which are disabled in production code (should be better reasoned)
  • Minimize the risk of misuse warning disabling macros, by only having those already used in production code generally available. Rests are only available for test code.

include/zephyr/toolchain/gcc.h Show resolved Hide resolved
include/zephyr/toolchain/gcc.h Outdated Show resolved Hide resolved
include/zephyr/toolchain/gcc.h Outdated Show resolved Hide resolved
include/zephyr/toolchain/gcc.h Show resolved Hide resolved
@thughes thughes force-pushed the add-toolchain-compiler-warning-macro-indirection branch 2 times, most recently from 98b28c6 to 66b1e0c Compare February 12, 2025 22:33
@thughes thughes force-pushed the add-toolchain-compiler-warning-macro-indirection branch from 66b1e0c to 32f1188 Compare February 15, 2025 00:14
@zephyrbot zephyrbot added the area: Testsuite Testsuite label Feb 15, 2025
@thughes
Copy link
Contributor Author

thughes commented Feb 15, 2025

See what you think of this version. I moved the test-specific warnings into a header that I think is only accessible to tests: subsys/testsuite/include/zephyr/test_toolchain.h. It's kind of annoying that this file can't just be named subsys/testsuite/include/zephyr/toolchain.h, but it looks like that would conflict with include/zephyr/toolchain.h. Open to other suggestions if you have any.

We could potentially include test_toolchain.h in ztest.h so people wouldn't have to manually include test_toolchain.h for their tests. Not sure if it's better to force them to explicitly acknowledge they're using it or not.

Here's the PR that shows what it looks like when you use it: #84065

@thughes thughes requested a review from tejlmand February 15, 2025 00:23
In order to support new toolchains that are not compatible with
gcc/clang (e.g., IAR), we need to add a level of indirection for
the name of warnings.

Signed-off-by: Tom Hughes <[email protected]>
@thughes thughes force-pushed the add-toolchain-compiler-warning-macro-indirection branch from 32f1188 to 8be90f6 Compare February 15, 2025 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants