[AddressSanitizer] Refactor ClDebug{Min,Max} handling

[thejh]

Summary

A following commit will split the loop over ToInstrument into two.
To avoid having to duplicate the condition for suppressing instrumentation
sites based on ClDebug{Min,Max}, refactor it out into a new function.

While we're at it, we can also avoid the indirection through
NumInstrumented for setting FunctionModified.

This is patch 1/4 of a patch series:
https://reviews.llvm.org/D77616 [PATCH 1/4] [AddressSanitizer] Refactor ClDebug{Min,Max} handling
https://reviews.llvm.org/D77617 [PATCH 2/4] [AddressSanitizer] Split out memory intrinsic handling
https://reviews.llvm.org/D77618 [PATCH 3/4] [AddressSanitizer] Refactor: Permit >1 interesting operands per instruction
https://reviews.llvm.org/D77619 [PATCH 4/4] [AddressSanitizer] Instrument byval call arguments

[glider]

I've skimmed through the four patches and they look good. Can you please fix the lint checks?

[thejh]

fix clang-format warnings

[thejh]

fix clang-tidy complaint

[thejh]

@glider Alright, I've fixed the clang-format stuff (and fixed a mistake where I put a variable declaration in the wrong patch, and another clang-tidy warning about coding conventions on variable naming). The build status is "passed" on all four patches now.

[thejh]

rebase to make harbormaster happy

[thejh]

I don't understand those latest build failures at all. I've run the regression tests, with mlir enabled. I see no failures, even when building off the exact commit in the build log. Manually running mlir/test/Dialect/Vector/vector-transforms.mlir doesn't show any errors either.

Is this just some random flake in the build system? Can I just ignore that? Should I resubmit? Should I do something else?

[thejh]

let's see whether the build system is more happy if I just resend this...

[glider]

Rebase

[glider]

I've landed this revision. There are some difficulties with the rest, sorting that out.

[glider]

Trying to fix the state of the things

[glider]

update

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

void instrumentUnusualSizeOrAlignment(Instruction *I,
                                         uint32_t Exp);
   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 suppressInstrumentationSiteForDebug(int &Instrumented);
   bool instrumentFunction(Function &F, const TargetLibraryInfo *TLI);
   bool maybeInsertAsanInitAtFunctionEntry(Function &F);
   void maybeInsertDynamicShadowAtFunctionEntry(Function &F);
   void markEscapedLocalAllocas(Function &F);
 
 private:
   friend struct FunctionStackPoisoner;
 
if (II && II->getIntrinsicID() == Intrinsic::localescape) {
                "non-static alloca arg to localescape");
         ProcessedAllocas[AI] = false;
       }
       break;
     }
   }
 }
 
+bool AddressSanitizer::suppressInstrumentationSiteForDebug(int &Instrumented) {
+  bool ShouldInstrument =
+      ClDebugMin < 0 || ClDebugMax < 0 ||
+      (Instrumented >= ClDebugMin && Instrumented <= ClDebugMax);
+  Instrumented++;
+  return !ShouldInstrument;
+}
+
 bool AddressSanitizer::instrumentFunction(Function &F,
                                           const TargetLibraryInfo *TLI) {
   if (F.getLinkage() == GlobalValue::AvailableExternallyLinkage) return false;
   if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) return false;
   if (F.getName().startswith("__asan_")) return false;
 
   bool FunctionModified = false;
 
bool AddressSanitizer::instrumentFunction(Function &F,
   const DataLayout &DL = F.getParent()->getDataLayout();
   ObjectSizeOpts ObjSizeOpts;
   ObjSizeOpts.RoundToAlign = true;
   ObjectSizeOffsetVisitor ObjSizeVis(DL, TLI, F.getContext(), ObjSizeOpts);
 
   // Instrument.
   int NumInstrumented = 0;
   for (auto Inst : ToInstrument) {
-    if (ClDebugMin < 0 || ClDebugMax < 0 ||
-        (NumInstrumented >= ClDebugMin && NumInstrumented <= ClDebugMax)) {
+    if (!suppressInstrumentationSiteForDebug(NumInstrumented)) {
       if (isInterestingMemoryAccess(Inst, &IsWrite, &TypeSize, &Alignment))
         instrumentMop(ObjSizeVis, Inst, UseCalls,
                       F.getParent()->getDataLayout());
       else
         instrumentMemIntrinsic(cast<MemIntrinsic>(Inst));
     }
-    NumInstrumented++;
+    FunctionModified = true;
   }
 
   FunctionStackPoisoner FSP(F, *this);
   bool ChangedStack = FSP.runOnFunction();
 
   // We must unpoison the stack before NoReturn calls (throw, _exit, etc).
   // See e.g. https://github.com/google/sanitizers/issues/37
   for (auto CI : NoReturnCalls) {
     IRBuilder<> IRB(CI);
     IRB.CreateCall(AsanHandleNoReturnFunc, {});
   }
 
   for (auto Inst : PointerComparisonsOrSubtracts) {
     instrumentPointerComparisonOrSubtraction(Inst);
-    NumInstrumented++;
+    FunctionModified = true;
   }
 
-  if (NumInstrumented > 0 || ChangedStack || !NoReturnCalls.empty())
+  if (ChangedStack || !NoReturnCalls.empty())
     FunctionModified = true;
 
   LLVM_DEBUG(dbgs() << "ASAN done instrumenting: " << FunctionModified << " "
                     << F << "\n");
 
   return FunctionModified;
 }