[hwasan] Update (Post-)DominatorTreeAnalysis and LoopAnalysis incrementally #66935

thurstond · 2023-09-20T18:18:11Z

HWAddressSanitizerPass::run sanitizes functions one by one. The sanitization of each function - which may split blocks via insertShadowTagCheck - may result in some cached analyses are invalid. This matters because sanitizeFunction(F', FAM) may indirectly call the global stack safety analysis, hence we need to make sure the analyses of F are up to date.

Bug report: #66934

…tized HWAddressSanitizerPass::run sanitizes functions one by one. The sanitization of each function - which may split blocks via insertShadowTagCheck - may result in some cached analyses are invalid. This matters because sanitizeFunction(F', FAM) may indirectly call the global stack safety analysis, hence we need to make sure the analyses of F are up to date. Bug report: llvm#66934

llvmbot · 2023-09-20T18:19:14Z

@llvm/pr-subscribers-llvm-transforms

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

Changes

HWAddressSanitizerPass::run sanitizes functions one by one. The sanitization of each function - which may split blocks via insertShadowTagCheck - may result in some cached analyses are invalid. This matters because sanitizeFunction(F', FAM) may indirectly call the global stack safety analysis, hence we need to make sure the analyses of F are up to date.

Bug report: #66934

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

1 Files Affected:

(modified) llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp (+9-1)

diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
index 29770ece9c61eb2..1dc550ba8b54ea6 100644
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -430,8 +430,16 @@ PreservedAnalyses HWAddressSanitizerPass::run(Module &M,
 
   HWAddressSanitizer HWASan(M, Options.CompileKernel, Options.Recover, SSI);
   auto &FAM = MAM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
-  for (Function &F : M)
+  for (Function &F : M) {
     HWASan.sanitizeFunction(F, FAM);
+    // After sanitizing F - which may split blocks via insertShadowTagCheck -
+    // some cached analyses are invalid. This matters because
+    // sanitizeFunction(F', FAM) may indirectly call the global stack safety
+    // analysis, hence we need to make sure the analyses of F are up to date.
+    PreservedAnalyses PA = PreservedAnalyses::all();
+    PA.abandon<DominatorTreeAnalysis>();
+    FAM.invalidate(F, PA);
+  }
 
   PreservedAnalyses PA = PreservedAnalyses::none();
   // GlobalsAA is considered stateless and does not get invalidated unless

fhahn · 2023-09-21T08:38:16Z

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

+    // some cached analyses are invalid. This matters because
+    // sanitizeFunction(F', FAM) may indirectly call the global stack safety
+    // analysis, hence we need to make sure the analyses of F are up to date.
+    PreservedAnalyses PA = PreservedAnalyses::all();


This is confusing, in particular because below it marks all analysis as not preserved. Should we invalidate all analysis here?

Also, the dominator tree should be very easy to preserve, to save some compile-time: SplitBlockAndInsertIfThen takes an optional DomTreeUpdater object to update the dominator tree.

I didn't take a look at how the pass uses the dominator tree, but if the dominator tree isn't updated after making changes to the CFG and is later used again in the same function, then it may use an out-of-date dominator tree.

This is confusing, in particular because below it marks all analysis as not preserved. Should we invalidate all analysis here?

ASan, MSan, DFSan and HWASan all share the same "invalidate all the analyses, including GlobalsAA, at the end of the pass" boilerplate code, so perhaps it's not strictly necessary for HWASan.

But even if all the analyses are stale at the end of the HWASan pass, they don't necessarily need to be eagerly invalidated after each function is sanitized, as long as the analysis is not used by the rest of the HWASan pass. The stack safety global analysis is a special case because, by definition, it looks at every function.

Also, the dominator tree should be very easy to preserve, to save some compile-time: SplitBlockAndInsertIfThen takes an optional DomTreeUpdater object to update the dominator tree.

That's a good idea, thanks! I'll update the pull request shortly.

P.S. I'm honored to have two Florians review my patch 😀

This patch incrementally updates the DominatorTreeAnalysis and LoopAnalysis whenever SplitBlockAndInsertIfThen is called, which fixes the following issue: Suppose we have two functions, F and G. HWAddressSanitizerPass::run will first sanitize F, which potentially makes the analyses of F out of date. When G is then sanitized, it may call the global stack safety analysis, which can crash if the analysis of F is stale. Additionally, since the DominatorTreeAnalysis and LoopAnalysis are now incrementally updated, they do not need to be abandoned at the end of the pass. Bug report: llvm#66934

fmayer · 2023-09-21T20:19:59Z

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

+  DominatorTree &DT = FAM.getResult<DominatorTreeAnalysis>(F);
+  PostDominatorTree &PDT = FAM.getResult<PostDominatorTreeAnalysis>(F);
+  DomTreeUpdater DTU(DT, PDT, DomTreeUpdater::UpdateStrategy::Lazy);
+  LoopInfo &LI = FAM.getResult<LoopAnalysis>(F);


We could pull the definitions for DT PDT and LI out of the if in 1538 then we need not repeat them.

Done, thanks!

This patch incrementally updates the DominatorTreeAnalysis and LoopAnalysis whenever SplitBlockAndInsertIfThen is called, which fixes the following issue: Suppose we have two functions, F and G. HWAddressSanitizerPass::run will first sanitize F, which potentially makes the analyses of F out of date. When G is then sanitized, it may call the global stack safety analysis, which can crash if the analysis of F is stale. Additionally, since the DominatorTreeAnalysis and LoopAnalysis are now incrementally updated, they do not need to be abandoned at the end of the pass. Bug report: llvm#66934

fmayer · 2023-09-21T21:15:42Z

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

+  // DominatorTreeAnalysis and LoopAnalysis are incrementally updated
+  // throughout this pass whenever SplitBlockAndInsertIfThen is called.
+  PA.preserve<DominatorTreeAnalysis>();
+  PA.preserve<LoopAnalysis>();


Can we add PostDominatorTreeAnalysis here too? Seems like DomTreeUpdater also takes care of that.

(And if not, please add a comment in the code on why not, because I think that's a natural question)

Good point, added.

…ntally This patch incrementally updates the DominatorTreeAnalysis, PostDominatorTreeAnalysis and LoopAnalysis whenever SplitBlockAndInsertIfThen is called, which fixes the following issue: Suppose we have two functions, F and G. HWAddressSanitizerPass::run will first sanitize F, which potentially makes the analyses of F out of date. When G is then sanitized, it may call the global stack safety analysis, which can crash if the analysis of F is stale. Additionally, since the DominatorTreeAnalysis, PostDominatorTreeAnalysis and LoopAnalysis are now incrementally updated, they do not need to be abandoned at the end of the pass. Bug report: llvm#66934

fmayer

I give this CL 1 / 2 Florians.

fhahn · 2023-09-22T09:56:25Z

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

@@ -1520,10 +1537,10 @@ void HWAddressSanitizer::sanitizeFunction(Function &F,
                   Mapping.WithFrameRecord &&
                   !SInfo.AllocasToInstrument.empty());

+  DominatorTree &DT = FAM.getResult<DominatorTreeAnalysis>(F);


This now may request the DT/LI/PDT in cases where it wasn't before, which should probably avoided. In cases they are only needed for updating them/preserving them, you can use FAM.getCachedResult, which won't cause the DT to be computed if it isn't already cached.

Done, thanks for the suggestion!

fhahn · 2023-09-22T09:56:57Z

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

-HWAddressSanitizer::insertShadowTagCheck(Value *Ptr,
-                                         Instruction *InsertBefore) {
+HWAddressSanitizer::insertShadowTagCheck(Value *Ptr, Instruction *InsertBefore,
+                                         DomTreeUpdater *DTU, LoopInfo *LI) {


Across all those interfaces, both DTU and LI will always be non-null. Better to pass as reference?

Good idea, done!

…ntally This patch incrementally updates the DominatorTreeAnalysis, PostDominatorTreeAnalysis and LoopAnalysis whenever SplitBlockAndInsertIfThen is called, which fixes the following issue: Suppose we have two functions, F and G. HWAddressSanitizerPass::run will first sanitize F, which potentially makes the analyses of F out of date. When G is then sanitized, it may call the global stack safety analysis, which can crash if the analysis of F is stale. Additionally, since the DominatorTreeAnalysis, PostDominatorTreeAnalysis and LoopAnalysis are now incrementally updated, they do not need to be abandoned at the end of the pass. Bug report: llvm#66934

aeubanks · 2023-09-28T01:50:27Z

title needs to be updated

fhahn

Thanks for the latest updates, LGTM!

thurstond · 2023-09-28T16:06:46Z

title needs to be updated

@aeubanks Done, thanks for the heads up!

thurstond · 2023-09-28T16:07:05Z

Thanks for the latest updates, LGTM!

@fhahn Thanks for all the helpful guidance!

…ntally (llvm#66935) HWAddressSanitizerPass::run sanitizes functions one by one. The sanitization of each function - which may split blocks via insertShadowTagCheck - may result in some cached analyses are invalid. This matters because sanitizeFunction(F', FAM) may indirectly call the global stack safety analysis, hence we need to make sure the analyses of F are up to date. Bug report: llvm#66934

llvmbot added compiler-rt:sanitizer llvm:transforms labels Sep 20, 2023

fmayer approved these changes Sep 20, 2023

View reviewed changes

fhahn reviewed Sep 21, 2023

View reviewed changes

fmayer reviewed Sep 21, 2023

View reviewed changes

fmayer approved these changes Sep 21, 2023

View reviewed changes

fhahn reviewed Sep 22, 2023

View reviewed changes

vitalybuka approved these changes Sep 27, 2023

View reviewed changes

thurstond requested a review from fhahn September 27, 2023 22:19

fhahn approved these changes Sep 28, 2023

View reviewed changes

thurstond changed the title ~~[hwasan] Invalidate DominatorTreeAnalysis after each function is sanitized~~ [hwasan] Update (Post-)DominatorTreeAnalysis and LoopAnalysis incrementally Sep 28, 2023

thurstond merged commit b3b6ede into llvm:main Sep 28, 2023

[hwasan] Update (Post-)DominatorTreeAnalysis and LoopAnalysis incrementally #66935

[hwasan] Update (Post-)DominatorTreeAnalysis and LoopAnalysis incrementally #66935

Uh oh!

Conversation

thurstond commented Sep 20, 2023

Uh oh!

llvmbot commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmayer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks commented Sep 28, 2023

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

thurstond commented Sep 28, 2023

Uh oh!

thurstond commented Sep 28, 2023

Uh oh!

Uh oh!

llvmbot commented Sep 20, 2023 •

edited

Loading