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

Revert "init: add missing initialization of dev pointer in SYS_INIT (alternative to PR68125) #68118

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Jan 25, 2024

…acro"

This reverts commit 2438dbb.

This commit breaks compilation on the Cadence XCC toolchain we use for Intel Tiger Lake products (this is the oldest platform we support in Zephyr upstream for the audio DSP).

Error:
/work/zephyr/lib/os/p4wq.c:216: error: unknown field ‘dev’ specified in initializer /work/zephyr/lib/os/p4wq.c:216: warning: missing braces around initializer /work/zephyr/lib/os/p4wq.c:216: warning: (near initialization for ‘__init_static_init.’)

This is XCC RG-2017.8-linux (xt-xcc version 12.0.8)

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jan 25, 2024

@barnas-michal @nashif Not sure this is the correct solution, but I'm hunting a series of regressions (#68110) that affect this configuration, so I wanted submit something that unblocks and allows me to continue.

@barnas-michal
Copy link
Collaborator

Is it problem that the dev field is inside the unnamed union, and your compiler couldn't assign a value to it? The dev field is not ifdef'ed with anything, so it should always be in the struct.

@barnas-michal
Copy link
Collaborator

If the above is the case, then the revert is not a solution, since the original fix was due to c++ (zephyr sdk) compiler complain about lack of field initialization. So the fix should be to make the code assign value in a way that will please all compilers, instead of reverting the assignment and making the c++ not compile.

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

