[PATCH] D64258: [InferFuncAttributes] extend 'dereferenceable' attribute based on loads

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D64258#1579501 <https://reviews.llvm.org/D64258#1579501>, @spatel wrote:

> In D64258#1579308 <https://reviews.llvm.org/D64258#1579308>, @jdoerfert wrote:
>
> > In D64258#1579214 <https://reviews.llvm.org/D64258#1579214>, @jfb wrote:
> >
> > > >> Also, would it make sense to separate readable from writable? We currently have this bug where LLVM will promote all const static globals to rodata, and sometimes generate atomic cmpxchg to them (e.g. because we're trying to load a 128-bit value). Similarly, we might want to honor R / W memory protection in general. Right now dereferenceable just means "you can load from this", because we can't speculate most stores.
> > > > 
> > > > I do not understand the problem but I have the feeling this is an orthogonal issue.
> > >
> > > `mprotect` can make memory readable but not writable, or writable but not readable... or neither. What does `dereferenceable` mean when faced with this fact? Further, what happens to `dereferenceable` when `mprotect` is called (any opaque function could call it)? I don't think this is an orthogonal problem at all.
> >
> >
> > So, I guess what the above means is "dereferenceable" is too coarse grained. We have "global dereferenceability" that cannot be changed, and we have "local dereferenceability" that can be changed, e.g., through calls to `free`, `realloc`, or `mprotect`. From accesses we can only deduce "local dereferenceability". Now, that is why we need D61652 <https://reviews.llvm.org/D61652>, or more precisely, D63243 <https://reviews.llvm.org/D63243>. After those changes landed, the reasoning introduced in this patch should be fine, before, it is as broken as Clang is when it emits `dereferenceable` for arguments passed by reference. (The logic above, with the same problems and more, is also used in ArgumentPromotion right now...).
>
>
> So we are saying that the current attribute is too vague/broken to be useful? Ie, this patch must be abandoned?


The current situation is broken but works "so far". Adding this will expose the broken part further, that is the reason why I started to fix the situation in the first place ;)
That being said, I think the patch is "fine", at the latest after D63243 <https://reviews.llvm.org/D63243> is in.

Now for timeline, in case you want to avoid exposing the problem with this patch:
I hope to land D61652 <https://reviews.llvm.org/D61652> this week, after I can test the latest update of D63243 <https://reviews.llvm.org/D63243> which depends on `nosync`. D63243 <https://reviews.llvm.org/D63243> would then land a week or so later, giving front-end people time to update from `dereferenceable` to `dereferenceable_globally` if they actually want that behavior.


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

https://reviews.llvm.org/D64258