`Unsafe.putOrderedObject` throws `ArrayIndexOutOfBoundsException` for correct offsets

[Kojoley]

import java.util.concurrent.atomic.AtomicReferenceArray;

public final class Main
{
    public static void main(String[] args) throws Exception
    {
        AtomicReferenceArray<Object> ao = new AtomicReferenceArray<Object>(20);
        for (int idx = 0; idx < ao.length(); ++idx) {
            System.out.println(idx);
            ao.lazySet(idx, null);
        }
    }
}

0
1
2
3
4
5
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException
    at java.util.concurrent.atomic.AtomicReferenceArray.lazySet(AtomicReferenceArray.java:152)
    at TestUnsafePutObject.main(TestUnsafePutObject.java:37)
    at java.lang.reflect.Method.invoke(Method.java:486)
    at IKVM.Java.Externs.ikvm.runtime.Launcher.run(Unknown Source)

[Kojoley]

https://github.com/ikvmnet/ikvm/blob/b26343bf7001ea226b132ea4f9f15a0f67d33439/src/IKVM.Runtime/Java/Externs/sun/misc/Unsafe.cs#L1411-L1420 I think an offset here needs to be converted: substract base (which is 0) and divide by element size (which is always 4 because it is always a pointer?).

[wasabii]

Maybe. Looking through it.

At some point Windward replaced the arrayIndexScale result for object[] with 4. Though the original IKVM code left it as 1, since it's not like we can do unaligned writes of object pointers anyways. I'm not exactly sure why they decided to do this, and they even notate it in their latest release: https://github.com/windward-studios/ikvm8/releases/tag/8.5.0.3

Discussed here: https://github.com/windward-studios/ikvm8/pull/5

I think the 'proper' result is to return IntPtr.Size. That is the true size of elements in object arrays on .NET. Whether 32 or 64 bit. But I'm not sure why they decided to just set it to 4. Frankly, I'm not sure why they decided to change it away from 1: unaligned writes wouldn't work anyways.

What's even more obvious and worse, is they test for type equality (ac == typeof(object[])). But not all arrays are object[]. Some are Foo[]. I'm thinking this change by Windward was very poorly considered.

What I want to know was what the original bug that '1' didn't work on was.

[wasabii]

Found it:

https://github.com/windward-studios/ikvm8/issues/4

So, an issue in Netty, which actually expects the result to be 4 or 8. Windward change it to 4. But didn't consider all the other effects, such as with non-object reference type arrays.

It also appears they didn't update the corresponding intrinsic or flow analyzer code.

So this might be a bit of work to get right.

[Kojoley]

What I want to know was what the original bug that '1' didn't work on was.

They linked https://github.com/windward-studios/ikvm8/issues/4 where shown that Netty uses more or less hardcoded values for arrayIndexScale.

[Kojoley]

So this might be a bit of work to get right.

Yeah... there seems to be bunch of bugs:

public final class Main
{
    public static sun.misc.Unsafe getUnsafe() throws Exception {
      try {
        return sun.misc.Unsafe.getUnsafe();
      }
      catch (SecurityException tryReflectionInstead) {
        Class<sun.misc.Unsafe> k = sun.misc.Unsafe.class;
        for (java.lang.reflect.Field f : k.getDeclaredFields()) {
          f.setAccessible(true);
          Object x = f.get(null);
          if (k.isInstance(x)) {
            return k.cast(x);
          }
        }
        throw new NoSuchFieldError("the Unsafe");
      }
    }

    public static void main(String[] args) throws Exception
    {
        sun.misc.Unsafe unsafe = getUnsafe();
        int base_offset = unsafe.arrayBaseOffset(Integer[].class);
        System.out.println("base_offset=" + base_offset);
        int elem_size = unsafe.arrayIndexScale(Integer[].class);
        System.out.println("elem_size=" + elem_size);
        int[] arr = new int[99];
        for (int idx = 0; idx < arr.length; ++idx) {
            System.out.println(idx);
            unsafe.putOrderedObject(arr, base_offset + idx * elem_size, idx);
            System.out.println(unsafe.getObject(arr, base_offset + idx * elem_size));
        }
    }
}

base_offset=0
elem_size=1
0
Exception in thread "main" java.lang.InternalError: cli.System.InvalidOperationException: Handle is not initialized.
    at IKVM.Java.Externs.sun.misc.Unsafe.PutFieldVolatile(Unknown Source)
    at sun.misc.Unsafe.putOrderedObject(Native Method)
    at TestUnsafePutObject.main(TestUnsafePutObject.java:31)
    at java.lang.reflect.Method.invoke(Method.java:486)
    at IKVM.Java.Externs.ikvm.runtime.Launcher.run(Unknown Source)
Caused by: cli.System.InvalidOperationException: Handle is not initialized.
    at cli.System.Runtime.InteropServices.GCHandle.FromIntPtr(Unknown Source)
    at IKVM.Internal.MemberWrapper.FromCookieImpl(Unknown Source)
    at IKVM.Java.Externs.sun.misc.Unsafe.PutFieldVolatile(Unknown Source)
    at IKVM.Internal.ExceptionHelper.toString(Unknown Source)
    at java.lang.Throwable.toString(map.xml:1358)
    at java.lang.Object.toString(map.xml)
    at ikvm.extensions.ExtensionMethods.toString(ExtensionMethods.java:244)
    at java.lang.Throwable.<init>(map.xml)
    at java.lang.Error.<init>(Error.java:106)
    at java.lang.VirtualMachineError.<init>(VirtualMachineError.java:88)
    at java.lang.InternalError.<init>(InternalError.java:88)
    ... 5 more

[wasabii]

That one is already fixed in hotfix 8.5.1, which isn't released yet.

[Kojoley]

It also seems to be strange that Unsafe things do bounds checking, ain't the purpose of them to skip bounds checking?

[wasabii]

Well, we aren't exactly doing bounds checking, as much as .NET is. Many Unsafe.java operations get implemented by manually generated IL that does stloca and such, which does bounds checking on it's own.

Some are implemented without bounds checking. For instance, getInt, as an example, works without bounds checking. Writing a reference type is a different matter though. It's not exactly well supported to get the raw address to an object and write it into the middle of a field or array.

[Kojoley]

https://github.com/ikvmnet/ikvm/blob/b26343bf7001ea226b132ea4f9f15a0f67d33439/src/IKVM.Runtime/intrinsics.cs#L561-L582 Stelem_Ref documentation says it does bounds checking, and IIUC it is also the reason why arrayIndexScale was returning 1.

There is also a null check presented, which I also doesn't think Unsafe needs.

[wasabii]

It is likely more efficient to pin both the array and the object, obtain the address of both, do math, and set the value directly, yes. But that isn't what it does today. It just pretends objects are at opaque offsets in an array. Hence why '1' was the correct value, given how it works presently. Which is why Windward's change wasn't thought out.

I'm working through changing it to IntPtr.Size, and doing the math in the various places.