Use SYSTEM INTERFACE includes to suppress warnings (IDFGH-7212)

[Barabas5532]

GCC suppresses warnings coming from files included from the system path. This is documented here: https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

It is useful in case the file including a header from esp-idf has more warnings enabled than the esp-idf default.

I have an example of how this can go wrong in this build of my project: https://github.com/ShrapnelDSP/ShrapnelMonorepo/runs/6102273709?check_suite_focus=true

Here is a snippet for illustration. The warning is from /opt/esp-idf/components/soc/include/soc/soc_memory_types.h:

In file included from /opt/esp-idf/components/esp_hw_support/include/soc/compare_set.h:12,
                 from /opt/esp-idf/components/esp_hw_support/include/soc/spinlock.h:13,
                 from /opt/esp-idf/components/freertos/port/xtensa/include/freertos/portmacro.h:42,
                 from /opt/esp-idf/components/freertos/include/freertos/portable.h:51,
                 from /opt/esp-idf/components/freertos/include/freertos/FreeRTOS.h:63,
                 from ../components/audio_events/include/audio_events.h:22,
                 from ../components/audio_events/src/audio_events.c:20:
/opt/esp-idf/components/soc/include/soc/soc_memory_types.h: In function 'esp_ptr_dma_ext_capable':
/opt/esp-idf/components/soc/include/soc/soc_memory_types.h:34:66: error: unused parameter 'p' [-Werror=unused-parameter]
 inline static bool IRAM_ATTR esp_ptr_dma_ext_capable(const void *p)
                                                      ~~~~~~~~~~~~^

[CLAassistant]

All committers have signed the CLA.

[DNedic]

Hello, can i ask why are you using
target_include_directories(${lib} SYSTEM INTERFACE ${_dir}) target_include_directories(${lib} PRIVATE ${_dir})
instead of just target_include_directories(${lib} SYSTEM PUBLIC ${_dir})?

[dobairoland]

@Barabas5532 Have you tried to specify additional warnings only for your project and not globally? I think developers usually do that in order to address your issue. If this is an acceptable solution then I would prefer it because it would avoid possible regressions for other developers projects.

BTW, we are accepting PRs for the master branch only. We can later backport serious bugfixes but I don't think this is a candidate for that.

[Barabas5532]

@DNedic

Hello, can i ask why are you using
target_include_directories(${lib} SYSTEM INTERFACE ${_dir}) target_include_directories(${lib} PRIVATE ${_dir})
instead of just target_include_directories(${lib} SYSTEM PUBLIC ${_dir})?

SYSTEM PUBLIC also uses the -isystem flag when the components sources include headers in the same component. This would cause all warnings coming from any header in the project to be suppressed.

Using SYSTEM INTERFACE combined with PRIVATE makes these warnings get suppressed only if the header is included into another component.

@dobairoland

Have you tried to specify additional warnings only for your project and not globally? I think developers usually do that in order to address your issue. If this is an acceptable solution then I would prefer it because it would avoid possible regressions for other developers projects.

I do not have any warning flags defined globally. The warnings I see in my build are from appearing while my files are being built, but are actually coming from a headers that were included from other components. You can see this in the snippet in the original description. The file being build is my source code at ../components/audio_events/src/audio_events.c, but the warning is from /opt/esp-idf/components/soc/include/soc/soc_memory_types.h.

I have a component called compiler_warning_flags which defines the flags. This is linked to each component in my project.

https://github.com/ShrapnelDSP/ShrapnelMonorepo/blob/02c4a78eba3d0022bc21aca94e0d93015e6cd0e2/firmware/components/compiler_warning_flags/CMakeLists.txt

https://github.com/ShrapnelDSP/ShrapnelMonorepo/blob/02c4a78eba3d0022bc21aca94e0d93015e6cd0e2/firmware/components/audio_events/CMakeLists.txt

The if(ESP_PLATFORM) is for unit testing the project on the host, and are not related to this change.

I think building all esp-idf examples and unit tests could be a good regression test. I assume you are doing this in CI already, if not, how could I build all of these easily on my machine? Maybe also need to check that things like deprecation warnings are still appearing.

I've found some discussions about this topic and using SYSTEM INTERFACE include seems to be the best solution:

https://stackoverflow.com/questions/1867065/how-to-suppress-gcc-warnings-from-library-headers

https://www.foonathan.net/2018/10/cmake-warnings/

[DNedic]

@Barabas5532 The problem i see with this is that if there are things inside the component headers that generate legitimate warnings like types, functions and their content etc and you use those outside of the component itself, you will not get those warnings. Therefore something like this can be pretty dangerous.

