[vmkit-commits] [PATCH 3/3] Define all methods in sun.misc.Unsafe, fixes to existing.

[Will Dietz]

On Wed, Nov 16, 2011 at 2:24 PM, Nicolas Geoffray
<nicolas.geoffray at gmail.com> wrote:
>>  JNIEXPORT bool JNICALL Java_sun_misc_Unsafe_compareAndSwapLong(
>> -JavaObject* unsafe, JavaObject* obj, jlong offset, jlong expect,
>> jlong update) {
>> +JavaObject* unsafe, JavaObject* base, jlong offset, jlong expect,
>> jlong update) {
>>
>>   llvm_gcroot(unsafe, 0);
>> -  llvm_gcroot(obj, 0);
>> +  llvm_gcroot(base, 0);
>>   jlong *ptr;
>>   jlong  value;
>>
>> -  ptr = (jlong *) (((uint8 *) obj) + offset);
>> +  // TODO: Why isn't this atomic?
>
> Why would it be on a 32 bit system?
>

I'm not sure I understand what you're saying.  Are you saying that
when you're on a 32bit system it shouldn't be atomic?  I apologize,
I'm probably missing something.

>> Java_sun_misc_Unsafe_copyMemory__Ljava_lang_Object_2JLjava_lang_Object_2JJ(
>> +JavaObject* unsafe,
>> +JavaObject* srcBase, jlong srcOffset,
>> +JavaObject* dstBase, jlong dstOffset,
>> +jlong size) {
>> +  BEGIN_NATIVE_EXCEPTION(0)
>> +  uint8_t* src = fieldPtr(srcBase, srcOffset, false /* Don't throw on
>> null base*/ );
>> +  uint8_t* dst = fieldPtr(dstBase, dstOffset, false /* Don't throw on
>> null base*/ );
>> +  memcpy(dst, src, size);
>
> How does that deal with GC barriers?
>

It doesn't, but I don't think it's supposed to either.  It's part of
'Unsafe' after all--and the documentation seems clear when an
operation is supposed to deal with write barriers, but doesn't mention
that for copyMemory.

I just checked the hotspot implementation, and it's more or less the
same, with a comment indicating such:

----------------------------------------------------------------------------
  if (dstp != NULL && !dstp->is_typeArray()) {
    // NYI:  This works only for non-oop arrays at present.
    // Generalizing it would be reasonable, but requires card marking.
    // Also, autoboxing a Long from 0L in copyMemory(x,y, 0L,z, n) would be bad.
    THROW(vmSymbols::java_lang_IllegalArgumentException());
  }
----------------------------------------------------------------------------

Which AFAICT doesn't even check the array type, just that the
destination /is/ an array, but I'm not sure.  Anyway I can add a
similar check if you'd like, but honestly I'd rather just keep things
simple and let programmers who misuse Unsafe have code that's unsafe
:).

Anyway, hopefully that makes sense and is okay with you, let me know please :).

~Will