Bug ID: JDK-8246770 Atomic::add() with 64 bit value fails to link on 32-bit platforms

Type: Bug
Component: hotspot
Sub-Component: jfr
Affected Version: 15

Priority: P2
Status: Closed
Resolution: Fixed

Submitted: 2020-06-08
Updated: 2024-10-17
Resolved: 2020-06-08

JDK 15
15 b27Fixed

Spotted on 32-bit builds post-check in for JDK-8242088.

Atomic::add(&_tip._value, (traceid)1);  // fails to link where type traceid is 64-bit

Manifests as link issues, for example:

jfrStorage.obj : error LNK2019: unresolved external symbol "public: unsigned __int64 __thiscall Atomic::PlatformAdd<8>::add_and_fetch<unsigned __int64,unsigned __int64>(unsigned __int64 volatile *,unsigned __int64,enum atomic_memory_order)const " (??$add_and_fetch@_K_K@?$PlatformAdd@$07@Atomic@@QBE_KPC_K_KW4atomic_memory_order@@@Z) referenced in function "public: static unsigned __int64 __cdecl Atomic::add<unsigned __int64,unsigned __int64>(unsigned __int64 volatile *,unsigned __int64,enum atomic_memory_order)" (??$add@_K_K@Atomic@@SA_KPC_K_KW4atomic_memory_order@@@Z)
jfrTraceIdKlassQueue.obj : error LNK2001: unresolved external symbol "public: unsigned __int64 __thiscall Atomic::PlatformAdd<8>::add_and_fetch<unsigned __int64,unsigned __int64>(unsigned __int64 volatile *,unsigned __int64,enum atomic_memory_order)const " (??$add_and_fetch@_K_K@?$PlatformAdd@$07@Atomic@@QBE_KPC_K_KW4atomic_memory_order@@@Z)
d:/priv/openjdk/nb/NTintel/nightly/output-jdk-dev/support/modules_libs/java.base/server/jvm.dll : fatal error LNK1120: 1 unresolved externals

Changing to a solution based on Atomic::cmpxchg() works.

