How to apply stricter compiler warnings on app

[riuriuriu]

How can we apply stricter compiler warnings only on the CMake target app?

Unfortunately, they will also be applied to all Zephyr interface headers which get included by the app (gcc uses -I and not -isystem option).

For example, this simple change to the hello world sample will trigger build errors:

diff --git a/samples/hello_world/CMakeLists.txt b/samples/hello_world/CMakeLists.txt
index ecb7d24bb8..cd246b1c75 100644
--- a/samples/hello_world/CMakeLists.txt
+++ b/samples/hello_world/CMakeLists.txt
@@ -2,7 +2,10 @@
 
 cmake_minimum_required(VERSION 3.20.0)
 
+set(BOARD nucleo_f429zi)
+
 find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
 project(hello_world)
 
 target_sources(app PRIVATE src/main.c)
+target_compile_options(app PRIVATE -Werror=sign-conversion)

Which results in:

ccache /home/user/bin/zephyr-sdk-0.13.2/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc -DCORE_CM4 -DHSE_VALUE=8000000 -DKERNEL -DSTM32F429xx -DUSE_FULL_LL_DRIVER -DUSE_HAL_DRIVER -D_FORTIFY_SOURCE=2 -D__PROGRAM_START -D__ZEPHYR__=1 -I/home/user/projects/company/zephyrproject2/zephyr/include -I/home/user/projects/company/zephyrproject2/zephyr/build/zephyr/include/generated -I/home/user/projects/company/zephyrproject2/zephyr/soc/arm/st_stm32/stm32f4 -I/home/user/projects/company/zephyrproject2/zephyr/drivers -I/home/user/projects/company/zephyrproject2/zephyr/soc/arm/st_stm32/common -I/home/user/projects/company/zephyrproject2/modules/hal/cmsis/CMSIS/Core/Include -I/home/user/projects/company/zephyrproject2/modules/hal/stm32/stm32cube/stm32f4xx/soc -I/home/user/projects/company/zephyrproject2/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include -I/home/user/projects/company/zephyrproject2/modules/hal/stm32/stm32cube/stm32f4xx/drivers/include/Legacy -I/home/user/projects/company/zephyrproject2/modules/hal/stm32/stm32cube/common_ll/include -isystem /home/user/projects/company/zephyrproject2/zephyr/lib/libc/minimal/include -isystem /home/user/bin/zephyr-sdk-0.13.2/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/10.3.0/include -isystem /home/user/bin/zephyr-sdk-0.13.2/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/10.3.0/include-fixed -Werror=sign-conversion -Os -imacros /home/user/projects/company/zephyrproject2/zephyr/build/zephyr/include/generated/autoconf.h -ffreestanding -fno-common -g -gdwarf-4 -fdiagnostics-color=always -mcpu=cortex-m4 -mthumb -mabi=aapcs -mfp16-format=ieee -imacros /home/user/projects/company/zephyrproject2/zephyr/include/toolchain/zephyr_stdint.h -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wno-main -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-asynchronous-unwind-tables -fno-pie -fno-pic -fno-reorder-functions -fno-defer-pop -fmacro-prefix-map=/home/user/projects/company/zephyrproject2/zephyr/samples/hello_world=CMAKE_SOURCE_DIR -fmacro-prefix-map=/home/user/projects/company/zephyrproject2/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/home/user/projects/company/zephyrproject2=WEST_TOPDIR -ffunction-sections -fdata-sections -std=c99 -nostdinc -MD -MT CMakeFiles/app.dir/src/main.c.obj -MF CMakeFiles/app.dir/src/main.c.obj.d -o CMakeFiles/app.dir/src/main.c.obj -c /home/user/projects/company/zephyrproject2/zephyr/samples/hello_world/src/main.c
In file included from /home/user/projects/company/zephyrproject2/zephyr/include/kernel_includes.h:21,
                 from /home/user/projects/company/zephyrproject2/zephyr/include/kernel.h:17,
                 from /home/user/projects/company/zephyrproject2/zephyr/include/zephyr.h:18,
                 from /home/user/projects/company/zephyrproject2/zephyr/samples/hello_world/src/main.c:7:
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h: In function 'atomic_test_bit':
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h:81:49: error: conversion to 'unsigned int' from 'int' may change the sign of the result [-Werror=sign-conversion]
   81 | #define ATOMIC_ELEM(addr, bit) ((addr) + ((bit) / ATOMIC_BITS))
      |                                                 ^
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h:130:32: note: in expansion of macro 'ATOMIC_ELEM'
  130 |  atomic_val_t val = atomic_get(ATOMIC_ELEM(target, bit));
      |                                ^~~~~~~~~~~
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h:132:27: error: conversion to 'unsigned int' from 'int' may change the sign of the result [-Werror=sign-conversion]
  132 |  return (1 & (val >> (bit & (ATOMIC_BITS - 1)))) != 0;
      |                           ^
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h: In function 'atomic_test_and_clear_bit':
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h:81:49: error: conversion to 'unsigned int' from 'int' may change the sign of the result [-Werror=sign-conversion]
   81 | #define ATOMIC_ELEM(addr, bit) ((addr) + ((bit) / ATOMIC_BITS))
      |                                                 ^
