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

Wed Nov 16 12:52:24 UTC 2016

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
> >
> >> >>>>>>
> >