[vmkit-commits] [PATCH] Various patches
Nicolas Geoffray
nicolas.geoffray at gmail.com
Sat Oct 22 11:43:32 PDT 2011
Hi Will,
Great work! I have a few meta-comments, and some comments on the code.
> 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.
Also, here are couple of comments on the patch:
> ---
> lib/J3/Compiler/JavaAOTCompiler.cpp | 14 +++++++-
> lib/J3/Compiler/JavaLLVMCompiler.cpp | 2 +-
> lib/J3/LLVMRuntime/runtime-default.ll | 2 +-
> lib/J3/VMCore/JavaClass.cpp | 22 +++++++++++++
> lib/J3/VMCore/JavaClass.h | 14 +++++++-
> lib/J3/VMCore/Jni.cpp | 57
> ++++++++++++++++++++++++++++++--
> lib/J3/VMCore/Precompiled.cpp | 1 +
> 7 files changed, 102 insertions(+), 10 deletions(-)
> diff --git a/lib/J3/Compiler/JavaAOTCompiler.cpp
> b/lib/J3/Compiler/JavaAOTCompiler.cpp
> index d16d57c..980aff7 100644
> --- a/lib/J3/Compiler/JavaAOTCompiler.cpp
> +++ b/lib/J3/Compiler/JavaAOTCompiler.cpp
> @@ -1058,11 +1058,21 @@ Constant*
> JavaAOTCompiler::CreateConstantFromJavaMethod(JavaMethod& method) {
> MethodElts.push_back(ConstantExpr::getCast(Instruction::BitCast, func,
> JavaIntrinsics.ptrType));
> }
> -
> +
> + // symName
> + if (method.symName)
> + MethodElts.push_back(getUTF8(method.symName));
> + else
> + MethodElts.push_back(Constant::getNullValue(JavaIntrinsics.UTF8Type));
> +
> + // resolvedOverridePtr
> + // (force us to re-look it up later)
> + MethodElts.push_back(Constant::getNullValue(JavaIntrinsics.ptrType));
> +
> // offset
>
> MethodElts.push_back(ConstantInt::get(Type::getInt32Ty(getLLVMContext()),
> method.offset));
>
> - return ConstantStruct::get(STy, MethodElts);
> + return ConstantStruct::get(STy, MethodElts);
> }
>
> Constant*
> JavaAOTCompiler::CreateConstantFromClassPrimitive(ClassPrimitive* cl) {
> diff --git a/lib/J3/Compiler/JavaLLVMCompiler.cpp
> b/lib/J3/Compiler/JavaLLVMCompiler.cpp
> index 95d93ae..4eca5c2 100644
> --- a/lib/J3/Compiler/JavaLLVMCompiler.cpp
> +++ b/lib/J3/Compiler/JavaLLVMCompiler.cpp
> @@ -62,7 +62,7 @@ Function* JavaLLVMCompiler::parseFunction(JavaMethod*
> meth, Class* customizeFor)
> if (func->getLinkage() == GlobalValue::ExternalWeakLinkage) {
> JavaJIT jit(this, meth, func, customizeFor);
> if (isNative(meth->access)) {
> - jit.nativeCompile();
> + jit.nativeCompile(meth->getOverridePtr());
> mvm::MvmModule::runPasses(func, JavaNativeFunctionPasses);
> mvm::MvmModule::runPasses(func, J3FunctionPasses);
> } else {
> diff --git a/lib/J3/LLVMRuntime/runtime-default.ll
> b/lib/J3/LLVMRuntime/runtime-default.ll
> index 048030a..6242582 100644
> --- a/lib/J3/LLVMRuntime/runtime-default.ll
> +++ b/lib/J3/LLVMRuntime/runtime-default.ll
> @@ -56,7 +56,7 @@
> i16 }
>
> %JavaMethod = type { i8*, i16, %Attribut*, i16, %JavaClass*,
> - %UTF8*, %UTF8*, i8, i8*, i32 }
> + %UTF8*, %UTF8*, i8, i8*, %UTF8*, i8*, i32 }
>
> %JavaClassPrimitive = type { %JavaCommonClass, i32 }
> %JavaClassArray = type { %JavaCommonClass, %JavaCommonClass* }
> diff --git a/lib/J3/VMCore/JavaClass.cpp b/lib/J3/VMCore/JavaClass.cpp
> index 7daf125..4d5c5c5 100644
> --- a/lib/J3/VMCore/JavaClass.cpp
> +++ b/lib/J3/VMCore/JavaClass.cpp
> @@ -26,6 +26,7 @@
> #include "Reader.h"
>
> #include <cstring>
> +#include <dlfcn.h>
>
> using namespace j3;
>
> @@ -635,6 +636,8 @@ void JavaMethod::initialise(Class* cl, const UTF8* N,
> const UTF8* T, uint16 A) {
> access = A;
> isCustomizable = false;
> offset = 0;
> + resolvedOverridePtr = 0;
> + symName = 0;
> }
>
> void JavaField::initialise(Class* cl, const UTF8* N, const UTF8* T, uint16
> A) {
> @@ -1774,3 +1777,22 @@ void JavaField::setStaticObjectField(JavaObject*
> val) {
> JavaObject** ptr = (JavaObject**)((uint64)classDef->getStaticInstance()
> + ptrOffset);
> mvm::Collector::objectReferenceNonHeapWriteBarrier((gc**)ptr, (gc*)val);
> }
> +
> +intptr_t JavaMethod::getOverridePtr() {
> + if (!symName) return 0;
> +
> + if (!resolvedOverridePtr) {
> + dlerror();
>
Is that first call of dlerror a way to remove previous errors?
> + 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()?
> + resolvedOverridePtr = dlsym(SELF_HANDLE, sym);
> + char * err = dlerror();
> + if (err) {
> + fprintf(stderr, "Failed to resolve method %s to symbol %s! Error:
> %s\n",
> + UTF8Buffer(name).cString(), sym, err);
> + abort();
> + }
> + free((void *)sym);
> + }
> +
> + return (intptr_t)resolvedOverridePtr;
> +}
> diff --git a/lib/J3/VMCore/JavaClass.h b/lib/J3/VMCore/JavaClass.h
> index 1fa9b77..e1dcac3 100644
> --- a/lib/J3/VMCore/JavaClass.h
> +++ b/lib/J3/VMCore/JavaClass.h
> @@ -887,7 +887,14 @@ public:
> /// code - Pointer to the compiled code of this method.
> ///
> void* code;
> -
> +
> + /// symName -- Name of the symbol (in self) that overrides the usual
> definition.
> + ///
> + const UTF8* symName;
> +
> + /// resolvedOverridePtr -- Cached version of the lookup done on symName
> + void * resolvedOverridePtr;
> +
> /// offset - The index of the method in the virtual table.
> ///
> uint32 offset;
> @@ -961,7 +968,10 @@ public:
> /// with the given class loader.
> ///
> JavaObject* getReturnType(JnjvmClassLoader* loader);
> -
> +
> + /// getOverridePtr - Returns the registered native override for this
> method,
> + /// resolving it dynamically if needed. Returns 0 if there is no
> override.
> + intptr_t getOverridePtr();
>
>
> //===----------------------------------------------------------------------===//
> //
> diff --git a/lib/J3/VMCore/Jni.cpp b/lib/J3/VMCore/Jni.cpp
> index e64b5c3..3995308 100644
> --- a/lib/J3/VMCore/Jni.cpp
> +++ b/lib/J3/VMCore/Jni.cpp
> @@ -22,11 +22,13 @@
> #include "Jnjvm.h"
> #include "Reader.h"
>
> +#include <dlfcn.h>
> +
> using namespace j3;
>
> #define NYI() do { \
> fprintf(stderr, "%s: Implement me!\n", __FUNCTION__); \
> -} while(0);
> +} while(0)
>
> Jnjvm* myVM(JNIEnv* env) {
> return JavaThread::get()->getJVM();
> @@ -3757,9 +3759,56 @@ void SetDoubleArrayRegion(JNIEnv *env, jdoubleArray
> array, jsize start,
>
> 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.
> + 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?
> + Dl_info info;
> + int res = dladdr(methods[i].fnPtr, &info);
> + if (!res) {
> + perror("Failed to lookup fptr for registerNatives");
> + abort();
> + }
> + if(info.dli_saddr != methods[i].fnPtr) {
> + fprintf(stderr, "Unsupported registerNative request!\n");
> + abort();
> + }
> + // TODO: Verify that this symbol is from *our* libjvm.
> + // (we need to be able to use it later!)
> +
> + // Phew, we were told to override a function with a definition
> + // that comes from within our program!
> + meth->symName =
> cl->classLoader->hashUTF8->lookupOrCreateAsciiz(info.dli_sname);
> + // TODO: Just put the fnptr here, looking it up elsewhere is silly
> + meth->resolvedOverridePtr = NULL;
> + }
> +
> + RETURN_FROM_JNI(0)
> +
> + END_JNI_EXCEPTION
> +
> + RETURN_FROM_JNI(0)
> }
>
>
> diff --git a/lib/J3/VMCore/Precompiled.cpp b/lib/J3/VMCore/Precompiled.cpp
> index e469eaf..dcb4baf 100644
> --- a/lib/J3/VMCore/Precompiled.cpp
> +++ b/lib/J3/VMCore/Precompiled.cpp
> @@ -102,6 +102,7 @@ extern "C" word_t vmjcNativeLoader(JavaMethod* meth) {
> char* buf = (char*)threadAllocator.Allocate(
> 3 + JNI_NAME_PRE_LEN + 1 + ((mnlen + clen + mtlen) << 3));
> word_t res = meth->classDef->classLoader->nativeLookup(meth, j3, buf);
> + if (!res) res = meth->getOverridePtr();
> assert(res && "Could not find required native method");
> return res;
> }
> --
> 1.7.5.1
On Wed, Oct 19, 2011 at 11:19 AM, Nicolas Geoffray <
nicolas.geoffray at gmail.com> wrote:
> 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
>
> On Tue, Oct 18, 2011 at 12:41 AM, Will Dietz <willdtz at gmail.com> wrote:
>
>> Hi,
>>
>> Attached are various patches that I've refactored out from the OpenJDK
>> port, as they aren't necessarily part of switching runtimes and help
>> the Classpath implementation too.
>>
>> Hopefully OpenJDK patches will be coming in the semi-near future...
>>
>> Please let me know if you have any questions or comments, I'm rather
>> happy to discuss :).
>>
>> Thanks, and have a good one!
>>
>> ~Will
>>
>> _______________________________________________
>> vmkit-commits mailing list
>> vmkit-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/vmkit-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/vmkit-commits/attachments/20111022/f69c7acd/attachment.html>
More information about the vmkit-commits
mailing list