[libc] Add compile tests for each public header #122527

frobtech · 2025-01-10T21:14:13Z

This adds a test that consists of compiling #include <...>,
pretty much alone, for each public header file in each different
language mode (-std=... compiler switch) with -Werror and many
warnings enabled.

There are several headers that have bugs when used alone, and
many more headers that have bugs in certain language modes. So
for now, compiling the new tests is gated on the cmake switch
-DLLVM_LIBC_BUILD_HEADER_TESTS=ON. When all the bugs are fixed,
the switch will be removed so future regressions don't land.

This adds a test that consists of compiling `#include <...>`, pretty much alone, for each public header file in each different language mode (`-std=...` compiler switch) with -Werror and many warnings enabled. There are several headers that have bugs when used alone, and many more headers that have bugs in certain language modes. So for now, compiling the new tests is gated on the cmake switch -DLLVM_LIBC_BUILD_HEADER_TESTS=ON. When all the bugs are fixed, the switch will be removed so future regressions don't land.

llvmbot · 2025-01-10T21:16:48Z

@llvm/pr-subscribers-libc

Author: Roland McGrath (frobtech)

Changes

This adds a test that consists of compiling #include <...>,
pretty much alone, for each public header file in each different
language mode (-std=... compiler switch) with -Werror and many
warnings enabled.

There are several headers that have bugs when used alone, and
many more headers that have bugs in certain language modes. So
for now, compiling the new tests is gated on the cmake switch
-DLLVM_LIBC_BUILD_HEADER_TESTS=ON. When all the bugs are fixed,
the switch will be removed so future regressions don't land.

Full diff: https://github.com/llvm/llvm-project/pull/122527.diff

2 Files Affected:

(modified) libc/cmake/modules/LLVMLibCHeaderRules.cmake (+2)
(modified) libc/test/include/CMakeLists.txt (+67-1)

diff --git a/libc/cmake/modules/LLVMLibCHeaderRules.cmake b/libc/cmake/modules/LLVMLibCHeaderRules.cmake
index 288e4dade0b472..ea8b76e0f62351 100644
--- a/libc/cmake/modules/LLVMLibCHeaderRules.cmake
+++ b/libc/cmake/modules/LLVMLibCHeaderRules.cmake
@@ -66,6 +66,7 @@ function(add_header target_name)
   set_target_properties(
     ${fq_target_name}
     PROPERTIES
+      HEADER_NAME ${dest_leaf_filename}
       HEADER_FILE_PATH ${dest_file}
       DEPS "${fq_deps_list}"
   )
