Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SanitizerCoverage] Add gated tracing callbacks support to trace-cmp #113227

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

thetruestblue
Copy link
Contributor

The option -sanitizer-coverage-gated-trace-callbacks gates the invocation of the trace-pc-guard callbacks based on the value of a global variable, which is stored in a specific section.
In this commit, we extend this feature to trace-cmp and gate the cmp callbacks to the same variable used for trace-pc-guard.

Update SanitizerCoverage doc with this flag.

rdar://135404160

Patch by: Andrea Fioraldi

@llvmbot llvmbot added clang Clang issues not falling into any other category compiler-rt:sanitizer llvm:transforms labels Oct 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-llvm-transforms

Author: None (thetruestblue)

Changes

The option -sanitizer-coverage-gated-trace-callbacks gates the invocation of the trace-pc-guard callbacks based on the value of a global variable, which is stored in a specific section.
In this commit, we extend this feature to trace-cmp and gate the cmp callbacks to the same variable used for trace-pc-guard.

Update SanitizerCoverage doc with this flag.

rdar://135404160

Patch by: Andrea Fioraldi


Full diff: https://github.com/llvm/llvm-project/pull/113227.diff

3 Files Affected:

  • (modified) clang/docs/SanitizerCoverage.rst (+14)
  • (modified) clang/test/CodeGen/sanitize-coverage-gated-callbacks.c (+1-1)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp (+58-29)
diff --git a/clang/docs/SanitizerCoverage.rst b/clang/docs/SanitizerCoverage.rst
index 45ad03cb43774c..6ea1d14829005c 100644
--- a/clang/docs/SanitizerCoverage.rst
+++ b/clang/docs/SanitizerCoverage.rst
@@ -385,6 +385,20 @@ Users need to implement a single function to capture the CF table at startup:
     // the collected control flow.
   }
 
+Gated Trace Callbacks
+=====================
+
+Gate the invocation of the tracing callbacks with
+``-sanitizer-coverage-gated-trace-callbacks``.
+
+When this option is enabled, the instrumentation will not call into the
+runtime-provided callbacks for tracing, thus only incurring in a trivial
+branch without going through a function call.
+
+It is up to the runtime to toggle the value of the global variable in order to
+enable tracing.
+
+This option is only supported for trace-pc-guard and trace-cmp.
 
 Disabling instrumentation with ``__attribute__((no_sanitize("coverage")))``
 ===========================================================================
diff --git a/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c b/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c
index 9a00d91d5ad086..f458ab51b0a1c8 100644
--- a/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c
+++ b/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c
@@ -9,7 +9,7 @@
 // PLAIN-NOT: section "__DATA,__sancov_gate"
 
 // Produce an error for all incompatible sanitizer coverage modes.
