This is an archive of the discontinued LLVM Phabricator instance.

[asan-asm-instrumentation] Fixed memory accesses with rbp as a base or an index register.
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

ygorshenin updated this revision to Diff 15007.Oct 16 2014, 4:21 AM
ygorshenin retitled this revision from to [asan-asm-instrumentation] Fixed memory accesses with rbp 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).
eugenis added inline comments.Oct 17 2014, 11:29 AM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
30 ↗(On Diff #15007)

why do you need cassert? This file has used assertions before.

121 ↗(On Diff #15007)

Can LocalFrameReg be part of RegisterContext?

332 ↗(On Diff #15007)

Maybe RegisterContext::addBusyReg() accepting a register or an X86Operand, and then choose the frame register in InstrumentMemOperandPrologue?

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s
27 ↗(On Diff #15007)

Hm, why is this test so complex? There are 4 memory accesses here => we emit prologue/epilogue 4 times, and it's not obvious which one is tested. Can you make it simpler? It looks like this test covers the new code, but I'm not 100% sure.

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
30 ↗(On Diff #15007)

And it uses them now. So, I thought, it's a right thing to include explicitly <cassert> and do not depend on an implicit inclusion.

121 ↗(On Diff #15007)

Good point, thanks!

332 ↗(On Diff #15007)

Good point, thanks!

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s
27 ↗(On Diff #15007)

Done.

eugenis accepted this revision.Oct 20 2014, 10:39 AM
eugenis edited edge metadata.

LGTM w/ nits

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
74 ↗(On Diff #15136)

I think these should start with a capital letter.

79 ↗(On Diff #15136)

I'd prefer named constants (or an enum) instead of magic index constants 0, 1, 2.

92 ↗(On Diff #15136)

This may add NoRegister to BusyRegs, better avoid it.

This revision is now accepted and ready to land.Oct 20 2014, 10:39 AM
ygorshenin updated this revision to Diff 15178.Oct 21 2014, 2:51 AM
ygorshenin edited edge metadata.

Fixes.

Many thanks!

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
74 ↗(On Diff #15136)

Done.

79 ↗(On Diff #15136)

Done.

92 ↗(On Diff #15136)

Done.

ygorshenin closed this revision.Oct 21 2014, 3:32 AM
ygorshenin updated this revision to Diff 15181.

Closed by commit rL220283 (authored by @ygorshenin).