[PATCH] D34789: Copy arguments passed by value into explicit allocas for ASan

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:2543
+  BasicBlock &FirstBB = *F.begin();
+  Instruction *InsBefore = dyn_cast<Instruction>(FirstBB.begin());
+  IRBuilder<> IRB(InsBefore);
----------------
Not sure if it matters here, but this is usually done with BasicBlock::getFirstInsertionPt()


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:2545
+  IRBuilder<> IRB(InsBefore);
+  unsigned argNum = 0;
+  for (Argument &arg : F.args()) {
----------------
Local variables start with a capital letter.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:2553
+          IRB.CreateAlloca(type, nullptr,
+                           Twine("Arg") + std::to_string(argNum) + "Copy");
+      AI->setAlignment(align);
----------------
Better call it something like arg.getName() + ".byval", or whatever looks good in ASan error report. Please also add compiler-rt test for ASan detecting overflow of a byval arg.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:2557
+
+      const Module *m = arg.getParent()->getParent();
+      uint64_t allocSize = m->getDataLayout().getTypeAllocSize(type);
----------------
F.getParent()


================
Comment at: test/Instrumentation/AddressSanitizer/stack-poisoning-byval-args.ll:16
+; CHECK: call i32 @bar
+; CHECK: ret void
+
----------------
Please make this test a little stronger: check that bar() gets a pointer in the combined alloca, not the original %a. Check that memcpy is from %a. Also the size of alloca (96) is irrelevant and checking it can only make the test more brittle.


https://reviews.llvm.org/D34789