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

Andrew John Hughes gnu_andrew at member.fsf.org
Thu Aug 20 05:22:34 PDT 2009 

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.

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.

One of the side effects of releasing code to the world is that people
are going to try and run into all sorts of corner cases.  Assuming the
defaults are good works fine when the only people building the code
are Sun developers, but this is no longer true and something like this
is just the kind of thing a casual builder of the code will run into
and wonder why it fails.

I agree this code is less used than the equivalent JDK and CORBA code,
but surely it's better to be as consistent as possible across the
codebase?

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

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