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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D58236#1417429 <https://reviews.llvm.org/D58236#1417429>, @efriedma wrote:

> In D58236#1416765 <https://reviews.llvm.org/D58236#1416765>, @Anastasia wrote:
>
> > In D58236#1414069 <https://reviews.llvm.org/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 likely cause compatibility issues; there are legitimate reasons to use casts which look weird.
> >
> >
> > The spec dioesn't allow these conversions either, it just simply doesn't cover this corner case at all. I don't think we are changing anything in terms of compatibility. If you have any examples of such casts that can be legitimate I would like to understand them better. What I have seen so far were the examples where `addrspacecast` was lost in IR for the memory segments translation and therefore wrong memory areas were accessed.
>
>
> The spec just says that the casts follow C rules... and C says you can cast a pointer to an object type to a pointer to another object type (subject to alignment restrictions).  By default, a pointer to a pointer isn't special.
>
> In practice, unusual casts tend to show up in code building a datastructure using union-like constructs.  In plain C, for example, sometimes you have a pointer to a float, and sometimes you have a pointer to an int, determined dynamically.  I expect similar cases show up where a pointer points to memory which contains either a pointer in the global address-space, or a pointer in the local address-space, determined dynamically.  In some cases, it might be clearer to use void* in more places, but that's mostly style issue.


Ok, do you have any example for pointers with different types by some chance? It would be very helpful because I can try to see what would be the behavior with different address spaces...

One fundamental difference between C and OpenCL C is that we wanted to give more clear semantic to address spaces. It is not possible to casts between pointers of arbitrary address spaces. We wanted to make such conversions very explicit using generic address space. If the code is written using unions (or some other way) that requires such "meaningless" conversions it can still be cast but the cast has to be written using pointer indirection with generic address spaces. That was done deliberately to prevent accidental erroneous patterns to be compiled.

> 
> 
>>> But that's a separate issue, and it needs a proper cost-benefit analysis, including an analysis of the false-positive rate on existing code.
>> 
>> Do you have any suggestions how to do this in practice with such rare corner case?
> 
> If the warning never triggers on any code you have access to, that would still be a useful datapoint.

To my knowledge these corner cases hasn't occurred yet in any code pattern known to me up. I created this upstream bug myself while sorting out some other address space related aspects. However, it's not impossible that it might happen in the future.

I still feel this might be the right direction for OpenCL, do you think this might not be right for C? If that's the case potentially we could add OpenCL specific checks for now and then try to clarify what should be the right strategy for C... it feels in C we just converts between addr spaces freely. The only danger of this conversion with nested pointers is that it produces `bitcast` instead of `addrspacecast`, which might result in accessing the wrong memory. Therefore, we wanted to restrict this in the first place.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58236/new/

https://reviews.llvm.org/D58236