[PATCH] D100676: [nofree] Attempt to further refine concurrency/capture requirements

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D100676#2696511 <https://reviews.llvm.org/D100676#2696511>, @nhaehnle wrote:

> I think the crux of the discussion is the second half of this sentence:
>
>> A `nofree` function is explicitly allowed to free memory which it (or a transitive callee) allocated or (if not `nosync`) arrange for another thread to free memory on its behalf.
>
> I'm trying to figure out whether that second half makes sense by analogy with other attributes. For example, consider a function that is `inaccessiblememonly`. Is such a function allowed to use the inaccessible memory to communicate with another thread which then touches memory that **is** accessible to the caller? I'm pretty sure the assumption in existing LLVM code is that the answer is "no".

The honest truth is that our concurrency specification, and in particular, how it interacts with attributes is lacking to say the least.

The case you raise is an interesting one.  Current code clearly assumes that such writes would not be well defined (note I carefully did not say can't happen), but as you point the specification isn't terrible clear.

I do want to note that inaccessiblememonly is a bit of a weird cornercase (precisely because it's modeling access to things outside the model).  We don't e.g. have a corresponding problem with readonly because synchronization must involve a write.  (We model all ordered atomics as being writes, even the ones which only read.  For exactly this reason.)

Longer term, it might very well be reasonable to allow readonly functions with ordered atomic loads.  We'd simply have to a) specify the change, b) infer nosync, and c) update *all* users to check both attributes.  The value would be in getting stronger aliasing results for uncaptured memory passed to a nocapture argument of a function which only read synchronizes.  Seems like a pretty minimal value.  Note that I'm not actively proposing doing this or volunteering for the work involved.

> But now, the equivalent question for `nofree` is answered "yes".
>
> This inconsistency worries me.
>
> In the original review D10041 <https://reviews.llvm.org/D10041>, @reames wrote:
>
>> To the best of my reading of the current code and specification, no having another thread free an object on the behalf of 'f' does not violate a nofree annotation on 'f'.  The reasoning here is that a) 'f' is not the one actually freeing, and b) it we picked anything else as a semantic, inferring nofree would require concurrency aware full program analysis.
>
> Is that full program analysis really required? Looking again at the analogy with `inaccessiblememonly`, it seems to be conservatively correct to say that a function that may communicate with another thread cannot be `inaccessiblememonly`. For example, a function containing an `atomicrmw` instruction cannot be `inaccessiblememonly`. The same argument should apply to `nofree` as well.
>
> The question is then whether we believe that this would be too conservative for the goal of `nofree`. My gut feeling is that it isn't too conservative, but admittedly I haven't spent a lot of time thinking about it. In any case, there is still the seeming inconsistency between `nofree` and other attributes, which feels likely to cause problems further down the road (e.g., developers forgetting a check against `nosync` somewhere).
>
> Assuming no new and strong arguments to the contrary, I would prefer that `nofree` works like older attributes such as `inaccessiblememonly` in this regard.

I'll be honest, I'm not terrible a fan of nofree and nosync being split the way they are either.  I find it confusing.  However, that was clearly the intent of the original introduction, and nosync serves effectively no other purpose.  If you want to propose (on llvm-dev) a change to this semantic, feel free.  I'm not motivated to do so as what we have works well enough and is a reasonable solution to the problem space.  Not necessarily the best, but a reasonable one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100676