[libc++] Define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER whenever we enable warnings in the test suite

[ldionne]

Summary

This should make CI consistent on all the compilers we support. Most of
this patch is working around various warnings emitted by GCC in our code
base, which are now being shown when we compile the tests.

After this patch, the whole test suite should be warning free on all
compilers we support and test, except for a few warnings on GCC that
we silence explicitly until we figure out the proper fix for them.

[• Quuxplusone]

libcxx/utils/libcxx/test/params.py

118–123

Do I understand correctly that this diff is intimately related to D118616?
It sounds like the status quo ante was "We use -isystem for our headers, but -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER on Clang so that in fact they aren't treated as system headers, thus we do get warnings (except, oops, -isystem is still in effect so we don't)".
And it sounds like the status after D118616 + D120684 will be "We use -I for our headers, so we get warnings, except we don't because of pragma GCC system_header, except (if we want warnings) we'll also pass -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER to nerf pragma GCC system_header so that we do get warnings."

I don't want to block either of D118616 + D120684, but I do wonder if it would be a viable followup solution to just eliminate pragma GCC system_header altogether. I mean, isn't the whole point of placing libc++ in a "system include directory" that the compiler already has heuristics for deciding what's a system header and what isn't? What was our original rationale for adding this pragma?

The pragma was already present in @howard.hinnant's first commit 3e519524c1186, and the original rationale for guarding it under _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER was apparently Win32 support, not anything at all related to warning suppression. Even the macro's name is a huge hint that we shouldn't be using it for warning suppression.

[ldionne]

libcxx/utils/libcxx/test/params.py

118–123

Yes, you are correct about the relationship between -isystem and -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER, and in particular the fact that we need both D118616 and this patch in order to really enable warnings in system headers.

I also agree that it would make sense to get rid of pragma GCC system_header altogether -- I didn't know it was for Win32 support originally. However, I am worried that removing pragma GCC system_header would break some users that are using -I include/c++/v1 and compiling with -Werror. Currently, they are not getting warnings from libc++ because of the pragma -- if we removed it, they would start getting those warnings (turned into errors). I suggest we take the shortest path towards re-enabling warnings, which is to land both this and D118616, and then we can go back and try to simply remove pragma GCC system_header altogether. I can run a build on a large code base to try to gauge the impact of it.

[• Quuxplusone]

libcxx/utils/libcxx/test/params.py

118–123

However, I am worried that removing pragma GCC system_header would break some users that are using -I include/c++/v1 and compiling with -Werror.

Good point; I agree with that concern.

I suggest we take the shortest path towards re-enabling warnings

Seems like warnings have been disabled since, like, forever, haven't they? so I'm temperamentally inclined to say "let's not move until we know exactly where we're going." But I don't actually wish to block your plan; go for it AFAIC.

One more thought: Are we sure there's no existing Clang/GCC compiler option like -Weven-in-system-headers? That's really what we... oh, bah, no, that's not what we want. We want to see errors from libc++ system headers only, but continue suppressing any errors from e.g. /usr/include/stdio.h. Yuck. In that case, your plan might get us to the best possible state, except that _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER will be somewhat misnamed; at this point it would be more like a _LIBCPP_DONT_SUPPRESS_WARNINGS.

TLDR, go for it.

[ldionne]

Fix GCC warnings

[EricWF]

Let's just turn -Wliteral-suffix off in the test suite, since it's literally not designed to be useful for standard libraries.

I really don't want to see our code littered with warning pragmas. Either we should clean up the warning everywhere, or turn it off everywhere.

[• Quuxplusone]

In D120684#3351864, @EricWF wrote:

Let's just turn -Wliteral-suffix off in the test suite

+1, assuming that would work.

[ldionne]

Address comments and rebase.

[ldionne]

Work around GCC's handling of [[deprecated]].

[ldionne]

Fix CI failure on GCC

[ldionne]

Rebase onto main (including the -I change)

[• Quuxplusone]

libcxx/test/std/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.pass.cpp

17 ↗

