[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339167: [Sema] Fix for crash on conditional operation with
address_space pointer (authored by leonardchan, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50278?vs=159559=159576#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50278
Files:
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/Sema/address_spaces.c
cfe/trunk/test/Sema/conditional-expr.c
Index: cfe/trunk/test/Sema/conditional-expr.c
===
--- cfe/trunk/test/Sema/conditional-expr.c
+++ cfe/trunk/test/Sema/conditional-expr.c
@@ -73,10 +73,10 @@
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() {
Index: cfe/trunk/test/Sema/address_spaces.c
===
--- cfe/trunk/test/Sema/address_spaces.c
+++ cfe/trunk/test/Sema/address_spaces.c
@@ -71,5 +71,5 @@
// 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}}
}
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -6460,20 +6460,18 @@
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();
-}
+
+ // 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();
@@ -6491,16 +6489,12 @@
// 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;
+ lhQual.removeAddressSpace();
+ rhQual.removeAddressSpace();
lhptee = S.Context.getQualifiedType(lhptee.getUnqualifiedType(), lhQual);
rhptee = S.Context.getQualifiedType(rhptee.getUnqualifiedType(), rhQual);
@@ -6516,6 +6510,7 @@
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
This revision was automatically updated to reflect the committed changes.
Closed by commit rC339167: [Sema] Fix for crash on conditional operation with
address_space pointer (authored by leonardchan, committed by ).
Repository:
rL LLVM
https://reviews.llvm.org/D50278
Files:
lib/Sema/SemaExpr.cpp
test/Sema/address_spaces.c
test/Sema/conditional-expr.c
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -6460,20 +6460,18 @@
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();
-}
+
+ // 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();
@@ -6491,16 +6489,12 @@
// 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;
+ lhQual.removeAddressSpace();
+ rhQual.removeAddressSpace();
lhptee = S.Context.getQualifiedType(lhptee.getUnqualifiedType(), lhQual);
rhptee = S.Context.getQualifiedType(rhptee.getUnqualifiedType(), rhQual);
@@ -6516,6 +6510,7 @@
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
@@ -6526,6 +6521,7 @@
S.Diag(Loc, diag::ext_typecheck_cond_incompatible_pointers)
<< LHSTy << RHSTy << LHS.get()->getSourceRange()
<< RHS.get()->getSourceRange();
+
return incompatTy;
}
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -71,5 +71,5 @@
// 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}}
}
Index: test/Sema/conditional-expr.c
===
--- test/Sema/conditional-expr.c
+++ test/Sema/conditional-expr.c
@@ -73,10 +73,10 @@
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 ?
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Sema/SemaExpr.cpp:6522 +bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; +bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace; + leonardchan wrote: > rjmccall wrote: > > 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. > 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`. 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. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added a comment. In https://reviews.llvm.org/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. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
leonardchan updated this revision to Diff 159559.
leonardchan marked an inline comment as done.
leonardchan added a comment.
- 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`.
Repository:
rC Clang
https://reviews.llvm.org/D50278
Files:
lib/Sema/SemaExpr.cpp
test/Sema/address_spaces.c
test/Sema/conditional-expr.c
Index: test/Sema/conditional-expr.c
===
--- test/Sema/conditional-expr.c
+++ test/Sema/conditional-expr.c
@@ -73,10 +73,10 @@
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() {
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -71,5 +71,5 @@
// 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}}
}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -6461,20 +6461,18 @@
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();
-}
+
+ // 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();
@@ -6492,16 +6490,12 @@
// 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;
+ lhQual.removeAddressSpace();
+ rhQual.removeAddressSpace();
lhptee = S.Context.getQualifiedType(lhptee.getUnqualifiedType(), lhQual);
rhptee = S.Context.getQualifiedType(rhptee.getUnqualifiedType(), rhQual);
@@ -6517,6 +6511,7 @@
S.Context.getAddrSpaceQualType(S.Context.VoidTy,
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
leonardchan added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6943
+def err_typecheck_incompatible_conditional_pointer_operands : Error<
+ "unable to find common type between %0 and %1 for conditional operation">;
ebevhan wrote:
> 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.
Done. Removing the check for OpenCL throws this instead of the warning.
Comment at: lib/Sema/SemaExpr.cpp:6522
+bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace;
+bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace;
+
rjmccall wrote:
> 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.
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`.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
ebevhan wrote:
> rjmccall wrote:
> > leonardchan wrote:
> > > rjmccall wrote:
> > > > Also, these diagnostics seem wrong. Where is `void *` coming from?
> > > 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.
> > > ```
> > 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.
> 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.
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 'void *'
|-ImplicitCastExpr 0xbdbc690 'unsigned long'
| `-DeclRefExpr 0xbdbc618 'unsigned long' lvalue Var 0xbdbc348
'test0' 'unsigned long'
|-ImplicitCastExpr 0xbdbc790 'void *'
| `-ImplicitCastExpr 0xbdbc6a8
'__attribute__((address_space(2))) int *'
| `-DeclRefExpr 0xbdbc640 '__attribute__((address_space(2)))
int *' lvalue Var 0xbdbc490 'adr2' '__attribute__((address_space(2))) int *'
`-ImplicitCastExpr 0xbdbc7a8 'void *'
`-ImplicitCastExpr 0xbdbc6c0
'__attribute__((address_space(3))) int *'
`-DeclRefExpr 0xbdbc668 '__attribute__((address_space(3)))
int *' lvalue Var 0xbdbc5a0 'adr3' '__attribute__((address_space(3))) int *'
```
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
ebevhan added a comment.
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.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6943
+def err_typecheck_incompatible_conditional_pointer_operands : Error<
+ "unable to find common type between %0 and %1 for conditional operation">;
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.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > Also, these diagnostics seem wrong. Where is `void *` coming from?
> > 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.
> > ```
> 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.
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.
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:6522 +bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; +bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace; + 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. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
leonardchan updated this revision to Diff 159405.
leonardchan marked an inline comment as done.
leonardchan added a comment.
- Replaced instances of a `pointer type mismatch` warning involving 2
conditional operands with different address spaces with a new error
specifically for this situation.
Repository:
rC Clang
https://reviews.llvm.org/D50278
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/address_spaces.c
test/Sema/conditional-expr.c
Index: test/Sema/conditional-expr.c
===
--- test/Sema/conditional-expr.c
+++ test/Sema/conditional-expr.c
@@ -73,10 +73,12 @@
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() {
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -71,5 +71,5 @@
// 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}}
}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -6517,16 +6517,29 @@
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();
+
+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;
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -6939,6 +6939,8 @@
" 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
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added inline comments.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
leonardchan wrote:
> rjmccall wrote:
> > Also, these diagnostics seem wrong. Where is `void *` coming from?
> 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.
> ```
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.
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
leonardchan added a comment.
In https://reviews.llvm.org/D50278#1189919, @rjmccall wrote:
> I would expect this to replace the existing warning, not to appear together
> with it.
Will do.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
rjmccall wrote:
> Also, these diagnostics seem wrong. Where is `void *` coming from?
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.
```
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added inline comments.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
Also, these diagnostics seem wrong. Where is `void *` coming from?
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added a comment. I would expect this to replace the existing warning, not to appear together with it. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
leonardchan updated this revision to Diff 159315.
leonardchan added reviewers: ebevhan, rjmccall.
leonardchan removed a subscriber: ebevhan.
leonardchan added a comment.
- 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.
Repository:
rC Clang
https://reviews.llvm.org/D50278
Files:
lib/Sema/SemaExpr.cpp
test/Sema/address_spaces.c
test/Sema/conditional-expr.c
Index: test/Sema/conditional-expr.c
===
--- test/Sema/conditional-expr.c
+++ test/Sema/conditional-expr.c
@@ -74,9 +74,12 @@
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}}
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
// 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}}
+ //
expected-error@-1{{converting '__attribute__((address_space(2))) int *' to type
'void *' changes address space of pointer}}
}
int Postgresql() {
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -72,4 +72,6 @@
// 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 *')}}
+// expected-error@-1{{converting
'__attribute__((address_space(1))) char *' to type 'void *' changes address
space of pointer}}
+// expected-error@-2{{converting
'__attribute__((address_space(2))) char *' to type 'void *' changes address
space of pointer}}
}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -6527,6 +6527,21 @@
S.Diag(Loc, diag::ext_typecheck_cond_incompatible_pointers)
<< LHSTy << RHSTy << LHS.get()->getSourceRange()
<< RHS.get()->getSourceRange();
+
+// If the addresse spacesare different, we do not allow a bitcast.
+if (!S.getLangOpts().OpenCL) {
+ if (LAddrSpace != ResultAddrSpace) {
+S.Diag(Loc, diag::err_typecheck_incompatible_address_space)
+<< LHSTy << incompatTy << Sema::AA_Converting
+<< LHS.get()->getSourceRange();
+ }
+ if (RAddrSpace != ResultAddrSpace) {
+S.Diag(Loc, diag::err_typecheck_incompatible_address_space)
+<< RHSTy << incompatTy << Sema::AA_Converting
+<< RHS.get()->getSourceRange();
+ }
+}
+
return incompatTy;
}
Index: test/Sema/conditional-expr.c
===
--- test/Sema/conditional-expr.c
+++ test/Sema/conditional-expr.c
@@ -74,9 +74,12 @@
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}}
+ // expected-error@-1{{converting '__attribute__((address_space(2))) int *' to type 'void *' changes address space of pointer}}
+ // expected-error@-2{{converting '__attribute__((address_space(3))) int *' to type 'void *' changes address space of pointer}}
// 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}}
+ // expected-error@-1{{converting '__attribute__((address_space(2))) int *' to type 'void *' changes address space of pointer}}
}
int Postgresql() {
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -72,4 +72,6 @@
// 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 *')}}
+// expected-error@-1{{converting
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
ebevhan added a comment.
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.
Comment at: lib/Sema/SemaExpr.cpp:6464
LangAS RAddrSpace = rhQual.getAddressSpace();
if (S.getLangOpts().OpenCL) {
// OpenCL v1.1 s6.5 - Conversion between pointers to distinct address
Here is an OpenCL condition.
Comment at: lib/Sema/SemaExpr.cpp:6473
S.Diag(Loc,
diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
<< LHSTy << RHSTy << 2 << LHS.get()->getSourceRange()
Aren't these the errors you actually want?
Comment at: lib/Sema/SemaExpr.cpp:8860
// if both are pointers check if operation is valid wrt address spaces
if (S.getLangOpts().OpenCL && isLHSPointer && isRHSPointer) {
const PointerType *lhsPtr = LHSExpr->getType()->getAs();
Here is another OpenCL AS error.
Comment at: lib/Sema/SemaExpr.cpp:10204
// Treat NULL constant as a special case in OpenCL.
if (getLangOpts().OpenCL && !LHSIsNull && !RHSIsNull) {
const PointerType *LHSPtr = LHSType->getAs();
And another.
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
leonardchan created this revision.
leonardchan added reviewers: phosek, mcgrathr, rsmith.
leonardchan added a project: clang.
Compiling the following causes clang to crash
void cmp(char *x, __attribute__((address_space(2))) char *y) {
0 ? 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 ensures that an AddressSpaceConversion is used instead while retaining
the `pointer type mismatch` warnings.
Repository:
rC Clang
https://reviews.llvm.org/D50278
Files:
lib/Sema/SemaExpr.cpp
test/Sema/address_spaces_cond_op.c
test/Sema/address_spaces_cond_op_ir.c
Index: test/Sema/address_spaces_cond_op_ir.c
===
--- /dev/null
+++ test/Sema/address_spaces_cond_op_ir.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -S -emit-llvm -o - %s | FileCheck %s
+
+#define _AS1 __attribute__((address_space(1)))
+#define _AS2 __attribute__((address_space(2)))
+
+char *cmp(_AS1 char *x, _AS2 char *y) {
+ return x < y ? x : y;
+}
+
+// CHECK: %x.addr = alloca i8 addrspace(1)*, align 8
+// CHECK-NEXT: %y.addr = alloca i8 addrspace(2)*, align 8
+// CHECK-NEXT: store i8 addrspace(1)* %x, i8 addrspace(1)** %x.addr, align 8
+// CHECK-NEXT: store i8 addrspace(2)* %y, i8 addrspace(2)** %y.addr, align 8
+// CHECK-NEXT: %0 = load i8 addrspace(1)*, i8 addrspace(1)** %x.addr, align 8
+// CHECK-NEXT: %1 = load i8 addrspace(2)*, i8 addrspace(2)** %y.addr, align 8
+// CHECK-NEXT: %2 = addrspacecast i8 addrspace(2)* %1 to i8 addrspace(1)*
Index: test/Sema/address_spaces_cond_op.c
===
--- /dev/null
+++ test/Sema/address_spaces_cond_op.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -ast-dump %s | FileCheck %s
+
+#define _AS1 __attribute__((address_space(1)))
+#define _AS2 __attribute__((address_space(2)))
+
+char *cmp(_AS1 char *x, _AS2 char *y) {
+ return x < y ? x : y;
+}
+
+// CHECK: |-ImplicitCastExpr {{.*}} 'void *'
+// CHECK-NEXT: | `-ImplicitCastExpr {{.*}} '__attribute__((address_space(1)))
char *'
+// CHECK-NEXT: | `-DeclRefExpr {{.*}} '__attribute__((address_space(1)))
char *' {{.*}} 'x' '__attribute__((address_space(1))) char *'
+
+// CHECK: `-ImplicitCastExpr {{.*}} 'void *'
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}} '__attribute__((address_space(2)))
char *'
+// CHECK-NEXT: `-DeclRefExpr {{.*}} '__attribute__((address_space(2))) char
*' {{.*}} 'y' '__attribute__((address_space(2))) char *'
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -6492,13 +6492,11 @@
// 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.
+ LHSCastKind =
+ LAddrSpace == ResultAddrSpace ? CK_BitCast : CK_AddressSpaceConversion;
+ RHSCastKind =
+ RAddrSpace == ResultAddrSpace ? CK_BitCast : CK_AddressSpaceConversion;
if (S.getLangOpts().OpenCL) {
-LHSCastKind = LAddrSpace == ResultAddrSpace
- ? CK_BitCast
- : CK_AddressSpaceConversion;
-RHSCastKind = RAddrSpace == ResultAddrSpace
- ? CK_BitCast
- : CK_AddressSpaceConversion;
lhQual.removeAddressSpace();
rhQual.removeAddressSpace();
}
Index: test/Sema/address_spaces_cond_op_ir.c
===
--- /dev/null
+++ test/Sema/address_spaces_cond_op_ir.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -S -emit-llvm -o - %s | FileCheck %s
+
+#define _AS1 __attribute__((address_space(1)))
+#define _AS2 __attribute__((address_space(2)))
+
+char *cmp(_AS1 char *x, _AS2 char *y) {
+ return x < y ? x : y;
+}
+
+// CHECK: %x.addr = alloca i8 addrspace(1)*, align 8
+// CHECK-NEXT: %y.addr = alloca i8 addrspace(2)*, align 8
+// CHECK-NEXT: store i8 addrspace(1)* %x, i8 addrspace(1)** %x.addr, align 8
+// CHECK-NEXT: store i8 addrspace(2)* %y, i8 addrspace(2)** %y.addr, align 8
+// CHECK-NEXT: %0 = load i8 addrspace(1)*, i8 addrspace(1)** %x.addr, align 8
+// CHECK-NEXT: %1 = load i8 addrspace(2)*, i8 addrspace(2)** %y.addr, align 8
+// CHECK-NEXT: %2 = addrspacecast i8 addrspace(2)* %1 to i8 addrspace(1)*
Index: test/Sema/address_spaces_cond_op.c
===
--- /dev/null
+++ test/Sema/address_spaces_cond_op.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -ast-dump %s | FileCheck %s
+
+#define _AS1 __attribute__((address_space(1)))
+#define _AS2 __attribute__((address_space(2)))
+
+char *cmp(_AS1 char *x, _AS2 char *y) {
+ return x < y ? x : y;
+}
+
+// CHECK: |-ImplicitCastExpr {{.*}} 'void *'
+// CHECK-NEXT: |