@@ -164,6 +165,7 @@ function(add_gen_header target_name)
   set_target_properties(
     ${fq_target_name}
     PROPERTIES
+      HEADER_NAME ${ADD_GEN_HDR_GEN_HDR}
       HEADER_FILE_PATH ${out_file}
       DECLS_FILE_PATH "${decl_out_file}"
       DEPS "${fq_deps_list}"
diff --git a/libc/test/include/CMakeLists.txt b/libc/test/include/CMakeLists.txt
index ba21a69a31a3b4..ddb69ebc0c166e 100644
--- a/libc/test/include/CMakeLists.txt
+++ b/libc/test/include/CMakeLists.txt
@@ -422,7 +422,7 @@ add_libc_test(
     -Werror
   DEPENDS
     libc.include.llvm-libc-macros.math_function_macros
-)  
+)
 
 add_libc_test(
   isfinite_c_test
@@ -483,3 +483,69 @@ add_libc_test(
   DEPENDS
     libc.include.llvm-libc-macros.math_function_macros
 )
+
+# Test `#include <...>` of each header in each available language mode.
+# This is gated on -DLLVM_LIBC_BUILD_HEADER_TESTS=ON until all the bugs
+# in headers are fixed so the tests all compile.
+set(TEST_STDC_VERSIONS 89;99;11;17;23)
+set(TEST_STDCXX_VERSIONS 03;11;14;17;20;23;26)
+
+function(add_header_test target_name source_file deps std_mode)
+  if(LLVM_LIBC_BUILD_HEADER_TESTS)
+    add_libc_test(
+      ${target_name}
+      C_TEST
+      UNIT_TEST_ONLY
+      SUITE
+	libc_include_tests
+      SRCS
+	${source_file}
+      COMPILE_OPTIONS
+	-Werror
+	-Wall
+	-Wextra
+	-std=${std_mode}
+      DEPENDS
+	${deps}
+    )
+  endif()
+endfunction()
+
+foreach(target ${TARGET_PUBLIC_HEADERS})
+  string(REPLACE "libc.include." "" header ${target})
+  get_target_property(HEADER_NAME ${target} HEADER_NAME)
+
+  set(test_stdc_file "${CMAKE_CURRENT_BINARY_DIR}/${header}_test.c")
+  configure_file(header-test-template.c ${test_stdc_file} @ONLY)
+  foreach(stdc_version ${TEST_STDC_VERSIONS})
+    add_header_test(
+      "${header}_c${stdc_version}_test"
+      ${test_stdc_file}
+      ${target}
+      "c${stdc_version}"
+    )
+    add_header_test(
+      "${header}_gnu${stdc_version}_test"
+      ${test_stdc_file}
+      ${target}
+      "gnu${stdc_version}"
+    )
+  endforeach()
+
+  set(test_stdcxx_file "${CMAKE_CURRENT_BINARY_DIR}/${header}_test.cpp")
+  configure_file(header-test-template.c ${test_stdcxx_file} @ONLY)
+  foreach(stdcxx_version ${TEST_STDCXX_VERSIONS})
+    add_header_test(
+      "${header}_cpp${stdcxx_version}_test"
+      ${test_stdcxx_file}
+      ${target}
+      "c++${stdcxx_version}"
+    )
+    add_header_test(
+      "${header}_gnucpp${stdcxx_version}_test"
+      ${test_stdcxx_file}
+      ${target}
+      "gnu++${stdcxx_version}"
+    )
+  endforeach()
+endforeach()

jhuber6

Very welcome, I've had lots of random failures from the headers in the past that would've been caught by this.

libc/test/include/CMakeLists.txt

github-actions · 2025-01-10T21:31:08Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nickdesaulniers

Cool; good idea!

libc/test/include/header-test-template.c

libc/test/include/CMakeLists.txt

nickdesaulniers · 2025-01-10T22:20:46Z

libc/test/include/CMakeLists.txt

+      COMPILE_OPTIONS
+	-Werror
+	-Wall
+	-Wextra


Do you need to be setting -Wsystem-headers?

I don't think we need to, but it shouldn't hurt. I'll try it.

These will only be considered "system headers" by the compiler if they're found in a directory that's put on the path with -isystem rather than -I or -idirafter or anything else. AFAICT we use -isystem for the clang headers (the ones copied from clang/lib/Headers) but not for our own. So if I'm right, then using -Wsystem-headers here will only make things fail if the clang headers are not warning-clean, not the libc headers. (I personally am a big fan of thorough warning-clean behavior and -Wsystem-headers, but many headers both in OS installs and perhaps clang and libc++ headers are not actually warning clean in my experience, unfortunately.) For the libc headers, -idirafter is actually the right thing to be using IMHO. But it only really matters to use that instead of -I if you're using toolchain-supplied libc++ headers, which the libc cmake build does not.

I am wrong! I'm not sure yet how we're plumbing things, but we are getting the system header exception for the libc headers. And boy howdy do we need it! To start with, every file uses // comments that -std=c89 rejects. But there are many other issues that look like real bugs. So I think I will indeed turn it on, and the list of fixes needed to get these tests passing is even longer than we thought.

From what I can recall, the use of <> vs "" for #include preprocessor directives changes one of the numbers in the generated line markers. Those control whether the diagnostic is considered from a system header or not. Unless -Wsystem-headers is set, such diagnostics are elided rather than emitted. There's a few other command line flags, a pragma, and specific system directories that can change this, too. I remember setting it once, seeing the additional diagnostics from my system headers, and thinking "ok, well, won't be fixing those any time soon, better leave that warning flag off for now..."

libc/test/include/header-test-template.c

…into p/libc-header-tests

libc/test/include/header-test-template.c

This adds a test that consists of compiling `#include <...>`, pretty much alone, for each public header file in each different language mode (`-std=...` compiler switch) with -Werror and many warnings enabled. There are several headers that have bugs when used alone, and many more headers that have bugs in certain language modes. So for now, compiling the new tests is gated on the cmake switch -DLLVM_LIBC_BUILD_HEADER_TESTS=ON. When all the bugs are fixed, the switch will be removed so future regressions don't land.

frobtech requested review from petrhosek, nickdesaulniers and Caslyn January 10, 2025 21:14

frobtech marked this pull request as ready for review January 10, 2025 21:16

llvmbot added the libc label Jan 10, 2025

jhuber6 reviewed Jan 10, 2025

View reviewed changes

libc/test/include/CMakeLists.txt Outdated Show resolved Hide resolved

add mising file

91da76c

frobtech added 2 commits January 10, 2025 13:31

Use HERMETIC_TEST_ONLY, not UNIT_TEST_ONLY

56d789a

Reformat template

76b24bb

nickdesaulniers approved these changes Jan 10, 2025

View reviewed changes

libc/test/include/header-test-template.c Outdated Show resolved Hide resolved

libc/test/include/CMakeLists.txt Show resolved Hide resolved

frobtech added 2 commits January 10, 2025 14:10

Use a single main declaration.

1370e51

Merge branch 'main' into p/libc-header-tests

cfa89f6

nickdesaulniers reviewed Jan 10, 2025

View reviewed changes

libc/test/include/header-test-template.c Outdated Show resolved Hide resolved

frobtech added 3 commits January 10, 2025 16:52

Use -Wsystem-headers; fix license typo.

36c1dc3

Merge branch 'main' into p/libc-header-tests

ca8e8cd

Merge remote-tracking branch 'refs/remotes/fork/p/libc-header-tests' …

57ee7ea

…into p/libc-header-tests

overmighty reviewed Jan 11, 2025

View reviewed changes

libc/test/include/header-test-template.c Show resolved Hide resolved

frobtech merged commit 5e4b41c into llvm:main Jan 12, 2025
11 checks passed

frobtech deleted the p/libc-header-tests branch January 12, 2025 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc] Add compile tests for each public header #122527

[libc] Add compile tests for each public header #122527

frobtech commented Jan 10, 2025

llvmbot commented Jan 10, 2025

jhuber6 left a comment

github-actions bot commented Jan 10, 2025 •

edited

Loading

nickdesaulniers left a comment

nickdesaulniers Jan 10, 2025

frobtech Jan 11, 2025

frobtech Jan 11, 2025

nickdesaulniers Jan 14, 2025

[libc] Add compile tests for each public header #122527

[libc] Add compile tests for each public header #122527

Conversation

frobtech commented Jan 10, 2025

llvmbot commented Jan 10, 2025

jhuber6 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2025 • edited Loading

nickdesaulniers left a comment

Choose a reason for hiding this comment

nickdesaulniers Jan 10, 2025

Choose a reason for hiding this comment

frobtech Jan 11, 2025

Choose a reason for hiding this comment

frobtech Jan 11, 2025

Choose a reason for hiding this comment

nickdesaulniers Jan 14, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2025 •

edited

Loading