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

Andrew John Hughes gnu_andrew at member.fsf.org
Fri Aug 21 20:17:56 PDT 2009 

2009/8/22 David Schlosnagle <schlosna at gmail.com>:
> Andrew,
>
> I'm just a beginner here trying to get a better understanding of the JDK by
> scanning code reviews and I have a comment on the changes in
> <http://cr.openjdk.java.net/~andrew/ecj/03/webrev.03/>.
>
> In all the sa.make files (Linux, Solaris & Windows), should the -g flag be
> removed from the two modified lines since it's included in the
> SA_JAVAC_FLAGS via JAVAC_FLAGS in defs.make? I'd assume that the duplicate
> -g won't cause failures, but it seems to force all debug info to always be
> included even if one tried to override the compile flags to disable or alter
> the debug information included.
>
> Thanks,
> David Schlosnagle
>

Well spotted! They should have been removed.  Revised in
http://cr.openjdk.java.net/~andrew/ecj/03/webrev.04

>
> On Thu, Aug 20, 2009 at 8:22 AM, 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.
>>
>> 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
>
>



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