[8u] RFR for JDK-8134918 - C2: Type speculation produces mismatched unsafe accesses

Wed Nov 16 16:51:09 UTC 2016

Looks good.

I would suggest to run all jtreg tests (or even RBT) when you apply all changes before pushing this.

Thanks,
Vladimir

On 11/16/16 4:52 AM, Shafi Ahmad wrote:
> Hi Vladimir,
>
> Thank you for the review and feedback.
>
> Please find updated webrevs:
> http://cr.openjdk.java.net/~shshahma/8140309/webrev.01/ => Removed the test case as it use only jdk9 APIs.
> http://cr.openjdk.java.net/~shshahma/8162101/webrev.02/ => Removed test methods testFixedOffsetHeaderArray17() and testFixedOffsetHeader17() which referenced jdk9 API UNSAFE.getIntUnaligned.
>
>
> Regards,
> Shafi
>
>
>> -----Original Message-----
>> From: Vladimir Kozlov
>> Sent: Wednesday, November 16, 2016 1:00 AM
>> To: Shafi Ahmad; hotspot-dev at openjdk.java.net
>> Subject: Re: [8u] RFR for JDK-8134918 - C2: Type speculation produces
>> mismatched unsafe accesses
>>
>> Hi Shafi
>>
>> You should not backport tests which use only new JDK 9 APIs. Like
>> TestUnsafeUnalignedMismatchedAccesses.java test.
>>
>> But it is perfectly fine to modify backport by removing part of changes which
>> use a new API. For example,  8162101 changes in OpaqueAccesses.java test
>> which use getIntUnaligned() method.
>>
>> It is unfortunate that 8140309 changes include also code which process new
>> Unsafe Unaligned intrinsics from JDK 9. It should not be backported but it will
>> simplify this and following backports. So I agree with changes you did for
>> 8140309 backport.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/14/16 10:34 PM, Shafi Ahmad wrote:
>>> Hi Vladimir,
>>>
>>> Thanks for the review.
>>>
>>>> -----Original Message-----
>>>
>>>> From: Vladimir Kozlov
>>>
>>>> Sent: Monday, November 14, 2016 11:20 PM
>>>
>>>> To: Shafi Ahmad; hotspot-dev at openjdk.java.net
>>>
>>>> Subject: Re: [8u] RFR for JDK-8134918 - C2: Type speculation produces
>>>
>>>> mismatched unsafe accesses
>>>
>>>>
>>>
>>>> On 11/14/16 1:03 AM, Shafi Ahmad wrote:
>>>
>>>>> Hi Vladimir,
>>>
>>>>>
>>>
>>>>> Thanks for the review.
>>>
>>>>>
>>>
>>>>> Please find updated webrevs.
>>>
>>>>>
>>>
>>>>> All webrevs are with respect to the base changes on JDK-8140309.
>>>
>>>>> http://cr.openjdk.java.net/~shshahma/8140309/webrev.00/
>>>
>>>>
>>>
>>>> Why you kept unaligned parameter in changes?
>>>
>>> The fix of JDK-8136473 caused many problems after integration (see JDK-
>> 8140267).
>>>
>>> The fix was backed out and re-implemented with JDK-8140309 by slightly
>> changing the assert:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2015-
>> Novem
>>> ber/019696.html
>>>
>>> The code change for the fix of JDK-8140309 is code changes for JDK-8136473
>> by slightly changing one assert.
>>>
>>> jdk9 original changeset is
>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/4bee38ba018c
>>>
>>> As this is a backport so I keep the changes as it is.
>>>
>>>>
>>>
>>>> The test TestUnsafeUnalignedMismatchedAccesses.java will not work
>>>> since
>>>
>>>> since Unsafe class in jdk8 does not have unaligned methods.
>>>
>>>> Hot did you run it?
>>>
>>> I am sorry, looks there is some issue with my testing.
>>>
>>> I have run jtreg test after merging the changes but somehow the test does
>> not run and I verified only the failing list of jtreg result.
>>>
>>> When I run the test case separately it is failing as you already pointed out
>> the same.
>>>
>>> $java -jar ~/Tools/jtreg/lib/jtreg.jar
>>> -jdk:build/linux-x86_64-normal-server-slowdebug/jdk/
>>>
>> hotspot/test/compiler/intrinsics/unsafe/TestUnsafeUnalignedMismatchedA
>>> ccesses.java
>>>
>>> Test results: failed: 1
>>>
>>> Report written to
>>> /scratch/shshahma/Java/jdk8u-dev-
>> 8140309_01/JTreport/html/report.html
>>>
>>> Results written to /scratch/shshahma/Java/jdk8u-dev-8140309_01/JTwork
>>>
>>> Error:
>>>
>>> /scratch/shshahma/Java/jdk8u-dev-
>> 8140309_01/hotspot/test/compiler/intr
>>> insics/unsafe/TestUnsafeUnalignedMismatchedAccesses.java:92: error:
>>> cannot find symbol
>>>
>>>          UNSAFE.putIntUnaligned(array,
>>> UNSAFE.ARRAY_BYTE_BASE_OFFSET+1, -1);
>>>
>>> Not sure if we should push without the test case.
>>>
>>>>
>>>
>>>>> http://cr.openjdk.java.net/~shshahma/8134918/webrev.01/
>>>
>>>>
>>>
>>>> Good. Did you run new UnsafeAccess.java test?
>>>
>>> Due to same process issue the test case is not run and when I run it
>> separately it fails.
>>>
>>> It passes after doing below changes:
>>>
>>> 1. Added /othervm
>>>
>>> 2. replaced import statement 'import jdk.internal.misc.Unsafe;'  by 'import
>> sun.misc.Unsafe;'
>>>
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~shshahma/8134918/webrev.02/
>>>
>>>>
>>>
>>>>> http://cr.openjdk.java.net/~shshahma/8162101/webrev.01/
>>>
>>> I am getting the similar compilation error as above for added test case.  Not
>> sure if we can push without the test case.
>>>
>>> Regards,
>>>
>>> Shafi
>>>
>>>>
>>>
>>>> Good.
>>>
>>>>
>>>
>>>> Thanks,
>>>
>>>> Vladimir
>>>
>>>>
>>>
>>>>>
>>>
>>>>> Regards,
>>>
>>>>> Shafi
>>>
>>>>>
>>>
>>>>>
>>>
>>>>>
>>>
>>>>>> -----Original Message-----
>>>
>>>>>> From: Vladimir Kozlov
>>>
>>>>>> Sent: Friday, November 11, 2016 1:26 AM
>>>
>>>>>> To: Shafi Ahmad;hotspot-dev at openjdk.java.net
>>>>>> <mailto:hotspot-dev at openjdk.java.net>
>>>
>>>>>> Subject: Re: [8u] RFR for JDK-8134918 - C2: Type speculation
>>>>>> produces
>>>
>>>>>> mismatched unsafe accesses
>>>
>>>>>>
>>>
>>>>>> On 11/9/16 10:42 PM, Shafi Ahmad wrote:
>>>
>>>>>>> Hi,
>>>
>>>>>>>
>>>
>>>>>>> Please review the backport of following dependent backports.
>>>
>>>>>>>
>>>
>>>>>>> jdk9 bug link:https://bugs.openjdk.java.net/browse/JDK-8136473
>>>
>>>>>>> Conflict in file src/share/vm/opto/memnode.cpp due to 1.
>>>
>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/fe311de64c61
>>>>>>> [JDK-
>>>
>>>>>> 8080289]. Manual merge is not done as the corresponding code is
>>>>>> not
>>>
>>>>>> there in jdk8u-dev.
>>>
>>>>>>> Multiple conflicts in file src/share/vm/opto/library_call.cpp and
>>>
>>>>>>> manual
>>>
>>>>>> merge is done.
>>>
>>>>>>> webrev link:
>>>
>>>> http://cr.openjdk.java.net/~shshahma/8136473/webrev.00/
>>>
>>>>>>
>>>
>>>>>> unaligned unsafe access methods were added in jdk 9 only. In your
>>>
>>>>>> changes unaligned argument is always false. You can simplify changes.
>>>
>>>>>>
>>>
>>>>>> Also you should base changes on JDK-8140309 (original 8136473
>>>>>> changes
>>>
>>>>>> were backout by 8140267):
>>>
>>>>>>
>>>
>>>>>> On 11/4/15 10:21 PM, Roland Westrelin wrote:
>>>
>>>>>>  >http://cr.openjdk.java.net/~roland/8140309/webrev.00/
>>>
>>>>>>  >
>>>
>>>>>>  > Same as 8136473 with only the following change:
>>>
>>>>>>  >
>>>
>>>>>>  > diff --git a/src/share/vm/opto/library_call.cpp
>>>
>>>>>> b/src/share/vm/opto/library_call.cpp
>>>
>>>>>>  > --- a/src/share/vm/opto/library_call.cpp
>>>
>>>>>>  > +++ b/src/share/vm/opto/library_call.cpp
>>>
>>>>>>  > @@ -2527,7 +2527,7 @@
>>>
>>>>>>  >     // of safe & unsafe memory.
>>>
>>>>>>  >     if (need_mem_bar) insert_mem_bar(Op_MemBarCPUOrder);
>>>
>>>>>>  >
>>>
>>>>>>  > -  assert(is_native_ptr || alias_type->adr_type() ==
>>>
>>>>>> TypeOopPtr::BOTTOM
>>>
>>>>>> ||  > +  assert(alias_type->adr_type() == TypeRawPtr::BOTTOM ||
>>>
>>>>>> alias_type->adr_type() == TypeOopPtr::BOTTOM ||
>>>
>>>>>>  >            alias_type->field() != NULL || alias_type->element() !=
>>>
>>>>>> NULL, "field, array element or unknown");
>>>
>>>>>>  >     bool mismatched = false;
>>>
>>>>>>  >     if (alias_type->element() != NULL || alias_type->field() != NULL) {
>>>
>>>>>>  >
>>>
>>>>>>  > alias_type->adr_type() == TypeRawPtr::BOTTOM covers the
>>>
>>>>>> is_native_ptr case and the case where the unsafe method is called
>>>>>> with a
>>>
>>>> null object.
>>>
>>>>>>
>>>
>>>>>>> jdk9 changeset:
>>>
>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/9108fab781a4
>>>
>>>>>>>
>>>
>>>>>>> jdk9 bug link:https://bugs.openjdk.java.net/browse/JDK-8134918
>>>
>>>>>>> Conflict in file src/share/vm/opto/library_call.cpp due to 1.
>>>
>>>>>>>
>>>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/4bee38ba018c#l5.165
>>>
>>>>>> [JDK-8140309]. Manual merge is not done as the corresponding code
>>>>>> is
>>>
>>>>>> not there in jdk8u-dev.
>>>
>>>>>>
>>>
>>>>>> I explained situation with this line above.
>>>
>>>>>>
>>>
>>>>>>> webrev link:
>>>
>>>> http://cr.openjdk.java.net/~shshahma/8134918/webrev.00/
>>>
>>>>>>
>>>
>>>>>> This webrev is not incremental for your 8136473 changes -
>>>
>>>>>> library_call.cpp has part from 8136473 changes.
>>>
>>>>>>
>>>
>>>>>>> jdk9 changeset:
>>>
>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/79dae2cd00ef
>>>
>>>>>>>
>>>
>>>>>>> jdk9 bug link:https://bugs.openjdk.java.net/browse/JDK-8155781
>>>
>>>>>>> Clean merge
>>>
>>>>>>> webrev link:
>>>
>>>> http://cr.openjdk.java.net/~shshahma/8155781/webrev.00/
>>>
>>>>>>
>>>
>>>>>> Thanks seems fine.
>>>
>>>>>>
>>>
>>>>>>> jdk9 changeset:
>>>
>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/cde17b3e2e70
>>>
>>>>>>>
>>>
>>>>>>> jdk9 bug link:https://bugs.openjdk.java.net/browse/JDK-8162101
>>>
>>>>>>> Conflict in file src/share/vm/opto/library_call.cpp due to 1.
>>>
>>>>
>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/4be0cada20ad#l1.7
>>>
>>>>>>> [JDK-8160360] - Resolved 2.
>>>
>>>>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/8b9fdaeb8c57#l10.2
>>>>>> 73
>>>
>>>>>> [JDK-8148146] - Manual merge is not done as the corresponding code
>>>>>> is
>>>
>>>>>> not there in jdk8u-dev.
>>>
>>>>>>> webrev link:
>>>
>>>> http://cr.openjdk.java.net/~shshahma/8162101/webrev.00/
>>>
>>>>>>
>>>
>>>>>> This webrev is not incremental in library_call.cpp. Difficult to
>>>>>> see
>>>
>>>>>> this part of changes.
>>>
>>>>>>
>>>
>>>>>> Thanks,
>>>
>>>>>> Vladimir
>>>
>>>>>>
>>>
>>>>>>> jdk9 changeset:
>>>
>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/10dad1d40843
>>>
>>>>>>>
>>>
>>>>>>> Testing: jprt and jtreg
>>>
>>>>>>>
>>>
>>>>>>> Regards,
>>>
>>>>>>> Shafi
>>>
>>>>>>>
>>>
>>>>>>>> -----Original Message-----
>>>
>>>>>>>> From: Shafi Ahmad
>>>
>>>>>>>> Sent: Thursday, October 20, 2016 10:08 AM
>>>
>>>>>>>> To: Vladimir Kozlov;hotspot-dev at openjdk.java.net
>>>>>>>> <mailto:hotspot-dev at openjdk.java.net>
>>>
>>>>>>>> Subject: RE: [8u] RFR for JDK-8134918 - C2: Type speculation
>>>
>>>>>>>> produces mismatched unsafe accesses
>>>
>>>>>>>>
>>>
>>>>>>>> Thanks Vladimir.
>>>
>>>>>>>>
>>>
>>>>>>>> I will create dependent  backport of 1.
>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8136473
>>>
>>>>>>>> 2.https://bugs.openjdk.java.net/browse/JDK-8155781
>>>
>>>>>>>> 3.https://bugs.openjdk.java.net/browse/JDK-8162101
>>>
>>>>>>>>
>>>
>>>>>>>> Regards,
>>>
>>>>>>>> Shafi
>>>
>>>>>>>>
>>>
>>>>>>>>> -----Original Message-----
>>>
>>>>>>>>> From: Vladimir Kozlov
>>>
>>>>>>>>> Sent: Wednesday, October 19, 2016 8:27 AM
>>>
>>>>>>>>> To: Shafi Ahmad;hotspot-dev at openjdk.java.net
>>>>>>>>> <mailto:hotspot-dev at openjdk.java.net>
>>>
>>>>>>>>> Subject: Re: [8u] RFR for JDK-8134918 - C2: Type speculation
>>>
>>>>>>>>> produces mismatched unsafe accesses
>>>
>>>>>>>>>
>>>
>>>>>>>>> Hi Shafi,
>>>
>>>>>>>>>
>>>
>>>>>>>>> You should also consider backporting following related fixes:
>>>
>>>>>>>>>
>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8155781
>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8162101
>>>
>>>>>>>>>
>>>
>>>>>>>>> Otherwise you may hit asserts added by 8134918 changes.
>>>
>>>>>>>>>
>>>
>>>>>>>>> Thanks,
>>>
>>>>>>>>> Vladimir
>>>
>>>>>>>>>
>>>
>>>>>>>>> On 10/17/16 3:12 AM, Shafi Ahmad wrote:
>>>
>>>>>>>>>> Hi All,
>>>
>>>>>>>>>>
>>>
>>>>>>>>>> Please review the backport of JDK-8134918 - C2: Type
>>>>>>>>>> speculation
>>>
>>>>>>>>>> produces
>>>
>>>>>>>>> mismatched unsafe accesses to jdk8u-dev.
>>>
>>>>>>>>>>
>>>
>>>>>>>>>> Please note that backport is not clean and the conflict is due to:
>>>
>>>>>>>>>>
>>>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/9108fab781a4#l5.
>>>
>>>>>>>>>> 1
>>>
>>>>>>>>>> 65
>>>
>>>>>>>>>>
>>>
>>>>>>>>>>  Getting debug build failure because of:
>>>
>>>>>>>>>>
>>>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/9108fab781a4#l5.
>>>
>>>>>>>>>> 1
>>>
>>>>>>>>>> 55
>>>
>>>>>>>>>>
>>>
>>>>>>>>>> The above changes are done under bug# 'JDK-8136473: failed: no
>>>
>>>>>>>>> mismatched stores, except on raw memory: StoreB StoreI' which
>>>>>>>>> is
>>>
>>>>>>>>> not back ported to jdk8u and the current backport is on top of
>>>
>>>>>>>>> above
>>>
>>>>>> change.
>>>
>>>>>>>>>>
>>>
>>>>>>>>>>  Please note that I am not sure if there is any dependency
>>>
>>>>>>>>>> between these
>>>
>>>>>>>>> two changesets.
>>>
>>>>>>>>>>
>>>
>>>>>>>>>> open webrev:
>>>
>>>>>>>> http://cr.openjdk.java.net/~shshahma/8134918/webrev.00/
>>>
>>>>>>>>>> jdk9 bug link:https://bugs.openjdk.java.net/browse/JDK-8134918
>>>
>>>>>>>>>> jdk9 changeset:
>>>
>>>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/79dae2cd00ef
>>>
>>>>>>>>>>
>>>
>>>>>>>>>> testing: Passes JPRT, jtreg not completed
>>>
>>>>>>>>>>
>>>
>>>>>>>>>> Regards,
>>>
>>>>>>>>>> Shafi
>>>
>>>>>>>>>>
>>>