<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">On 09.05.2020 11:22, Rémi Bernon wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3fc89c13-a5a2-7360-e1aa-793daa57bc33@codeweavers.com">On
5/9/20 1:18 AM, Jacek Caban wrote:
<br>
<blockquote type="cite">On 08.05.2020 22:12, Alexandre Julliard
wrote:
<br>
<blockquote type="cite">Rémi Bernon
<a class="moz-txt-link-rfc2396E" href="mailto:rbernon@codeweavers.com"><rbernon@codeweavers.com></a> writes:
<br>
<br>
<blockquote type="cite">On 5/7/20 10:45 PM, Alexandre Julliard
wrote:
<br>
<blockquote type="cite">Rémi Bernon
<a class="moz-txt-link-rfc2396E" href="mailto:rbernon@codeweavers.com"><rbernon@codeweavers.com></a> writes:
<br>
<br>
<blockquote type="cite">+#if defined(__GNUC__) &&
((__GNUC__ > 4) || ((__GNUC__ == 4) &&
(__GNUC_MINOR__ >= 7)))
<br>
+ __atomic_store_n(&ptr->SystemTime.High2Time,
system_time_high, __ATOMIC_SEQ_CST);
<br>
+ __atomic_store_n(&ptr->SystemTime.LowPart,
system_time_low, __ATOMIC_SEQ_CST);
<br>
+ __atomic_store_n(&ptr->SystemTime.High1Time,
system_time_high, __ATOMIC_SEQ_CST);
<br>
+
<br>
+
__atomic_store_n(&ptr->InterruptTime.High2Time,
interrupt_time_high, __ATOMIC_SEQ_CST);
<br>
+
__atomic_store_n(&ptr->InterruptTime.LowPart,
interrupt_time_low, __ATOMIC_SEQ_CST);
<br>
+
__atomic_store_n(&ptr->InterruptTime.High1Time,
interrupt_time_high, __ATOMIC_SEQ_CST);
<br>
+
<br>
+ __atomic_store_n(&ptr->TickCount.High2Time,
tick_count_high, __ATOMIC_SEQ_CST);
<br>
+ __atomic_store_n(&ptr->TickCount.LowPart,
tick_count_low, __ATOMIC_SEQ_CST);
<br>
+ __atomic_store_n(&ptr->TickCount.High1Time,
tick_count_high, __ATOMIC_SEQ_CST);
<br>
+
__atomic_store_n(&ptr->TickCountLowDeprecated,
tick_count_low, __ATOMIC_SEQ_CST);
<br>
</blockquote>
Is this gcc-specific code really necessary?
<br>
<br>
</blockquote>
I'm not sure it is strictly necessary but then it depends
what
<br>
guarantees we want to have. The volatile qualifier only acts
as a
<br>
compiler barrier and will make sure that the stores aren't
optimized
<br>
or reordered by the compiler. However it doesn't enforce
anything on
<br>
the CPU. I believe that X86 has global store ordering
guarantees, but
<br>
a quick search tells me that ARM or POWER don't, so the
stores to the
<br>
various members may be seen out of order from another CPU
depending on
<br>
the architecture.
<br>
</blockquote>
My concern is that if the code is necessary, it means that the
fallback
<br>
path for older gccs would yield a broken implementation. But
if the
<br>
fallback works correctly on x86 that's probably good enough.
<br>
</blockquote>
<br>
<br>
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).
<br>
<br>
<br>
Jacek
<br>
<br>
</blockquote>
<br>
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?
<br>
<br>
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.
<br>
<br>
Or, do the complete portable implementation with the asm variants
for all the architectures we need.</blockquote>
<p><br>
</p>
<p>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<span style="color: #000000;"> __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.<br>
</span></p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:3fc89c13-a5a2-7360-e1aa-793daa57bc33@codeweavers.com">What
compilers / architectures are we willing for wineserver?
<br>
</blockquote>
<br>
<p>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.</p>
<p><br>
</p>
<p>Jacek<br>
</p>
</body>
</html>