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

[Florian Mayer via Phabricator via cfe-commits]

fmayer marked an inline comment as done and an inline comment as not done.
fmayer added a comment.

Addressed inline comments.



================
Comment at: clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c:4
+
+int main(int argc, char **argv) {
+  char buf[10];
----------------
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?


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h:32
+                                  Triple TargetTriple = {});
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM);
   static bool isRequired() { return true; }
----------------
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?


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:390
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    if (shouldUseStackSafetyAnalysis(TargetTriple)) {
+      AU.addRequired<StackSafetyGlobalInfoWrapperPass>();
----------------
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).


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:1275
 bool HWAddressSanitizer::isInterestingAlloca(const AllocaInst &AI) {
+  // clang-format off
   return (AI.getAllocatedType()->isSized() &&
----------------
vitalybuka wrote:
> Instead of // clang-format off
> could you replace this with
> 
> ```
> # comment1
> if (...)
>   return false;
> 
> # comment2
> if (...)
>   return false;
> ...
> 
> ```
> 
> in a separate patch?
OK, will do in a followup (or see how I can get clang-tidy to do the right thing here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105703