<div>Hi Will,</div><div><br></div><div>Great work! I have a few meta-comments, and some comments on the code.</div><div> </div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
>From fb3971d4914e2902716e1c860c47331cca5a1ea4 Mon Sep 17 00:00:00 2001<br>From: Will Dietz <<a href="mailto:w@wdtz.org" target="_blank">w@wdtz.org</a>><br>Date: Fri, 14 Oct 2011 05:34:37 -0500<br>Subject: [PATCH 8/8] Implement some basic registerNatives support.<br>
Strategy: we're given the dynamic address of a particular java function.<br>Attempt to reverse-lookup the memory address to see if a)we implement<br>the mentioned function and b)the function has a symbol we can use to<br>
look it up.<br>If so, store the symbol with the function, and use that to resolve the<br>function in the future (works across AOT compilation).<br></blockquote><div><br></div><div><br></div><div>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.</div>
<div><br></div><div>About AOT, we are going to always execute the bootstrap of OpenJDK, so the RegisterNatives will always be called, right?</div><div><br></div><div>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.</div>
<div><br></div><div>Also, here are couple of comments on the patch:</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
---<br> lib/J3/Compiler/JavaAOTCompiler.cpp | 14 +++++++-<br> lib/J3/Compiler/JavaLLVMCompiler.cpp | 2 +-<br> lib/J3/LLVMRuntime/runtime-default.ll | 2 +-<br> lib/J3/VMCore/JavaClass.cpp | 22 +++++++++++++<br>
lib/J3/VMCore/JavaClass.h | 14 +++++++-<br> lib/J3/VMCore/Jni.cpp | 57 ++++++++++++++++++++++++++++++--<br> lib/J3/VMCore/Precompiled.cpp | 1 +<br> 7 files changed, 102 insertions(+), 10 deletions(-)<br>
diff --git a/lib/J3/Compiler/JavaAOTCompiler.cpp b/lib/J3/Compiler/JavaAOTCompiler.cpp<br>index d16d57c..980aff7 100644<br>--- a/lib/J3/Compiler/JavaAOTCompiler.cpp<br>+++ b/lib/J3/Compiler/JavaAOTCompiler.cpp<br>@@ -1058,11 +1058,21 @@ Constant* JavaAOTCompiler::CreateConstantFromJavaMethod(JavaMethod& method) {<br>
MethodElts.push_back(ConstantExpr::getCast(Instruction::BitCast, func,<br> JavaIntrinsics.ptrType));<br> }<br>- <br>+<br>+ // symName<br>+ if (method.symName)<br>+ MethodElts.push_back(getUTF8(method.symName));<br>
+ else<br>+ MethodElts.push_back(Constant::getNullValue(JavaIntrinsics.UTF8Type));<br>+<br>+ // resolvedOverridePtr<br>+ // (force us to re-look it up later)<br>+ MethodElts.push_back(Constant::getNullValue(JavaIntrinsics.ptrType));<br>
+<br> // offset<br> MethodElts.push_back(ConstantInt::get(Type::getInt32Ty(getLLVMContext()), method.offset));<br> <br>- return ConstantStruct::get(STy, MethodElts); <br>+ return ConstantStruct::get(STy, MethodElts);<br>
}<br> <br> Constant* JavaAOTCompiler::CreateConstantFromClassPrimitive(ClassPrimitive* cl) {<br>diff --git a/lib/J3/Compiler/JavaLLVMCompiler.cpp b/lib/J3/Compiler/JavaLLVMCompiler.cpp<br>index 95d93ae..4eca5c2 100644<br>
--- a/lib/J3/Compiler/JavaLLVMCompiler.cpp<br>+++ b/lib/J3/Compiler/JavaLLVMCompiler.cpp<br>@@ -62,7 +62,7 @@ Function* JavaLLVMCompiler::parseFunction(JavaMethod* meth, Class* customizeFor)<br> if (func->getLinkage() == GlobalValue::ExternalWeakLinkage) {<br>
JavaJIT jit(this, meth, func, customizeFor);<br> if (isNative(meth->access)) {<br>- jit.nativeCompile();<br>+ jit.nativeCompile(meth->getOverridePtr());<br> mvm::MvmModule::runPasses(func, JavaNativeFunctionPasses);<br>
mvm::MvmModule::runPasses(func, J3FunctionPasses);<br> } else {<br>diff --git a/lib/J3/LLVMRuntime/runtime-default.ll b/lib/J3/LLVMRuntime/runtime-default.ll<br>index 048030a..6242582 100644<br>--- a/lib/J3/LLVMRuntime/runtime-default.ll<br>
+++ b/lib/J3/LLVMRuntime/runtime-default.ll<br>@@ -56,7 +56,7 @@<br> i16 }<br> <br> %JavaMethod = type { i8*, i16, %Attribut*, i16, %JavaClass*,<br>- %UTF8*, %UTF8*, i8, i8*, i32 }<br>
+ %UTF8*, %UTF8*, i8, i8*, %UTF8*, i8*, i32 }<br> <br> %JavaClassPrimitive = type { %JavaCommonClass, i32 }<br> %JavaClassArray = type { %JavaCommonClass, %JavaCommonClass* }<br>diff --git a/lib/J3/VMCore/JavaClass.cpp b/lib/J3/VMCore/JavaClass.cpp<br>
index 7daf125..4d5c5c5 100644<br>--- a/lib/J3/VMCore/JavaClass.cpp<br>+++ b/lib/J3/VMCore/JavaClass.cpp<br>@@ -26,6 +26,7 @@<br> #include "Reader.h"<br> <br> #include <cstring><br>+#include <dlfcn.h><br>
<br> using namespace j3;<br> <br>@@ -635,6 +636,8 @@ void JavaMethod::initialise(Class* cl, const UTF8* N, const UTF8* T, uint16 A) {<br> access = A;<br> isCustomizable = false;<br> offset = 0;<br>+ resolvedOverridePtr = 0;<br>
+ symName = 0;<br> }<br> <br> void JavaField::initialise(Class* cl, const UTF8* N, const UTF8* T, uint16 A) {<br>@@ -1774,3 +1777,22 @@ void JavaField::setStaticObjectField(JavaObject* val) {<br> JavaObject** ptr = (JavaObject**)((uint64)classDef->getStaticInstance() + ptrOffset);<br>
mvm::Collector::objectReferenceNonHeapWriteBarrier((gc**)ptr, (gc*)val);<br> }<br>+<br>+intptr_t JavaMethod::getOverridePtr() {<br>+ if (!symName) return 0;<br>+<br>+ if (!resolvedOverridePtr) {<br>+ dlerror();<br>
</blockquote><div><br></div><div>Is that first call of dlerror a way to remove previous errors?</div><div> </div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
+ const char * sym = strdup(UTF8Buffer(symName).cString());<br></blockquote><div><br></div><div>Do you need to strdup? Why don't you just create a local variable UTF8Buffer name(symName), and then pass name.cString()?</div>
<div> </div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
+ resolvedOverridePtr = dlsym(SELF_HANDLE, sym);<br>+ char * err = dlerror();<br>+ if (err) {<br>+ fprintf(stderr, "Failed to resolve method %s to symbol %s! Error: %s\n",<br>
+ UTF8Buffer(name).cString(), sym, err);<br>+ abort();<br>+ }<br>+ free((void *)sym);<br>+ }<br>+<br>+ return (intptr_t)resolvedOverridePtr;<br>+}<br>diff --git a/lib/J3/VMCore/JavaClass.h b/lib/J3/VMCore/JavaClass.h<br>
index 1fa9b77..e1dcac3 100644<br>--- a/lib/J3/VMCore/JavaClass.h<br>+++ b/lib/J3/VMCore/JavaClass.h<br>@@ -887,7 +887,14 @@ public:<br> /// code - Pointer to the compiled code of this method.<br> ///<br> void* code;<br>
- <br>+<br>+ /// symName -- Name of the symbol (in self) that overrides the usual definition.<br>+ ///<br>+ const UTF8* symName;<br>+<br>+ /// resolvedOverridePtr -- Cached version of the lookup done on symName<br>+ void * resolvedOverridePtr;<br>
+<br> /// offset - The index of the method in the virtual table.<br> ///<br> uint32 offset;<br>@@ -961,7 +968,10 @@ public:<br> /// with the given class loader.<br> ///<br> JavaObject* getReturnType(JnjvmClassLoader* loader);<br>
- <br>+<br>+ /// getOverridePtr - Returns the registered native override for this method,<br>+ /// resolving it dynamically if needed. Returns 0 if there is no override.<br>+ intptr_t getOverridePtr();<br> <br> //===----------------------------------------------------------------------===//<br>
//<br>diff --git a/lib/J3/VMCore/Jni.cpp b/lib/J3/VMCore/Jni.cpp<br>index e64b5c3..3995308 100644<br>--- a/lib/J3/VMCore/Jni.cpp<br>+++ b/lib/J3/VMCore/Jni.cpp<br>@@ -22,11 +22,13 @@<br> #include "Jnjvm.h"<br>
#include "Reader.h"<br>
<br>+#include <dlfcn.h><br>+<br> using namespace j3;<br> <br> #define NYI() do { \<br> fprintf(stderr, "%s: Implement me!\n", __FUNCTION__); \<br>-} while(0);<br>+} while(0)<br> <br> Jnjvm* myVM(JNIEnv* env) {<br>
return JavaThread::get()->getJVM();<br>@@ -3757,9 +3759,56 @@ void SetDoubleArrayRegion(JNIEnv *env, jdoubleArray array, jsize start,<br> <br> jint RegisterNatives(JNIEnv *env, jclass clazz, const JNINativeMethod *methods,<br>
<span style="white-space:pre-wrap"> </span> jint nMethods) {<br>- NYI();<br>- abort();<br>- return 0;<br>+ BEGIN_JNI_EXCEPTION<br>+<br>+ JavaThread* th = JavaThread::get();<br>+ UserClass* currentClass = th->getCallingClassLevel(0);<br>
</blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
+ assert(currentClass);<br>+<br>+ UserClass * cl = currentClass->asClass();<br>+ assert(cl->isResolved());<br>+<br>+ for(int i = 0; i < nMethods; ++i)<br>+ {<br>+ //fprintf(stderr, "JNINativeMethod: %s, %s, %p\n",<br>
+ // methods[i].name,<br>+ // methods[i].signature,<br>+ // methods[i].fnPtr);<br>+ const UTF8* name = cl->classLoader->hashUTF8->lookupAsciiz(methods[i].name);<br>+ const UTF8* sign = cl->classLoader->hashUTF8->lookupAsciiz(methods[i].signature);<br>
+ assert(name);<br>+ assert(sign);<br>+<br>+ JavaMethod * meth = cl->lookupMethodDontThrow(name, sign, true, true, 0);<br>+ if (!meth) meth = cl->lookupMethodDontThrow(name, sign, false, true, 0);<br>+ assert(meth);<br>
+ assert(isNative(meth->access));<br>+<br></blockquote><div><br></div><div>Instead of doing this dladdr logic (which is for AOT?), I would just set the field 'code' on the method. Would that be enough?</div>
<div> </div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">+ Dl_info info;<br>
+ int res = dladdr(methods[i].fnPtr, &info);<br>+ if (!res) {<br>+ perror("Failed to lookup fptr for registerNatives");<br>+ abort();<br>
+ }<br>+ if(info.dli_saddr != methods[i].fnPtr) {<br>+ fprintf(stderr, "Unsupported registerNative request!\n");<br>+ abort();<br>+ }<br>+ // TODO: Verify that this symbol is from *our* libjvm.<br>
+ // (we need to be able to use it later!)<br>+<br>+ // Phew, we were told to override a function with a definition<br>+ // that comes from within our program!<br>+ meth->symName = cl->classLoader->hashUTF8->lookupOrCreateAsciiz(info.dli_sname);<br>
+ // TODO: Just put the fnptr here, looking it up elsewhere is silly<br>+ meth->resolvedOverridePtr = NULL;<br>+ }<br>+<br>+ RETURN_FROM_JNI(0)<br>+<br>+ END_JNI_EXCEPTION<br>+<br>+ RETURN_FROM_JNI(0)<br> }<br>
<br> <br>diff --git a/lib/J3/VMCore/Precompiled.cpp b/lib/J3/VMCore/Precompiled.cpp<br>index e469eaf..dcb4baf 100644<br>--- a/lib/J3/VMCore/Precompiled.cpp<br>+++ b/lib/J3/VMCore/Precompiled.cpp<br>@@ -102,6 +102,7 @@ extern "C" word_t vmjcNativeLoader(JavaMethod* meth) {<br>
char* buf = (char*)threadAllocator.Allocate(<br> 3 + JNI_NAME_PRE_LEN + 1 + ((mnlen + clen + mtlen) << 3));<br> word_t res = meth->classDef->classLoader->nativeLookup(meth, j3, buf);<br>+ if (!res) res = meth->getOverridePtr();<br>
assert(res && "Could not find required native method");<br> return res;<br> }<br>-- <br>1.7.5.1</blockquote><div><br></div><br><div class="gmail_quote">On Wed, Oct 19, 2011 at 11:19 AM, Nicolas Geoffray <span dir="ltr"><<a href="mailto:nicolas.geoffray@gmail.com" target="_blank">nicolas.geoffray@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Will,<div><br></div><div>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.</div>
<div><br></div><div>Thanks!</div><div>Nicolas<font color="#888888"><br><br></font><div class="gmail_quote"><div><div></div><div>
On Tue, Oct 18, 2011 at 12:41 AM, Will Dietz <span dir="ltr"><<a href="mailto:willdtz@gmail.com" target="_blank">willdtz@gmail.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div></div><div>
Hi,<br>
<br>
Attached are various patches that I've refactored out from the OpenJDK<br>
port, as they aren't necessarily part of switching runtimes and help<br>
the Classpath implementation too.<br>
<br>
Hopefully OpenJDK patches will be coming in the semi-near future...<br>
<br>
Please let me know if you have any questions or comments, I'm rather<br>
happy to discuss :).<br>
<br>
Thanks, and have a good one!<br>
<font color="#888888"><br>
~Will<br>
</font><br></div></div><div>_______________________________________________<br>
vmkit-commits mailing list<br>
<a href="mailto:vmkit-commits@cs.uiuc.edu" target="_blank">vmkit-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits</a><br>
<br></div></blockquote></div><br></div>
</blockquote></div><br>