On Wed, Nov 16, 2011 at 11:50 PM, Will Dietz <span dir="ltr"><<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On Wed, Nov 16, 2011 at 2:24 PM, Nicolas Geoffray<br>
<<a href="mailto:nicolas.geoffray@gmail.com">nicolas.geoffray@gmail.com</a>> wrote:<br>
>>  JNIEXPORT bool JNICALL Java_sun_misc_Unsafe_compareAndSwapLong(<br>
>> -JavaObject* unsafe, JavaObject* obj, jlong offset, jlong expect,<br>
>> jlong update) {<br>
>> +JavaObject* unsafe, JavaObject* base, jlong offset, jlong expect,<br>
>> jlong update) {<br>
>><br>
>>   llvm_gcroot(unsafe, 0);<br>
>> -  llvm_gcroot(obj, 0);<br>
>> +  llvm_gcroot(base, 0);<br>
>>   jlong *ptr;<br>
>>   jlong  value;<br>
>><br>
>> -  ptr = (jlong *) (((uint8 *) obj) + offset);<br>
>> +  // TODO: Why isn't this atomic?<br>
><br>
> Why would it be on a 32 bit system?<br>
><br>
<br>
</div>I'm not sure I understand what you're saying.  Are you saying that<br>
when you're on a 32bit system it shouldn't be atomic?  I apologize,<br>
I'm probably missing something.<br></blockquote><div><br></div><div>Yes, a long store on a 32bits system is not atomic.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<div class="im"><br>
>> Java_sun_misc_Unsafe_copyMemory__Ljava_lang_Object_2JLjava_lang_Object_2JJ(<br>
>> +JavaObject* unsafe,<br>
>> +JavaObject* srcBase, jlong srcOffset,<br>
>> +JavaObject* dstBase, jlong dstOffset,<br>
>> +jlong size) {<br>
>> +  BEGIN_NATIVE_EXCEPTION(0)<br>
>> +  uint8_t* src = fieldPtr(srcBase, srcOffset, false /* Don't throw on<br>
>> null base*/ );<br>
>> +  uint8_t* dst = fieldPtr(dstBase, dstOffset, false /* Don't throw on<br>
>> null base*/ );<br>
>> +  memcpy(dst, src, size);<br>
><br>
> How does that deal with GC barriers?<br>
><br>
<br>
</div>It doesn't, but I don't think it's supposed to either.  It's part of<br>
'Unsafe' after all--and the documentation seems clear when an<br>
operation is supposed to deal with write barriers, but doesn't mention<br>
that for copyMemory.<br>
<br>
I just checked the hotspot implementation, and it's more or less the<br>
same, with a comment indicating such:<br>
<br>
----------------------------------------------------------------------------<br>
  if (dstp != NULL && !dstp->is_typeArray()) {<br>
    // NYI:  This works only for non-oop arrays at present.<br>
    // Generalizing it would be reasonable, but requires card marking.<br>
    // Also, autoboxing a Long from 0L in copyMemory(x,y, 0L,z, n) would be bad.<br>
    THROW(vmSymbols::java_lang_IllegalArgumentException());<br>
  }<br>
----------------------------------------------------------------------------<br>
<br>
Which AFAICT doesn't even check the array type, just that the<br>
destination /is/ an array, but I'm not sure.  Anyway I can add a<br>
similar check if you'd like, but honestly I'd rather just keep things<br>
simple and let programmers who misuse Unsafe have code that's unsafe<br>
:).<br></blockquote><div><br></div><div>I think I'd feel more secure with adding these checks. Making sure it's a primitive array is easy in vmkit.</div><div><br></div><div>Cheers,</div><div>Nicolas</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
Anyway, hopefully that makes sense and is okay with you, let me know please :).<br>
<span class="HOEnZb"><font color="#888888"><br>
~Will<br>
</font></span></blockquote></div><br>