[PATCH] [asan-assembly-instrumentation] Added instrumentation for REP MOVS.

Yuri Gorshenin ygorshenin at chromium.org
Wed Jul 30 06:28:54 PDT 2014


PTAL

================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:54
@@ +53,3 @@
+      const MCInstrInfo &MII, MCStreamer &Out, bool *SkipInstruction) override {
+    InstrumentMOVS(Inst, Operands, Ctx, MII, Out);
+    if (RepPrefix)
----------------
Evgeniy Stepanov wrote:
> InstrumentMOVS has a return value that you probably want to check here for early exit.
> 
I'd prefer to get rid of return values for Instrument*() methods, as they're complicate the logic.

================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:117
@@ +116,3 @@
+    MCContext &Ctx, MCStreamer &Out) {
+  // Test (%SrcReg)
+  {
----------------
Evgeniy Stepanov wrote:
> In memcpy() we check shadow for entire source and destination ranges. I guess it's not easy to do efficiently in asm, but at least leave a FIXME.
> 
> Also, this code does push eax/ecx/edx/flags four times - once for each InstrumentMemOperand. Factor prologue/epilogue generation out of InstrumentMemOperand?
> 
I've added FIXME comments for both issues. Would you mind if I'll resolve second issue in a next CL?

================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2289
@@ +2288,3 @@
+                                         Out, &SkipInstruction);
+  if (!SkipInstruction)
+    Out.EmitInstruction(Inst, STI);
----------------
Evgeniy Stepanov wrote:
> This looks too complex, with some of the original instructions emitted directly in AsmParser, and some (REP) in instrumentation code. Could we always emit original instruction (as needed) in InstrumentInstruction?
> 
Done.

================
Comment at: test/Instrumentation/AddressSanitizer/X86/asm_rep_movs.ll:71
@@ +70,3 @@
+
+!0 = metadata !{metadata !"clang version 3.5.0 "}
+!1 = metadata !{i32 61, i32 74}
----------------
Evgeniy Stepanov wrote:
> get rid of this metadata if it's not used in the test
Done.

http://reviews.llvm.org/D4719






More information about the llvm-commits mailing list