This is an archive of the discontinued LLVM Phabricator instance.

[asan-asm-instrumentation] Prologue and epilogue are moved out from InstrumentMemOperand().
ClosedPublic

Authored by ygorshenin on Aug 15 2014, 4:07 AM.

Details

Summary

[asan-asm-instrumentation] Prologue and epilogue are moved out from InstrumentMemOperand().

  • Prologue and epilogue are movedo ut from InstrumentMemOperand();
  • Instrumentation for rep movs doesn't save/restore working register set between consecutive instrumentations for bounds of memory ranges;
  • Fixed a bug that on x86_64 incorrect address was passed to __asan_report functions;

Diff Detail

Event Timeline

ygorshenin updated this revision to Diff 12548.Aug 15 2014, 4:07 AM
ygorshenin retitled this revision from to [asan-asm-instrumentation] Prologue and epilogue arem oved out from InstrumentMemOperand()..
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 retitled this revision from [asan-asm-instrumentation] Prologue and epilogue arem oved out from InstrumentMemOperand(). to [asan-asm-instrumentation] Prologue and epilogue are moved out from InstrumentMemOperand()..Aug 15 2014, 4:10 AM
ygorshenin updated this object.
eugenis added inline comments.Aug 28 2014, 5:17 AM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
70

ExpectedShadowValueReg - if I understand this correctly, this is simply an available scratch register. Please find a better name.

73

AsanContext seems unnecessary and it mixes information about a memory access (which actually changes between prologue and epilogue in case of MOVS) and the instrumentation state (i.e. the spilled scratch register). Also, there can be no more than 1 outstanding AsanContext at any time, and it can be merged with X86AddressSanitizer class.

I'd rather

  • pass accesssize and iswrite to InstrumentMemOperand as before.
  • pass the scratch register to InstrumentMemOperandPrologue and save it right in X86AddressSanitizer (smth like a set of extra spilled registers that must be restored in the epilogue).
165

Too many InstrumentMemOperand* methods. Better repeat this prologue/epilogue calls in InstrumentMOV.

ygorshenin updated this revision to Diff 13039.Aug 28 2014, 8:19 AM

Comments are addressed.

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
70

Renamed to ScratchReg.

73

AccessSize and IsWrite are passed directly as arguments, but I'd prefer to keep scratch register in the register context.

165

Done.

ygorshenin updated this revision to Diff 13077.Aug 29 2014, 4:25 AM

Removed target_link_libraries from CMakeLists.txt.

eugenis accepted this revision.Aug 29 2014, 7:17 AM
eugenis edited edge metadata.

LGTM w/ nit

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
555

Is it possible to remove accesssize argument here and in prologue, and instead pass an invalid/zero value for the scratch register?
And check in instrumentmemoperandsmall that a scratch register is available, for sanity.

This revision is now accepted and ready to land.Aug 29 2014, 7:17 AM
ygorshenin updated this revision to Diff 13084.Aug 29 2014, 7:48 AM
ygorshenin edited edge metadata.

Fixed RegisterContext ctor.

ygorshenin updated this revision to Diff 13086.Aug 29 2014, 7:53 AM

Added checks in InstrumentMemOperandSmall().

Many thanks!

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
555

Done.

ygorshenin closed this revision.Sep 1 2014, 3:23 AM

r216869.

Excuse me to comment the older commit.

This brought new dependency AsmParser on CodeGen. For example, llvm-mc requires LLVMX86CodeGen, despite other LLVM*CodGen(s) are not required.

Any better idea to dissolve such a dependency?

  • Instantiate RegInfo also in X86AsmInstrumentation.cpp.
  • Move X86RegisterInfo to another module
  • Introduce a new module, X86RegisterInfo.

Introduce a new module, X86RegisterInfo.

This sounds OK to me.