[vmkit-commits] [PATCH] Various patches

[Will Dietz]

On Sat, Oct 22, 2011 at 1:43 PM, Nicolas Geoffray
<nicolas.geoffray at gmail.com> wrote:
> Hi Will,
> Great work! I have a few meta-comments, and some comments on the code.
>

Thanks!  Sorry for not responding to your earlier request, got caught
up in a few other things unfortunately.

>>
>> From fb3971d4914e2902716e1c860c47331cca5a1ea4 Mon Sep 17 00:00:00 2001
>> From: Will Dietz <w at wdtz.org>
>> Date: Fri, 14 Oct 2011 05:34:37 -0500
>> Subject: [PATCH 8/8] Implement some basic registerNatives support.
>> Strategy: we're given the dynamic address of a particular java function.
>> Attempt to reverse-lookup the memory address to see if a)we implement
>> the mentioned function and b)the function has a symbol we can use to
>> look it up.
>> If so, store the symbol with the function, and use that to resolve the
>> function in the future (works across AOT compilation).
>
>
> If it wasn't for AOT, would you need the two fields overridePtr and symName?
> It looks to me like you could just set the 'code' field of a JavaMethod and
> be done with it.
> About AOT, we are going to always execute the bootstrap of OpenJDK, so the
> RegisterNatives will always be called, right?
> Basically, I'd like to know why/if these two extra fields on a JavaMethod
> are necessary. I haven't really tuned vmkit for space, but adding two extra
> fields for each method, when it's really only used for a couple of methods
> seems like a waste of space. If really needed, maybe we can move them to the
> class loader.


Hmm.

Upfront, I'm all for a)removing both those fields for the reasons you
mentioned and b)if we need to store the mapping, putting the mapping
somewhere external--in the class's loading classloader seems fine to
me.

Bit more on why I did things the way I did:
Earlier in hacking on VMKit I had thought it might be useful to not
just emit the already-compiled java code used while bootstrapping, but
the state of the VM itself post-bootstrap.  "symName" is an artifact
of that goal, allowing for function pointers to native code survive
across executions.  However, since we're not doing that, you're
right--no need for the symName trick/hack, since all valid paths to
invoking the method in question need to call registerNatives
appropriately.

Small caveat--we need to make sure we don't store any native wrappers
(that contain these bare function pointers) in precompiled code for
this to work.  I'm not sure if this is an issue or not presently.

As for the the other field, "overrideptr": this field was meant to be
the native function for a given method (not callable directly from
java code), while the existing 'code' field holds a java-callable
function pointer (which for 'native' methods is the wrapper emitted in
JavaJIT::nativeCompile() that calls either overrideptr, or the result
of the appropriate symbol lookup).  I didn't (don't) see a good way to
merge the two, so I added the new 'overrideptr' field you see in the
patch to record this additional information.

However, this information can almost certainly go in the class's
classloader instead, and avoid bloating the type.

All that sound good to you? I'm working towards trying this all out
presently, but let me know if you have any
thoughts/comments/objections :).

Looks like this functionality just got a lot simpler, which is great!

> Also, here are couple of comments on the patch:

>> +intptr_t JavaMethod::getOverridePtr() {
>> +  if (!symName) return 0;
>> +
>> +  if (!resolvedOverridePtr) {
>> +    dlerror();
>
> Is that first call of dlerror a way to remove previous errors?
>

Yep, it's there to clear out previous errors.  Luckily I'm going to
rip this all out, so hopefully this is a moot point.

>>
>> +    const char * sym = strdup(UTF8Buffer(symName).cString());
>
> Do you need to strdup? Why don't you just create a local variable UTF8Buffer
> name(symName), and then pass name.cString()?
>

Good call, although I _think_ this code is dead now.

>>  jint RegisterNatives(JNIEnv *env, jclass clazz, const JNINativeMethod
>> *methods,
>>       jint nMethods) {
>> -  NYI();
>> -  abort();
>> -  return 0;
>> +  BEGIN_JNI_EXCEPTION
>> +
>> +  JavaThread* th = JavaThread::get();
>> +  UserClass* currentClass = th->getCallingClassLevel(0);
>
> Shouldn't you use clazz instead of the caller, according to the spec? I
> guess the only places where you saw the RegisterNatives were in the class
> itself.
>

You're right, and I'll include this fix in the next version of this
patch.  Forest for the trees and whatnot, but luckily easy fix.
Thanks for catching this!

>>
>> +  assert(currentClass);
>> +
>> +  UserClass * cl = currentClass->asClass();
>> +  assert(cl->isResolved());
>> +
>> +  for(int i = 0; i < nMethods; ++i)
>> +  {
>> +    //fprintf(stderr, "JNINativeMethod: %s, %s, %p\n",
>> +    //    methods[i].name,
>> +    //    methods[i].signature,
>> +    //    methods[i].fnPtr);
>> +    const UTF8* name =
>> cl->classLoader->hashUTF8->lookupAsciiz(methods[i].name);
>> +    const UTF8* sign =
>> cl->classLoader->hashUTF8->lookupAsciiz(methods[i].signature);
>> +    assert(name);
>> +    assert(sign);
>> +
>> +    JavaMethod * meth = cl->lookupMethodDontThrow(name, sign, true, true,
>> 0);
>> +    if (!meth) meth = cl->lookupMethodDontThrow(name, sign, false, true,
>> 0);
>> +    assert(meth);
>> +    assert(isNative(meth->access));
>> +
>
> Instead of doing this dladdr logic (which is for AOT?), I would just set the
> field 'code' on the method. Would that be enough?
>

See my comments above, but I think a)no need for dladdr at all and
b)we can just store the pointer directly (wherever is belongs, either
in the JavaMethod (this patch) or in the class's loading classloader
(see above)).

>> Hi Will,
>> Do you think you can inline the last patch in an email? It would make the
>> review process easier, as I have some comments/questions.
>> Thanks!
>> Nicolas
>>

Again, my apologies for dropping the ball on this one.  Is the patch
format I used before okay in general (and I'll inline ones on
request)?  Let me know, don't mean to push any formatting burden on
you :).

Thanks again for your time, and I'll hopefully be sending an updated
patch incorporating the above in the near future.

~Will