[PATCH] D101701: [nofree] Refine concurrency requirements

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> Hi Johannes,
>
> before I give a more complete response, I'd like to understand your position more fully. To that end:
>
> 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 am not aware of a way to pass to another thread that does not capture a pointer. Please feel free to show me one.

>> In addition to the examples I brought up that are impacted, let me reiterate what I said in my first post:
>> Other attributes work in the same composable way:
>>
>>   void foo(int * __restrict__ flag, char* p) {
>>     argmemonly(flag);                 // Says argmemonly and p is not passed nor does p alias flag, still p is potentially written.
>>                                       // This is like a call to a `nofree` function which might still free pointers via synchronization.
>>                                       // `nosync` is a orthogonal component to these attributes as we can otherwise not derive them.
>>                                       // However, as with `nofree`, we can use the attributes even without `nosync` in some situations.
>>   }
>
> I don't think this is true. LangRef certainly doesn't support that claim as far as I can tell (the definition of argmemonly talks about there being no side effects at all, without any exception for side effects triggered via other threads), and it turns out that due to confusion about this in the attributor, I can actually provoke a miscompilation fairly easily. So there may be a bigger issue at play there. If you do think that there is consensus and/or documentation about argmemonly working the way you think it should work, can you please point to it?

So you are arguing `armemonly` is also implying `nosync`? Then we get into the same problems we have here. You don't actually gain expressiveness, after all you can check if both attribute `X` and `nosync` are present, but you loose the ability to derive and utilize them separately. One conceptual thing to consider is that almost all of this predates `nosync` in the first place. That said, I don't see how you would interpret the text as there are no side effects by other threads:

  argmemonly
  
      This attribute indicates that the only memory accesses inside function are loads and stores from objects pointed to by its pointer-typed arguments, with arbitrary offsets. Or in other words, all memory operations in the function can refer to memory only using pointers based on its function arguments.
  
      Note that argmemonly can be used together with readonly attribute in order to specify that function reads only from its arguments.
  
      If an argmemonly function reads or writes memory other than the pointer arguments, or has other side-effects, the behavior is undefined.

What in the above definition prevents me from doing this:

  void foo(atomic flag) { atomic_write(flag, 1); while(atomic_read(flag) == 1); atomic_write(flag, 2); }

Inside the function all memory that is accesses are loads/stores from objects pointed by pointer-type arguments (with 0 offset).  Do you disagree?

FWIW, this basically breaks down with stuff like `malloc` which we assume is `inaccessiblememonly` but it is in fact *not*, and cannot be, `nosync`. So if we require attributes to imply `nosync`, `malloc` cannot be `inaccessiblememonly` anymore. (see https://reviews.llvm.org/D98605#2625243)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701