[clang][AST] Bail out when handling union access with virtual inheritance

[antoniofrighetto]

An assertion issue that arose when handling union member access with virtual base class has been addressed. As pointed out by @zygoloid, there is no need for further derived-to-base analysis in this instance, so we can bail out upon encountering a virtual base class.

As per doc-comment in HandleUnionActiveMemberChange, it turns out we might not be handling a union, so minor refinement on the function name as well. No problem in undoing this, if any though.

Fixes: #65982.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

An assertion issue that arose when handling union member access with virtual base class has been addressed. As pointed out by @zygoloid, there is no need for further derived-to-base analysis in this instance, so we can bail out upon encountering a virtual base class.

As per doc-comment in HandleUnionActiveMemberChange, it turns out we might not be handling a union, so minor refinement on the function name as well. No problem in undoing this, if any though.

Fixes: #65982.

Full diff: https://github.com/llvm/llvm-project/pull/66243.diff

2 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+12-5)
• (added) clang/test/SemaCXX/cxx2a-virtual-base-used.cpp (+11)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index dfa48e9c030b6a3..fea06b97259fe31 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -6062,8 +6062,9 @@ const AccessKinds StartLifetimeOfUnionMemberHandler::AccessKind;
 /// operator whose left-hand side might involve a union member access. If it
 /// does, implicitly start the lifetime of any accessed union elements per
 /// C++20 [class.union]5.
-static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
-                                          const LValue &LHS) {
+static bool MaybeHandleUnionActiveMemberChange(EvalInfo &Info,
+                                               const Expr *LHSExpr,
+                                               const LValue &LHS) {
   if (LHS.InvalidBase || LHS.Designator.Invalid)
     return false;
 
@@ -6118,8 +6119,14 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
         break;
       // Walk path backwards as we walk up from the base to the derived class.
       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()));
@@ -7806,7 +7813,7 @@ class ExprEvaluatorBase
         // per C++20 [class.union]5.
         if (Info.getLangOpts().CPlusPlus20 && OCE &&
             OCE->getOperator() == OO_Equal && MD->isTrivial() &&
-            !HandleUnionActiveMemberChange(Info, Args[0], ThisVal))
+            !MaybeHandleUnionActiveMemberChange(Info, Args[0], ThisVal))
           return false;
 
         Args = Args.slice(1);
@@ -8679,7 +8686,7 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {
     return false;
 
   if (Info.getLangOpts().CPlusPlus20 &&
-      !HandleUnionActiveMemberChange(Info, E->getLHS(), Result))
+      !MaybeHandleUnionActiveMemberChange(Info, E->getLHS(), Result))
     return false;
 
   return handleAssignment(this->Info, E, Result, E->getLHS()->getType(),
diff --git a/clang/test/SemaCXX/cxx2a-virtual-base-used.cpp b/clang/test/SemaCXX/cxx2a-virtual-base-used.cpp
new file mode 100644
index 000000000000000..196a3ab05564b96
--- /dev/null
+++ b/clang/test/SemaCXX/cxx2a-virtual-base-used.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++20 -verify=cxx20 -triple=x86_64-linux-gnu %s
+// Fixes assertion triggered by https://github.com/llvm/llvm-project/issues/65982
+
+struct A { int y; };
+struct B : virtual public A {};
+struct X : public B {};
+
+void member_with_virtual_inheritance() {
+  X x;
+  x.B::y = 1;
+}

[shafik]

This looks good to me but I would like @zygoloid to look at it as well.

[zygoloid]

LGTM

[antoniofrighetto]

Closing this as landed in 660876a. Thanks.