[PATCH] 8202414: Unsafe crash in C2

[Schmidt, Lutz]

Hi Andy, 

unfortunately, we have two mail thread heads on the same topic. So I will try to give a very brief summary: 

  -  I agree, it's not your fault. The "user" is InitializeNode::complete_stores().
  -  clear_memory() is not prepared to handle unaligned (less than BytesPerInt) offsets.
  -  Your patch just leaves the memory uninitialized in case of unaligned offsets.
  -  Vladimir's patch fixes the root cause, i.e. the caller of clear_memory().
  -  Your patch removes the safety net from clear_memory(). Another reason why I don't like it.

In essence, I suggest to go with Vladimir's patch, provided the tests Vladimir requested work out ok: 

---BEGIN Vladimir's patch ---
diff -r b9f6a4427da9 src/hotspot/share/opto/memnode.cpp
--- a/src/hotspot/share/opto/memnode.cpp
+++ b/src/hotspot/share/opto/memnode.cpp
@@ -4095,10 +4095,11 @@
        // See if this store needs a zero before it or under it.
        intptr_t zeroes_needed = st_off;

-      if (st_size < BytesPerInt) {
+      if (st_size < BytesPerInt || (zeroes_needed % BytesPerInt) != 0) {
          // Look for subword stores which only partially initialize words.
          // If we find some, we must lay down some word-level zeroes first,
          // underneath the subword stores.
+        // Do the same for unaligned stores.
          //
          // Examples:
          //   byte[] a = { p,q,r,s }  =>  a[0]=p,a[1]=q,a[2]=r,a[3]=s
---END Vladimir's patch ---

BTW, I requested to be precise, so I have to correct myself. The length field of ArrayOop is at offset 12 (@klass_gap_offset_in_bytes()) only if UseCompressedClassPointers is true. It does not depend on UseCompressedOops.

Thanks,
Lutz

From: Andy Law <944797358 at qq.com>
Date: Tuesday, 11. September 2018 at 02:36
To: "hotspot-compiler-dev at openjdk.java.net" <hotspot-compiler-dev at openjdk.java.net>, Lutz Schmidt <lutz.schmidt at sap.com>, "aph at redhat.com" <aph at redhat.com>
Subject: Re: [PATCH] 8202414: Unsafe crash in C2

Hi Lutz and Andrew, 

Thank you for your reply and sorry for my typos :)

TL;DR
I think it is the optimization of `clear_memory()`which cause this problem, in my understanding it may not be a user fault :)

When running the example on the bug list using `-XX:DisableIntrinsic=_putInt`, or use interpreter/C1 only will make this problem go away, due to the fact that program will go to another branch.

In function `clear_memory()`, it will make an optimization which will clear the context of the memory, in fact

    if (done_offset > start_offset) {  // [1]
        // it will clear the memory from start_offset to done_offset
    }

    if (done_offset < end_offset) {  // [2]
        // it will clear the memory by using a Int (0) to clear the memory of done_offset
    }

|<--------------- 16-byte header —--——>| <—— 4-byte arr length (new byte[397]) ———>
                                                              |       0000     0001     1000     1101     |
If it is aligned, it won’t have any problem but, if it is not aligned as the example, this optimization will mis-clear the `0000 0001` to `0000 0000`, so the array length becomes 141. Then it will crash when gc happens.

It is the optimization which cause this problem, so when it is not aligned, we don’t do this optimization for this unaligned address may solve the problem.
By the way I didn’t find the unaligned message on the doc:( but I think you’re right and it should be aligned when using Unsafe, though it is an deprecated API :) It won’t be reproduced using the templateInterpreter or C1 compiler, due to the fact that they won’t do this optimization.

Thank you:),
Andy