<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Apr 29, 2016 at 3:37 PM Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Dean, Eric,<br>
<br>
This is great!  I'll wait for others from the community to chime in,<br>
but I'm happy to see this land upstream.<br>
<br></blockquote><div><br></div><div>Thanks Sanjoy!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 > details are contained in the whitepaper at:<br>
 > <a href="https://storage.googleapis.com/xray-downloads/whitepaper/XRayAFunctionCallTracingSystem.pdf" rel="noreferrer" target="_blank">https://storage.googleapis.com/xray-downloads/whitepaper/XRayAFunctionCallTracingSystem.pdf</a><br>
<br>
In the code that you patch in, have you considered excising the "r10d<br>
= <function id>" bit and just having a 5 byte call to<br>
__xray_FunctionEntryStub?  If "<function id>" is a function of the PC<br>
being instrumented then you should be able to reconstruct it by<br>
peeking into the return PC on the stack.  This means your return<br>
sequence will have to call @__xray_FunctionExitStub (instead of<br>
branching), so that the return PC on the stack is correct.<br>
<br></blockquote><div><br></div><div>This was a tradeoff between code-size and runtime cost. Every time we need to find the function id given the return PC on the stack, we'd have to look it up from a table mapping PC->function id. We've made the trade-off such that we pay some cost in code size but save on the costs at runtime.</div><div><br></div><div>This might turn out to be something that could be tuned depending on the situation, but the current implementation favours lower overheads at runtime when tracing more than trying to save on binary size. I'm open to probably having an alternative implementation of this later that optimises for code-size so that's definitely worth exploring.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Using a 5 byte call will let you keep one 5 byte nop instead of the 10/11<br>
byte nop sequence you have today (will really only help for the prologue).<br>
<br>
 > *LLVM:* Add new LLVM pseudo instructions for indicating where the<br>
 > instrumentation sleds should be placed, and what kind of sleds these<br>
 > are. For entry sleds, we can use the `PATCHABLE_OP` instruction, and a<br>
 > new instruction for patching exits (`PATCHABLE_RET`?) that get handled<br>
 > by the assembly printer per platform. Currently only implemented for<br>
 > x86_64.<br>
<br>
Nitpicky comment -- we can discuss this in more detail during code<br>
review:<br>
<br>
I'd prefer to add more functionality to `PATCHABLE_OP` instead of<br>
introducing yet one more pseudo instruction.  `PATCHABLE_OP` can<br>
already wrap an arbitrary machine instruction, teaching it a few<br>
different flavors of nop-insertion should not be a big deal.<br>
<br></blockquote><div><br></div><div>Indeed -- although I'm more worried about the stack lowering code getting affected by the fact that `PATCHABLE_OP` is not a return instruction nor is it a terminator. Whether it's possible to make `PATCHABLE_OP` count as both a normal instruction and a terminator/return at the same time is something I haven't considered.</div><div><br></div><div>Note that I tried doing this with a single pseudo instruction, and in my prototype implementation it got really messy (mostly because stack adjustments in prologue emission is predicated on finding a return instruction to trigger the logic). Maybe we just need to change that part to handle `PATCHABLE_OP` differently to work appropriately, but definitely something to discuss in code review. :D</div><div><br></div><div>Cheers</div></div></div>