[clang][bytecode] Assert on virtual func call from array elem

[mariusdr]

Fixes #152893.

An assert was raised when a constexpr virtual function was called from an constexpr array element with -fexperimental-new-constant-interpreter set.

[llvmbot]

@llvm/pr-subscribers-clang

Author: marius doerner (mariusdr)

Changes

Fixes #152893.

An assert was raised when a constexpr virtual function was called from an constexpr array element with -fexperimental-new-constant-interpreter set.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ByteCode/Interp.cpp (+7-2)
• (modified) clang/test/AST/ByteCode/cxx20.cpp (+22)

diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index f1b9104c04feb..c739fa0c19d84 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -1664,10 +1664,15 @@ bool CallVirt(InterpState &S, CodePtr OpPC, const Function *Func,
       TypePtr = TypePtr.getBase();
 
     QualType DynamicType = TypePtr.getType();
-    if (DynamicType->isPointerType() || DynamicType->isReferenceType())
+    if (DynamicType->isPointerType() || DynamicType->isReferenceType()) {
       DynamicDecl = DynamicType->getPointeeCXXRecordDecl();
-    else
+    } else if (DynamicType->isArrayType()) {
+      const Type *ElemType = DynamicType->getPointeeOrArrayElementType();
+      assert(ElemType);
+      DynamicDecl = ElemType->getAsCXXRecordDecl();
+    } else {
       DynamicDecl = DynamicType->getAsCXXRecordDecl();
+    }
   }
   assert(DynamicDecl);
 
diff --git a/clang/test/AST/ByteCode/cxx20.cpp b/clang/test/AST/ByteCode/cxx20.cpp
index 67bf9a732d8b7..8e77a081e3e60 100644
--- a/clang/test/AST/ByteCode/cxx20.cpp
+++ b/clang/test/AST/ByteCode/cxx20.cpp
@@ -1100,3 +1100,25 @@ namespace DiscardedTrivialCXXConstructExpr {
   constexpr int y = foo(12); // both-error {{must be initialized by a constant expression}} \
                              // both-note {{in call to}}
 }
+
+namespace VirtualFunctionCallThroughArrayElem {
+  struct X {
+    constexpr virtual int foo() const {
+      return 3;
+    }
+  };
+  constexpr X xs[5];
+  static_assert(xs[3].foo() == 3);
+
+  constexpr X xs2[1][2];
+  static_assert(xs2[0].foo() == 3); // both-error {{is not a structure or union}}
+  static_assert(xs2[0][0].foo() == 3);
+
+  struct Y: public X {
+    constexpr int foo() const override {
+      return 1;
+    }
+  };
+  constexpr Y ys[20];
+  static_assert(ys[12].foo() == static_cast<const X&>(ys[12]).foo());
+}

[tbaederr]

can you include the original reproducer from #152893 as well? This patch fixes the assertion failure but the diagnostic output is still different between the two interpreters.

[mariusdr]

Added that test case and the diagnostic. For normal globals the interpreters also disagreed in their diagnostics outputs (https://godbolt.org/z/84zeeYxd3), now they raise the same messages.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[tbaederr]

So what is actually the relevant part of CheckConstant? Is it just the call to QualType::isConstant()?

[mariusdr]

Checking only DynamicType.isConstant() would reject valid code like

  struct A {
    constexpr virtual void foo(int &a) {
      a = 1;
    }
  };

  constexpr int m() {
    A b;
    int a;
    b.foo(a);
    return a;
  }
  static_assert(m() == 1, "");

here TypePtr.isStatic() in GetDynamicDecl() is false so CheckConstant() would return early but I'm not sure if it is sufficient to only check those two things. Tests pass at least with if (TypePtr.isStatic() && !DynamicType.isConstant(S.getASTContext())) instead of CheckConstant().

[mariusdr]

So what is actually the relevant part of CheckConstant? Is it just the call to QualType::isConstant()?

Removed the call to CheckConstant, added more test cases. Check should be more explicit now, i.e. check storage class of this, and if its var decl is constexpr.

[tbaederr]

Did Pointer::toDiagnosticString() not work here?

[mariusdr]

The output is slightly different because toDiagnosticString() (~ APValue::getAsString(V, TypePtr.getType()) adds a '&' prefix to the output while the code above does not, because the input QualType is not a reference type, refer to:

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

Lines 819 to 821 in 6a7aec1

	if (!IsReference)
	Out << '&';
	else if (isLValueOnePastTheEnd())

we would get messages like

note: virtual function called on object '&k' whose dynamic type is not constant

while the other interpreter outputs

note: virtual function called on object 'k' whose dynamic type is not constant

[tbaederr]

okay, thanks!

[mariusdr]

Thanks for the review! Could you merge this for me (I can't commit). Thank you.