Request for approval: Backport of 6781583 to hs14/OpenJDK6

[Andrew John Hughes]

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/jni_md.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/os_linux.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 jvm_path() 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 size_t 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