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

Andrew John Hughes gnu_andrew at member.fsf.org
Tue Sep 1 03:01:30 PDT 2009 

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

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