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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: docs/LangRef.rst:1460
+    environments where the function might communicate the pointer to another thread
+    which then deallocates the memory).
 ``noimplicitfloat``
----------------
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.


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:271
+  return getAllocationData(V, ReallocLike, TLI, LookThroughBitCast).hasValue();
+}
+
----------------
See my comment at the use.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1194
   case Attribute::ShadowCallStack: return 1ULL << 59;
+  case Attribute::NoFree:          return 1ULL << 60;
   case Attribute::SpeculativeLoadHardening:
----------------
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`.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1228
+  StringRef FnName = Callee->getName();
+  if (TLI.getLibFunc(FnName, TLIFn))
+    return false;
----------------
efriedma wrote:
> hfinkel wrote:
> > 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?
> > 
> 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.
I also think we should move deduction logic for library functions to `inferLibFuncAttributes` and annotate intrinsics properly.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1242
+  if (isFreeCall(&I, &TLI) || isReallocLikeFn(&I, &TLI))
+    return true;
+
----------------
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".


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1244
+
+  if (!isa<CallInst>(I) && !isa<InvokeInst>(I))
+    return false;
----------------
Check `if(!CS)` (below) instead of these `isa's` which are not exhaustive.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1253
+  if (Callee->doesNotFreeMemory())
+    return false;
+
----------------
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.


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

https://reviews.llvm.org/D49165