cmake: use the warnings_as_errors flag for cpp files #78419

fabiobaltieri · 2024-09-13T20:01:14Z

C++ file compilation is actually missing the warning-as-error handling, causing warnings in build files to be unnoticed in CI. Add a flag to handle them as well.

Fixes #78348

Pushed on https://github.com/zephyrproject-rtos/zephyr-testing/tree/vfabio-testing-branch as well

nashif · 2024-09-13T20:49:50Z

and now to all those build issues we missed previous and exposed through this change....

yperess · 2024-09-14T19:33:57Z

@fabiobaltieri would you like me to make these fixes in the tree?

fabiobaltieri · 2024-09-14T20:29:33Z

@fabiobaltieri would you like me to make these fixes in the tree?

That'd be cool if you have time, I'm out for the next couple of days.

nordicjm · 2024-09-16T07:56:25Z

Just noticed this is a duplicate of #69226

fabiobaltieri · 2024-09-18T14:34:47Z

Thanks @nordicjm, borrowed some commits from there and added few more, still not done though but I could use a CI run, moving back to draft.

cfriedt

cpp changes look fine

fabiobaltieri · 2024-09-19T13:57:05Z

include/zephyr/sys/device_mmio.h

@@ -63,13 +63,13 @@ struct z_device_mmio_rom {

 #define Z_DEVICE_MMIO_ROM_INITIALIZER(node_id) \
 	{ \
-		.phys_addr = DT_REG_ADDR(node_id), \
+		.phys_addr = UINT32_C(DT_REG_ADDR(node_id)), \


tempted to have this as part of DT_REG_ADDR (and potentially DT_REG_SIZE too) macro itself but that would be a substantial change, definitely worth its own PR, I propose to patch this one up for now. Somewhat surprised CI does not hit more cases of this warning though.

This looks really odd to me offhand, is the ADDR a 32 bit unsigned or not? If it is, then the DT macro should ensure that's the type being provided. If its not, then .phys_addr should have its type corrected. This looks a bit like a quick fix at the wrong place.

it's a quick fix, that's pretty much what I meant in the comment above

Please create an issue for this then, if its meant to be revisited

There already exist an issue:
#53505
I gave it a shot, but gave up:
#70761

@jeppenodgaard what was wrong with that? CI errors? I was considering giving it a go as well

@fabiobaltieri see this closing comment:
#70761 (comment)

teburd

Some answers I think are needed for some of the more suspicious changes to appease the compiler, or perhaps some inline comments for the really odd ones (void)foo; looks like a no-op

include/zephyr/drivers/mspi.h

teburd · 2024-09-19T15:02:54Z

include/zephyr/sys/device_mmio.h

@@ -63,13 +63,13 @@ struct z_device_mmio_rom {

 #define Z_DEVICE_MMIO_ROM_INITIALIZER(node_id) \
 	{ \
-		.phys_addr = DT_REG_ADDR(node_id), \
+		.phys_addr = UINT32_C(DT_REG_ADDR(node_id)), \


This looks really odd to me offhand, is the ADDR a 32 bit unsigned or not? If it is, then the DT macro should ensure that's the type being provided. If its not, then .phys_addr should have its type corrected. This looks a bit like a quick fix at the wrong place.

teburd · 2024-09-19T15:03:48Z

tests/drivers/i2c/i2c_emul/src/emulated_target.cpp

@@ -42,7 +42,7 @@ struct i2c_target_callbacks emulated_callbacks[FORWARD_COUNT] = {
 	DT_FOREACH_PROP_ELEM(CONTROLLER_LABEL, forwards, DEFINE_EMULATED_CALLBACK)};

 #define DEFINE_EMULATED_TARGET_CONFIG(node_id, prop, n)                                            \
-	[n] = {                                                                                    \
+	{                                                                                          \


This looks like its doing something completely different than it was before? Confused by this change, why is "[n] = " no longer needed, why was it needed before?

no idea why it was introduced originally, I guess to do something with that n variable? regardless, the compiler does not like it, the alternative would be to shut off that warning, happy with either. @yperess?

tests/drivers/i2c/i2c_emul/src/test_forwarding_pio.cpp

tests/lib/cbprintf_package/src/main.c

tests/lib/cpp/cxx/src/main.cpp

Fix a type mismatch compiler warning in the c++ tests. Signed-off-by: Yuval Peress <peress@google.com> Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

teburd · 2024-09-19T16:07:12Z

Dropped the XS tag as its a bit more than one line, hopefully no one is offended by this.

Add a dummy byte to struct mspi_timing_cfg, for C++ compatibility, same as what's done in include/zephyr/arch/structs.h, include/zephyr/arch/arm/structs.h, include/zephyr/kernel/thread.h and others. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Fix the compiler error: comparison of integer expressions of different signedness: ‘const int’ and ‘unsigned int’ Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

On some architecture (qemu_x86) this tests generate an unused reference warning for the PM device structure, as nothing in the structure macro definition is referencing itself. Reference the struct in the device define, keep the compiler happy. Also change one of the device priority to dodge a '__device_dts_ord_8' causes a section type conflict with '__device_dts_ord_9' error with llvm. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Drop the array dasignators as they generate a warning: array designators are a C99 extension warning when compiling with clang. Not too sure if this is an actual problem or if the warning should be disabled instead but since it's just a test it should be fine to just drop them. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

Add an explicit unsigned literal suffix (U) for addresses obtained from devicetree, this is to work around a specific warning: zephyrproject/zephyr/include/zephyr/arch/arm/cortex_a_r/timer.h:40:2: warning: this decimal constant is unsigned only in ISO C90 When compling west build -p -b qemu_cortex_a9 -T tests/lib/cpp/cxx/cpp.main.cpp98 Argueably all devicetree addresses and sizes should be unsigned but this is enough to make CI pass, may look into changing the device.h macro as a followup. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

C++ file compilation is actually missing the warning-as-error handling, causing warnings in build files to be unnoticed in CI. Add a flag to handle them as well. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri · 2024-09-20T09:52:22Z

Ok this is interesting now:

$ export ZEPHYR_TOOLCHAIN_VARIANT=llvm
$ west build -p -b native_sim -T tests/lib/cpp/cxx/cpp.main.minimal
...
zephyr/tests/lib/cpp/cxx/src/main.cpp:154:1: error: '__device_dts_ord_8' causes a section type conflict with '__device_dts_ord_9'

Only on LLVM, seems to not like the two devices one with PM data one without at the same priority, this is the expanded definition if anyone wants to have a go at figuring what is going on

static __attribute__((__aligned__(__alignof(struct device_state)))) struct device_state __devstate_dts_ord_9 __attribute__((__section__(".z_devstate"))); static_assert(sizeof("\"dev1\"") <= 48U, "" "DEVICE_NAME_GET(\"dev1\")" " too long"); static const struct device_dt_nodelabels __dev_dt_nodelabels_dts_ord_9 = { .num_nodelabels = 1, .nodelabels = { "test_dev1_dfr", }, }; static const struct device_dt_metadata __dev_dt_meta_dts_ord_9 = { .nl = &__dev_dt_nodelabels_dts_ord_9, };; const __attribute__((__aligned__(__alignof(struct device)))) struct device __device_dts_ord_9 __attribute__((section("." "_device" "." "static" "." "3_33_"))) __attribute__((__used__)) = { .name = "dev1", .config = (__null), .api = (__null), .state = (&__devstate_dts_ord_9), .data = (__null), .pm_base = (__null), .dt_meta = &__dev_dt_meta_dts_ord_9, }; static const __attribute__((__aligned__(__alignof(struct init_entry)))) struct init_entry __attribute__((__used__)) __attribute__((__section__(".z_deferred_init"))) __init___device_dts_ord_9 = { .init_fn = {.dev = (__null)}, .dev = &__device_dts_ord_9, };;

static int fake_pm_action(const struct device *dev,
  enum pm_device_action pm_action) { return -1; }
; static struct pm_device __pm_device_dts_ord_8 = { .base = { .flags = (((0 << PM_DEVICE_FLAG_WS_CAPABLE) | (0 << PM_DEVICE_FLAG_RUNTIME_AUTO) | (0 << PM_DEVICE_FLAG_PD)) | (0 ? (1UL << (PM_DEVICE_FLAG_ISR_SAFE)) : 0)), .state = PM_DEVICE_STATE_ACTIVE, .action_cb = fake_pm_action, .domain = __null, }, .lock = { .wait_q = { { {(&(&(__pm_device_dts_ord_8.lock).wait_q)->waitq)}, {(&(&(__pm_device_dts_ord_8.lock).wait_q)->waitq)} } }, .count = (1), .limit = (1), }, .event = { .wait_q = { { {(&(&__pm_device_dts_ord_8.event.wait_q)->waitq)}, {(&(&__pm_device_dts_ord_8.event.wait_q)->waitq)} } }, .events = 0 }, };

static __attribute__((__aligned__(__alignof(struct device_state)))) struct device_state __devstate_dts_ord_8 __attribute__((__section__(".z_devstate"))); static_assert(sizeof("\"dev0\"") <= 48U, "" "DEVICE_NAME_GET(\"dev0\")" " too long"); static const struct device_dt_nodelabels __dev_dt_nodelabels_dts_ord_8 = { .num_nodelabels = 1, .nodelabels = { "test_dev0_boot", }, }; static const struct device_dt_metadata __dev_dt_meta_dts_ord_8 = { .nl = &__dev_dt_nodelabels_dts_ord_8, };; const __attribute__((__aligned__(__alignof(struct device)))) struct device __device_dts_ord_8 __attribute__((section("." "_device" "." "static" "." "3_33_"))) __attribute__((__used__)) = { .name = "dev0", .config = (__null), .api = (__null), .state = (&__devstate_dts_ord_8), .data = (__null), .pm_base = (((struct pm_device_base *)&__pm_device_dts_ord_8)), .dt_meta = &__dev_dt_meta_dts_ord_8, }; static const __attribute__((__aligned__(__alignof(struct init_entry)))) struct init_entry __attribute__((__used__)) __attribute__((__section__( ".z_init_" "POST_KERNEL" "33""_" "00008""_"))) __init___device_dts_ord_8 = { .init_fn = {.dev = (__null)}, .dev = &__device_dts_ord_8, };;

I'll change the priority for now, no doubt it'll come to bite me back down the road anyway.

fabiobaltieri · 2024-09-20T10:08:09Z

Dug a bit more, seems like having that field set moves the device structure from rodata to data, with different prio and no pm:

        0x00000000 - 0x000000fe is .text
...
        0x00000130 - 0x00000180 is .bss
...
        0x00000388 - 0x000003a0 is .rodata
        0x000003a0 - 0x000003bc is ._device.static.3_33_
        0x000003bc - 0x000003c4 is .z_deferred_init
        0x000003c4 - 0x000003e0 is ._device.static.3_34_
        0x000003e0 - 0x000003e8 is .z_init_POST_KERNEL33_00008_
        0x000003e8 - 0x000003ec is .init_array

with pm:

        0x00000000 - 0x00000102 is .text
...
        0x00000134 - 0x00000184 is .bss
...
        0x0000038c - 0x000003a4 is .rodata
        0x000003a4 - 0x000003c0 is ._device.static.3_33_
        0x000003c0 - 0x000003c8 is .z_deferred_init
        0x000003c8 - 0x00000428 is .data
        0x00000428 - 0x00000444 is ._device.static.3_34_
        0x00000444 - 0x0000044c is .z_init_POST_KERNEL33_00008_
        0x0000044c - 0x00000450 is .init_array

no idea why it's doing that, does not look right to me but I'll just add it to the ever growing list of c++ stuff I don't understand.

teburd · 2024-09-20T12:30:35Z

Dug a bit more, seems like having that field set moves the device structure from rodata to data, with different prio and no pm:

        0x00000000 - 0x000000fe is .text
...
        0x00000130 - 0x00000180 is .bss
...
        0x00000388 - 0x000003a0 is .rodata
        0x000003a0 - 0x000003bc is ._device.static.3_33_
        0x000003bc - 0x000003c4 is .z_deferred_init
        0x000003c4 - 0x000003e0 is ._device.static.3_34_
        0x000003e0 - 0x000003e8 is .z_init_POST_KERNEL33_00008_
        0x000003e8 - 0x000003ec is .init_array

with pm:

        0x00000000 - 0x00000102 is .text
...
        0x00000134 - 0x00000184 is .bss
...
        0x0000038c - 0x000003a4 is .rodata
        0x000003a4 - 0x000003c0 is ._device.static.3_33_
        0x000003c0 - 0x000003c8 is .z_deferred_init
        0x000003c8 - 0x00000428 is .data
        0x00000428 - 0x00000444 is ._device.static.3_34_
        0x00000444 - 0x0000044c is .z_init_POST_KERNEL33_00008_
        0x0000044c - 0x00000450 is .init_array

no idea why it's doing that, does not look right to me but I'll just add it to the ever growing list of c++ stuff I don't understand.

Unsolved Mysteries of C++, maybe one of the people who wished to add C++ can help here, I have no idea either.

fabiobaltieri · 2024-09-20T12:50:36Z

Yeah, anyway CI seems to be happy again so at least the workarounds are working around.

teburd

Noted issue exists for the unsigned dt stuff, all other mysterious things look better. Seems good to me.

fabiobaltieri mentioned this pull request Sep 13, 2024

twister missing build warning in cpp files #78348

Closed

zephyrbot added area: Build System size: XS A PR changing only a single line of code labels Sep 13, 2024

zephyrbot requested review from 57300, jeremybettis, nashif, nordicjm and tejlmand September 13, 2024 20:01

zephyrbot assigned tejlmand Sep 13, 2024

kartben previously approved these changes Sep 13, 2024

View reviewed changes

nashif previously approved these changes Sep 13, 2024

View reviewed changes

yperess previously approved these changes Sep 14, 2024

View reviewed changes

tejlmand previously approved these changes Sep 15, 2024

View reviewed changes

nordicjm previously approved these changes Sep 16, 2024

View reviewed changes

pdgendt previously approved these changes Sep 16, 2024

View reviewed changes

fabiobaltieri dismissed stale reviews from pdgendt, nordicjm, tejlmand, yperess, nashif, and kartben via 2471799 September 18, 2024 14:35

fabiobaltieri force-pushed the warning-error-cpp branch from a4bdf0a to 2471799 Compare September 18, 2024 14:35

fabiobaltieri marked this pull request as draft September 18, 2024 14:35

zephyrbot added area: I2C area: C++ labels Sep 18, 2024

cfriedt previously approved these changes Sep 19, 2024

View reviewed changes

zephyrbot added the area: Base OS Base OS Library (lib/os) label Sep 19, 2024

fabiobaltieri commented Sep 19, 2024

View reviewed changes

teburd requested changes Sep 19, 2024

View reviewed changes

teburd reviewed Sep 19, 2024

View reviewed changes

tests/lib/cpp/cxx/src/main.cpp Outdated Show resolved Hide resolved

fabiobaltieri dismissed cfriedt’s stale review via f783f3b September 19, 2024 15:42

fabiobaltieri force-pushed the warning-error-cpp branch 2 times, most recently from f783f3b to 333a11a Compare September 19, 2024 15:58

tests: lib: cbprintf_package: fix compiler warning

b73da5a

Fix a type mismatch compiler warning in the c++ tests. Signed-off-by: Yuval Peress <peress@google.com> Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

teburd removed the size: XS A PR changing only a single line of code label Sep 19, 2024

jeppenodgaard mentioned this pull request Sep 20, 2024

cmake: add compile-warnings-as-errors for c++ #69226

Closed

fabiobaltieri added 6 commits September 20, 2024 09:38

tests: i2c_emul: fix sign comparison warning

0ffeb16

Fix the compiler error: comparison of integer expressions of different signedness: ‘const int’ and ‘unsigned int’ Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

cmake: use the warnings_as_errors flag for cpp files

66a71f3

C++ file compilation is actually missing the warning-as-error handling, causing warnings in build files to be unnoticed in CI. Add a flag to handle them as well. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>

fabiobaltieri force-pushed the warning-error-cpp branch from 333a11a to 66a71f3 Compare September 20, 2024 09:52

teburd approved these changes Sep 20, 2024

View reviewed changes

fabiobaltieri mentioned this pull request Sep 20, 2024

devicetree: make DT_..._REG_ADDR return unsigned #78766

Merged

cfriedt approved these changes Sep 21, 2024

View reviewed changes

tejlmand approved these changes Sep 23, 2024

View reviewed changes

nashif merged commit 56dcafe into zephyrproject-rtos:main Sep 23, 2024
24 checks passed

kartben mentioned this pull request Oct 22, 2024

boards: rakwireless: Use proper vendor prefix as board folder #80160

Merged

fabiobaltieri deleted the warning-error-cpp branch February 28, 2025 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: use the warnings_as_errors flag for cpp files #78419

cmake: use the warnings_as_errors flag for cpp files #78419

fabiobaltieri commented Sep 13, 2024 •

edited

Loading

nashif commented Sep 13, 2024

yperess commented Sep 14, 2024

fabiobaltieri commented Sep 14, 2024

nordicjm commented Sep 16, 2024

fabiobaltieri commented Sep 18, 2024

cfriedt left a comment

fabiobaltieri Sep 19, 2024 •

edited

Loading

teburd Sep 19, 2024

fabiobaltieri Sep 19, 2024

teburd Sep 19, 2024

jeppenodgaard Sep 20, 2024

fabiobaltieri Sep 20, 2024

jeppenodgaard Sep 20, 2024

teburd left a comment

teburd Sep 19, 2024

teburd Sep 19, 2024

fabiobaltieri Sep 19, 2024

teburd commented Sep 19, 2024

fabiobaltieri commented Sep 20, 2024

fabiobaltieri commented Sep 20, 2024

teburd commented Sep 20, 2024

fabiobaltieri commented Sep 20, 2024

teburd left a comment

cmake: use the warnings_as_errors flag for cpp files #78419

cmake: use the warnings_as_errors flag for cpp files #78419

Conversation

fabiobaltieri commented Sep 13, 2024 • edited Loading

nashif commented Sep 13, 2024

yperess commented Sep 14, 2024

fabiobaltieri commented Sep 14, 2024

nordicjm commented Sep 16, 2024

fabiobaltieri commented Sep 18, 2024

cfriedt left a comment

Choose a reason for hiding this comment

fabiobaltieri Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd commented Sep 19, 2024

fabiobaltieri commented Sep 20, 2024

fabiobaltieri commented Sep 20, 2024

teburd commented Sep 20, 2024

fabiobaltieri commented Sep 20, 2024

teburd left a comment

Choose a reason for hiding this comment

fabiobaltieri commented Sep 13, 2024 •

edited

Loading

fabiobaltieri Sep 19, 2024 •

edited

Loading