[PATCH] D101701: [nofree] Refine concurrency requirements

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

nhaehnle added a comment.
Herald added a subscriber: ormris.

In D101701#2752587 <https://reviews.llvm.org/D101701#2752587>, @jdoerfert wrote:

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

The part of the definition that I would argue prevents you from doing this is the "or has other side-effects".

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

There are two directions in which one can take this.

First, your desired path forward appears to be that `argmemonly` on a callee can only be taken at face value if the callee is also `nosync`. Presumably, you would want the same treatment to apply to `inaccessiblememonly`. So then if `malloc` cannot be `nosync`, what's the point of making it `inaccessiblememonly`? What would that still allow you to deduce that you cannot deduce even without the `inaccessiblememonly`?

Second, perhaps `malloc` can be `nosync`. I see the quote from the C standard, but it's unclear to me whether that language is good for anything other than ensuring no data races in a formal model, and surely there are other means to achieve the same goal, e.g. saying that a re-allocated portion of memory is a different memory location as far as the memory model is concerned. The only way I can see to perform meaningful synchronization through `free` and `malloc` would be to compare the result of malloc with some pointer that was allocated earlier (if they're equal, this implies that the earlier allocation was freed, and we have a synchronization edge that can be relied upon). It may be possible to make this undefined somehow, perhaps involving the non-determinism trick that is proposed for pointer provenance can be applied here as well.

[...]

>> 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).
>
> I think once I manage to explain the examples you will reconsider this point. To give you another one:
>
>   __attribute__((nosync)) // or __attribute__((assume("nosync")) or #pragma omp assume ext_no_synchronization
>   void user_code() {
>     a = malloc();
>     b = malloc();
>     unknown_function_with_a_now_nosync_call_site(a, b);
>     ...
>     free(a);
>   }

Not sure what you're saying here. Do you want the unknown function to be `nofree` but it *does* contain synchronization, except that then the external `nosync` "overrides" that fact and we pray that the programmer knew what they were doing?

>> That leaves as a single argument the complexity of inference, but how strong of an argument is that, really?
>
> I think there are a lot of reasons to keep it separated, the examples above still stand. That said, why would
> we combine it? It saves one attribute lookup?

To me the fundamental problem is having an attribute that says "calling this function doesn't free memory" on a call that does, in fact, free memory. Why would/should the caller care about whether the freeing happens in the same thread or not? I think Nuno's line of argument about separating the semantics from the implementation goes in a similar direction.

This doesn't mean that we can't have the more structural (as opposed to semantic) attribute you want to have, but I'd be in favor of naming it more accurately, e.g. `nothreadlocalfree`.

>> 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.
>
> Agreed this is a bug. Though, it's in instcombine (or probably MemorySSA) not the Attributor. If you disagree you have to tell me which part of the `argmemonly` definition is violated by `release`: "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."

As stated above, it's at the very least the part that says there can be no other side effects. Synchronizing with another thread which then modifies other memory is surely a side effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701