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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
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;
----------------
why do we need bool if missing instruction is equivalent to false?
std::set?


================
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:
> 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.




================
Comment at: llvm/include/llvm/Analysis/StackSafetyAnalysis.h:69
   const InfoTy &getInfo() const;
+  const std::map<const Instruction *, bool> &getAccesses() const;
 
----------------
eugenis wrote:
> you want either DenseMap or SmallPtrSet here. std::map is unnecessarily ordered, logarithmic, and wastes memory
std set/map seems fine
class is movable, DenseMap or SmallPtrSet don't support efficient move.




================
Comment at: llvm/include/llvm/Analysis/StackSafetyAnalysis.h:69
   const InfoTy &getInfo() const;
+  const std::map<const Instruction *, bool> &getAccesses() const;
 
----------------
vitalybuka wrote:
> eugenis wrote:
> > you want either DenseMap or SmallPtrSet here. std::map is unnecessarily ordered, logarithmic, and wastes memory
> std set/map seems fine
> class is movable, DenseMap or SmallPtrSet don't support efficient move.
> 
> 
I don't see value in ::getAccesses when accessIsSafe can access the field.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:136-139
+    bool Inserted;
+    std::map<const Instruction *, const ConstantRange>::iterator It;
+    std::tie(It, Inserted) = Accesses.emplace(I, R);
+    if (!Inserted) {
----------------
Less boilerplate this way:
auto Ins = Accesses.emplace(I, R);
Then use Ins.first and Ins.second where needed.



================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:141-143
+      Accesses.erase(It);
+      std::tie(It, Inserted) = Accesses.emplace(I, Union);
+      assert(Inserted);
----------------
the point of <it, bool> result that you can update if insert failed and avoid the second lookup


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:455
 
-  return true;
+  return !US.Range.isFullSet();
 }
----------------
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.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:813
   getInfo().Info.print(O, F->getName(), dyn_cast<Function>(F));
+  O << "\n";
+}
----------------
this is a separate NFC patch


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:826
+        for (const auto &A : KV.second.Accesses) {
+          bool IsSafe = AIRange.contains(A.second);
+          bool Inserted;
----------------
so we lazily construct this on the request
if so I am pretty sure we can have sorted std::vector<Instruction*> Accesses for free;
and accessIsSafe can use std::binary_search and it will be faster then map/set lookups.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:836
         }
+        for (const auto &A : KV.second.Accesses) {
+          bool IsSafe = AIRange.contains(A.second);
----------------
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.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:290
   bool instrumentMemAccess(InterestingMemoryOperand &O);
-  bool ignoreAccess(Value *Ptr);
+  bool ignoreAccess(Instruction *Inst, Value *Ptr);
   void getInterestingMemoryOperands(
----------------
HWAsan and related tests should be in a separate patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108457