aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, nickdesaulniers, erichkeane, jyknight,
clang-language-wg.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added a project: clang.
We were accepting invalid code where the qualifiers of the anonymous structure
were not taken into account when forming a member access to one of the indirect
fields, as in:
struct S {
const struct {
int i;
};
} s;
void foo(void) {
s.i = 12; // previously accepted, now rejected
}
We now expose the anonymous structure's field as having a qualified type
instead of an unqualified type so that access checking notes that an
intermediate object in the expression is qualified. We also adjusted the
diagnostic so that it's slightly more user friendly despite this being a rather
rare case.
Note, this only impacts C; in C++ and with the Microsoft C anonymous structure
extension, the qualifiers are stripped.
Fixes #48099
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D125167
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/anonymous-struct-union.c
clang/test/Sema/anonymous-struct-union.cpp
Index: clang/test/Sema/anonymous-struct-union.cpp
===================================================================
--- /dev/null
+++ clang/test/Sema/anonymous-struct-union.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions -Wno-microsoft-anon-tag -x c %s
+// expected-no-diagnostics
+
+struct trivial {
+ int a;
+};
+
+struct GH48099 {
+ const struct {
+ int i;
+ };
+#ifndef __cplusplus
+ // This extension is only allowed in C.
+ const struct trivial;
+#endif
+};
+
+// In C++, qualifiers are ignored on the anonymous objects.
+void GH48099_test(void) {
+ struct GH48099 x;
+
+#ifdef __cplusplus
+ // Test that the member access ignores the qualifiers. This would fail in C.
+ x.i = 12; // ok
+#else
+ // Test that the member access ignores the qualifiers for a Microsoft C
+ // anonymous structure declaration. This would fail in C++ because the
+ // declaration in GH48099 does not declare anything (so 'a' isn't lifted into
+ // the outer structure).
+ x.a = 12; // ok
+#endif
+}
+
Index: clang/test/Sema/anonymous-struct-union.c
===================================================================
--- clang/test/Sema/anonymous-struct-union.c
+++ clang/test/Sema/anonymous-struct-union.c
@@ -119,3 +119,26 @@
struct s3 s;
s.A = 1; // expected-warning {{'A' is deprecated}}
}
+
+struct GH48099 {
+ const struct { // expected-note {{anonymous struct declared const here}}
+ int i;
+ };
+ volatile union {
+ int j;
+ };
+};
+
+// Ensure that qualifiers from the anonymous object are reflected on the
+// fields being hoisted into the outer context.
+void GH48099_test(void) {
+ struct GH48099 x;
+
+ // It's the access path that picks up the qualifiers, not the direct
+ // declaration of the field itself. So 'i' and 'j' are both 'int'.
+ _Static_assert(_Generic(x.i, int : 1, default : 0), "i is not int?");
+ _Static_assert(_Generic(x.j, int : 1, default : 0), "j is not int?");
+
+ // Test that the member access picks up the qualifiers.
+ x.i = 12; // expected-error {{cannot assign to non-static data member 'i' with const-qualified type 'const int'}}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13351,6 +13351,7 @@
ConstMember,
ConstMethod,
NestedConstMember,
+ ConstAnonymousObject, // Only used as a note
ConstUnknown, // Keep as last element
};
@@ -13370,6 +13371,10 @@
// next checked expression is the result of a dereference.
bool IsDereference = false;
bool NextIsDereference = false;
+ // Used when diagnosing indirect fields that are const because the anonymous
+ // object is const rather than the field. This allows us to diagnose the
+ // field rather than the anonymous object.
+ const FieldDecl *LastNamedField = nullptr;
// Loop to process MemberExpr chains.
while (true) {
@@ -13388,16 +13393,44 @@
if (!IsTypeModifiable(Field->getType(), IsDereference)) {
if (!DiagnosticEmitted) {
- S.Diag(Loc, diag::err_typecheck_assign_const)
- << ExprRange << ConstMember << false /*static*/ << Field
- << Field->getType();
+ // If the field is an anonymous union or structure, we want to
+ // diagnose the last named field rather than the unnamed object.
+ // This requires us to combine the qualifiers from the anonymous
+ // object with those of the field in order for the diagnostic to
+ // make sense.
+ if (Field->isAnonymousStructOrUnion()) {
+ assert(LastNamedField &&
+ "we should have seen a named field by this point");
+ S.Diag(Loc, diag::err_typecheck_assign_const)
+ << ExprRange << ConstMember
+ << false /*static*/ << LastNamedField
+ << LastNamedField->getType().withFastQualifiers(
+ Field->getType().getLocalFastQualifiers());
+ } else {
+ S.Diag(Loc, diag::err_typecheck_assign_const)
+ << ExprRange << ConstMember << false /*static*/ << Field
+ << Field->getType();
+ }
DiagnosticEmitted = true;
}
- S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
- << ConstMember << false /*static*/ << Field << Field->getType()
- << Field->getSourceRange();
+ // If the field is an anonymous union or structure, we want the note
+ // to be about the anonymous object being const rather than the
+ // indirect field.
+ if (Field->isAnonymousStructOrUnion()) {
+ S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
+ << ConstAnonymousObject
+ << Field->getType()
+ ->castAs<RecordType>()
+ ->getDecl()
+ ->isStruct();
+ } else {
+ S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
+ << ConstMember << false /*static*/ << Field << Field->getType()
+ << Field->getSourceRange();
+ }
}
E = ME->getBase();
+ LastNamedField = Field;
continue;
} else if (const VarDecl *VDecl = dyn_cast<VarDecl>(VD)) {
if (VDecl->getType().isConstQualified()) {
Index: clang/lib/Sema/SemaDecl.cpp
===================================================================
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -5498,7 +5498,10 @@
if (RecordDecl *OwningClass = dyn_cast<RecordDecl>(Owner)) {
Anon = FieldDecl::Create(
Context, OwningClass, DS.getBeginLoc(), Record->getLocation(),
- /*IdentifierInfo=*/nullptr, Context.getTypeDeclType(Record), TInfo,
+ /*IdentifierInfo=*/nullptr,
+ Context.getTypeDeclType(Record).withFastQualifiers(
+ DS.getTypeQualifiers()),
+ TInfo,
/*BitWidth=*/nullptr, /*Mutable=*/false,
/*InitStyle=*/ICIS_NoInit);
Anon->setAccess(AS);
@@ -5520,7 +5523,9 @@
assert(DS.getAttributes().empty() && "No attribute expected");
Anon = VarDecl::Create(Context, Owner, DS.getBeginLoc(),
Record->getLocation(), /*IdentifierInfo=*/nullptr,
- Context.getTypeDeclType(Record), TInfo, SC);
+ Context.getTypeDeclType(Record).withFastQualifiers(
+ DS.getTypeQualifiers()),
+ TInfo, SC);
// Default-initialize the implicit variable. This initialization will be
// trivial in almost all cases, except if a union member has an in-class
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7010,6 +7010,7 @@
"cannot assign to non-static data member within const member function %1|"
"cannot assign to %select{variable %2|non-static data member %2|lvalue}1 "
"with %select{|nested }3const-qualified data member %4|"
+ "|" // Unused for anonymous objects
"read-only variable is not assignable}0">;
def note_typecheck_assign_const : Note<
@@ -7018,7 +7019,8 @@
"variable %1 declared const here|"
"%select{non-|}1static data member %2 declared const here|"
"member function %q1 is declared const here|"
- "%select{|nested }1data member %2 declared const here}0">;
+ "%select{|nested }1data member %2 declared const here|"
+ "anonymous %select{union|struct}1 declared const here}0">;
def warn_unsigned_always_true_comparison : Warning<
"result of comparison of %select{%3|unsigned expression}0 %2 "
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -149,6 +149,12 @@
because there is no way to fully qualify the enumerator name, so this
"extension" was unintentional and useless. This fixes
`Issue 42372 <https://github.com/llvm/llvm-project/issues/42372>`_.
+- When forming a member expression, now consider any qualifiers written on an
+ anonymous structure or union as also applying to the field being referenced.
+ This fixes an issue where qualifiers were being ignored, allowing you to
+ assign to a ``const`` field. Note that qualifiers are ignored in C++ and for
+ Microsoft's extension of anonymous objects. This fixes
+ `Issues 48099 <https://github.com/llvm/llvm-project/issues/48099>`_.
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