The source of the issue here and #8881 as well is the fact that every component is linked to the project_elf. We will look at this at a later date as it requires significant changes to the build system.

[DNedic]

@Barabas5532 We are currently working on a solution that involves fixing the root cause of both this and #8881 by linking all components as PRIVATE to the project_elf.

This however is a breaking change and users using target_link_libraries in project-level CMakeLists (to add linker flags for example) or users with custom CMake projects that only include the esp-idf will have to specify PUBLIC, PRIVATE or INTERFACE as an argument to every target_link_libraries call aimed at project_elf.

For this reason, we aim to fix this in ESP-IDF 5.0 and hope that you are able to switch to it when it releases.

[elcojacobs]

I'd like to set the SYSTEM property on all files that are included from the esp-idf path, so they are excluded by clang-tidy.
Can we override the current behaviour in the top level CMakeLists.txt where project(...) is called before 5.0 is out?

After project(), can we set the SYSTEM property for certain targets or can we get the list of these include directories to include them again as SYSTEM, overwriting the previous include?

I see that the list is generated in project(), but also unset again.
@CvRXX

[Barabas5532]

I'm just testing out ESP-IDF 5.0 for C++20 support, and this issue is not fixed by the linked commit. I have created a repo for a minimal reproduction example here: https://github.com/Barabas5532/esp-idf-include-warnings-issue. I've just taken the blink_cxx example, enabled Wsign-compare and included freertos/FreeRTOS.h.

The error produced with ESP-IDF at the v5.0-beta1 tag is:

