[sanitizers] Disable target-specific lowering of string functions.

[• koriakin]

Summary

CodeGen has hooks that allow targets to emit specialized code instead
of calls to memcmp, memchr, strcpy, stpcpy, strcmp, strlen, strnlen.
When ASan/MSan/TSan/ESan is in use, this sidesteps its interceptors, resulting
in uninstrumented memory accesses. To avoid that, make these sanitizers
mark the calls as nobuiltin.

Depends on D19801.

[echristo]

Seems like a good idea. Is there a place you could hoist the no isNoBuiltin checks?

[• koriakin]

Now the nobuiltin check applies to all libcalls (which makes sense, after all).

[filcab]

Thanks for working on this.
Can you split this patch in two, please?
The SelectionDAG + CodeGen tests on one, the ASan-specific code on another.
Not having a proper way to test SelectionDAG is annoying. To have perfecct coverage, we'd need to replicate those tests for other targets. This will do for now, though. :-)

BTW, have you measured performance impact?
If it's too big, we probably want to put this under a flag (the sanitizer philosophy is that we know we have false negatives, but no false positives, so it fits not doing this by default if it's a big performance hit).

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

6177 ↗

(On Diff #55799)

We probably want to check for isNoBuiltin before we query LibInfo, which involves lookups.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1859

We probably want to add some of the other intercepted functions (but instead of duplicating everything, I guess it's better to only add those that actually have special handling).
That can be a follow up patch, though, as needed.

[• koriakin]

In D19781#418442, @filcab wrote:

Thanks for working on this.
Can you split this patch in two, please?
The SelectionDAG + CodeGen tests on one, the ASan-specific code on another.
Not having a proper way to test SelectionDAG is annoying. To have perfecct coverage, we'd need to replicate those tests for other targets. This will do for now, though. :-)

Alright, I'll split it up. However, there's not much to test for other platforms - only SystemZ currently has customised lowering for those functions.

BTW, have you measured performance impact?
If it's too big, we probably want to put this under a flag (the sanitizer philosophy is that we know we have false negatives, but no false positives, so it fits not doing this by default if it's a big performance hit).

I haven't - I'm still trying to get the testsuite passing on SystemZ. However, all other platforms are perfectly fine with always running the interceptors, so I doubt there's anything to gain here.

[filcab]

Since no-one else does anything special with the libcalls for now, I'm totally ok with the patch.
LGTM with the change + split I asked, then. But wait to see if Eric has more comments.

[filcab]

test/Instrumentation/AddressSanitizer/str-nobuiltin.ll

3

Do we need -asan-module? We should be able to get away with just the FunctionPass, right?

[• koriakin]

Removed -asan-module

[eugenis]

Do we want this in the other sanitizers? Probably yes, and with the same list of functions.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1785

We typically write single-statement ifs without the braces.

1844

Did you mean maybeMarkCallNoBuiltin ?

[• koriakin]

In D19781#419110, @eugenis wrote:

Do we want this in the other sanitizers? Probably yes, and with the same list of functions.

Yeah, I think so. What would be a good place to put the maybeMark...() function?

[eugenis]

lib/Transforms/Utils/ModuleUtils.cpp, or maybe a new file in that directory.

[• koriakin]

Added MSan and TSan to the mix. LSan and UBSan don't intercept these, DFSan already mangles all symbols and avoids the issue, and I have no clue about ESan.

[• koriakin]

Added ESan as well.

[eugenis]

include/llvm/Transforms/Utils/Local.h

358 ↗

(On Diff #56004)

I don't think this is a good name for a global function. It is specific to the sanitizer interceptors. How about maybeMarkSanitizerLibraryCallNoBuiltin?

[• koriakin]

Rather wordy (I'm having problems wrapping the lines), but it's indeed better.

[eugenis]

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp

struct AddressSanitizer : public FunctionPass {
   Value *createSlowPathCmp(IRBuilder<> &IRB, Value *AddrLong,
                            Value *ShadowValue, uint32_t TypeSize);
   Instruction *generateCrashCode(Instruction *InsertBefore, Value *Addr,
                                  bool IsWrite, size_t AccessSizeIndex,
                                  Value *SizeArgument, uint32_t Exp);
   void instrumentMemIntrinsic(MemIntrinsic *MI);
   Value *memToShadow(Value *Shadow, IRBuilder<> &IRB);
   bool runOnFunction(Function &F) override;
+  void maybeMarkCallNoInline(CallInst *CI, const TargetLibraryInfo *TLI);
   bool maybeInsertAsanInitAtFunctionEntry(Function &F);
   void markEscapedLocalAllocas(Function &F);
   bool doInitialization(Module &M) override;
   bool doFinalization(Module &M) override;
   static char ID;  // Pass identification, replacement for typeid
 
   DominatorTree &getDominatorTree() const { return *DT; }
 
bool AddressSanitizer::runOnFunction(Function &F) {
   SmallVector<Instruction *, 16> ToInstrument;
   SmallVector<Instruction *, 8> NoReturnCalls;
   SmallVector<BasicBlock *, 16> AllBlocks;
   SmallVector<Instruction *, 16> PointerComparisonsOrSubtracts;
   int NumAllocas = 0;
   bool IsWrite;
   unsigned Alignment;
   uint64_t TypeSize;
+  const TargetLibraryInfo *TLI =
+      &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
 
   // Fill the set of memory operations to instrument.
   for (auto &BB : F) {
     AllBlocks.push_back(&BB);
     TempsToInstrument.clear();
     int NumInsnsPerBB = 0;
     for (auto &Inst : BB) {
       if (LooksLikeCodeInBug11395(&Inst)) return false;
for (auto &Inst : BB) {
       } else {
         if (isa<AllocaInst>(Inst)) NumAllocas++;
         CallSite CS(&Inst);
         if (CS) {
           // A call inside BB.
           TempsToInstrument.clear();
           if (CS.doesNotReturn()) NoReturnCalls.push_back(CS.getInstruction());
         }
+        if (CallInst *CI = dyn_cast<CallInst>(&Inst)) {
+          maybeMarkCallNoInline(CI, TLI);
+        }
         continue;
       }
       ToInstrument.push_back(&Inst);
       NumInsnsPerBB++;
       if (NumInsnsPerBB >= ClMaxInsnsToInstrumentPerBB) break;
     }
   }
 
   bool UseCalls =
       CompileKernel ||
       (ClInstrumentationWithCallsThreshold >= 0 &&
        ToInstrument.size() > (unsigned)ClInstrumentationWithCallsThreshold);
-  const TargetLibraryInfo *TLI =
-      &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
   const DataLayout &DL = F.getParent()->getDataLayout();
   ObjectSizeOffsetVisitor ObjSizeVis(DL, TLI, F.getContext(),
                                      /*RoundToAlign=*/true);
 
   // Instrument.
   int NumInstrumented = 0;
   for (auto Inst : ToInstrument) {
     if (ClDebugMin < 0 || ClDebugMax < 0 ||
bool AddressSanitizer::runOnFunction(Function &F) {
 
   bool res = NumInstrumented > 0 || ChangedStack || !NoReturnCalls.empty();
 
   DEBUG(dbgs() << "ASAN done instrumenting: " << res << " " << F << "\n");
 
   return res;
 }
 
+// CodeGen has special handling for some string functions that may replace
+// them with target-specific intrinsics.  Since that'd skip our interceptors,
+// and thus make us miss some memory accesses, we mark affected calls
+// as NoBuiltin, which will disable optimization in CodeGen.
+void AddressSanitizer::maybeMarkCallNoInline(CallInst *CI,
+                                             const TargetLibraryInfo *TLI) {
+  Function *F = CI->getCalledFunction();
+  LibFunc::Func Func;
+  if (!F || F->hasLocalLinkage() || !F->hasName() ||
+      !TLI->getLibFunc(F->getName(), Func))
+    return;
+  switch (Func) {
+    default: break;
+    case LibFunc::memcmp:
+    case LibFunc::memchr:
+    case LibFunc::strcpy:
+    case LibFunc::stpcpy:
+    case LibFunc::strcmp:
+    case LibFunc::strlen:
+    case LibFunc::strnlen:
+      CI->addAttribute(AttributeSet::FunctionIndex, Attribute::NoBuiltin);
+      break;
+  }
+}
+
 // Workaround for bug 11395: we don't want to instrument stack in functions
 // with large assembly blobs (32-bit only), otherwise reg alloc may crash.
 // FIXME: remove once the bug 11395 is fixed.
 bool AddressSanitizer::LooksLikeCodeInBug11395(Instruction *I) {
   if (LongSize != 32) return false;
   CallInst *CI = dyn_cast<CallInst>(I);
   if (!CI || !CI->isInlineAsm()) return false;
   if (CI->getNumArgOperands() <= 5) return false;

test/Instrumentation/AddressSanitizer/str-nobuiltin.ll

+; Test marking string functions as nobuiltin in address sanitizer.
+;
+; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i8* @memchr(i8* %a, i32 %b, i64 %c)
+declare i32 @memcmp(i8* %a, i8* %b, i64 %c)
+declare i32 @strcmp(i8* %a, i8* %b)
+declare i8* @strcpy(i8* %a, i8* %b)
+declare i8* @stpcpy(i8* %a, i8* %b)
+declare i64 @strlen(i8* %a)
+declare i64 @strnlen(i8* %a, i64 %b)
+
+; CHECK: call{{.*}}@memchr{{.*}} #[[ATTR:[0-9]+]]
+; CHECK: call{{.*}}@memcmp{{.*}} #[[ATTR]]
+; CHECK: call{{.*}}@strcmp{{.*}} #[[ATTR]]
+; CHECK: call{{.*}}@strcpy{{.*}} #[[ATTR]]
+; CHECK: call{{.*}}@stpcpy{{.*}} #[[ATTR]]
+; CHECK: call{{.*}}@strlen{{.*}} #[[ATTR]]
+; CHECK: call{{.*}}@strnlen{{.*}} #[[ATTR]]
+; attributes #[[ATTR]] = { nobuiltin }
+
+define void @f1(i8* %a, i8* %b) nounwind uwtable sanitize_address {
+  tail call i8* @memchr(i8* %a, i32 1, i64 12)
+  tail call i32 @memcmp(i8* %a, i8* %b, i64 12)
+  tail call i32 @strcmp(i8* %a, i8* %b)
+  tail call i8* @strcpy(i8* %a, i8* %b)
+  tail call i8* @stpcpy(i8* %a, i8* %b)
+  tail call i64 @strlen(i8* %a)
+  tail call i64 @strnlen(i8* %a, i64 12)
+  ret void
+}