[SYCL] Prohibit arithmetic operations for incompatible pointers

[bader]

Summary

This change enables OpenCL diagnostics for the pointers annotated with
address space attribute SYCL mode.

[bader]

clang/lib/Sema/SemaExpr.cpp

10089–10090

Alternative approach would be to remove S.getLangOpts().OpenCL to enable this check for all modes.

@Anastasia, do you know if it's safe to do?

[Anastasia]

clang/lib/Sema/SemaExpr.cpp

10089–10090

If I look at embedded C (ISO/IEC TR 18037) s5.3 rules that we are following largely in Clang I believe this is a universal rule!

Especially looking at the followong statement:

Clause 6.5.6 - Additive operators, add another constraint paragraph:
For subtraction, if the two operands are pointers into different address spaces, the address spaces must overlap.

So I think that it should apply to at least OpenCL, C and C++. I am surprised though that this has not been fixed yet.

I am CCing @rjmccall and @jeroen.dobbelaere in case they have any more insights on this.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

10089–10090

Yes, I agree, this should apply in all modes.

You should able to restructure the isAddressSpaceOverlapping function so that it can work directly on the address spaces of LHSPointeeTy and RHSPointeeTy; the code will be both cleaner and faster.

[bader]

Enable diagnostics for non-OpenCL modes and applied refactoring proposed by John.

[bader]

Thanks for the review.
I've enabled the diagnostics for all the modes.
I also applied the refactoring suggested by @rjmccall. Hopefully I understand it correctly.

[rjmccall]

Please add a C test case just using the address_space attribute.

clang/include/clang/AST/Type.h

1069

It's idiomatic to take QualType by value rather than const &.

Can you rewrite the PointerType method to delegate to this? Assuming it isn't completely dead, that is.

[bader]

• Added C test case with address_space attribute.
• Move isAddressSpaceOverlapping from PointerType to QualType.
• Move C++ test case to clang/test/SemaCXX

[bader]

clang/include/clang/AST/Type.h

1069

It isn't completely dead, but there were just a few uses of the PointerType method, so I've updated all of them to avoid code duplication in two classes.

[rjmccall]

LGTM

clang/include/clang/AST/Type.h

1069

Even better, thanks.

[bader]

Fix formatting in clang/test/Sema/address_spaces.c

clang/include/clang/AST/Type.h

public:
   }
 
   /// Check if this type has any address space qualifier.
   inline bool hasAddressSpace() const;
 
   /// Return the address space of this type.
   inline LangAS getAddressSpace() const;
 
+  /// Returns true if address space qualifiers overlap with T address space
+  /// qualifiers.
+  /// OpenCL C defines conversion rules for pointers to different address spaces
+  /// and notion of overlapping address spaces.
+  /// CL1.1 or CL1.2:
+  ///   address spaces overlap iff they are they same.
+  /// OpenCL C v2.0 s6.5.5 adds:
+  ///   __generic overlaps with any address space except for __constant.
+  bool isAddressSpaceOverlapping(QualType T) const {
+    Qualifiers Q = getQualifiers();
+    Qualifiers TQ = T.getQualifiers();
+    // Address spaces overlap if at least one of them is a superset of another
+    return Q.isAddressSpaceSupersetOf(TQ) || TQ.isAddressSpaceSupersetOf(Q);
+  }
+
   /// Returns gc attribute of this type.
   inline Qualifiers::GC getObjCGCAttr() const;
 
   /// true when Type is objc's weak.
   bool isObjCGCWeak() const {
     return getObjCGCAttr() == Qualifiers::Weak;
   }
 
class PointerType : public Type, public llvm::FoldingSetNode {
 
   PointerType(QualType Pointee, QualType CanonicalPtr)
       : Type(Pointer, CanonicalPtr, Pointee->getDependence()),
         PointeeType(Pointee) {}
 
