<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div>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:</div><div><br></div><div>memset(&functions, 0, sizeof(functions));</div><div><br></div><div>And then the LLVM bindings also memset according to what LLVM sees and does a copy:</div><div><br></div><div>memset(&myFunctions, 0, sizeof(myFunctions));</div><div>memcpy(&myFunctions, PassedFunctions, SizeOfPassedFunctions);</div><div><br></div><div>This ensures both forward source compatibility and forward binary compatibility, except if we wanted to remove a function:</div><div><br></div><div>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. </div><div><br></div><div>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. </div><div><br></div><div>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. :-)</div><div><br></div><div>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</div><div><br>On May 16, 2013, at 1:50 PM, Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, May 16, 2013 at 1:28 PM, Filip Pizlo <span dir="ltr"><<a href="mailto:fpizlo@apple.com" target="_blank">fpizlo@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="im">
<div>On May 16, 2013, at 10:42 AM, Sean Silva <<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>> wrote:</div><br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr">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">https://bugs.webkit.org/show_bug.cgi?id=112840</a>>.</div>
</div></blockquote><div><br></div></div><div>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, </div>
</div></div></blockquote><div><br></div><div style="">Great!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div>but given the amount of work remaining I cannot commit to anything.</div><div class="im"><br></div></div></div></blockquote><div><br></div><div style="">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.</div>
<div style=""><br></div><div style="">(Now to review the patch).</div><div style=""><br></div><div style="">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.</div>
<div style=""><br></div><div style="">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.</div>
<div style=""><br></div><div style="">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?</div><div style=""><br></div><div style="">
<div>+ if (options.SizeOfMCJMMFunctions > sizeof(functions)) {</div><div>+ *OutError = strdup(</div><div>+ "Refusing to use MCJIT memory manager functions struct that is larger "</div><div>+ "than my own; assuming LLVM library mismatch.");</div>
<div>+ return 1;</div><div>+ }</div><div><br></div><div style="">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.</div>
<div style=""><br></div><div style="">-- Sean Silva</div></div></div></div></div>
</div></blockquote></body></html>