This is an archive of the discontinued LLVM Phabricator instance.

Add, and infer, a nofree function attribute
ClosedPublic

Authored by homerdin on Jul 10 2018, 6:29 PM.

Details

Summary

This patch adds a function attribute, nofree, to indicate that a function does not, directly or indirectly, call a memory-deallocation function (e.g., free, C++'s operator delete).

The primary motivation for adding this attribute some from the discussion about how to fix the semantics, or the use of, the dereferenceable attribute on C++ references passed as function parameters. The problem is that, while a reference argument is known to be dereferenceable when the function starts executing, nothing prevents the function from freeing the memory. As a result, it is wrong to assume that the pointer is dereferenceable over the entire body of the function unless we can rule out a situation where the memory might be deallocated during the course of the function's execution. For more information, please see the discussion in https://reviews.llvm.org/D48239.

For more information, see the RFC.

Diff Detail

Repository
rL LLVM

Event Timeline

hfinkel created this revision.Jul 10 2018, 6:29 PM
efriedma added inline comments.Jul 10 2018, 6:59 PM
docs/LangRef.rst
1439 ↗(On Diff #154913)

The "uncaptured" bit makes this useless for D48239; we have to assume any argument that isn't noalias is captured.

lib/Transforms/IPO/FunctionAttrs.cpp
1228 ↗(On Diff #154913)

This seems delicate; there are some C library functions which aren't "free", but can still free memory (like mmap). Not sure we recognize any functions like that at the moment, but we could add them. I'd prefer to add explicit logic to inferLibFuncAttributes.

If you are going to call getLibFunc, use the overload that takes a CallSite.

hfinkel added inline comments.Jul 10 2018, 7:07 PM
docs/LangRef.rst
1439 ↗(On Diff #154913)

I agree. In the RFC I just sent out a few seconds ago, I proposed a different solution (to also have a nosynch attribute).

lib/Transforms/IPO/FunctionAttrs.cpp
1228 ↗(On Diff #154913)

This seems delicate; there are some C library functions which aren't "free", but can still free memory (like mmap). Not sure we recognize any functions like that at the moment, but we could add them. I'd prefer to add explicit logic to inferLibFuncAttributes.

This also makes me uneasy. On the other hand, maintaining a separate list of functions which don't free memory, which is nearly all of them, also seems bad. My best suggestion is to add a strongly-worded note to include/llvm/Analysis/TargetLibraryInfo.def about what to do if you add such a function that frees memory. Would that be okay?

efriedma added inline comments.Jul 11 2018, 11:31 AM
lib/Transforms/IPO/FunctionAttrs.cpp
1228 ↗(On Diff #154913)

I guess it's okay to default all known functions which aren't free-like/realloc-like to infer nofree, with an appropriate comment.

I'd still prefer to move the code to infer nofree on library calls to inferLibFuncAttributes.

Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2019, 9:48 AM
Herald added a subscriber: jvesely. · View Herald Transcript
uenoku added a subscriber: uenoku.May 7 2019, 11:50 PM
homerdin commandeered this revision.Jun 12 2019, 1:07 PM
homerdin added a reviewer: hfinkel.
homerdin added a subscriber: homerdin.

Going to rebase this.

jdoerfert added inline comments.Jun 13 2019, 9:06 AM
docs/LangRef.rst
1460 ↗(On Diff #204348)

The question now is, do we want nofree to be orthogonal to nosync or not? I'd prefer yes but it can confuse people so we should add language here to make that explicit.

lib/Analysis/MemoryBuiltins.cpp
271 ↗(On Diff #204348)

See my comment at the use.

lib/Bitcode/Reader/BitcodeReader.cpp
1194 ↗(On Diff #204348)

I don't think you can do this. All existing attributes have to keep their "encoding" and new ones get the next free number which has to match include/llvm/Bitcode/LLVMBitCodes.h.

lib/Transforms/IPO/FunctionAttrs.cpp
1228 ↗(On Diff #154913)

I also think we should move deduction logic for library functions to inferLibFuncAttributes and annotate intrinsics properly.

1242 ↗(On Diff #204348)

Why is this needed? The below logic should be sufficient because we want to check for "must not free" and therefore it doesn't matter if a function is "must-free" or "may-free".

1244 ↗(On Diff #204348)

Check if(!CS) (below) instead of these isa's which are not exhaustive.

1253 ↗(On Diff #204348)

I'd like to "simply" ask the call site for nofree and doesNotReadMemory, or make doesNotFreeMemory a CallBase method with redirects in CallSite. That allows to annotate the call site directly.

@homerdin
I want to use nofree enum attribute in D62687. If it's OK with you, can I take over this patch?

@uenoku Sure. I'm working on moving the logic for library functions to inferLibFuncAttributes, but I can add that in later.

homerdin updated this revision to Diff 205113.Jun 17 2019, 10:25 AM

I moved the logic for library functions to inferLibFuncAttributes, added a note to TargetLibraryInfo.def and updated the tests

homerdin marked 3 inline comments as done.Jun 18 2019, 7:54 AM
jdoerfert accepted this revision.Jul 3 2019, 9:19 AM

Except the TLI related changes (see below) I think this looks good now.

lib/Transforms/IPO/FunctionAttrs.cpp
1521 ↗(On Diff #205113)

TLI should be unused in this file and all related changes should be undone.

This revision is now accepted and ready to land.Jul 3 2019, 9:19 AM
This revision was automatically updated to reflect the committed changes.