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

util: add type checking to CONTAINER_OF #61962

Merged

Conversation

fabiobaltieri
Copy link
Member

@fabiobaltieri fabiobaltieri commented Aug 28, 2023

@fabiobaltieri fabiobaltieri force-pushed the container-of-typecheck branch 8 times, most recently from ad7c966 to 1bcd92d Compare August 29, 2023 09:51
@fabiobaltieri fabiobaltieri force-pushed the container-of-typecheck branch 9 times, most recently from 0c75c9f to 522427a Compare September 4, 2023 15:13
@zephyrbot
Copy link
Collaborator

zephyrbot commented Sep 4, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
sof zephyrproject-rtos/sof@ee40f61 zephyrproject-rtos/sof@fcd3f18 (zephyr) zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@fabiobaltieri fabiobaltieri force-pushed the container-of-typecheck branch 4 times, most recently from 24465ce to 151de10 Compare September 6, 2023 19:58
Add a SAME_TYPE checking macro and use it to add type validation to
CONTAINER_OF. This is inspired by the Linux container_of implementation.

The check is inhibited when using C++ as __builtin_types_compatible_p is
not available there.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri fabiobaltieri marked this pull request as ready for review September 7, 2023 16:39
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Nice. I'm actually surprised this isn't finding more problems.

@fabiobaltieri
Copy link
Member Author

Nice. I'm actually surprised this isn't finding more problems.

Yeah it wasn't too bad, sure there will be more in the weekly run.

@carlescufi carlescufi merged commit 9c5dafe into zephyrproject-rtos:main Sep 7, 2023
22 checks passed
@fabiobaltieri fabiobaltieri deleted the container-of-typecheck branch September 7, 2023 18:18
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 8, 2023

Nice. I'm actually surprised this isn't finding more problems.

You asked, I delivered :-)

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Not sure why this wasn't posted the first time

@@ -15,6 +15,7 @@
#define ZEPHYR_INCLUDE_SYS_UTIL_H_

#include <zephyr/sys/util_macro.h>
#include <zephyr/toolchain/common.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This addition just revealed a funny ZRESTRICT conflict between include/zephyr/toolchain/common.h and include/zephyr/toolchain/gcc.h when compiling C++

As a datapoint/evidence, replacing this line with #include <zephyr/toolchain/gcc.h> 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

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 8, 2023

As ZRESTRICT does not seem to be related to CONTAINER_OF, I filed new bug

trond-snekvik added a commit to trond-snekvik/fw-nrfconnect-nrf that referenced this pull request Nov 17, 2023
The bas_client used the k_delayed_work struct instead of the k_work
struct to resolve the bt_bas_client with CONTAINER_OF. As k_work is the
first member in k_delayed_work, this isn't an active bug, but
zephyrproject-rtos/zephyr#61962 adds a static assert for this, which
starts failing in the upmerge.

Signed-off-by: Trond Einar Snekvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Base OS Base OS Library (lib/os)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants