Re: RFR (M) 8186903: Remove j-types from Atomic

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M) 8186903: Remove j-types from Atomic

coleen.phillimore
resending.

On 12/6/17 11:00 PM, [hidden email] wrote:

>
>
> On 12/6/17 9:41 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 7/12/2017 7:59 AM, [hidden email] wrote:
>>> Summary: Change parameter types from jlong, jint, jbyte to int64_t,
>>> int32_t and int8_t respectively
>>
>> That's fine. But you also made a bunch of changes to rename the "ptr"
>> variants to "long", and in the process changed intptr_t to int64_t.
>> The change ptr->long isn't really accurate. And intptr_t and int64_t
>> are different sizes on 32-bit! So this change seems problematic and
>> out of scope by your own description. That said I thought we had/were
>> getting rid of the "ptr" variants ??
>
> Yes, I did rename atomic_add_ptr_entry to atomic_add_long_entry. This
> is to match the other names which are currently empty, "long" and
> "byte".   This was a last vestige of the ptr variants.   It's only
> used on 64 bit windows, so changing it to intptr_t to int64_t is
> correct.   For some reason, there isn't a long variant on 32 bits. 
> Possibly because we don't add jlong values in the vm?
> ent
> So this renaming makes it consistent with the other names, which then
> can all be changed at once, when nice names are agreed upon.
>>
>>> Leave renaming functions for later change.
>>>
>>> Ran JPRT which builds more Oracle platforms, mach5 tier1-5 on
>>> windows/linux x64 and tier1 on solaris-sparcv9.  Also built Zero
>>> product mode (fails building fastdebug for some other reason).
>>>
>>> Other platforms: could you please test this patch?
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186903.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8186903
>>>
>>> This change is for JDK 11.
>>
>> src/hotspot/cpu/x86/stubGenerator_x86_32.cpp
>>
>> Return type should be int32_t not int
>>
>> src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp
>>
>> -  static intptr_t  (*atomic_xchg_long_func)     (jlong, volatile
>> jlong*);
>> -  static intptr_t  atomic_xchg_long_bootstrap   (jlong, volatile
>> jlong*);
>>
>> Wow - that looks like a bug! Only returns 32-bits on 32-bit!
>
> This one also is only used on 64 bit.  I have to confess I don't
> follow all the paths of the atomics and their various translations
> down to the platform levels.   I'm not sure how windows 32 bit calls
> atomic_add or xchg for 64 bit values (or if it is not implemented).
>>
>> Everything else seems fine.
>>
>
> Thanks for the eagle eye review.
> Coleen
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>