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

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

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

145

Can LocalFrameReg be part of RegisterContext?

352

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

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

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.

145

Good point, thanks!

352

Good point, thanks!

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s
27

Done.

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

LGTM w/ nits

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
97–98

I think these should start with a capital letter.

103–111

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

123

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
97–98

Done.

103–111

Done.

123

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).