[PATCH] D101701: [nofree] Refine concurrency requirements

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

nhaehnle added a comment.

In D101701#2750507 <https://reviews.llvm.org/D101701#2750507>, @nhaehnle wrote:

> In D101701#2747864 <https://reviews.llvm.org/D101701#2747864>, @jdoerfert wrote:
>
>> `nofree` functions imply `nofree` arguments so it very much makes a difference if we make it harder to infer
>> and annotate the former. As we already moved to a more semantic-based definition of escaped we can actually
>> look at an example where this makes a difference. Take pointers passed via memory to see how you cannot just
>>  move the `nofree` to account for the change made by this patch:
>>
>>   struct { int *a } s;
>>   s.a = malloc()               // this is a store.
>>   nofree_nocapture_only(&s)    // we can potentially show `s.a` is not captured in the function but we cannot
>>                                // annotate that fact as a `nofree` argument without argument promotion, which
>>                                // we cannot always do.
>>   // s.a is still known to be dereferenceable even without `nosync`. The function `nofree` is needed, `nofree`
>>   // argument attributes are not involved.
>
> Is that actually true? Regardless of the nocapture flag, s.a may be passed to another thread which then frees it. (nocapture only applies to &s in the first place, and even then, it does not say anything about **temporary** copies of the pointer that may be passed to other threads.)

I realize now that I was partially wrong about this: the definition of the `nocapture` attribute does say that the function cannot send the pointer to another thread, but the pointer in question is &s. So it still seems to me that in the example, the callee is allowed to send s.a to another thread which then frees it (given today's definition of `nofree`).

If this example does fall like I think it does, is there still an optimization benefit to having the **function attribute** `nofree` as defined today vs. as defined in this patch? As I see it, there were three arguments:

1. Inference doesn't have to worry about synchronization.
2. Other attributes work the same way; however, this is false at least for `argmemonly` and arguably also for `inaccessiblememonly`, except that one presumably never gets inferred?
3. Today's `nofree` enables annotations and optimizations that the proposed change would no longer allow. I can't say for certain whether this is false, but at least the examples given so far seem to fail (the first one because you can just use the `nofree` **argument attribute** instead, and the second is discussed above in this comment).

That leaves as a single argument the complexity of inference, but how strong of an argument is that, really?

P.S.: The example I have in mind where confusion about `argmemonly` can be used to trigger a miscompile is at https://godbolt.org/z/dEG8n6W3E. Current main merges the two loads from %q, which is incorrect since `@setup` could arrange for a second thread to be created which changes its value in between.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701