[PATCH] 8202414: Unsafe crash in C2

[Schmidt, Lutz]

Hi Andy,

to avoid misunderstandings, please be precise when talking about bits and bytes. ArrayObjects (with -XX:+UseCompressedOops) have a 16-byte header, whereof the last four bytes (byte# 12..15) designate the array length (in #elements). 

And now, just checking if I got your intention right:
I read your text below as well as description and comments in https://bugs.openjdk.java.net/browse/JDK-8202414. In essence, you are trying to perform a ?-byte store into a byte array by means of a unaligned putInt() call.

To my understanding, putInt() is not designed for unaligned accesses. Even "worse", it relies on the store address to be at least 4-byte aligned. That's what I learn e.g. from http://www.docjar.com/docs/api/sun/misc/Unsafe.html. And that's the reason why your code (sometimes) destroys the length field of the ArrayObject header.

Your fix would just ignore (copy nothing) calls with unaligned end_offset. Why would you then call the unsafe function at all?

Yes, your patch would probably help in your situation. It just puts a blanket of silence over a call with unsupported parameters. 

That's how far I can comment. I am neither a reviewer nor in a position to decide if such interface violation should be handled gracefully (e.g. by throwing an exception) or if the status quo is ok. 

Thank you
Lutz



On 10.09.18, 13:10, "hotspot-compiler-dev on behalf of Andy Law" <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of 944797358 at qq.com> wrote:

    This change is only about:
    Disabling the un-aligned C2 `clean_memory()` optimization when using Unsafe to write to an unaligned address.
    
    ```
    java -version
    openjdk version "1.8.0-internal-debug"
    OpenJDK Runtime Environment (build 1.8.0-internal-debug-***_2018_09_03_19_31-b00)
    OpenJDK 64-Bit Server VM (build 25.71-b00-debug, mixed mode)
    ```
    
    This issue 8202414 is about:
    ArrayObjects of -XX:+UseCompressedOops on 64-bit has a 12 bits header and a 4 bits length. So the length address is from 12th to 16th bytes.
    If we use Unsafe.putInt() to write at the 17th bit, the C2 `clean_memory()` will mistakenly do `done_offset -= BytesPerInt;`, then `done_offset` will become 13.  And then it will clear the address from the 13th bit, make the array length changes to a different value. When a GC happens, it will crash.
    
    I didn’t find the unaligned memory support of `clear_memory()`, so I only do a small fix to make the affect be the least:
    When Unsafe.put*() writes to an aligned memory as above, it will cause the assert fail. So when it fails, we don’t do any optimizations instead, and the problem solves.
    
    I don’t know if it is a good solution? It is only 3 lines of code, so please have a look:) Thank you!
    
    
    Andy