[PATCH] D58236: Make address space conversions a bit stricter.

This revision was automatically updated to reflect the committed changes. Closed by commit rL360258: [Sema][OpenCL] Make address space conversions a bit stricter. (authored by stulova, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a comment. In D58236#1494722 , @ebevhan wrote: > This was accepted a while ago, but never landed. I don't have commit access; > could someone commit it? Sure! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan added a comment. This was accepted a while ago, but never landed. I don't have commit access; could someone commit it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 ___ cfe-commits

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall accepted this revision. rjmccall added a comment. Alright, this seems reasonable to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 ___ cfe-commits mailing list

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan updated this revision to Diff 195520. ebevhan edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 Files: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaCast.cpp

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan added a comment. In D58236#1467151 , @Anastasia wrote: > In D58236#1466263 , @ebevhan wrote: > > > Well, it doesn't seem to me like there is consensus on prohibiting nested > > address space conversion

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a comment. In D58236#1466263 , @ebevhan wrote: > Well, it doesn't seem to me like there is consensus on prohibiting nested > address space conversion like this. > > I can simply redo the patch to only include the bugfix on implicit >

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan added a comment. Well, it doesn't seem to me like there is consensus on prohibiting nested address space conversion like this. I can simply redo the patch to only include the bugfix on implicit conversions and drop the nesting level checks. CHANGES SINCE LAST ACTION

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added a comment. In D58236#1443556 , @ebevhan wrote: > In D58236#1443082 , @rjmccall wrote: > > > I think C probably requires us to allow this under an explicit cast, but we > > can at least warn about

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan added a comment. In D58236#1443082 , @rjmccall wrote: > I think C probably requires us to allow this under an explicit cast, but we > can at least warn about it. It does not require us to allow this as an > implicit conversion, and that should

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added a comment. I think C probably requires us to allow this under an explicit cast, but we can at least warn about it. It does not require us to allow this as an implicit conversion, and that should be an error. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/

[PATCH] D58236: Make address space conversions a bit stricter.

efriedma added a comment. If nobody else agrees with my position on this, I'm not going to continue arguing on the explicit cast behavior. But please add a testcase showing that at least `(global int**)(void*)(local int**)p` works without an error. CHANGES SINCE LAST ACTION

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a comment. In D58236#1437690 , @ebevhan wrote: > Any more input on this? > > I could redo the patch to simply fix the bug and not make the conversions > stricter, if that's preferable. I was playing a bit with some examples of enum with

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan added a comment. Any more input on this? I could redo the patch to simply fix the bug and not make the conversions stricter, if that's preferable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a comment. In D58236#1417429 , @efriedma wrote: > In D58236#1416765 , @Anastasia wrote: > > > In D58236#1414069 , @efriedma > > wrote: > > > > > > I think

[PATCH] D58236: Make address space conversions a bit stricter.

efriedma added a comment. In D58236#1416765 , @Anastasia wrote: > In D58236#1414069 , @efriedma wrote: > > > > I think trying to reject code that is doing something dangerous is a good > > > thing! > > > >

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a comment. In D58236#1414069 , @efriedma wrote: > > I think trying to reject code that is doing something dangerous is a good > > thing! > > Refusing to compile code which is suspicious, but not forbidden by the > specification, will

[PATCH] D58236: Make address space conversions a bit stricter.

efriedma added a comment. > I think trying to reject code that is doing something dangerous is a good > thing! Refusing to compile code which is suspicious, but not forbidden by the specification, will likely cause compatibility issues; there are legitimate reasons to use casts which look

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a comment. > Along those lines, in general, the normal C rules should allow casting `foo*` > to `bar*` for any object types foo and bar, even if foo and bar are pointers > with address spaces, like `__local int *` and `__global int *`. I don't see > anything in the OpenCL

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan added a comment. In D58236#1412267 , @Anastasia wrote: > LGTM! Thanks a lot for fixing this old bug! Btw, do you plan to look at > generalizing this to C++ as well? That does sound like a good idea and I will probably look into it when I have

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added inline comments. Comment at: test/SemaOpenCL/address-spaces.cl:87 + + // FIXME: This doesn't seem right. This should be an error, not a warning. + __local int * __global * __private * lll; Anastasia wrote: > ebevhan wrote: > > Anastasia wrote: >

[PATCH] D58236: Make address space conversions a bit stricter.

efriedma added a comment. Generally, with an explicit cast, C allows any pointer cast with a reasonable interpretation, even if the underlying operation is suspicious. For example, you can cast an "long*" to a "int*" (as in "(int*)(long*)p") without any complaint, even though dereferencing

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks a lot for fixing this old bug! Btw, do you plan to look at generalizing this to C++ as well? I don't feel we need anything for mixing OpenCL with GNU style address spaces

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: test/SemaOpenCL/address-spaces.cl:87 + + // FIXME: This doesn't seem right. This should be an error, not a warning. + __local int * __global * __private * lll; Anastasia

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan updated this revision to Diff 188504. ebevhan marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 Files: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaCast.cpp

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added inline comments. Comment at: lib/Sema/SemaExpr.cpp:7795 +if (lhq.getAddressSpace() != rhq.getAddressSpace()) + return Sema::IncompatibleNestedPointerDiscardsQualifiers; + I am wondering since the behavior is so specialized for

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan marked 2 inline comments as done. ebevhan added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996 + "|%diff{casting $ to type $|casting between types}0,1}2" + " changes address space of nested pointer">; def

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan updated this revision to Diff 187531. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 Files: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaCast.cpp lib/Sema/SemaExpr.cpp

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added a subscriber: arsenm. Anastasia added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996 + "|%diff{casting $ to type $|casting between types}0,1}2" + " changes address space of nested pointer">; def

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: test/SemaOpenCL/address-spaces.cl:89 + __local int * __global * __private * lll; + lll = gg; // expected-warning {{incompatible pointer types assigning to '__local int *__global **' from

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan marked 5 inline comments as done. ebevhan added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996 + "|%diff{casting $ to type $|casting between types}0,1}2" + " changes address space of nested pointer">; def

[PATCH] D58236: Make address space conversions a bit stricter.

Anastasia added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996 + "|%diff{casting $ to type $|casting between types}0,1}2" + " changes address space of nested pointer">; def err_typecheck_incompatible_ownership : Error< I am

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan marked 6 inline comments as done. ebevhan added inline comments. Comment at: lib/Sema/SemaCast.cpp:2295 + // FIXME: C++ might want to emit different errors here. if (Self.getLangOpts().OpenCL) { +const Type *DestPtr, *SrcPtr; I'd also like to

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan updated this revision to Diff 187230. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaCast.cpp lib/Sema/SemaExpr.cpp test/CodeGenOpenCL/numbered-address-space.cl

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:2306 + SrcPPointee.getAddressSpace()) || + !DestPPtr->isAddressSpaceOverlapping(*SrcPPtr)) { +Self.Diag(OpRange.getBegin(), This should `if (Nested ?

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: test/SemaOpenCL/address-spaces.cl:89 + __local int * __global * __private * lll; + lll = gg; // expected-warning {{incompatible pointer types assigning to '__local int *__global **' from

[PATCH] D58236: Make address space conversions a bit stricter.

ebevhan created this revision. ebevhan added reviewers: Anastasia, rjmccall. Herald added subscribers: cfe-commits, jdoerfert. Herald added a project: clang. The semantics for converting nested pointers between address spaces are not very well defined. Some conversions which do not really carry