ccache /home/barabas/.espressif/tools/xtensa-esp32-elf/esp-2022r1-RC1-11.2.0/xtensa-esp32-elf/bin/xtensa-esp32-elf-g++ -DMBEDTLS_CONFIG_FILE=\"mbedtls/esp_config.h\" -DUNITY_INCLUDE_CONFIG_H -I/home/barabas/source/esp-idf-bugs/warning-in-esp-idf-system-files/build/config -I/home/barabas/source/esp-idf-bugs/warning-in-esp-idf-system-files/main -I/home/barabas/source/esp-idf/components/newlib/platform_include -I/home/barabas/source/esp-idf/components/freertos/FreeRTOS-Kernel/include -I/home/barabas/source/esp-idf/components/freertos/esp_additions/include/freertos -I/home/barabas/source/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/include -I/home/barabas/source/esp-idf/components/freertos/esp_additions/include -I/home/barabas/source/esp-idf/components/esp_hw_support/include -I/home/barabas/source/esp-idf/components/esp_hw_support/include/soc -I/home/barabas/source/esp-idf/components/esp_hw_support/include/soc/esp32 -I/home/barabas/source/esp-idf/components/esp_hw_support/port/esp32/. -I/home/barabas/source/esp-idf/components/esp_hw_support/port/esp32/private_include -I/home/barabas/source/esp-idf/components/heap/include -I/home/barabas/source/esp-idf/components/log/include -I/home/barabas/source/esp-idf/components/soc/include -I/home/barabas/source/esp-idf/components/soc/esp32/. -I/home/barabas/source/esp-idf/components/soc/esp32/include -I/home/barabas/source/esp-idf/components/hal/esp32/include -I/home/barabas/source/esp-idf/components/hal/include -I/home/barabas/source/esp-idf/components/hal/platform_port/include -I/home/barabas/source/esp-idf/components/esp_rom/include -I/home/barabas/source/esp-idf/components/esp_rom/include/esp32 -I/home/barabas/source/esp-idf/components/esp_rom/esp32 -I/home/barabas/source/esp-idf/components/esp_common/include -I/home/barabas/source/esp-idf/components/esp_system/include -I/home/barabas/source/esp-idf/components/esp_system/port/soc -I/home/barabas/source/esp-idf/components/esp_system/port/include/private -I/home/barabas/source/esp-idf/components/xtensa/include -I/home/barabas/source/esp-idf/components/xtensa/esp32/include -I/home/barabas/source/esp-idf/components/lwip/include -I/home/barabas/source/esp-idf/components/lwip/include/apps -I/home/barabas/source/esp-idf/components/lwip/include/apps/sntp -I/home/barabas/source/esp-idf/components/lwip/lwip/src/include -I/home/barabas/source/esp-idf/components/lwip/port/esp32/include -I/home/barabas/source/esp-idf/components/lwip/port/esp32/include/arch -I/home/barabas/source/esp-idf/components/esp_ringbuf/include -I/home/barabas/source/esp-idf/components/efuse/include -I/home/barabas/source/esp-idf/components/efuse/esp32/include -I/home/barabas/source/esp-idf/components/esp_adc/include -I/home/barabas/source/esp-idf/components/esp_adc/interface -I/home/barabas/source/esp-idf/components/esp_adc/esp32/include -I/home/barabas/source/esp-idf/components/esp_adc/deprecated/include -I/home/barabas/source/esp-idf/components/driver/include -I/home/barabas/source/esp-idf/components/driver/deprecated -I/home/barabas/source/esp-idf/components/driver/esp32/include -I/home/barabas/source/esp-idf/components/esp_pm/include -I/home/barabas/source/esp-idf/components/mbedtls/port/include -I/home/barabas/source/esp-idf/components/mbedtls/mbedtls/include -I/home/barabas/source/esp-idf/components/mbedtls/mbedtls/library -I/home/barabas/source/esp-idf/components/mbedtls/esp_crt_bundle/include -I/home/barabas/source/esp-idf/components/app_update/include -I/home/barabas/source/esp-idf/components/spi_flash/include -I/home/barabas/source/esp-idf/components/bootloader_support/include -I/home/barabas/source/esp-idf/components/bootloader_support/bootloader_flash/include -I/home/barabas/source/esp-idf/components/pthread/include -I/home/barabas/source/esp-idf/components/esp_timer/include -I/home/barabas/source/esp-idf/components/app_trace/include -I/home/barabas/source/esp-idf/components/esp_event/include -I/home/barabas/source/esp-idf/components/nvs_flash/include -I/home/barabas/source/esp-idf/components/esp_phy/include -I/home/barabas/source/esp-idf/components/esp_phy/esp32/include -I/home/barabas/source/esp-idf/components/vfs/include -I/home/barabas/source/esp-idf/components/esp_netif/include -I/home/barabas/source/esp-idf/components/wpa_supplicant/include -I/home/barabas/source/esp-idf/components/wpa_supplicant/port/include -I/home/barabas/source/esp-idf/components/wpa_supplicant/esp_supplicant/include -I/home/barabas/source/esp-idf/components/esp_wifi/include -I/home/barabas/source/esp-idf/components/unity/include -I/home/barabas/source/esp-idf/components/unity/unity/src -I/home/barabas/source/esp-idf/components/cmock/CMock/src -I/home/barabas/source/esp-idf/components/console -I/home/barabas/source/esp-idf/components/http_parser -I/home/barabas/source/esp-idf/components/esp-tls -I/home/barabas/source/esp-idf/components/esp-tls/esp-tls-crypto -I/home/barabas/source/esp-idf/components/esp_eth/include -I/home/barabas/source/esp-idf/components/esp_gdbstub/include -I/home/barabas/source/esp-idf/components/esp_gdbstub/xtensa -I/home/barabas/source/esp-idf/components/esp_gdbstub/esp32 -I/home/barabas/source/esp-idf/components/esp_hid/include -I/home/barabas/source/esp-idf/components/tcp_transport/include -I/home/barabas/source/esp-idf/components/esp_http_client/include -I/home/barabas/source/esp-idf/components/esp_http_server/include -I/home/barabas/source/esp-idf/components/esp_https_ota/include -I/home/barabas/source/esp-idf/components/esp_lcd/include -I/home/barabas/source/esp-idf/components/esp_lcd/interface -I/home/barabas/source/esp-idf/components/protobuf-c/protobuf-c -I/home/barabas/source/esp-idf/components/protocomm/include/common -I/home/barabas/source/esp-idf/components/protocomm/include/security -I/home/barabas/source/esp-idf/components/protocomm/include/transports -I/home/barabas/source/esp-idf/components/esp_local_ctrl/include -I/home/barabas/source/esp-idf/components/esp_psram/include -I/home/barabas/source/esp-idf/components/sdmmc/include -I/home/barabas/source/esp-idf/components/esp_serial_slave_link/include -I/home/barabas/source/esp-idf/components/espcoredump/include -I/home/barabas/source/esp-idf/components/espcoredump/include/port/xtensa -I/home/barabas/source/esp-idf/components/wear_levelling/include -I/home/barabas/source/esp-idf/components/fatfs/diskio -I/home/barabas/source/esp-idf/components/fatfs/vfs -I/home/barabas/source/esp-idf/components/fatfs/src -I/home/barabas/source/esp-idf/components/idf_test/include -I/home/barabas/source/esp-idf/components/idf_test/include/esp32 -I/home/barabas/source/esp-idf/components/ieee802154/include -I/home/barabas/source/esp-idf/components/json/cJSON -I/home/barabas/source/esp-idf/components/mqtt/esp-mqtt/include -I/home/barabas/source/esp-idf/components/perfmon/include -I/home/barabas/source/esp-idf/components/spiffs/include -I/home/barabas/source/esp-idf/components/ulp/ulp_common/include -I/home/barabas/source/esp-idf/components/ulp/ulp_common/include/esp32 -I/home/barabas/source/esp-idf/components/wifi_provisioning/include -I/home/barabas/source/esp-idf/examples/cxx/experimental/experimental_cpp_component/include -mlongcalls -Wno-frame-address  -ffunction-sections -fdata-sections -Wall -Werror=all -Wno-error=unused-function -Wno-error=unused-variable -Wno-error=deprecated-declarations -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-error=format= -Wno-format -Wno-enum-conversion -gdwarf-4 -ggdb -Og -fmacro-prefix-map=/home/barabas/source/esp-idf-bugs/warning-in-esp-idf-system-files=. -fmacro-prefix-map=/home/barabas/source/esp-idf=/IDF -fstrict-volatile-bitfields -Wno-error=unused-but-set-variable -fno-jump-tables -fno-tree-switch-conversion -DconfigENABLE_FREERTOS_DEBUG_OCDAWARE=1 -std=gnu++20 -fexceptions -fno-rtti -D_GNU_SOURCE -DIDF_VER=\"v5.0-beta1-2-ga1135a3899\" -DESP_PLATFORM -D_POSIX_READER_WRITER_LOCKS -Wsign-compare -MD -MT esp-idf/main/CMakeFiles/__idf_main.dir/main.cpp.obj -MF esp-idf/main/CMakeFiles/__idf_main.dir/main.cpp.obj.d -o esp-idf/main/CMakeFiles/__idf_main.dir/main.cpp.obj -c /home/barabas/source/esp-idf-bugs/warning-in-esp-idf-system-files/main/main.cpp
In file included from /home/barabas/source/esp-idf/components/freertos/FreeRTOS-Kernel/portable/xtensa/include/freertos/portmacro.h:74,
                 from /home/barabas/source/esp-idf/components/freertos/FreeRTOS-Kernel/include/freertos/portable.h:58,
                 from /home/barabas/source/esp-idf/components/freertos/FreeRTOS-Kernel/include/freertos/FreeRTOS.h:70,
                 from /home/barabas/source/esp-idf-bugs/warning-in-esp-idf-system-files/main/main.cpp:14:
