[PATCH] D40932: Hardware-assisted AddressSanitizer (llvm part).

[Kostya Serebryany via Phabricator via llvm-commits]

kcc added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:15
+
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/ArrayRef.h"
----------------
eugenis wrote:
> kcc wrote:
> > Do you really need all these includes? 
> I'm not sure how to find the ones that are unused.
I don't think we have a tool for that. 
I've done it like this:
  given e.g. #include "llvm/IR/MDBuilder.h" search for MDBuilder in the code. 
  If none found, remove the include and rebuild. If build passes, you don't need the header. 

There is clearly a risk that some other file actually includes MDBuilder and this code will break
with further refactoring of header files. But that's tolerable IMHO. 


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:226
+  } else if (auto CI = dyn_cast<CallInst>(I)) {
+    auto *F = dyn_cast<Function>(CI->getCalledValue());
+    if (F && (F->getName().startswith("llvm.masked.load.") ||
----------------
do you need this section of code at all? 
If not, I'd suggest to remove it and reinstate if needed, with a test. 


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:256
+
+  if (PtrOperand) {
+    // Do not instrument acesses from different address spaces; we cannot deal
----------------
Do you need this? Do you have a test? 
I'd prefer if we don't add code from asan/msan that we might not need in hwasan 


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:336
+
+  for (auto Inst : ToInstrument) {
+    Changed |= instrumentMemAccess(Inst);
----------------
no {}


https://reviews.llvm.org/D40932