RFR: 8248817: Windows: Improving common cross-platform code
David Holmes
david.holmes at oracle.com
Thu Jul 16 02:30:30 UTC 2020
Hi Ludovic,
On 16/07/2020 4:29 am, Ludovic Henry wrote:
> Hi David,
>
> I gave a try to using `__int32` and `int` in place of `long`, but MSVC complains of type differences in the parameters passed to the `Interlocked*` functions.
>
> ```
> C:\git\jdk\src\hotspot\os_cpu\windows_x86\atomic_windows_x86.hpp(103): error C2665: '_InterlockedCompareExchange': none of the 4
> overloads could convert all the argument types
> C:\git\jdk\build\devkit\10\include\um\winbase.h(9501): note: could be 'unsigned __int64 _InterlockedCompareExchange(volatile unsigned __int64 *,unsigned __int64,unsigned __int64)'
> C:\git\jdk\build\devkit\10\include\um\winbase.h(9488): note: or 'unsigned long _InterlockedCompareExchange(volatile unsigned long *,unsigned long,unsigned long)'
> C:\git\jdk\build\devkit\10\include\um\winbase.h(9477): note: or 'unsigned int _InterlockedCompareExchange(volatile unsigned int *,unsigned int,unsigned int)'
> [.....]
> ```
That's a shame - and somewhat surprising to me. But so be it.
> To clarify the use of `long` over `__int32` or `int`, I've instead added a comment (see http://cr.openjdk.java.net/~burban/luhenry/8248817-atomics/webrev.02/src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp.udiff.html).
>
> Another complementary solution is to call the `DEFINE_*` macros, not with a numerical constant as the first argument, but with the value returned by the sizeof of the second argument. For example, we can have the following for cmpxchg:
>
> ```
> DEFINE_STUB_CMPXCHG(sizeof(char), char, _InterlockedCompareExchange8) // Use the intrinsic as InterlockedCompareExchange8 does not exist
> DEFINE_STUB_CMPXCHG(sizeof(long), long, InterlockedCompareExchange)
> DEFINE_STUB_CMPXCHG(sizeof(__int64), __int64, InterlockedCompareExchange64)
> ```
>
> In that case, we can even do away with the first argument altogether, like the following:
>
> ```
> #define DEFINE_STUB_CMPXCHG(StubName, StubType) \
> template<> \
> template<typename T> \
> inline T Atomic::PlatformCmpxchg<sizeof(StubType)>::operator()(T volatile* dest, \
> T compare_value, \
> T exchange_value, \
> atomic_memory_order order) const { \
> STATIC_ASSERT(sizeof(StubType) == sizeof(T)); \
> return PrimitiveConversions::cast<T>( \
> StubName(reinterpret_cast<StubType volatile *>(dest), \
> PrimitiveConversions::cast<StubType>(exchange_value), \
> PrimitiveConversions::cast<StubType>(compare_value))); \
> }
>
> DEFINE_STUB_CMPXCHG(_InterlockedCompareExchange8, char) // Use the intrinsic as InterlockedCompareExchange8 does not exist
> DEFINE_STUB_CMPXCHG(InterlockedCompareExchange, long)
> DEFINE_STUB_CMPXCHG(InterlockedCompareExchange64, __int64)
>
> #undef DEFINE_STUB_CMPXCHG
> ```
>
> That makes it very clear that the type is for the Interlocked* function, not the source data type (like int32_t/int64_t or jint/jlong).
>
> I uploaded updated webrevs at http://cr.openjdk.java.net/~burban/luhenry/8248817-atomics/webrev.02/ and http://cr.openjdk.java.net/~burban/luhenry/8248817-atomics/webrev.03/, with the former not containing this `sizeof(StupType)` change, and the latter containing it.
I'm not very good with templates so I've asked Kim Barrett if he can
take a look at this aspect.
As a style nit can you realign the parameters for those declaration you
modified e.g.
59 inline D Atomic::PlatformAdd<sizeof(StubType)>::add_and_fetch(D
volatile* dest, \
60 I
add_value, \
61
atomic_memory_order order) const { \
76 inline T Atomic::PlatformXchg<sizeof(StubType)>::operator()(T
volatile* dest, \
77 T
exchange_value, \
78
atomic_memory_order order) const { \
etc.
Thanks,
David
-----
> Thank you!
>
> ________________________________________
> From: David Holmes <david.holmes at oracle.com>
> Sent: Wednesday, July 15, 2020 06:32
> To: Ludovic Henry; hotspot-runtime-dev at openjdk.java.net
> Cc: openjdk-aarch64
> Subject: Re: RFR: 8248817: Windows: Improving common cross-platform code
>
> Hi Ludovic,
>
> On 15/07/2020 11:15 pm, Ludovic Henry wrote:
>> Hi David,
>>
>> Thanks for your feedback.
>>
>>> can we use __int32 for clarity rather than "long"?
>>
>> The Win32 API explicitly uses `long`, and I made sure for these `DEFINE_` macros to use the type used in the declaration of the API. If you are ok with the difference, I'm happy to change that to __int32.
>
> I prefer to see _int32 in our code as "long" can be quite ambiguous
> depending on the reader (and something we are trying to eradicate from
> shared code -not everyone is aware of the LLP64 programming model versus
> LP64).
>
> Thanks,
> David
> -----
>
>> I've uploaded the new webrevs at https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-atomics%2Fwebrev.01&data=02%7C01%7Cluhenry%40microsoft.com%7C6b7ccc27a32e4a54478e08d828c3901c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637304167686421708&sdata=GfaCMQYLrWynyShUR%2B6eRjpMR4Y%2Bj05PyaVAstTUoKU%3D&reserved=0
>>
>> (I've also moved the previous webrevs to their respective webrev.00 folders).
>>
>> Thank you,
>>
>> --
>> Ludovic
>> ________________________________________
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Sunday, July 12, 2020 19:43
>> To: Ludovic Henry; hotspot-runtime-dev at openjdk.java.net
>> Cc: openjdk-aarch64
>> Subject: Re: RFR: 8248817: Windows: Improving common cross-platform code
>>
>> Hi Ludovic,
>>
>> On 9/07/2020 11:55 pm, Ludovic Henry wrote:
>>> Hello,
>>>
>>> As part of adding support for Windows-AArch64, I've had the opportunity to read through most of the Windows-x86 code. In doing so, I found some code that I think can be simplified and made easier to read and maintain.
>>>
>>> The three areas I have found are:
>>> - Atomics: Hotspot doesn't make use of existing intrinsics provided by MSVC and Win32, even ones available since Windows XP.
>>> - Exception handling: there is some code repetition which, even if functional, is subpar.
>>> - Frames: we can use the existing os::fetch_frame_from_context to simplify the code and reduce frame parsing logic duplication.
>>>
>>> I've split the webrevs along the above lines, making each simpler to review. I'm also hosting these webrevs on Bernhard Urban's CR as I currently do not have authorship. I'll also work with him to update the description of the JBS.
>>
>> Thanks for doing the split!
>>
>> As a general comment can you please ensure that the Oracle copyright
>> second year is updated to 2020. Thanks.
>>
>> Overall these cleanups look good. Thanks for providing them.
>>
>>> JBS: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8248817&data=02%7C01%7Cluhenry%40microsoft.com%7C6b7ccc27a32e4a54478e08d828c3901c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637304167686421708&sdata=7X%2FV4ILiMMUmimw7XkvjDS9qD%2FhgnGqv%2FjNQpLFsARs%3D&reserved=0
>>> Webrevs:
>>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-atomics%2F&data=02%7C01%7Cluhenry%40microsoft.com%7C6b7ccc27a32e4a54478e08d828c3901c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637304167686421708&sdata=WlTIIZQG8qP2M0qnDQgjYiQcDa5RjvOV4Dqa7zi9JrI%3D&reserved=0
>>
>> Love this cleanup! Great to see all the stubroutines go for x86.
>>
>> src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp
>>
>> Please delete this entire (archaic) comment block.
>>
>> 42 // The following alternative implementations are needed because
>> 43 // Windows 95 doesn't support (some of) the corresponding Windows NT
>> 44 // calls. Furthermore, these versions allow inlining in the caller.
>> 45 // (More precisely: The documentation for InterlockedExchange says
>> 46 // it is supported for Windows 95. However, when single-stepping
>> 47 // through the assembly code we cannot step into the routine and
>> 48 // when looking at the routine address we see only garbage code.
>> 49 // Better safe then sorry!). Was bug 7/31/98 (gri).
>> 50 //
>> 51 // Performance note: On uniprocessors, the 'lock' prefixes are not
>> 52 // necessary (and expensive). We should generate separate cases if
>> 53 // this becomes a performance problem.
>>
>> In this (and elsewhere):
>>
>> 80 DEFINE_STUB_ADD(4, long, InterlockedAdd)
>> 81 DEFINE_STUB_ADD(8, __int64, InterlockedAdd64)
>>
>> can we use __int32 for clarity rather than "long"?
>>
>>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-exception-handling%2F&data=02%7C01%7Cluhenry%40microsoft.com%7C6b7ccc27a32e4a54478e08d828c3901c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637304167686421708&sdata=WIO7rvn6PEqROcyRJlNxGw0etcvNWP6Me8s4Q2PXcCE%3D&reserved=0
>>
>> Looks good!
>>
>>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-frames%2F&data=02%7C01%7Cluhenry%40microsoft.com%7C6b7ccc27a32e4a54478e08d828c3901c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637304167686421708&sdata=oENY9tbPbPV4lhcmBsH%2FYFwb5e76OtC1EcALGXClKDY%3D&reserved=0
>>
>> Looks good!
>>
>> Thanks,
>> David
>> -----
>>
>>> Tests: jtreg:hotspot:tier, jtreg:jdk:tier1, jtreg:jdk:tier2, jtreg:langtools on Windows-x86 and Windows-x86_64, no regressions.
>>>
>>> Thank you,
>>>
>>> --
>>> Ludovic
>>>
More information about the hotspot-runtime-dev
mailing list