/home/user/projects/company/zephyrproject2/zephyr/include/sys/atomic.h:151:19: note: in expansion of macro 'ATOMIC_ELEM'
  151 |  old = atomic_and(ATOMIC_ELEM(target, bit), ~mask);

And many more errors.

Is there a preferred way to set app specific flags and ignore zephyr headers?

Thanks

[stephanosio]

cc @tejlmand

[riuriuriu]

@henrikbrixandersen wrote in #44830 :

You are including said headers in your application, so how would the compiler know not to check them? The headers are included by the preprocessor before handing the source off to the compiler, for which you have set the compiler flags.

Most compilers (gcc, clang) would have the option to declare these headers as System Headers which gives them special treatment concerning warnings.
As noted above, Zephyr headers seem to be included by the option -I and not -isystem. Would it be worth checking the possibility to include them via -isystem?

CMake support System Headers via INTERFACE_SYSTEM_INCLUDE_DIRECTORIES but I didn't see any option how an application could set this. Or is there any?

[phillipjohnston]

Another pro-isystem argument: Including Zephyr headers with -isystem would also improve the use of tools like clang-tidy, where a number of headers (such as in the logging subsystem) produce warnings that cannot be easily filtered out on a per-header basis due to macro expansion. If they were marked as system headers, warnings would be localized to the application.

[yashi]

I love the idea of using specific compiler options for our own apps. But not sure about -isystem. It reads:

All warnings, other than those generated by ‘#warning’ (see Diagnostics), are suppressed while GCC is processing a system header.

I'm sure we want to check both Zephyr proper's as well as our own headers.

[phillipjohnston]

Even if global use of -isystem isn't enforced, it sure would be nice as a build option so people interested in dealing with the headers in this way could do so.

[JPHutchins]

Just dropping in to say this is needed. Maybe I'm crazy, but adding SYSTEM to the zephyr_include_directories definition here:

https://github.com/zephyrproject-rtos/zephyr/blob/8d8b02ef7d25118da94635e4764fa64188a97b8e/cmake/modules/extensions.cmake#LL90C1-L93C14

AKA

# https://cmake.org/cmake/help/latest/command/target_include_directories.html
function(zephyr_include_directories)
  target_include_directories(zephyr_interface SYSTEM INTERFACE ${ARGV})
endfunction()

(making it the same as zephyr_system_include_directories) seems to start to solve my problem (add more compiler warnings to my application source without raising them in the entire zephyr dependency tree). So, what is the intended distinction between the usage of these two shortcuts for target_include_directories?

Should it be possible for the user to add compiler warnings for their application source alone? If so, how will we go about supporting this?

[henrikbrixandersen]

CC: @tejlmand

[JPHutchins]

So far, a wrinkle is that we need the "system" warnings during zephyr dev, but don't want them during a regular application compile.

https://cmake.org/cmake/help/latest/prop_tgt/INTERFACE_SYSTEM_INCLUDE_DIRECTORIES.html#prop_tgt:INTERFACE_SYSTEM_INCLUDE_DIRECTORIES

[phillipjohnston]

That is why I suggested making it a build option.

[JPHutchins]

