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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

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".

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100676