This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Build and test with -Wundef warning. NFC.
ClosedPublic

Authored by curdeius on Mar 29 2021, 9:27 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG5c703f0fd819: [libc++] Build and test with -Wundef warning. NFC.
Summary

This will avoid typos like _LIBCPP_STD_VERS (<future>) or using #if TEST_STD_VER > 17 without including "test_macros.h".

Diff Detail

Event Timeline

curdeius created this revision.Mar 29 2021, 9:27 AM
curdeius requested review of this revision.Mar 29 2021, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2021, 9:27 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius added inline comments.Mar 29 2021, 9:31 AM
libcxx/include/future
504

This was already dead code, so just commented it out in this patch.
I'll create another patch to fix this if that's wanted. However, the future_error(error_code __ec); gets called with arguments of type future_errc, so adding future_error(future_errc) ctor as future_error(future_errc _Ev) : future_error(make_error_code(_Ev) {} will not add anything.
We might however want to do something more standard conformant (mind that future(error_code) ctor is "exposition only").

curdeius added inline comments.Mar 29 2021, 9:43 AM
libcxx/CMakeLists.txt
582

Maybe that's not the best place to put this flag... Should I put it twice for clang (line 586+), and gcc (line 611+)?

LGTM mod nits.
It's unfortunate that we can't build the entire library with -Wundef — or rather, if we did, we'd have to go through and replace a ton of X > Y with defined(X) && X > Y, and I don't think it'd end up being worth it. This PR shows that -Wundef does catch real bugs, though!

libcxx/CMakeLists.txt
582

It looks to me as if you should put it in the big list of "if supported" flags on lines 582–584, alongside -Wwrite-strings and -Wextra-semi.

libcxx/include/future
504

I recommend just deleting lines 504–506. That clearly preserves the existing behavior (which is not buggy AFAWCT, right?). If the existing behavior is buggy, then you should add a test that detects the bug.
I would not check in commented-out code.

libcxx/test/std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq_all_zero.pass.cpp
29–30

I recommend just omitting the TEST_STD_VER check entirely, and/or omitting the entire lines 28–30. As D99309 shows, <algorithm> is already guaranteed to include <initializer_list>; we don't have to re-include it if re-including it complicates the code (which it seems it does).

curdeius updated this revision to Diff 333963.Mar 29 2021, 12:29 PM
curdeius marked an inline comment as done.
  • Apply review comments.
  • Fix macOS build.

@Quuxplusone, this change broke mac build (it defines _LIBCPP_COMPILER_CLANG but doesn't define _LIBCPP_CLANG_VER) and to fix it I've uglified what you did in D98720.
If you have a better idea, I'm all ears.

BTW, this change finds less real bugs *now* than it could, because I've fixed some tests that used TEST_STD_VER but were missing includes of "test_macros.h" a week or two ago.

curdeius added inline comments.Mar 29 2021, 12:39 PM
libcxx/include/future
504

The existing behaviour is correct but not very strict (it accepts what standard prescribes, but also more).
Precisely:

  • the ctor is not explicit
  • the ctor accepts not only future_errc (which is indirect through error_code), but also error_code

That's seems acceptable though (because changing that will break some code depending on this).
Anyway, it's out of scope of this patch IMO.

curdeius marked 2 inline comments as done.Mar 29 2021, 12:41 PM
libcxx/include/type_traits
839

These are pretty icky. I would tentatively suggest at least breaking the line as

#if __has_keyword(__is_pointer) &&                                             \
    !(defined(_LIBCPP_COMPILER_CLANG) &&                                       \
      defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1100)

but I'd also want to wait and see if anyone has a better idea (e.g. pulling out a _LIBCPP_CLANG_VER_LESS_THAN(x) macro or something).

ldionne requested changes to this revision.Mar 29 2021, 7:04 PM

Requesting changes because of the _LIBCPP_COMPILER_CLANG comment, but this LGTM overall.

libcxx/include/future
504

I'd welcome a patch that makes the constructors strictly standard conforming. Agreed this belongs to a different patch though.

libcxx/include/type_traits
839

I think this is equivalent:

!(defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1100)

In other words, I think checking for _LIBCPP_COMPILER_CLANG is redundant if you already check for _LIBCPP_CLANG_VER unless I'm mistaken.

This revision now requires changes to proceed.Mar 29 2021, 7:04 PM
libcxx/include/type_traits
839

@ldionne: Sadly no; see D98720. Apple Clang defines _LIBCPP_COMPILER_CLANG without _LIBCPP_CLANG_VER (because their version numbers are all screwed up), and so we need to treat Apple Clang essentially as "Clang version 0.0" for the purposes of these feature tests.

Actually, is there any appetite to just #define _LIBCPP_CLANG_VER 0 on Apple Clang?

Double actually, @curdeius, isn't this change wrong? It should be

#if __has_keyword(__is_pointer) &&                                             \
    !(defined(_LIBCPP_COMPILER_CLANG) &&                                       \
      (!defined(_LIBCPP_CLANG_VER) || _LIBCPP_CLANG_VER < 1100))

So I am now more confident in my suggestion that we need either _LIBCPP_CLANG_VER_LESS_THAN or simply to define _LIBCPP_CLANG_VER as 0 on Apple Clang.

curdeius updated this revision to Diff 334065.Mar 30 2021, 12:58 AM
curdeius marked 3 inline comments as done.
  • Define _LIBCPP_CLANG_VER=0 for AppleClang. Fix \#if conditions.
libcxx/include/type_traits
839

Indeed, that was too-quick-a-change from my part.
So, now I #define _LIBCPP_CLANG_VER 0 on AppleClang. It seems to be cleaner this way. It preserves the current behaviour as well.

curdeius updated this revision to Diff 334077.Mar 30 2021, 1:56 AM
  • Fix _LIBCPP_CLANG_VER check (and macOS build).
curdeius added inline comments.Mar 30 2021, 1:59 AM
libcxx/include/__config
513–515

I know, that's ugly...
Should I use __clang__ / __clang_major directly (not sure how it's defined on AppleClang though?

Or, is mapping AppleClang version to clang version somehow possible, @ldionne?

ldionne added inline comments.Mar 30 2021, 5:51 AM
libcxx/include/__config
186

I fear this will have the consequence to disable any feature guarded by #if _LIBCPP_CLANG_VER > XXXX on AppleClang, even if the compiler is perfectly able to handle that feature.

Edit: I looked and this doesn't seem to be an issue, but I would still suggest the following:

#if defined(__clang__) && !defined(__apple_build_version__)
#  define _LIBCPP_COMPILER_CLANG_BASED
#  define _LIBCPP_CLANG_VER (__clang_major__ * 100 + __clang_minor__)
#elif defined(__apple_build_version__)
#  define _LIBCPP_COMPILER_CLANG_BASED
#  define _LIBCPP_APPLE_CLANG_VER (__clang_major__ * 100) + __clang_minor__
#elif ...

Notice the rename of _LIBCPP_COMPILER_CLANG to _LIBCPP_COMPILER_CLANG_BASED. In the few places where we currently use _LIBCPP_COMPILER_CLANG, it seems like what we really want to ask is whether the compiler is Clang-like and supports Clang-like features, which is the case on both LLVM Clang and Apple Clang.

Introducing a different macro _LIBCPP_APPLE_CLANG_VER also makes it clear that we consider it a different compiler.

ldionne added inline comments.Mar 30 2021, 5:53 AM
libcxx/include/__config
513–515

Or, is mapping AppleClang version to clang version somehow possible, @ldionne?

Not really. Since we fork at roughly arbitrary points in time, we would often fall between two official Clang releases and the mapping wouldn't be accurate.

curdeius marked 2 inline comments as done.Mar 30 2021, 6:30 AM
curdeius added inline comments.
libcxx/include/__config
186

Should we really # define _LIBCPP_APPLE_CLANG_VER (__clang_major__ * 100) + __clang_minor__?

Does __clang_minor__ includes the patch number in "10.0.1"?
In the cases where this version is taken into account, we look at the patch number as well (checking __apple_build_version__ directly). E.g.:

// Literal operators ""d and ""y are supported starting with LLVM Clang 8 and AppleClang 10.0.1
#if (defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 800) ||                 \
    (defined(__apple_build_version__) && __apple_build_version__ < 10010000)
#define _LIBCPP_HAS_NO_CXX20_CHRONO_LITERALS
#endif

Shouldn't it be something like: # define _LIBCPP_APPLE_CLANG_VER (__apple_build_version__ / 10000)? I.e. just removing 4 trailing zeroes (e.g. 10010000 -> 1001).

Note: I have no access to apple clang.

libcxx/include/future
504
curdeius updated this revision to Diff 334170.Mar 30 2021, 8:08 AM
curdeius marked an inline comment as done.
  • Define _LIBCPP_APPLE_CLANG_VER.

So with the proposed changes, we'd have:

_LIBCPP_COMPILER_CLANG_BASED // defined whenever the compiler is based on Clang, i.e. LLVM Clang or Apple Clang (but there could be others). This is used to check for general capabilities available on Clang-based compilers like a few extensions or attributes.
_LIBCPP_CLANG_VER // defined to the version of LLVM Clang if the compiler is LLVM Clang, undefined otherwise
_LIBCPP_APPLE_CLANG_VER // defined to the version of Apple Clang if the compiler is Apple Clang, undefined otherwise

I think this makes sense. @curdeius @Quuxplusone I'm curious to hear if you think that makes sense too, but if so let's move forward with that. Perhaps you could also add a comment where _LIBCPP_COMPILER_CLANG_BASED is first defined to explain what I wrote above?

libcxx/include/__config
186

My definition of _LIBCPP_APPLE_CLANG_VER was indeed buggy. I agree with _LIBCPP_APPLE_CLANG_VER == (__apple_build_version__ / 10000). This way, for example AppleClang 12.0.5 would give __apple_build_version__ == 12050019, so _LIBCPP_APPLE_CLANG_VER == 1205, which is quite natural.

libcxx/include/__config
191

@ldionne wrote:

_LIBCPP_COMPILER_CLANG_BASED // defined whenever the compiler is based on Clang, i.e. LLVM Clang or Apple Clang (but there could be others). This is used to check for general capabilities available on Clang-based compilers like a few extensions or attributes.
_LIBCPP_CLANG_VER // defined to the version of LLVM Clang if the compiler is LLVM Clang, undefined otherwise
_LIBCPP_APPLE_CLANG_VER // defined to the version of Apple Clang if the compiler is Apple Clang, undefined otherwise

I think this fits in well with the existing style, but I'm a little worried about how it plays with this patch specifically. Combined with -Wundef, this will lead to pretty complicated conditions. Like, let's say we wanted to enable __is_fundamental on Clang 10+ and Apple Clang 12.5+:

#if __has_keyword(__is_fundamental) &&                                         \
    !(defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1000) &&               \
    !(defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1205) &&   \
    !defined(_LIBCPP_CXX03_LANG)

Or let's say we wanted to enable __is_fundamental on Clang 10+ and never on Apple Clang (which is the current state of the art, because we don't know the Apple Clang version that made this work):

#if __has_keyword(__is_fundamental) &&                                         \
    !(defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1000) &&               \
    !defined(_LIBCPP_APPLE_CLANG_VER) &&                                       \
    !defined(_LIBCPP_CXX03_LANG)

This isn't too bad, I guess; but it's complicated enough that we should audit the final version of this PR very carefully to make sure none of the existing conditions broke or changed behavior accidentally.

curdeius updated this revision to Diff 334190.Mar 30 2021, 9:21 AM

Change definition of _LIBCPP_APPLE_CLANG_VER.

curdeius updated this revision to Diff 334191.Mar 30 2021, 9:22 AM

Fix patch.

curdeius marked 2 inline comments as done.Mar 30 2021, 9:25 AM
curdeius added inline comments.
libcxx/include/__config
191

There's one place only where I used _LIBCPP_APPLE_CLANG_VER instead of __apple_build_version__, line 513.
It should be pretty straightforward.

ldionne added inline comments.Mar 30 2021, 9:27 AM
libcxx/include/__config
191

If we disregard the need to make -Wundef work, we get:

#if __has_keyword(__is_fundamental) &&                                         \
    !(_LIBCPP_CLANG_VER < 1000) &&               \
    !(_LIBCPP_APPLE_CLANG_VER < 1205) &&                                       \
    !defined(_LIBCPP_CXX03_LANG)

I think that's really straightforward, so I think that design is good. I agree it doesn't play nicely with -Wundef though, as it adds a bunch of syntactic noise. In fact, I do think that -Wundef is just generally noisy.

One option (which you suggested before) would be to define macros like _LIBCPP_CLANG_VER_LESSTHAN(xxx) that is considered false if _LIBCPP_CLANG_VER isn't defined. We'd have to do it for each compiler we support, though (so we'd also have _LIBCPP_APPLE_CLANG_VER_LESSTHAN and _LIBCPP_GCC_VER_LESSTHAN, etc.).

libcxx/include/__config
191

If we disregard the need to make -Wundef work ... I think that design is good. I agree it doesn't play nicely with -Wundef though ...

We're on the same page. I think this design is good except in how it interacts with -Wundef. But recall that the original point of this PR is to enable -Wundef! So. :)

It now occurs to me that it might be even better to split "Clang" and "Apple Clang" as utterly as we split "Clang" and "GCC". So we'd have

_LIBCPP_COMPILER_CLANG_BASED // defined whenever the compiler is LLVM Clang or Apple Clang or maybe others
_LIBCPP_COMPILER_CLANG // defined whenever the compiler is LLVM Clang
_LIBCPP_CLANG_VER // defined to the version of LLVM Clang if the compiler is LLVM Clang, undefined otherwise
_LIBCPP_COMPILER_APPLE_CLANG // defined whenever the compiler is Apple Clang
_LIBCPP_APPLE_CLANG_VER // defined to the version of Apple Clang if the compiler is Apple Clang, undefined otherwise

So then my two examples would look like

#if __has_keyword(__is_fundamental) &&                                             \
    !(defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER < 1000) &&              \
    !(defined(_LIBCPP_COMPILER_APPLE_CLANG) && _LIBCPP_APPLE_CLANG_VER < 1205) &&  \
    !defined(_LIBCPP_CXX03_LANG)

and

#if __has_keyword(__is_fundamental) &&                                         \
    !(defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER < 1000) &&          \
    !defined(_LIBCPP_COMPILER_APPLE_CLANG) &&                                  \
    !defined(_LIBCPP_CXX03_LANG)

and I think we'd probably find little use for _LIBCPP_COMPILER_CLANG_BASED.

ldionne accepted this revision.Mar 30 2021, 1:42 PM

LGTM with my fixes (assuming they fix the macOS CI, which they should unless I messed up somewhere).

libcxx/include/type_traits
836–841

AppleClang 12.0.5 should contain commit b25ec4580. See comment below for more info on why this is needed.

1459–1464

We now need to handle AppleClang as well, otherwise we'll be using the intrinsic even on (slightly older) versions of AppleClang where it's broken.

Note that we need to start handling AppleClang "specially" here because we were using _LIBCPP_COMPILER_CLANG, and the proper translation of that is _LIBCPP_COMPILER_CLANG_BASED, not _LIBCPP_CLANG_VER. Previously AppleClang was kept off of this #if by means of _LIBCPP_CLANG_VER < 1300 being considered true.

I checked and we don't need this sort of fix in the other similar places where we have workarounds for Clang 10, not because we don't have this bug, but because the versions of AppleClang that would be impacted are not going to be supported for building libc++ for much longer.

This revision is now accepted and ready to land.Mar 30 2021, 1:42 PM
curdeius updated this revision to Diff 334371.Mar 31 2021, 1:08 AM
curdeius marked 2 inline comments as done.
  • Fix mac build.
ldionne accepted this revision.Mar 31 2021, 10:18 AM

Thanks, ship it!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 1:06 AM