/home/barabas/source/esp-idf/components/esp_hw_support/include/spinlock.h: In function 'bool spinlock_acquire(spinlock_t*, int32_t)':
/home/barabas/source/esp-idf/components/esp_hw_support/include/spinlock.h:117:94: error: comparison of integer expressions of different signedness: 'esp_cpu_cycle_count_t' {aka 'long unsigned int'} and 'int32_t' {aka 'long int'} [-Werror=sign-compare]
  117 |     } while ((timeout == SPINLOCK_WAIT_FOREVER) || (esp_cpu_get_cycle_count() - start_count) <= timeout);
      |                                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

The problem i see with this is that if there are things inside the component headers that generate legitimate warnings like types, functions and their content etc and you use those outside of the component itself, you will not get those warnings. Therefore something like this can be pretty dangerous.

I don't think this is a problem, because the component will usually include its own header, and the warnings will show up there. For header only components, you can use a unit test or an empty source file in the same component that includes the header.

For me, the benefit of more error checking at compile time is worth it, but maybe it's not a good default for all users.

[DNedic]

@Barabas5532 The linked issue adds these flags in the main component which has special treatment, explaining the propagation, see here: https://github.com/espressif/esp-idf/blob/master/tools/cmake/project.cmake#L455. We will take a look at this.

I don't think this is a problem, because the component will usually include its own header, and the warnings will show up there. For header only components, you can use a unit test or an empty source file in the same component that includes the header.

Functions and macros inside of the component headers would have their warnings not emitted regardless of them being used in source files, which is rather dangerous.

[DNedic]

Apparently i was wrong about the cause of this, the reason for the failiure to build appears to be the fact that the function generating the failiure is inside of a header that gets included in main.cpp indirectly.

This makes this less of an issue considering there are not many things in component headers from Espressif that can make builds fail, however it does make the issue harder to solve.

[DNedic]

We're not going to include component headers as SYSTEM because this provides valuable data from our customers, please report all failiures in separate issues and we will try to fix them by fixing the headers not to generate warnings instead of hiding them.