This patch improves the diagnostic on uninitialized subobjects in constexpr variables by modifying the diagnostic message to display the subobject's name instead of its type.
Details
Diff Detail
Event Timeline
clang/lib/AST/Interp/Interp.cpp | ||
---|---|---|
373 | If we are sure it can never be nullptr then we should assert(SubObjDecl) |
"subobject named 'foo'" sounds a bit weird to me, I'd expect just "subobject 'foo'", but that's just a suggestion and I'll wait for a native spearker to chime in on this.
clang/lib/AST/Interp/Interp.cpp | ||
---|---|---|
373 | Should never be null here. |
My expert brain likes subobject of type 'T', but it's very wordy, and potentially not useful additional info. I'm partial to subobject 'T' due to it being the thing we're more likely to say in conversation (e.g. "that int isn't initialised"). I've also put out a mini-survey on the #include <C++> Discord server, and will update in a few hours.
Hi, @cjdb thanks for your feedback and the mini-survey.
The objective of this patch is to print the subobject's name instead of its type when it is not initialized in constexpr variable initializations. Thus, I think it would be appropriate to add some more options to the survey like "subobject named 'foo' is not initialized" and "subobject 'foo' is not initialized" when the code looks like the following:
template <typename T> struct F { T foo; constexpr F(){} }; constexpr F <int>f;
@simideveloper Thanks for your interest in collaborating on this issue, and sorry if I discouraged you from contributing by working on the same problem.
I assume this patch is in the its final stage, awaiting feedback on wording. However, there's more room for improvement around the uninitialized subobject diagnostics of clang, and your experience working on this issue could be helpful in resolving those issues.
One example of further improvement:
struct Type1 { int val; }; // note emitted struct Derived { Type1 val; // we could note on this as well constexpr Derived(){} }; constexpr Derived D;
Link: https://godbolt.org/z/v3rhz88aM
I am open to future collaboration opportunities as well. Happy hacking!
Thank you for working on this! The changes should also come with a release note in clang/docs/ReleaseNotes.rst to note the improvement.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2356–2360 | 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. |
@aaron.ballman
Thanks for the review! I'll add a release note.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2356–2360 | Yeah, I was looking into when SubobjectDecl can be null here. I asserted 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. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2356–2360 |
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. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2356–2360 | 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. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2356–2360 | 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. 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: 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.
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. |
So, if I understand the code correctly, we call CheckEvaluationResult with SubObjectDecl=nullptr when we're not checking an actual field but just an array/record, so we can't run into this problem anyway, so the assert seems fine.
I don't fully understand the problem with the gnu::weak stuff, in any case, adding it to this patch seems wrong. https://godbolt.org/z/qn997n85n does emit a note, but it's not the one this patch is about, so is that even relevant?
Thanks. I opened another differential for the gnu::weak stuff (D148419).
If my understanding is correct, Info.FFDiag displays only the note of its first call and discard the notes after it. So, the uninitialized-note is not displayed.
You can confirm this by deleting only the FFDiag call (https://github.com/llvm/llvm-project/blob/6f7e5c0f1ac6cc3349a2e1479ac4208465b272c6/clang/lib/AST/ExprConstant.cpp#L13143-L13144) from the clang trunk and compile my godbolt code to see the note (note: subobject of type 'const bool' is not initialized)
Although the return true says the [[gnu::weak]] member pointer comparison is valid, constexpr engine considers the constant initialization has failed if there is note emitted during the initializer evaluation(https://github.com/llvm/llvm-project/blob/6f7e5c0f1ac6cc3349a2e1479ac4208465b272c6/clang/lib/AST/Decl.cpp#L2591-L2592) and it's why [[gnu::weak]] comparison note is actually emitted in spite of the wrong signal about the validness of the expression.
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
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2356–2360 |
Correct.
Agreed, that's a bug, good catch and thank you for the fix!
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. |
Our downstream discovered that this causes a regression when compiling utility with ast-dump. I've put up a minimal reproducer: https://godbolt.org/z/oaezas7Ws
@hazohelet : Will you be able to look into this soon?
Thanks for the report and the reproducer.
I cannot take sufficient time for about 36 hours, but I'll be able to investigate it after that.
That is long enough away that I unfortunately have to revert (see 34e49d3e85a6dd03856af0fb4b7f8d8ae1f4f06a). Please note that in the LLVM project reverts are common/frequent (we have a policy of 'revert often'), and should not be taken negatively! Please feel free to re-submit this patch with a fix/the test provided, and we'll re-review promptly!
Thanks for the revert!
It seems that the problem is around the list initializer in template variable definition.
Minimum reproducible example:
template <typename T> constexpr T foo{};
When the TextNodeDumper enabled through -ast-dump visits a constexpr VarDecl with initializer, it evaluates the VarDecl and prints the value if evaluation comes successful.
(https://github.com/llvm/llvm-project/blob/55903151a2a505284ce3fcd955b1d394df0b55ea/clang/lib/AST/TextNodeDumper.cpp#L1825)
The VarDecl::evaluateValue() does not show the notes generated during its failed evaluation. The uninitialized-subobject note is actually generated BEFORE this patch when compiling the reproducers with -ast-dump flag on, but is not shown to the user.
The message should not be generated here and I assume this is an internal bug that we have to fix before resubmitting this patch.
It seems to me that perhaps the assert you added is not appropriate? Instead, could we have the OLD diagnostic 'back' as an alternate form (when this is empty)?
If we use the old diagnostic, subobject of type 'const T' is not initialized will be generated internally when clang dumps AST of the reproducer template<typename T> constexpr T foo{};. This is not what we want, right? I want to keep the assert here because it seems to be a nice bug catcher so far.
I did some more investigation and now I am sure that TextNodeDumper should not evaluate the initializer of constexpr-qualified VarDecl if it is an variable template definition(in the code I provided as permalink to GitHub). This approach fixes the assertion failure in the regression.
I opened another differential D151033 for that change, and I think it would be safe to reland this patch once the other is landed.
The cause of the regression has been fixed in the other patch.
This is a resubmission with rebase to the upstream.
LGTM, feel free to land again. I believe the failing precommit CI test is unrelated to the changes here, but please keep an eye on the post-commit bots just in case.
To conform to clang-tidy bugprone-argument-comment format