[8u] RFR for JDK-8134918 - C2: Type speculation produces mismatched unsafe accesses
Tobias Hartmann
tobias.hartmann at oracle.com
Wed Nov 23 11:42:00 UTC 2016
Hi Shafi,
On 21.11.2016 07:29, Shafi Ahmad wrote:
> Hi All,
>
> May I get the second review on this.
>
> I am putting together all the webrevs to make it simple for reviewer.
> http://cr.openjdk.java.net/~shshahma/8140309/webrev.01/
> http://cr.openjdk.java.net/~shshahma/8162101/webrev.02/
> http://cr.openjdk.java.net/~shshahma/8134918/webrev.02/
> http://cr.openjdk.java.net/~shshahma/8155781/webrev.00/
This looks good to me (not a 8u reviewer).
Best regards,
Tobias
>
> Please note that I tested with jprt, all jtreg and rbt tests.
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Vladimir Kozlov
>> Sent: Wednesday, November 16, 2016 10:21 PM
>> To: Shafi Ahmad; hotspot-dev at openjdk.java.net
>> Subject: Re: [8u] RFR for JDK-8134918 - C2: Type speculation produces
>> mismatched unsafe accesses
>>
>> 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/TestUnsafeUnalignedMismatched
>>>> A
>>>>> 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.16
>>>>>> 5
>>>>>
>>>>>>>> [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
>>>>>
>>>>>>>>>>>>
>>>>>
More information about the hotspot-dev
mailing list