Fixed memory accesses with rbp as a base or an index register.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |