[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/docs/ReleaseNotes.rst:382-383
   on overload resolution, when the actual reason for the failure is loss of other qualifiers.
+- 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.
 
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:2422
+              << BS.getType();
+          Info.Note(BS.getBeginLoc(), diag::note_constexpr_base_inherited_here);
+          return false;
----------------
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?


================
Comment at: clang/test/SemaCXX/constexpr-subobj-initialization.cpp:30
+} // namespace baseclass_uninit
+
----------------
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) {}
}
```


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

https://reviews.llvm.org/D153969