This is an archive of the discontinued LLVM Phabricator instance.

[test] Make tests pass regardless of gnu++14/gnu++17 default
ClosedPublic

Authored by MaskRay on Aug 8 2022, 10:09 PM.

Details

Summary

GCC from 11 onwards defaults to -std=gnu++17 for C++ source files. We want to do the same
(https://discourse.llvm.org/t/c-objc-switch-to-gnu-17-as-the-default-dialect/64360).
Add __cplusplus < 201703L, adjust -verify, or add -std=c++14 so that tests
will pass regardless of gnu++14/gnu++17 default.

We have a desire to mark a test compatible with multiple language standards.
There are ongoing discussions how to add markers in the long term:

As a workaround in the short term, add lit substitutions %std_cxx98-,
%std_cxx11-14, etc. They can be used for tests which work across multiple
language standards. If a range has n standards, run lit multiple times, with
LIT_CLANG_STD_GROUP=0, LIT_CLANG_STD_GROUP=1, etc to cover all n standards.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 8 2022, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 10:09 PM
MaskRay requested review of this revision.Aug 8 2022, 10:09 PM
Herald added a project: Restricted Project. · View Herald Transcript

I wonder what's the difference between this patch and https://reviews.llvm.org/D124434 ? Also add some reviewers that get involved in the previous discussion.

aaron.ballman added inline comments.Aug 9 2022, 7:20 AM
clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
1–2

I'm not really keen on this sort of change because it loses test coverage for other language standard versions. We usually try to write our tests to be standard version agnostic and only specify a specific language mode only when absolutely necessary, which doesn't seem to be the case for a lot of the tests in this patch (like this one).

clang/test/CXX/basic/basic.stc/basic.stc.dynamic/p2.cpp
1

This is another example where we lose all testing in the future when we go to change the default language standard version. It's fine to have a C++14-specific test, but we should still have a test which has no mode specified.

MaskRay added inline comments.Aug 9 2022, 10:36 AM
clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
1–2

I think we can identify such issues (tests which want to test the latest mature standard) and fix them post-transition. This way the transition patch feels more isolated and can more easily be reverted.

Not sure whether @junaire wants to work on this...

aaron.ballman added inline comments.Aug 9 2022, 12:58 PM
clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
1–2

I think we can identify such issues (tests which want to test the latest mature standard) and fix them post-transition. This way the transition patch feels more isolated and can more easily be reverted.

That feels backwards to me, but maybe I'm misunderstanding. If there are tests that are specifically testing C++14 behavior (that did not carry forward into C++17 or later) but don't have a language standard specified on the RUN line, I think we should fix those *before* transitioning the default language standard version because those are NFC fixes that improve the test clarity even if we never make the transition. The transition patch should remain isolated and easily revertible with that approach, but there's less risk that nobody goes back and fixes the tests post-transition.

junaire added inline comments.Aug 9 2022, 4:08 PM
clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
1–2

Not sure whether @junaire wants to work on this...

I don't think I have enough knowledge and experience to do this work, So I would like to abandon my previous patch to hope you can pick it up!

MaskRay added inline comments.Aug 10 2022, 12:58 AM
clang/test/AST/ast-dump-openmp-begin-declare-variant_11.c
1–2

I'll fix some tests.

MaskRay updated this revision to Diff 451377.Aug 10 2022, 1:24 AM

fix some tests in a better way

I didn't do a full review because I spotted enough concerns that we might want to talk about the approach more first. I think the goal should be to avoid adding a -std= to any RUN line unless the test really requires it. I spotted a lot of instances where we went from testing the current language standard mode to losing testing for all language modes later than 14 or 17, which will give us problems when we go to update to C++20 or beyond. I think we want to use more #if __cplusplus or disable diagnostics when possible.

I made another lamentation about the lack of lit facilities that I think would solve these problems for us, but that's a big ask and is not something I insist upon for this. But if you felt like it was a good approach and wanted to work on it, I certainly wouldn't be sad about it. :-D

clang/lib/Basic/LangStandards.cpp
81 ↗(On Diff #451377)

Unintended change used to find the tests that needed updating?

clang/test/AST/ast-dump-undeduced-expr.cpp
1

What from this test requires C++14?

clang/test/AST/sourceranges.cpp
0–2

I assume this change is because we're testing c++17 on the next RUN line and you want to be sure we still get the old test coverage?

The trouble is, when we bump from C++17 to C++20, we'll lose the test coverage for the latest standard.

(Not your problem per se, but I continue to think we have a lit deficiency where we need some facilities to say "run this test in C++N and later", "run this test in C89 through CN", or "run this test in all C++ language modes", etc.)

clang/test/Analysis/blocks.m
2

What from this test is invalid in C++17?

clang/test/CXX/except/except.spec/p2-dynamic-types.cpp
1

Why not pass -Wno-dynamic-exception-spec instead of limiting the test (we support dynamic exception specifications in newer language modes and it would be good to not lose that test coverage there).

clang/test/CXX/except/except.spec/p9-dynamic.cpp
0–1

Same question here as above

MaskRay updated this revision to Diff 451586.Aug 10 2022, 11:38 AM
MaskRay marked 5 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Add lit substitutions

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 11:38 AM

It will take some time to fix all tests properly. Let's have a guideline how to fix them properly. I tried fixing some using several patterns in the last revision. I didn't fix all, because it would likely lead to an unsatisfactory result and I would spend more time :)

A construct which causes an error with C++17 onwards

I keep the existing RUN: lines and use something like

#if __cplusplus < 201703L
void* operator new[](std::size_t) throw(std::bad_alloc); 
#endif

it the construct appears tested elsewhere or

register int ro; // expected-error {{illegal storage class on file-scoped variable}}
#if __cplusplus >= 201703L
// expected-error@-2 {{ISO C++17 does not allow 'register' storage class specifier}}
#elif __cplusplus >= 201103L
// expected-warning@-4 {{'register' storage class specifier is deprecated}}
#endif

if the test file appears the proper place to test the construct.

Pre-C++17 and C++17 have different results

I use something like

// RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 -std=c++14 -fdata-sections -fcolor-diagnostics
// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++17 -fdata-sections -fcolor-diagnostics

...
  TypedefAligned4 TA8c = TA8a + TA8b;  // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'operator+' may result in an unaligned pointer access}}
                                       // expected-warning@-1 {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}}
                                       // precxx17-warning@-2 {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'StructAligned8' may result in an unaligned pointer access}}

Since every RUN line uses -std=.
Pros: using -verify avoids #if directives which make the result look better.
Cons: when we upgrade to C++20, 23, ..., the tests will become less relevant.

Discussion

Do we want to prefer #if in all cases? They work well with -verify tests but not so well with FileCheck since FileCheck does not ignore preprocessor skipped ranges.
Do all CodeGenCXX tests have to be fixed?

clang/test/AST/ast-dump-undeduced-expr.cpp
1

The C++17 diagnostics uses static inline constexpr while pre-C++17 uses static constexpr. Used a regex instead.

clang/test/AST/sourceranges.cpp
0–2

FileCheck does not ignore preprocessor skipped ranges, so it is very difficult to work with both C++14/C++17 defaults, if our intention is to not touch such tests in the default changing patch.

I think the best strategy is to do another pass to clean up the tests after the transition, not trying to address everything in this patch.

clang/test/Analysis/blocks.m
2

Added __attribute__((__nothrow__)) in C++17 mode.

clang/test/CXX/except/except.spec/p2-dynamic-types.cpp
1

TIL -Wno-dynamic-exception-spec. Adopted.

clang/test/CXX/except/except.spec/p9-dynamic.cpp
0–1

The check prefixes assume that this is for C++14.

I use something like

// RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 -std=c++14 -fdata-sections -fcolor-diagnostics
// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++17 -fdata-sections -fcolor-diagnostics

...
  TypedefAligned4 TA8c = TA8a + TA8b;  // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'operator+' may result in an unaligned pointer access}}
                                       // expected-warning@-1 {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}}
                                       // precxx17-warning@-2 {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'StructAligned8' may result in an unaligned pointer access}}

Since every RUN line uses -std=.
Pros: using -verify avoids #if directives which make the result look better.
Cons: when we upgrade to C++20, 23, ..., the tests will become less relevant.

Yeah, that's the main/a major concern as @aaron.ballman has mentioned.

I think in this case a regex over either would be acceptable, since the test probably wasn't intending to test the particular wording for a particular version (presumably if this is the place where this separate wording is uniquely used (rather than some generic diagnostic infrastructure change) then it's got a C++17 test already)

