Skip to content

[libc][cmake] Tidy compiler includes #66783

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 19, 2023
Merged

[libc][cmake] Tidy compiler includes #66783

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e9d486
[libc][cmake] Tidy compiler includes
gchatelet Sep 19, 2023
f5dbbcc
Use set_directory_properties
gchatelet Sep 19, 2023
ba68116
Also update tests
gchatelet Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions libc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ endif()
include(${LLVM_COMMON_CMAKE_UTILS}/Modules/CMakePolicy.cmake
NO_POLICY_SCOPE)

# `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 "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use set_directory_properties: https://cmake.org/cmake/help/latest/command/set_directory_properties.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


# Default to C++17
set(CMAKE_CXX_STANDARD 17)

Expand Down
31 changes: 13 additions & 18 deletions libc/cmake/modules/LLVMLibCObjectRules.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ function(_build_gpu_objects fq_target_name internal_target_name)
${ARGN}
)

set(include_dirs ${LIBC_SOURCE_DIR} ${LIBC_INCLUDE_DIR})
set(common_compile_options ${ADD_GPU_OBJ_COMPILE_OPTIONS})
if(NOT ADD_GPU_OBJ_CXX_STANDARD)
set(ADD_GPU_OBJ_CXX_STANDARD ${CMAKE_CXX_STANDARD})
Expand Down Expand Up @@ -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})
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gchatelet gchatelet Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

target_include_directories(${gpu_target_name} PRIVATE ${LIBC_SOURCE_DIR})
target_compile_definitions(${gpu_target_name} PRIVATE LIBC_COPT_PUBLIC_PACKAGING)
set_target_properties(
${gpu_target_name}
PROPERTIES
CXX_STANDARD ${ADD_GPU_OBJ_CXX_STANDARD}
)
set_target_properties(${gpu_target_name} PROPERTIES CXX_STANDARD ${ADD_GPU_OBJ_CXX_STANDARD})
if(ADD_GPU_OBJ_DEPENDS)
add_dependencies(${gpu_target_name} ${ADD_GPU_OBJ_DEPENDS})
endif()
Expand Down Expand Up @@ -261,7 +257,8 @@ function(_build_gpu_objects fq_target_name internal_target_name)
target_compile_options(${fq_target_name} PRIVATE
"SHELL:-Xclang -fembed-offload-object=${packaged_gpu_binary}")
endforeach()
target_include_directories(${fq_target_name} PRIVATE ${include_dirs})
target_include_directories(${fq_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR})
target_include_directories(${fq_target_name} PRIVATE ${LIBC_SOURCE_DIR})
add_dependencies(${fq_target_name}
${full_deps_list} ${packaged_gpu_names} ${stub_target_name})

Expand All @@ -285,7 +282,8 @@ function(_build_gpu_objects fq_target_name internal_target_name)
get_nvptx_compile_options(nvptx_options ${LIBC_GPU_TARGET_ARCHITECTURE})
target_compile_options(${internal_target_name} PRIVATE ${nvptx_options})
endif()
target_include_directories(${internal_target_name} PRIVATE ${include_dirs})
target_include_directories(${internal_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR})
target_include_directories(${internal_target_name} PRIVATE ${LIBC_SOURCE_DIR})
if(full_deps_list)
add_dependencies(${internal_target_name} ${full_deps_list})
endif()
Expand Down Expand Up @@ -369,12 +367,8 @@ function(create_object_library fq_target_name)
${ADD_OBJECT_SRCS}
${ADD_OBJECT_HDRS}
)
target_include_directories(
${fq_target_name}
PRIVATE
${LIBC_SOURCE_DIR}
${LIBC_INCLUDE_DIR}
)
target_include_directories(${fq_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR})
target_include_directories(${fq_target_name} PRIVATE ${LIBC_SOURCE_DIR})
target_compile_options(${fq_target_name} PRIVATE ${compile_options})
endif()

Expand Down Expand Up @@ -633,7 +627,6 @@ function(create_entrypoint_object fq_target_name)
"${ADD_ENTRYPOINT_OBJ_FLAGS}"
${ADD_ENTRYPOINT_OBJ_COMPILE_OPTIONS}
)
set(include_dirs ${LIBC_SOURCE_DIR} ${LIBC_INCLUDE_DIR})
get_fq_deps_list(fq_deps_list ${ADD_ENTRYPOINT_OBJ_DEPENDS})
set(full_deps_list ${fq_deps_list} libc.src.__support.common)

Expand Down Expand Up @@ -670,7 +663,8 @@ function(create_entrypoint_object fq_target_name)
${ADD_ENTRYPOINT_OBJ_HDRS}
)
target_compile_options(${internal_target_name} BEFORE PRIVATE ${common_compile_options})
target_include_directories(${internal_target_name} PRIVATE ${include_dirs})
target_include_directories(${internal_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR})
target_include_directories(${internal_target_name} PRIVATE ${LIBC_SOURCE_DIR})
add_dependencies(${internal_target_name} ${full_deps_list})
target_link_libraries(${internal_target_name} ${full_deps_list})

Expand All @@ -684,7 +678,8 @@ function(create_entrypoint_object fq_target_name)
${ADD_ENTRYPOINT_OBJ_HDRS}
)
target_compile_options(${fq_target_name} BEFORE PRIVATE ${common_compile_options} -DLIBC_COPT_PUBLIC_PACKAGING)
target_include_directories(${fq_target_name} PRIVATE ${include_dirs})
target_include_directories(${fq_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR})
target_include_directories(${fq_target_name} PRIVATE ${LIBC_SOURCE_DIR})
add_dependencies(${fq_target_name} ${full_deps_list})
target_link_libraries(${fq_target_name} ${full_deps_list})
endif()
Expand Down
2 changes: 1 addition & 1 deletion libc/utils/HdrGen/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ add_tablegen(libc-hdrgen LIBC
PublicAPICommand.h
)

target_include_directories(libc-hdrgen PRIVATE ${LIBC_SOURCE_DIR})
target_include_directories(libc-hdrgen PRIVATE ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})
target_link_libraries(libc-hdrgen PRIVATE LibcTableGenUtil)

add_subdirectory(PrototypeTestGen)
1 change: 1 addition & 0 deletions libc/utils/LibcTableGenUtil/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ add_llvm_library(
LINK_COMPONENTS Support TableGen
)
target_include_directories(LibcTableGenUtil PUBLIC ${LIBC_SOURCE_DIR})
target_include_directories(LibcTableGenUtil PRIVATE ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})