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

Kelly O'Hair Kelly.Ohair at Sun.COM
Mon Jun 8 15:44:07 PDT 2009

Andrew John Hughes wrote:

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

Humm... ok, ... ... sorry... if this is in jdk7 already, then so be it.

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.

Yeah, I probably would have separated them, oh well.

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)

Agreed. Matching jdk7 is the best route to take.

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.

Yeah... I think the idea of the xxx_md.h files was to avoid ifdef's, then we turn around and fill them with ifdef's :^{

Note that there has been a long standing issue that the jni*.h files in hotspot may not match the jni*.h files in the resulting jdk install image, they come from the copies checked into the jdk repository (see the directories jdk/src/{share,solaris,windows}/javavm/export). So one of these days, the public jni interface will be defined by hotspot 'and' it will deliver the jni include files too. ;^)

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.

If the hotspot guys made this change in jdk7, I guess that's what they want. The 'gamma' and '_g' stuff isn't executed in most cases anyway.

* 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 x8664 does seem wrong though.

Yeah, seemed like it to me. Oh well.

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

ok.

I'm just never a fan of planting compiler implementation specifics into the source.

-kto

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?

More information about the jdk6-dev mailing list