[libc++] Use -I instead of -isystem to include headers in the test suite

[philnik]

Summary

Using -isystem marks the headers as system headers, which means that we
don't actually get all the warnings that we'd normally get if we included
the headers as user headers.

The goal of the test suite is normally to mirror as closely as possible
how users would use the library. Technically, this change goes against
that philosophy, since users should be using -isystem (if they ever
need to specify the libc++ path explicitly, which should be a rare
occurence). However, I believe fishing out additional warnings from
the headers provides more value, hence this change. Ideally, we'd be
able to still use -isystem, but instruct Clang to still emit warnings
from the libc++ headers (e.g. we could tell Clang to emit warnings in
any file inside <...>/usr/include/c++/v1).

[jloser]

Ideally, we'd be able to still use -isystem, but instruct Clang to still emit warnings from the libc++ headers (e.g. we could tell Clang to emit warnings in any file inside <...>/usr/include/c++/v1).

I think that this can be done with --no-system-header-prefix=path/to/usr/include/c++/v1 FWIW. Note that while Clang supports this, GCC does not for example.

In any case, I like the ideas for this patch; I'm fine with your current solution.

[ldionne]

In D118616#3284245, @jloser wrote:

Ideally, we'd be able to still use -isystem, but instruct Clang to still emit warnings from the libc++ headers (e.g. we could tell Clang to emit warnings in any file inside <...>/usr/include/c++/v1).

I think that this can be done with --no-system-header-prefix=path/to/usr/include/c++/v1 FWIW. Note that while Clang supports this, GCC does not for example.

In any case, I like the ideas for this patch; I'm fine with your current solution.

Thanks a lot for the information, I wasn't aware of that option at all. This is quite nice.

However, I just had a look into using that option and it looks like it doesn't help us at all. Indeed, the documentation says:

The –system-header-prefix= and –no-system-header-prefix= command-line arguments can be used to override whether subsets of an include path are treated as system headers. When the name in a #include directive is found within a header search path and starts with a system prefix, the header is treated as a system header.

Since we always include libc++ headers without any prefix in the #include <...> directive, this doesn't help. I would have imagined that doing something like --no-system-header-prefix=<...>/include/c++/v1 would do the trick, but trying it out confirms that it would only work if we included libc++ headers from the test suite as #include <include/c++/v1/chrono> for example.

[ldionne]

This is the alternative patch I tried, and it didn't work as one would expect:

commit 09058c0c732ea895def9f3e08b08672b2d5f1bc9
Author: Louis Dionne <[e-mail]>
Date:   Mon Jan 31 11:14:07 2022 -0500

    [libc++] Don't treat libc++ headers as system headers in the test suite
    
    This makes it possible to fish out warnings from our headers inside the
    test suite without having to use `-I` instead of `-isystem` to include
    those headers. This patch should pretty much negate the need for
    _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER, which was only used to enable
    these warnings in the test suite and when building the library.
    
    Differential Revision: https://reviews.llvm.org/D118616

diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index 429e2d82c97b..2c5837075f4a 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -42,6 +42,8 @@ DEFAULT_FEATURES = [
                                                                  sys.platform.lower().strip() == 'darwin'), # TODO: this doesn't handle cross-compiling to Apple platforms.
   Feature(name='objective-c++',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc')),
   Feature(name='verify-support',                when=lambda cfg: hasCompileFlag(cfg, '-Xclang -verify-ignore-unexpected')),
+  Feature(name='warnings-in-system-headers',    when=lambda cfg: hasCompileFlag(cfg, '--no-system-header-prefix=%{include}'),
+                                                actions=[AddCompileFlag('--no-system-header-prefix=%{include}')]),
 
   Feature(name='non-lockfree-atomics',
           when=lambda cfg: sourceBuilds(cfg, """
@@ -89,8 +91,7 @@ DEFAULT_FEATURES = [
   Feature(name=lambda cfg: 'apple-clang-{__clang_major__}.{__clang_minor__}'.format(**compilerMacros(cfg)),                        when=_isAppleClang),
   Feature(name=lambda cfg: 'apple-clang-{__clang_major__}.{__clang_minor__}.{__clang_patchlevel__}'.format(**compilerMacros(cfg)), when=_isAppleClang),
 
-  Feature(name='clang',                                                                                                            when=_isClang,
-          actions=[AddCompileFlag('-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER')]),
+  Feature(name='clang',                                                                                                            when=_isClang),
   Feature(name=lambda cfg: 'clang-{__clang_major__}'.format(**compilerMacros(cfg)),                                                when=_isClang),
   Feature(name=lambda cfg: 'clang-{__clang_major__}.{__clang_minor__}'.format(**compilerMacros(cfg)),                              when=_isClang),
   Feature(name=lambda cfg: 'clang-{__clang_major__}.{__clang_minor__}.{__clang_patchlevel__}'.format(**compilerMacros(cfg)),       when=_isClang),

[ldionne]

Workaround failure on AIX

[ldionne]

Try to fix the failure differently.

[philnik]

Commandeering to fix clang-tidy, and Louis is not available at the moment.

[philnik]

• Also fix clang-tidy in this patch

[philnik]

• Fix modules build

[philnik]

• Fix CI

[philnik]

• Hopefully fix Windows CI

[philnik]

• Next try to fix Windows

[philnik]

• And another try

[philnik]

• Maybe this works

[mstorsjo]

libcxx/include/__config_site.in

39 ↗

(On Diff #410084)

For the cases where this was needed, the safest fix would be to make sure to include libcxx headers before standard c headers. Or alternatively we could maybe consider if _LIBCPP_EXTRA_SITE_DEFINES should add an #undef before each define.

[EricWF]

Wait... How long have we been using -isystem in the test suite. That suppresses all warnings in the headers along that path.

[• Quuxplusone]

LGTM mod comments, but I consider this to be @ldionne's jurisdiction, and it certainly doesn't seem super urgent, so I think we should just wait for him to take a look.

Can you explain in the commit message how this relates to clang-tidy? Even looking at https://reviews.llvm.org/D118616?vs=404892&id=409993#toc I don't really understand your comment "Also fix clang-tidy in this patch."

libcxx/include/__config

94–96 ↗

(On Diff #410084)

Then personally I'd prefer to pass -Wno-macro-redefined in those specific tests. We shouldn't do anything in <__config> that is beneficial only for certain tests in the test suite; we should do it here iff we think it'll benefit real users. Do we want real users to be able to do, like, -D_LIBCPP_ABI_ALTERNATE_STRING_LAYOUT=1? If so, then lines 95-96 are good but the comment on line 94 should talk about that, not about tests in the test suite.

libcxx/include/__iterator/iterator_traits.h

201 ↗

(On Diff #410084)

Please file a bug for this (I assume a clang-tidy bug, right?) and link it from here and/or in the commit message.

libcxx/include/__ranges/reverse_view.h

113–129 ↗

(On Diff #410084)

These changes LGTM but should be committed separately from whatever else is going on here.
You can just say "Reviewed as part of D118616". Or I'll do it myself if I get around to it tomorrow morning. :)

libcxx/include/ext/__hash

19–22 ↗

(On Diff #410084)

Here and below, this shouldn't be necessary. Remember the economist's $100 bill: if this were needed, why wouldn't we be doing it everywhere? (E.g., in <string>.) It seems likely that there's some master list of "libc++ headers" somewhere and that <ext/__hash> and <ext/hash_map> are accidentally missing from that list.

libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp

16

I might think this also needs to be guarded by && defined(__clang__), but I suppose we never test any non-Clang compilers on AIX. Okay.

[philnik]

In D118616#3333754, @Quuxplusone wrote:

LGTM mod comments, but I consider this to be @ldionne's jurisdiction, and it certainly doesn't seem super urgent, so I think we should just wait for him to take a look.

Can you explain in the commit message how this relates to clang-tidy? Even looking at https://reviews.llvm.org/D118616?vs=404892&id=409993#toc I don't really understand your comment "Also fix clang-tidy in this patch."

Louis opened this PR before I landed the clang-tidy PR and this happens to be the fix for clang-tidy ignoring the headers. With the "Also fix clang-tidy in this patch" I meant that I'm fixing the warnings that clang-tidy produced after rebasing. I'm not sure what to add to the commit message. It's exactly the same problem, only that it's not clang producing the warnings, but clang-tidy.

libcxx/include/__config_site.in

39 ↗

(On Diff #410084)

I don't think the first option makes sense. That would mean that it makes a difference in which order the headers are included, which I'm not a fan of. The second one is an option, but I didn't have any luck with it yet.

libcxx/include/__iterator/iterator_traits.h

201 ↗

(On Diff #410084)

clang-tidy is kind-of right. __i - __i is a redundant expression, and I had to look at it a few times before I got why it's correct in this case. Where else would subtracting something from itself make sense?

libcxx/include/ext/__hash

19–22 ↗

(On Diff #410084)

I thought the <ext/*> headers weren't in module.modulemap, because they were deprecated. It's also possible they are not in there by accident.

[Mordante]

I didn't look at the patch closely.

libcxx/include/__iterator/iterator_traits.h

201 ↗

(On Diff #410084)

I agree with Arthur, this seems like a bug. clang-tidy uses the AST so it knows the code is in a requires clause.

I have a strong preference to at least mention the bug here, that makes validating whether it's fixed in the future easier. I don't object to adding it to the commit message.

[ldionne]

Thanks for commandeering. Can you also change the testing configurations in libcxxabi/test/configs/?

libcxx/include/__config

94–96 ↗

(On Diff #410084)

+1 here, we should just add those suppressions in the tests that need them

[philnik]

• Address comments

[ldionne]

libcxx/include/ext/__hash

19–22 ↗

(On Diff #410084)

I don't understand why this diff is necessary in this patch. What happens if you remove it? What fails?

[philnik]

libcxx/include/ext/__hash

19–22 ↗

(On Diff #410084)

Because we replace -isystem with -I. All warnings in headers are now part of the tests, so there must not be any warnings because of -Werror. So basically, the modules build would fail.

[• Quuxplusone]

libcxx/include/__locale

24–25 ↗

(On Diff #411805)

I don't think you should touch the order of these two lines (because I'm paranoid). But if your goal is to alphabetize the includes in this file, then you should also touch lines 31-32 at the same time.

libcxx/include/__ranges/reverse_view.h

113–129 ↗

(On Diff #410084)

I finally remembered this comment, and landed these six lines in 6d751c410d6f5400c2c26d47e5033679fbd736d6. Please rebase. :)

[EricWF]

libcxx/include/ext/__hash

19–22 ↗

(On Diff #410084)

But there's a #pragma system_header?

[• Quuxplusone]

libcxx/include/ext/__hash

19–22 ↗

(On Diff #410084)

I thought the <ext/*> headers weren't in module.modulemap, because they were deprecated. It's also possible they are not in there by accident.

I hypothesize that they're missing only by accident. I've posted D120758, and I'd be interested to see if you can eliminate these diffs by rebasing on it (or just adopting its diffs into this PR; I'll be happy to abandon it).

[philnik]

• Rebased
• Address comments
• Exclude ext/ from modules build

Adding the things in ext/ to the modulemap doesn't work, because it would ignore __DEPRECATED.

[• Quuxplusone]

libcxx/include/ext/__hash

13 ↗

(On Diff #412460)

I notice this is not guarded correctly by _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER. We should fix that, but probably fix it after the dust has settled on all these reviews.

libcxx/utils/libcxx/test/params.py

80 ↗

(On Diff #412460)

I think this macro should have the word TEST in it somewhere.

However, since you use it only for the generated header tests, IMHO it would be quite reasonable to just rip out the ext/ headers from those generated tests specifically (i.e. adopt a small portion of D120831's changes), and then you don't need the macro at all, right?

[ldionne]

This LGTM if CI is passing, however I would much rather rip out the ext/ headers from tests and avoid introducing the _LIBCXX_MODULES_BUILD macro.

FWIW, this is going to interact with D120684 almost certainly. The second one to land his patch is probably going to have additional work to do. If CI passes on your patch, I don't mind you shipping it as-is, and then tackling the removal of _LIBCXX_MODULES_BUILD separately, just for the sake of making progress on this (and allowing me to start fixing any remaining issues with my patch).

[philnik]

• Disable some warnings in bad_engine tests

[philnik]

• Disable warnings in trivial_abi tests

libcxx/test/configs/apple-libc++-shared.cfg.in

 # Testing configuration for Apple's system libc++.
 #
 # This configuration differs from a normal LLVM shared library configuration in
 # that we must use DYLD_LIBRARY_PATH to run the tests against the just-built library,
 # since Apple's libc++ has an absolute install_name.
 #
 # We also don't use a per-target include directory layout, so we have only one
 # include directory for the libc++ headers.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}',
     '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{install}/include/c++/v1 -I %{libcxx}/test/support'
+    '-nostdinc++ -I %{install}/include/c++/v1 -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib++ -L %{install}/lib -lc++'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --env DYLD_LIBRARY_PATH=%{install}/lib -- '
 ))
 

libcxx/test/configs/ibm-libc++-shared.cfg.in

 # This testing configuration handles running the test suite against libc++ on
 # AIX using a shared library.
 #
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', ''))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -isystem %{install}/include/c++/v1 -I %{libcxx}/test/support'
+    '-nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -I %{install}/include/c++/v1 -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib++ -L %{install}/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --env LIBPATH=%{install}/lib -- '
 ))
 

libcxx/test/configs/llvm-libc++-mingw.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using either a DLL or a static library, with MinGW/Clang on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', ''))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{include} -isystem %{target-include} -I %{libcxx}/test/support'
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib++ -L %{lib} -lc++'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --env PATH=%{lib} -- '
 ))
 

libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{include} -isystem %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --env PATH=%{lib} -- '
 ))
 

libcxx/test/configs/llvm-libc++-shared-gcc.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a shared library, with GCC. This is done differently from Clang because
 # GCC does not support the -nostdlib++ command-line flag.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', ''))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{include} -isystem %{target-include} -I %{libcxx}/test/support'
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-L %{lib} -Wl,-rpath,%{lib} -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T -- '
 ))
 

libcxx/test/configs/llvm-libc++-shared.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a shared library.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}',
     '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{include} -isystem %{target-include} -I %{libcxx}/test/support'
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++ -pthread'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T -- '
 ))
 

libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a static library, with Clang-cl on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{include} -isystem %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib -L %{lib} -llibc++ -lmsvcrt -lmsvcprt -loldnames'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --env PATH=%{lib} -- '
 ))
 

libcxx/test/configs/llvm-libc++-static.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a static library.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}',
     '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -isystem %{include} -isystem %{target-include} -I %{libcxx}/test/support'
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib++ -L %{lib} -lc++ -lc++abi -pthread'
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T -- '
 ))
 

libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <array>
 
 // template <class T, size_t N >
 // struct array
 
 // Test the size and alignment matches that of an array of a given type.
 
 #include <array>
 #include <iterator>
 #include <type_traits>
 #include <cstddef>
 
 #include "test_macros.h"
 
+// Ignore error about requesting a large alignment not being ABI compatible with older AIX systems.
+#if defined(_AIX)
+# pragma clang diagnostic ignored "-Waix-compat"
+#endif
 
 template <class T, size_t Size>
 struct MyArray {
   T elems[Size];
 };
 
 template <class T, size_t Size>
 void test() {