aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM! Did you end up requesting commit access? If not, now is probably a good time: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access ================ 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); + } ---------------- hazohelet wrote: > 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. > 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. Correct. > 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 Agreed, that's a bug, good catch and thank you for the fix! > 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. That's the approach I would try initially as well -- unfortunately, precommit CI is currently broken due to configuration issues, so we might only learn about issue post-commit. 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
