[clang][ExprConst] Fix crash on uninitialized array subobject

[hazohelet]

https://reviews.llvm.org/D146358 was assuming that all subobjects have
their own name (SubobjectDecl), but it was not true for array
elements.

Fixes #67317

[llvmbot]

@llvm/pr-subscribers-clang

Changes

https://reviews.llvm.org/D146358 was assuming that all subobjects have
their own name (SubobjectDecl), but it was not true for array
elements.

Fixes #67317

---

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

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticASTKinds.td (+1-1)
• (modified) clang/lib/AST/ExprConstant.cpp (+9-4)
• (modified) clang/lib/AST/Interp/Interp.cpp (+1-1)
• (modified) clang/test/SemaCXX/eval-crashes.cpp (+7)

diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index d2656310e79c9b8..71dabac2d32402a 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -69,7 +69,7 @@ def note_consteval_address_accessible : Note<
   "%select{pointer|reference}0 to a consteval declaration "
   "is not a constant expression">;
 def note_constexpr_uninitialized : Note<
-  "subobject %0 is not initialized">;
+  "subobject %select{of type |}0%1 is not initialized">;
 def note_constexpr_uninitialized_base : Note<
   "constructor of base class %0 is not called">;
 def note_constexpr_static_local : Note<
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f0d53d6ae18e804..acf4b33242c4a27 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2411,10 +2411,15 @@ static bool CheckEvaluationResult(CheckEvaluationResultKind CERK,
                                   const FieldDecl *SubobjectDecl,
                                   CheckedTemporaries &CheckedTemps) {
   if (!Value.hasValue()) {
-    assert(SubobjectDecl && "SubobjectDecl shall be non-null");
-    Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << SubobjectDecl;
-    Info.Note(SubobjectDecl->getLocation(),
-              diag::note_constexpr_subobject_declared_here);
+    if (SubobjectDecl) {
+      Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized)
+          << true << SubobjectDecl;
+      Info.Note(SubobjectDecl->getLocation(),
+                diag::note_constexpr_subobject_declared_here);
+    } else {
+      // FIXME: We should add a test to check the output of this case.
+      Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << false << Type;
+    }
     return false;
   }
 
diff --git a/clang/lib/AST/Interp/Interp.cpp b/clang/lib/AST/Interp/Interp.cpp
index e1951574edb6288..0e7e9bac1c8079a 100644
--- a/clang/lib/AST/Interp/Interp.cpp
+++ b/clang/lib/AST/Interp/Interp.cpp
@@ -404,7 +404,7 @@ bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD) {
 static void DiagnoseUninitializedSubobject(InterpState &S, const SourceInfo &SI,
                                            const FieldDecl *SubObjDecl) {
   assert(SubObjDecl && "Subobject declaration does not exist");
-  S.FFDiag(SI, diag::note_constexpr_uninitialized) << SubObjDecl;
+  S.FFDiag(SI, diag::note_constexpr_uninitialized) << true << SubObjDecl;
   S.Note(SubObjDecl->getLocation(),
          diag::note_constexpr_subobject_declared_here);
 }
diff --git a/clang/test/SemaCXX/eval-crashes.cpp b/clang/test/SemaCXX/eval-crashes.cpp
index 3e59ad31c559da8..ac04b113f99b7aa 100644
--- a/clang/test/SemaCXX/eval-crashes.cpp
+++ b/clang/test/SemaCXX/eval-crashes.cpp
@@ -54,3 +54,10 @@ namespace pr33140_10 {
   int a(const int &n = 0);
   bool b() { return a() == a(); }
 }
+
+namespace GH67317 {
+struct array {
+  int (&data)[2];
+  array() : data(*new int[1][2]) {}
+};
+}

[tbaederr]

Unfortunate, but the changes look good to me. Since the crash was introduced in clang 17, this should also be backported.

[shafik]

I am a bit concerned we don't fully understand how to recover the old diagnostic. I left a comment that I think may help but I think we need to understand this a bit better before committing to a fix.

[shafik]

So if we look back at https://reviews.llvm.org/D146358 you used to get the location which could be invalid and the diagnostic that was based on that is now lost b/c we lost that information b/c in many cases you just pass nullptr explicitly. So Perhaps you should have been encoding that information as well and then understand why that is important here.

[hazohelet]

I'm not worrying about SubobjectLoc that was used for subobject declared here note. https://reviews.llvm.org/D146358 introduced the explicit nullptr args, but it only happens on calls where invalid source location was explicitly passed before that patch. (There was one exception about base classes but it was fixed in https://reviews.llvm.org/D153969. Despite passing null SubobjectDecl there, the passed APValue is known to be isStruct(), so its subobjects do not inherit the nullness of SubobjectDecl)
So, we always had an invalid source location where we now have nullSubobjectDecl; thus no information being lost.

[hazohelet]

Ping

[AaronBallman]

I think we should add that test case now as this is a new code path we previously didn't have.

[hazohelet]

Finally I found a test case that can check the fallback diagnostic message: https://godbolt.org/z/Mc3nPq15n
Clang 16's output is a little weird in that it does not mention new *char[3][1] part, and this patch's fallback approach inherits this behavior.

[cor3ntin]

Can you add a comment before true ? and maybe use an integer instead << /* of type */ 1 << - same in other places

[hazohelet]

Ping

[AaronBallman]

LGTM

[tbaederr]

Thanks. Don't forget to trigger a backport to clang 17

[hazohelet]

Thanks for the heads up. I'll request backporting in a few hours when buildbots finish running tests.