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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

Other than missing llvm test is LGTM



================
Comment at: clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c:4
+
+int main(int argc, char **argv) {
+  char buf[10];
----------------
fmayer wrote:
> vitalybuka wrote:
> > this patch mostly change code under llvm/ so tests should be also there, as IR tests
> > 
> > 
> I don't have strong feelings, but clang/test/CodeGen/lifetime-sanitizer.c is a very similar test, so I think we should either move all of these to llvm/ or add the new ones here to clang/. What do you think?
That lifetime tests how clang inserts lifetime markers. So it must be in clang/ this is from https://reviews.llvm.org/D20759 which is clang only patch.
Here the only change for clang is forwarded BuilderWrapper.getTargetTriple().
I don't mind if you keep your tests here, but we also need something which tests llvm without clang as you change llvm tranformation.
Usually if contributor changes code in llvm/, expectation is that check-llvm should discover regression. It's not always possible, but that's the goal and easy to do with this patch.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h:40
+  Triple TargetTriple;
+  bool IsOptNull;
 };
----------------
!IsOptNull -> Optimize
or
IsOptNull -> DisableOptimization



================
Comment at: llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h:32
+                                  Triple TargetTriple = {});
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM);
   static bool isRequired() { return true; }
----------------
fmayer wrote:
> vitalybuka wrote:
> > Why not from M.getTargetTriple() ?
> Mostly for consistency with the legacy pass. Either way is fine for me though, what do you prefer?
I don't know if will cause any issues, but usually most passes get triple from the module.
I prefer we stay consistent with the rest of the code if possible.



================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:444
+  const StackSafetyGlobalInfo *SSI = nullptr;
+  if (shouldUseStackSafetyAnalysis(TargetTriple, IsOptNull)) {
+    SSI = &MAM.getResult<StackSafetyGlobalAnalysis>(M);
----------------
we usually don't use {} for single line 
also maybe good candidate for ?: operator


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:390
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    if (shouldUseStackSafetyAnalysis(TargetTriple)) {
+      AU.addRequired<StackSafetyGlobalInfoWrapperPass>();
----------------
fmayer wrote:
> vitalybuka wrote:
> > why we need to check TargetTriple for that?
> Because we only need the stack safety analysis if we instrument the stack, which we do not do on x86_64 (see shouldInstrumentStack).
I see, I forgot about this limitation.
LGTM, but unconditional is fine as well, assuming we are going to have stack instrumentation at some point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703