[PATCH] enable stackmap generation for ELF

[Filip Pizlo]

> On Dec 12, 2013, at 8:06 PM, Andrew Trick <atrick at apple.com> wrote:
> 
> I’ll try to answer a few of your questions and let Lang follow up on your patch, which seems ok to me…
> 
>> On Dec 12, 2013, at 3:44 PM, Kevin Modzelewski <kmod at dropbox.com> wrote:
>> 
>> Hi, I've been playing around with patchpoint and stackmap, but it looks like right now it's only turned on for MachO, so here are the changes I needed to get it working on Linux+ELF.  This is my first attempt to contribute something to LLVM so please direct me as appropriate :)
>> 
>> I just tried to copy the way it works for MachO, since I don't have a great understanding of the requirements either for ELF or stackmaps.  Also, I modified Intrinsic::getType to support variadic intrinsics so that Intrinsic::getDeclaration() works (is there some other way that I should be getting a reference to the intrinsics instead?).  I also included a test to try to be a good citizen but it's pretty much just a straight copy of the macho test; does it make sense to cut it down to just a simple test that makes sure the stackmap gets emitted, and leave the in-depth testing to the current test?
> 
> I don’t think you need to copy all the tests, just a a few carefully chosen ones.
> 
>> I've been using these changes and I've gotten patchpoint-based inline caches (for callsites) working, so first of all thanks for adding these features!  I had a couple observations from using it; I'm not trying to claim that the changes should be made, but I just wanted to give them as data points:
>> - It would have been nice if stackmap ids were 64 bits, since then you can embed pointers into them instead of having to have an indirection layer to map ints back to your compiler-level objects
> 
> An argument for 32-bit ID’s is that some runtimes may want to materialize the constant in the code stream and materializing 64-bits is more expensive. That said, I don’t mind   growing the stack map format and would rather do it now than later.
> 
> Phil, do you have any opinion? Mind if we change the format?

Yeah it's fine!

> 
>> - Since I actually want to use the call that patchpoint() generates, and then lazily patch it the first time it's hit, it made a lot more sense for me to put the call at the end of the patchpoint region rather than the beginning.  Otherwise, when the call returns you're in the middle of the patchpoint region and you have to make sure to patch it carefully in a way that leaves that valid.  If it's at the end, you return to the end of the patchpoint which feels more natural, and also makes it slightly more robust to map the return address to the patchpoint.  I'm not suggesting that it needs to be done this way by default, but in my system I move all the calls to the end as soon as they get emitted.
> 
> I have been saying that the JIT/runtime should make no assumption about the code LLVM emits inside the patchpoint’s reserved area. When the runtime patches, it must patch over all reserved bytes.
> 
> But, I agree, it would be simpler if the return address were one past the end of the patchpoint rather than potentially somewhere in the middle. It seems like a good change. If you submit a patch, I think we can take it.
> 
>> - I don't quite understand the idea of using the MemoryManager to get access to the stackmap section; in fact I had originally not added the SHF_ALLOC flag to the section which meant that it didn't get allocated through the MemoryManager at all, but regardless it still ends up in the emitted object file.  I end up getting the stackmap data by parsing it out in a JITEventListener, which feels a little more natural to me.
> 
> Ideally, the stack map section should not need to be loaded. i.e. it would be nice to avoid making a copy of it. But the MCJIT client does not necessarily know how to parse the object format, so I don’t know if WebKit could use an event listener. Maybe Lang can answer this better.

Is JITEventListener available for MCJIT clients?

> 
> -Andy
> 
>> 
>> 
>> Thanks again for adding the feature,
>> Kevin
>> <enable_elf_stackmaps.patch>
>