hazohelet added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+ if (SubobjectDecl) {
+ Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) <<
SubobjectDecl;
+ Info.Note(SubobjectDecl->getLocation(),
+ diag::note_constexpr_subobject_declared_here);
+ }
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > hazohelet wrote:
> > > aaron.ballman wrote:
> > > > Hmm, this breaks one of the contracts of our constexpr evaluation
> > > > engine, doesn't it? My understanding is that if constexpr evaluation
> > > > fails, we will have emitted a note diagnostic for why it failed. But if
> > > > the caller doesn't pass a nonnull `SubobjectDecl`, we'll return `false`
> > > > but we won't issue a diagnostic.
> > > >
> > > > I'm surprised no tests lost notes as a result of this change, that
> > > > suggests we're missing test coverage for the cases where nullptr is
> > > > passed in explicitly to this function.
> > > Yeah, I was looking into when `SubobjectDecl` can be null here. I
> > > `assert`ed the non-nullness of `SubobjectDecl` before and found that
> > > there exists two lines of code
> > > (https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
> > > and
> > > https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
> > > in the test codes that nulls here.
> > > It seems they are doing the same thing, doing comparison against a
> > > pointer to a `[[gnu::weak]]` member. A simple reproducing code is here:
> > > https://godbolt.org/z/qn997n85n
> > > As you can see from the compiler explorer, there's no note emitted here
> > > before the patch.
> > > I inserted some `printf` into the code before this patch and confirmed
> > > `Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true <<
> > > Type` was actually called when compiling the reproducing code and that
> > > somehow it is ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.
> > > It seems they are doing the same thing, doing comparison against a
> > > pointer to a [[gnu::weak]] member. A simple reproducing code is here:
> > > https://godbolt.org/z/qn997n85n
> > > As you can see from the compiler explorer, there's no note emitted here
> > > before the patch.
> >
> > I see a note generated there:
> > ```
> > <source>:4:41: note: comparison against pointer to weak member
> > 'Weak::weak_method' can only be performed at runtime
> > constexpr bool val = &Weak::weak_method != nullptr;
> > ^
> > ```
> > The situations I'm concerned about are the changes to ExprConstant.cpp:2270
> > or line 2399 and so on.
> The `FFDiag` call can just stay as it was, right? And then only the
> `Info.Note` call needs to be conditional for whether we have a
> `SubobjectDecl`.
In my understanding, the concern is that there could be note loss when
`Value.hasValue()` evaluates to `false` and at the same time `SubobjectDecl` is
`nullptr` explicitly passed in this change.
`[[gnu::weak]]` member pointer comparison is the only example of this happening
in the clang test codes where `Subobject` is `nullptr` passed at
ExprConstant.cpp:2454 in `CheckFullyInitialized`. In this case the
"uninitialized subobject" note was not displayed before this patch as well
because there is another note issued elsewhere. Thus, the note loss does not
happen.
BTW, I checked where the `[[gnu::weak]]` note is issued and I think there might
be a mistake in the return statements. The `return true` should be `return
false` because the constexpr evaluator fails to evaluate the expression, right?
Correct me if I am wrong. Here's the reference:
https://github.com/llvm/llvm-project/blob/e58a49300e757ff61142f6abd227bd1437c1cf87/clang/lib/AST/ExprConstant.cpp#L13140-L13151
If we change them to `false`, `CheckFullyInitialized` will not be called in
this scenario, and we will have no test cases where `SubobjectDecl` is
`nullptr` and at the same time `Value.hasValue()` is `false`. I am unsure
whether this is due to the lack of test coverage or it is guaranteed that this
condition does not hold here.
> The `FFDiag` call can just stay as it was, right? And then only the
> `Info.Note` call needs to be conditional for whether we have a
> `SubobjectDecl`.
If `SubobjectDecl` is null, it would cause segfault when the `FFDiag` note is
displayed, I think.
The best way I can think of would be to `assert(SubobjectDecl)` here, but I am
still struggling and would like to ask for comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146358/new/
https://reviews.llvm.org/D146358
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
