[PATCH] 8202414: Unsafe crash in C2

[Andy Law]

Hi Lutz,

Nice summary and I totally agree with your points.

Thanks,
Andy

> On Sep 11, 2018, at 18:43, Schmidt, Lutz <lutz.schmidt at sap.com> wrote:
> 
> 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
>