[PATCH] 8202414: Unsafe crash in C2

[Vladimir Kozlov]

Dean have additional comments in bug report regarding unaligned store in general (need to set C2_UNALIGNED).
This make me nervous because stores collected by Initialized node are converted to raw stores [1], could be combined and 
may change such properties.

I think we need make sure to not collect such unaligned stores by Initialize node. And in fact we do have such check [2] 
but in this case store is not marked as unaligned. Because, as Dean pointed before, intrinsic for putInt() do not mark 
store as unaligned [3].

We can argue that putIntUnaligned() should be used but we can't guarantee that user will use correct one or it is even 
available as this bug shows. That is why we need to check if store/load is unaligned regardless of which Unsafe method 
is used. At least for cases when offset is constant.

I think we need to fix LibraryCallKit::inline_unsafe_access() and also InitializeNode::can_capture_store() because 
during parse phase offset could be not constant.

I am retracting my suggested fix and let someone to work on this.

Thanks,
Vladimir

[1] http://hg.openjdk.java.net/jdk/jdk/file/74dde8b66b7f/src/hotspot/share/opto/memnode.cpp#l3721
[2] http://hg.openjdk.java.net/jdk/jdk/file/74dde8b66b7f/src/hotspot/share/opto/memnode.cpp#l3478
[3] http://hg.openjdk.java.net/jdk/jdk/file/74dde8b66b7f/src/hotspot/share/opto/library_call.cpp#l2315

On 9/11/18 4:16 AM, Andy Law wrote:
> 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
>>
> 
> 
>