ZRESTRICT conflict between toolchain/common.h and toolchain/gcc.h #62464

marc-hb · 2023-09-08T23:41:33Z

The addition of the #include toolchain/common.h in #61962 just revealed a funny ZRESTRICT conflict between include/zephyr/toolchain/common.h and include/zephyr/toolchain/gcc.h when compiling C++

toolchain: alias restrict to __restrict for GNUC #27245

As a datapoint/evidence, replacing the common.h include with #include <zephyr/toolchain/gcc.h> in #61962 avoids the warning.

https://github.com/thesofproject/sof/actions/runs/6127039819/job/16632163797?pr=8175

/zep_workspace/sof/src/audio/module_adapter/iadk/module_initial_settings_concrete.cpp
In file included from /zep_workspace/zephyr/include/zephyr/toolchain.h:50,
                 from /zep_workspace/zephyr/include/zephyr/sys/time_units.h:10,
                 from /zep_workspace/zephyr/include/zephyr/sys/util.h:640,
                 from /zep_workspace/sof/src/include/sof/audio/module_adapter/iadk/adsp_stddef.h:15,
                 from /zep_workspace/sof/src/audio/module_adapter/iadk/module_initial_settings_concrete.cpp:7:
/zep_workspace/zephyr/include/zephyr/toolchain/gcc.h:87: error: "ZRESTRICT" redefined [-Werror]
   87 | #define ZRESTRICT __restrict
      | 
In file included from /zep_workspace/zephyr/include/zephyr/sys/util.h:18:
/zep_workspace/zephyr/include/zephyr/toolchain/common.h:33: note: this is the location of the previous definition
   33 | #define ZRESTRICT
      | 
cc1plus: all warnings being treated as errors

Originally posted by @marc-hb in #61962 (comment)

The text was updated successfully, but these errors were encountered:

dcpleung · 2023-09-09T00:05:37Z

This is introduced by commit fbf851c where sys/util.h includes sys/time_units.h.

sys/unit.h includes toolchain/common.h in the beginning which sets ZRESTRICT to empty.
sys/unit.h includes sys/time_units.h late in the file.
sys/time_units.h includes toolchain.h which includes toolchain/gcc.h and it also defines ZRESTRICT to actual usable value.

Normally, toolchain/common.h is included by toolchain/<toolchain>.h so each toolchain can have their own macros. toolchain/common.h then defines the macros used by the code but not defined in toolchain specific include file. The inclusion of sys/time_units.h in sys/unit.h reverses this order... which causes the issue.

fabiobaltieri · 2023-09-10T10:17:25Z

@dcpleung is the fix to move the zephyr/sys/time_units.h up in util.h?

dcpleung · 2023-09-11T17:02:56Z

@dcpleung is the fix to move the zephyr/sys/time_units.h up in util.h?

Maybe? I haven't tried it.

IMHO, hierarchy wise, util.h is more fundamental that it should not include time_units.h as it is forcing its content on source files which have nothing to do with time units.

marc-hb · 2023-09-11T20:19:12Z

This is introduced by commit fbf851c where sys/util.h includes sys/time_units.h.

Having two .h files that include each other directly does not make any sense to me; I think this commit should never have been merged.

git checkout fbf851cdc4d8f
git grep -n ...

include/zephyr/sys/time_units.h:11:#include <zephyr/sys/util.h>
include/zephyr/sys/util.h:565:#include <zephyr/sys/time_units.h>

Two .h files including each other means the code in these two .h files can intentionally end-up in one order in some .c files but in another order in other .c file. I imagine it's technically possible for both orders to compile depending on which macros are in use but does anyone seriously want that? Structuring .h files and debugging #include is already incredibly hard (see #41543), I think we really don't want to make it even harder by opening this sort of "varying #include order" possibility. This can make the .h combinatorial explosion blow up even more.

I don't fully understand what the #56006 problem was and I bet it wasn't an easy one but I'm pretty sure the one-line commit fbf851c was not the fix. I guess the proper fix is as usual to split/combine/shuffle the content of .h files into a proper DAG.

cc:

Improve header structure and usage #41543

fabiobaltieri · 2023-09-12T10:53:55Z

Two .h files including each other means the code in these two .h files can intentionally end-up in one order in some .c files but in another order in other .c file. I imagine it's technically possible for both orders to compile depending on which macros are in use but does anyone seriously want that? Structuring .h files and debugging #include is already incredibly hard (see #41543), I think we really don't want to make it even harder by opening this sort of "varying #include order" possibility. This can make the .h combinatorial explosion blow up even more.

