Skip to content

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

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 9 commits into from
Jan 12, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cc2061a
[libc] Add compile tests for each public header
frobtech Jan 10, 2025
91da76c
add mising file
frobtech Jan 10, 2025
56d789a
Use HERMETIC_TEST_ONLY, not UNIT_TEST_ONLY
frobtech Jan 10, 2025
76b24bb
Reformat template
frobtech Jan 10, 2025
1370e51
Use a single main declaration.
frobtech Jan 10, 2025
cfa89f6
Merge branch 'main' into p/libc-header-tests
frobtech Jan 10, 2025
36c1dc3
Use -Wsystem-headers; fix license typo.
frobtech Jan 11, 2025
ca8e8cd
Merge branch 'main' into p/libc-header-tests
frobtech Jan 11, 2025
57ee7ea
Merge remote-tracking branch 'refs/remotes/fork/p/libc-header-tests' …
frobtech Jan 11, 2025
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
1 change: 1 addition & 0 deletions libc/test/include/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,7 @@ function(add_header_test target_name source_file deps std_mode)
${source_file}
COMPILE_OPTIONS
-Werror
-Wsystem-headers
-Wall
-Wextra
Copy link
Member

@nickdesaulniers nickdesaulniers Jan 10, 2025

Choose a reason for hiding this comment

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

Do you need to be setting -Wsystem-headers?

Copy link
Contributor Author

@frobtech frobtech Jan 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@frobtech frobtech Jan 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@nickdesaulniers nickdesaulniers Jan 14, 2025

Choose a reason for hiding this comment

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

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..."

-std=${std_mode}
Expand Down
2 changes: 1 addition & 1 deletion libc/test/include/header-test-template.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
*
* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
* See https://llvm.org/LICENSE.txt for license information.
* SPDSList-License-Identifier: Apache-2.0 WITH LLVM-exception
* SPDXList-License-Identifier: Apache-2.0 WITH LLVM-exception
*/

#include <@HEADER_NAME@>
Expand Down
Loading