[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

2379

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

2422

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

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2422

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

2422

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

378–379

clang/lib/AST/ExprConstant.cpp

2422

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

2422

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

2422

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

2422

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

2422

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

[hazohelet]

clang/lib/AST/ExprConstant.cpp

2422

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

2422

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

 - Clang now diagnoses unexpected tokens after a
   ``#pragma clang|GCC diagnostic push|pop`` directive.
   (`#13920: <https://github.com/llvm/llvm-project/issues/13920>`_)
 - Clang now does not try to analyze cast validity on variables with dependent alignment (`#63007: <https://github.com/llvm/llvm-project/issues/63007>`_).
 - Clang constexpr evaluator now displays member function calls more precisely
   by making use of the syntactical structure of function calls. This avoids display
   of syntactically invalid codes in diagnostics.
   (`#57081: <https://github.com/llvm/llvm-project/issues/57081>`_)
+- Clang contexpr evaluator now displays notes as well as an error when a constructor
+  of base class is not called in the constructor of its derived class.
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix segfault while running clang-rename on a non existing file.
   (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.

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_base_inherited_here : Note<
+  "base class inherited 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<
   "floating point arithmetic produces %select{an infinity|a NaN}0">;
 def note_constexpr_dynamic_rounding : Note<
   "cannot evaluate this expression if rounding mode is dynamic">;
 def note_constexpr_float_arithmetic_strict : 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()) {
+          Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized_base)
+              << BS.getType();
+          Info.Note(BS.getBeginLoc(), diag::note_constexpr_base_inherited_here);
+          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/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{{base class inherited here}}
+  constexpr Foo() {}; // expected-error {{call to deleted constructor of 'DelBase'}}
+};
+constexpr Foo f; // expected-error {{must be initialized by a constant expression}} \
+                 // expected-note {{constructor of base class 'DelBase' is not called}}
+struct Bar : Foo {
+  constexpr Bar() {};
+};
+constexpr Bar bar; // expected-error {{must be initialized by a constant expression}} \
+                   // expected-note {{constructor of base class 'DelBase' is not called}}
+
+struct Base {};
+struct A : Base { // expected-note {{base class inherited here}}
+  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}} \
+               // expected-note {{constructor of base class 'Base' is not called}}
+
+struct B : Base { // expected-note {{base class inherited here}}
+  constexpr B() : {} // expected-error {{expected class member or base class name}}
+};
+
+constexpr B b; // expected-error {{must be initialized by a constant expression}} \
+               // expected-note {{constructor of base class 'Base' is not called}}
+} // namespace baseclass_uninit
+