[PATCH FOR REVIEW]: Make source/target options explicit for MakeDeps and jvmti

John Coomes John.Coomes at sun.com
Thu Aug 20 17:41:04 PDT 2009 

Andrew John Hughes (gnu_andrew at member.fsf.org) wrote:
> 2009/8/20 John Coomes <John.Coomes at sun.com>:
> > Andrew John Hughes (gnu_andrew at member.fsf.org) wrote:
> >> 2009/8/18 John Coomes <John.Coomes at sun.com>:
> >> > Daniel D. Daugherty (Daniel.Daugherty at Sun.COM) wrote:
> >> >> Andrew,
> >> >>
> >> >> A couple of comments:
> >> >>
> >> >> - if you specify '-source 6', then '-target 6' is implied
> >> >>   so you don't really need both
> >> >> - I would prefer to see a top-level macro that defines the
> >> >>   bootstrap tools javac option and then have the various
> >> >>   Makefiles use that macro. Something in make/defs.make:
> >> >>
> >> >>       BOOTSTRAP_TOOLS.JAVAC.FLAGS="-source 6"
> >> >>
> >> >>   or something like that.
> >> >
> >> > +1 on the top-level macro.  In HotSpot, I would call it JAVAC_FLAGS or
> >> > COMPILE_JAVAC_FLAGS, and add it to the COMPILE.JAVAC macro (spelled
> >> > COMPILE_JAVAC on windows) so that every compilation uses it.  Then
> >> > fewer places need to be touched.
> >> >
> >> > Also, please leave the default value empty.  On platforms that need
> >> > it, it can be set on the command line or in the environment.
> >> > Specifying a value requires periodic maintenance.  Yes, the period is
> >> > long, but it's a pain when the build breaks because of some stale
> >> > value in a makefile.
> >> >
> >>
> >> Ok, here's the changeset I was working on before I received your mail
> >> and which I've since successfully tested with hotspot-rt:
> >>
> >> http://cr.openjdk.java.net/~andrew/ecj/03/webrev.02/
> >
> > I submitted your patch to the build queue; it will take several hours,
> > there's one job ahead of it.
> >
> > After looking at it again, the include of make/defs.make in
> > make/windows/makefiles/rules.make will most likely fail to build.  The
> > latter is a windows nmake file, so the gnu-specific syntax will be
> > rejected and there are other unixisms in there (e.g., shell until).
> >
> 
> I had a feeling Windows would be the one to bite...
> 
> > Instead, try updating MAKE_ARGS in make/defs.make, I believe those are
> > exported to sub-makes.  Since it has to be portable, I believe you'll
> > have to use '_' instead of '.' in the macro name.
> >
> 
> Ok, in http://cr.openjdk.java.net/~andrew/ecj/03/webrev.03/ I've
> reverted the addtion
> of defs.make in favour of using MAKE_ARGS and renamed the variables
> using underscores (which I prefer anyway).
> 
> Where I am confused is how the MAKE_ARGS changes get through to the
> Windows build if it doesn't parse defs.make.
> makefiles/windows/defs.make seems to imply that the top-level one will
> be read.
> 
> >> This defines the variables in the top-level defs.make file.  The
> >> versions are stored separately, in the same way as they are in the
> >> JDK:
> >>
> >> http://hg.openjdk.java.net/jdk7/build/jdk/rev/80368890a2a0
> >>
> >> SA is still done separately as it's currently 1.4.  The default for others is 6.
> >> The rest of the patch ensures this makefile is picked up and its
> >> values used for compilation across all three platforms.
> >>
> >> This is added to the invocations rather than COMPILE.JAVAC.  As you
> >> imply in your mail, this value varies between platforms and even
> >> within the same platform.  Just the GNU/Linux port itself defines it
> >> in about four ways.
> >
> > Take a look, it's simply selecting which path to use.  After the def
> > of COMPILE.JAVAC, a simple
> >
> >        COMPILE.JAVAC += $(BOOTSTRAP_JAVAC.FLAGS)
> >
> > should do it.  Windows will have to be different, though.
> >
> 
> Yes, the other problem is this forces the same options to SA as well
> which uses COMPILE.JAVAC.  To keep the current status quo (at least
> for now), these need a different set of flags.
> 
> >> I don't agree with leaving the value blank.  The issue is relevant on
> >> all platforms, as leaving it blank makes the build open to whatever
> >> the default value is of the bootstrap JDK.  For instance, this just
> >> changed to 7 with OpenJDK, so current builds will produce 1.7 bytecode
> >> while builds using an older OpenJDK7 or OpenJDK6 will use 6.  From
> >> personal experience, it's much harder to debug a problem when you have
> >> to take into account the boot JDK being used, which may vary, than
> >> when you have an explicit value.
> >>
> >> It seems much clearer to specify it explicitly in one common place for
> >> all platforms, which can also be easily overridden from the command
> >> line if needed.  This is the same as is now done for JDK, langtools,
> >> jaxp and jaxws and will shortly be the case in corba.
> >
> > The jdk, langtools, etc., repos are much more sensitive to this
> > setting than hotspot, which uses it only for makedeps and jvmti header
> > generation.  It's non-critical, and I think it's safe to assume that
> > the default class file version produced by a boot jdk can also be run
> > by that same boot jdk.
> >
> > So far, ecj is the only known boot env needing this set explicitly.
> 
> That kind of assumption is the thing that tends to come back and bite
> you.  It's especially true because:
> 
>   * The boot JDK is something determined on a per-user basis, so just
> updating the system JDK may cause a failure that's hard to diagnose.
>   * Failures in HotSpot javac builds are already hard to diagnose
> because it doesn't print out the javac invocation.  It took me longer
> to track this down than the equivalents in JDK and CORBA, and I've
> been working with the OpenJDK build for about two years now.

