[PATCH] D100141: [nofree] Restrict semantics to memory visible to caller

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D100141#2691664 <https://reviews.llvm.org/D100141#2691664>, @nhaehnle wrote:

> Thank you for doing this.
>
> The interaction with multi-threading and capturing makes me mildly nervous. Perhaps I'm just confused, but the second paragraph of the definition there seems to imply that a `nofree` (but not-nosync) function `f` is allowed to free any memory that had a pointer to it captured somewhere. But this seems to contradict the first paragraph, which says that `f` "does not, directly or indirectly, call a memory-deallocation function (free, for example) on a memory allocation which existed before the call."

That certainly wasn't the intent.  Which bit of wording gives that impression?

(See the bit below which is essentially the inverse of this case, and is intention.)

> So which is it?
>
> If `f` communicates to another thread in a way that causes that thread to free memory, does that count as an indirect call to a memory-deallocation function? If not, why does capturing the pointer make a difference? An argument to `f` could be temporarily passed to another thread even if it is `nocapture`...

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.

You can divide the above into two cases:

1. The object has already been captured before the call.
2. The object is captured by the call.

Having some other thread free the captured object in case 1 is clearly allowed.  Case 2 appears not the have been considered in the current wording from what I can tell, and probably needs further consideration.  I do request we separate that into a separate patch though.

> I have a feeling that this confusion already existed in the previous definition.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100141