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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
----------------
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.


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

https://reviews.llvm.org/D153969