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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

In D105703#2883666 <https://reviews.llvm.org/D105703#2883666>, @eugenis wrote:

> Btw Vitaly, will this work with LTO out of the box? I think we used to add pre-LTO StackSafety pass explicitly for memtag only, but it looks like that code is gone.

What do you mean gone? Can you show me the patch?

We probably needs something to add analysis for HWASAN as well.



================
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:
> > fmayer wrote:
> > > vitalybuka wrote:
> > > > 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.
> > > > 
> > > I'll leave it as is, for consistency within this file, as we need to do it this way for the new pass manager.
> > That's what I don't like, passing Triple from BackendUtil.cpp
> > I've updated the patch. You can download it with "arc patch D105703"
> OK, no strong feelings but now we addRequire the pass even if we don't need it, so there isn't much point turning it off anymore, no?
In case of legacy pass? Correct. I don't think we need to over optimize this deprecated case which is going to be removed at some point anyway.
Let's check DisableOptimization as we have it here anyway, but avoid forwarding Triple.

For new PM it's irrelevant as it's does not declare requirements in advance.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:427
+                                             bool DisableOptimization) {
   assert(!CompileKernel || Recover);
+  return new HWAddressSanitizerLegacyPass(CompileKernel, Recover,
----------------
@eugenis unrelated to the patch, but why do we this args if then we assert(!CompileKernel || Recover);


================
Comment at: llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll:5
+; ModuleID = '/usr/local/google/home/fmayer/llvm-project/clang/test/CodeGen/hwasan-stack-safety-analysis.c'
+source_filename = "/usr/local/google/home/fmayer/llvm-project/clang/test/CodeGen/hwasan-stack-safety-analysis.c"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
----------------
you don't need to hardcode local path here


================
Comment at: llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll:10
+; Function Attrs: mustprogress nofree nounwind uwtable willreturn
+define i32 @test_load(i32* %a) sanitize_hwaddress {
+entry:
----------------
would be nice to have also function which have __hwasan_generate_tag in either case to make sure 
-hwasan-use-stack-safety=1 does not disable tagging completely.


================
Comment at: llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll:27
+
+attributes #0 = { mustprogress nofree nounwind uwtable willreturn "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" }
+attributes #1 = { argmemonly mustprogress nofree nosync nounwind willreturn }
----------------
usually we remove irrelevant attributes #* and module flags !* to make test simpler


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703