[PATCH v5 2/3] server: Add USD support with timestamp updates.

[Jacek Caban]

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 at codeweavers.com> writes:
>>>
>>>> On 5/7/20 10:45 PM, Alexandre Julliard wrote:
>>>>> Rémi Bernon <rbernon at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200509/b99f19b7/attachment.htm>