[PATCH] D131464: [test] Make tests pass regardless of gnu++14/gnu++17 default

Thu Aug 11 08:16:28 PDT 2022

aaron.ballman added a comment.

In D131464#3713705 <https://reviews.llvm.org/D131464#3713705>, @MaskRay wrote:

> 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 at -2 {{ISO C++17 does not allow 'register' storage class specifier}}
>   #elif __cplusplus >= 201103L
>   // expected-warning at -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 at -1 {{passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'operator+' may result in an unaligned pointer access}}
>                                          // precxx17-warning at -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).

================
Comment at: clang/test/AST/sourceranges.cpp:1-2
-// RUN: %clang_cc1 -triple i686-mingw32 -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -triple i686-mingw32 -ast-dump %s | FileCheck %s
 // RUN: %clang_cc1 -triple i686-mingw32 -std=c++1z -ast-dump %s | FileCheck %s -check-prefix=CHECK-1Z

----------------
dblaikie wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > 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.)
> > `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.
> > 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)
> 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.

================
Comment at: llvm/utils/lit/lit/llvm/config.py:570
+        clang_std_values = ('98', '11', '14', '17', '20', '2b')
+        def add_stdcxx(s):
+            t = s[8:]
----------------
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).

================
Comment at: llvm/utils/lit/lit/llvm/config.py:573
+            if t.endswith('-'):
+                t += '2b'
+            l = clang_std_values.index(t[0:2])
----------------
Maybe we can use `clang_std_values[-1]` so we don't have to hardcode this?

================
Comment at: llvm/utils/lit/lit/llvm/config.py:579
+            l = h - clang_std_group % (h-l+1)
+            self.config.substitutions.append((s, '-std=c++' + clang_std_values[l]))
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131464/new/

https://reviews.llvm.org/D131464