From 9411957410813aeefc1bb299abcb856ce1ef8aae Mon Sep 17 00:00:00 2001 From: Bjorn Pettersson Date: Tue, 19 Sep 2017 13:10:30 +0000 Subject: [PATCH] [Sema] Disallow assigning record lvalues with nested const-qualified fields. Summary: According to C99 6.3.2.1p1, structs and unions with nested const-qualified fields (that is, const-qualified fields declared at some recursive level of the aggregate) are not modifiable lvalues. However, Clang permits assignments of these lvalues. With this patch, we both prohibit the assignment of records with const-qualified fields and emit a best-effort diagnostic. This fixes https://bugs.llvm.org/show_bug.cgi?id=31796 . Committing on behalf of bevinh (Bevin Hansson). Reviewers: rtrieu, rsmith, bjope Reviewed By: bjope Subscribers: Ka-Ka, rogfer01, bjope, fhahn, cfe-commits Differential Revision: https://reviews.llvm.org/D37254 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@313628 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Expr.h | 2 + include/clang/AST/Type.h | 7 +- include/clang/Basic/DiagnosticSemaKinds.td | 5 +- lib/AST/ExprClassification.cpp | 3 +- lib/AST/Type.cpp | 13 ++++ lib/Sema/SemaExpr.cpp | 84 +++++++++++++++++++--- test/Sema/assign.c | 43 +++++++++++ 7 files changed, 141 insertions(+), 16 deletions(-) diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index aa0eab9678..0a8edd6ddc 100644 --- a/include/clang/AST/Expr.h +++ b/include/clang/AST/Expr.h @@ -275,6 +275,7 @@ public: MLV_LValueCast, // Specialized form of MLV_InvalidExpression. MLV_IncompleteType, MLV_ConstQualified, + MLV_ConstQualifiedField, MLV_ConstAddrSpace, MLV_ArrayType, MLV_NoSetterProperty, @@ -324,6 +325,7 @@ public: CM_LValueCast, // Same as CM_RValue, but indicates GCC cast-as-lvalue ext CM_NoSetterProperty,// Implicit assignment to ObjC property without setter CM_ConstQualified, + CM_ConstQualifiedField, CM_ConstAddrSpace, CM_ArrayType, CM_IncompleteType diff --git a/include/clang/AST/Type.h b/include/clang/AST/Type.h index 2fe070c9af..eaf599511e 100644 --- a/include/clang/AST/Type.h +++ b/include/clang/AST/Type.h @@ -3791,10 +3791,9 @@ public: return reinterpret_cast(TagType::getDecl()); } - // FIXME: This predicate is a helper to QualType/Type. It needs to - // recursively check all fields for const-ness. If any field is declared - // const, it needs to return false. - bool hasConstFields() const { return false; } + /// Recursively check all fields in the record for const-ness. If any field + /// is declared const, return true. Otherwise, return false. + bool hasConstFields() const; bool isSugared() const { return false; } QualType desugar() const { return QualType(this, 0); } diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index dd628877f8..44a223d1c6 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -5888,6 +5888,8 @@ def err_typecheck_assign_const : Error< "cannot assign to %select{non-|}1static data member %2 " "with const-qualified type %3|" "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|" "read-only variable is not assignable}0">; def note_typecheck_assign_const : Note< @@ -5895,7 +5897,8 @@ def note_typecheck_assign_const : Note< "function %1 which returns const-qualified type %2 declared here|" "variable %1 declared const here|" "%select{non-|}1static data member %2 declared const here|" - "member function %q1 is declared const here}0">; + "member function %q1 is declared const here|" + "%select{|nested }1data member %2 declared const here}0">; def warn_mixed_sign_comparison : Warning< "comparison of integers of different signs: %0 and %1">, diff --git a/lib/AST/ExprClassification.cpp b/lib/AST/ExprClassification.cpp index d149bdd0cd..3bb2b4eb5f 100644 --- a/lib/AST/ExprClassification.cpp +++ b/lib/AST/ExprClassification.cpp @@ -641,7 +641,7 @@ static Cl::ModifiableType IsModifiable(ASTContext &Ctx, const Expr *E, // Records with any const fields (recursively) are not modifiable. if (const RecordType *R = CT->getAs()) if (R->hasConstFields()) - return Cl::CM_ConstQualified; + return Cl::CM_ConstQualifiedField; return Cl::CM_Modifiable; } @@ -695,6 +695,7 @@ Expr::isModifiableLvalue(ASTContext &Ctx, SourceLocation *Loc) const { llvm_unreachable("CM_LValueCast and CL_LValue don't match"); case Cl::CM_NoSetterProperty: return MLV_NoSetterProperty; case Cl::CM_ConstQualified: return MLV_ConstQualified; + case Cl::CM_ConstQualifiedField: return MLV_ConstQualifiedField; case Cl::CM_ConstAddrSpace: return MLV_ConstAddrSpace; case Cl::CM_ArrayType: return MLV_ArrayType; case Cl::CM_IncompleteType: return MLV_IncompleteType; diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp index 8533cf24c3..bf49dfd8de 100644 --- a/lib/AST/Type.cpp +++ b/lib/AST/Type.cpp @@ -2994,6 +2994,19 @@ bool TagType::isBeingDefined() const { return getDecl()->isBeingDefined(); } +bool RecordType::hasConstFields() const { + for (FieldDecl *FD : getDecl()->fields()) { + QualType FieldTy = FD->getType(); + if (FieldTy.isConstQualified()) + return true; + FieldTy = FieldTy.getCanonicalType(); + if (const RecordType *FieldRecTy = FieldTy->getAs()) + if (FieldRecTy->hasConstFields()) + return true; + } + return false; +} + bool AttributedType::isQualifier() const { switch (getAttrKind()) { // These are type qualifiers in the traditional C sense: they annotate diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 7fd9cd48b0..55fbd9ca57 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -10260,22 +10260,23 @@ static bool IsTypeModifiable(QualType Ty, bool IsDereference) { return !Ty.isConstQualified(); } +// Update err_typecheck_assign_const and note_typecheck_assign_const +// when this enum is changed. +enum { + ConstFunction, + ConstVariable, + ConstMember, + ConstMethod, + NestedConstMember, + ConstUnknown, // Keep as last element +}; + /// Emit the "read-only variable not assignable" error and print notes to give /// more information about why the variable is not assignable, such as pointing /// to the declaration of a const variable, showing that a method is const, or /// that the function is returning a const reference. static void DiagnoseConstAssignment(Sema &S, const Expr *E, SourceLocation Loc) { - // Update err_typecheck_assign_const and note_typecheck_assign_const - // when this enum is changed. - enum { - ConstFunction, - ConstVariable, - ConstMember, - ConstMethod, - ConstUnknown, // Keep as last element - }; - SourceRange ExprRange = E->getSourceRange(); // Only emit one error on the first const found. All other consts will emit @@ -10385,6 +10386,66 @@ static void DiagnoseConstAssignment(Sema &S, const Expr *E, S.Diag(Loc, diag::err_typecheck_assign_const) << ExprRange << ConstUnknown; } +enum OriginalExprKind { + OEK_Variable, + OEK_Member, + OEK_LValue +}; + +static void DiagnoseRecursiveConstFields(Sema &S, const ValueDecl *VD, + const RecordType *Ty, + SourceLocation Loc, SourceRange Range, + OriginalExprKind OEK, + bool &DiagnosticEmitted, + bool IsNested = false) { + // We walk the record hierarchy breadth-first to ensure that we print + // diagnostics in field nesting order. + // First, check every field for constness. + for (const FieldDecl *Field : Ty->getDecl()->fields()) { + if (Field->getType().isConstQualified()) { + if (!DiagnosticEmitted) { + S.Diag(Loc, diag::err_typecheck_assign_const) + << Range << NestedConstMember << OEK << VD + << IsNested << Field; + DiagnosticEmitted = true; + } + S.Diag(Field->getLocation(), diag::note_typecheck_assign_const) + << NestedConstMember << IsNested << Field + << Field->getType() << Field->getSourceRange(); + } + } + // Then, recurse. + for (const FieldDecl *Field : Ty->getDecl()->fields()) { + QualType FTy = Field->getType(); + if (const RecordType *FieldRecTy = FTy->getAs()) + DiagnoseRecursiveConstFields(S, VD, FieldRecTy, Loc, Range, + OEK, DiagnosticEmitted, true); + } +} + +/// Emit an error for the case where a record we are trying to assign to has a +/// const-qualified field somewhere in its hierarchy. +static void DiagnoseRecursiveConstFields(Sema &S, const Expr *E, + SourceLocation Loc) { + QualType Ty = E->getType(); + assert(Ty->isRecordType() && "lvalue was not record?"); + SourceRange Range = E->getSourceRange(); + const RecordType *RTy = Ty.getCanonicalType()->getAs(); + bool DiagEmitted = false; + + if (const MemberExpr *ME = dyn_cast(E)) + DiagnoseRecursiveConstFields(S, ME->getMemberDecl(), RTy, Loc, + Range, OEK_Member, DiagEmitted); + else if (const DeclRefExpr *DRE = dyn_cast(E)) + DiagnoseRecursiveConstFields(S, DRE->getDecl(), RTy, Loc, + Range, OEK_Variable, DiagEmitted); + else + DiagnoseRecursiveConstFields(S, nullptr, RTy, Loc, + Range, OEK_LValue, DiagEmitted); + if (!DiagEmitted) + DiagnoseConstAssignment(S, E, Loc); +} + /// CheckForModifiableLvalue - Verify that E is a modifiable lvalue. If not, /// emit an error and return true. If so, return false. static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) { @@ -10460,6 +10521,9 @@ static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) { case Expr::MLV_ConstAddrSpace: DiagnoseConstAssignment(S, E, Loc); return true; + case Expr::MLV_ConstQualifiedField: + DiagnoseRecursiveConstFields(S, E, Loc); + return true; case Expr::MLV_ArrayType: case Expr::MLV_ArrayTemporary: DiagID = diag::err_typecheck_array_not_modifiable_lvalue; diff --git a/test/Sema/assign.c b/test/Sema/assign.c index 1fff778644..5bdfa9a3a5 100644 --- a/test/Sema/assign.c +++ b/test/Sema/assign.c @@ -16,3 +16,46 @@ void test3() { b[4] = 1; // expected-error {{read-only variable is not assignable}} b2[4] = 1; // expected-error {{read-only variable is not assignable}} } + +typedef struct I { + const int a; // expected-note 4{{nested data member 'a' declared const here}} \ + expected-note 6{{data member 'a' declared const here}} +} I; +typedef struct J { + struct I i; +} J; +typedef struct K { + struct J *j; +} K; + +void testI(struct I i1, struct I i2) { + i1 = i2; // expected-error {{cannot assign to variable 'i1' with const-qualified data member 'a'}} +} +void testJ1(struct J j1, struct J j2) { + j1 = j2; // expected-error {{cannot assign to variable 'j1' with nested const-qualified data member 'a'}} +} +void testJ2(struct J j, struct I i) { + j.i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}} +} +void testK1(struct K k, struct J j) { + *(k.j) = j; // expected-error {{cannot assign to lvalue with nested const-qualified data member 'a'}} +} +void testK2(struct K k, struct I i) { + k.j->i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}} +} + +void testI_(I i1, I i2) { + i1 = i2; // expected-error {{cannot assign to variable 'i1' with const-qualified data member 'a'}} +} +void testJ1_(J j1, J j2) { + j1 = j2; // expected-error {{cannot assign to variable 'j1' with nested const-qualified data member 'a'}} +} +void testJ2_(J j, I i) { + j.i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}} +} +void testK1_(K k, J j) { + *(k.j) = j; // expected-error {{cannot assign to lvalue with nested const-qualified data member 'a'}} +} +void testK2_(K k, I i) { + k.j->i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}} +} -- 2.40.0