[PATCH] D49165: Add, and infer, a nofree function attribute

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: docs/LangRef.rst:1439
+    indirectly, call a memory-deallocation function (free, for example). 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
----------------
efriedma wrote:
> The "uncaptured" bit makes this useless for D48239; we have to assume any argument that isn't noalias is captured.
I agree. In the RFC I just sent out a few seconds ago, I proposed a different solution (to also have a nosynch attribute).


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1228
+  StringRef FnName = Callee->getName();
+  if (TLI.getLibFunc(FnName, TLIFn))
+    return false;
----------------
efriedma wrote:
> 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.
> 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?



Repository:
  rL LLVM

https://reviews.llvm.org/D49165