[Clang] Fix variant crashes from GH58028, GH57370

[royjacobson]

Summary

Fixes a null dereference in some diagnostic issuing code.

Closes https://github.com/llvm/llvm-project/issues/57370
Closes https://github.com/llvm/llvm-project/issues/58028

[royjacobson]

Rebase on main (build was broken)

[shafik]

LFTM besides the comment about the test.

clang/test/SemaCXX/specialization-diagnose-crash.cpp

3

Shouldn't we see a diagnostic in order to exercise the code that is being fixed?

[royjacobson]

Slightly modify the test

[royjacobson]

clang/test/SemaCXX/specialization-diagnose-crash.cpp

3

Since this fixes an ICE I'm not sure we actually need to, but I modified it a bit so we have assurance that the specialization code runs.

[royjacobson]

Apparently some of the workers crashed with the test - https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't reproduce this locally. @shafik any idea why the diagnostics might change? :/

[erichkeane]

In D134885#3826335, @royjacobson wrote:

Apparently some of the workers crashed with the test - https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't reproduce this locally. @shafik any idea why the diagnostics might change? :/

I have no idea where that warning comes from. That shouldn't happen on ANY machine, since it is a public member (a is), not a private one (which should cause the diagnostic). Is that the only bot with this problem?

I don't know what the 'sie' build is... but it gets me wondering if that diagnostic is getting an uninitialized version of "VerifyOnly" somewhere? It seems to me that it should be suppressing that diagnostic (if that is what VerifyOnly means here?)?

It IS bizarre to me that this crash would be noticed by something that DIDN'T try to evaluate that diagnostic though, since the ICE is on that code path.

[royjacobson]

In D134885#3827383, @erichkeane wrote:

In D134885#3826335, @royjacobson wrote:

Apparently some of the workers crashed with the test - https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't reproduce this locally. @shafik any idea why the diagnostics might change? :/

I have no idea where that warning comes from. That shouldn't happen on ANY machine, since it is a public member (a is), not a private one (which should cause the diagnostic). Is that the only bot with this problem?

I don't know what the 'sie' build is... but it gets me wondering if that diagnostic is getting an uninitialized version of "VerifyOnly" somewhere? It seems to me that it should be suppressing that diagnostic (if that is what VerifyOnly means here?)?

It IS bizarre to me that this crash would be noticed by something that DIDN'T try to evaluate that diagnostic though, since the ICE is on that code path.

Hmmm, uninitialized access might explain why it's runner dependent. I'll try and see if I can run it through USan.

[erichkeane]

Only thing I can think of is the 'Aggregate' calculation for this diagnostic is going wrong somewhere. See SemaDeclcXX.cpp ~6759 for where this all happens.

Aggregate IS initialized correctly to 'true' in CXXRecordDecl's DefinitionData as far as I can tell. BUT I can't imagine that some sort of optimization or mis-initialization of aggregate wouldn't cause some significant problems elsewhere. I'm thoroughly confused.

[shafik]

In D134885#3826335, @royjacobson wrote:

Apparently some of the workers crashed with the test - https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't reproduce this locally. @shafik any idea why the diagnostics might change? :/

I can't see what is going on here either. It looks like this is a windows PS5 bot. If you have access to a windows machine it might be worth trying a windows release w/ asserts build to see if it reproduces there.

[royjacobson]

In D134885#3827479, @shafik wrote:

In D134885#3826335, @royjacobson wrote:

Apparently some of the workers crashed with the test - https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't reproduce this locally. @shafik any idea why the diagnostics might change? :/

I can't see what is going on here either. It looks like this is a windows PS5 bot. If you have access to a windows machine it might be worth trying a windows release w/ asserts build to see if it reproduces there.

Ahhh, that exaplains it. The PlayStation targets didn't switch their default standard version to C++17, and using --std=c++14 will indeed reproduce the warnings. Some C++17 aggregate initialization changes, I guess. Thank you!

[royjacobson]

Lock the test to standard version

[erichkeane]

In D134885#3827535, @royjacobson wrote:

In D134885#3827479, @shafik wrote:

In D134885#3826335, @royjacobson wrote:

Apparently some of the workers crashed with the test - https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't reproduce this locally. @shafik any idea why the diagnostics might change? :/

I can't see what is going on here either. It looks like this is a windows PS5 bot. If you have access to a windows machine it might be worth trying a windows release w/ asserts build to see if it reproduces there.

Ahhh, that exaplains it. The PlayStation targets didn't switch their default standard version to C++17, and using --std=c++14 will indeed reproduce the warnings. Some C++17 aggregate initialization changes, I guess. Thank you!

Hah, wow! Good catch! I'd not known about the PS not changing defaults, but I DID have the DeclCXX.cpp 'setBases' function up on my screen that set that aggregate and was like, "huh... this is interesting, but I don't see how that applies here".

clang/lib/Sema/SemaInit.cpp

if (Init >= NumInits || !ILE->getInit(Init)) {
 
     if (Field->getType()->isReferenceType()) {
       if (!VerifyOnly) {
         // C++ [dcl.init.aggr]p9:
         //   If an incomplete or empty initializer-list leaves a
         //   member of reference type uninitialized, the program is
         //   ill-formed.
         SemaRef.Diag(Loc, diag::err_init_reference_member_uninitialized)
             << Field->getType()
-          << ILE->getSyntacticForm()->getSourceRange();
-        SemaRef.Diag(Field->getLocation(),
-                     diag::note_uninit_reference_member);
+            << (ILE->isSyntacticForm() ? ILE : ILE->getSyntacticForm())
+                   ->getSourceRange();
+        SemaRef.Diag(Field->getLocation(), diag::note_uninit_reference_member);
       }
       hadError = true;
       return;
     }
 
     ExprResult MemberInit = PerformEmptyInit(Loc, MemberEntity);
     if (MemberInit.isInvalid()) {
       hadError = true;

clang/test/SemaCXX/specialization-diagnose-crash.cpp

+// RUN: %clang_cc1 -fsyntax-only %s --std=c++17 -verify
+// This is a reduction of GH57370 and GH58028, originally appearing
+// in libstdc++'s variant code.
+
+struct V1 {};
+struct V2 : V1 {
+  int &a;
+};
+
+template <class T> using void_t = void;
+
+template <class T> struct X { T x; };
+
+template <class T1, class T2, class = void> struct Variant {
+  Variant() = delete; // expected-note {{deleted here}}
+};
+
+template <class T1, class T2>
+struct Variant<T1, T2, void_t<decltype(X<T2>{T1()})>> {};
+
+void f() {
+  Variant<V1, V1>();
+  Variant<V1, V2>(); // expected-error {{call to deleted constructor}}
+}