This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add fallback standard flags and normalize corresponding feature.
AbandonedPublic

Authored by curdeius on Apr 2 2021, 12:17 AM.

Details

Reviewers
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Summary

Using std=c++2a is now equivalent to std=c++20 (if both flags are supported by the compiler).
Features use non-fallback standard names (c++20 instead of c++2a).
E.g. using --param=std=c++2a will detect feature c++20.
Note: you still cannot pass --param=std=c++23 if the compiler doesn't support -std=c++23.

Diff Detail

Event Timeline

curdeius created this revision.Apr 2 2021, 12:17 AM
curdeius requested review of this revision.Apr 2 2021, 12:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 12:17 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/utils/libcxx/test/params.py
11–15

I have two weak rationales here:

  • Technically, it's not "C++23" until it's released. I don't want to get into a situation where we have to remove the string c++23 from this codepath. So we should leave it as c++2b until 2023.
  • I'd like to see us develop a "deploy plan" for moving from one standardization-epoch to the next, which we execute atomically every 3 years. I described my ideal here: [SADLY I CANNOT LOCATE THIS REVIEW ANYMORE maybe @Mordante remembers our discussion and can dig it up?] Therefore I'd like this patch to get us into the "C++20 has been released, C++2b has not yet been released" state. Three years from now, we can atomically move into the "C++23 has been released, C++2c has not yet been released" state. I don't want us to ever be in some halfway state like the "C++23 has been released sorta not really" state described in this patch right now.

D93383 was related. D65043 was loosely related.

I think (but am not 100% sure) that all I'm asking for here is to replace 'c++23' with 'c++2b' in the one place it appears. Everything else can stay the same, right?

I'm ok with removing c++23. I was actually hesitant when adding it.

I like this patch a lot. This makes it a lot easier to switch to newer C++ versions shortly after their release without the need to wait for the compilers to update.
Please update or rebase the patch to trigger the CI.

libcxx/utils/libcxx/test/params.py
11–15

I recall the discussion, but also can't find it anymore. I'm also slightly in favour to use c++2b instead of c++23. I get the impression the members in the committee feel more comfortable with the 3 year plan so I would be very surprised when c++23 becomes c++24. Still I think using c++2b safer and give a nice statement the standard isn't released.

curdeius updated this revision to Diff 335447.Apr 6 2021, 3:01 AM
  • Don't use c++23.
  • Rebase.
libcxx/utils/libcxx/test/params.py
11–15

@ldionne, I'd like to hear you input on the changes in params.py. At least just to check that I haven't done anything stupid (given my lack of knowledge of lit and related stuff).

Apart from that, I start thinking of the plan for adding new standard, if anyone sees other points, please LMK.

Example: adding c++2c, c++23 (before: c++2b) released:

  • In libcxx/utils/libcxx/test/params.py:
    • Change c++2b to c++23, add c++2c to _allStandards.
    • Add 'c++23': 'c++2b' to _standardFallback dictionary.
  • Change c++2b to c++23 and add c++2c to std_dialects list in libcxx/utils/generate_feature_test_macro_components.py.
  • Add C++2c CI job to libcxx/utils/ci/buildkite-pipeline.yml.
  • Add generic-cxx2c to libcxx/utils/ci/run-buildbot.
  • Add libcxx/cmake/caches/Generic-cxx2c.cmake CMake cache file.
  • [Optionally] Add new feature-test macros libcxx/utils/generate_feature_test_macro_components.py.
  • In libcxx/include/__config, bump _LIBCPP_STD_VER for C++2b and add a new value for C++2c.
  • In libcxx/test/support/test_macros.h, set TEST_STD_VER to 23 for C++2b and to 99 for C++2c.
  • In libcxx/test/support/msvc_stdlib_force_include.h, bump TEST_STD_VER for MSVC.
  • In libcxx/CMakeLists.txt, use CXX_STANDARD 23 (or higher?).

