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 ↗

(On Diff #131422)

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 ↗

(On Diff #131422)

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

[rjmccall]

lib/AST/ExprClassification.cpp

652 ↗

(On Diff #131422)

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

4402

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/Sema/SemaExpr.cpp

if (LHSTy->isDependentType() || RHSTy->isDependentType()) {
     }
   } else if (const VectorType *VTy = LHSTy->getAs<VectorType>()) {
     BaseExpr = LHSExp;    // vectors: V[123]
     IndexExpr = RHSExp;
     VK = LHSExp->getValueKind();
     if (VK != VK_RValue)
       OK = OK_VectorComponent;
 
-    // FIXME: need to deal with const...
     ResultType = VTy->getElementType();
+    QualType BaseType = BaseExpr->getType();
+    Qualifiers BaseQuals = BaseType.getQualifiers();
+    Qualifiers MemberQuals =
+        Context.getCanonicalType(ResultType).getQualifiers();
+    Qualifiers Combined = BaseQuals + MemberQuals;
+    if (Combined != MemberQuals)
+      ResultType = Context.getQualifiedType(ResultType, Combined);
   } 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();
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) {

lib/Sema/SemaExprMember.cpp

if (BaseType->isExtVectorType()) {
     if (IsArrow)
       VK = VK_LValue;
     else {
       if (PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(BaseExpr.get()))
         VK = POE->getSyntacticForm()->getValueKind();
       else
         VK = BaseExpr.get()->getValueKind();
     }
+
     QualType ret = CheckExtVectorComponent(S, BaseType, VK, OpLoc,
                                            Member, MemberLoc);
     if (ret.isNull())
       return ExprError();
+    Qualifiers BaseQ =
+        S.Context.getCanonicalType(BaseExpr.get()->getType()).getQualifiers();
+    ret = S.Context.getQualifiedType(ret, BaseQ);
 
     return new (S.Context)
         ExtVectorElementExpr(ret, VK, BaseExpr.get(), *Member, MemberLoc);
   }
 
   // Adjust builtin-sel to the appropriate redefinition type if that's
   // not just a pointer to builtin-sel again.
   if (IsArrow && BaseType->isSpecificBuiltinType(BuiltinType::ObjCSel) &&

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/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}}
+};