[PATCH] D153969: [clang][ExprConstant] Fix crash on uninitialized base class subobject

Tue Aug 1 07:42:25 PDT 2023

hazohelet added inline comments.

================
Comment at: clang/lib/AST/ExprConstant.cpp:2422
+              << BS.getType();
+          Info.Note(BS.getBeginLoc(), diag::note_constexpr_base_inherited_here);
+          return false;
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > hazohelet wrote:
> > > > aaron.ballman wrote:
> > > > > hazohelet wrote:
> > > > > > hazohelet wrote:
> > > > > > > tbaeder wrote:
> > > > > > > > Can you pass `<< BS.getSourceRange()` here? Does that improve things?
> > > > > > > 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)
> > > > > > 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.
> > > > > 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?
> > > > 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.
> > > 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.
> > 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/
> Oops, I munged that link. I meant this one: https://godbolt.org/z/qrMchvh1f
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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153969/new/

https://reviews.llvm.org/D153969