(On Diff #412694)

How many of the diffs are just refactoring #define into ADDITIONAL_COMPILER_FLAGS: -D? I don't think that's related to the point of this patch; I also think it's uncontroversial and you could ship it right now. Or am I missing something?

(Further down, I see some where the #define was separated from the REQUIRES by several lines of code. I suggest moving it up to the line immediately after the REQUIRES.)

[ldionne]

libcxx/test/std/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.pass.cpp

17 ↗

(On Diff #412694)

Will do in a separate commit!

[ldionne]

Hopefully fix all remaining warnings

[• Quuxplusone]

libcxx/include/__algorithm/copy.h

56 ↗

(On Diff #412808)

Here and throughout: what is the compiler complaining about if you don't do this? Implicit conversion from T* to void* is absolutely part of C++ since forever, until forever; and this isn't an ADL situation either; so I can't imagine what a compiler could possibly see to warn about here.

libcxx/include/any

367 ↗

(On Diff #412808)

#include <__utility/unreachable.h>, right?

libcxx/include/experimental/simd

1473–1474 ↗

(On Diff #412808)

Just from a quick glance at this code, I'm confused. Where are the definitions of these friend functions? If they don't actually exist, then perhaps these lines should just be removed, or at least #if 0'd out.

libcxx/include/latch

94 ↗

(On Diff #412808)

Let's use [&] here, because that's the "default, 99% of the time" thing. We're not doing anything special here that would justify deviating from the default thing.
Optionally, inline the lambda in the one place it's used:

__cxx_atomic_wait(&__a.__a_, [&]() {
    return try_wait();
});

libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp

12–14 ↗

(On Diff #412808)

I don't understand this comment.

libcxx/test/support/type_classification/movable.h

71–72 ↗

(On Diff #412808)

I don't understand the relevance of these changes, but I certainly don't object.

[ldionne]

libcxx/include/__algorithm/copy.h

56 ↗

(On Diff #412808)

It is complaining that we're using memmove (or memcpy) on a type that doesn't have a trivial copy constructor or copy assignment operator. Above, we check for is_trivially_copy_assignable, which means that we can sometimes call this function on a type that has a trivial copy assignment, but not a trivial copy constructor.

The same goes for __construct_range_forward, but there instead we are calling memcpy on a type that has a trivial copy constructor, but no trivial copy assignment operator.

I'm not sure why GCC sees fit to warn about this. LMK if you think another approach is better to silence the warnings.

libcxx/include/experimental/simd

1473–1474 ↗

(On Diff #412808)

They are not defined anywhere. Our implementation of experimental::simd is not really complete. I'll #if 0 them out.

libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp

12–14 ↗

(On Diff #412808)

GCC produces the following warning:

In file included from /llvm/build/generic-gcc/include/c++/v1/__concepts/arithmetic.h:13,
                 from /llvm/build/generic-gcc/include/c++/v1/concepts:132,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/incrementable_traits.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/concepts.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/distance.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__algorithm/equal.h:15,
                 from /llvm/build/generic-gcc/include/c++/v1/array:111,
                 from /llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:21:
/llvm/build/generic-gcc/include/c++/v1/type_traits: In instantiation of 'struct std::__1::is_constructible<void (Empty::* const)(const int&)>':
/llvm/build/generic-gcc/include/c++/v1/type_traits:2927:77:   required from 'constexpr const bool std::__1::is_constructible_v<void (Empty::* const)(const int&)>'
/llvm/build/generic-gcc/include/c++/v1/__concepts/constructible.h:28:26:   required from 'void test_not_const() [with T = void (Empty::*)(const int&)]'
/llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:192:62:   required from here
/llvm/build/generic-gcc/include/c++/v1/type_traits:2922:38: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
 2922 |     : public integral_constant<bool, __is_constructible(_Tp, _Args...)>
      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /llvm/build/generic-gcc/include/c++/v1/concepts:138,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/incrementable_traits.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/concepts.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/distance.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__algorithm/equal.h:15,
                 from /llvm/build/generic-gcc/include/c++/v1/array:111,
                 from /llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:21:
/llvm/build/generic-gcc/include/c++/v1/__concepts/constructible.h: In instantiation of 'void test_not_const() [with T = void (Empty::*)(const int&)]':
/llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:192:62:   required from here
/llvm/build/generic-gcc/include/c++/v1/__concepts/constructible.h:37:16: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
   37 |     requires { _Tp{}; } && __default_initializable<_Tp>;
      |                ^~~~~

I believe the relevant bit here is that we're calling std::is_constructible_v<void (Empty::* const)(const int&)>, and GCC is basically saying that we don't care about the const qualifier in Empty::* const.

libcxx/test/support/type_classification/movable.h

71–72 ↗

(On Diff #412808)

Otherwise, GCC warns about deprecated implicit definition of the copy constructor.

[• Quuxplusone]

libcxx/include/__algorithm/copy.h

56 ↗

(On Diff #412808)

We could fix this by adding -Wno-class-memaccess to the tests.
On the one hand, -Wno-class-memaccess feels more philosophically honest; just adding a cast to void* doesn't make the UB go away, and the wording of GCC's warning message doesn't imply that they think adding the cast should shut up the warning. We might find that in GCC n+1, the compiler can see through the cast and warns anyway.
https://godbolt.org/z/K5Ex3hxP6

warning: 'void* memcpy(void*, const void*, size_t)' writing to an object
of non-trivially copyable type 'struct A'; use copy-assignment or
copy-initialization instead [-Wclass-memaccess]

On the other hand, passing -Wno-class-memaccess to our own tests doesn't fix the problem for anybody else... if there even is a problem for anyone else. We're moving to -I for the tests, but surely we expect actual users to be using -isystem and so they'll never care about this warning, right?
Also, passing -Wno-class-memaccess to the tests would fail to alert us if anywhere unintentionally ran foul of this warning.

If this were my own codebase, my preferences might be
(1) -Wno-class-memaccess globally, or
(2) #define _LIBCPP_MEMMOVE(x, y, n) std::memmove((void*)x, (const void*)y, n) and use that in the specific places where the GCC warning would come up; ditto presumably _LIBCPP_MEMCPY.
But (2) kinda sucks.

How about just doing what you're doing (cast to void*), but add a comment on the line above, like

if (__n > 0) {
    // Cast the arguments to silence GCC's -Wclass-memaccess
    std::memmove(static_cast<void*>(__result), static_cast<void const*>(__first), __n * sizeof(_Up));
}

libcxx/include/__iterator/size.h

44–51 ↗

(On Diff #412808)

I didn't notice this before. Isn't this just a "bug" on our and/or the standard's part? Shouldn't this be

template <class _Tp, size_t _Sz>
_LIBCPP_INLINE_VISIBILITY
constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return static_cast<ptrdiff_t>(_Sz); }

It is currently specified to take a ptrdiff_t _Sz parameter, but that's surprising, and if it causes compiler warnings, then we should just file an LWG issue to get it changed.
https://eel.is/c++draft/iterator.range#lib:ssize(T_(&array)%5bN%5d)
I'll send an email.

I think we should just preemptively change our _Sz's type to suppress the warning. I don't think the change is detectable by a conforming program.

[EricWF]

libcxx/utils/libcxx/test/params.py

118–123

No, warnings have not been disabled since forever. I was quite careful to not that that happen prior to my departure when COVID started.

Are we sure there's no existing Clang/GCC compiler option like -Weven-in-system-headers

You mean -Wsystem-headers?

[ldionne]

libcxx/utils/libcxx/test/params.py

118–123

Oooh, I wasn't aware of that flag. This is really neat, and in fact I think it does remove the need for _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER. I suggest:

1. I finish and ship this patch so that we have warnings enabled on all compilers, in all configurations.
2. I go back and investigate the removal of _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER, changing -I to -isystem in our test suite and adding -Wsystem-headers. That would be equivalent to what we have after (1) but simpler.

[• Quuxplusone]

libcxx/include/__iterator/size.h

44–51 ↗

(On Diff #412808)

It turns out it's not that simple. :(
I'm still hoping that we could do something like

template <class _Tp, size_t _Sz>
#ifndef _LIBCPP_HAS_NO_CONCEPTS
  requires (_Sz < numeric_limits<ptrdiff_t>::max())
#endif
_LIBCPP_INLINE_VISIBILITY
constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return _Sz; }

but the first step is to get a regression test (working on that in D121154).

libcxx/utils/libcxx/test/params.py

118–123

Oooh, I wasn't aware of that flag. This is really neat, and in fact I think it does remove the need for _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER.

Eric replied to only the first clause of my stream-of-consciousness, which continued: "... oh, bah, no, that's not what we want. We want to see errors from libc++ system headers only, but continue suppressing any errors from e.g. /usr/include/stdio.h. Yuck."

So sure, investigate the possibility, but I bet -Wsystem-headers isn't going to help us in practice.

[ldionne]

Fix CI and address a few comments.

I've decided to add some -Wno-foo when running with GCC in order to unblock this review ASAP.
The patch still improves the status quo from "no warnings whatsoever" to "all warnings except a few".

[philnik]

libcxx/include/barrier

126–129 ↗

(On Diff #415101)

Is this a drive-by or does any compiler complain here?

libcxx/utils/libcxx/test/params.py

27

Why are you disabling -Wshadow? I'm pretty certain this will regress very fast.

39

This doesn't look like it should be here. Why do we disable this warning?

[ldionne]

[forgot to submit some comments last time I updated the patch]

libcxx/include/__algorithm/copy.h

56 ↗

(On Diff #412808)

I'm actually going to defer solving this problem by adding -Wno-class-memaccess when running the tests with GCC, because I'd rather postpone making this decision than making the wrong call. We can investigate this warning separately without blocking this review.

This won't be a problem for users, since they should all be using -isystem anyway -- this is really just for us, and that's why I'd rather take the time to investigate this warning.

libcxx/include/__iterator/size.h

44–51 ↗

(On Diff #412808)

I'll keep the pragma as-is and I'll pick up D121154.

libcxx/include/barrier

126–129 ↗

(On Diff #415101)

This was a -Wshadow warning. I decided to turn it off because there were many benign instances of it.

libcxx/utils/libcxx/test/params.py

27

Because it results in a lot of not very useful warnings. I'll try enabling it again and give you more details.

39

Last I checked, it resulted in a *ton* of warnings because we often use typedefs in the test suite just to check that a type can be formed. Imagine for example typedef std::vector<int>::pointer Ptr just to confirm that std::vector<int>::pointer is well-formed.

[ldionne]

Address comments and re-enable -Wshadow

[ldionne]

Fix -Wshadow.

[ldionne]

This should finally be good to go. @philnik and/or @EricWF does this look OK to you?

[philnik]

I'm not perfectly happy with some of the new names, but I'm OK with them. I also don't have any better ideas for them currently.

[ldionne]

In D120684#3383749, @philnik wrote:

I'm not perfectly happy with some of the new names, but I'm OK with them. I also don't have any better ideas for them currently.

Yeah, that's the problem. -Wshadow creates the need to dance around quite a bit to get things to compile sometimes.

libcxx/utils/libcxx/test/features.py

DEFAULT_FEATURES = [
   Feature(name='has-clang-tidy',
           when=lambda cfg: runScriptExitCode(cfg, ['clang-tidy --version']) == 0),
 
   Feature(name='apple-clang',                                                                                                      when=_isAppleClang),
   Feature(name=lambda cfg: 'apple-clang-{__clang_major__}'.format(**compilerMacros(cfg)),                                          when=_isAppleClang),
   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),
 
   Feature(name='gcc',                                                                                                              when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}'.format(**compilerMacros(cfg)),                                                         when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}.{__GNUC_MINOR__}'.format(**compilerMacros(cfg)),                                        when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}.{__GNUC_MINOR__}.{__GNUC_PATCHLEVEL__}'.format(**compilerMacros(cfg)),                  when=_isGCC),

libcxx/utils/libcxx/test/params.py

_warningFlags = [
   '-Wno-unused-command-line-argument',
   '-Wno-attributes',
   '-Wno-pessimizing-move',
   '-Wno-c++11-extensions',
   '-Wno-user-defined-literals',
   '-Wno-noexcept-type',
   '-Wno-aligned-allocation-unavailable',
   '-Wno-atomic-alignment',
 
   # GCC warns about places where we might want to add sized allocation/deallocation
   # functions, but we know better what we're doing/testing in the test suite.
   '-Wno-sized-deallocation',
 
   # These warnings should be enabled in order to support the MSVC
   # team using the test suite; They enable the warnings below and
   # expect the test suite to be clean.
   '-Wsign-compare',
   '-Wunused-variable',
   '-Wunused-parameter',
   '-Wunreachable-code',
   '-Wno-unused-local-typedef',
 ]
 
 _allStandards = ['c++03', 'c++11', 'c++14', 'c++17', 'c++20', 'c++2b']
 def getStdFlag(cfg, std):
   fallbacks = {
     'c++11': 'c++0x',
     'c++14': 'c++1y',
     'c++17': 'c++1z',
Parameter(name='stdlib', choices=['llvm-libc++', 'apple-libc++', 'libstdc++', 'msvc'], type=str, default='llvm-libc++',
                 """,
             actions=lambda stdlib: filter(None, [
               AddFeature('stdlib={}'.format(stdlib)),
               # Also add an umbrella feature 'stdlib=libc++' for all flavors of libc++, to simplify
               # the test suite.
               AddFeature('stdlib=libc++') if re.match('.+-libc\+\+', stdlib) else None
             ])),
 
   Parameter(name='enable_warnings', choices=[True, False], type=bool, default=True,
             help="Whether to enable warnings when compiling the test suite.",
-            actions=lambda warnings: [] if not warnings else [
-              AddOptionalWarningFlag(w) for w in _warningFlags
-            ]),
+            actions=lambda warnings: [] if not warnings else
+              [AddOptionalWarningFlag(w) for w in _warningFlags] +
+              [AddCompileFlag('-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER')]
+            ),
 
   Parameter(name='debug_level', choices=['', '0', '1'], type=str, default='',
             help="The debugging level to enable in the test suite.",
             actions=lambda debugLevel: [] if debugLevel == '' else filter(None, [
               AddFeature('debug_level={}'.format(debugLevel)),
               AddCompileFlag('-D_LIBCPP_DEBUG={}'.format(debugLevel)),
               AddFeature('LIBCXX-DEBUG-FIXME') if debugLevel == '1' else None
             ])),