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

[Nicolas Geoffray]

On Wed, Nov 16, 2011 at 11:50 PM, Will Dietz <wdietz2 at illinois.edu> wrote:

> 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.
>

Yes, a long store on a 32bits system is not atomic.


>
> >>
> 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
> :).
>

I think I'd feel more secure with adding these checks. Making sure it's a
primitive array is easy in vmkit.

Cheers,
Nicolas


>
> Anyway, hopefully that makes sense and is okay with you, let me know
> please :).
>
> ~Will
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/vmkit-commits/attachments/20111117/ce27ad60/attachment.html>