> 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?
> 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.
>> Return type should be int32_t not int
>> - static intptr_t (*atomic_xchg_long_func) (jlong, volatile
>> - static intptr_t atomic_xchg_long_bootstrap (jlong, volatile
>> 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.