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

Evgeniy Stepanov eugenis at google.com
Wed Jul 30 04:34:47 PDT 2014


================
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)
----------------
InstrumentMOVS has a return value that you probably want to check here for early exit.


================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:117
@@ +116,3 @@
+    MCContext &Ctx, MCStreamer &Out) {
+  // Test (%SrcReg)
+  {
----------------
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?


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2289
@@ +2288,3 @@
+                                         Out, &SkipInstruction);
+  if (!SkipInstruction)
+    Out.EmitInstruction(Inst, STI);
----------------
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?


================
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}
----------------
get rid of this metadata if it's not used in the test

http://reviews.llvm.org/D4719






More information about the llvm-commits mailing list