> This is my main worry, but it looks like the inlined asm assumes cx8() support is present, for example: x86 provides cmpxchg8b whether a 32-bit or 64-bit chip ARM 32-bit only has a direct 64-bit atomic capability on v7a but there is a Linux kernel helper available otherwise. PPC 32-bit was the only problematic platform and in that case we could not have 64-bit atomic ops in the VM, and the Java volatile long/double requirements were handled using locking at the Java library and/or Unsafe level.
10-06-2020
Thank you David. "We used to have a lock-based fallback for 32-bit platforms without the ability to do 64-bit cmpxchg, but that seems to have gone now. " This is my main worry, but it looks like the inlined asm assumes cx8() support is present, for example: // atomic_windows_x86.cpp #else // !AMD64 template<> template<typename T> inline T Atomic::PlatformCmpxchg<8>::operator()(T volatile* dest, T compare_value, T exchange_value, atomic_memory_order order) const { STATIC_ASSERT(8 == sizeof(T)); int32_t ex_lo = (int32_t)exchange_value; int32_t ex_hi = ( ((int32_t)&exchange_value) + 1 ); int32_t cmp_lo = (int32_t)compare_value; int32_t cmp_hi = ( ((int32_t)&compare_value) + 1 ); __asm { push ebx push edi mov eax, cmp_lo mov edx, cmp_hi mov edi, dest mov ebx, ex_lo mov ecx, ex_hi lock cmpxchg8b qword ptr [edi] pop edi pop ebx } } So to summarize for this context: inline JfrVersionSystem::Type JfrVersionSystem::tip() const { return Atomic::load(&_tip._value); <<------------------------------------------- valid 64-bit access, even for 32-bit systems } inline JfrVersionSystem::Type JfrVersionSystem::increment() { if (!VM_Version::supports_cx8()) { <---------------- JfrSpinlockHelper lock(&_spinlock); <------------- // Redundant block to be removed return ++_tip._value; <------------------------------- } <---------------------------------------------------------- traceid cmp; traceid xchg; do { cmp = _tip._value; xchg = cmp + 1; } while (Atomic::cmpxchg(&_tip._value, cmp, xchg) != cmp); // Assumes platforms can do lock cmpxchg8b. If not, the issue is much bigger. return xchg; } Atomic::cmpxchg() and Atomic::load() sufficient for this context, spinlock to be dropped. Thanks for your help.
10-06-2020
It is a bit complex and I still tend to recall the pre-template details, which doesn't always help. Some atomic operations are required on all platforms to support atomic operations on Java volatile long and double, so Atomic::cmpxchg(jlong), Atomic::load(jlong) and Atomic::store(jlong) must all work on 32-bit systems. We used to have a lock-based fallback for 32-bit platforms without the ability to do 64-bit cmpxchg, but that seems to have gone now. So here is the basic situation, if you attempt to use a 64-bit Atomic operation (other than cmpxchg, load, store) on a 32-bit platform that doesn't provide the full range of 64-bit atomic ops then you will get the link failure for the Atomic::PlatformXXX<8u> function that the template tries to use. So to use e.g. Atomic::add your options are: 1. Conditional compilation #ifdef SUPPORTS_NATIVE_CX8 Atomic::add(&_tip._value, (traceid)1); // the platform must still provide the definition of course #else < alternative implementation > #endif 2. Use cmpxchg to implement the missing atomic op If you use #2 then it will work on all platforms and you don't need to combine it with either #1 or with a runtime supports_cx8() check. I think the support for supports_cx8() is broken with the template based definitions as you will still get a linker error if you try to do: if (VM_Version::supports_cx8()) { Atomic::add(&_tip._value, (traceid)1); } else { < alternative implementation > } Bottom line is that for this particular fix you can do away with the spinlock usage and just use the cmpxchg on all platforms.
10-06-2020
It was just this expression that casued a link error: Atomic::add(&_tip._value, (traceid)1); reporting unable to link to Atomic::PlatformAdd<8u>::add_and_fetch: "undefined reference to 'unsigned long long Atomic::PlatformAdd<8u>::add_and_fetch<unsigned long long, unsigned long long>(unsigned long long volatile*, unsigned long long, atomic_memory_order) const'" Reported on Win32 and also on ARM32. I have seen similar things earlier, so i went back to see how we treat atomic accesses for 64-bit traceid's, and there are no uses of Atomic::inc(), Atomic::add() or Atomic::dec() for traceid. If I remember correctly it is just because of similar problems, 32-bit platforms not happy, so all atomic writes instead use Atomic:cmpxchg(). I did not have time to investigate what is the root of these link failures against Atomic::PlatformAdd<8u>::add_and_fetch; the focus was instead to get systems to build again. Regarding VM_Version::supports_cx8(), it is an older pattern, the relevance of which is uncertain, and as you point out, the SUPPORTS_NATIVE_CX8 guard should probably be used instead. I would like not to use it but to rely instead on the Atomic::cmpxchg() and Atomic::load() pair (ideally Atomic::inc() and Atomic::load()). But is this guaranteed to work on 64-bit values on 32-bit platforms? Even when VM_Version::supports_cx8() reports false? You are correct that the spinlock would be needed in the load as well.
09-06-2020
I don't understand why you added an !VM_Version::supports_cx8() code section and also changed the use of Atomic::add ? I would have expected this to suffice: if (!VM_Version::supports_cx8()) { JfrSpinlockHelper lock(&_spinlock); return ++_tip._value; } else { return Atomic::add(&_tip._value, (traceid)1); } Possibly an Ifdef SUPPORTS_NATIVE_CX8 guard would also be needed. And you would only need the spinlock field when SUPPORTS_NATIVE_CX8 is not defined. That said, ARMv7a does set SUPPORTS_NATIVE_CX8 as it has 64-bit load/store with reservation. What was the failing ARM platform? ARMv6?
09-06-2020
That said I don't see how the spinlock solution is complete as you would also need the spinlock around any accessor to ensure no word tearing.
08-06-2020
URL: https://hg.openjdk.java.net/jdk/jdk/rev/06458ef8a5c2 User: mgronlun Date: 2020-06-08 17:50:48 +0000
08-06-2020
Thanks to [~mbaesken] for reporting this.
08-06-2020