Request for approval: Backport of 6781583 to hs14/OpenJDK6

[Kelly O'Hair]

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, ... <wipe egg off face>... 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/jni_md.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/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.

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

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