<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><base href="x-msg://11054/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On May 17, 2013, at 10:05 AM, "Kaylor, Andrew" <<a href="mailto:andrew.kaylor@intel.com">andrew.kaylor@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Hi Filip,<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Let me clarify that I’m actually in favor of getting this into the API. I just wanted to highlight the imminent obstacles.<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I’m flexible on the method for passing in the memory manager functions. If you can convince Sean, I’ll be happy with your method.</span></div></div></div></blockquote><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I do have a few suggestions, though.<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><span>1.<span style="font-style: normal; font-variant: normal; font-weight: normal; font-size: 7pt; line-height: normal; font-family: 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">We could put the size of the structure into the structure as the first member. This isn’t a big deal, but it strikes me as a bit odd to have it outside, especially in the options case where it’s an additional parameter.</span></div></div></div></blockquote><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><span>2.<span style="font-style: normal; font-variant: normal; font-weight: normal; font-size: 7pt; line-height: normal; font-family: 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">We could put a version number in the structure. If we did that, we could arguably even change the signatures of functions in future versions if there were a backward compatible way to call older versions of the same functions.</span></div></div></div></blockquote><div><br></div><div>We could "change" the signatures of functions by just extending the struct in the future, with new functions, that have different signatures. We could then require that the user only sets either the old, or new, version of the function.</div><div><br></div><div>Example:</div><div><br></div><div>struct LLVMMCJITMemoryManagerFunctions {</div><div> uint8_t *(*AllocateCodeSegment)(things); /* old, deprecated */</div><div> ... /* more stuff */</div><div> uint8_t *(*AllocateCodeSegmentForModule)(things, LLVMModuleRef); /* new function we added */</div><div>};</div><div><br></div><div>But more on this below...</div><br><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><span>3.<span style="font-style: normal; font-variant: normal; font-weight: normal; font-size: 7pt; line-height: normal; font-family: 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">We could put some sort of a signature in the structure that was set by the binding layer when you made the call to initialize to default values. This would give us a way to be sure that the caller had used our initialization function and not just initialized the values that they knew about.</span></div></div></div></blockquote><div><br></div><div>Note, right now I'm using memset(ptr, 0, size) instead of an initialization function. But this could change.</div><br><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><span>4.<span style="font-style: normal; font-variant: normal; font-weight: normal; font-size: 7pt; line-height: normal; font-family: 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">We could add some kind of a checksum for the function pointer structure so we could verify that what we received and what the user intended to pass in matched. I might be getting paranoid with this one.<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Passing in a structure of pointers to functions that we’re going to call makes me a bit nervous from a security perspective. If the structure grows in a way that the caller doesn’t know about but malicious code might, it’s a point of vulnerability. I just want to make sure that we’ve done enough to protect that point against possible attack.</span></div></div></div></blockquote><div><br></div><div>So it seems that we have a couple of things going on:</div><div><br></div><div>- My current version uses the size-of-structure as a kind of versioning. You're suggesting a version number. A version number could obviate the need for a size-of-structure. Version numbers are better than size-of-structure because having a struct that has multiple versions of the same callback, with a requirement that you only set either the old or new version, is likely to be confusing to users.</div><div><br></div><div>- The current approach doesn't give us a static way of ensuring that the user initialized all of the functions that they should have, or that the user initialized the structure in a binary-compatibiltiy-aware way.</div><div><br></div><div>So what about going with this (I don't know if this is what Sean was thinking or if this is my idea):</div><div><br></div><div>- LLVMCustomMCJITMemoryManager is an opaque.</div><div><br></div><div>- You create it with:</div><div><br></div><div>LLVMCreateCustomMCJITMemoryManager(void *Object, uint8_t *(*AllocateCodeSegment)(...), uint8_t *(*AllocateDataSegment)(...), LLVMBool (*ApplyPermissions)(...), void (*Destroy)(...));</div><div><br></div><div>I.e. the creation function takes all of the callbacks in one go. The consequence of this is that versioning is implicit, and we have a static guarantee that everything was initialized. If we ever wanted to change the callback API in the future we would just create a new construction function:</div><div><br></div><div><div>LLVMCreateCustomMCJITMemoryManagerNew(void *Object, uint8_t *(*AllocateCodeSegment)(...), uint8_t *(*AllocateDataSegment)(...), LLVMBool (*ApplyPermissions)(...), void (*Destroy)(...));</div><div><br></div><div>Or somesuch. I'd prefer to future-proof this API a bit by having the current creation function be called:</div><div><br></div><div><div>LLVMCreateSimpleCustomMCJITMemoryManager(void *Object, uint8_t *(*AllocateCodeSegment)(...), uint8_t *(*AllocateDataSegment)(...), LLVMBool (*ApplyPermissions)(...), void (*Destroy)(...));</div></div><div><br></div><div>Where "Simple" refers to the fact that there is no support for remote JITing and the allocation callbacks don't allow the allow the allocator to reason about multiple modules.</div><div><br></div></div><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">As to your suggestion about having an abstract base class that both SectionMemoryManager and BindingMemoryManager inherit from, I’d rather have an external class that both SectionMemoryManager and BindingMemoryManager aggregate. I expect that it will be at least as common for clients to want to provide their own implementation of getPointerToNamedFunction while accepting the default allocation scheme as the reverse, and probably more common. The registerEHFrames implementation is more architecture specific, so it doesn’t really belong with getPointerToNamedFunction either. In fact, that probably makes sense to go in a base class.</span></div></div></div></blockquote><div><br></div><div>Are you suggesting entirely separate classes like EHFrameRegistrar and NamedFunctionResolver, which are set in the EngineBuilder separately from the RTDyldMemoryManager?</div><div><br></div><div>I agree that this would be good, but I was more suggesting an incremental step that would allow me to extend the C API without also having to make a significant change to the C++ API. A bunch of code currently assumes that RTDyldMemoryManager is also the thing that knows about resolution and EH frames. It will take some carnage to change that, and I was thinking that the intermediate class solution would just be a first step towards both having a sensible C API story and also nudging the C++ API in the right direction.</div><div><br></div><div>-Filip</div><div><br></div><br><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">-Andy<o:p></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(181, 196, 223); padding: 3pt 0in 0in; "><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; ">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; "><span class="Apple-converted-space"> </span>Filip Pizlo [mailto:fpizlo@<a href="http://apple.com" style="color: purple; text-decoration: underline; ">apple.com</a>]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, May 16, 2013 4:48 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Kaylor, Andrew<br><b>Cc:</b><span class="Apple-converted-space"> </span>Sean Silva;<span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a>; Rafael Ávila de Espíndola<br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [PATCH] Expose custom MC-JIT memory allocation through the C API<o:p></o:p></span></div></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">On May 16, 2013, at 3:44 PM, "Kaylor, Andrew" <<a href="mailto:andrew.kaylor@intel.com" style="color: purple; text-decoration: underline; ">andrew.kaylor@intel.com</a>> wrote:<o:p></o:p></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><br><br><o:p></o:p></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I’m a bit concerned about the implications this has for the future rigidity of the memory manager interface. There are definitely some things about that interface that I can see changing.</span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">First, as you mention registerEHFrames isn’t exactly a memory manager function. In the same way, getPointerToNamedFunction isn’t either. The reason these two functions are in the memory manager is that the memory manager is the component that knows where the JITed code is going to end up (i.e. in another process or local). But I can see us wanting to change that.</span><o:p></o:p></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">We can remove those functions from the C API for now. See below.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><blockquote style="margin-top: 5pt; margin-bottom: 5pt; "><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Second, at some point we’re probably going to want to add something to communicate the memory manager what code model is being used. That will probably be just another function being added.</span><o:p></o:p></div></div></blockquote><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Then we can add another function. I don't think that's a showstopper.<o:p></o:p></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><br><br><o:p></o:p></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Third, it’s entirely possible that we’ll want to add a way to associate allocations with a particular module that’s being JITed. Right now, there’s a 1-to-1-to-1 relationship between MCJIT engines, modules and memory managers, but in the near future the MCJIT engine will support multiple modules, and it may be desirable for the memory manager to know which of the sections it is allocating go together. This would involve changing function signatures.</span><o:p></o:p></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">I agree that there are many things that we could add, and that the API may need to be amended. But I don't like the idea of not exposing any API just because of hypotheticals. For example, while it's true that MCJIT will ultimately support multiple modules, it's not clear that this will necessitate changing the MM interface.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">That being said:<o:p></o:p></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><br><br><o:p></o:p></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I realize that this is quite inconvenient for C API usage purposes, but if there’s any way we can design the API to anticipate these sorts of changes I think we should. And of course there are always the changes we don’t yet know we’ll need.</span><o:p></o:p></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">What about making the current API be:<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">allocateCodeSection(size, alignment, sectionID, module)<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">allocateDataSection(size, alignment, sectionID, module, isReadOnly)<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">applyPermissions(module)<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">In the initial cut, the API will provide default implementations of registerEHFrames and getPointerToNamedFunction that do what SectionMemoryManager does. This can initially be done by having an intermediate abstract class that implements registerEHFrames and getPointerToNamedFunction in the same way that SectionMemoryManager does currently, and both SectionMemoryManager and BindingMemoryManager will inherit from it.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">-Filip<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><blockquote style="margin-top: 5pt; margin-bottom: 5pt; "><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">-Andy</span><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span><o:p></o:p></div></div><div><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(181, 196, 223); padding: 3pt 0in 0in; "><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; ">From:</span></b><span class="apple-converted-space"><span style="font-size: 10pt; font-family: Tahoma, sans-serif; "> </span></span><span style="font-size: 10pt; font-family: Tahoma, sans-serif; ">Filip Pizlo [<a href="mailto:fpizlo@apple.com" style="color: purple; text-decoration: underline; ">mailto:fpizlo@apple.com</a>]<span class="apple-converted-space"> </span><br><b>Sent:</b><span class="apple-converted-space"> </span>Thursday, May 16, 2013 2:10 PM<br><b>To:</b><span class="apple-converted-space"> </span>Sean Silva<br><b>Cc:</b><span class="apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a>; Rafael Ávila de Espíndola; Kaylor, Andrew<br><b>Subject:</b><span class="apple-converted-space"> </span>Re: [PATCH] Expose custom MC-JIT memory allocation through the C API</span><o:p></o:p></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">I considered using an opaque struct with getters and setters. But instead I went with the old-school C idiom of having a struct that the user memset's to zero up to the size they saw:<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">memset(&functions, 0, sizeof(functions));<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">And then the LLVM bindings also memset according to what LLVM sees and does a copy:<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">memset(&myFunctions, 0, sizeof(myFunctions));<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">memcpy(&myFunctions, PassedFunctions, SizeOfPassedFunctions);<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">This ensures both forward source compatibility and forward binary compatibility, except if we wanted to remove a function:<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Source compatibility for added functions: the user's compiler would see a larger sizeof(functions), and the memset() would zero-initialize those pointers, causing the bindings to provide default implementations. <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Binary compatibility for added functions: the user would end up passing a value of SizeOfPassedFunctions that is smaller than LLVM expected, and LLVM would zero-initialize the added functions. <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">AFAIK, this is no less robust than an opaque struct. Both handle added functions gracefully, and neither can handle removed functions gracefully unless we do something crazy. The un-opaque struct just makes writing the code a bit easier, both for LLVM and for the client. But that's just my opinion. :-)<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">I am curious what y'all think about the weirder functions like registerEHFrames. It feels weird that this is part of the MM to begin with. <br><br>-Filip<o:p></o:p></div></div><div><p class="MsoNormal" style="margin: 0in 0in 12pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><br>On May 16, 2013, at 1:50 PM, Sean Silva <<a href="mailto:silvas@purdue.edu" style="color: purple; text-decoration: underline; "><span style="color: purple; ">silvas@purdue.edu</span></a>> wrote:<o:p></o:p></p></div><blockquote style="margin-top: 5pt; margin-bottom: 5pt; "><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><p class="MsoNormal" style="margin: 0in 0in 12pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></p><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">On Thu, May 16, 2013 at 1:28 PM, Filip Pizlo <<a href="mailto:fpizlo@apple.com" target="_blank" style="color: purple; text-decoration: underline; "><span style="color: purple; ">fpizlo@apple.com</span></a>> wrote:<o:p></o:p></div></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">On May 16, 2013, at 10:42 AM, Sean Silva <<a href="mailto:silvas@purdue.edu" target="_blank" style="color: purple; text-decoration: underline; "><span style="color: purple; ">silvas@purdue.edu</span></a>> wrote:<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><br><br><br><o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Is basing the JSC fourth tier on LLVM something that you guys have committed to, or mainly exploratory? You seem to describe it as a "study" on <<a href="https://bugs.webkit.org/show_bug.cgi?id=112840" target="_blank" style="color: purple; text-decoration: underline; "><span style="color: purple; ">https://bugs.webkit.org/show_bug.cgi?id=112840</span></a>>.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">If we can get LLVM to provide a speed-up over our own optimizing JIT, then it will be turned on in WebKit trunk. As you can see from that bug, we've put a lot of work into this so far, and still have a lot of work ahead of us. The results so far are promising and I like where it's going,<o:p></o:p></div></div></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Great!<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><blockquote style="border-style: none none none solid; border-left-width: 1pt; border-left-color: rgb(204, 204, 204); padding: 0in 0in 0in 6pt; margin: 5pt 0in 5pt 4.8pt; "><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">but given the amount of work remaining I cannot commit to anything.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div></blockquote><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Given that this is generally useful functionality that will probably be needed by any serious use case, and that your work is already pretty far along, it's probably fine to expose this in the C API.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">(Now to review the patch).<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Factoring out RTDyldMemoryManager into its own header should be its own patch. This code move is probably a good idea to do anyway independently of adding functionality to the C API.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">As for the API change, my concern is that it potentially exposes too much. As far as I can tell, `struct LLVMMCJITMemoryManagerFunctions` is basically a thin wrapper around the vtable of RTDyldMemoryManager, which raises the question of what will happen if RTDyldMemoryManager changes.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Rafael, Andrew: could you take a look at this patch? In particular, is this API stable enough that it will be OK to proxy the RTDyldMemoryManager API like this?<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ if (options.SizeOfMCJMMFunctions > sizeof(functions)) {<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ *OutError = strdup(<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ "Refusing to use MCJIT memory manager functions struct that is larger "<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ "than my own; assuming LLVM library mismatch.");<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ return 1;<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ }<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">In order to avoid this, it would be better to expose a an opaque struct, and have all manipulation of that struct happen through getter/setter functions, which will push library mismatch errors to link time rather than runtime and overall be easier to maintain/extend. That opaque struct could also hold the `void *` callback data. Sadly, the surrounding code already falls into the brittle "sizeof" pattern.<o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "> <o:p></o:p></div></div><div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">-- Sean Silva<o:p></o:p></div></div></div></div></div></blockquote></blockquote></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div></div></blockquote></div><br></body></html>