[Sema] Fix for crash on conditional operation with address_space pointer

[leonardchan]

Summary

Compiling the following causes clang to crash

char *cmp(__attribute__((address_space(1))) char *x, __attribute__((address_space(2))) char *y) {
  return x < y ? x : y;
}

with the message: "wrong cast for pointers in different address spaces(must be an address space cast)!"

This is because during IR emission, the source and dest type for a bitcast should not have differing address spaces.

This fix prints an error since the code shouldn't compile in the first place.

[ebevhan]

When I try the test case on our downstream (and when compiling for our target with an extra flag that enables a bunch of OpenCL-related address space code), I get:

ascrash.c:5:12: error: comparison between ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which
      are pointers to non-overlapping address spaces
  return x < y ? x : y;
         ~ ^ ~
ascrash.c:5:16: error: conditional operator with the second and third operands of type ('__attribute__((address_space(1))) char *' and
      '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces
  return x < y ? x : y;
               ^ ~   ~

A lot of address space code is hidden behind LangOptions.OpenCL.

lib/Sema/SemaExpr.cpp

6464

Here is an OpenCL condition.

6473

Aren't these the errors you actually want?

8875

Here is another OpenCL AS error.

10219

And another.

[leonardchan]

• Changed diff such that an error is dumped instead. The code shouldn't compile in the first place since it involves conversion between pointers from different address_spaces.

[rjmccall]

I would expect this to replace the existing warning, not to appear together with it.

[rjmccall]

test/Sema/conditional-expr.c

78

Also, these diagnostics seem wrong. Where is void * coming from?

[leonardchan]

In D50278#1189919, @rjmccall wrote:

I would expect this to replace the existing warning, not to appear together with it.

Will do.

test/Sema/conditional-expr.c

78

When dumping the AST this is what the resulting type is for the conditional expression already is if the operands are 2 pointers with different address spaces.

According to this comment, the reason seems to be because this is what GCC does:

6512     // In this situation, we assume void* type. No especially good
6513     // reason, but this is what gcc does, and we do have to pick
6514     // to get a consistent AST.

[rjmccall]

test/Sema/conditional-expr.c

78

That makes sense in general, but in this case it's not a great diagnostic; we should just emit an error when trying to pick a common type.

[leonardchan]

• Replaced instances of a pointer type mismatch warning involving 2 conditional operands with different address spaces with a new error specifically for this situation.

[rjmccall]

lib/Sema/SemaExpr.cpp

6522

I was going to tell you to use the predicate Qualifiers::isAddressSpaceSupersetOf here, but then I was looking at the uses of that, and I think the real fix is to just go into the implementation of checkConditionalPointerCompatibility and make the compatibility logic not OpenCL-specific. The fast-path should just be whether the address spaces are different.

And it looks like this function has a bug where it always uses LangAS::Default outside of OpenCL even if the pointers are in the same address space.

[ebevhan]

I think the solution to a lot of diagnostic and behavior issues with address spaces is to remove predication on OpenCL. It's a bit odd to have a language feature that is enabled out of the box regardless of langoptions (address spaces) but won't actually work properly unless you're building OpenCL.

include/clang/Basic/DiagnosticSemaKinds.td

6943

This error is very similar to the one in my first comment, conditional operator with the second and third operands of type ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces. It would normally be emitted at 6472, but won't be since OpenCL isn't enabled.

test/Sema/conditional-expr.c

78

Is it possible that you are getting void * because we aren't running the qualifier removal at 6495? I don't think I've ever seen spurious void *'s show up in our downstream diagnostics.

[leonardchan]

• Removed checks for OpenCL in checkConditionalPointerCompatibility. This allows for the error err_typecheck_op_on_nonoverlapping_address_space_pointers to be dumped on getting pointers with different address spaces instead of the warning ext_typecheck_cond_incompatible_pointers.

include/clang/Basic/DiagnosticSemaKinds.td

6943

Done. Removing the check for OpenCL throws this instead of the warning.

lib/Sema/SemaExpr.cpp

6522

I'm not sure how the LangAS::Default, but removing all checks for OpenCL does the trick and prints an existing error relating to different address_spaces on conditional operands to replace the warning. Only 2 tests needed the change from the expected warning to expected error without having to change any OpenCL tests.

I also think the address_space comparison is already done with the lhQual.isAddressSpaceSupersetOf and rhQual.isAddressSpaceSupersetOf.

test/Sema/conditional-expr.c

78

So the void * is what get's dumped for me using the latest upstream version of clang and is the result of the ConditionalOperator.

An AST dump of

3   unsigned long test0 = 5;
4   int __attribute__((address_space(2))) *adr2;
5   int __attribute__((address_space(3))) *adr3;
6   test0 ? adr2 : adr3;

for me returns

`-ConditionalOperator 0xbdbcab0 <line:6:3, col:18> 'void *'
  |-ImplicitCastExpr 0xbdbc690 <col:3> 'unsigned long' <LValueToRValue>
  | `-DeclRefExpr 0xbdbc618 <col:3> 'unsigned long' lvalue Var 0xbdbc348 'test0' 'unsigned long'
  |-ImplicitCastExpr 0xbdbc790 <col:11> 'void *' <BitCast>
  | `-ImplicitCastExpr 0xbdbc6a8 <col:11> '__attribute__((address_space(2))) int *' <LValueToRValue>
  |   `-DeclRefExpr 0xbdbc640 <col:11> '__attribute__((address_space(2))) int *' lvalue Var 0xbdbc490 'adr2' '__attribute__((address_space(2))) int *'
  `-ImplicitCastExpr 0xbdbc7a8 <col:18> 'void *' <BitCast>
    `-ImplicitCastExpr 0xbdbc6c0 <col:18> '__attribute__((address_space(3))) int *' <LValueToRValue>
      `-DeclRefExpr 0xbdbc668 <col:18> '__attribute__((address_space(3))) int *' lvalue Var 0xbdbc5a0 'adr3' '__attribute__((address_space(3))) int *'

[rjmccall]

In D50278#1190544, @ebevhan wrote:

I think the solution to a lot of diagnostic and behavior issues with address spaces is to remove predication on OpenCL. It's a bit odd to have a language feature that is enabled out of the box regardless of langoptions (address spaces) but won't actually work properly unless you're building OpenCL.

I agree; almost all of the address-space-related restrictions predicated on OpenCL are actually general restrictions that apply across all address-space extensions. OpenCL's rules only differ from what's laid out in TR 18037 in how certain declarations default to specific address spaces. It's unfortunate that the code reviewers at the time (which probably included me at least a little) didn't try harder to push for the more general rule. As it is, it would be a good project for someone who's working on address spaces a lot to just audit all the OpenCL-specific checks in Sema to see which of them should be generalized.

[rjmccall]

LGTM.

lib/Sema/SemaExpr.cpp

6522

Er, you're right, of course, since isAddressSpaceSupersetOf is a non-strict ordering. If that operation ever gets big enough that we don't want to inline the whole thing, we can at least make sure the fast-path is inlinable and then outline the complicated stuff. We can also worry about that later.

include/clang/Basic/DiagnosticSemaKinds.td

def err_typecheck_incompatible_ownership : Error<
   "|%diff{initializing $ with an expression of type $|"
   "initializing with expression of different type}0,1"
   "|%diff{sending $ to parameter of type $|"
   "sending to parameter of different type}0,1"
   "|%diff{casting $ to type $|casting between types}0,1}2"
   " changes retain/release properties of pointer">;
 def err_typecheck_comparison_of_distinct_blocks : Error<
   "comparison of distinct block types%diff{ ($ and $)|}0,1">;
+def err_typecheck_incompatible_conditional_pointer_operands : Error<
+  "unable to find common type between %0 and %1 for conditional operation">;
 
 def err_typecheck_array_not_modifiable_lvalue : Error<
   "array type %0 is not assignable">;
 def err_typecheck_non_object_not_modifiable_lvalue : Error<
   "non-object type %0 is not assignable">;
 def err_typecheck_expression_not_modifiable_lvalue : Error<
   "expression is not assignable">;
 def err_typecheck_incomplete_type_not_modifiable_lvalue : Error<

lib/Sema/SemaExpr.cpp

static QualType checkConditionalPointerCompatibility(Sema &S, ExprResult &LHS,
   // be incompatible with address space 3: they may live on different devices or
   // anything.
   Qualifiers lhQual = lhptee.getQualifiers();
   Qualifiers rhQual = rhptee.getQualifiers();
 
   LangAS ResultAddrSpace = LangAS::Default;
   LangAS LAddrSpace = lhQual.getAddressSpace();
   LangAS RAddrSpace = rhQual.getAddressSpace();
   if (S.getLangOpts().OpenCL) {
     // OpenCL v1.1 s6.5 - Conversion between pointers to distinct address
     // spaces is disallowed.
     if (lhQual.isAddressSpaceSupersetOf(rhQual))
       ResultAddrSpace = LAddrSpace;
     else if (rhQual.isAddressSpaceSupersetOf(lhQual))
       ResultAddrSpace = RAddrSpace;
     else {
       S.Diag(Loc,
              diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
           << LHSTy << RHSTy << 2 << LHS.get()->getSourceRange()
           << RHS.get()->getSourceRange();
       return QualType();
     }
   }
 
   unsigned MergedCVRQual = lhQual.getCVRQualifiers() | rhQual.getCVRQualifiers();
   auto LHSCastKind = CK_BitCast, RHSCastKind = CK_BitCast;
if (CompositeTy.isNull()) {
     // In this situation, we assume void* type. No especially good
     // reason, but this is what gcc does, and we do have to pick
     // to get a consistent AST.
     QualType incompatTy;
     incompatTy = S.Context.getPointerType(
         S.Context.getAddrSpaceQualType(S.Context.VoidTy, ResultAddrSpace));
     LHS = S.ImpCastExprToType(LHS.get(), incompatTy, LHSCastKind);
     RHS = S.ImpCastExprToType(RHS.get(), incompatTy, RHSCastKind);
+
+    bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace;
+    bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace;
+
+    if (S.getLangOpts().OpenCL ||
+        !(HasDifferingLAddrSpace || HasDifferingRAddrSpace)) {
       // FIXME: For OpenCL the warning emission and cast to void* leaves a room
       // for casts between types with incompatible address space qualifiers.
       // For the following code the compiler produces casts between global and
       // local address spaces of the corresponded innermost pointees:
       // local int *global *a;
       // global int *global *b;
       // a = (0 ? a : b); // see C99 6.5.16.1.p1.
       S.Diag(Loc, diag::ext_typecheck_cond_incompatible_pointers)
           << LHSTy << RHSTy << LHS.get()->getSourceRange()
           << RHS.get()->getSourceRange();
+    } else {
+      // If the addresse spacesare different, we do not allow a bitcast.
+      S.Diag(Loc, diag::err_typecheck_incompatible_conditional_pointer_operands)
+          << LHSTy << RHSTy << Sema::AA_Converting
+          << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
+    }
+
     return incompatTy;
   }
 
   // The pointer types are compatible.
   // In case of OpenCL ResultTy should have the address space qualifier
   // which is a superset of address spaces of both the 2nd and the 3rd
   // operands of the conditional operator.
   QualType ResultTy = [&, ResultAddrSpace]() {
static bool checkArithmeticBinOpPointerOperands(Sema &S, SourceLocation Loc,
   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()->getAs<PointerType>();
     const PointerType *rhsPtr = RHSExpr->getType()->getAs<PointerType>();
     if (!lhsPtr->isAddressSpaceOverlapping(*rhsPtr)) {
       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(),
         diagnoseFunctionPointerToVoidComparison(*this, Loc, LHS, RHS,
                                                 /*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->getAs<PointerType>();
         if (!LHSPtr->isAddressSpaceOverlapping(*RHSType->getAs<PointerType>())) {
           Diag(Loc,
                diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
               << LHSType << RHSType << 0 /* comparison */
               << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
         }
       }

test/Sema/address_spaces.c

void access_as_field()
     (void) bar.as_field;
 }
 
 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-warning {{pointer type mismatch ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *')}}
+  return x < y ? x : y; // expected-error{{unable to find common type between '__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *' for conditional operation}}
 }

test/Sema/conditional-expr.c

void foo() {
   // The composite type here should be "int (*)[12]", fine for the sizeof
   int (*incomplete)[];
   int (*complete)[12];
   sizeof(*(test0 ? incomplete : complete)); // expected-warning {{expression result unused}}
   sizeof(*(test0 ? complete : incomplete)); // expected-warning {{expression result unused}}
 
   int __attribute__((address_space(2))) *adr2;
   int __attribute__((address_space(3))) *adr3;
-  test0 ? adr2 : adr3; // expected-warning {{pointer type mismatch}} expected-warning {{expression result unused}}
+  test0 ? adr2 : adr3; // expected-warning {{expression result unused}}
+                       // expected-error@-1{{unable to find common type between '__attribute__((address_space(2))) int *' and '__attribute__((address_space(3))) int *' for conditional operation}}
 
   // Make sure address-space mask ends up in the result type
-  (test0 ? (test0 ? adr2 : adr2) : nonconst_int); // expected-warning {{pointer type mismatch}} expected-warning {{expression result unused}}
+  (test0 ? (test0 ? adr2 : adr2) : nonconst_int); // expected-warning {{expression result unused}}
+                                                  // expected-error@-1{{unable to find common type between '__attribute__((address_space(2))) int *' and 'int *' for conditional operation}}
 }
 
 int Postgresql() {
   char x;
   return ((((&x) != ((void *) 0)) ? (*(&x) = ((char) 1)) : (void) ((void *) 0)), (unsigned long) ((void *) 0)); // expected-warning {{C99 forbids conditional expressions with only one void side}}
 }
 
 #define nil ((void*) 0)