Skip to content

Reapply "[sanitizer][NFCI] Add Options parameter to LowerAllowCheckPass" (#122833) #122994

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

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

thurstond
Copy link
Contributor

This reverts commit 1515caf (#122833) i.e., relands 7d8b4eb
(#122765), with the addition of std::move to fix a stack use-after-scope error.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. llvm:transforms labels Jan 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Thurston Dang (thurstond)

Changes

This reverts commit 1515caf (#122833) i.e., relands 7d8b4eb
(#122765), with the addition of std::move to fix a stack use-after-scope error.


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

4 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+6-4)
  • (modified) llvm/include/llvm/Transforms/Instrumentation/LowerAllowCheckPass.h (+9)
  • (modified) llvm/lib/Passes/PassBuilder.cpp (+15)
  • (modified) llvm/lib/Passes/PassRegistry.def (+4-1)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 79e6bf3d24dffb..ffe640fd4b9287 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -795,11 +795,13 @@ static void addSanitizers(const Triple &TargetTriple,
   }
 
   if (LowerAllowCheckPass::IsRequested()) {
+    LowerAllowCheckPass::Options Opts;
     // We want to call it after inline, which is about OptimizerEarlyEPCallback.
-    PB.registerOptimizerEarlyEPCallback([](ModulePassManager &MPM,
-                                           OptimizationLevel Level,
-                                           ThinOrFullLTOPhase Phase) {
-      MPM.addPass(createModuleToFunctionPassAdaptor(LowerAllowCheckPass()));
+    PB.registerOptimizerEarlyEPCallback([Opts = std::move(Opts)](ModulePassManager &MPM,
+                                               OptimizationLevel Level,
+                                               ThinOrFullLTOPhase Phase) {
+      MPM.addPass(createModuleToFunctionPassAdaptor(
+          LowerAllowCheckPass(Opts)));
     });
   }
 }
diff --git a/llvm/include/llvm/Transforms/Instrumentation/LowerAllowCheckPass.h b/llvm/include/llvm/Transforms/Instrumentation/LowerAllowCheckPass.h
index af974818fec5f3..3ee907606e12b8 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/LowerAllowCheckPass.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/LowerAllowCheckPass.h
@@ -24,9 +24,18 @@ namespace llvm {
 // from the hot code.
 class LowerAllowCheckPass : public PassInfoMixin<LowerAllowCheckPass> {
 public:
+  struct Options {
+    std::vector<unsigned int> placeholder; // TODO: cutoffs
+  };
+
+  explicit LowerAllowCheckPass(LowerAllowCheckPass::Options Opts)
+      : Opts(std::move(Opts)) {};
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 
   static bool IsRequested();
+
+private:
+  LowerAllowCheckPass::Options Opts;
 };
 
 } // namespace llvm
diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
index 94782547325ed1..1e9f825dc3dc4c 100644
--- a/llvm/lib/Passes/PassBuilder.cpp
+++ b/llvm/lib/Passes/PassBuilder.cpp
@@ -821,6 +821,21 @@ Expected<EmbedBitcodeOptions> parseEmbedBitcodePassOptions(StringRef Params) {
   return Result;
 }
 
+Expected<LowerAllowCheckPass::Options>
+parseLowerAllowCheckPassOptions(StringRef Params) {
+  LowerAllowCheckPass::Options Result;
+  while (!Params.empty()) {
+    StringRef ParamName;
+    std::tie(ParamName, Params) = Params.split(';');
+
+    return make_error<StringError>(
+        formatv("invalid LowerAllowCheck pass parameter '{0}' ", ParamName)
+            .str(),
+        inconvertibleErrorCode());
+  }
+  return Result;
+}
+
 Expected<MemorySanitizerOptions> parseMSanPassOptions(StringRef Params) {
   MemorySanitizerOptions Result;
   while (!Params.empty()) {
diff --git a/llvm/lib/Passes/PassRegistry.def b/llvm/lib/Passes/PassRegistry.def
index a93a995655a147..0eb050c8adb047 100644
--- a/llvm/lib/Passes/PassRegistry.def
+++ b/llvm/lib/Passes/PassRegistry.def
@@ -402,7 +402,6 @@ FUNCTION_PASS("loop-load-elim", LoopLoadEliminationPass())
 FUNCTION_PASS("loop-simplify", LoopSimplifyPass())
 FUNCTION_PASS("loop-sink", LoopSinkPass())
 FUNCTION_PASS("loop-versioning", LoopVersioningPass())
-FUNCTION_PASS("lower-allow-check", LowerAllowCheckPass())
 FUNCTION_PASS("lower-atomic", LowerAtomicPass())
 FUNCTION_PASS("lower-constant-intrinsics", LowerConstantIntrinsicsPass())
 FUNCTION_PASS("lower-expect", LowerExpectIntrinsicPass())
@@ -553,6 +552,10 @@ FUNCTION_PASS_WITH_PARAMS(
     parseLoopVectorizeOptions,
     "no-interleave-forced-only;interleave-forced-only;no-vectorize-forced-only;"
     "vectorize-forced-only")
+FUNCTION_PASS_WITH_PARAMS(
+    "lower-allow-check", "LowerAllowCheckPass",
+    [](LowerAllowCheckPass::Options Opts) { return LowerAllowCheckPass(Opts); },
+    parseLowerAllowCheckPassOptions, "")
 FUNCTION_PASS_WITH_PARAMS(
     "lower-matrix-intrinsics", "LowerMatrixIntrinsicsPass",
     [](bool Minimal) { return LowerMatrixIntrinsicsPass(Minimal); },

Copy link

github-actions bot commented Jan 15, 2025

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

thurstond and others added 3 commits January 22, 2025 16:57
@thurstond thurstond force-pushed the lowerallowcheck_options_reland branch from ff3309b to c94bee3 Compare January 22, 2025 17:30
@thurstond thurstond merged commit 2476417 into llvm:main Jan 22, 2025
5 of 7 checks passed
thurstond added a commit to thurstond/llvm-project that referenced this pull request Jan 23, 2025
This passes through the values of -fsanitize-skip-hot-cutoff (introduced
patch in llvm#121619) to the
LowerAllowCheckPass, via the Options parameter (introduced in
llvm#122994), and adjusts the
instrumentation accordingly.

The net effect is that -fsanitize-skip-hot-cutoff now combines the functionality of -ubsan-guard-checks and -lower-allow-check-percentile-cutoff (though this patch does not remove those yet), and generalizes the latter to allow per-sanitizer cutoffs.

Note: this patch replaces Intrinsic::allow_ubsan_check's SanitizerHandler parameter
with SanitizerOrdinal; this is necessary because the hot cutoffs are
specified in terms of SanitizerOrdinal (e.g., null, alignment), not
SanitizerHandler (e.g., TypeMismatch).
thurstond added a commit that referenced this pull request Jan 28, 2025
…LowerAllowCheckPass (#124211)

This adds and utilizes a cutoffs parameter for LowerAllowCheckPass, via the Options parameter (introduced in #122994).

Future work will connect -fsanitize-skip-hot-cutoff (introduced patch in #121619) in the clang frontend to the cutoffs parameter used here.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 28, 2025
…=90000> in LowerAllowCheckPass (#124211)

This adds and utilizes a cutoffs parameter for LowerAllowCheckPass, via the Options parameter (introduced in llvm/llvm-project#122994).

Future work will connect -fsanitize-skip-hot-cutoff (introduced patch in llvm/llvm-project#121619) in the clang frontend to the cutoffs parameter used here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants