[Sema] Prevent binding references with mismatching address spaces to temporaries

[Anastasia]

Summary

This is a straw-man idea for solving binding references with address spaces to temporaries. If the logic below makes sense I will apply it elsewhere in similar checks.

Arbitrary address space references can't be bound to temporaries. The reference binding logic should check that the address space of temporary can be implicitly converted to address space in reference when temporary materialization is performed.

Example:

_AS1 float gf;
...
_AS1 const int& ref = gf; //this can only be accepted if implicit conversion from _AS1 to LangAS::Default is allowed

This review depends on: https://reviews.llvm.org/D58060

[rjmccall]

lib/Sema/SemaInit.cpp

4836 ↗

(On Diff #197349)

Isn't cv1T1IgnoreAS.getQualifiers() always LangAS::Default?

[Anastasia]

lib/Sema/SemaInit.cpp

4836 ↗

(On Diff #197349)

Yes, this is true currently. However, we don't have an overload of isAddressSpaceSupersetOf that takes LangAS as a parameter. Are you suggesting to add it?

[rjmccall]

lib/Sema/SemaInit.cpp

4836 ↗

(On Diff #197349)

I think it would be sensible if there was a function that could answer the superset question with just two LangAS values, yeah. Even with the current API, it would at least be clearer if you just manufactured a Qualifiers with no address space, since there's no dependency on any of the other qualifiers from cv1T1IgnoreAS.

[Anastasia]

Added extra overload for isAddressSpaceSupersetOf.

[rjmccall]

include/clang/AST/Type.h

463 ↗

(On Diff #197769)

"equal to or a superset of", just for clarity's sake.

478 ↗

(On Diff #197769)

How about: "if the address space in these qualifiers is equal to or a superset of the address space in the argument qualifiers".

[Anastasia]

Updated comments!

[rjmccall]

LGTM.

cfe/trunk/include/clang/AST/Type.h

assert(getAddressSpace() == qs.getAddressSpace() ||
            !hasAddressSpace() || !qs.hasAddressSpace());
     assert(getObjCGCAttr() == qs.getObjCGCAttr() ||
            !hasObjCGCAttr() || !qs.hasObjCGCAttr());
     assert(getObjCLifetime() == qs.getObjCLifetime() ||
            !hasObjCLifetime() || !qs.hasObjCLifetime());
     Mask |= qs.Mask;
   }
 
-  /// Returns true if this address space is a superset of the other one.
+  /// Returns true if address space A is equal to or a superset of B.
   /// OpenCL v2.0 defines conversion rules (OpenCLC v2.0 s6.5.5) and notion of
   /// overlapping address spaces.
   /// CL1.1 or CL1.2:
   ///   every address space is a superset of itself.
   /// CL2.0 adds:
   ///   __generic is a superset of any address space except for __constant.
-  bool isAddressSpaceSupersetOf(Qualifiers other) const {
-    return
-        // Address spaces must match exactly.
-        getAddressSpace() == other.getAddressSpace() ||
+  static bool isAddressSpaceSupersetOf(LangAS A, LangAS B) {
+    return A == B ||
            // Otherwise in OpenCLC v2.0 s6.5.5: every address space except
            // for __constant can be used as __generic.
-        (getAddressSpace() == LangAS::opencl_generic &&
-         other.getAddressSpace() != LangAS::opencl_constant);
+           (A == LangAS::opencl_generic && B != LangAS::opencl_constant);
+  }
+
+  /// Returns true if the address space in these qualifiers is equal to or
+  /// a superset of the address space in the argument qualifiers.
+  bool isAddressSpaceSupersetOf(Qualifiers other) const {
+    return isAddressSpaceSupersetOf(getAddressSpace(), other.getAddressSpace());
   }
 
   /// Determines if these qualifiers compatibly include another set.
   /// Generally this answers the question of whether an object with the other
   /// qualifiers can be safely used as an object with these qualifiers.
   bool compatiblyIncludes(Qualifiers other) const {
     return isAddressSpaceSupersetOf(other) &&
            // ObjC GC qualifiers can match, be added, or be removed, but can't

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def err_reference_bind_drops_quals : Error<
   "binding value %diff{of type $ to reference to type $|to reference}0,1 "
   "drops %select{<<ERROR>>|'const'|'restrict'|'const' and 'restrict'|"
   "'volatile'|'const' and 'volatile'|'restrict' and 'volatile'|"
   "'const', 'restrict', and 'volatile'}2 qualifier%plural{1:|2:|4:|:s}2">;
 def err_reference_bind_failed : Error<
   "reference %diff{to %select{type|incomplete type}1 $ could not bind to an "
   "%select{rvalue|lvalue}2 of type $|could not bind to %select{rvalue|lvalue}2 of "
   "incompatible type}0,3">;
+def err_reference_bind_temporary_addrspace : Error<
+  "reference of type %0 cannot bind to a temporary object because of "
+  "address space mismatch">;
 def err_reference_bind_init_list : Error<
   "reference to type %0 cannot bind to an initializer list">;
 def err_init_list_bad_dest_type : Error<
   "%select{|non-aggregate }0type %1 cannot be initialized with an initializer "
   "list">;
 def warn_cxx2a_compat_aggregate_init_with_ctors : Warning<
   "aggregate initialization of type %0 with user-declared constructors "
   "is incompatible with C++2a">, DefaultIgnore, InGroup<CXX2aCompat>;

cfe/trunk/include/clang/Sema/Initialization.h

enum FailureKind {
     FK_NonConstLValueReferenceBindingToUnrelated,
 
     /// Rvalue reference binding to an lvalue.
     FK_RValueReferenceBindingToLValue,
 
     /// Reference binding drops qualifiers.
     FK_ReferenceInitDropsQualifiers,
 
+    /// Reference with mismatching address space binding to temporary.
+    FK_ReferenceAddrspaceMismatchTemporary,
+
     /// Reference binding failed.
     FK_ReferenceInitFailed,
 
     /// Implicit conversion failed.
     FK_ConversionFailed,
 
     /// Implicit conversion failed.
     FK_ConversionFromPropertyFailed,

cfe/trunk/lib/Sema/SemaInit.cpp

bool InitializationSequence::isAmbiguous() const {
   case FK_PlainStringIntoUTF8Char:
   case FK_UTF8StringIntoPlainChar:
   case FK_AddressOfOverloadFailed: // FIXME: Could do better
   case FK_NonConstLValueReferenceBindingToTemporary:
   case FK_NonConstLValueReferenceBindingToBitfield:
   case FK_NonConstLValueReferenceBindingToVectorElement:
   case FK_NonConstLValueReferenceBindingToUnrelated:
   case FK_RValueReferenceBindingToLValue:
+  case FK_ReferenceAddrspaceMismatchTemporary:
   case FK_ReferenceInitDropsQualifiers:
   case FK_ReferenceInitFailed:
   case FK_ConversionFailed:
   case FK_ConversionFromPropertyFailed:
   case FK_TooManyInitsForScalar:
   case FK_ParenthesizedListInitForScalar:
   case FK_ReferenceBindingToInitList:
   case FK_InitListBadDestinationType:
if (RefRelationship >= Sema::Ref_Related && !isLValueRef &&
       InitCategory.isLValue()) {
     Sequence.SetFailed(
                     InitializationSequence::FK_RValueReferenceBindingToLValue);
     return;
   }
 
   Sequence.AddReferenceBindingStep(cv1T1IgnoreAS, /*bindingTemporary=*/true);
 
-  if (T1Quals.hasAddressSpace())
+  if (T1Quals.hasAddressSpace()) {
+    if (!Qualifiers::isAddressSpaceSupersetOf(T1Quals.getAddressSpace(),
+                                              LangAS::Default)) {
+      Sequence.SetFailed(
+          InitializationSequence::FK_ReferenceAddrspaceMismatchTemporary);
+      return;
+    }
     Sequence.AddQualificationConversionStep(cv1T1, isLValueRef ? VK_LValue
                                                                : VK_XValue);
   }
+}
 
 /// Attempt character array initialization from a string literal
 /// (C++ [dcl.init.string], C99 6.7.8).
 static void TryStringLiteralInitialization(Sema &S,
                                            const InitializedEntity &Entity,
                                            const InitializationKind &Kind,
                                            Expr *Initializer,
                                        InitializationSequence &Sequence) {
case FK_NonConstLValueReferenceBindingToVectorElement:
     break;
 
   case FK_RValueReferenceBindingToLValue:
     S.Diag(Kind.getLocation(), diag::err_lvalue_to_rvalue_ref)
       << DestType.getNonReferenceType() << OnlyArg->getType()
       << Args[0]->getSourceRange();
     break;
 
+  case FK_ReferenceAddrspaceMismatchTemporary:
+    S.Diag(Kind.getLocation(), diag::err_reference_bind_temporary_addrspace)
+        << DestType << Args[0]->getSourceRange();
+    break;
+
   case FK_ReferenceInitDropsQualifiers: {
     QualType SourceType = OnlyArg->getType();
     QualType NonRefType = DestType.getNonReferenceType();
     Qualifiers DroppedQualifiers =
         SourceType.getQualifiers() - NonRefType.getQualifiers();
 
     S.Diag(Kind.getLocation(), diag::err_reference_bind_drops_quals)
       << SourceType
case FailedSequence: {
     case FK_RValueReferenceBindingToLValue:
       OS << "rvalue reference bound to an lvalue";
       break;
 
     case FK_ReferenceInitDropsQualifiers:
       OS << "reference initialization drops qualifiers";
       break;
 
+    case FK_ReferenceAddrspaceMismatchTemporary:
+      OS << "reference with mismatching address space bound to temporary";
+      break;
+
     case FK_ReferenceInitFailed:
       OS << "reference initialization failed";
       break;
 
     case FK_ConversionFailed:
       OS << "conversion failed";
       break;
 

cfe/trunk/test/SemaOpenCLCXX/address-space-references.cl

+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify -fsyntax-only
+
+__global const int& f(__global float &ref) {
+  return ref; // expected-error{{reference of type 'const __global int &' cannot bind to a temporary object because of address space mismatch}}
+}