zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.14k stars 6.23k forks source link

CMake: SCA: Cppcheck finds no defects when including <zephyr/kernel.h> #68946

Open alfonsoespinosa opened 5 months ago

alfonsoespinosa commented 5 months ago

Bug description

When running static analysis through CodeChecker support in the build system (-DZEPHYR_SCA_VARIANT=codechecker), Cppcheck fails to analyze the code.

Found on 3.4, but also verified to fail in 3.5.

To Reproduce Steps to reproduce the behavior:

  1. Introduce some undefined behaviour construct (for instance, zero-division) in for example zephyr/samples/hello_world/src/main.c
  2. west build -b qemu_x86 -- -DZEPHYR_SCA_VARIANT=codechecker
  3. The defect is detected
  4. Include <zephyr/kernel.h> at the top of the source file
  5. Build again (west build), and no defect is found for the source file containing the issue

Expected behavior Native integration of static code analysis should work when including Zephyr headers. Otherwise, it offers no functionality for real projects

Impact I can temporarily get static code analysis with a workaround using a script that removes zephyr/include from the list of include paths (-I parameter) for the call to cppcheck executable

Environment (please complete the following information):

Additional context I have tried running CodeChecker analyze with the same arguments set by the build system. Then, when I omit the Cppcheck option --suppress=preprocessorErrorDirective, error #error "Invalid/unknown toolchain configuration" is raising in zephyr/include/zephyr/toolchain.h. When <zephyr/kernel.h> is not included in sample source file, probably zephyr/toolchain.h gets never included, because even removing --suppress=preprocessorErrorDirective, this #error is not thrown, performing the static code analysis as expected.

Ccing @pdgendt

github-actions[bot] commented 5 months ago

Hi @alfonsoespinosa! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

alfonsoespinosa commented 4 months ago

For the Cortex-M I am dealing with, in order to prevent cppcheck from triggering the #error directives that raise in zephyr/toolchain.h, zephyr/toolchain/gcc.h, zephyr/sys/util.h, zephyr/arch/arm/aarch32/error.h, and zephyr/kernel.h these are the definitions I need to provide as arguments to the analyzer.

-D__GNUC__ -D__BYTE_ORDER__=1234 -DCONFIG_ARM -D__CHAR_BIT__=8 -D__SIZEOF_LONG__=4 -D__SIZEOF_LONG_LONG__=8 -DCONFIG_ARMV7_M_ARMV8_M_MAINLINE -DCONFIG_NUM_COOP_PRIORITIES=16 -DCONFIG_NUM_PREEMPT_PRIORITIES=15

They can be placed in a file to let CodeChecker analyze forward them to cppcheck with option --cppcheckargs. That can be added to the input arguments in https://github.com/zephyrproject-rtos/zephyr/blob/a6eef0ba3755f2530c5ce93524e5ac4f5be30194/cmake/sca/codechecker/sca.cmake#L22 so I will probably try to figure out how to generate automatically the file with the definitions, depending some of them from the compiler type, architecture or kernel priorities configuration

pdgendt commented 4 months ago
-D__GNUC__ -D__BYTE_ORDER__=1234 -DCONFIG_ARM -D__CHAR_BIT__=8 -D__SIZEOF_LONG__=4 -D__SIZEOF_LONG_LONG__=8 -DCONFIG_ARMV7_M_ARMV8_M_MAINLINE -DCONFIG_NUM_COOP_PRIORITIES=16 -DCONFIG_NUM_PREEMPT_PRIORITIES=15

They can be placed in a file to let CodeChecker analyze forward them to cppcheck with option --cppcheckargs. That can be added to the input arguments in

Thanks @alfonsoespinosa for the investigation, it does look like the generated Kconfig symbols are not taken into account? Is it possible that cppcheck isn't including the generated autoconf.h file?

alfonsoespinosa commented 4 months ago

Thanks @alfonsoespinosa for the investigation, it does look like the generated Kconfig symbols are not taken into account? Is it possible that cppcheck isn't including the generated autoconf.h file?

It's not being added to the command line arguments by CodeChecker analyze, but indeed that is the purpose of cppcheck option --include:

--include=<file>
                         Force inclusion of a file before the checked file. Can
                         be used for example when checking the Linux kernel,
                         where autoconf.h needs to be included for every file
                         compiled. Works the same way as the GCC -include
                         option.

So it could be also added from CodeChecker analyze in the file passed with --cppcheckargs, and at least the 3 Kconfig-related missing defines would be solved.

Bad news is that doing this causes a really weird collateral damage:

zephyr/include/zephyr/devicetree.h:4231:0: error: failed to expand '_EXC_IRQ_DEFAULT_PRIO', Invalid ## usage when expanding 'DT_CAT3'.
github-actions[bot] commented 2 months ago

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

alfonsoespinosa commented 2 months ago

We have bumped to Zephyr 3.5 and the behaviour changes slightly. The workaround in my previous comment is not working anymore. Apart from now having to define manually also CONFIG_CPU_CORTEX_M (otherwise preprocessor fails here), there are a couple of __ASSERT that do not work in the Memory Management Internal APIs header file (in this case probably an issue with Nordic's porting).

I will try to catch up to 3.5 and then later try to work out something for newer versions

Could you please remove the stale label @MaureenHelm ?

alfonsoespinosa commented 2 months ago

Getting back to this on v3.5.0, cppcheck is giving

`error: failed to expand '__ASSERT', it is invalid to use a preprocessor directive as macro parameter [preprocessorErrorDirective]

if CONFIG_SRAM_BASE_ADDRESS != 0`

here and here in include/zephyr/sys/mem_manage.h. One possible solution is to put the preprocessor directive outside, conditionally defining some macro that later would be used in the __ASSERT(), like this diff does.

There is later an error: failed to expand '_EXC_IRQ_DEFAULT_PRIO', Invalid ## usage when expanding 'DT_CAT3': Unexpected token ')' [preprocessorErrorDirective] here in include/zephyr/arch/arm/cortex_m/exc.h, that seems to be a cppcheck bug about failing to handle ## in macro definitions: https://github.com/danmar/simplecpp/issues/228

github-actions[bot] commented 1 week ago

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.