Oh I see what you mean now, sorry took me a bit to realize that the problem was the headers including each other

I don't fully understand what the #56006 problem was and I bet it wasn't an easy one but I'm pretty sure the one-line commit fbf851c was not the fix. I guess the proper fix is as usual to split/combine/shuffle the content of .h files into a proper DAG.

Yeah I agree the cross-include makes no sense and is asking for troubles, obviously no one caught that in code review.

@yperess is that something you could look into? Moving the definition between headers and removing the cross include seems to be the right thing.

yperess · 2023-09-12T14:26:31Z

Two .h files including each other means the code in these two .h files can intentionally end-up in one order in some .c files but in another order in other .c file. I imagine it's technically possible for both orders to compile depending on which macros are in use but does anyone seriously want that? Structuring .h files and debugging #include is already incredibly hard (see #41543), I think we really don't want to make it even harder by opening this sort of "varying #include order" possibility. This can make the .h combinatorial explosion blow up even more.

Oh I see what you mean now, sorry took me a bit to realize that the problem was the headers including each other

I don't fully understand what the #56006 problem was and I bet it wasn't an easy one but I'm pretty sure the one-line commit fbf851c was not the fix. I guess the proper fix is as usual to split/combine/shuffle the content of .h files into a proper DAG.

Yeah I agree the cross-include makes no sense and is asking for troubles, obviously no one caught that in code review.

@yperess is that something you could look into? Moving the definition between headers and removing the cross include seems to be the right thing.

I make no guarantee on timeline but I'll be happy to add it to my list

marc-hb · 2023-09-12T14:45:08Z

I make no guarantee on timeline but I'll be happy to add it to my list

Thanks! May I ask what order of magnitude is your current list? I mean days-long or months-long? According to @dcpleung this is what has just turned (in an unfortunate sequence of events) our CI red.

fabiobaltieri · 2023-09-13T10:18:59Z

Ok I'll take a stab at this myself since it's messing with CI.

Commit 9c5dafe ("util: add type checking to CONTAINER_OF") made `util.h` #include `toolchain/common.h` in order to get `BUILD_ASSERT()` used by CONTAINER_OF_VALIDATE() when compiling C code. However `toolchain/common.h` is not supposed to be included directly but indirectly through `toolchain/<your_toolchain>.h` and in a very specific order. The direct inclusion caused the following warning when compiling C++: ``` toolchain/gcc.h:87: error: "ZRESTRICT" redefined [-Werror] 87 | #define ZRESTRICT __restrict | In file included from include/zephyr/sys/util.h:18: note: this is the location of the previous definition: include/zephyr/toolchain/common.h:33: 33 | #define ZRESTRICT ``` Fix this issue zephyrproject-rtos#62464 by including `zephyr/toolchain.h` instead, as done by 350 other files. Fixes commit 9c5dafe ("util: add type checking to CONTAINER_OF") Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2023-09-14T04:15:48Z

Thanks a lot @fabiobaltieri . I tested your #62580 and while it looks like it does fix the circular #include issue, I found it makes no difference for the ZRESTRICT redefinition bug I reported here.

So I spent a bit more more time and I found a much simpler, one-line fix for the ZRESTRICT redefinition bug:

util.h: #include zephyr/toolchain.h, not zephyr/toolchain/common.h #62620

Your fix and mine do not conflict with each other, I believe they're unrelated after all. Sorry for the confusion.

fabiobaltieri · 2023-09-14T07:13:25Z

Ok thanks for checking that @marc-hb, I guess let's go ahead with both, fixing the cross include can't be a bad thing anyway.

Commit 9c5dafe ("util: add type checking to CONTAINER_OF") made `util.h` #include `toolchain/common.h` in order to get `BUILD_ASSERT()` used by CONTAINER_OF_VALIDATE() when compiling C code. However `toolchain/common.h` is not supposed to be included directly but indirectly through `toolchain/<your_toolchain>.h` and in a very specific order. The direct inclusion caused the following warning when compiling C++: ``` toolchain/gcc.h:87: error: "ZRESTRICT" redefined [-Werror] 87 | #define ZRESTRICT __restrict | In file included from include/zephyr/sys/util.h:18: note: this is the location of the previous definition: include/zephyr/toolchain/common.h:33: 33 | #define ZRESTRICT ``` Fix this issue #62464 by including `zephyr/toolchain.h` instead, as done by 350 other files. Fixes commit 9c5dafe ("util: add type checking to CONTAINER_OF") Signed-off-by: Marc Herbert <marc.herbert@intel.com>

dcpleung · 2023-09-14T16:32:23Z

