[PATCH] D108457: [hwasan] Do not instrument accesses to uninteresting allocas.

[Florian Mayer via Phabricator via llvm-commits]

fmayer added a comment.

I split the stack safety change to D109503 <https://reviews.llvm.org/D109503> and added more tests to that.



================
Comment at: llvm/include/llvm/Analysis/StackSafetyAnalysis.h:67
   mutable std::unique_ptr<InfoTy> Info;
+  mutable std::unique_ptr<std::map<const Instruction *, bool>> Accesses;
   const InfoTy &getInfo() const;
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > why do we need bool if missing instruction is equivalent to false?
> > std::set?
> just std::map<const Instruction *, bool>Accesses;
> 
> InfoTy in unique_ptr to avoid exposing it into the header.
> 
> 
While I build this up, there are two cases:

the instruction hadn't been considered yet
the instruction was considered but wasn't safe.
This allows to conveniently keep them apart.


================
Comment at: llvm/include/llvm/Analysis/StackSafetyAnalysis.h:67
   mutable std::unique_ptr<InfoTy> Info;
+  mutable std::unique_ptr<std::map<const Instruction *, bool>> Accesses;
   const InfoTy &getInfo() const;
----------------
fmayer wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > why do we need bool if missing instruction is equivalent to false?
> > > std::set?
> > just std::map<const Instruction *, bool>Accesses;
> > 
> > InfoTy in unique_ptr to avoid exposing it into the header.
> > 
> > 
> While I build this up, there are two cases:
> 
> the instruction hadn't been considered yet
> the instruction was considered but wasn't safe.
> This allows to conveniently keep them apart.
I reverted to calculating this in getInfo and putting it into the InfoTy.

@eugenis is that okay?


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:141-143
+      Accesses.erase(It);
+      std::tie(It, Inserted) = Accesses.emplace(I, Union);
+      assert(Inserted);
----------------
vitalybuka wrote:
> the point of <it, bool> result that you can update if insert failed and avoid the second lookup
That doesn't work, because it's not move constructible (I think, I don't fully remember what exactly it complained about when I did that).


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:455
 
-  return true;
+  return !US.Range.isFullSet();
 }
----------------
vitalybuka wrote:
> may I ask you to extract two NFC patches which you can land without review:
> 1. break -> return !US.Range.isFullSet();   I assume no tests need updates.
> 2. Wrap updateRange into trivial:
> ```
> void addRange(I, R) {
>    // the rest you will add in D108457
>    updateRange(R);
>  }
> ```
> I assume also not tests should be affected.
Actually no one looks at the return value, so I just made it `void`.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:836
         }
+        for (const auto &A : KV.second.Accesses) {
+          bool IsSafe = AIRange.contains(A.second);
----------------
vitalybuka wrote:
> eugenis wrote:
> > This affect compilation time and memory. Ideally we would not do it if the client can not use this info (ex. MTE).
> I am a little bit skeptical that we can measure a difference. I'd rather keep it simple.
I reverted it to what it was before for simplicity. We can change it again later if it becomes a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108457