 public:
   QualType getPointeeType() const { return PointeeType; }
 
-  /// Returns true if address spaces of pointers overlap.
-  /// OpenCL v2.0 defines conversion rules for pointers to different
-  /// address spaces (OpenCLC v2.0 s6.5.5) and notion of overlapping
-  /// address spaces.
-  /// CL1.1 or CL1.2:
-  ///   address spaces overlap iff they are they same.
-  /// CL2.0 adds:
-  ///   __generic overlaps with any address space except for __constant.
-  bool isAddressSpaceOverlapping(const PointerType &other) const {
-    Qualifiers thisQuals = PointeeType.getQualifiers();
-    Qualifiers otherQuals = other.getPointeeType().getQualifiers();
-    // Address spaces overlap if at least one of them is a superset of another
-    return thisQuals.isAddressSpaceSupersetOf(otherQuals) ||
-           otherQuals.isAddressSpaceSupersetOf(thisQuals);
-  }
-
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 
   void Profile(llvm::FoldingSetNodeID &ID) {
     Profile(ID, getPointeeType());
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID, QualType Pointee) {

clang/lib/Sema/SemaCast.cpp

static TryCastResult TryAddressSpaceCast(Sema &Self, ExprResult &SrcExpr,
   auto SrcPtrType = SrcType->getAs<PointerType>();
   if (!SrcPtrType)
     return TC_NotApplicable;
   auto DestPtrType = DestType->getAs<PointerType>();
   if (!DestPtrType)
     return TC_NotApplicable;
   auto SrcPointeeType = SrcPtrType->getPointeeType();
   auto DestPointeeType = DestPtrType->getPointeeType();
-  if (!DestPtrType->isAddressSpaceOverlapping(*SrcPtrType)) {
+  if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
     msg = diag::err_bad_cxx_cast_addr_space_mismatch;
     return TC_Failed;
   }
   auto SrcPointeeTypeWithoutAS =
       Self.Context.removeAddrSpaceQualType(SrcPointeeType.getCanonicalType());
   auto DestPointeeTypeWithoutAS =
       Self.Context.removeAddrSpaceQualType(DestPointeeType.getCanonicalType());
   if (Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
if (Self.getLangOpts().OpenCL) {
     DestPtr = Self.getASTContext().getCanonicalType(DestType.getTypePtr()),
     SrcPtr  = Self.getASTContext().getCanonicalType(SrcType.getTypePtr());
 
     while (isa<PointerType>(DestPtr) && isa<PointerType>(SrcPtr)) {
       const PointerType *DestPPtr = cast<PointerType>(DestPtr);
       const PointerType *SrcPPtr = cast<PointerType>(SrcPtr);
       QualType DestPPointee = DestPPtr->getPointeeType();
       QualType SrcPPointee = SrcPPtr->getPointeeType();
-      if (Nested ? DestPPointee.getAddressSpace() !=
-                   SrcPPointee.getAddressSpace()
-                 : !DestPPtr->isAddressSpaceOverlapping(*SrcPPtr)) {
+      if (Nested
+              ? DestPPointee.getAddressSpace() != SrcPPointee.getAddressSpace()
+              : !DestPPointee.isAddressSpaceOverlapping(SrcPPointee)) {
         Self.Diag(OpRange.getBegin(), DiagID)
             << SrcType << DestType << Sema::AA_Casting
             << SrcExpr.get()->getSourceRange();
         if (!Nested)
           SrcExpr = ExprError();
         return;
       }
 

clang/lib/Sema/SemaExpr.cpp

static bool checkArithmeticBinOpPointerOperands(Sema &S, SourceLocation Loc,
   bool isLHSPointer = LHSExpr->getType()->isAnyPointerType();
   bool isRHSPointer = RHSExpr->getType()->isAnyPointerType();
   if (!isLHSPointer && !isRHSPointer) return true;
 
   QualType LHSPointeeTy, RHSPointeeTy;
   if (isLHSPointer) LHSPointeeTy = LHSExpr->getType()->getPointeeType();
   if (isRHSPointer) RHSPointeeTy = RHSExpr->getType()->getPointeeType();
 
   // if both are pointers check if operation is valid wrt address spaces
-  if (S.getLangOpts().OpenCL && isLHSPointer && isRHSPointer) {
-    const PointerType *lhsPtr = LHSExpr->getType()->castAs<PointerType>();
-    const PointerType *rhsPtr = RHSExpr->getType()->castAs<PointerType>();
-    if (!lhsPtr->isAddressSpaceOverlapping(*rhsPtr)) {
+  if (isLHSPointer && isRHSPointer) {
+    if (!LHSPointeeTy.isAddressSpaceOverlapping(RHSPointeeTy)) {
       S.Diag(Loc,
              diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
           << LHSExpr->getType() << RHSExpr->getType() << 1 /*arithmetic op*/
           << LHSExpr->getSourceRange() << RHSExpr->getSourceRange();
       return false;
     }
   }
 
if (Context.typesAreCompatible(LCanPointeeTy.getUnqualifiedType(),
                                                 /*isError*/false);
     } else {
       // Invalid
       diagnoseDistinctPointerComparison(*this, Loc, LHS, RHS, /*isError*/false);
     }
     if (LCanPointeeTy != RCanPointeeTy) {
       // Treat NULL constant as a special case in OpenCL.
       if (getLangOpts().OpenCL && !LHSIsNull && !RHSIsNull) {
-        const PointerType *LHSPtr = LHSType->castAs<PointerType>();
-        if (!LHSPtr->isAddressSpaceOverlapping(*RHSType->castAs<PointerType>())) {
+        if (!LCanPointeeTy.isAddressSpaceOverlapping(RCanPointeeTy)) {
           Diag(Loc,
                diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
               << LHSType << RHSType << 0 /* comparison */
               << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
         }
       }
       LangAS AddrSpaceL = LCanPointeeTy.getAddressSpace();
       LangAS AddrSpaceR = RCanPointeeTy.getAddressSpace();

clang/test/Sema/address_spaces.c

 typedef int PR4997 __attribute__((address_space(Foobar))); // expected-error {{use of undeclared identifier 'Foobar'}}
 __attribute__((address_space("12"))) int *i; // expected-error {{'address_space' attribute requires an integer constant}}
 
 // Clang extension doesn't forbid operations on pointers to different address spaces.
 char* cmp(_AS1 char *x,  _AS2 char *y) {
   return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
 }
 
+char *sub(_AS1 char *x, _AS2 char *y) {
+  return x - y; // expected-error {{arithmetic operation with operands of type  ('_AS1 char *' and '_AS2 char *') which are pointers to non-overlapping address spaces}}
+}
+
 struct SomeStruct {
   int a;
   long b;
   int c;
 };
 
 // Compound literals in function scope are lvalues with automatic storage duration,
 // so they cannot realistically be qualified with an address space.
 void as_compound_literal() {
   (_AS1 struct SomeStruct){1, 2, 3}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
   (_AS1 char[]){"test"}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
   (_AS1 char[]){'a', 'b', 'c'}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
 }

clang/test/SemaCXX/address-space-arithmetic.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int *foo(__attribute__((opencl_private)) int *p,
+         __attribute__((opencl_local)) int *l) {
+  return p - l; // expected-error {{arithmetic operation with operands of type  ('__private int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+}