Request for approval: Backport of 6781583 to hs14/OpenJDK6

[Andrew John Hughes]

2009/6/8 Kelly O'Hair <Kelly.Ohair at sun.com>:
>
>
> 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.
>

No apology required.  It just shows this patch should have had an open
review before being committed.

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

Yeah, I'm not trying to defend it by any means :)

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



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