Can anyone help me with the CI failure (Runtimes build)?
https://buildkite.com/llvm-project/libcxx-ci/builds/2347#91e9025e-e17d-41eb-a9ba-3bd5fd28c086
(TBH, I'm looking for a confirmation that it's unrelated to this patch, as I've never seen a failure in this test, test_demangle.pass.cpp, and the message libc++abi: terminating is not very informative)

Can anyone help me with the CI failure (Runtimes build)?
https://buildkite.com/llvm-project/libcxx-ci/builds/2347#91e9025e-e17d-41eb-a9ba-3bd5fd28c086
(TBH, I'm looking for a confirmation that it's unrelated to this patch, as I've never seen a failure in this test, test_demangle.pass.cpp, and the message libc++abi: terminating is not very informative)

I'm seeing that in my recent CI runs too - so it's most probably unrelated to your patch, but no idea what triggered it...

OK, right, it seems that it's a non-libc++ change that provoked this failure (Runtimes build uses just-built ToT clang).
FYI.
There are few possible culprits out there, some commit between today (6th April 2021) 8:00 (green https://buildkite.com/llvm-project/libcxx-ci/builds/2342#7a94194a-b986-449a-8c6c-4952a1ec8763, commit b7ef804807855e607da3eba221c1fc59e27f778e) and 10:00 UTC (this patch, red: https://buildkite.com/llvm-project/libcxx-ci/builds/2347#91e9025e-e17d-41eb-a9ba-3bd5fd28c086, based on main's commit f8f4d8f87ba4c1fbb18a4e7f4a5ea03a8b8ec061).

Looks acceptable to me, but I think my comment on sized_delete11.pass.cpp is worth addressing.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete11.pass.cpp
14

Here and above, it looks like the intent is "REQUIRES: c++03 OR c++11". I don't know if there's any way to express that in llvm-lit. However, I predict that these tests fail in c++2b mode; am I right?
It might be better to duplicate these files into sized_delete11.pass.cpp and sized_delete03.pass.cpp, add an appropriate REQUIRES: to each of them, and then never touch them again.

libcxx/utils/libcxx/test/params.py
11–15

Yes, thank you! Specifically https://reviews.llvm.org/D97365#2604643 was the comment I was looking for.

14

I believe this line could be

_allStandardsAndFallbacks = _allStandards + list(_standardFallback.values())

Orthogonally, how about s/_standardFallbackReverse/_undoStandardFallback/? I think that would make line 62 read a little clearer.

curdeius updated this revision to Diff 335518.Apr 6 2021, 8:19 AM
curdeius marked 4 inline comments as done.

Apply review comments.

  • Use REQUIRES.
  • Improving my Python-fu.

Thank you for your comments, Arthur.

curdeius updated this revision to Diff 335521.Apr 6 2021, 8:26 AM
  • Use REQUIRES once more.
ldionne requested changes to this revision.Apr 6 2021, 8:38 AM

The patch looks good to me, but I think we can simplify the implementation in params.py a bit IMO. I left some comments to that effect.

I also agree with the list of things to do when a new standard comes out - do you think it would be possible to document it somewhere? Maybe in Contributing.rst?

libcxx/utils/libcxx/test/params.py
11–14
45–53
58–62
This revision now requires changes to proceed.Apr 6 2021, 8:38 AM
curdeius added inline comments.Apr 6 2021, 8:47 AM
libcxx/utils/libcxx/test/params.py
58–62

Unless I'm mistaken, we still need to call AddFeature, no? @ldionne

curdeius added inline comments.Apr 6 2021, 8:50 AM
libcxx/utils/libcxx/test/params.py
58–62

Also, in actions, I don't have cfg, only std...

curdeius added inline comments.Apr 6 2021, 8:58 AM
libcxx/utils/libcxx/test/params.py
58–62

WDYT about:

AddFeature(next((s for s, f in reversed(_allStandards) if f == std), std)),

?

ldionne added inline comments.Apr 6 2021, 9:10 AM
libcxx/utils/libcxx/test/params.py
58–62

Yeah, it seems I really botched my code change suggestion :-).

Basically anything that works is fine by me, but I found the logic with multiple variables at the beginning of the file to be hard to follow.

curdeius updated this revision to Diff 335556.Apr 6 2021, 9:39 AM
  • Apply Louis' comments.
curdeius marked 4 inline comments as done.Apr 6 2021, 9:53 AM
curdeius added inline comments.
libcxx/utils/libcxx/test/params.py
11–14

I've kept c++1z as I sometimes use oldish gcc (e.g. cross-compiling for AVR). Hope that doesn't bother.

58

We need to have both keys and values here to allow fallbacks.

I'll land it with this suggestion but I don't want to trigger yet another CI only for this (given that the choices is visible only when the user provides a bad argument to --param=std, it isn't really tested anyway).

60

The default needs to be [1] because we don't want to use -std=c++20 on the compiler that supports only -std=c++2a.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete11.pass.cpp
14

Nice! I did not know || would work here. :)

libcxx/utils/libcxx/test/params.py
11–15

I like this. Is it worth adding c++0x, c++1y as fallbacks for those two? I assume the logic of leaving them out is simply that we don't officially support any compiler old enough to lack a c++14 mode. But it might be useful-to-someone and/or encyclopedic to keep them in the script, I dunno.

58–62

I think all we're actually doing here is saying, "If the user gave us c++2a, turn it into c++20." Right? So that would be, like,

def what_were_doing(flag):
  if flag in _allStandards.values():
    AddFeature(next(s for s, f in _allStandards.items() if f == flag))
  else:
    AddFeature(flag)

which is like... exactly what you wrote, except without the reversed, right? The reversed would make a difference only if _allStandards.items() were in some relevant order to begin with.

I'm not thrilled by the readability of this expression, but I have no concrete improvements to suggest.

So I think I'm just asking to remove the reversed(...).

curdeius marked 2 inline comments as done.Apr 6 2021, 12:04 PM
curdeius added inline comments.
libcxx/utils/libcxx/test/params.py
11–15

I can add these if Louis is OK. Louis removed them in his suggested edit.

58–62

Ok, I'll remove reversed.

ldionne added inline comments.Apr 6 2021, 12:14 PM
libcxx/utils/libcxx/test/params.py
60

It seems that we don't ever use getLatestStdFlag(cfg)[0] anymore, do we?

62

I don't understand the if f == std part -- isn't that always False?

curdeius updated this revision to Diff 335624.Apr 6 2021, 12:16 PM
curdeius marked 2 inline comments as done.
  • Fix choices.
  • Remove reversed.
curdeius added inline comments.Apr 6 2021, 12:20 PM
libcxx/utils/libcxx/test/params.py
60

We don't indeed. Should I remove?

62

std here can be the a fallback flag.

curdeius updated this revision to Diff 336397.Apr 9 2021, 4:47 AM
curdeius marked 2 inline comments as done.
  • Clean up.

Sorry it took me so long to understand what this patch was doing, however IMO we don't want to allow people to pass --param std=c++2a to Lit. We want to use --param std=c++20, and then have that query the compiler for either -std=c++20 or -std=c++2a if that's the only thing it supports. I think doing that is simpler, and it leaves the "legacy" -std=XXX parameter names to be handled as implementation details of the test suite without making them prominent when using the test suite.

@ldionne wrote:

IMO we don't want to allow people to pass --param std=c++2a to Lit. We want to use --param std=c++20

Unfortunately that doesn't scale. Today we force people to use --param std=c++2a (because this patch hasn't been committed); and we also force people to use --param std=c++2b (because C++2b hasn't got an official number yet). We don't want to force everyone to have a "flag day" where they all simultaneously get forced over from std=c++2a to std=c++20; we want them to be able to adjust their scripts and stuff on their own time. So, since we need to support std=c++2a as a synonym for std=c++20 here anyway at the compiler level, I think it's quite correct to just reuse that list of synonyms for argument parsing as well. Which is what this patch does.

TLDR: This morning I ran ./bin/llvm-lit --param std=c++2a ../libcxx/test/. I want to run that same command tomorrow morning.

@ldionne wrote:

IMO we don't want to allow people to pass --param std=c++2a to Lit. We want to use --param std=c++20

Unfortunately that doesn't scale. Today we force people to use --param std=c++2a (because this patch hasn't been committed); and we also force people to use --param std=c++2b (because C++2b hasn't got an official number yet). We don't want to force everyone to have a "flag day" where they all simultaneously get forced over from std=c++2a to std=c++20; we want them to be able to adjust their scripts and stuff on their own time. So, since we need to support std=c++2a as a synonym for std=c++20 here anyway at the compiler level, I think it's quite correct to just reuse that list of synonyms for argument parsing as well. Which is what this patch does.

TLDR: This morning I ran ./bin/llvm-lit --param std=c++2a ../libcxx/test/. I want to run that same command tomorrow morning.

I disagree - this is just the interface for running Lit from the command line, I think it's entirely fine if there's a "flag" day, and even if it's not official. There are some things that we need to create engineering solutions for in order not to break people, but I don't think that's one. I think pretty much the only places where there's --param std=c++2a is going to be in the CI scripts (which can be updated in lockstep with this patch), and in the libc++ developers' shell command history or local helper functions. I think it's fine if they just need to change it to --param std=c++20 at some point.

curdeius abandoned this revision.Apr 10 2021, 7:38 AM

Abandoning in favour of D100210.