Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da49c93
arch: riscv: Fix warning when C++ is enabled
thughes Apr 15, 2025
ff2cdb8
tests: riscv: Add test to verify C++ compilation
thughes May 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/zephyr/arch/riscv/structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ struct _cpu_arch {
atomic_ptr_val_t fpu_owner;
uint32_t fpu_state;
#endif
#if defined(CONFIG_CPP) && !defined(CONFIG_USERSPACE) && \
!(defined(CONFIG_SMP) || (CONFIG_MP_MAX_NUM_CPUS > 1)) && !defined(CONFIG_FPU_SHARING)
/* Empty struct has size 0 in C, size 1 in C++. Force them to be the same. */
uint8_t unused_cpp_size_compatibility;
#endif
};

#if defined(CONFIG_CPP)
BUILD_ASSERT(sizeof(struct _cpu_arch) >= 1);
#endif

#endif /* ZEPHYR_INCLUDE_RISCV_STRUCTS_H_ */
9 changes: 9 additions & 0 deletions include/zephyr/arch/riscv/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,17 @@ struct _thread_arch {
unsigned long m_mode_pmpaddr_regs[PMP_M_MODE_SLOTS];
unsigned long m_mode_pmpcfg_regs[PMP_M_MODE_SLOTS / sizeof(unsigned long)];
#endif
#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
Comment on lines +87 to +91
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@ycsin ycsin May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

};

#if defined(CONFIG_CPP)
BUILD_ASSERT(sizeof(struct _thread_arch) >= 1);
#endif

typedef struct _thread_arch _thread_arch_t;

#endif /* _ASMLANGUAGE */
Expand Down
8 changes: 8 additions & 0 deletions tests/lib/cpp/cxx/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,11 @@ tests:
build_only: true
extra_configs:
- 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:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ycsin

Copy link
Member

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

build_only: true
extra_configs:
- CONFIG_USERSPACE=n
- CONFIG_SMP=n
- CONFIG_FPU_SHARING=n