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

Andrew John Hughes gnu_andrew at member.fsf.org
Fri Sep 4 10:21:26 PDT 2009 

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

Hello?

Can I push this change or not?
-- 
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