@llvm/pr-subscribers-llvm-ir

Author: Nikita Popov (nikic)

diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 912cca72e1ad5..57e5743dde39e 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1543,8 +1543,11 @@ Currently, only the following parameter attributes are defined:
       is null is captured in some other way.
 
 ``nofree``
-    This indicates that the callee does not free the pointer argument. This is not
-    a valid attribute for return values.
+    This indicates that the callee does not free the pointer argument, or
+    cause it to be freed through synchronization.
+
+    This is not a valid attribute for return values. This attribute applies
+    only to the particular copy of the pointer passed in this argument.
 
 .. _nest:
 
@@ -2328,20 +2331,17 @@ For example:
 ``nofree``
     This function attribute indicates that the function does not, directly or
     transitively, call a memory-deallocation function (``free``, for example)
-    on a memory allocation which existed before the call.
+    on a memory allocation which existed before the call, or make such a call
+    visible through synchronization.
 
-    As a result, uncaptured pointers that are known to be dereferenceable
-    prior to a call to a function with the ``nofree`` attribute are still
-    known to be dereferenceable after the call. The capturing condition is
-    necessary in environments where the function might communicate the
-    pointer to another thread which then deallocates the memory.  Alternatively,
-    ``nosync`` would ensure such communication cannot happen and even captured
-    pointers cannot be freed by the function.
+    As a result, pointers that are known to be dereferenceable prior to a call
+    to a function with the ``nofree`` attribute are still known to be
+    dereferenceable after the call.
 
     A ``nofree`` function is explicitly allowed to free memory which it
-    allocated or (if not ``nosync``) arrange for another thread to free
-    memory on its behalf.  As a result, perhaps surprisingly, a ``nofree``
-    function can return a pointer to a previously deallocated
+    allocated or arrange for another thread to free such memory on its behalf.
+    As a result, perhaps surprisingly, a ``nofree`` function can return a
+    pointer to a previously deallocated
     :ref:`allocated object<allocatedobjects>`.
 ``noimplicitfloat``
     Disallows implicit floating-point code. This inhibits optimizations that

I'd recommend defining nofree for pointer arguments in terms of provenance: if you put nofree on an argument, the provenance of that pointer is not used to free the pointer. Seems pretty easy to reason about: if nothing else has the provenance, the pointer won't be freed; if something else has the provenance, all bets are off. Pairs well with captures(none) or captures(address).

Yeah, defining it in terms of provenance makes a lot of sense.

I think you need to take a non-provenance approach for whole-function nofree, though. Your current definition might be okay? Hard to infer, though; it excludes any form of atomic operation (including "monotonic" operations).

For the monotonic operations, I guess the concern is around something like this?

T1:
shared_loc.store_monotonic(p)
while (shared_loc.load_monotonic()) { /* spin */ }

T2:
loop {
   p = shared_loc.load_monotonic()
   if (p) {
     free(p)
     shared_loc.store_monotonic(nullptr)
   }
}

And then T1 cannot be nofree?

We currently use the combination of nofree + nosync to determine that it's "really nofree" and nosync excludes monotonic (though in terms of implementation, we treat this inconsistently between Attributor and FunctionAttrs).

nosync excludes monotonic

You just rewrote the definition of nosync so that it explicitly references the C++ "synchronize with"... and monotonic operations can't create such an edge on their own, I think. (You can pair a monotonic operation with a fence, but the edge is defined in terms of the fences.)

Thinking about it a bit more, I guess it's actually fine for a nofree function to contain monotonic stores? Monotonic stores do weird things to causality; a wait loop doesn't actually form an edge like you might expect.

Probably want to try to use the C++ vocabulary in the definition, though. "make such a call visible through synchronization" doesn't map directly to anything. Maybe what we want is something like "any call to free which happens-before the function returns also happens-before the function entry".

nosync excludes monotonic

You just rewrote the definition of nosync so that it explicitly references the C++ "synchronize with"... and monotonic operations can't create such an edge on their own, I think.

Sorry, what I wrote there was very unclear. I meant "excludes" as in "does not cover" not as in "prohibits".

Thinking about it a bit more, I guess it's actually fine for a nofree function to contain monotonic stores? Monotonic stores do weird things to causality; a wait loop doesn't actually form an edge like you might expect.

Okay, then we should be good. That matches our existing modelling.

I've tried to update the wording based on feedback.

I guess the argument nofree is still somewhat ambiguous in how "during the execution of the function" interacts with synchronization. This is probably something we should be defining more generally somewhere, because we have lots of attributes with function-local effects.

Argument-level nofree is now specified to only apply to the particular pointer (similar to captures etc). The underlying object may still be freed via a different argument to which the same pointer is passed. (I'm open to changing this, but that would essentially limit inference to noalias arguments.)

So, given:

define void @callee(ptr nofree %a, ptr %b) {
  call void @free(ptr %b)
  ret void
}

We are saying that nofree only constrains frees performed through a pointer derived from %a, regardless of whether %a and %b happen to alias, correct? I wonder if the alternative would be more correct (free(%b) UB if they alias), though I guess this would be not particularly useful. Would this be primarily helpful to optimizations on the caller side (i.e., we would like to conclude that nothing derived from %a is free'd?).

So, given:

define void @callee(ptr nofree %a, ptr %b) {
  call void @free(ptr %b)
  ret void
}

We are saying that nofree only constrains frees performed through a pointer derived from %a, regardless of whether %a and %b happen to alias, correct?

Yes, this is correct. The above code is well-defined.

I wonder if the alternative would be more correct (free(%b) UB if they alias), though I guess this would be not particularly useful. Would this be primarily helpful to optimizations on the caller side (i.e., we would like to conclude that nothing derived from %a is free'd?).

For optimization inside the callee, effectively only the combination of noalias nofree is useful. For optimization inside the caller (and inference), nofree can be useful in isolation.

Some years ago we also discussed the possibility of introducing another attribute like nofreeobj which is stronger than nofree and does not allow freeing the underlying object even through aliasing pointers. This means it's usable in isolation inside the function, but on the other hand it can't really be inferred for non-noalias pointers.

I believe the noalias nofree combination covers pretty much exactly the use cases needed in Rust -- in particular, the two cases that are not noalias (& !Freeze and &mut !Unpin) are also the ones that are not nofree. So we wouldn't have a use case for something like nofreeobj at this time.

Which makes me realize why, among other things, the old spec was not addressing / engaging with synchronization at all. Just wondering, could it make sense to mirror the function-level happens-before formulation for the argument attribute as well, e.g., a free through a pointer based on this argument on another thread violates argument nofree only if it happens-before the function exit and does not happens-before the function entry, or might it be too contrived? I think the temporal / informal form (until function returns) is still fine though.

Done, though I omitted the part about happens-before the function entry, as the pointer/provenance is only created on function entry.

TBH I'm not entirely happy with this definition of "well-defined if not (free happens-before return)". I think for the provenance-based argument attribute, a more natural definition would have been "well-defined if return happens-before free", because this allows us to determine whether UB occurs at the point of the free. But this definition would be incompatible with the function-level nofree, which is also undesirable, because it prevents cross-inference.