[PATCH] D101701: [nofree] Refine concurrency requirements

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

nhaehnle added a comment.

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

> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701