Discussion

Do we want to prefer #if in all cases? They work well with -verify tests but not so well with FileCheck since FileCheck does not ignore preprocessor skipped ranges.

I think where #if works well, it seems good to prefer it, yeah.

Do all CodeGenCXX tests have to be fixed?

I think so.

clang/test/AST/sourceranges.cpp
0–2

I think the best strategy is to do another pass to clean up the tests after the transition, not trying to address everything in this patch.

Not everything needs to be addressed in this patch - these cleanups can be in several preliminary patches that are isolated and not dependent on the switch to C++17.

It looks like in this case there's one CHECK-1Z that's out of place (line 111, test seems to pass OK even if that's a plain CHECK so I guess it probably should be) - and the rest are all at the end, which could be moved to a separate file and done with a hardcoded C++17 target. (but yeah, after the default switch we might end up wanting to remove the hardcoded 17 (allowing the test to become "17 and above"), move more test coverage over to here and leave whatever's 14 specific in the old test)

(maybe as a separate pass, though, to your point - we might be able to go through and remove -std=c++17 from tests once that's the default, so the tests are forwards compatible to the next language version, most likely - but, again, a broader LIT feature that @aaron.ballman is referring to, where tests could specify which configurations (beyond language versions, this could also help cover things like... oh, right here: https://reviews.llvm.org/D125946, tests could be assumed to be correct for clang-repl too, and some mode where clang-repl's used instead of clang for broad-scale testing of clang-repl)

