[clang][ExprConstant] Fix crash on uninitialized base class subobject

[hazohelet]

Summary

This patch fixes the reported regression caused by D146358 through adding notes about an uninitialized base class when we diagnose uninitialized constructor.

This also changes the wording from the old one in order to make it clear that the uninitialized subobject is a base class and its constructor is not called.
Wording changes:
BEFORE: subobject of type 'Base' is not initialized
AFTER: constructor of base class 'Base' is not called

Fixes https://github.com/llvm/llvm-project/issues/63496

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2414

The reason I put the assertion here in the original patch was that there were not enough test cases to see what could cause null here, and I thought we could rethink later about this assertion's validness through crash reports like this. FWIW, I thought this assertion could be beneficial at that time because it had caught two internal bugs.

If reviewers do not agree with the assertion here anymore, we can fallback to the old diagnostic(subobject of type 'A' is not initialized) when we have invalid subobject, or revert the original patch.
Currently this patch is a partial fallback to the old diagnostic.

[tbaeder]

clang/lib/AST/ExprConstant.cpp

2457

Can you pass << BS.getSourceRange() here? Does that improve things?

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2457

Currently, DiagLoc points to the variable declaration and the BS.getSourceRange covers the line where the base class is inherited. This causes distant source range and thus unnecessarily many lines of snippet printing.
e.g.

struct Base {
  Base() = delete;
};
struct Derived : Base {
  constexpr Derived() {}
};
constexpr Derived dd;

Output:

source.cpp:7:19: error: constexpr variable 'dd' must be initialized by a constant expression
    7 | constexpr Derived dd;
      |                   ^~
source.cpp:7:19: note: constructor of base class 'Base' is not called
    7 | struct Derived : Base {
      |                  ~~~~
    8 |   constexpr Derived() {}
    9 | };
   10 | constexpr Derived dd;
      |                   ^
