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