clang/test/CXX/except/except.spec/p9-dynamic.cpp
0–1

so in this case the 17 test could become language-agnostic after the version default change?

It will take some time to fix all tests properly. Let's have a guideline how to fix them properly. I tried fixing some using several patterns in the last revision. I didn't fix all, because it would likely lead to an unsatisfactory result and I would spend more time :)

A construct which causes an error with C++17 onwards

I keep the existing RUN: lines and use something like

#if __cplusplus < 201703L
void* operator new[](std::size_t) throw(std::bad_alloc); 
#endif

it the construct appears tested elsewhere or

register int ro; // expected-error {{illegal storage class on file-scoped variable}}
#if __cplusplus >= 201703L
// expected-error@-2 {{ISO C++17 does not allow 'register' storage class specifier}}
#elif __cplusplus >= 201103L
// expected-warning@-4 {{'register' storage class specifier is deprecated}}
#endif

if the test file appears the proper place to test the construct.

This makes sense to me for sema tests. For tests which cannot have errors in them, like codegen tests or analysis tests, I would say this is a case where we would exhaustively test the old language standards up to C++17 and not beyond (perhaps with a boilerplate comment about why no newer versions need to be tested).

Pre-C++17 and C++17 have different results

I use something like

// RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 -std=c++14 -fdata-sections -fcolor-diagnostics
// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++17 -fdata-sections -fcolor-diagnostics

...
  TypedefAligned4 TA8c = TA8a + TA8b;  // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'operator+' may result in an unaligned pointer access}}
                                       // expected-warning@-1 {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}}
                                       // precxx17-warning@-2 {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'StructAligned8' may result in an unaligned pointer access}}

Since every RUN line uses -std=.
Pros: using -verify avoids #if directives which make the result look better.
Cons: when we upgrade to C++20, 23, ..., the tests will become less relevant.

Yeah, we want to avoid making the tests less relevant in the future when possible. The suggestion from @dblaikie to use a regex in this case makes a lot of sense to me in the situation where what's being tested is not specifically the divergence in diagnostic behavior. e.g., if the test is "do we give a sensible warning here?" then use a regex for this situation, but if the test is "do we give the correct form of the warning here?" then use -verify= and try to leave a RUN line without a specific language standard on it if possible.

Discussion

Do we want to prefer #if in all cases? They work well with -verify tests but not so well with FileCheck since FileCheck does not ignore preprocessor skipped ranges.

I wouldn't say "in all cases", but certainly in cases where it improves the readability of the test or ensures we don't lose test coverage. Cases with FileCheck are a problem, but hopefully aren't too prevalent in the Parser and Sema tests.

Do all CodeGenCXX tests have to be fixed?

Yes, but in that case I expect (at least for errors), it's more a matter of adding RUN lines for all the relevant modes we need to test (since codegen tests really should not be caring about warning diagnostic wording, though some tests still do).

clang/test/AST/sourceranges.cpp
0–2

Not everything needs to be addressed in this patch - these cleanups can be in several preliminary patches that are isolated and not dependent on the switch to C++17.

+1 -- I am imagining the way we want to go forward is to have several patches handling these cleanups. One can handle cases where we need to add a regex or a verify prefix, another which splits files into two as needed, another can introduce new lit features, another can start to use those features, and so on.

