[aarch64-port-dev ] RFR: aarch64: Fix vm crash with CDS on

[Ningsheng Jian]

On 21 June 2016 at 16:38, Andrew Haley <aph at redhat.com> wrote:
> On 21/06/16 07:51, Ningsheng Jian wrote:
>> Hi,
>>
>> I found that the following hotspot jtreg test failures in aarch64
>> openjdk9 are caused by the same issue:
>>
>> runtime/CDSCompressedKPtrs/CDSCompressedKPtrs.java
>> runtime/CDSCompressedKPtrs/XShareAuto.java
>> runtime/CompressedOops/CompressedClassPointers.java
>> runtime/NMT/NMTWithCDS.java
>> runtime/SharedArchiveFile/CdsSameObjectAlignment.java
>> runtime/SharedArchiveFile/SASymbolTableTest.java
>> runtime/SharedArchiveFile/SharedArchiveFile.java
>> runtime/SharedArchiveFile/SharedBaseAddress.java
>> runtime/SharedArchiveFile/SharedStrings.java
>> runtime/SharedArchiveFile/SharedStringsRunAuto.java
>>
>>
>> It is caused by incorrectly patching un-patchable bytecodes in CDS
>> image. Refer to [1] for details.
>>
>> The crash can be reproduced by a simple hello-world case:
>>
>> $ java -XX:+UnlockDiagnosticVMOptions -XX:SharedArchiveFile=1.jsa
>> -Xshare:dump Hello
>> $ java -XX:+UnlockDiagnosticVMOptions -XX:SharedArchiveFile=1.jsa
>> -Xshare:on Hello
>>
>>
>> The following patch can fix that crash and the jtreg tests above (and
>> no new failure found):
>>
>> http://people.linaro.org/~ningsheng.jian/cds-fix/webrev.01/
>>
>>
>> Could someone please help to review and process this patch?
>
> I am concerned that your patch may be incomplete.  The x86 and sparc
> versions of this code use
>
> @@ -2661,7 +2684,7 @@
>
>    __ load_signed_byte(rax, field);
>    __ push(btos);
>    // Rewrite bytecode to be faster
> -  if (!is_static) {
> +  if (!is_static && rc == may_rewrite) {
>      patch_bytecode(Bytecodes::_fast_bgetfield, bc, rbx);
>    }
>    __ jmp(Done);
>
> But you use
>
> @@ -2434,7 +2434,7 @@
>    __ ldrsb(r0, field);
>    __ push(ztos);
>    // Rewrite bytecode to be faster
> -  if (!is_static) {
> +  if (rc == may_rewrite) {
>      // use btos rewriting, no truncating to t/f bit is needed for getfield.
>      patch_bytecode(Bytecodes::_fast_bgetfield, bc, r1);
>    }
>
> The patch is at
>
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/0b0b09a60061
>
> Please make sure that AArch64 is not missing anything else.
>
> Andrew.
>
>

Hi Andrew,

Thank for reviewing.

In AArch64 implementation, we already set rc to may_not_rewrite before
those checking. I think it should be complete.

+
+  // Don't rewrite getstatic, only getfield
+  if (is_static) rc = may_not_rewrite;
+

The original patch at:

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/d818fe2baeb6

Thanks,
Ningsheng