[PATCH] D60047: [CaptureTracking] Don't let comparisons against null escape inbounds pointers

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D60047#1491532 <https://reviews.llvm.org/D60047#1491532>, @sanjoy wrote:

> In D60047#1491414 <https://reviews.llvm.org/D60047#1491414>, @jdoerfert wrote:
>
> > Should we interpret your comment as "the reasoning seems sound", or did you only want to point out the special case which I discuss below?
>
>
> You should interpret it as a drive by comment. :)  I'm fine if you've thought about the case I suggest and are convinced that this change is sound nevertheless.


These things are always tricky. It's better if multiple ppl think about the corner cases and implications :)

>>> @sanjoy wrote:
>>>  However, this example is kind of iffy because in general the `deref_or_null` attribute is not valid since `p` in `main` could have been something other than `-4`.  But *if* the execution has `p` == `-4` then the attribute is well defined.
>> 
>> I thought about this case, though I didn't spell it out explicitly in my earlier comment (the part: //as the addition "with infinitely precise signed arithmetic" would not overflow//).
>> 
>> Let me start by stating that I interpret pointers as unsigned values which, under certain conditions, are allowed to overflow. (Maybe that is in itself a problem in my thinking.)
>>  So for me `p` is not `-4` but always some positive value, probably `2^N - 1 - 4` where `N` is the pointer bit-width. If that is acceptable, then the inbounds wording in the lang ref
>>  prevents the problem you describe. Any addition with a positive value, e.g., the one you choose `(2^N - 1 - 4) + 4`, cannot be `null` without an overflow and inbounds prohibits this overflow.
> 
> I'm not sure if inbounds play into this, and I also noticed my example has typos and also is needlessly confusing; corrected example below:
> 
>   int** global_ptr;
>   
>   void unknown_func();
>   
>   void foo(int32* /*deref_or_null(4)*/ ptr) {
>     if (ptr == null) {
>       int32* ptr_leaked = (int32*)(intptr_t)-4;
>       *global_ptr = ptr_leaked;
>     }
>   }
>   
>   void bar() {
>     int32* p = new int32;
>     // p happens to numerically be -4 == 2^64-4
>     int32* p_with_offset = p + 1; // non-inbounds GEP, evaluates to null
>     foo(p_with_offset);
>   
>     // p has been escaped and we can't DCE the first store:
>     *p = 10;
>     unknown_func();
>     *p = 20;
>   }
> 
> 
> Now lets say we're only looking at `foo`.  I think the current code will conclude that `foo` does not capture `ptr` (we're comparing a `deref_or_null` pointer against `null`) and we will (lets assume) mark `ptr` as `nocapture` which I believe is incorrect, modulo the iffy bit I mentioned before.

I see your point. My initial argumentation is problematic here: "//If Ptr is null to begin with, we are fine (I think).//"
Due to the information leakage when a modified pointer is compared to `null`, which can actually be `null`, capturing is possible. Though, one basically has to guess the pointer. Idk. if it is useful/problematic to treat that as no-capture. If it is problematic, then we need to require `dereferenceable` (without the `or_null` part).

Btw. GEP inbounds (with a non-null offset) transition from `dereferenceable_or_null` to `dereferenceable`, though idk if there is already logic for that upstream.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60047