This way, the default changing patch can hopefully be relatively small and narrow in scope. Once it's landed, we probably will want to do even more cleanup like removing -std=c++17 RUN lines when possible, etc.

Ultimately, I think the lit feature improvements will require us to go through a lot/most/all-ish of the tests to fix up their RUN lines. When we made the switch to C++14, I don't think we went through and cleaned up tests that already specified -std=c++1y or -std=c++14, so I would not be surprised to see a bunch of tests that lost future version coverage from that switch as well. Whether we want to bite the bullet and do that work up front instead of looking at/touching a lot of tests twice is worth thinking about.

llvm/utils/lit/lit/llvm/config.py
574

If we like this approach, we should probably add add_stdc as well (not as part of this patch, we can do all of C++ first, then come back and hit up C after we've finished).

577

Maybe we can use clang_std_values[-1] so we don't have to hardcode this?

583

One thing we should consider is whether we want to run in *all* the specified language modes instead of just the newest mode. This will make running tests slower because we'll run significantly more of them, and it might get awkward if a lot of tests change behavior in the different language modes, so I don't suggest it as part of this patch.

Sorry, my previous main comment had been written before I introduced LIT_CLANG_STD_GROUP in llvm/utils/lit/lit/llvm/config.py. The multiple %clang_cc1 approach actually looks like the following.
Note the use of %stdcxx_17- to make the test future-proof.
(It is non-trivial to run one RUN line multiples times with different LIT_CLANG_STD_GROUP. For now I just test locally with different LIT_CLANG_STD_GROUP.)

// RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 %stdcxx_11-14 -fdata-sections -fcolor-diagnostics
// RUN: %clang_cc1 %s -fsyntax-only -verify %stdcxx_17- -fdata-sections -fcolor-diagnostics

...
  TypedefAligned4 TA8c = TA8a + TA8b;  // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'operator+' may result in an unaligned pointer access}} \
                                       // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}} \
                                       // precxx17-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'StructAligned8' may result in an unaligned pointer access}}

If this is changed to use #if __cplusplus >= 201703L, there will be multiple lines with relative line numbers (e.g. @-2 @-4)

register int ro; // expected-error {{illegal storage class on file-scoped variable}}
#if __cplusplus >= 201703L
// expected-error@-2 {{ISO C++17 does not allow 'register' storage class specifier}}
#elif __cplusplus >= 201103L
// expected-warning@-4 {{'register' storage class specifier is deprecated}}
#endif

Personally I prefer multiple %clang_cc1 over #if. The first few lines give users a first impression. The dispatch makes it clear the test has different behaviors with the %stdcxx_* described dialects.

llvm/utils/lit/lit/llvm/config.py
583

This is difficult in lit. Will answer in my main comment.

Sorry, my previous main comment had been written before I introduced LIT_CLANG_STD_GROUP in llvm/utils/lit/lit/llvm/config.py. The multiple %clang_cc1 approach actually looks like the following.
Note the use of %stdcxx_17- to make the test future-proof.
(It is non-trivial to run one RUN line multiples times with different LIT_CLANG_STD_GROUP. For now I just test locally with different LIT_CLANG_STD_GROUP.)

// RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 %stdcxx_11-14 -fdata-sections -fcolor-diagnostics
// RUN: %clang_cc1 %s -fsyntax-only -verify %stdcxx_17- -fdata-sections -fcolor-diagnostics

...
  TypedefAligned4 TA8c = TA8a + TA8b;  // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'operator+' may result in an unaligned pointer access}} \
                                       // expected-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}} \
                                       // precxx17-warning {{passing 4-byte aligned argument to 8-byte aligned parameter 'this' of 'StructAligned8' may result in an unaligned pointer access}}

Personally, I like this style. I tend to be a heavy user of -verify prefixes though, so I might be biased.

If this is changed to use #if __cplusplus >= 201703L, there will be multiple lines with relative line numbers (e.g. @-2 @-4)

register int ro; // expected-error {{illegal storage class on file-scoped variable}}
#if __cplusplus >= 201703L
// expected-error@-2 {{ISO C++17 does not allow 'register' storage class specifier}}
#elif __cplusplus >= 201103L
// expected-warning@-4 {{'register' storage class specifier is deprecated}}
#endif

FWIW, you don't have to use relative markers, but that uses an even less common idiom of bookmarks. e.g.,

register int ro; // expected-error {{illegal storage class on file-scoped variable}} \
                    #bookmark
#if __cplusplus >= 201703L
// expected-error@#bookmark {{ISO C++17 does not allow 'register' storage class specifier}}
#elif __cplusplus >= 201103L
// expected-warning@#bookmark {{'register' storage class specifier is deprecated}}
#endif

Personally I prefer multiple %clang_cc1 over #if. The first few lines give users a first impression. The dispatch makes it clear the test has different behaviors with the %stdcxx_* described dialects.

I tend to have the same opinion.

MaskRay updated this revision to Diff 454282.Aug 21 2022, 1:38 AM
MaskRay marked 6 inline comments as done.

fix tests

I like the direction this is going; I ran into some questions on the tests about whether we should use a range or not and other small things, but I think this is getting close.

clang/test/CXX/temp/temp.res/temp.local/p3.cpp
2

Should this be C++17+?

clang/test/CodeGen/typedef_alignment_mismatch_warning.cpp
2

Same question here about C++17+?

clang/test/CodeGenCXX/exception-spec-decay.cpp
1

Should we drop the %stdcxx_98- entirely from tests and not have any -std flag (e.g., no such flags tells lit to run the test in all language modes, eventually)?

clang/test/CodeGenCXX/override-bit-field-layout.cpp
2

17+?

clang/test/CodeGenCXX/override-layout.cpp
1–14

Pre 14? Post 17?

clang/test/Layout/ms-x86-vtordisp.cpp
1–3

Is this test specific to C++14?

clang/test/Parser/cxx-casting.cpp
1–3

LG given that lit doesn't run multiple tests yet, so switching to a range would lose coverage.

clang/test/SemaCXX/libstdcxx_is_pod_hack.cpp
1

Is this one only for C++14 or should there be a range used instead?

llvm/utils/lit/lit/llvm/config.py
577

Thoughts?

583

It's unfortunate that it's difficult in lit. I'm fine punting on that work for now, but I think we should try to invest in it (or are you saying "difficult" as in "not worth the effort"?)

MaskRay updated this revision to Diff 456524.Aug 29 2022, 7:31 PM
MaskRay marked 6 inline comments as done.

address comments

MaskRay added inline comments.Aug 29 2022, 7:33 PM
clang/test/CodeGenCXX/exception-spec-decay.cpp
1

The proposal is probably clean if we the majority of tests work with C++98, but I think we have accrued many tests which don't work with C++98 so we need directives like %stdcxx_11-.

Since C++98 is actually uncommon now. I prefer explicit %stdcxx_98- to indicate a test works with C++98.

clang/test/CodeGenCXX/override-layout.cpp
1–14

Unfortunately, C++17 and C++20 have different behaviors. I haven't investigated why it is the case.

clang/test/Layout/ms-x86-vtordisp.cpp
1–3

This is similar to the previous -fdump-record-layouts test that the dump order is different across 14, 17, 20. I do now know whether there is something which should be improved to add the coverage.

For now I assume it is not this patch's responsibility to address it.

clang/test/SemaCXX/libstdcxx_is_pod_hack.cpp
1

This is a hack for presumably older language dialects. Switched to a range.

MaskRay marked 5 inline comments as done.Aug 29 2022, 8:19 PM
MaskRay added inline comments.
llvm/utils/lit/lit/llvm/config.py
574

Agree. This can be left as another patch.

583
aaron.ballman accepted this revision.Aug 30 2022, 4:39 AM

This LGTM but I'd appreciate it if @dblaikie could also give it a once over so we have more than one sets of eyes on the changes in case I've missed something.

clang/test/CodeGenCXX/exception-spec-decay.cpp
1

Since C++98 is actually uncommon now. I prefer explicit %stdcxx_98- to indicate a test works with C++98.

Heheh, there's still a *ton* of C++98 code out there, but I'm okay being explicit just the same (it makes the tests easier to read).

clang/test/CodeGenCXX/override-layout.cpp
1–14

Ah, that's fine to do in a follow-up then. Thanks!

clang/test/Layout/ms-x86-vtordisp.cpp
1–3

Agreed, thanks for the explanation!

This revision is now accepted and ready to land.Aug 30 2022, 4:39 AM
MaskRay updated this revision to Diff 457367.Sep 1 2022, 1:00 PM
MaskRay marked 2 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Fix a test

This revision was landed with ongoing or failed builds.Sep 3 2022, 10:29 PM
This revision was automatically updated to reflect the committed changes.