-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang] Fix false positive -Wmissing-field-initializer for anonymous unions #70829
Conversation
…unions Normally warning is not reported when a field has default initializer. Do so for anonymous unions with default initializers as well. No release note since it is a regression in clang 18. Fixes llvm#70384
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesNormally warning is not reported when a field has default initializer. Do so for anonymous unions with default initializers as well. No release note since it is a regression in clang 18. Fixes #70384 Full diff: https://github.com/llvm/llvm-project/pull/70829.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index ec796def96ad3d8..881e67587e430e7 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -349,17 +349,13 @@ class InitListChecker {
bool SubobjectIsDesignatorContext, unsigned &Index,
InitListExpr *StructuredList,
unsigned &StructuredIndex);
- bool CheckDesignatedInitializer(const InitializedEntity &Entity,
- InitListExpr *IList, DesignatedInitExpr *DIE,
- unsigned DesigIdx,
- QualType &CurrentObjectType,
- RecordDecl::field_iterator *NextField,
- llvm::APSInt *NextElementIndex,
- unsigned &Index,
- InitListExpr *StructuredList,
- unsigned &StructuredIndex,
- bool FinishSubobjectInit,
- bool TopLevelObject);
+ bool CheckDesignatedInitializer(
+ const InitializedEntity &Entity, InitListExpr *IList,
+ DesignatedInitExpr *DIE, unsigned DesigIdx, QualType &CurrentObjectType,
+ RecordDecl::field_iterator *NextField, llvm::APSInt *NextElementIndex,
+ unsigned &Index, InitListExpr *StructuredList, unsigned &StructuredIndex,
+ bool FinishSubobjectInit, bool TopLevelObject,
+ llvm::SmallPtrSetImpl<FieldDecl *> *InitializedFields = nullptr);
InitListExpr *getStructuredSubobjectInit(InitListExpr *IList, unsigned Index,
QualType CurrentObjectType,
InitListExpr *StructuredList,
@@ -2248,7 +2244,8 @@ void InitListChecker::CheckStructUnionTypes(
// the next field that we'll be initializing.
bool DesignatedInitFailed = CheckDesignatedInitializer(
Entity, IList, DIE, 0, DeclType, &Field, nullptr, Index,
- StructuredList, StructuredIndex, true, TopLevelObject);
+ StructuredList, StructuredIndex, true, TopLevelObject,
+ &InitializedFields);
if (DesignatedInitFailed)
hadError = true;
@@ -2256,7 +2253,6 @@ void InitListChecker::CheckStructUnionTypes(
DesignatedInitExpr::Designator *D = DIE->getDesignator(0);
if (!VerifyOnly && D->isFieldDesignator()) {
FieldDecl *F = D->getFieldDecl();
- InitializedFields.insert(F);
if (!DesignatedInitFailed) {
QualType ET = SemaRef.Context.getBaseElementType(F->getType());
if (checkDestructorReference(ET, InitLoc, SemaRef)) {
@@ -2365,21 +2361,43 @@ void InitListChecker::CheckStructUnionTypes(
!RD->isUnion()) {
// It is possible we have one or more unnamed bitfields remaining.
// Find first (if any) named field and emit warning.
- for (RecordDecl::field_iterator it = HasDesignatedInit ? RD->field_begin()
- : Field,
- end = RD->field_end();
- it != end; ++it) {
- if (HasDesignatedInit && InitializedFields.count(*it))
- continue;
+ auto MissingFieldCheck = [&](const RecordDecl *Record,
+ RecordDecl::field_iterator StartField,
+ auto &&MissingFieldCheck) -> bool {
+ FieldDecl *FirstUninitialized = nullptr;
+ for (RecordDecl::field_iterator it = StartField,
+ end = Record->field_end();
+ it != end; ++it) {
+ bool AllSet = false;
+ if (it->isAnonymousStructOrUnion()) {
+ RecordDecl *RDAnon = it->getType()->getAsRecordDecl();
+ AllSet = MissingFieldCheck(RDAnon, RDAnon->field_begin(),
+ MissingFieldCheck);
+ }
+
+ if ((HasDesignatedInit && InitializedFields.count(*it)) ||
+ it->hasInClassInitializer() || AllSet) {
+ if (Record->isUnion())
+ return true;
+ continue;
+ }
- if (!it->isUnnamedBitfield() && !it->hasInClassInitializer() &&
- !it->getType()->isIncompleteArrayType()) {
+ if (!it->isUnnamedBitfield() &&
+ !it->getType()->isIncompleteArrayType() &&
+ !it->isAnonymousStructOrUnion() && !FirstUninitialized)
+ FirstUninitialized = *it;
+ }
+
+ if (FirstUninitialized) {
SemaRef.Diag(IList->getSourceRange().getEnd(),
diag::warn_missing_field_initializers)
- << *it;
- break;
+ << FirstUninitialized;
+ return false;
}
- }
+ return true;
+ };
+ MissingFieldCheck(RD, HasDesignatedInit ? RD->field_begin() : Field,
+ MissingFieldCheck);
}
// Check that any remaining fields can be value-initialized if we're not
@@ -2537,19 +2555,13 @@ class FieldInitializerValidatorCCC final : public CorrectionCandidateCallback {
/// actually be initialized.
///
/// @returns true if there was an error, false otherwise.
-bool
-InitListChecker::CheckDesignatedInitializer(const InitializedEntity &Entity,
- InitListExpr *IList,
- DesignatedInitExpr *DIE,
- unsigned DesigIdx,
- QualType &CurrentObjectType,
- RecordDecl::field_iterator *NextField,
- llvm::APSInt *NextElementIndex,
- unsigned &Index,
- InitListExpr *StructuredList,
- unsigned &StructuredIndex,
- bool FinishSubobjectInit,
- bool TopLevelObject) {
+bool InitListChecker::CheckDesignatedInitializer(
+ const InitializedEntity &Entity, InitListExpr *IList,
+ DesignatedInitExpr *DIE, unsigned DesigIdx, QualType &CurrentObjectType,
+ RecordDecl::field_iterator *NextField, llvm::APSInt *NextElementIndex,
+ unsigned &Index, InitListExpr *StructuredList, unsigned &StructuredIndex,
+ bool FinishSubobjectInit, bool TopLevelObject,
+ llvm::SmallPtrSetImpl<FieldDecl *> *InitializedFields) {
if (DesigIdx == DIE->size()) {
// C++20 designated initialization can result in direct-list-initialization
// of the designated subobject. This is the only way that we can end up
@@ -2853,8 +2865,11 @@ InitListChecker::CheckDesignatedInitializer(const InitializedEntity &Entity,
// Update the designator with the field declaration.
- if (!VerifyOnly)
+ if (!VerifyOnly) {
D->setFieldDecl(*Field);
+ if (InitializedFields)
+ InitializedFields->insert(*Field);
+ }
// Make sure that our non-designated initializer list has space
// for a subobject corresponding to this field.
@@ -2929,10 +2944,10 @@ InitListChecker::CheckDesignatedInitializer(const InitializedEntity &Entity,
InitializedEntity MemberEntity =
InitializedEntity::InitializeMember(*Field, &Entity);
- if (CheckDesignatedInitializer(MemberEntity, IList, DIE, DesigIdx + 1,
- FieldType, nullptr, nullptr, Index,
- StructuredList, newStructuredIndex,
- FinishSubobjectInit, false))
+ if (CheckDesignatedInitializer(
+ MemberEntity, IList, DIE, DesigIdx + 1, FieldType, nullptr,
+ nullptr, Index, StructuredList, newStructuredIndex,
+ FinishSubobjectInit, false, InitializedFields))
return true;
}
diff --git a/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp b/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
index 510ace58c35a6aa..87bc01a51d2f297 100644
--- a/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ b/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -4,7 +4,7 @@
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,reorder -Wno-c99-designator -Werror=reorder-init-list -Wno-initializer-overrides
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,override -Wno-c99-designator -Wno-reorder-init-list -Werror=initializer-overrides
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
-// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
+// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides -D NON_PEDANTIC
namespace class_with_ctor {
@@ -247,3 +247,67 @@ void foo() {
//
}
}
+
+namespace GH70384 {
+
+struct A {
+ int m;
+ union { int a; float n = 0; };
+};
+
+struct B {
+ int m;
+ int b;
+ union { int a ; };
+};
+
+union CU {
+ int a = 1;
+ double b;
+};
+
+struct C {
+ int a;
+ union { int b; CU c;};
+};
+
+struct CC {
+ int a;
+ CU c;
+};
+
+void foo() {
+ A a = A{.m = 0};
+ A aa = {0};
+ A aaa = {.a = 7}; // wmissing-warning {{missing field 'm' initializer}}
+ B b = {.m = 1, .b = 3 }; //wmissing-warning {{missing field 'a' initializer}}
+ B bb = {1}; // wmissing-warning {{missing field 'b' initializer}}
+ // wmissing-warning@-1 {{missing field 'a' initializer}}
+ C c = {.a = 1}; // wmissing-warning {{missing field 'b' initializer}}
+ CC cc = {.a = 1}; //// wmissing-warning {{missing field 'c' initializer}}
+}
+
+#if defined NON_PEDANTIC
+struct C1 {
+ int m;
+ union { float b; union {int n = 1; }; };
+};
+
+struct C2 {
+ int m;
+ struct { float b; int n = 1; };
+};
+
+struct C3 {
+ int m;
+ struct { float b = 1; union {int a;}; int n = 1; };
+};
+
+C1 c = C1{.m = 1};
+C1 cc = C1{.b = 1}; // wmissing-warning {{missing field 'm' initializer}}
+C2 c1 = C2{.m = 1}; // wmissing-warning {{missing field 'b' initializer}}
+C2 c22 = C2{.m = 1, .b = 1};
+C3 c2 = C3{.b = 1}; // wmissing-warning {{missing field 'a' initializer}}
+ // wmissing-warning@-1 {{missing field 'm' initializer}}
+#endif // NON_PEDANTIC
+}
|
Thanks for looking into this! I wonder if it would make sense to move the checking for this warning here, to the llvm-project/clang/lib/Sema/SemaInit.cpp Lines 717 to 734 in ac30780
That should remove the need to build and check the |
@@ -4,7 +4,7 @@ | |||
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,reorder -Wno-c99-designator -Werror=reorder-init-list -Wno-initializer-overrides | |||
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,override -Wno-c99-designator -Wno-reorder-init-list -Werror=initializer-overrides | |||
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides | |||
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides | |||
// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides -D NON_PEDANTIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo it's clearer to unconditionally compile the same code for each test case, and instead introduce another -verify
prefix for the diagnostics that aren't emitted by this invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
Thanks for the suggestion, I moved a warning. It turned out with a couple of questionable moments, but still looks simpler than the first version. |
clang/lib/Sema/SemaInit.cpp
Outdated
@@ -727,6 +729,44 @@ void InitListChecker::FillInEmptyInitForField(unsigned Init, FieldDecl *Field, | |||
if (hadError || VerifyOnly) { | |||
// Do nothing | |||
} else if (Init < NumInits) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it OK that we don't warn on the else
case here? Do we still warn for:
struct A { int x, y; };
A a = {.x = 1}; // missing `.y` initializer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we still warn for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how does that work? If this if
condition doesn't hold and we created an ImplicitValueInitExpr
, we wouldn't produce the warning.
Ah, I see what's happening: the if
condition on line 736 is always true, because since cb77930 we always resize the init list for a struct to be large enough for all fields, and our optimization to leave off trailing struct members was (accidentally, I think) removed. And we can see the optimization stops working between Clang 3.6 and Clang 3.7.
Given that we've been living without that optimization for 8 years and no-one seems to have noticed, I suppose that it's fine that we've lost it, but it looks like this function can be simplified a bit as a result. Half of the if
condition on line 674 is always false, and this if
condition is always true. Maybe consider changing the L674 condition to an assert(Init < NumInits)
and simplify this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe consider changing the L674 condition to an assert(Init < NumInits) and simplify this too?
I'm not sure I understand the suggestion. The condition on L674 looks like:
if (Init >= NumInits || !ILE->getInit(Init)) {
Even if I only remove Init >= NumInits
, build of check-clang starts asserting with
Assertion `Init < getNumInits() && "Initializer access out of range!"'
Removing !ILE->getInit(Init)
also doesn't seem logical to me since FillInEmptyInitForField
is called for all fields in a record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, right, we still call this with Init >= NumInits
when in VerifyOnly
mode (but only in that mode).
My concern here is that we're only warning in the Init < NumInits
case, which is in principle wrong -- we should warn regardless of whether Init < NumInits
. But in fact, the caller always resizes the InitListExpr
so that Init >= NumInits
whenever we're not in VerifyOnly
mode, so I think this is probably not actually wrong, but it still looks wrong.
To make it not look wrong, I think we should remove all the dead code here: instead of checking whether Init < NumInits
in non-VerifyOnly
mode, we should assert that Init < NumInits
, and delete the unreachable code that is trying to handle the impossible case: lines 684-685, 704-707, 772-778. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems Init < NumInits
is always true for non-VerifyOnly
mode only for structs. So, removing lines 684-685, 704-707, 772-778
breaks a number of tests with unions. The assertion on line 676 confirms:
if (Init >= NumInits || !ILE->getInit(Init)) {
if (const RecordType *RType = ILE->getType()->getAs<RecordType>())
if (!RType->getDecl()->isUnion())
assert((Init < NumInits || VerifyOnly) &&
"This ILE should have been expanded");
I can still put warning emission out of Init < NumInits
check though if that seems more correct.
clang/lib/Sema/SemaInit.cpp
Outdated
for (auto *F : RD->fields()) { | ||
if (F->isAnonymousStructOrUnion()) | ||
Uninitialized = CheckAnonMember(F, CheckAnonMember); | ||
else if (!F->isUnnamedBitfield() && | ||
!F->getType()->isIncompleteArrayType() && !Uninitialized && | ||
!F->hasInClassInitializer()) | ||
Uninitialized = F; | ||
|
||
if (RD->isUnion() && (F->hasInClassInitializer() || !Uninitialized)) | ||
return nullptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will not warn on
struct A {
int a;
struct {
int x;
struct { int y = 0; };
};
};
A a = A{.a = 0};
... because the Uninitialized
value for x
will get overwritten by the nullptr
returned from the struct containing y
. How about:
for (auto *F : RD->fields()) { | |
if (F->isAnonymousStructOrUnion()) | |
Uninitialized = CheckAnonMember(F, CheckAnonMember); | |
else if (!F->isUnnamedBitfield() && | |
!F->getType()->isIncompleteArrayType() && !Uninitialized && | |
!F->hasInClassInitializer()) | |
Uninitialized = F; | |
if (RD->isUnion() && (F->hasInClassInitializer() || !Uninitialized)) | |
return nullptr; | |
} | |
for (auto *F : RD->fields()) { | |
FieldDecl *UninitializedFieldInF; | |
if (F->isAnonymousStructOrUnion()) | |
UninitializedFieldInF = CheckAnonMember(F, CheckAnonMember); | |
else if (!F->isUnnamedBitfield() && | |
!F->getType()->isIncompleteArrayType() && | |
!F->hasInClassInitializer()) | |
UninitializedFieldInF = F; | |
if (RD->isUnion() && !UninitializedFieldInF) | |
return nullptr; | |
if (!Uninitialized) | |
Uninitialized = UninitializedFieldInF; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done. Thanks for the catch!
clang/lib/Sema/SemaInit.cpp
Outdated
for (const Stmt *Init : *IL) | ||
if (isa_and_nonnull<DesignatedInitExpr>(Init)) | ||
return true; | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use llvm::any_of ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
Ping. |
Ping. |
1 similar comment
Ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes here look reasonable. CC @zygoloid to make sure he agrees as there's still one potentially outstanding comment (if we don't hear back from Richard, I think we can address his concerns post-commit as it seems like a refactoring rather than a behavioral fix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please wait until sometime mid-next week to land in case Richard has concerns.
Hi @Fznamznon and others! It looks like this patch not only avoids a false positive, but it also causes warnigns on stuff it didn't warn on before.
Without the patch it's silent, but with the patch I get
so it warns on
and
I suppose this is not as expected? We see thousands of warnings like this with our downstream compiler. |
@mikaelholmen , thanks for the report. These warnings are kind of expected, there is no big difference between |
Sounds good. Thank you! |
Hi again @Fznamznon
We get
with this patch. EDIT: we saw 2870 -Wmissing-field-initializers warnings with the original version of the patch for one kind of downstream build. |
@mikaelholmen , I see. I'll revert both patches to unblock you and I'll think about the proper solution after the holidays. |
@Fznamznon : Thank you! |
Normally warning is not reported when a field has default initializer. Do so for anonymous unions with default initializers as well. No release note since it is a regression in clang 18.
Fixes #70384