RFR: 8150426: Wrong cast in metadata_at_put

[David Holmes]

On 23/02/2016 1:49 PM, Kim Barrett wrote:
>> On Feb 22, 2016, at 9:36 PM, David Holmes <david.holmes at oracle.com> wrote:
>> On 23/02/2016 11:57 AM, Kim Barrett wrote:
>>> Please review this fix of an incorrect cast.
>>>
>>> This fix was contributed by Timo Kinnunen.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8150426
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8150426/webrev/
>>
>> Fix seems fine, but have to wonder given:
>>
>> 128 #ifdef _LP64
>> 129   Metadata* metadata_at(int which) const {
>> 130     return (Metadata*)*long_at_addr(which); }
>> 131   void metadata_at_put(int which, Metadata* contents) {
>> 132     *long_at_addr(which) = (jlong)contents;
>> 133   }
>> 134 #else
>>
>> is Windows setting _LP64 even though it is LLP64? Or are we (incorrectly) forcing _LP64 for all 64-bit builds?
>
> Apparently yes.
>
> Coleen wasted a bunch of time on this very same bug a couple of months
> ago, when she used metadata_at and metadata_at_put as a starting point
> for something she was working on. I helped her track that down, so I
> was already familiar with this problem when Timo reported it.
>
> egrep -r -H --exclude-dir=.hg LP64 | egrep -v "\.(h|c|hpp|cpp|xml):" | grep windows
>
> produces around 15 hits in hotspot/make/windows and common/autoconf,
> though I think the autoconf hits don't affect windows, so it's only
> those in hotspot that are interesting.
>
> There is only one occurrence of LLP64 in the JDK forest, in a comment here:
> ./jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java:    // This means it is 0xffffffff in ILP32/LLP64 but 0xffffffffffffffff in LP64.
>
> I have a *very* strong memory (from soon after I started working here)
> of seeing a comment or document somewhere that described why we're
> building Windows with _LP64, but haven't been able to re-find it. I
> know I was surprised at the time.

I'm always surprised to re-discover that Windows is LLP64 rather than 
LP64. :)

I suspect historically we have always conflated CPU bit-ness with the 
programming model - hence LP64/_LP64 really means "on a 64-bit CPU" in 
our code, with very few places where we really rely on "long is 
64-bits". Really we should "never" be using plain long in the VM code.

> I also wonder whether this still a problem?
> https://bugs.openjdk.java.net/browse/JDK-7000871
> _LP64 is not always set up correctly
>
> It looks like, in the current jdk9 forest, _LP64 is only set by the
> build system, never by headers. I think there might be a couple more
> open bugs related to LP64 that could be closed.

I always thought the _LP64 version was something that was supposed to be 
set by the compiler for the code to see, and we also set the LP64 
variable for our own use. For Solaris Studio at least we don't pass 
_LP64 nor should we - except for the odd case of building adlc, and a 
dtrace option.

As you say we don't set _LP64 in the sources - which is a good thing. 
And it seems the sources don't actually use LP64 either, which is also 
good IMHO. So I think there is some scope for cleanup here.

Cheers,
David