-Werror=missing-field-initializers might be a good thing to have, but in fact C guarantees that all uninitialised fields in structure (or array) definitions are set to 0 (don't quote me on this, not a precise statement of course).

@barnas-michal
Copy link
Collaborator

@lyakh : I understand that the static values are initialized to the 0, but even so, I do not think that removing the assignment is a fix here. If someone has to use zephyr in project with strict regulations and has to have this flag enabled, then the compiler will complain about lack of this assignment. Proper fix IMO, would be to make this assignment work with your compiler. If the problem is unnamed union, add some name to it and change assignment, but not remove it.

@nashif
Copy link
Member

nashif commented Jan 25, 2024

@barnas-michal not related to this being the right way to "fix" or not, but we should not be fixing issues of compiler warnings only enabled outside of the project, if this compiler flag is useful we should enable it in zephyr as well and fix issues everywhere and deal with it within the project for all toolchains and support this accordingly.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jan 25, 2024

One alternative that seems to keep our ancient XCC toolchain happy:
#68125

@kv2019i kv2019i force-pushed the 202401-fix-intel-tgl-xcc-build branch from 32280e7 to b1dd098 Compare January 25, 2024 18:59
@kv2019i
Copy link
Collaborator Author

kv2019i commented Jan 25, 2024

This does not seem to be easy to keep all old/new C/C++ compilers happy (with warnings).

I pushed this again to fix the commit, and also did another version of #68125 (the first one didn't pass the ARM Zephyr SDK compilers).

Neither is very pretty, but given we get a hard error on SOF builds (not just a warning), we need some solution. It's not like you can not use SYS_INIT .

@kv2019i kv2019i changed the title Revert "init: add missing initialization of dev pointer in SYS_INIT m… Revert "init: add missing initialization of dev pointer in SYS_INIT (alternative to PR68125) Jan 25, 2024
…acro"

This reverts commit 2438dbb.

This commit breaks compilation on the Cadence XCC toolchain we
use for Intel Tiger Lake products (this is the oldest platform we
support in Zephyr upstream for the audio DSP).

Error:
lib/os/p4wq.c:216: error: unknown field ‘dev’ specified in initializer
lib/os/p4wq.c:216: warning: missing braces around initializer
lib/os/p4wq.c:216: warning: (near initialization for
  ‘__init_static_init.<anonymous>’)

This is XCC RG-2017.8-linux (xt-xcc version 12.0.8)

Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i kv2019i force-pushed the 202401-fix-intel-tgl-xcc-build branch from b1dd098 to a2e3bb8 Compare January 25, 2024 19:11
@marc-hb
Copy link
Collaborator

marc-hb commented Jan 25, 2024

If someone has to use zephyr in project with strict regulations and has to have this flag enabled, then the compiler will complain about lack of this assignment

"Very strict regulations" is very... vague; there are many different variations:
https://thenewstack.io/can-c-be-saved-bjarne-stroustrup-on-ensuring-memory-safety/

-Werror=missing-field-initializers looks superficially good but as usual there's a benefit/ratio trade-off: how many bugs does it catch and what is their typical severity versus how much does it break.

Maybe @ceolin can point to more formal guidelines and approaches.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 25, 2024

This rang a bell so I quickly searched my archives and I found that this revert is actually not just a revert. More like a re-re-revert: 153f38a

cc: @fabiobaltieri

we should not be fixing issues of compiler warnings only enabled outside of the project, if this compiler flag is useful we should enable it in zephyr as well and fix issues everywhere and deal with it within the project for all toolchains and support this accordingly.

Right, otherwise we go back and forth and back and forth,...

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jan 26, 2024

#68125 merged, closing this one.

marc-hb added a commit to marc-hb/zephyr that referenced this pull request Feb 21, 2024
Fixes commit 25173f7 ("pm: device_runtime: Extend with synchronous
runtime PM")

Fixes compilation with gcc 4.2-based toolchain used by SOF for TGL
generation products. As seen in the error message below, that gcc
version requires braces for initialization of anonymous unions:

```
zephyr/soc/xtensa/intel_adsp/common/mem_window.c:62:
   error: unknown field ‘pm_base’ specified in initializer
   warning: missing braces around initializer
   warning: (near initialization for ‘__device_dts_ord_66.<anonymous>’)
```

This is a well-known and recurring issue, see past example(s) in zephyrproject-rtos#68118

As the Zephyr build is deterministic, I could easily verify that this
commit makes zero .obj difference (when using the Zephyr SDK).

Signed-off-by: Marc Herbert <[email protected]>
henrikbrixandersen pushed a commit that referenced this pull request Feb 21, 2024
Fixes commit 25173f7 ("pm: device_runtime: Extend with synchronous
runtime PM")

Fixes compilation with gcc 4.2-based toolchain used by SOF for TGL
generation products. As seen in the error message below, that gcc
version requires braces for initialization of anonymous unions:

```
zephyr/soc/xtensa/intel_adsp/common/mem_window.c:62:
   error: unknown field ‘pm_base’ specified in initializer
   warning: missing braces around initializer
   warning: (near initialization for ‘__device_dts_ord_66.<anonymous>’)
```

This is a well-known and recurring issue, see past example(s) in #68118

As the Zephyr build is deterministic, I could easily verify that this
commit makes zero .obj difference (when using the Zephyr SDK).

Signed-off-by: Marc Herbert <[email protected]>
HesselM pushed a commit to Sendrato/zephyr that referenced this pull request Mar 4, 2024
Fixes commit 25173f7 ("pm: device_runtime: Extend with synchronous
runtime PM")

Fixes compilation with gcc 4.2-based toolchain used by SOF for TGL
generation products. As seen in the error message below, that gcc
version requires braces for initialization of anonymous unions:

```
zephyr/soc/xtensa/intel_adsp/common/mem_window.c:62:
   error: unknown field ‘pm_base’ specified in initializer
   warning: missing braces around initializer
   warning: (near initialization for ‘__device_dts_ord_66.<anonymous>’)
```

This is a well-known and recurring issue, see past example(s) in zephyrproject-rtos#68118

As the Zephyr build is deterministic, I could easily verify that this
commit makes zero .obj difference (when using the Zephyr SDK).

Signed-off-by: Marc Herbert <[email protected]>
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.

7 participants