FWIW, you can get hotspot builds to be verbose with

	gmake ... QUIETLY=

(i.e., set QUIETLY to an empty value) and it should show all command
invocations.

> Yes, ecj is the only one that fails.  But we now have at least two
> JDKs that will produce different results: OpenJDK6 (which defaults to
> 6) and OpenJDK7 (which defaults to 7).  There are even more if you
> include the proprietary JDKs and JDK 1.5 which I assume can also build
> OpenJDK at a push.
> ...

Different, but still correct.  I'm resisting a default because it will
get stale (they always do, SA is a good example).  And it's worked for
*years* (6? 8?) without a problem.  But I won't reject the rest of it
because there's a default.  Sigh.

> Finally, switching SA to use the defaults would be a visible change.

Dan mentioned the SA values in passing; my impression was he thought
they should be updated or removed.  I don't work with the SA, but I
suspect the settings are relics from using a boot jdk that had 1.3 as
a default when SA needed 1.4 features.  Anyone recall?

Even if for some odd reason SA source should be limited to using 1.4
features, if you update JAVAC_COMPILE to include JAVAC_FLAGS (or
whatever it is), and the SA makefile also includes -source/-target
options on the command-line, the latter will override the ones in
JAVAC_FLAGS (the SA command line may need to include both -source and
-target).  At least try it, I think it'll be a simpler change.

I submitted your latest patch to the build queue for test builds;
hopefully it will send you mail when it's finished (but I've never
tried including a non-Sun email address before).

-John

> >> >> Which repo are you targeting for this change?
> >> >
> >> > I'd suggest hotspot-rt.
> >> >
> >> > Note that all hotspot changes have to be built on all platforms before
> >> > being pushed; we have an automated system called jprt that does builds
> >> > and tests on the various platforms.  Unfortunately, it isn't available
> >> > externally.  Sync up with the latest hotspot-rt and then contact me
> >> > privately with a patch or your repo location and I'll run it through.
> >> >
> >>
> >> The patch from this webrev is against hotspot-rt so can be tested.
> >>
> >> >> Yes, I see that SA uses '-source 1.4' in each platform makefile.
> >> >> That should be fixed also, but that's another issue...
> >> >
> >> > Sigh.  Stale values in makefiles ...
> >> >
> >>
> >> Also fixed now by this patch.  At least 1.4 is in one common makefile
> >> rather than occurring six times over three makefiles... :)
> >>
> >> > -John
> >> >
> >> >> > Andrew John Hughes wrote:
> >> >> > The makeDeps and jvmti bootstrap tools are built without explicit
> >> >> > source and target options.
> >> >> >
> >> >> > This webrev:
> >> >> >
> >> >> > http://cr.openjdk.java.net/~andrew/ecj/03/webrev.01/
> >> >> >
> >> >> > sets them to 6 explicitly. The change has to be replicated three
> >> >> > times, because for some reason, the javac invocations are in
> >> >> > platform-specific makefiles.  I've tested the change successfully on
> >> >> > GNU/Linux, where it allows ecj to be used as javac.  ecj defaults to
> >> >> > <1.5 and thus otherwise fails to build.  I've assumed that the same
> >> >> > changes are applicable for Solaris and Windows, and that the source
> >> >> > and target options don't have to be written in Latin or something to
> >> >> > work on these platforms.
> >> >> >
> >> >> > Is this ok to push?
> >> >> >
> >> >> > Thanks,
> >> >> >
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >> Andrew :-)
> >>
> >> Free Java Software Engineer
> >> Red Hat, Inc. (http://www.redhat.com)
> >>
> >> Support Free Java!
> >> Contribute to GNU Classpath and the OpenJDK
> >> http://www.gnu.org/software/classpath
> >> http://openjdk.java.net
> >>
> >> PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
> >> Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8
> >
> >
> 
> 
> 
> -- 
> Andrew :-)
> 
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
> 
> Support Free Java!
> Contribute to GNU Classpath and the OpenJDK
> http://www.gnu.org/software/classpath
> http://openjdk.java.net
> 
> PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
> Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

More information about the hotspot-dev mailing list