On 5/9/20 1:11 PM, Jacek Caban wrote:
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.
Yes, I'm planning on adding a __clang__ version check, as some quick test tells me that __atomic builtins are only supported for clang >= 3.1.
But then, as the fallback isn't very reliable and depends on the guarantees that the target architecture provides, it can either simply be completely unsupported and report a compilation error, or we can try to be a bit clever and have it only for X86 (but that may not be a good idea).