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

Andrew John Hughes gnu_andrew at member.fsf.org
Mon Aug 24 09:57:34 PDT 2009 

2009/8/22 Andrew John Hughes <gnu_andrew at member.fsf.org>:
> 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
>


Is this now ok to push? It passed the build.
-- 
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