[vmkit-commits] [PATCH] Add support for extra VM-only fields, use this to add fields to j.l.Class in OpenJDK

[Will Dietz]

On Mon, Oct 31, 2011 at 4:34 PM, Nicolas Geoffray
<nicolas.geoffray at gmail.com> wrote:
> Thanks Will for the simplified patch! It almost looks good, with the
> exception of the forgotten write barrier. Feel free to commit once that is
> fixed.
>
> On Mon, Oct 31, 2011 at 9:44 PM, Will Dietz <wdietz2 at illinois.edu> wrote:
>>
>> On Fri, Oct 28, 2011 at 5:31 PM, Nicolas Geoffray
>> <nicolas.geoffray at gmail.com> wrote:
>> > Hi Will!
>> > I think this patch could work, but I would definitely try a more
>> > 'localized'
>> > approach before doing it this way.
>> > I think that in the bootstrap process of OpenJDK, you should update the
>> > Class::virtualSize field for java.lang.Class to include these two new
>> > fields, and that would be it. No need to extend the JavaField array (the
>> > Java side of things does not have to know we added new fields).
>> > Of course, you need to make sure no java.lang.Class objects are
>> > allocated
>> > *before* you change the size.
>> > Thanks, and let me know if that worked!
>> > Nicolas
>>
>> I was playing nice with the JavaFields not so that anything from Java
>> could see the fields (I actually intentionally tried to hide them from
>> Java code), but so that the created LLVM types (LLVMInfo) for
>> java.lang.Class matched up.  Not knowing better, that seemed like
>> something to ensure was correct.
>>
>> However, what you suggested is much simpler and seems to work equally
>> well, thanks for the good suggestion.
>>
>> Updated patch inlined below :)
>>
>> ~Will
>>
>> From f57fed146de2962a5e3c1306515ab9374862647e Mon Sep 17 00:00:00 2001
>> From: Will Dietz <w at wdtz.org>
>> Date: Mon, 31 Oct 2011 15:23:24 -0500
>> Subject: [PATCH] Add extra fields to JavaObjectClass to hold CommonClass
>> and
>>  protection domain.
>>
>> ---
>>  lib/J3/ClassLib/OpenJDK/ClasspathReflect.h |   13 +++++++------
>>  lib/J3/ClassLib/OpenJDK/JavaUpcalls.cpp    |    2 ++
>>  2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/J3/ClassLib/OpenJDK/ClasspathReflect.h
>> b/lib/J3/ClassLib/OpenJDK/ClasspathReflect.h
>> index 75d35a7..4075a5f 100644
>> --- a/lib/J3/ClassLib/OpenJDK/ClasspathReflect.h
>> +++ b/lib/J3/ClassLib/OpenJDK/ClasspathReflect.h
>> @@ -38,30 +38,31 @@ private:
>>   JavaObject* annotations;
>>   JavaObject* declaredAnnotations;
>>   JavaObject* annotationType;
>> +  // Extra fields added by VM
>> +  UserCommonClass * internalClass;
>> +  JavaObject * pd;
>>
>>  public:
>>
>>   static UserCommonClass* getClass(JavaObjectClass* cl) {
>>     llvm_gcroot(cl, 0);
>> -    UNIMPLEMENTED();
>> -    return NULL;
>> +    return cl->internalClass;
>>   }
>>
>>   static void setClass(JavaObjectClass* cl, UserCommonClass* vmdata) {
>>     llvm_gcroot(cl, 0);
>> -    UNIMPLEMENTED();
>> +    cl->internalClass = vmdata;
>>   }
>>
>>   static void setProtectionDomain(JavaObjectClass* cl, JavaObject* pd) {
>>     llvm_gcroot(cl, 0);
>>     llvm_gcroot(pd, 0);
>> -    UNIMPLEMENTED();
>> +    cl->pd = pd;
>
> This should be a write barrier.

Okay, will do.

>
>>
>>   }
>>
>>   static JavaObject* getProtectionDomain(JavaObjectClass* cl) {
>>     llvm_gcroot(cl, 0);
>> -    UNIMPLEMENTED();
>> -    return NULL;
>> +    return cl->pd;
>>   }
>>
>>   static void staticTracer(JavaObjectClass* obj, word_t closure) {
>> diff --git a/lib/J3/ClassLib/OpenJDK/JavaUpcalls.cpp
>> b/lib/J3/ClassLib/OpenJDK/JavaUpcalls.cpp
>> index a4a48c8..476b6e3 100644
>> --- a/lib/J3/ClassLib/OpenJDK/JavaUpcalls.cpp
>> +++ b/lib/J3/ClassLib/OpenJDK/JavaUpcalls.cpp
>> @@ -468,6 +468,8 @@ void
>> Classpath::initialiseClasspath(JnjvmClassLoader* loader) {
>>
>>   newClass =
>>     UPCALL_CLASS(loader, "java/lang/Class");
>> +  // Add space for extra fields, see ClasspathReflect.h
>> +  newClass->virtualSize += 2*sizeof(void*);
>
> Did you actually make sure no java.lang.Class  objects were created before
> executing that line? I'm not sure how you could ensure that with an assert.
> But you could just trace allocations (printf in Class::doNew) manually and
> see if it holds today.

Yeah I couldn't assert it, and already tested what you said (printf
tracing in doNew() and abort() at place in code where I fiddle with
virtualSize) and things seem good.

Will commit in a bit, and thanks!

~Will