[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 ↗

(On Diff #159116)

Here is an OpenCL condition.

6473 ↗

(On Diff #159116)

Aren't these the errors you actually want?

8860 ↗

(On Diff #159116)

Here is another OpenCL AS error.

10204 ↗

(On Diff #159116)

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 ↗

(On Diff #159315)

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 ↗

(On Diff #159315)

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 ↗

(On Diff #159315)

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 ↗

(On Diff #159405)

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 ↗

(On Diff #159405)

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 ↗

(On Diff #159315)

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 ↗

(On Diff #159405)

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

lib/Sema/SemaExpr.cpp

6522 ↗

(On Diff #159405)

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 ↗

(On Diff #159315)

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 ↗

(On Diff #159405)

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.

cfe/trunk/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();
-    }
-  }
+
+    S.Diag(Loc, diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
 
   unsigned MergedCVRQual = lhQual.getCVRQualifiers() | rhQual.getCVRQualifiers();
   auto LHSCastKind = CK_BitCast, RHSCastKind = CK_BitCast;
   lhQual.removeCVRQualifiers();
   rhQual.removeCVRQualifiers();
 
   // OpenCL v2.0 specification doesn't extend compatibility of type qualifiers
   // (C99 6.7.3) for address spaces. We assume that the check should behave in
   // the same manner as it's defined for CVR qualifiers, so for OpenCL two
   // qual types are compatible iff
   //  * corresponded types are compatible
   //  * CVR qualifiers are equal
   //  * address spaces are equal
   // Thus for conditional operator we merge CVR and address space unqualified
   // pointees and if there is a composite type we return a pointer to it with
   // merged qualifiers.
-  if (S.getLangOpts().OpenCL) {
-    LHSCastKind = LAddrSpace == ResultAddrSpace
-                      ? CK_BitCast
-                      : CK_AddressSpaceConversion;
-    RHSCastKind = RAddrSpace == ResultAddrSpace
-                      ? CK_BitCast
-                      : CK_AddressSpaceConversion;
-    lhQual.removeAddressSpace();
-    rhQual.removeAddressSpace();
-  }
+  LHSCastKind =
+      LAddrSpace == ResultAddrSpace ? CK_BitCast : CK_AddressSpaceConversion;
+  RHSCastKind =
+      RAddrSpace == ResultAddrSpace ? CK_BitCast : CK_AddressSpaceConversion;
 
   lhptee = S.Context.getQualifiedType(lhptee.getUnqualifiedType(), lhQual);
   rhptee = S.Context.getQualifiedType(rhptee.getUnqualifiedType(), rhQual);
 
   QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee);
 
   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);
+
     // 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();
+
     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]() {

cfe/trunk/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{{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}}
 }

cfe/trunk/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-error{{conditional operator with the second and third operands of type  ('__attribute__((address_space(2))) int *' and '__attribute__((address_space(3))) int *') which are pointers to non-overlapping address spaces}}
 
   // 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-error{{conditional operator with the second and third operands of type  ('__attribute__((address_space(2))) int *' and 'int *') which are pointers to non-overlapping address spaces}}
 }
 
 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)