Clang permits assignment to vector/extvector elements in a const method

[avt77]

Summary

This patch should close Bug 35951.

[rjmccall]

lib/AST/ExprClassification.cpp

652

Is there a reason why the places that compute the type of these l-value expressions don't propagate qualiifers? This hardly seems restricted to 'const'.

[avt77]

lib/AST/ExprClassification.cpp

652

Do you mean ExtVectorElementExpr/ArraySubscriptExpr should retrun isConstQualified() == true by themselves (and other qualifiers as well)?

[rjmccall]

lib/AST/ExprClassification.cpp

652

In the sense that Sema should create them with a type that is const-qualified when the underlying vector l-value is const-qualified, yes.

[avt77]

In fact we have here another problem: it is not an attempt to assign to const variable but it is an attempt to use the field assingment inside const method: I'm working on it.

[rjmccall]

That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value.

[avt77]

In D42530#995227, @rjmccall wrote:

That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value.

I'm afraid it's wrong because from my point of view Clang generates wrong message for test OhNo3 (see err_typecheck_assign_const.cpp). It should not be "read-only variable is not assignable" because v[4] is not read-only variable. Any other non-const method is able to change this variable w/o any problems. The the same is for OhNo and OhNo2.

BTW, gcc generates

error: assignment of read-only location ‘*(const float*)(&((const OhNo2*)this)->OhNo2::v)’

and that's OK because we try to change the value through const-pointer-to-const. Am I right?

[rjmccall]

If you just want a better diagnostic, there's quite a bit of machinery already set up to give more specific errors about why such-and-such l-value isn't modifiable. There may even be vector-specific diagnostics for that already, just triggered from the wrong place in the code.

[avt77]

I re-implemented the patch. Now it works properly with const methods in CXX classes.

[rjmccall]

No, really, in CreateBuiltinArraySubscriptExpr and LookupMemberExpr, in the clauses where they handle vector types, you just need to propagate qualifiers from the base type like is done in BuildFieldReferenceExpr. There's even a FIXME about it in the former.

[avt77]

I propagated qualifiers accordingly to rjmccall's suggestion. But I changed the diagnostic: now it's more realistic from my point of view.

[rjmccall]

Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM.

lib/Sema/SemaExpr.cpp

4403

You shouldn't have to get the canonical type here.

test/Sema/assign.c

17

These are nice improvements, thanks. There's still room for getting ever better, but this is already nice progress.

[avt77]

Committed revision 324721.
BTW, could you review D42728: it's rather similar to this one and rather small as well.

[RKSimon]

@avt77 Close this now that rL324721 has landed?

[avt77]

Fixed by 324721.

lib/AST/ExprClassification.cpp

