[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface
From:       Christian Thalinger <christian.thalinger () oracle ! com>
Date:       2015-09-29 1:12:07
Message-ID: 2C323894-C058-442D-92CF-BDF969CD6504 () oracle ! com
[Download RAW message or body]


> On Sep 27, 2015, at 11:25 PM, Magnus Ihse Bursie <magnus.ihse.bursie@oracle.com> \
> wrote: 
> On 2015-09-25 22:00, Christian Thalinger wrote:
> > 
> > > On Sep 25, 2015, at 12:20 AM, Magnus Ihse Bursie <magnus.ihse.bursie@oracle.com \
> > > <mailto:magnus.ihse.bursie@oracle.com>> wrote: 
> > > On 2015-09-24 20:57, Christian Thalinger wrote:
> > > > > On Sep 21, 2015, at 12:24 PM, Christian Thalinger \
> > > > > <christian.thalinger@oracle.com <mailto:christian.thalinger@oracle.com>> \
> > > > > wrote: 
> > > > > 
> > > > > > On Sep 18, 2015, at 2:19 PM, Christian Thalinger \
> > > > > > <christian.thalinger@oracle.com <mailto:christian.thalinger@oracle.com>> \
> > > > > > wrote: 
> > > > > > 
> > > > > > > On Sep 18, 2015, at 7:00 AM, Christian Thalinger \
> > > > > > > <christian.thalinger@oracle.com \
> > > > > > > <mailto:christian.thalinger@oracle.com>> wrote: 
> > > > > > > 
> > > > > > > > On Sep 17, 2015, at 11:00 PM, Volker Simonis \
> > > > > > > > <volker.simonis@gmail.com <mailto:volker.simonis@gmail.com>> wrote: 
> > > > > > > > To which repository will you finally push these changes?
> > > > > > > > 
> > > > > > > > The current webrevs are against jdk9/jdk9/hotspot but I doubt that
> > > > > > > > they will be integrated there.
> > > > > > > Good point.
> > > > > > > 
> > > > > > > > Unfortunately the patches don't apply to jdk9/dev/hotspot anymore
> > > > > > > > because of '8135067: Preparatory refactorings for compiler control'
> > > > > > > > and  "8134626: Misc cleanups after generation array removal":
> > > > > > > > 
> > > > > > > > 3 out of 18 hunks FAILED -- saving rejects to file
> > > > > > > > src/share/vm/compiler/compileBroker.cpp.rej
> > > > > > > > 1 out of 5 hunks FAILED -- saving rejects to file
> > > > > > > > src/share/vm/compiler/compileBroker.hpp.rej
> > > > > > > > 6 out of 40 hunks FAILED -- saving rejects to file
> > > > > > > > src/share/vm/runtime/vmStructs.cpp.rej
> > > > > > > > 
> > > > > > > > Will you update the patches and if yes against which repositories?
> > > > > > > Yes.  Let me update the graal-jvmci-9 repository to hs-comp.
> > > > > > Done.  Now I have to create new webrevs…
> > > > > Here are new webrevs against hs-comp:
> > > > > 
> > > > > http://cr.openjdk.java.net/~twisti/8136421/webrev/ \
> > > > > <http://cr.openjdk.java.net/%7Etwisti/8136421/webrev/> \
> > > > > <http://cr.openjdk.java.net/~twisti/8136421/webrev/ \
> > > > > <http://cr.openjdk.java.net/%7Etwisti/8136421/webrev/>> \
> > > > > http://cr.openjdk.java.net/~twisti/8136421/hotspot/webrev/ \
> > > > > <http://cr.openjdk.java.net/%7Etwisti/8136421/hotspot/webrev/> \
> > > > > <http://cr.openjdk.java.net/~twisti/8136421/hotspot/webrev/ \
> > > > > <http://cr.openjdk.java.net/%7Etwisti/8136421/hotspot/webrev/>>
> > > > I have refreshed these webrevs.  They contain all the changes we discussed \
> > > > here plus a bunch of new tests that SQE finished.
> > > 
> > > Hi Christian,
> > > 
> > > I didn't see Volkers proposed fix about undefined-bool-conversion:
> > > 
> > > > If this comes only from adlc, you could fix for adlc only by adding
> > > > 
> > > > WARNINGS_ARE_ERRORS += -Wno-undefined-bool-conversion
> > > > 
> > > > to make/linux/makefiles/adlc.make instead of make/linux/makefiles/gcc.make
> > > 
> > > While this is really a fix for an unrelated issue (support for a newer version \
> > > of clang), I do believe Volker's suggestion to only add it to adlc is much \
> > > better, if you really need to take this in as part of the JVMCI integration.
> > 
> > Sorry I dropped this.  After Gilles' hint I remembered the code in question was \
> > removed with: 
> > [JDK-8041620] Solaris Studio 12.4 C++ 5.13 change in behavior for placing friend \
> > declarations within surrounding scope - Java Bug System \
> > <https://bugs.openjdk.java.net/browse/JDK-8041620> 
> > and the remaining ones were fixed with:
> > 
> > [JDK-8077364] "if( !this )" construct prevents build on Xcode 6.3 - Java Bug \
> > System <https://bugs.openjdk.java.net/browse/JDK-8077364> 
> > I've removed the change:
> > 
> > http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6de55f563ee1
> Good!
> 
> > 
> > > 
> > > Apart from this, the makefile changes (still) look good.
> > 
> > Btw. we found a bug in creating the OptionDescriptors files; we get duplicate \
> > entries: 
> > $ cat build/macosx-x86_64-normal-server-release/jdk/modules/java.base/META-INF/services/jdk.internal.jvmci.options.OptionDescriptors \
> >  jdk.internal.jvmci.compiler.Compiler_OptionDescriptors
> > jdk.internal.jvmci.hotspot.HotSpotConstantReflectionProvider_OptionDescriptors
> > jdk.internal.jvmci.hotspot.HotSpotResolvedJavaFieldImpl_OptionDescriptors
> > jdk.internal.jvmci.hotspot.HotSpotResolvedJavaMethodImpl_OptionDescriptors
> > jdk.internal.jvmci.compiler.Compiler_OptionDescriptors
> > jdk.internal.jvmci.hotspot.HotSpotConstantReflectionProvider_OptionDescriptors
> > jdk.internal.jvmci.hotspot.HotSpotResolvedJavaFieldImpl_OptionDescriptors
> > jdk.internal.jvmci.hotspot.HotSpotResolvedJavaMethodImpl_OptionDescriptors
> > …
> > 
> > Would this be the right fix?
> > 
> > diff -r db1a815d2f6c make/gensrc/Gensrc-java.base.gmk
> > --- a/make/gensrc/Gensrc-java.base.gmkThu Sep 24 15:35:49 2015 -1000
> > +++ b/make/gensrc/Gensrc-java.base.gmkFri Sep 25 18:18:35 2015 +0200
> > @@ -94,6 +94,7 @@
> > $(GENSRC_DIR)/_gensrc_proc_done
> > $(MKDIR) -p $(@D)
> > ($(CD) $(GENSRC_DIR)/META-INF/jvmci.options && \
> > +    $(RM) -f $@; \
> > for i in $$(ls); do \
> > echo $${i}_OptionDescriptors >> $@; \
> > done)
> > 
> That seems like a reasonable fix, yes.

Thanks, but… (see below)

> 
> /Magnus
> 
> > And I see the same behavior for HotSpotJVMCIBackendFactory:
> > 
> > $ cat build/macosx-x86_64-normal-server-release/jdk/modules/java.base/META-INF/services/jdk.internal.jvmci.hotspot.HotSpotJVMCIBackendFactory \
> >  jdk.internal.jvmci.hotspot.amd64.AMD64HotSpotJVMCIBackendFactory
> > jdk.internal.jvmci.hotspot.sparc.SPARCHotSpotJVMCIBackendFactory
> > jdk.internal.jvmci.hotspot.amd64.AMD64HotSpotJVMCIBackendFactory
> > jdk.internal.jvmci.hotspot.sparc.SPARCHotSpotJVMCIBackendFactory
> > …
> > 
> > So I think a similar fix needs to be applied there.

…I've look at the code that creates this file and it isn't obvious to me how to fix \
it.  Any good ideas?

> > 
> > > 
> > > /Magnus
> > > 
> > > > 
> > > > > > > > If I want to work on the ppc64 implementation, which repository \
> > > > > > > > should I use?
> > > > > > > graal-jvmci-9.  Are you working on this for fun or do you want to have \
> > > > > > > that integrated with this JEP? 
> > > > > > > > Thank you and best regards,
> > > > > > > > Volker
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Fri, Sep 18, 2015 at 10:20 AM, Volker Simonis
> > > > > > > > <volker.simonis@gmail.com <mailto:volker.simonis@gmail.com>> wrote:
> > > > > > > > > For the top-level change I always get a strange error when \
> > > > > > > > > importing it: 
> > > > > > > > > patch failed, unable to continue (try -v)
> > > > > > > > > patch failed, rejects left in working dir
> > > > > > > > > errors during apply, please fix and refresh \
> > > > > > > > > 8062493_jvmci_top_0911.v1.patch 
> > > > > > > > > It is because of the strange path syntax of the last hunk in the \
> > > > > > > > > patch file: 
> > > > > > > > > --- old/./modules.xml 2015-09-16 15:11:14.000000000 -0700
> > > > > > > > > +++ new/./modules.xml 2015-09-16 15:11:14.000000000 -0700
> > > > > > > > > @@ -254,6 +254,14 @@
> > > > > > > > > <to>jdk.jfr</to>
> > > > > > > > > </export>
> > > > > > > > > <export>
> > > > > > > > > +      <name>jdk.internal.jvmci.hotspot</name>
> > > > > > > > > +      <to>jdk.jfr</to>
> > > > > > > > > +    </export>
> > > > > > > > > +    <export>
> > > > > > > > > +      <name>jdk.internal.jvmci.hotspot.events</name>
> > > > > > > > > +      <to>jdk.jfr</to>
> > > > > > > > > +    </export>
> > > > > > > > > +    <export>
> > > > > > > > > <name>sun.misc</name>
> > > > > > > > > <to>java.corba</to>
> > > > > > > > > <to>java.desktop</to>
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Notice the strange syntax "old/./modules.xml" and \
> > > > > > > > > "new/./modules.xml". If I remove the redundant './' from the path, \
> > > > > > > > > everything works fine. For some reason only the diffs for \
> > > > > > > > > modules.xml has this strange path syntax in the patch.
> > > > > > > > > 
> > > > > > > > > Regards,
> > > > > > > > > Volker
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Thu, Sep 17, 2015 at 10:32 PM, Christian Thalinger
> > > > > > > > > <christian.thalinger@oracle.com \
> > > > > > > > > <mailto:christian.thalinger@oracle.com>> wrote:
> > > > > > > > > > Since there are no objections I'm going to push this…
> > > > > > > > > > 
> > > > > > > > > > http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6a6766a8cbbf
> > > > > > > > > >  
> > > > > > > > > > > On Sep 16, 2015, at 3:05 PM, Christian Thalinger \
> > > > > > > > > > > <christian.thalinger@oracle.com> wrote: 
> > > > > > > > > > > I would like to add this change:
> > > > > > > > > > > 
> > > > > > > > > > > diff -r 2134e08cc132 src/share/vm/utilities/vmError.cpp
> > > > > > > > > > > --- a/src/share/vm/utilities/vmError.cpp      Wed Sep 16 \
> > > > > > > > > > >                 14:28:33 2015 -1000
> > > > > > > > > > > +++ b/src/share/vm/utilities/vmError.cpp      Wed Sep 16 \
> > > > > > > > > > > 15:04:02 2015 -1000 @@ -517,6 +517,10 @@ void \
> > > > > > > > > > > VMError::report(outputStream* st) { \
> > > > > > > > > > > Abstract_VM_Version::vm_release(), \
> > > > > > > > > > > Abstract_VM_Version::vm_info_string(), TieredCompilation ? ", \
> > > > > > > > > > > tiered" : "", +#if INCLUDE_JVMCI
> > > > > > > > > > > +                   EnableJVMCI ? ", jvmci" : "",
> > > > > > > > > > > +                   UseJVMCICompiler ? ", jvmci compiler" : "",
> > > > > > > > > > > +#endif
> > > > > > > > > > > UseCompressedOops ? ", compressed oops" : "",
> > > > > > > > > > > gc_mode(),
> > > > > > > > > > > Abstract_VM_Version::vm_platform_string()
> > > > > > > > > > > 
> > > > > > > > > > > It would be useful to see in the crash logs if this \
> > > > > > > > > > > experimental feature was turned on. 
> > > > > > > > > > > > On Sep 16, 2015, at 12:43 PM, Vladimir Kozlov \
> > > > > > > > > > > > <vladimir.kozlov@oracle.com> wrote: 
> > > > > > > > > > > > I updated top and hotspot webrev with latest (build) changes.
> > > > > > > > > > > > 
> > > > > > > > > > > > Vladimir
> > > > > > > > > > > > 
> > > > > > > > > > > > On 9/16/15 2:28 PM, Christian Thalinger wrote:
> > > > > > > > > > > > > > On Sep 16, 2015, at 6:52 AM, Christian Thalinger \
> > > > > > > > > > > > > > <christian.thalinger@oracle.com> wrote: 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie \
> > > > > > > > > > > > > > > <magnus.ihse.bursie@oracle.com> wrote: 
> > > > > > > > > > > > > > > On 2015-09-14 09:24, Christian Thalinger wrote:
> > > > > > > > > > > > > > > > The JEP itself can be found here:
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8062493 \
> > > > > > > > > > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8062493> 
> > > > > > > > > > > > > > > > Here are the webrevs:
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ \
> > > > > > > > > > > > > > > > <http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/> \
> > > > > > > > > > > > > > > > http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > The change has already undergone a few iterations of \
> > > > > > > > > > > > > > > > internal review by different people of different \
> > > > > > > > > > > > > > > > teams.  Most comments and suggestions were handled \
> > > > > > > > > > > > > > > > accordingly.  Although there is one open item we \
> > > > > > > > > > > > > > > > agreed we will address after the integration of JEP \
> > > > > > > > > > > > > > > > 243 and that work is captured in RFE: 
> > > > > > > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8134994 \
> > > > > > > > > > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8134994> 
> > > > > > > > > > > > > > > > SQE is still working on the tests and all test tasks \
> > > > > > > > > > > > > > > > can be seen as sub-tasks of the JEP. 
> > > > > > > > > > > > > > > > The integration will happen under the bug number:
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8136421 \
> > > > > > > > > > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8136421> 
> > > > > > > > > > > > > > > Hi Christian,
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (Adding build-dev since this review includes some major \
> > > > > > > > > > > > > > > build changes.) 
> > > > > > > > > > > > > > > The makefile changes looks good in general to me. I \
> > > > > > > > > > > > > > > have not reviewed the source code changes. 
> > > > > > > > > > > > > > > Some comments:
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > * modules.xml:
> > > > > > > > > > > > > > > Make sure you get sign-off from a Jigsaw developer for \
> > > > > > > > > > > > > > > modifying this file.
> > > > > > > > > > > > > > I've asked Alan to take a look.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > * hotspot/make/linux/makefiles/gcc.make:
> > > > > > > > > > > > > > > Seems unfortunate to have to disable a new warning \
> > > > > > > > > > > > > > > (undefined-bool-conversion) for newly incorporated \
> > > > > > > > > > > > > > > code. Is it not possible to fix the code emitting this \
> > > > > > > > > > > > > > > warning instead?
> > > > > > > > > > > > > > We had this question before.  It's not obvious because \
> > > > > > > > > > > > > > you can't see it in the regular diff views but this is \
> > > > > > > > > > > > > > under: 
> > > > > > > > > > > > > > ifeq ($(USE_CLANG), true)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > * make/common/MakeBase.gmk and \
> > > > > > > > > > > > > > > hotspot/make/gensrc/Gensrc-java.base.gmk: I see a \
> > > > > > > > > > > > > > > coming merge conflict. In jdk9/dev, there is now a new \
> > > > > > > > > > > > > > > function that performs the same function as CreatePath, \
> > > > > > > > > > > > > > > but it's named PathList. (It's a bit unfortunate that \
> > > > > > > > > > > > > > > this code snippet has bounced around as patches without \
> > > > > > > > > > > > > > > a definite name.) I assume you are going to push this \
> > > > > > > > > > > > > > > through a hotspot forest. If the PathList patch reaches \
> > > > > > > > > > > > > > > the hotspot repo before this, please remove the \
> > > > > > > > > > > > > > > CreatePath from MakeBase, and change the calls to \
> > > > > > > > > > > > > > > CreatePath to PathList instead. (I could only find such \
> > > > > > > > > > > > > > > calls in hotspot/make/gensrc/Gensrc-java.base.gmk). If \
> > > > > > > > > > > > > > > this patch goes in before that, we'll need to give a \
> > > > > > > > > > > > > > > heads-up to the integrator about this conflict.
> > > > > > > > > > > > > > Thanks for the heads up.
> > > > > > > > > > > > > Erik sent me a patch to avoid merge conflicts.  I've \
> > > > > > > > > > > > > integrated two changesets: 
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > \
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Another potential coming merge conflict relates to \
> > > > > > > > > > > > > > > ListPathsSafely in Gensrc-java.base.gmk. There is \
> > > > > > > > > > > > > > > currenlty a review out from Erik which modifies the API \
> > > > > > > > > > > > > > > for ListPathsSafely. If/when it goes in, the call to \
> > > > > > > > > > > > > > > ListPathsSafely in Gensrc-java.base.gmk will need to be \
> > > > > > > > > > > > > > > modified (Erik can give advice on how). Depending on \
> > > > > > > > > > > > > > > timing, this too might hit the integrator rather than \
> > > > > > > > > > > > > > > your push.
> > > > > > > > > > > > > > Ok, thanks.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > /Magnus
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread] 

Configure | About | News | Add a list | Sponsored by KoreLogic