arch: riscv: Fix warning when C++ is enabled #88734

thughes · 2025-04-16T23:55:13Z

When compiling with C++ enabled (CONFIG_CPP), add an unused member to
prevent an empty struct; this makes the struct size the same for both C
and C++.

Fixes the following warnings:

In file included from include/zephyr/drivers/gpio.h:22:
In file included from include/zephyr/tracing/tracing.h:9:
In file included from include/zephyr/kernel.h:17:
In file included from include/zephyr/kernel_includes.h:32:
In file included from include/zephyr/kernel_structs.h:29:
In file included from include/zephyr/arch/structs.h:29:
include/zephyr/arch/riscv/structs.h:11:1: error: empty struct has size 0
in C, size 1 in C++ [-Werror,-Wextern-c-compat]
11 | struct _cpu_arch {
| ^

In file included from include/zephyr/drivers/gpio.h:22:
In file included from include/zephyr/tracing/tracing.h:9:
In file included from include/zephyr/kernel.h:17:
In file included from include/zephyr/kernel_includes.h:36:
In file included from include/zephyr/arch/cpu.h:25:
In file included from include/zephyr/arch/riscv/arch.h:18:
include/zephyr/arch/riscv/thread.h:68:1: error: empty struct has size 0
in C, size 1 in C++ [-Werror,-Wextern-c-compat]
68 | struct _thread_arch {
| ^

When compiling with C++ enabled (CONFIG_CPP), add an unused member to prevent an empty struct; this makes the struct size the same for both C and C++. Fixes the following warnings: In file included from include/zephyr/drivers/gpio.h:22: In file included from include/zephyr/tracing/tracing.h:9: In file included from include/zephyr/kernel.h:17: In file included from include/zephyr/kernel_includes.h:32: In file included from include/zephyr/kernel_structs.h:29: In file included from include/zephyr/arch/structs.h:29: include/zephyr/arch/riscv/structs.h:11:1: error: empty struct has size 0 in C, size 1 in C++ [-Werror,-Wextern-c-compat] 11 | struct _cpu_arch { | ^ In file included from include/zephyr/drivers/gpio.h:22: In file included from include/zephyr/tracing/tracing.h:9: In file included from include/zephyr/kernel.h:17: In file included from include/zephyr/kernel_includes.h:36: In file included from include/zephyr/arch/cpu.h:25: In file included from include/zephyr/arch/riscv/arch.h:18: include/zephyr/arch/riscv/thread.h:68:1: error: empty struct has size 0 in C, size 1 in C++ [-Werror,-Wextern-c-compat] 68 | struct _thread_arch { | ^ Signed-off-by: Tom Hughes <[email protected]>

fkokosinski · 2025-04-17T10:32:54Z

Hi, thanks for this PR! Unfortunately, I can't seem to reproduce this warning using the following command:

~ % west build -p -b qemu_riscv32 samples/philosophers -- -DCONFIG_CPP=y

Could you perhaps create an issue with information on how to reproduce this warning?

thughes · 2025-04-17T21:18:06Z

Could you perhaps create an issue with information on how to reproduce this warning?

There are two things in my setup that are different than the way you're testing:

I'm using LLVM (clang version 20).
I'm building a board that hasn't yet been integrated into upstream Zephyr: https://crrev.com/c/6349236.

It should be reproducible if you use LLVM and make sure that the relevant configs are disabled that make struct _cpu_arch and struct _thread_arch empty.

Here's an example of what the failure looks like in CI when building with LLVM in another part of the code: #87088 (comment)

fkokosinski · 2025-04-18T08:48:58Z

There are two things in my setup that are different than the way you're testing:

I'm using LLVM (clang version 20).

Right. This is a configuration that, as far as I’m aware, is unsupported by Zephyr out of the box.

I'm building a board that hasn't yet been integrated into upstream Zephyr: https://crrev.com/c/6349236.

Unfortunately, I can’t access the link without signing in, but I assume you had to at least add a CMake script in cmake/compiler/clang/riscv.cmake. Would it be possible for you to extract that part, and contribute it as well as a part of this PR? And preferably test it with a platform that has at least one entry in SUPPORTED_EMU_PLATFORMS?

This is something we’re not testing right now in CI, and I’d rather we add this configuration to our suite as well and make it much more easily accessible for maintainers to test and use.

thughes · 2025-04-18T15:53:44Z

This is a configuration that, as far as I’m aware, is unsupported by Zephyr out of the box.

LLVM is officially supported and tested with CI: #85542. For example, see #87088 (comment). However, the CI is currently only building for native-sim.

Unfortunately, I can’t access the link without signing in

https://crrev.com/c/6349236 is publicly accessible. No sign in is required to view it.

I'll see if I can create an upstream change to cover this path in CI. At the very least, it should be possible to demonstrate the size issue with gcc using BUILD_ASSERT(sizeof(struct _thread_arch) >= 1);.

thughes · 2025-05-16T21:51:33Z

@fkokosinski I added another commit that adds a build_only test case. You can see it fail with gcc by commenting out the struct change in include/zephyr/arch/riscv/structs.h, while leaving the BUILD_ASSERT(sizeof(struct _cpu_arch) >= 1); and running it:

./scripts/twister -c -p qemu_riscv32 -s arch.riscv.test_build_cpp

For reference, here's another change in the code where we did the same thing: #87088 (review) and #88778.

ycsin · 2025-05-17T06:27:46Z

include/zephyr/arch/riscv/thread.h

+#if defined(CONFIG_CPP) && !defined(CONFIG_FPU_SHARING) && !defined(CONFIG_USERSPACE) &&           \
+	!defined(CONFIG_PMP_STACK_GUARD)
+	/* Empty struct has size 0 in C, size 1 in C++. Force them to be the same. */
+	uint8_t unused_cpp_size_compatibility;
+#endif


I wonder if we should have something like the following defined somewhere

#ifdef __cplusplus #define CPP_DUMMY_STRUCT_MEMBER int __dummy__ #else #define CPP_DUMMY_STRUCT_MEMBER #endif

and used in structs that can be empty

struct _thread_arch { #ifdef CONFIG_FPU_SHARING struct z_riscv_fp_context saved_fp_context; bool fpu_recently_used; uint8_t exception_depth; #endif #ifdef CONFIG_USERSPACE unsigned long priv_stack_start; unsigned long u_mode_pmpaddr_regs[CONFIG_PMP_SLOTS]; unsigned long u_mode_pmpcfg_regs[CONFIG_PMP_SLOTS / sizeof(unsigned long)]; unsigned int u_mode_pmp_domain_offset; unsigned int u_mode_pmp_end_index; unsigned int u_mode_pmp_update_nr; #endif #ifdef CONFIG_PMP_STACK_GUARD unsigned int m_mode_pmp_end_index; unsigned long m_mode_pmpaddr_regs[PMP_M_MODE_SLOTS]; unsigned long m_mode_pmpcfg_regs[PMP_M_MODE_SLOTS / sizeof(unsigned long)]; #endif CPP_DUMMY_STRUCT_MEMBER; }

so that the code looks cleaner, at the expense of an extra int for every such struct when compiled for cpp, regardless of whether or not they are actually empty.

I like the idea of the dummy struct member, but I think you got the condition backwards? Plus, the dummy member should probably be a char instead.

We could avoid the dummy member this way:

#ifdef __cplusplus #define C_DUMMY_STRUCT_MEMBER #else #define C_DUMMY_STRUCT_MEMBER char __dummy__ #endif struct foobar { #ifdef CONFIG_FOO int foo; #endif #ifdef CONFIG_BAR int bar; #endif #if !(CONFIG_FOO || CONFIG_BAR) C_DUMMY_STRUCT_MEMBER; #endif };

How about I implement something like this in a followup PR?

but I think you got the condition backwards?

Not really, I think we are talking about different idea - I'm proposing an alternative where we always have a dummy variable in every struct that might be empty when CONFIG_CPP=y

#ifdef __cplusplus #define CPP_DUMMY_STRUCT_MEMBER char __dummy__ <- make sure that a struct can never be empty when CONFIG_CPP=y #else #define CPP_DUMMY_STRUCT_MEMBER #endif

and whenever we have a struct that can be empty due to Kconfigs, simply append CPP_DUMMY_STRUCT_MEMBER at the end to avoid more preprocessor macros like these:

#if !(CONFIG_FOO || CONFIG_BAR) CPP_DUMMY_STRUCT_MEMBER; #endif

fkokosinski · 2025-05-19T11:12:59Z

@fkokosinski I added another commit that adds a build_only test case. You can see it fail with gcc by commenting out the struct change in include/zephyr/arch/riscv/structs.h, while leaving the BUILD_ASSERT(sizeof(struct _cpu_arch) >= 1); and running it:
./scripts/twister -c -p qemu_riscv32 -s arch.riscv.test_build_cpp
For reference, here's another change in the code where we did the same thing: #87088 (review) and #88778.

Thanks!

ycsin

Probably a RFC to streamline how we would deal with possibly empty struct in the future, but +1 to this PR

include/zephyr/arch/riscv/structs.h

tests/arch/riscv/test_build_cpp/CMakeLists.txt

Using clang to build for RISC-V with these configs: CONFIG_CPP=y CONFIG_USERSPACE=n CONFIG_SMP=n CONFIG_FPU_SHARING=n results in the following warnings: include/zephyr/arch/riscv/structs.h:11:1: error: empty struct has size 0 in C, size 1 in C++ [-Werror,-Wextern-c-compat] 11 | struct _cpu_arch { | ^ include/zephyr/arch/riscv/thread.h:68:1: error: empty struct has size 0 in C, size 1 in C++ [-Werror,-Wextern-c-compat] 68 | struct _thread_arch { | ^ This commit adds a test with those configs to verify compilation succeeds. Although gcc doesn't emit these warnings, we can test with gcc as well with a BUILD_ASSERT in include/zephyr/arch/riscv/thread.h and include/zephyr/arch/riscv/structs.h. Signed-off-by: Tom Hughes <[email protected]>

thughes · 2025-06-03T05:39:39Z

tests/lib/cpp/cxx/testcase.yaml

      - CONFIG_STD_CPP2B=y
+  # Verify struct _cpu_arch struct _thread_arch are compatible with C++ when
+  # specific configs are enabled.
+  cpp.main.empty_struct_size:


The test is arch agnostic, but the configs required to trigger the empty struct are architecture dependent. We could put an arch_allow: riscv on the test to make that clear. What do you think?

The issue of empty struct can happen in all archs under different configurations, but keeping the test updated to track all possible scenarios seems tedious, I wonder if we should just drop the test and resort to PR review / static code analysis (can sonarqube detect possibly empty C structs?)

keeping the test updated to track all possible scenarios seems tedious

I agree, but I don't think we need to track all possible scenarios. We can add cases as we find ones that people care about. I don't think the empty struct issue is going to be caught in PR review; it's too easy to miss.

sonarqubecloud · 2025-06-03T06:26:20Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2025-08-09T00:39:14Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

thughes · 2025-10-02T22:50:54Z

Please remove stale label and re-open the PR.

thughes · 2025-10-02T23:54:24Z

I can't seem to re-open this, so I created #96943.

thughes · 2025-10-03T21:08:56Z

Test moved from this PR to #96944

thughes marked this pull request as ready for review April 17, 2025 02:54

github-actions bot added area: Architectures area: RISCV RISCV Architecture (32-bit & 64-bit) labels Apr 17, 2025

github-actions bot requested review from carlocaione, dcpleung, edersondisouza, fkokosinski, katsuster, kgugala, mgielda, nashif, npitre, tgorochowik, VynDragon and ycsin April 17, 2025 02:55

github-actions bot assigned dcpleung and nashif Apr 17, 2025

ycsin reviewed May 17, 2025

View reviewed changes

ycsin reviewed May 20, 2025

View reviewed changes

include/zephyr/arch/riscv/structs.h Show resolved Hide resolved

tests/arch/riscv/test_build_cpp/CMakeLists.txt Outdated Show resolved Hide resolved

thughes force-pushed the push-nluyuksvrull branch from 65ccd3a to ff2cdb8 Compare June 3, 2025 05:32

github-actions bot added the area: C++ label Jun 3, 2025

github-actions bot requested a review from alexanderwachter June 3, 2025 05:33

github-actions bot requested a review from stephanosio June 3, 2025 05:33

thughes commented Jun 3, 2025

View reviewed changes

thughes requested a review from ycsin June 3, 2025 05:39

github-actions bot added the Stale label Aug 9, 2025

github-actions bot closed this Aug 24, 2025

thughes mentioned this pull request Oct 2, 2025

arch: riscv: Fix warning when C++ is enabled #96943

Open

arch: riscv: Fix warning when C++ is enabled #88734

arch: riscv: Fix warning when C++ is enabled #88734

Uh oh!

Conversation

thughes commented Apr 16, 2025

Uh oh!

fkokosinski commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thughes commented Apr 17, 2025

Uh oh!

fkokosinski commented Apr 18, 2025

Uh oh!

thughes commented Apr 18, 2025

Uh oh!

thughes commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycsin May 17, 2025

Choose a reason for hiding this comment

Uh oh!

fkokosinski May 19, 2025

Choose a reason for hiding this comment

Uh oh!

thughes May 19, 2025

Choose a reason for hiding this comment

Uh oh!

ycsin May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkokosinski commented May 19, 2025

Uh oh!

ycsin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thughes Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

thughes Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

ycsin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

thughes Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 3, 2025

Quality Gate passed

Uh oh!

github-actions bot commented Aug 9, 2025

Uh oh!

thughes commented Oct 2, 2025

Uh oh!

thughes commented Oct 2, 2025

Uh oh!

thughes commented Oct 3, 2025

Uh oh!

Uh oh!

fkokosinski commented Apr 17, 2025 •

edited

Loading

thughes commented May 16, 2025 •

edited

Loading

ycsin May 20, 2025 •

edited

Loading