-// INCOMPATIBLE: error: 'sanitizer-coverage-gated-trace-callbacks' is only supported with trace-pc-guard
+// INCOMPATIBLE: error: 'sanitizer-coverage-gated-trace-callbacks' is only supported with trace-pc-guard and trace-cmp
 
 int x[10];
 
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index f7461127ec51cf..72e6a1c2f11ade 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -159,8 +159,8 @@ static cl::opt<bool>
 
 static cl::opt<bool> ClGatedCallbacks(
     "sanitizer-coverage-gated-trace-callbacks",
-    cl::desc("Gate the invocation of the tracing callbacks on a global "
-             "variable. Currently only supported for trace-pc-guard."),
+    cl::desc("Gate the invocation of the tracing callbacks on a global variable"
+             ". Currently only supported for trace-pc-guard and trace-cmp."),
     cl::Hidden, cl::init(false));
 
 namespace {
@@ -235,7 +235,8 @@ class ModuleSanitizerCoverage {
   void instrumentFunction(Function &F);
   void InjectCoverageForIndirectCalls(Function &F,
                                       ArrayRef<Instruction *> IndirCalls);
-  void InjectTraceForCmp(Function &F, ArrayRef<Instruction *> CmpTraceTargets);
+  void InjectTraceForCmp(Function &F, ArrayRef<Instruction *> CmpTraceTargets,
+                         Value *&FunctionGateCmp);
   void InjectTraceForDiv(Function &F,
                          ArrayRef<BinaryOperator *> DivTraceTargets);
   void InjectTraceForGep(Function &F,
@@ -243,14 +244,17 @@ class ModuleSanitizerCoverage {
   void InjectTraceForLoadsAndStores(Function &F, ArrayRef<LoadInst *> Loads,
                                     ArrayRef<StoreInst *> Stores);
   void InjectTraceForSwitch(Function &F,
-                            ArrayRef<Instruction *> SwitchTraceTargets);
+                            ArrayRef<Instruction *> SwitchTraceTargets,
+                            Value *&FunctionGateCmp);
   bool InjectCoverage(Function &F, ArrayRef<BasicBlock *> AllBlocks,
-                      bool IsLeafFunc = true);
+                      Value *&FunctionGateCmp, bool IsLeafFunc = true);
   GlobalVariable *CreateFunctionLocalArrayInSection(size_t NumElements,
                                                     Function &F, Type *Ty,
                                                     const char *Section);
   GlobalVariable *CreatePCArray(Function &F, ArrayRef<BasicBlock *> AllBlocks);
   void CreateFunctionLocalArrays(Function &F, ArrayRef<BasicBlock *> AllBlocks);
+  Instruction *CreateGateBranch(Function &F, Value *&FunctionGateCmp,
+                                Instruction *I);
   Value *CreateFunctionLocalGateCmp(IRBuilder<> &IRB);
   void InjectCoverageAtBlock(Function &F, BasicBlock &BB, size_t Idx,
                              Value *&FunctionGateCmp, bool IsLeafFunc = true);
@@ -493,9 +497,9 @@ bool ModuleSanitizerCoverage::instrumentModule() {
     SanCovLowestStack->setInitializer(Constant::getAllOnesValue(IntptrTy));
 
   if (Options.GatedCallbacks) {
-    if (!Options.TracePCGuard) {
+    if (!Options.TracePCGuard && !Options.TraceCmp) {
       C->emitError(StringRef("'") + ClGatedCallbacks.ArgStr +
-                   "' is only supported with trace-pc-guard");
+                   "' is only supported with trace-pc-guard or trace-cmp");
       return true;
     }
 
@@ -724,10 +728,11 @@ void ModuleSanitizerCoverage::instrumentFunction(Function &F) {
   if (Options.CollectControlFlow)
     createFunctionControlFlow(F);
 
-  InjectCoverage(F, BlocksToInstrument, IsLeafFunc);
+  Value *FunctionGateCmp = nullptr;
+  InjectCoverage(F, BlocksToInstrument, FunctionGateCmp, IsLeafFunc);
   InjectCoverageForIndirectCalls(F, IndirCalls);
-  InjectTraceForCmp(F, CmpTraceTargets);
-  InjectTraceForSwitch(F, SwitchTraceTargets);
+  InjectTraceForCmp(F, CmpTraceTargets, FunctionGateCmp);
+  InjectTraceForSwitch(F, SwitchTraceTargets, FunctionGateCmp);
   InjectTraceForDiv(F, DivTraceTargets);
   InjectTraceForGep(F, GepTraceTargets);
   InjectTraceForLoadsAndStores(F, Loads, Stores);
@@ -816,12 +821,30 @@ Value *ModuleSanitizerCoverage::CreateFunctionLocalGateCmp(IRBuilder<> &IRB) {
   return Cmp;
 }
 
+Instruction *ModuleSanitizerCoverage::CreateGateBranch(Function &F,
+                                                       Value *&FunctionGateCmp,
+                                                       Instruction *IP) {
+  if (!FunctionGateCmp) {
+    // Create this in the entry block
+    BasicBlock &BB = F.getEntryBlock();
+    BasicBlock::iterator IP = BB.getFirstInsertionPt();
+    IP = PrepareToSplitEntryBlock(BB, IP);
+    IRBuilder<> EntryIRB(&*IP);
+    FunctionGateCmp = CreateFunctionLocalGateCmp(EntryIRB);
+  }
+  // Set the branch weights in order to minimize the price paid when the
+  // gate is turned off, allowing the default enablement of this
+  // instrumentation with as little of a performance cost as possible
+  auto Weights = MDBuilder(*C).createBranchWeights(1, 100000);
+  return SplitBlockAndInsertIfThen(FunctionGateCmp, IP, false, Weights);
+}
+
 bool ModuleSanitizerCoverage::InjectCoverage(Function &F,
                                              ArrayRef<BasicBlock *> AllBlocks,
+                                             Value *&FunctionGateCmp,
                                              bool IsLeafFunc) {
   if (AllBlocks.empty()) return false;
   CreateFunctionLocalArrays(F, AllBlocks);
-  Value *FunctionGateCmp = nullptr;
   for (size_t i = 0, N = AllBlocks.size(); i < N; i++)
     InjectCoverageAtBlock(F, *AllBlocks[i], i, FunctionGateCmp, IsLeafFunc);
   return true;
@@ -855,7 +878,8 @@ void ModuleSanitizerCoverage::InjectCoverageForIndirectCalls(
 //      {NumCases, ValueSizeInBits, Case0Value, Case1Value, Case2Value, ... })
 
 void ModuleSanitizerCoverage::InjectTraceForSwitch(
-    Function &, ArrayRef<Instruction *> SwitchTraceTargets) {
+    Function &F, ArrayRef<Instruction *> SwitchTraceTargets,
+    Value *&FunctionGateCmp) {
   for (auto *I : SwitchTraceTargets) {
     if (SwitchInst *SI = dyn_cast<SwitchInst>(I)) {
       InstrumentationIRBuilder IRB(I);
@@ -886,7 +910,13 @@ void ModuleSanitizerCoverage::InjectTraceForSwitch(
           *CurModule, ArrayOfInt64Ty, false, GlobalVariable::InternalLinkage,
           ConstantArray::get(ArrayOfInt64Ty, Initializers),
           "__sancov_gen_cov_switch_values");
-      IRB.CreateCall(SanCovTraceSwitchFunction, {Cond, GV});
+      if (Options.GatedCallbacks) {
+        auto GateBranch = CreateGateBranch(F, FunctionGateCmp, I);
+        IRBuilder<> GateIRB(GateBranch);
+        GateIRB.CreateCall(SanCovTraceSwitchFunction, {Cond, GV});
+      } else {
+        IRB.CreateCall(SanCovTraceSwitchFunction, {Cond, GV});
+      }
     }
   }
 }
@@ -950,7 +980,8 @@ void ModuleSanitizerCoverage::InjectTraceForLoadsAndStores(
 }
 
 void ModuleSanitizerCoverage::InjectTraceForCmp(
-    Function &, ArrayRef<Instruction *> CmpTraceTargets) {
+    Function &F, ArrayRef<Instruction *> CmpTraceTargets,
+    Value *&FunctionGateCmp) {
   for (auto *I : CmpTraceTargets) {
     if (ICmpInst *ICMP = dyn_cast<ICmpInst>(I)) {
       InstrumentationIRBuilder IRB(ICMP);
@@ -978,8 +1009,15 @@ void ModuleSanitizerCoverage::InjectTraceForCmp(
       }
 
       auto Ty = Type::getIntNTy(*C, TypeSize);
-      IRB.CreateCall(CallbackFunc, {IRB.CreateIntCast(A0, Ty, true),
-              IRB.CreateIntCast(A1, Ty, true)});
+      if (Options.GatedCallbacks) {
+        auto GateBranch = CreateGateBranch(F, FunctionGateCmp, I);
+        IRBuilder<> GateIRB(GateBranch);
+        GateIRB.CreateCall(CallbackFunc, {GateIRB.CreateIntCast(A0, Ty, true),
+                                          GateIRB.CreateIntCast(A1, Ty, true)});
+      } else {
+        IRB.CreateCall(CallbackFunc, {IRB.CreateIntCast(A0, Ty, true),
+                                      IRB.CreateIntCast(A1, Ty, true)});
+      }
     }
   }
 }
@@ -1013,19 +1051,10 @@ void ModuleSanitizerCoverage::InjectCoverageAtBlock(Function &F, BasicBlock &BB,
                       ConstantInt::get(IntptrTy, Idx * 4)),
         PtrTy);
     if (Options.GatedCallbacks) {
-      if (!FunctionGateCmp) {
-        // Create this in the entry block
-        assert(IsEntryBB);
-        FunctionGateCmp = CreateFunctionLocalGateCmp(IRB);
-      }
-      // Set the branch weights in order to minimize the price paid when the
-      // gate is turned off, allowing the default enablement of this
-      // instrumentation with as little of a performance cost as possible
-      auto Weights = MDBuilder(*C).createBranchWeights(1, 100000);
-      auto ThenTerm =
-          SplitBlockAndInsertIfThen(FunctionGateCmp, &*IP, false, Weights);
-      IRBuilder<> ThenIRB(ThenTerm);
-      ThenIRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
+      Instruction *I = &*IP;
+      auto GateBranch = CreateGateBranch(F, FunctionGateCmp, I);
+      IRBuilder<> GateIRB(GateBranch);
+      GateIRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
     } else {
       IRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
     }

@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-clang

Author: None (thetruestblue)

Changes

The option -sanitizer-coverage-gated-trace-callbacks gates the invocation of the trace-pc-guard callbacks based on the value of a global variable, which is stored in a specific section.
In this commit, we extend this feature to trace-cmp and gate the cmp callbacks to the same variable used for trace-pc-guard.

Update SanitizerCoverage doc with this flag.

rdar://135404160

Patch by: Andrea Fioraldi


Full diff: https://github.com/llvm/llvm-project/pull/113227.diff

3 Files Affected:

  • (modified) clang/docs/SanitizerCoverage.rst (+14)
  • (modified) clang/test/CodeGen/sanitize-coverage-gated-callbacks.c (+1-1)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp (+58-29)
diff --git a/clang/docs/SanitizerCoverage.rst b/clang/docs/SanitizerCoverage.rst
index 45ad03cb43774c..6ea1d14829005c 100644
--- a/clang/docs/SanitizerCoverage.rst
+++ b/clang/docs/SanitizerCoverage.rst
@@ -385,6 +385,20 @@ Users need to implement a single function to capture the CF table at startup:
     // the collected control flow.
   }
 
+Gated Trace Callbacks
+=====================
+
+Gate the invocation of the tracing callbacks with
+``-sanitizer-coverage-gated-trace-callbacks``.
+
+When this option is enabled, the instrumentation will not call into the
+runtime-provided callbacks for tracing, thus only incurring in a trivial
+branch without going through a function call.
+
+It is up to the runtime to toggle the value of the global variable in order to
+enable tracing.
+
+This option is only supported for trace-pc-guard and trace-cmp.
 
 Disabling instrumentation with ``__attribute__((no_sanitize("coverage")))``
 ===========================================================================
diff --git a/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c b/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c
index 9a00d91d5ad086..f458ab51b0a1c8 100644
--- a/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c
+++ b/clang/test/CodeGen/sanitize-coverage-gated-callbacks.c
@@ -9,7 +9,7 @@
 // PLAIN-NOT: section "__DATA,__sancov_gate"
 
 // Produce an error for all incompatible sanitizer coverage modes.
-// INCOMPATIBLE: error: 'sanitizer-coverage-gated-trace-callbacks' is only supported with trace-pc-guard
+// INCOMPATIBLE: error: 'sanitizer-coverage-gated-trace-callbacks' is only supported with trace-pc-guard and trace-cmp
 
 int x[10];
 
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index f7461127ec51cf..72e6a1c2f11ade 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -159,8 +159,8 @@ static cl::opt<bool>
 
 static cl::opt<bool> ClGatedCallbacks(
     "sanitizer-coverage-gated-trace-callbacks",
-    cl::desc("Gate the invocation of the tracing callbacks on a global "
-             "variable. Currently only supported for trace-pc-guard."),
+    cl::desc("Gate the invocation of the tracing callbacks on a global variable"
+             ". Currently only supported for trace-pc-guard and trace-cmp."),
     cl::Hidden, cl::init(false));
 
 namespace {
@@ -235,7 +235,8 @@ class ModuleSanitizerCoverage {
   void instrumentFunction(Function &F);
   void InjectCoverageForIndirectCalls(Function &F,
                                       ArrayRef<Instruction *> IndirCalls);
-  void InjectTraceForCmp(Function &F, ArrayRef<Instruction *> CmpTraceTargets);
+  void InjectTraceForCmp(Function &F, ArrayRef<Instruction *> CmpTraceTargets,
+                         Value *&FunctionGateCmp);
   void InjectTraceForDiv(Function &F,
                          ArrayRef<BinaryOperator *> DivTraceTargets);
   void InjectTraceForGep(Function &F,
@@ -243,14 +244,17 @@ class ModuleSanitizerCoverage {
   void InjectTraceForLoadsAndStores(Function &F, ArrayRef<LoadInst *> Loads,
                                     ArrayRef<StoreInst *> Stores);
   void InjectTraceForSwitch(Function &F,
-                            ArrayRef<Instruction *> SwitchTraceTargets);
+                            ArrayRef<Instruction *> SwitchTraceTargets,
+                            Value *&FunctionGateCmp);
   bool InjectCoverage(Function &F, ArrayRef<BasicBlock *> AllBlocks,
-                      bool IsLeafFunc = true);
+                      Value *&FunctionGateCmp, bool IsLeafFunc = true);
   GlobalVariable *CreateFunctionLocalArrayInSection(size_t NumElements,
                                                     Function &F, Type *Ty,
                                                     const char *Section);
   GlobalVariable *CreatePCArray(Function &F, ArrayRef<BasicBlock *> AllBlocks);
   void CreateFunctionLocalArrays(Function &F, ArrayRef<BasicBlock *> AllBlocks);
+  Instruction *CreateGateBranch(Function &F, Value *&FunctionGateCmp,
+                                Instruction *I);
   Value *CreateFunctionLocalGateCmp(IRBuilder<> &IRB);
   void InjectCoverageAtBlock(Function &F, BasicBlock &BB, size_t Idx,
                              Value *&FunctionGateCmp, bool IsLeafFunc = true);
@@ -493,9 +497,9 @@ bool ModuleSanitizerCoverage::instrumentModule() {
     SanCovLowestStack->setInitializer(Constant::getAllOnesValue(IntptrTy));
 
   if (Options.GatedCallbacks) {
-    if (!Options.TracePCGuard) {
+    if (!Options.TracePCGuard && !Options.TraceCmp) {
       C->emitError(StringRef("'") + ClGatedCallbacks.ArgStr +
-                   "' is only supported with trace-pc-guard");
+                   "' is only supported with trace-pc-guard or trace-cmp");
       return true;
     }
 
@@ -724,10 +728,11 @@ void ModuleSanitizerCoverage::instrumentFunction(Function &F) {
   if (Options.CollectControlFlow)
     createFunctionControlFlow(F);
 
-  InjectCoverage(F, BlocksToInstrument, IsLeafFunc);
+  Value *FunctionGateCmp = nullptr;
+  InjectCoverage(F, BlocksToInstrument, FunctionGateCmp, IsLeafFunc);
   InjectCoverageForIndirectCalls(F, IndirCalls);
-  InjectTraceForCmp(F, CmpTraceTargets);
-  InjectTraceForSwitch(F, SwitchTraceTargets);
+  InjectTraceForCmp(F, CmpTraceTargets, FunctionGateCmp);
+  InjectTraceForSwitch(F, SwitchTraceTargets, FunctionGateCmp);
   InjectTraceForDiv(F, DivTraceTargets);
   InjectTraceForGep(F, GepTraceTargets);
   InjectTraceForLoadsAndStores(F, Loads, Stores);
@@ -816,12 +821,30 @@ Value *ModuleSanitizerCoverage::CreateFunctionLocalGateCmp(IRBuilder<> &IRB) {
   return Cmp;
 }
 
+Instruction *ModuleSanitizerCoverage::CreateGateBranch(Function &F,
+                                                       Value *&FunctionGateCmp,
+                                                       Instruction *IP) {
+  if (!FunctionGateCmp) {
+    // Create this in the entry block
+    BasicBlock &BB = F.getEntryBlock();
+    BasicBlock::iterator IP = BB.getFirstInsertionPt();
+    IP = PrepareToSplitEntryBlock(BB, IP);
+    IRBuilder<> EntryIRB(&*IP);
+    FunctionGateCmp = CreateFunctionLocalGateCmp(EntryIRB);
+  }
+  // Set the branch weights in order to minimize the price paid when the
+  // gate is turned off, allowing the default enablement of this
+  // instrumentation with as little of a performance cost as possible
+  auto Weights = MDBuilder(*C).createBranchWeights(1, 100000);
+  return SplitBlockAndInsertIfThen(FunctionGateCmp, IP, false, Weights);
+}
+
 bool ModuleSanitizerCoverage::InjectCoverage(Function &F,
                                              ArrayRef<BasicBlock *> AllBlocks,
+                                             Value *&FunctionGateCmp,
                                              bool IsLeafFunc) {
   if (AllBlocks.empty()) return false;
   CreateFunctionLocalArrays(F, AllBlocks);
-  Value *FunctionGateCmp = nullptr;
   for (size_t i = 0, N = AllBlocks.size(); i < N; i++)
     InjectCoverageAtBlock(F, *AllBlocks[i], i, FunctionGateCmp, IsLeafFunc);
   return true;
@@ -855,7 +878,8 @@ void ModuleSanitizerCoverage::InjectCoverageForIndirectCalls(
 //      {NumCases, ValueSizeInBits, Case0Value, Case1Value, Case2Value, ... })
 
 void ModuleSanitizerCoverage::InjectTraceForSwitch(
-    Function &, ArrayRef<Instruction *> SwitchTraceTargets) {
+    Function &F, ArrayRef<Instruction *> SwitchTraceTargets,
+    Value *&FunctionGateCmp) {
   for (auto *I : SwitchTraceTargets) {
     if (SwitchInst *SI = dyn_cast<SwitchInst>(I)) {
       InstrumentationIRBuilder IRB(I);
@@ -886,7 +910,13 @@ void ModuleSanitizerCoverage::InjectTraceForSwitch(
           *CurModule, ArrayOfInt64Ty, false, GlobalVariable::InternalLinkage,
           ConstantArray::get(ArrayOfInt64Ty, Initializers),
           "__sancov_gen_cov_switch_values");
-      IRB.CreateCall(SanCovTraceSwitchFunction, {Cond, GV});
+      if (Options.GatedCallbacks) {
+        auto GateBranch = CreateGateBranch(F, FunctionGateCmp, I);
+        IRBuilder<> GateIRB(GateBranch);
+        GateIRB.CreateCall(SanCovTraceSwitchFunction, {Cond, GV});
+      } else {
+        IRB.CreateCall(SanCovTraceSwitchFunction, {Cond, GV});
+      }
     }
   }
 }
@@ -950,7 +980,8 @@ void ModuleSanitizerCoverage::InjectTraceForLoadsAndStores(
 }
 
 void ModuleSanitizerCoverage::InjectTraceForCmp(
-    Function &, ArrayRef<Instruction *> CmpTraceTargets) {
+    Function &F, ArrayRef<Instruction *> CmpTraceTargets,
+    Value *&FunctionGateCmp) {
   for (auto *I : CmpTraceTargets) {
     if (ICmpInst *ICMP = dyn_cast<ICmpInst>(I)) {
       InstrumentationIRBuilder IRB(ICMP);
@@ -978,8 +1009,15 @@ void ModuleSanitizerCoverage::InjectTraceForCmp(
       }
 
       auto Ty = Type::getIntNTy(*C, TypeSize);
-      IRB.CreateCall(CallbackFunc, {IRB.CreateIntCast(A0, Ty, true),
-              IRB.CreateIntCast(A1, Ty, true)});
+      if (Options.GatedCallbacks) {
+        auto GateBranch = CreateGateBranch(F, FunctionGateCmp, I);
+        IRBuilder<> GateIRB(GateBranch);
+        GateIRB.CreateCall(CallbackFunc, {GateIRB.CreateIntCast(A0, Ty, true),
+                                          GateIRB.CreateIntCast(A1, Ty, true)});
+      } else {
+        IRB.CreateCall(CallbackFunc, {IRB.CreateIntCast(A0, Ty, true),
+                                      IRB.CreateIntCast(A1, Ty, true)});
+      }
     }
   }
 }
@@ -1013,19 +1051,10 @@ void ModuleSanitizerCoverage::InjectCoverageAtBlock(Function &F, BasicBlock &BB,
                       ConstantInt::get(IntptrTy, Idx * 4)),
         PtrTy);
     if (Options.GatedCallbacks) {
-      if (!FunctionGateCmp) {
-        // Create this in the entry block
-        assert(IsEntryBB);
-        FunctionGateCmp = CreateFunctionLocalGateCmp(IRB);
-      }
-      // Set the branch weights in order to minimize the price paid when the
-      // gate is turned off, allowing the default enablement of this
-      // instrumentation with as little of a performance cost as possible
-      auto Weights = MDBuilder(*C).createBranchWeights(1, 100000);
-      auto ThenTerm =
-          SplitBlockAndInsertIfThen(FunctionGateCmp, &*IP, false, Weights);
-      IRBuilder<> ThenIRB(ThenTerm);
-      ThenIRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
+      Instruction *I = &*IP;
+      auto GateBranch = CreateGateBranch(F, FunctionGateCmp, I);
+      IRBuilder<> GateIRB(GateBranch);
+      GateIRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
     } else {
       IRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
     }

Copy link

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

thetruestblue added a commit that referenced this pull request Nov 22, 2024
A Pre-commit for use in adding gated tracing callbacks support to
trace-cmp
[#113227](53b316d)

rdar://135404160

Patch by: Andrea Fioraldi
The option -sanitizer-coverage-gated-trace-callbacks gates the invocation of the
trace-pc-guard callbacks based on the value of a global variable, which is stored
in a specific section.
In this commit, we extend this feature to trace-cmp and gate the cmp callbacks to
the same variable used for trace-pc-guard.

Update SanitizerCoverage doc with this flag.

rdar://135404160

Patch by: Andrea Fioraldi
@thetruestblue
Copy link
Contributor Author

Requesting review after addressing feedback.

Copy link

github-actions bot commented Nov 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@thetruestblue thetruestblue merged commit 7800d59 into llvm:main Nov 25, 2024
9 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 25, 2024

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building clang,llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/5000

Here is the relevant piece of the build log for the reference
Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************


@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 25, 2024

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-5 while building clang,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/10148

Here is the relevant piece of the build log for the reference
Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll' FAILED ********************
Exit Code: 2

Command Output (stderr):
--
RUN: at line 1: /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll | /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
+ /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
+ /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
 #0 0x0000000105abac54 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100e76c54)
 #1 0x0000000105ab8cd8 llvm::sys::RunSignalHandlers() (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100e74cd8)
 #2 0x0000000105abb310 SignalHandler(int) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100e77310)
 #3 0x0000000198f82584 (/usr/lib/system/libsystem_platform.dylib+0x18047a584)
 #4 0x0000000198f5121c (/usr/lib/system/libsystem_pthread.dylib+0x18044921c)
 #5 0x0000000198e77ad0 (/usr/lib/libc++.1.dylib+0x18036fad0)
 #6 0x000000010567d1c0 void llvm::detail::UniqueFunctionBase<void, llvm::Expected<llvm::DenseMap<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef, llvm::DenseMapInfo<llvm::orc::SymbolStringPtr, void>, llvm::detail::DenseMapPair<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef>>>>::CallImpl<llvm::orc::Platform::lookupInitSymbols(llvm::orc::ExecutionSession&, llvm::DenseMap<llvm::orc::JITDylib*, llvm::orc::SymbolLookupSet, llvm::DenseMapInfo<llvm::orc::JITDylib*, void>, llvm::detail::DenseMapPair<llvm::orc::JITDylib*, llvm::orc::SymbolLookupSet>> const&)::$_44>(void*, llvm::Expected<llvm::DenseMap<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef, llvm::DenseMapInfo<llvm::orc::SymbolStringPtr, void>, llvm::detail::DenseMapPair<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef>>>&) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100a391c0)
 #7 0x0000000105678974 llvm::orc::AsynchronousSymbolQuery::handleComplete(llvm::orc::ExecutionSession&)::RunQueryCompleteTask::run() (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100a34974)
 #8 0x0000000105727720 void* std::__1::__thread_proxy[abi:un170006]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, llvm::orc::DynamicThreadPoolTaskDispatcher::dispatch(std::__1::unique_ptr<llvm::orc::Task, std::__1::default_delete<llvm::orc::Task>>)::$_0>>(void*) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100ae3720)
 #9 0x0000000198f51f94 (/usr/lib/system/libsystem_pthread.dylib+0x180449f94)
#10 0x0000000198f4cd34 (/usr/lib/system/libsystem_pthread.dylib+0x180444d34)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll

--

********************


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category compiler-rt:sanitizer llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants