Don't consider allocsize functions to be allocation functions.

This patch fixes some ASAN unittest failures on FreeBSD. See the
cfe-commits email thread for r290169 for more on those.

According to the LangRef, the allocsize attribute only tells us about
the number of bytes that exist at the memory location pointed to by the
return value of a function. It does not necessarily mean that the
function will only ever allocate. So, we need to be very careful about
treating functions with allocsize as general allocation functions. This
patch makes us fully conservative in this regard, though I suspect that
we have room to be a bit more aggressive if we want.

This has a FIXME that can be fixed by a relatively straightforward
refactor; I just wanted to keep this patch minimal. If this sticks, I'll
come back and fix it in a few days.

llvm-svn: 290397

Author: gburgessiv

Diff for: llvm/lib/Analysis/MemoryBuiltins.cpp

@@ -109,25 +109,6 @@ static Optional<AllocFnsTy> getAllocationData(const Value *V, AllocType AllocTy,
   if (!Callee)
     return None;
 
-  // If it has allocsize, we can skip checking if it's a known function.
-  //
-  // MallocLike is chosen here because allocsize makes no guarantees about the
-  // nullness of the result of the function, nor does it deal with strings, nor
-  // does it require that the memory returned is zeroed out.
-  const AllocType AllocSizeAllocTy = MallocLike;
-  if ((AllocTy & AllocSizeAllocTy) == AllocSizeAllocTy &&
-      Callee->hasFnAttribute(Attribute::AllocSize)) {
-    Attribute Attr = Callee->getFnAttribute(Attribute::AllocSize);
-    std::pair<unsigned, Optional<unsigned>> Args = Attr.getAllocSizeArgs();
-
-    AllocFnsTy Result;
-    Result.AllocTy = AllocSizeAllocTy;
-    Result.NumParams = Callee->getNumOperands();
-    Result.FstParam = Args.first;
-    Result.SndParam = Args.second.getValueOr(-1);
-    return Result;
-  }
-
   // Make sure that the function is available.
   StringRef FnName = Callee->getName();
   LibFunc::Func TLIFn;
@@ -163,6 +144,32 @@ static Optional<AllocFnsTy> getAllocationData(const Value *V, AllocType AllocTy,
   return None;
 }
 
+static Optional<AllocFnsTy> getAllocationSize(const Value *V,
+                                              const TargetLibraryInfo *TLI) {
+  // Prefer to use existing information over allocsize. This will give us an
+  // accurate AllocTy.
+  if (Optional<AllocFnsTy> Data =
+          getAllocationData(V, AnyAlloc, TLI, /*LookThroughBitCast=*/false))
+    return Data;
+
+  // FIXME: Not calling getCalledFunction twice would be nice.
+  const Function *Callee = getCalledFunction(V, /*LookThroughBitCast=*/false);
+  if (!Callee || !Callee->hasFnAttribute(Attribute::AllocSize))
+    return None;
+
+  Attribute Attr = Callee->getFnAttribute(Attribute::AllocSize);
+  std::pair<unsigned, Optional<unsigned>> Args = Attr.getAllocSizeArgs();
+
+  AllocFnsTy Result;
+  // Because allocsize only tells us how many bytes are allocated, we're not
+  // really allowed to assume anything, so we use MallocLike.
+  Result.AllocTy = MallocLike;
+  Result.NumParams = Callee->getNumOperands();
+  Result.FstParam = Args.first;
+  Result.SndParam = Args.second.getValueOr(-1);
+  return Result;
+}
+
 static bool hasNoAliasAttr(const Value *V, bool LookThroughBitCast) {
   ImmutableCallSite CS(LookThroughBitCast ? V->stripPointerCasts() : V);
   return CS && CS.paramHasAttr(AttributeSet::ReturnIndex, Attribute::NoAlias);
@@ -505,8 +512,7 @@ SizeOffsetType ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
 }
 
 SizeOffsetType ObjectSizeOffsetVisitor::visitCallSite(CallSite CS) {
-  Optional<AllocFnsTy> FnData =
-      getAllocationData(CS.getInstruction(), AnyAlloc, TLI);
+  Optional<AllocFnsTy> FnData = getAllocationSize(CS.getInstruction(), TLI);
   if (!FnData)
     return unknown();
 
@@ -765,8 +771,7 @@ SizeOffsetEvalType ObjectSizeOffsetEvaluator::visitAllocaInst(AllocaInst &I) {
 }
 
 SizeOffsetEvalType ObjectSizeOffsetEvaluator::visitCallSite(CallSite CS) {
-  Optional<AllocFnsTy> FnData =
-      getAllocationData(CS.getInstruction(), AnyAlloc, TLI);
+  Optional<AllocFnsTy> FnData = getAllocationSize(CS.getInstruction(), TLI);
   if (!FnData)
     return unknown();
 

Diff for: llvm/unittests/Analysis/CMakeLists.txt

@@ -14,6 +14,7 @@ add_llvm_unittest(AnalysisTests
   CGSCCPassManagerTest.cpp
   LazyCallGraphTest.cpp
   LoopPassManagerTest.cpp
+  MemoryBuiltinsTest.cpp
   ScalarEvolutionTest.cpp
   TBAATest.cpp
   ValueTrackingTest.cpp

Diff for: llvm/unittests/Analysis/MemoryBuiltinsTest.cpp

@@ -0,0 +1,50 @@
+//===- MemoryBuiltinsTest.cpp - Tests for utilities in MemoryBuiltins.h ---===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Analysis/MemoryBuiltins.h"
+#include "llvm/IR/Attributes.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+// allocsize should not imply that a function is a traditional allocation
+// function (e.g. that can be optimized out/...); it just tells us how many
+// bytes exist at the pointer handed back by the function.
+TEST(AllocSize, AllocationBuiltinsTest) {
+  LLVMContext Context;
+  Module M("", Context);
+  IntegerType *ArgTy = Type::getInt32Ty(Context);
+
+  Function *AllocSizeFn = Function::Create(
+      FunctionType::get(Type::getInt8PtrTy(Context), {ArgTy}, false),
+      GlobalValue::ExternalLinkage, "F", &M);
+
+  AllocSizeFn->addFnAttr(Attribute::getWithAllocSizeArgs(Context, 1, None));
+
+  // 100 is arbitrary.
+  std::unique_ptr<CallInst> Caller(
+      CallInst::Create(AllocSizeFn, {ConstantInt::get(ArgTy, 100)}));
+
+  const TargetLibraryInfo *TLI = nullptr;
+  EXPECT_FALSE(isNoAliasFn(Caller.get(), TLI));
+  EXPECT_FALSE(isMallocLikeFn(Caller.get(), TLI));
+  EXPECT_FALSE(isCallocLikeFn(Caller.get(), TLI));
+  EXPECT_FALSE(isAllocLikeFn(Caller.get(), TLI));
+
+  // FIXME: We might be able to treat allocsize functions as general allocation
+  // functions. For the moment, being conservative seems better (and we'd have
+  // to plumb stuff around `isNoAliasFn`).
+  EXPECT_FALSE(isAllocationFn(Caller.get(), TLI));
+}
+}