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

Shafi Ahmad shafi.s.ahmad at oracle.com
Mon Nov 21 06:29:42 UTC 2016 

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/

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