[clang] Avoid an assertion in APValue::hasArrayFiller()

[tbaeder]

Summary

When calling hasArrayFiller() on a APValue that's not an array, the two functions called inside hasArrayFiller() would both run into their isArray() assertions.

This triggered for me with the included test case in ExprConstant.cpp:

if (!Result.hasArrayFiller())
  return Success;

adding a !Result.isArray() || there would also work of course but I think the version in this patch is safer.

[tbaeder]

Failing tests are just because the test file is not formatted it seems.

[aaron.ballman]

clang/include/clang/AST/APValue.h

511–512

I think this makes the interface somewhat self-inconsistent. hasLValuePath() asserts on isLValue(), and getArraySize() was providing a similar assertion (on isArray()) when called from hasArrayFiller().

I take that to mean that the expectation for this interface is that the caller validates the APValue in the cases where it doesn't already know the answer.

[shafik]

Are you sure it is failing on the line you indicated? After looking at the code, a little before that line we have:

Result = APValue(APValue::UninitArray(), NumEltsToInit, NumElts);

which should assure that we have an array at this point. Looking at the result of the code prior to this they evaluate the check for Result.isArray() and so we should be consistent.

[tbaeder]

If you apply e.g.

@@ -10739,6 +10779,7 @@ bool ArrayExprEvaluator::VisitInitListExpr(const InitListExpr *E,
                           << NumEltsToInit << ".\n");

   Result = APValue(APValue::UninitArray(), NumEltsToInit, NumElts);
+  llvm::errs() << "Result " << &Result << ": " << Result.isArray() << "\n";

   // If the array was previously zero-initialized, preserve the
   // zero-initialized values.
@@ -10764,6 +10805,7 @@ bool ArrayExprEvaluator::VisitInitListExpr(const InitListExpr *E,
     }
   }

+  llvm::errs() << "Result " << &Result << ": " << Result.isArray() << "\n";
   if (!Result.hasArrayFiller())
     return Success;

and run the test case, the first added line prints 1 and the second one 0. Result is being mutated when doing the in-place evaluation.

I take that to mean that the expectation for this interface is that the caller validates the APValue in the cases where it doesn't already know the answer.

Sure, that was the other option.

[shafik]

and run the test case, the first added line prints 1 and the second one 0. Result is being mutated when doing the in-place evaluation.

I did not catch that. That is unfortunate, I am wondering now if we need a Result.isArray() && before the EvaluateInPlace in that loop.

[shafik]

clang/include/clang/AST/APValue.h

511–512

I agree with Aaron here, the correct place to fix this is in VisitInitListExpr and check Result.isArray().

[tbaeder]

I added the isArray() check in both places now, but the first one is not necessary for the test case (at least).

[aaron.ballman]

Basically LGTM -- can you also add a release note about the assertion fix?

clang/test/SemaCXX/constexpr-array-init.cpp

2

Can remove spurious newline

16

Same here.

Also, I'd appreciate a FIXME comment about what this is testing (it's not obvious from the context) and that this code is expected to compile without a diagnostic in the future (so it's a bit more obviously correct when we do fix that).

[shafik]

LGTM after addressing all of Aaron's comments.

[tbaeder]

In D128248#3622775, @shafik wrote:

LGTM after addressing all of Aaron's comments.

I've already addressed them; did you miss the update or are you saying I haven't addressed them properly?

[tbaeder]

Abandoning as per the discussion in https://discourse.llvm.org/t/apvalue-lifetime-problem-when-evaluating-constant-expressions/64002/5. This patch just hides a problem.

clang/include/clang/AST/APValue.h

APValue &getArrayInitializedElt(unsigned I) {
     assert(isArray() && "Invalid accessor");
     assert(I < getArrayInitializedElts() && "Index out of range");
     return ((Arr *)(char *)&Data)->Elts[I];
   }
   const APValue &getArrayInitializedElt(unsigned I) const {
     return const_cast<APValue*>(this)->getArrayInitializedElt(I);
   }
   bool hasArrayFiller() const {
+    if (!isArray())
+      return false;
     return getArrayInitializedElts() != getArraySize();
   }
   APValue &getArrayFiller() {
     assert(isArray() && "Invalid accessor");
     assert(hasArrayFiller() && "No array filler");
     return ((Arr *)(char *)&Data)->Elts[getArrayInitializedElts()];
   }
   const APValue &getArrayFiller() const {

clang/test/SemaCXX/constexpr-array-init.cpp

+
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+struct Foo {
+  int a; // expected-note {{subobject declared here}}
+  constexpr Foo()
+      : a(get_int()) {
+  }
+
+  constexpr int get_int() {
+    return 5;
+  }
+};
+
+
+static constexpr Foo bar[2][1] = { // expected-error {{constexpr variable 'bar' must be initialized by a constant expression}} \
+                                   // expected-note {{subobject of type 'int' is not initialized}}
+    {{}},
+};