[libc][cmake] Tidy compiler includes #66783
Conversation
|
Before this patch for
After this patch
|
|
Works fine on the GPU build |
libc/CMakeLists.txt
Outdated
| # `llvm-project/llvm/CMakeLists.txt` adds the following directive | ||
| # `include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})` | ||
| # We undo it to be able to precisely control what is getting included. | ||
| set_property(DIRECTORY . PROPERTY INCLUDE_DIRECTORIES "") |
There was a problem hiding this comment.
You can use set_directory_properties: https://cmake.org/cmake/help/latest/command/set_directory_properties.html
|
I've used |
|
Feel free to land this change. We can cleanup as required. |
This reverts commit a35a3b7. This broke libc benchmarks.
We want to activate `llvm-header-guard` (llvm#66477) but the current CMake configuration includes paths that should be `isystem`. This PR restricts the number of `-I` passed to the clang command line and correctly marks the llvm libc include path as `isystem`. This is a reland of llvm#66783 a35a3b7 fixing the benchmark breakage.
| @@ -189,13 +188,10 @@ function(_build_gpu_objects fq_target_name internal_target_name) | |||
| ) | |||
|
|
|||
| target_compile_options(${gpu_target_name} PRIVATE ${compile_options}) | |||
| target_include_directories(${gpu_target_name} PRIVATE ${include_dirs}) | |||
| target_include_directories(${gpu_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR}) | |||
There was a problem hiding this comment.
Marking include directory as "system" will hide warnings from the headers located in this directory.
It might be worth adding -Wsystem-headers to compilation flags to mitigate the issue.
There was a problem hiding this comment.
Thx for chiming in here :)
Marking include directory as "system" will hide warnings from the headers located in this directory.
Yes this is on purpose. At least for clang-tidy we don't want the libc generated headers to participate in the clang-tidy checks (see #66477 (comment))
-Wsystem-headers is indeed a good way to catch errors in generated headers but it will potentially also catch warnings in the headers we don't own (clang provided headers or linux headers). WDYT @sivachandra ?
There was a problem hiding this comment.
Ah, right. For some reason I thought that libc is fully isolated from the OS, but it may indeed pull OS headers, and those probably aren't clean (I can imagine warnings about unsupported attributes, pragmas, etc.).
On the other hand, -isystem suppresses warnings coming from libc's headers when building libc itself. This is probably lesser evil.
We want to activate
llvm-header-guard(#66477) but the current CMake configuration includes paths that should beisystem. This PR restricts the number of-Ipassed to the clang command line and correctly marks the llvm libc include path asisystem.There are still two paths that should be removed but I couldn't find where they are set.
@jhuber6 can I ask you to try this patch for the GPU ? I can't test it on my side.