What do you think would be a good interface for this?

west west build ... --zephyr-system-headers

CMake cmake ... -DZEPHYR_SYSTEM_HEADERS

Not attached to the name "zephyr system headers" since it does not communicate the result.

And technically I think the behavior should be default wherein we have to turn the (apparently not extensive) warnings back on when working on the SDK.

[tejlmand]

Is there a preferred way to set app specific flags and ignore zephyr headers?

unfortunately not atm, as it is the compiler which expands the included headers provided with zephyr_interface().
As you noticed, the compiler may treat header inclusion differently when include dirs are added as SYSTEM includes by CMake.
However,changing this for Zephyr interface includes will not only impact compilation of app lib (sources), but every Zephyr library.
Remember, SYSTEM is not only how includes are treated by compiler, but also order in which include directories are searched.

Even if global use of -isystem isn't enforced, it sure would be nice as a build option so people interested in dealing with the headers in this way could do so.

although it sounds like a very simply thing to do, allowing users to change this as a build option, then the devil is in the details.
Such a change impacts the search order of include directories, which again means that another header could be picked up, which again means code is compiled differently.
Such an option would therefore imply that we should build all subsystems, drivers, etc. both with and without this option to ensure the behavior of each subsys, driver, etc is not changed with this option.

Should it be possible for the user to add compiler warnings for their application source alone?

This sounds like a valid use-case, but would need to be prioritized among lot of other features.
Of course, anyone are welcome to provide a proposal, but care must be taken not to have such proposal impacting every other Zephyr library.

If so, how will we go about supporting this?

Open proposal, feel free to comment or adjust recommendation.

app is a Zephyr library defined here:

zephyr/cmake/modules/kernel.cmake

Lines 207 to 210 in e7fdb31

	# "app" is a CMake library containing all the application code and is
	# modified by the entry point ${APPLICATION_SOURCE_DIR}/CMakeLists.txt
	# that was specified when cmake was called.
	zephyr_library_named(app)

Being a Zephyr library means that it will always inherit zephyr_interface.
We could adjust the creation of only the app lib in such a way that per-default it will inherit zephyr_interface, but can be requested not to do so.
When not inheriting zephyr_interface, then it will be the sample's / application's CMakeLists.txt which would be in charge of defining what interface to inherit.

Such a CMakeLists.txt could then do similar as today:

# top-level CMakeLists.txt
find_package(Zephyr ....)

target_link_libraries(app PUBLIC zephyr_interface)

or it could decide to adjust the inheriting slightly, for example:

target_include_directories(app SYSTEM PUBLIC $<TARGET_PROPERTY:zephyr_interface,INTERFACE_INCLUDE_DIRECTORIES>)

the snippet above will take all include directories from the zephyr_interface, and populate them as system includes, just for the app lib.
Creation of all other zephyr libraries will not be impacted by this, and thus pose no risk in rest of build system.

The above proposal is a very simplistic description, but if anyone wants to pick up such work, then I will be happy to provide bit more information as well as reviewing any implementation proposals.

[phillipjohnston]

Your proposed build override with CMake seems fine to me and is probably easier to deal with than a flag anyway. What confuses me is that I don’t see how it addresses any of your concerns about -isystem changing the search order. That detail either matters or it doesn’t. Of course, I don’t think that’s a problem to begin with. Compiling with/without -isystem solution is exactly the way that libc/libc++ projects are built and used (putting aside every other library that uses this strategy). -isystem exists for the use case motivating this issue. If there’s a behavior change cause by search order shift and you didn’t intend that, there is an underlying build flaw and you’ve got naming collisions or accidental pickups that need to be addressed for correctness. In any case, I’m not a Zephyr user, so good luck to the proposal writer :)

[tejlmand]

there is an underlying build flaw and you’ve got naming collisions or accidental pickups that need to be addressed for correctness.

sometimes one can use the order of search to adjust behavior of inclusion, an example can be inclusion of a 3rd party library where one would not want to patch the library but still want to build such library using adjusted headers.
In such a case, control of the include paths can ensure the the header the overruling header will be used in favor of the 3rd party provided header.