#62620 is merged. So this should have been fixed.

marc-hb · 2023-09-14T17:22:00Z

You mean like this? https://github.com/orgs/community/discussions/17308

I'll close this when daily compilation works again: https://github.com/thesofproject/sof/actions/workflows/daily-tests.yml

Should happen tomorrow.

marc-hb · 2023-09-15T14:36:01Z

Daily build is passing again: https://github.com/thesofproject/sof/actions/runs/6192376904

Commit 9c5dafe ("util: add type checking to CONTAINER_OF") made `util.h` #include `toolchain/common.h` in order to get `BUILD_ASSERT()` used by CONTAINER_OF_VALIDATE() when compiling C code. However `toolchain/common.h` is not supposed to be included directly but indirectly through `toolchain/<your_toolchain>.h` and in a very specific order. The direct inclusion caused the following warning when compiling C++: ``` toolchain/gcc.h:87: error: "ZRESTRICT" redefined [-Werror] 87 | #define ZRESTRICT __restrict | In file included from include/zephyr/sys/util.h:18: note: this is the location of the previous definition: include/zephyr/toolchain/common.h:33: 33 | #define ZRESTRICT ``` Fix this issue zephyrproject-rtos#62464 by including `zephyr/toolchain.h` instead, as done by 350 other files. Fixes commit 9c5dafe ("util: add type checking to CONTAINER_OF") Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb mentioned this issue Sep 8, 2023

util: add type checking to CONTAINER_OF #61962

Merged

marc-hb added bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug platform: Intel ADSP Intel Audio platforms area: Toolchains Toolchains labels Sep 8, 2023

marc-hb mentioned this issue Sep 8, 2023

sof-glk-nocodec: disable pipelines when disabling SSPs thesofproject/sof#8175

Merged

marc-hb mentioned this issue Sep 9, 2023

util.h: Add dependency on time_units.h #56014

Merged

kv2019i mentioned this issue Sep 11, 2023

Remove sparse step1 thesofproject/sof#8166

Merged

fabiobaltieri assigned yperess Sep 12, 2023

kv2019i mentioned this issue Sep 12, 2023

pipeline 2.0: Introducing Data Processing Queue thesofproject/sof#8020

Merged

3 tasks

fabiobaltieri assigned fabiobaltieri and unassigned yperess Sep 13, 2023

fabiobaltieri mentioned this issue Sep 13, 2023

sys: move WAIT_FOR into time_units.h #62580

Closed

marc-hb mentioned this issue Sep 14, 2023

util.h: #include zephyr/toolchain.h, not zephyr/toolchain/common.h #62620

Merged

marc-hb closed this as completed Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZRESTRICT conflict between toolchain/common.h and toolchain/gcc.h #62464

ZRESTRICT conflict between toolchain/common.h and toolchain/gcc.h #62464

marc-hb commented Sep 8, 2023 •

edited

Loading

dcpleung commented Sep 9, 2023 •

edited

Loading

fabiobaltieri commented Sep 10, 2023

dcpleung commented Sep 11, 2023

marc-hb commented Sep 11, 2023 •

edited

Loading

fabiobaltieri commented Sep 12, 2023

yperess commented Sep 12, 2023

marc-hb commented Sep 12, 2023

fabiobaltieri commented Sep 13, 2023

marc-hb commented Sep 14, 2023

fabiobaltieri commented Sep 14, 2023

dcpleung commented Sep 14, 2023

marc-hb commented Sep 14, 2023

marc-hb commented Sep 15, 2023

ZRESTRICT conflict between toolchain/common.h and toolchain/gcc.h #62464

ZRESTRICT conflict between toolchain/common.h and toolchain/gcc.h #62464

Comments

marc-hb commented Sep 8, 2023 • edited Loading

dcpleung commented Sep 9, 2023 • edited Loading

fabiobaltieri commented Sep 10, 2023

dcpleung commented Sep 11, 2023

marc-hb commented Sep 11, 2023 • edited Loading

fabiobaltieri commented Sep 12, 2023

yperess commented Sep 12, 2023

marc-hb commented Sep 12, 2023

fabiobaltieri commented Sep 13, 2023

marc-hb commented Sep 14, 2023

fabiobaltieri commented Sep 14, 2023

dcpleung commented Sep 14, 2023

marc-hb commented Sep 14, 2023

marc-hb commented Sep 15, 2023

marc-hb commented Sep 8, 2023 •

edited

Loading

dcpleung commented Sep 9, 2023 •

edited

Loading

marc-hb commented Sep 11, 2023 •

edited

Loading