source.cpp:4:18: note: base class inherited here
    4 | struct Derived : Base {
      |                  ^

(The line numbers seem incorrect but is already reported in https://github.com/llvm/llvm-project/issues/63524)

I think we don't need two notes here because the error is already pointing to the variable declaration. Having something like the following would be succint.

source.cpp:7:19: error: constexpr variable 'dd' must be initialized by a constant expression
    7 | constexpr Derived dd;
      |                   ^~
source.cpp:4:18: note: constructor of base class 'Base' is not called
    4 | struct Derived : Base {
      |                  ^~~~

Providing source range would be beneficial because the inherited class often spans in a few lines (the code in the crashing report, for example)

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2457

Sorry, I was looking at the line above. The distant source range problem doesn't occur.

I tested another input

struct Base {
  Base() = delete;
  constexpr Base(int){}
};

struct Derived : Base {
  constexpr Derived() {}
  constexpr Derived(int n): Base(n) {}
};

constexpr Derived darr[3] = {1, Derived(), 3};

expecting that the DiagLoc points to the second initializer Derived(), but it pointed to the same location as the error, so I'm still in favor of the idea of having a single note here.

[hazohelet]

• Removed the base class inherited here redundant note
• Provided source range and added test for it

The provided source range is NOT directly calling CXXBaseSpecifier::getSourceRange so as not to cover the access specifiers like public, private or protected

[hazohelet]

Ping

[aaron.ballman]

clang/docs/ReleaseNotes.rst

123–124

clang/lib/AST/ExprConstant.cpp

2457

Erich's suggestion in https://github.com/llvm/llvm-project/issues/63496#issuecomment-1607415201 was to continue to evaluate the constructor because there may be further follow-on diagnostics that are relevant and not related to the base class subobject. I tend to agree -- is there a reason why you're not doing that here?

clang/test/SemaCXX/constexpr-subobj-initialization.cpp

31

I'd also like to see a test case with more interesting base class specifiers:

struct Foo {
  constexpr Foo();
};

struct Bar : protected Foo {
  int i;
  constexpr Bar() : i(12) {}
}

template <typename Ty>
struct Baz {
  constexpr Baz();
};

struct Quux : Baz<Foo>, private Bar {
  int i;
  constexpr Quux() : i(12) {}
}

[hazohelet]

Address comments from aaron.ballman

• Added more tests

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2457

My question (https://github.com/llvm/llvm-project/issues/63496#issuecomment-1607177233) was whether or not we should utilize constant evaluator even if the evaluated expression is a semantically-invalid constructor like the crashing case.
So in my understanding, Erich's suggestion was that we should continue utilizing the constant evaluator in these cases, and stopping the evaluator here at uninitialized base class subobject is something else.

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

2457

Our usual strategy is to continue compilation to try to find follow-on issues. For example: https://godbolt.org/z/qrMchvh1f -- even though the constructor declaration is not valid, we still go on to diagnose issues within the constructor body.

[hazohelet]

Ping

[hazohelet]

Ping

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

2457

This is the only outstanding discussion that I see left in the review, and it may be due to a misunderstanding. Am I correct that your changes cause us to not diagnose further issues in the constructor body because we're stopping here at the uninitialized base class subobject? e.g., do we still diagnose this https://godbolt.org/

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

2457

Oops, I munged that link. I meant this one: https://godbolt.org/z/qrMchvh1f

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2457

I don't think it's correct. This patch does not change clang's behavior for the code in that link.
This patch does not introduce any change in diagnostic behavior from clang 16 (the previous patch is NOT included) other than the wording. Clang16 was also stopping the interpreter here. Links below are clang 16 codes.

https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/clang/lib/AST/ExprConstant.cpp#L2395-L2399
Clang 16 calls CheckEvaluationResult here.
https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/clang/lib/AST/ExprConstant.cpp#L2355-L2361
And the CheckEvaluationResult called there immediately returns false if Value.hasValue() evaluates to false.

This patch only adds this Value.hasValue() check here before calling CheckEvaluationResult to emit more precise diagnostics and fix the regression. It does nothing more than that.

[aaron.ballman]

LGTM!

clang/lib/AST/ExprConstant.cpp

2457

Ah thank you for the explanation!

[aaron.ballman]

Because this is fixing a regression, it may be worth backporting it to the 17.x branch: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

clang/docs/ReleaseNotes.rst

 -------------------------
 
 Attribute Changes in Clang
 --------------------------
 
 Improvements to Clang's diagnostics
 -----------------------------------
 - Clang constexpr evaluator now prints template arguments when displaying
   template-specialization function calls.
+- Clang contexpr evaluator now displays notes as well as an error when a constructor
+  of a base class is not called in the constructor of its derived class.
 
 Bug Fixes in This Version
 -------------------------
 - Fixed an issue where a class template specialization whose declaration is
   instantiated in one module and whose definition is instantiated in another
   module may end up with members associated with the wrong declaration of the
   class, which can result in miscompiles in some cases.
 - Fix crash on use of a variadic overloaded operator.

clang/include/clang/Basic/DiagnosticASTKinds.td

 def note_constexpr_dynamic_alloc : Note<
   "%select{pointer|reference}0 to %select{|subobject of }1"
   "heap-allocated object is not a constant expression">;
 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">;
+def note_constexpr_uninitialized_base : Note<
+  "constructor of base class %0 is not called">;
 def note_constexpr_static_local : Note<
   "control flows through the definition of a %select{static|thread_local}0 variable">;
 def note_constexpr_subobject_declared_here : Note<
   "subobject declared here">;
 def note_constexpr_array_index : Note<"cannot refer to element %0 of "
   "%select{array of %2 element%plural{1:|:s}2|non-array object}1 "
   "in a constant expression">;
 def note_constexpr_float_arithmetic : Note<

clang/lib/AST/ExprConstant.cpp

 
 static bool CheckEvaluationResult(CheckEvaluationResultKind CERK,
                                   EvalInfo &Info, SourceLocation DiagLoc,
                                   QualType Type, const APValue &Value,
                                   ConstantExprKind Kind,
                                   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);
     return false;
   }
 
   // We allow _Atomic(T) to be initialized from anything that T can be
   // initialized from.
return CheckEvaluationResult(
         CERK, Info, DiagLoc, Value.getUnionField()->getType(),
         Value.getUnionValue(), Kind, Value.getUnionField(), CheckedTemps);
   }
   if (Value.isStruct()) {
     RecordDecl *RD = Type->castAs<RecordType>()->getDecl();
     if (const CXXRecordDecl *CD = dyn_cast<CXXRecordDecl>(RD)) {
       unsigned BaseIndex = 0;
       for (const CXXBaseSpecifier &BS : CD->bases()) {
-        if (!CheckEvaluationResult(CERK, Info, DiagLoc, BS.getType(),
-                                   Value.getStructBase(BaseIndex), Kind,
-                                   /*SubobjectDecl=*/nullptr, CheckedTemps))
+        const APValue &BaseValue = Value.getStructBase(BaseIndex);
+        if (!BaseValue.hasValue()) {
+          SourceLocation TypeBeginLoc = BS.getBaseTypeLoc();
+          Info.FFDiag(TypeBeginLoc, diag::note_constexpr_uninitialized_base)
+              << BS.getType() << SourceRange(TypeBeginLoc, BS.getEndLoc());
+          return false;
+        }
+        if (!CheckEvaluationResult(CERK, Info, DiagLoc, BS.getType(), BaseValue,
+                                   Kind, /*SubobjectDecl=*/nullptr,
+                                   CheckedTemps))
           return false;
         ++BaseIndex;
       }
     }
     for (const auto *I : RD->fields()) {
       if (I->isUnnamedBitfield())
         continue;
 

clang/test/Misc/constexpr-subobj-init-source-ranges.cpp

+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-print-source-range-info %s 2>&1 | FileCheck %s --strict-whitespace
+
+struct DelBase {
+  constexpr DelBase() = delete;
+};
+
+// CHECK:      :{[[@LINE+1]]:21-[[@LINE+1]]:28}
+struct Foo : public DelBase {
+  constexpr Foo() {};
+};
+constexpr Foo f;

clang/test/SemaCXX/constexpr-subobj-initialization.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+namespace baseclass_uninit {
+struct DelBase {
+  constexpr DelBase() = delete; // expected-note {{'DelBase' has been explicitly marked deleted here}}
+};
+
+struct Foo : DelBase {  // expected-note 2{{constructor of base class 'DelBase' is not called}}
+  constexpr Foo() {}; // expected-error {{call to deleted constructor of 'DelBase'}}
+};
+constexpr Foo f; // expected-error {{must be initialized by a constant expression}}
+struct Bar : Foo {
+  constexpr Bar() {};
+};
+constexpr Bar bar; // expected-error {{must be initialized by a constant expression}}
+
+struct Base {};
+struct A : Base { // expected-note {{constructor of base class 'Base' is not called}}
+  constexpr A() : value() {} // expected-error {{member initializer 'value' does not name a non-static data member or base class}}
+};
+
+constexpr A a; // expected-error {{must be initialized by a constant expression}}
+
+struct B : Base { // expected-note {{constructor of base class 'Base' is not called}}
+  constexpr B() : {} // expected-error {{expected class member or base class name}}
+};
+
+constexpr B b; // expected-error {{must be initialized by a constant expression}}
+} // namespace baseclass_uninit
+
+
+struct Foo {
+  constexpr Foo(); // expected-note 2{{declared here}}
+};
+
+constexpr Foo ff; // expected-error {{must be initialized by a constant expression}} \
+                  // expected-note {{undefined constructor 'Foo' cannot be used in a constant expression}}
+
+struct Bar : protected Foo {
+  int i;
+  constexpr Bar() : i(12) {} // expected-note {{undefined constructor 'Foo' cannot be used in a constant expression}}
+};
+
+constexpr Bar bb; // expected-error {{must be initialized by a constant expression}} \
+                  // expected-note {{in call to 'Bar()'}}
+
+template <typename Ty>
+struct Baz {
+  constexpr Baz(); // expected-note {{declared here}}
+};
+
+struct Quux : Baz<Foo>, private Bar {
+  int i;
+  constexpr Quux() : i(12) {} // expected-note {{undefined constructor 'Baz' cannot be used in a constant expression}}
+};
+
+constexpr Quux qx; // expected-error {{must be initialized by a constant expression}} \
+                   // expected-note {{in call to 'Quux()'}}