[PATCH] [asan-asm-instrumentation] Fixed memory references which includes %rsp as a base or an index register.

Yuri Gorshenin ygorshenin at chromium.org
Mon Oct 13 00:52:10 PDT 2014


PTAL

================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:137
@@ +136,3 @@
+    MCInst Inst;
+    switch (VT) {
+    case MVT::i32:
----------------
eugenis wrote:
> this can be coded in 2 lines:
> assert(VT==MVT::i32 || VT== MVT::i64);
> Inst.setOpcode(VT==MVT::i32 ? X86::LEA32r : X86::LEA64r);
> 
Done.

================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:366
@@ +365,3 @@
+         OrigDisplacement <= MaxAllowedDisplacement);
+  Displacement += OrigDisplacement;
+
----------------
eugenis wrote:
> Both of this can be either positive or negative, right?
> You need to clip against both MaxAllowedDisplacement and MinAllowedDisplacement then.
Done, but there're no need to check against MinAllowedDisplacement, since Displacement is always non-negative, and OrigDisplacement is always 32-bit signed integer.

================
Comment at: test/Instrumentation/AddressSanitizer/X86/asm_rsp_mem_op.s:11
@@ +10,3 @@
+# CHECK: pushfq
+# CHECK: leaq 160(%rsp), %rdi
+# CHECK: callq __asan_report_load8 at PLT
----------------
eugenis wrote:
> Please add tests for borderline cases (i.e. when we require 2 LEAs).
> Btw, are you sure that the displacement limits in the code are correct? Are they defined somewhere in X86 target? Can they be tested? Please make sure that at least 1 test fails when displacement limits are set too large.
Added border-line test. Not sure that the displacement limits are tested somewhere. Manual tests shows that large displacements (more than 32-bit) are just cut by assembler.

http://reviews.llvm.org/D5599






More information about the llvm-commits mailing list