Page MenuHomePhabricator

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

Authored by ygorshenin on Oct 3 2014, 4:21 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

ygorshenin updated this revision to Diff 14372.Oct 3 2014, 4:21 AM
ygorshenin retitled this revision from to [asan-asm-instrumentation] Fixed memory references which includes %rsp as a base or an index register..
ygorshenin updated this object.
ygorshenin edited the test plan for this revision. (Show Details)
ygorshenin added a reviewer: eugenis.
ygorshenin added a subscriber: Unknown Object (MLST).
ygorshenin updated this revision to Diff 14448.Oct 6 2014, 5:34 AM

Fixed a case when displacement runs over 32-bit limit.

ygorshenin updated this revision to Diff 14449.Oct 6 2014, 5:37 AM

Tiny fixes.

ygorshenin updated this revision to Diff 14450.Oct 6 2014, 5:53 AM

Added assertion about possible values of displacement.

eugenis added inline comments.Oct 7 2014, 1:37 PM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
302 ↗(On Diff #14450)

I think the common term is "spill".

707 ↗(On Diff #14450)

I wonder if this could be moved to the parent class to reduce code duplication.

test/Instrumentation/AddressSanitizer/X86/asm_rsp_mem_op.s
12 ↗(On Diff #14450)

This could be done with 1 lea instruction.

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
302 ↗(On Diff #14450)

Done.

707 ↗(On Diff #14450)

Done.

test/Instrumentation/AddressSanitizer/X86/asm_rsp_mem_op.s
12 ↗(On Diff #14450)

It significantly complicates an algorithm, but done.

eugenis added inline comments.Oct 9 2014, 1:59 AM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
137 ↗(On Diff #14577)

this can be coded in 2 lines:
assert(VT==MVT::i32 || VT== MVT::i64);
Inst.setOpcode(VT==MVT::i32 ? X86::LEA32r : X86::LEA64r);

366 ↗(On Diff #14577)

Both of this can be either positive or negative, right?
You need to clip against both MaxAllowedDisplacement and MinAllowedDisplacement then.

test/Instrumentation/AddressSanitizer/X86/asm_rsp_mem_op.s
11 ↗(On Diff #14577)

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.

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
137 ↗(On Diff #14577)

Done.

366 ↗(On Diff #14577)

Done, but there're no need to check against MinAllowedDisplacement, since Displacement is always non-negative, and OrigDisplacement is always 32-bit signed integer.

test/Instrumentation/AddressSanitizer/X86/asm_rsp_mem_op.s
11 ↗(On Diff #14577)

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.

eugenis accepted this revision.Oct 13 2014, 1:05 AM
eugenis edited edge metadata.

LGTM w/ some minor comments

compiler-rt tests for this case would be nice, too

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
366 ↗(On Diff #14577)

Ah, then add an assert(Displacement >= 0) instead of the lower bound check, or just leave it like this. Your choice.

42 ↗(On Diff #14786)

ApplyDisplacementBounds or smth

test/Instrumentation/AddressSanitizer/X86/asm_rsp_mem_op.s
11 ↗(On Diff #14577)

Nice.

This revision is now accepted and ready to land.Oct 13 2014, 1:05 AM
ygorshenin closed this revision.Oct 13 2014, 2:48 AM
ygorshenin updated this revision to Diff 14789.

Closed by commit rL219602 (authored by @ygorshenin).