[9] RFR (S): 8161720: Better byte behavior for off-heap data

[Volker Simonis]

Hi Zoltan,

are you sure your tests are doing what they are supposed to do?

If for example I run UnsafeOnHeapBooleanTest with '-Xcomp
-XX:-TieredCompilation -XX:CompileCommand="option
UnsafeOnHeapBooleanTest::main,PrintOptoAssembly"' I can see that
UnsafeOnHeapBooleanTest::main will be compiled but it only contains an
uncommon trap:

000   B1: #    N1 <- BLOCK HEAD IS JUNK   Freq: 1
000       # stack bang (216 bytes)
    pushq   rbp    # Save rbp
    subq    rsp, #16    # Create frame

00c       movl    RSI, #41    # int
          nop     # 2 bytes pad for loops and calls
013       call,static  wrapper for: uncommon_trap(reason='unloaded'
action='reinterpret' index='41' debug_id='0')
        # UnsafeOnHeapBooleanTest::main @ bci:0  L[0]=_ L[1]=_ L[2]=_
L[3]=_ L[4]=_ L[5]=_ L[6]=_ L[7]=_ L[8]=_
        # OopMap{off=24}
018       int3    # ShouldNotReachHere
018

So the test will be actually executed in the interpreter and not in
the C2-compiled code as expected.

I in generally prefer to put tests in a function and call them in a
loop (with -XX:-UseOnStackReplacement) which I think is a much more
stable way for exercising the different compilation levels.

And I have a question about JNI: is it only a convention that JNI
functions should return JNI_TRUE/JNI_FALSE or is this required by the
specification? Unfortunately, bug "JDK-8149170: Better byte behavior
for native arguments" which is mentioned in this bug description is
not visible.

Regards,
Volker

On Mon, Aug 22, 2016 at 4:25 PM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
> Hi,
>
>
> please review the fix for JDK-8161720.
>
> https://bugs.openjdk.java.net/browse/JDK-8161720
>
>
> Problem: As John pointed out in the bug description, when a boolean value is
> loaded using Unsafe.getBoolean(), the VM should normalize the byte value
> from the range 0..255 to 0..1.
>
> The patch I'm proposing includes a new test, UnsafeOffHeapBooleanTest.java
> [1].  The test does the following (in essence):
>
> boolean bool1 = Unsafe.getBoolean(<off-heap memory region 1 containing a
> non-null value, e.g., 0x2>);
> boolean bool2 = Unsafe.getBoolean(<off-heap memory region 2 containing a
> non-null value, e.g., 0x4>);
> boolean result = bool1 & bool2; // Is supposed to be true
>
> The value of 'result' is supposed to be true.  However, because values
> returned by Unsafe.getBoolean() are currently not normalized by the VM,
> 'value' can be either true or false.  The value of 'result' depends on (1)
> whether the code above is interpreted or compiled and (2) also on the
> platform where the test is executed.
>
> The same problem can be triggered with on-heap accesses as well. Here is a
> second test, UnsafeOnHeapBooleanTest.java [2] that does that.  I'd like to,
> therefore, propose that the VM normalizes not only off-heap data read by
> Unsafe.getBoolean() (as originally proposed by John), but also on-heap data
> returned by Unsafe.getBoolean().
>
> Maybe the UnsafeOnHeapBooleanTest.java test mis-uses the Unsafe API in the
> way it reads data from the heap.  But as any user of the Unsafe API could do
> what the test does, maybe it would be good to "protect" the VM from the
> problem of having to deal with many different 'true' values (e.g., 0x2 and
> 0x4).  Also, Unsafe.getBoolean() is declared as a native, so maybe it's not
> too bad if the method respects JNI conventions.
>
>
> Solution: Normalize the result returned by Unsafe.getBoolean in
> - src/share/vm/prims/unsafe.cpp (used by the interpreter and by compiled
> code if the compiler intrinsics for Unsafe.getBoolean() are disabled)
> - the C1 and C2 intrinsics for Unsafe.getBoolean().
>
>
> Webrev:
> http://cr.openjdk.java.net/~zmajo/8161720/webrev.00/
>
> Testing:
> - JPRT (incl. Unsafe[On|Off]HeapBooleanTest.java);
> - RBT testing with all hotspot tests both w/ -Xmixed and -Xcomp (no new
> problems have showed up).
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
> [1]
> http://cr.openjdk.java.net/~zmajo/8161720/webrev.00/test/compiler/unsafe/UnsafeOffHeapBooleanTest.java.html
> [2]
> http://cr.openjdk.java.net/~zmajo/8161720/webrev.00/test/compiler/unsafe/UnsafeOnHeapBooleanTest.java.html
>