[PATCH] D105703: [hwasan] Use stack safety analysis.

Vitaly Buka via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 21 17:24:56 PDT 2021


vitalybuka accepted this revision.
vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:409
+    // analysis.
+    // This is so we don't need to plumb TargetTriple all the way to here.
+    if (mightUseStackSafetyAnalysis(DisableOptimization))
----------------
I think this function without comment is clear enough. But up to you.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:410
+    Dummy.setArch(Triple::aarch64);
+    if (shouldUseStackSafetyAnalysis(Dummy, DisableOptimization))
+      AU.addRequired<StackSafetyGlobalInfoWrapperPass>();
----------------
fmayer wrote:
> fmayer wrote:
> > vitalybuka wrote:
> > > Why not just:
> > > ```
> > > if (!DisableOptimization || ClUseStackSafety)
> > >   AU.addRequired<StackSafetyGlobalInfoWrapperPass>();
> > > ```
> > Because `ClUseStackSafety` defaults to true, so this will be quite useless except if the user explicitly sets it to false. So then you end up duplicating the logic in `shouldUseStackSafetyAnalysis`, which is why I went for the Dummy (thinking about it, we don't actually need to setArch on it).
> How about this (mightUseStackSafetyAnalysis)? This seems like a nice tradeoff without duplication of the logic.
LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105703/new/

https://reviews.llvm.org/D105703



More information about the llvm-commits mailing list