Request for approval: Backport of 6781583 to hs14/OpenJDK6 (original) (raw)

Andrew John Hughes gnu_andrew at member.fsf.org
Mon Jun 8 14:40:24 PDT 2009

2009/6/8 Kelly O'Hair <Kelly.Ohair at sun.com>:

Someone from the hotspot team really needs to review this

Errr.... 4 of them already did: http://hg.openjdk.java.net/jdk7/jdk7/hotspot/rev/2328d1d3f8cf

I'm not the author of this patch, I just backported it from OpenJDK7 where it's been applied for over six months. I don't remember this patch being reviewed openly and I disliked having all the fixes rolled into one megafix like this. But given this had been reviewed and was in 7, it seemed better to backport this (just a matter of hg export/import) rather than trying to get each individual IcedTea patch through, especially as one of ours just turns off -Werror... (to some degree anyway)

The relevant IcedTea patches are icedtea-fortify-source.patch (author: Matthias Klose), hotspot/default/icedtea-format.patch (author: me), icedtea-no-bcopy.patch (author: Matthias Klose), hotspot/default/icedtea-gcc-4.3.patch (originally part of a patch by Bernhard Rosenkraenzer according to the ChangeLog).

, but my

observations on these changes:

 * Changing the typedef for jlong seemed really risky to me, but I  investigated it more and I'm pretty sure this is the right change,  and forces it to match the public definition from jni.h/jnimd.h.  So it looks right.

It's kind of annoying it has to be defined repeatedly for each arch. We also had to add this to Zero in IcedTea.

 * The realpath() changes in src/os/linux/vm/oslinux.cpp seem to change  the definition of this method when an error occurs, the control  flow has changed, and I'm not exactly sure what the return value is  when realpath() fails. I doubt realpath() fails very often here,  but won't the buf[] array get some arbitrary directory when realpath()  fails? Is that ok? That jvmpath() is a bizarre little method. :^{

In IcedTea, an unused variable is used which isn't ideal either, but at least doesn't change the semantics.

 * Changing the lineno field in src/share/vm/utilities/vmError.hpp  from an int to a sizet might seem right type wise, but sure seems  like a waste of space. Does a lineno really need to use up 64bits of space?

This doesn't seem to be patched in IcedTea, so I don't know the reasoning for the change. Increasing the size to 64 bits on x86_64 does seem wrong though.

 * The PTR changes look right.

Yeah, our patches are virtually identical on this :)

 * The addition of an unused local variable like in  src/share/vm/utilities/ostream.cpp  to avoid a warning message just seems wrong to me, and will likely  create a new warning message about an unused local variable.  Seems like I've had this discussion before.  I prefer an explicit (void) cast on a function call whose return  value is not needed and I am frustrated with GCC's inability to  understand what that (void) cast means.  I really like the idea of using -Werror and not allowing warnings,  but at times like this I have to wonder if we should just remove  the -Werror unless we know that specific version of GCC is < 4.3.

IcedTea does the same for this and the one above, but uses attribute(unused) to avoid the warning.

But like I said, a 'real' hotspot developer should probably review this. ;^)

-kto

Andrew John Hughes wrote:

The following webrev: http://fuseyism.com/6781583/webrev.00/ is backported from OpenJDK7.  It allows hs14 to be built using GCC 4.3 and above.  Otherwise, the build fails.  IcedTea has been applying an equivalent fix as three separately developed patches, two of which have been there pretty much since its inception in the summer of 2007. Ok to commit?

-- 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 jdk6-dev mailing list