static Cl::ModifiableType IsModifiable(ASTContext &Ctx, const Expr *E,
   // Incomplete types are not modifiable.
   if (CT->isIncompleteType())
     return Cl::CM_IncompleteType;
 
   // Records with any const fields (recursively) are not modifiable.
   if (const RecordType *R = CT->getAs<RecordType>())
     if (R->hasConstFields())
       return Cl::CM_ConstQualifiedField;
-
+  // CXXThis can't point to const marked field of the given class
+  // to be modifiable
+  while (true) {
+    if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
+      E = ME->getBase()->IgnoreParenImpCasts();
+      continue;
+    } else if (const ArraySubscriptExpr *ASE =
+                   dyn_cast<ArraySubscriptExpr>(E)) {
+      E = ASE->getBase()->IgnoreParenImpCasts();
+      continue;
+    } else if (const ExtVectorElementExpr *EVE =
+                   dyn_cast<ExtVectorElementExpr>(E)) {
+      E = EVE->getBase()->IgnoreParenImpCasts();
+      continue;
+    } else if (const CXXThisExpr *CXXThis = dyn_cast<CXXThisExpr>(E)) {
+      CT = Ctx.getCanonicalType(
+          CXXThis->getType().getTypePtr()->getPointeeType());
+      if (CT.isConstQualified())
+        return Cl::CM_ConstQualified;
+    }
+    break;
+  }
   return Cl::CM_Modifiable;
 }
 
 Expr::LValueClassification Expr::ClassifyLValue(ASTContext &Ctx) const {
   Classification VC = Classify(Ctx);
   switch (VC.getKind()) {
   case Cl::CL_LValue: return LV_Valid;
   case Cl::CL_XValue: return LV_InvalidExpression;

lib/Sema/SemaExpr.cpp

if (LHSTy->isDependentType() || RHSTy->isDependentType()) {
     if (VK != VK_RValue)
       OK = OK_VectorComponent;
 
     // FIXME: need to deal with const...
     ResultType = VTy->getElementType();
   } else if (LHSTy->isArrayType()) {
     // If we see an array that wasn't promoted by
     // DefaultFunctionArrayLvalueConversion, it must be an array that
     // wasn't promoted because of the C90 rule that doesn't
     // allow promoting non-lvalue arrays.  Warn, then
     // force the promotion here.
     Diag(LHSExp->getLocStart(), diag::ext_subscript_non_lvalue) <<
         LHSExp->getSourceRange();
     LHSExp = ImpCastExprToType(LHSExp, Context.getArrayDecayedType(LHSTy),
                                CK_ArrayToPointerDecay).get();
     LHSTy = LHSExp->getType();
 
static bool IsReadonlyMessage(Expr *E, Sema &S) {
   return Base->getMethodDecl() != nullptr;
 }
 
 /// Is the given expression (which must be 'const') a reference to a
 /// variable which was originally non-const, but which has become
 /// 'const' due to being captured within a block?
 enum NonConstCaptureKind { NCCK_None, NCCK_Block, NCCK_Lambda };
 static NonConstCaptureKind isReferenceToNonConstCapture(Sema &S, Expr *E) {
-  assert(E->isLValue() && E->getType().isConstQualified());
+  assert(E->isLValue());
   E = E->IgnoreParens();
 
   // Must be a reference to a declaration from an enclosing scope.
   DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E);
   if (!DRE) return NCCK_None;
   if (!DRE->refersToEnclosingVariableOrCapture()) return NCCK_None;
 
   // The declaration must be a variable which is not declared 'const'.
while (true) {
     IsDereference = NextIsDereference;
 
     E = E->IgnoreImplicit()->IgnoreParenImpCasts();
     if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
       NextIsDereference = ME->isArrow();
       const ValueDecl *VD = ME->getMemberDecl();
       if (const FieldDecl *Field = dyn_cast<FieldDecl>(VD)) {
         // Mutable fields can be modified even if the class is const.
-        if (Field->isMutable()) {
-          assert(DiagnosticEmitted && "Expected diagnostic not emitted.");
+        if (Field->isMutable())
           break;
-        }
 
         if (!IsTypeModifiable(Field->getType(), IsDereference)) {
           if (!DiagnosticEmitted) {
             S.Diag(Loc, diag::err_typecheck_assign_const)
                 << ExprRange << ConstMember << false /*static*/ << Field
                 << Field->getType();
             DiagnosticEmitted = true;
           }
if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
           }
           S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
               << ConstMember << true /*static*/ << VDecl << VDecl->getType()
               << VDecl->getSourceRange();
         }
         // Static fields do not inherit constness from parents.
         break;
       }
-      break;
-    } // End MemberExpr
+      break; // End MemberExpr
+    } else if (const ArraySubscriptExpr *ASE =
+                   dyn_cast<ArraySubscriptExpr>(E)) {
+      E = ASE->getBase()->IgnoreParenImpCasts();
+      continue;
+    } else if (const ExtVectorElementExpr *EVE =
+                   dyn_cast<ExtVectorElementExpr>(E)) {
+      E = EVE->getBase()->IgnoreParenImpCasts();
+      continue;
+    }
     break;
   }
 
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     // Function calls
     const FunctionDecl *FD = CE->getDirectCallee();
     if (FD && !IsTypeModifiable(FD->getReturnType(), IsDereference)) {
       if (!DiagnosticEmitted) {

test/Sema/assign.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 void *test1(void) { return 0; }
 
 void test2 (const struct {int a;} *x) {
   // expected-note@-1 {{variable 'x' declared const here}}
 
   x->a = 10;
   // expected-error-re@-1 {{cannot assign to variable 'x' with const-qualified type 'const struct (anonymous struct at {{.*}}assign.c:5:19) *'}}
 }
 
 typedef int arr[10];
 void test3() {
-  const arr b;
-  const int b2[10]; 
-  b[4] = 1; // expected-error {{read-only variable is not assignable}}
-  b2[4] = 1; // expected-error {{read-only variable is not assignable}}
+  const arr b;  // expected-note {{variable 'b' declared const here}}
+  const int b2[10];   // expected-note {{variable 'b2' declared const here}}
+  b[4] = 1; // expected-error {{cannot assign to variable 'b' with const-qualified type 'const arr' (aka 'int const[10]')}}
+  b2[4] = 1; // expected-error {{cannot assign to variable 'b2' with const-qualified type 'const int [10]'}}
 }
 
 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;

test/Sema/typedef-retain.c

 void test2(float4 a, int4p result, int i) {
     result[i] = a; // expected-error {{assigning to 'int4' (vector of 4 'int' values) from incompatible type 'float4' (vector of 4 'float' values)}}
 }
 
 // PR2039
 typedef int a[5];
 void test3() {
   typedef const a b;
-  b r;
-  r[0]=10;  // expected-error {{read-only variable is not assignable}}
+  b r;      // expected-note {{variable 'r' declared const here}}
+  r[0]=10;  // expected-error {{cannot assign to variable 'r' with const-qualified type 'b' (aka 'int const[5]')}}
 }
 
 int test4(const a y) {
   y[0] = 10; // expected-error {{read-only variable is not assignable}}
 }
 

test/SemaCXX/anonymous-union.cpp

void X::test_unqual_references() {
   f = 0.0;
   f2 = f;
   d = f;
   f3 = 0; // expected-error{{use of undeclared identifier 'f3'}}
   a = 0;
 }
 
 void X::test_unqual_references_const() const { // expected-note 2{{member function 'X::test_unqual_references_const' is declared const here}}
-  d = 0.0;
+  // TODO: it seems we should not see any error here becuase 'd' is mutable
+  d = 0.0; // expected-error {{read-only variable is not assignable}}
   f2 = 0; // expected-error{{cannot assign to non-static data member within const member function 'test_unqual_references_const'}}
   a = 0; // expected-error{{cannot assign to non-static data member within const member function 'test_unqual_references_const'}}
 }
 
 void test_unqual_references(X x, const X xc) {
   // expected-note@-1 2{{variable 'xc' declared const here}}
   x.i = 0;
   x.f = 0.0;

test/SemaCXX/err_typecheck_assign_const.cpp

 }
 
 void test() {
   typedef const int &Func();
 
   Func &bar();
   bar()() = 0; // expected-error {{read-only variable is not assignable}}
 }
+
+typedef float float4 __attribute__((ext_vector_type(4)));
+struct OhNo {
+  float4 v;
+  void AssignMe() const { v.x = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \
+                                        expected-note {{member function 'OhNo::AssignMe' is declared const here}}
+};
+
+typedef float float4_2 __attribute__((__vector_size__(16)));
+struct OhNo2 {
+  float4_2 v;
+  void AssignMe() const { v[0] = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \
+                                        expected-note {{member function 'OhNo2::AssignMe' is declared const here}}
+};
+
+struct OhNo3 {
+  float v[4];
+  void AssignMe() const { v[0] = 1; } // expected-error {{cannot assign to non-static data member within const member function 'AssignMe'}} \
+                                        expected-note {{member function 'OhNo3::AssignMe' is declared const here}}
+};