[clang] Assertion compiling a varadic template when using C++20, but not C++17

[ormris]

When -std=c++20 is specified, clang fails to build a simple program that instantiates a varadic template due to an assertion. Compiling the same program with -std=c++17 works as expected.

Code:

struct base {
    int k{2};
};

struct A : virtual public base {
};

struct B : virtual public base {
};

template<typename...args>
struct X : public args... {
    using args::k ...;
};

void t(void) {
    X<A, B> x;
    x.A::k=1;
    return;
}

Output:

$ clang++ --version |& grep version
clang version 18.0.0 ([email protected]:llvm/llvm-project.git abacab6e0c30ff0f6327e7b4fbc044af0a195efe)
$ clang++ -c -std=c++20 test.cpp
clang++: /home/mvoss/llvm-project/llvm/include/llvm/ADT/SmallVector.h:298: llvm::SmallVectorTemplateCommon::const_reference llvm::SmallVectorTemplateCommon<clang::APValue::LValuePathEntry>::operator[](llvm::SmallVectorTemplateCommon::size_type) const [T = clang::APValue::LValuePathEntry]: Assertion `idx < size()' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: /home/mvoss/llvm-project/build/bin/clang++ -c -std=c++20 test.cpp
1.      test.cpp:24:5: current parser token 'return'
2.      test.cpp:20:20: parsing function body 't'
3.      test.cpp:20:20: in compound statement ('{}')
...
$ clang++ -c -std=c++17 test.cpp
$

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[shafik]

We are crashing in HandleUnionActiveMemberChange(...) here:

llvm-project/clang/lib/AST/ExprConstant.cpp

Lines 6121 to 6125 in eb81493

	--PathLength;
	(void)Elt;
	assert(declaresSameEntity(Elt->getType()->getAsCXXRecordDecl(),
	LHS.Designator.Entries[PathLength]
	.getAsBaseOrMember().getPointer()));

b/c PathLength is now 4294967295 b/c we have subtracted from it one too many times.

When we start LHS.Designator.Entries.size() gives us 2 but the AST looks like this:

MemberExpr 0x14a80a238 'int' lvalue .k 0x15a8c4ba8
|-NestedNameSpecifier TypeSpec 'struct A'
`-ImplicitCastExpr 0x14a80a208 'struct base' lvalue <UncheckedDerivedToBase (virtual base)>
  `-ImplicitCastExpr 0x14a80a1e8 'struct A' lvalue <UncheckedDerivedToBase (A)>
    `-DeclRefExpr 0x14a80a1c0 'X<A, B>':'struct X<struct A, struct B>' lvalue Var 0x14a809d88 'x' 'X<A, B>':'struct X<struct A, struct B>'

We subtract once for the MemberExpr case and twice for each ImplicitCastExpr case and boom.

CC @zygoloid since he wrote this code in 31c69a3 and there is bad assumption someplace but it is not obvious where it is.

[antoniofrighetto]

It seems like we are not taking into account the case in which have nested casts. After entering the outer ICE, we traverse the ICE backwards (namely, from base to X<A, B>), as shown here:

llvm-project/clang/lib/AST/ExprConstant.cpp

Lines 6119 to 6126 in eb81493

	// Walk path backwards as we walk up from the base to the derived class.
	for (const CXXBaseSpecifier *Elt : llvm::reverse(ICE->path())) {
	--PathLength;
	(void)Elt;
	assert(declaresSameEntity(Elt->getType()->getAsCXXRecordDecl(),
	LHS.Designator.Entries[PathLength]
	.getAsBaseOrMember().getPointer()));
	}

However, we are not actually stepping over the inner ICE.

@@ -6109,8 +6109,6 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
       --PathLength;
 
     } else if (auto *ICE = dyn_cast<ImplicitCastExpr>(E)) {
-      // Step over a derived-to-base conversion.
-      E = ICE->getSubExpr();
       if (ICE->getCastKind() == CK_NoOp)
         continue;
       if (ICE->getCastKind() != CK_DerivedToBase &&
@@ -6125,6 +6123,10 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
                                       .getAsBaseOrMember().getPointer()));
       }
 
+      // Step over a derived-to-base conversion.
+      if (isa<ImplicitCastExpr>(ICE->getSubExpr()))
+        E = cast<ImplicitCastExpr>(ICE->getSubExpr())->getSubExpr();
+
     //   -- Otherwise, S(E) is empty.
     } else {
       break;

@zygoloid, could this look like a proper fix? If so, would be happy to open a patch for it.
EDIT: Apologize, I messed a bit with the patches, this latest one should be the good one.

[zygoloid]

It sounds like the AST representation might be broken here? Shouldn't each ImplicitCastExpr in this case have a path of length 1? The inner one should be converting from X to A, and the outer one should be converting from A to base, but it sounds like what you're seeing is that they both contain the complete path from X to base.

[zygoloid]

Hm, I think the problem here is likely to be that we're not handling virtual base classes properly in this code at all. We can't just truncate the path on the LHS in this case; we'd need to find the derived type that was the source of the derived -> base conversion. And we don't have enough information to do that.

Fortunately, we don't actually need to do that at all, because a class with a virtual base class can never have a trivial default constructor, so if we see a conversion from derived to virtual base class, we're done finding entries in S(E) and can immediately stop. So:

       for (const CXXBaseSpecifier *Elt : llvm::reverse(ICE->path())) {
+        if (Elt->isVirtual()) {
+          // A class with virtual base classes never has a trivial default
+          // constructor, so S(E) is empty in this case.
+          E = nullptr;
+          break;
+        }
         --PathLength;
-        (void)Elt;
         assert(declaresSameEntity(Elt->getType()->getAsCXXRecordDecl(),
                                   LHS.Designator.Entries[PathLength]
                                       .getAsBaseOrMember().getPointer()));
       }

[antoniofrighetto]

Thanks for the answer! I see that now, indeed a default constructor is trivial if its class has no virtual base class, amongst others. Thus, no need for further derived-to-base analysis in this case, so it would make sense to bail out upon encountering a virtual base class. @zygoloid, do you want me to address this?

[shafik]

Thanks for the answer! I see that now, indeed a default constructor is trivial if its class has no virtual base class, amongst others. Thus, no need for further derived-to-base analysis in this case, so it would make sense to bail out upon encountering a virtual base class. @zygoloid, do you want me to address this?

I don't know if you realize you are creating duplicate responses or not.

I made the change to a local branch and tested them and they don't seem to break anything. If you would like this to be your first clang change feel free to assign it to yourself and see the github documents here: https://llvm.org/docs/GitHub.html#github-reviews

[antoniofrighetto]

Apologies for that, I didn't mean to create any duplicate responses here. I didn't draw any conclusions about @zygoloid being OK with that, so I preferred to ensure myself I was not stepping over anyone else, waiting for their reply. I had quickly tested the change this morning locally as well, and it looked OK to me – but if you have a ready branch, please go ahead.

[zygoloid]

@antoniofrighetto Please go ahead :)

[ormris]

/cherry-pick 660876a

[llvmbot]

/branch llvm/llvm-project-release-prs/issue65982

[llvmbot]

/pull-request llvm/llvm-project-release-prs#703

[ormris]

/cherry-pick 660876a c990d94

[ormris]

Re-doing the cherry-pick to fix failing test.

[llvmbot]

/branch llvm/llvm-project-release-prs/issue65982