[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 ↗

(On Diff #438612)

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 ↗

(On Diff #438612)

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/docs/ReleaseNotes.rst

- Clang will allow calling a ``consteval`` function in a default argument. This
   fixes `Issue 48230 <https://github.com/llvm/llvm-project/issues/48230>`_.
 - Fixed memory leak due to ``VarTemplateSpecializationDecl`` using
   ``TemplateArgumentListInfo`` instead of ``ASTTemplateArgumentListInfo``.
 - An initializer for a static variable declaration, which is nested
   inside a statement expression in an aggregate initializer, is now
   emitted as a dynamic initializer. Previously the variable would
   incorrectly be zero-initialized. In contexts where a dynamic
   initializer is not allowed this is now diagnosed as an error.
+- Fixed an assertion failure when evaluating a constexpr constructor
+  in an array initializer.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/lib/AST/ExprConstant.cpp

if (Result.hasArrayFiller())
       Result.getArrayFiller() = Filler;
   }
 
   LValue Subobject = This;
   Subobject.addArray(Info, E, CAT);
   for (unsigned Index = 0; Index != NumEltsToInit; ++Index) {
     const Expr *Init =
         Index < E->getNumInits() ? E->getInit(Index) : FillerExpr;
-    if (!EvaluateInPlace(Result.getArrayInitializedElt(Index),
-                         Info, Subobject, Init) ||
+    if (Result.isArray() &&
+        (!EvaluateInPlace(Result.getArrayInitializedElt(Index), Info, Subobject,
+                          Init) ||
          !HandleLValueArrayAdjustment(Info, Init, Subobject,
-                                     CAT->getElementType(), 1)) {
+                                      CAT->getElementType(), 1))) {
       if (!Info.noteFailure())
         return false;
       Success = false;
     }
   }
 
-  if (!Result.hasArrayFiller())
+  if (!Result.isArray() || !Result.hasArrayFiller())
     return Success;
 
   // If we get here, we have a trivial filler, which we can just evaluate
   // once and splat over the rest of the array elements.
   assert(FillerExpr && "no array filler for incomplete init list");
   return EvaluateInPlace(Result.getArrayFiller(), Info, Subobject,
                          FillerExpr) && Success;
 }

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

+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+// FIXME: This code should compile without errors but as of right now doesn't.
+//   We're only testing that clang doesn't crash or run into an assertion.
+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}}
+    {{}},
+};