Page MenuHomePhabricator

[nofree] Restrict semantics to memory visible to caller
ClosedPublic

Authored by reames on Apr 8 2021, 2:34 PM.

Details

Summary

This patch clarifies the semantics of the nofree function attribute to make clear that it provides an "as if" semantic. That is, a nofree function is guaranteed not to free memory which existed before the call, but might allocate and then deallocate that same memory within the lifetime of the callee.

This is the result of the discussion on llvm-dev under the thread "Ambiguity in the nofree function attribute".

The most important part of this change is the LangRef wording. The rest is minor comment changes to emphasize the new semantics where code was accidentally consistent, and fix one place which wasn't consistent. That one place is currently narrowly used as it is primarily part of the ongoing (and not yet enabled) deref-at-point semantics work.

Diff Detail

Event Timeline

reames created this revision.Apr 8 2021, 2:34 PM
reames requested review of this revision.Apr 8 2021, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 2:34 PM
reames abandoned this revision.Apr 8 2021, 2:36 PM

Realized the alternative was strictly more in sync with existing code, so disregard this.

reames updated this revision to Diff 337263.Tue, Apr 13, 2:55 PM
reames edited the summary of this revision. (Show Details)
reames set the repository for this revision to rG LLVM Github Monorepo.
nlopes accepted this revision.Wed, Apr 14, 3:27 AM

LGTM

This revision is now accepted and ready to land.Wed, Apr 14, 3:27 AM

Thank you for doing this.

The interaction with multi-threading and capturing makes me mildly nervous. Perhaps I'm just confused, but the second paragraph of the definition there seems to imply that a nofree (but not-nosync) function f is allowed to free any memory that had a pointer to it captured somewhere. But this seems to contradict the first paragraph, which says that f "does not, directly or indirectly, call a memory-deallocation function (free, for example) on a memory allocation which existed before the call."

So which is it?

If f communicates to another thread in a way that causes that thread to free memory, does that count as an indirect call to a memory-deallocation function? If not, why does capturing the pointer make a difference? An argument to f could be temporarily passed to another thread even if it is nocapture...

I have a feeling that this confusion already existed in the previous definition.

Suggestion to make the text clearer.

llvm/docs/LangRef.rst
1600–1616

Thank you for doing this.

The interaction with multi-threading and capturing makes me mildly nervous. Perhaps I'm just confused, but the second paragraph of the definition there seems to imply that a nofree (but not-nosync) function f is allowed to free any memory that had a pointer to it captured somewhere. But this seems to contradict the first paragraph, which says that f "does not, directly or indirectly, call a memory-deallocation function (free, for example) on a memory allocation which existed before the call."

That certainly wasn't the intent. Which bit of wording gives that impression?

(See the bit below which is essentially the inverse of this case, and is intention.)

So which is it?

If f communicates to another thread in a way that causes that thread to free memory, does that count as an indirect call to a memory-deallocation function? If not, why does capturing the pointer make a difference? An argument to f could be temporarily passed to another thread even if it is nocapture...

To the best of my reading of the current code and specification, no having another thread free an object on the behalf of 'f' does not violate a nofree annotation on 'f'. The reasoning here is that a) 'f' is not the one actually freeing, and b) it we picked anything else as a semantic, inferring nofree would require concurrency aware full program analysis.

You can divide the above into two cases:

  1. The object has already been captured before the call.
  2. The object is captured by the call.

Having some other thread free the captured object in case 1 is clearly allowed. Case 2 appears not the have been considered in the current wording from what I can tell, and probably needs further consideration. I do request we separate that into a separate patch though.

I have a feeling that this confusion already existed in the previous definition.

reames marked an inline comment as done.Fri, Apr 16, 11:15 AM

@jdoerfert - Thanks for the wording suggestions; I took all of them.

jdoerfert accepted this revision.Fri, Apr 16, 11:26 AM

LGTM, @nhaehnle should comment on the new wording probably.

LGTM, @nhaehnle should comment on the new wording probably.

JFYI, I don't intend to hold for @nhaehnle as the concern he raised appears orthogonal to the change being made in this review. I am happy to continue this discussion and post another patch if we thing further clarification is warranted for the concurrency case. I am leaning in that direction myself.

Doing a final rebuild now, and will submit after that.

This revision was landed with ongoing or failed builds.Fri, Apr 16, 11:39 AM
This revision was automatically updated to reflect the committed changes.

Posted https://reviews.llvm.org/D100676 with an attempt to clarifying the nocapture case raised in my response to @nhaehnle above.

I've updated Alive2 with the new semantics and I see one regression:
llvm/test/Transforms/InstCombine/malloc-free-delete.ll

define void @test14(* %foo) nofree {
  free * %foo
  ret void
}
=>
define void @test14(* %foo) nofree {
  call void #trap() nowrite noreturn
  assume i1 0
}

Transformation doesn't verify!

ERROR: Source is more defined than target

Example:
* %foo = null

free(null) is a no-op, so I think the test case is buggy. This transformation can only be done if the argument is non-null.

I've updated Alive2 with the new semantics and I see one regression:
llvm/test/Transforms/InstCombine/malloc-free-delete.ll

define void @test14(* %foo) nofree {
  free * %foo
  ret void
}
=>
define void @test14(* %foo) nofree {
  call void #trap() nowrite noreturn
  assume i1 0
}

Transformation doesn't verify!

ERROR: Source is more defined than target

Example:
* %foo = null

free(null) is a no-op, so I think the test case is buggy. This transformation can only be done if the argument is non-null.

Right, the transformation should be llvm.assume(%foo == null).

I've updated Alive2 with the new semantics and I see one regression:

Ok, wow. Thank you!

This patch and the alive2 tooling combined just paid off big time.

In the background, I've been trying to figure out a miscompile (https://github.com/emscripten-core/emscripten/issues/9443), and I'm pretty sure this exactly the issue. Or at least, it seems pretty likely.

I will be posting a fix for the issue identified here later today.

Thanks again!

Proposed fix for the issue Nuno found here: https://reviews.llvm.org/D100779