I don't believe any libs in Zephyr does anything like this, but downstream users of Zephyr could make use of such "feature".
But without specific knowledge if this could impact code, then I would consider it risky to allow a flag / setting which impacts compilation in a way that impacts every Zephyr library built in the project, without such a feature being thoroughly tested.

What confuses me is that I don’t see how it addresses any of your concerns about -isystem changing the search order.

The difference is that a change to zephyr_interface on how include directories are passed to compiler impacts every zephyr library, from usb driver, to network lib, to bluetooth stack, etc.

The proposal only impacts app lib itself and only impact app lib for the specific application in question.
Hence it will be the responsibility of the author of such application to decide if app should link to zephyr_interface directly, or man-handle the app lib to apply the Zephyr includes as system includes.
And the app user can decide to build the app one way or another, as it make little sense to built same app in both ways.

Samples provided by Zephyr would not use this feature, and hence Zephyr project itself does not need to verify this part.
Zephyr kernel and other zephyr libraries are compiled as always.
(Zephyr project may provide an example of how to use such feature, just as it provides an example on how to include a Make-based project into Zephyr)

[phillipjohnston]

I understand how overrides function and why to do it - I was hinting in this way with "and you didn't intend that", because if you intended to override the header, it would work as desired once you go to -isystem (and not before, unless by absolute luck or if you used the "" include form to pick up local headers first, which would still work with -isystem).

then I would consider it risky to allow a flag / setting which impacts compilation in a way that impacts every Zephyr library built in the project, without such a feature being thoroughly tested.

Getting back to the part that confuses me: it still needs to be tested if you're going to provide this behavior for application developers. If the concern is that there's a subtle include override behavior within the Zephyr framework/libs and -isystem changes that, the proposed implementation of app would cause a problem for that application but not for the Zephyr targets (e.g., desired headers found properly when building framework targets, but wrong header picked up for application targets). I'm not sure that "we'll wash our hands of this because the application developer must be responsible for fixing that" is really a meaningful angle to take when the concern is header ordering for Zephyr framework/lib/driver/etc. headers. If the intent is to provide a way for users to specify the flag, but the actual use doesn't work, you haven't solved the problem. Better then to just focus on the "user warnings don't get applied to framework headers" angle.

Again, I'm not a Zephyr user. There may be aspects to the build that I simply don't get. I'm only speaking as one of those pesky libc/libc++/other-library maintainers who deals with this flag on a regular basis in non-Zephyr contexts.

[tejlmand]

If the concern is that there's a subtle include override behavior within the Zephyr framework/libs and -isystem changes that, the proposed implementation of app would cause a problem for that application but not for the Zephyr targets

not much different than an app doing a target_include_directories(app PRIVATE <extra-dir>) which is the app author's responsibility.

You may say that if app user use this feature and include headers from e.g. network subsys, and that subsys is flawed, then that app may misbehave, and user should report that design of network subsys seems flawed.
Or it could be a Zephyr module being flawed. Zephyr modules are maintained slightly different than Zephyr itself.

But it may only happen in cases where app directly include headers from a flawed subsystem.
A general change to the way all Zephyr libraries are built is much further reaching and thus a higher risk that something somewhere is flawed.

In the above case, if it was tinycrypt which was flawed, but app doesn't include tinycrypt headers directly (it's used by another subsys, e.g. Bluetooth), then things still works, cause tinycrypt and bluetooth are booth built with the order verified by Zephyr community.

This is the reason for being cautious and the reason for limiting the scope, while providing the possibility of the requested feature to users who has the need.
It also ensures that all warnings will properly be printed when building subsystems, drivers, etc, as only app is using -isystem.

Better then to just focus on the "user warnings don't get applied to framework headers" angle.

and isn't that what we are doing, while at the same time not changing how Zephyr and its drivers / subsystems are compiled ?

[tejlmand]

also note, debugging / comparing a single app which only includes a fraction of the headers from Zephyr is a much smaller and well-bordered task, compared to investigate why something in a completely different parts of the system starts to misbehave.

I don't expect flaws like the described to be present in Zephyr, but the sheer size of Zephyr, source files and headers including additional Zephyr modules simply means I cannot rule out that everything everywhere is perfect.
All I know is the current setup is working and has be well-tested.