On 09.05.2020 11:22, Rémi Bernon wrote:
On 5/9/20 1:18 AM, Jacek Caban wrote:
On 08.05.2020 22:12, Alexandre Julliard wrote:
Rémi Bernon rbernon@codeweavers.com writes:
On 5/7/20 10:45 PM, Alexandre Julliard wrote:
Rémi Bernon rbernon@codeweavers.com writes:
+#if defined(__GNUC__) && ((__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 7))) + __atomic_store_n(&ptr->SystemTime.High2Time, system_time_high, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->SystemTime.LowPart, system_time_low, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->SystemTime.High1Time, system_time_high, __ATOMIC_SEQ_CST);
- __atomic_store_n(&ptr->InterruptTime.High2Time,
interrupt_time_high, __ATOMIC_SEQ_CST);
- __atomic_store_n(&ptr->InterruptTime.LowPart,
interrupt_time_low, __ATOMIC_SEQ_CST);
- __atomic_store_n(&ptr->InterruptTime.High1Time,
interrupt_time_high, __ATOMIC_SEQ_CST);
+ __atomic_store_n(&ptr->TickCount.High2Time, tick_count_high, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->TickCount.LowPart, tick_count_low, __ATOMIC_SEQ_CST); + __atomic_store_n(&ptr->TickCount.High1Time, tick_count_high, __ATOMIC_SEQ_CST);
- __atomic_store_n(&ptr->TickCountLowDeprecated, tick_count_low,
__ATOMIC_SEQ_CST);
Is this gcc-specific code really necessary?
I'm not sure it is strictly necessary but then it depends what guarantees we want to have. The volatile qualifier only acts as a compiler barrier and will make sure that the stores aren't optimized or reordered by the compiler. However it doesn't enforce anything on the CPU. I believe that X86 has global store ordering guarantees, but a quick search tells me that ARM or POWER don't, so the stores to the various members may be seen out of order from another CPU depending on the architecture.
My concern is that if the code is necessary, it means that the fallback path for older gccs would yield a broken implementation. But if the fallback works correctly on x86 that's probably good enough.
The check should probably be adjusted for clang through. It pretends to be GCC 4.2 and supports __atomic_store_n (and is a required compiler on aarch64).
Jacek
So would it be acceptable to extend the check for clang and simply error in the case where __atomic builtins aren't supported? Or should we keep the fallback but check that it's on X86 only?
There's also a possible intermediate but heavier fallback, usable on clang 3.0 or gcc >= 4.1, with __sync_synchronize, but still GCC-like specific again.
Or, do the complete portable implementation with the asm variants for all the architectures we need.
The implementation and use of __atomic_store_n is fine, it's just values of __GNUC__ and __GNUC_MINOR__ that we can't trust on clang, so it's only #if that could be improved. In practice, you could probably just add "|| defined(__clang__)" there. There is also__has_builtin(__atomic_store_n) that could be used, but I'm not sure it's worth it. We could also avoid dealing with hardcoded compiler version and use a configure check.
What compilers / architectures are we willing for wineserver?
It should be generally portable (although GCC and Clang are primary targets in practice). In this case, I noticed it because you mentioned ARMs and on aarch64 Clang is currently the only supported compiler because of __builtin_ms_va_list.
Jacek