[FFmpeg-devel] [PATCH] compat/atomics/win32: improve similarity to stdatomic.h

[Frank Plowman]

> On 9 Apr 2023, at 09:59, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 
> Le mercredi 5 avril 2023, 18:26:29 EEST Frank Plowman a écrit :
>> Some preliminary info:
>> * The win32 atomic compatibility header is based on VLC's (c91e72ed52). This
>> patch makes the header more in line with what it was before they got rid of
>> this way of doing things:
>> https://code.videolan.org/videolan/vlc/-/blob/ce150f3849cebe97bc7fc028674d3
>> c7f8c73f64d/include/vlc_atomic.h
>> * The Windows API does not support atomic
>> operations on 8-bit types and only has functions for atomic operations on
>> 16-bit types on Windows Desktop.
> 
> FWIW, Windows atomic wait/notify system call functions support 8- and 16-bit 
> types, so I would expect that there is a mean to perform atomic load/store/
> exchange/compare-exchange too.

The functions for 16-bit atomic operations do exist and have been supported by Windows Desktop and Server versions since around 2003, however support for non-Desktop/Server platforms seems to only have been introduced with UWP in 2019. I have since found there are also 8-bit interlocked variable access functions (the links to them in the documentation were missing). These were only added in Windows Desktop 8 and Windows Server 2012 however. These could be used, but at the cost of dropping support for some old/obscure systems.

> 
>> * Makes atomic types the same size as their non-atomic counterparts.
>> Previously, all atomic types were intptr_t to get around the lack of 8- and
>> 16-bit atomic operations. This could lead to overreads. Now, each atomic
>> type is the same size as its non-atomic counterpart, in line with the C11
>> specification.
> 
> There are no requirements in C11 that atomic variables have the same size as 
> their non-atomic-qualified equivalent. It is not clear what you mean here.

While not a requirement, the specification does say atomic types and their corresponding regular types "should have the same size whenever possible”. This is an alternative way I see we could support 8- and 16-bit atomic types though.

> I also don't know what you mean by overread. If you don't want values to 
> overflow their specified boundaries, the "correct" approach is to mask the 
> excess bits (like how compilers implement small atomics on RISC-V).

I think I was a bit confused writing this to be honest. The functions where this was a potential problem are atomic_compare_exchange_* as they take a pointer to a non-atomic type. This point was nullified however as atomic_compare_exchange_strong was defined as a function rather than a macro. The patch allows atomic_compare_exchange_strong to be defined as a macro taking various sized types, making it easier to use. Previously, the following code

atomic_int atomic = 0;
int regular = 0;
atomic_compare_exchange_strong(&atomic, &regular, 0);

would produce a compilation error when compiled with 64-bit MSVC (where ints are 32 bits). The snippet above is how I would expect most developers to try to use the function - having to define regular as an intptr_t seems unintuitive and other typical implementations of stdatomic.h require an int.

Thanks for your feedback. I will produce a second version of the patch including support for 8- and 16-bit atomic types. I see two ways of doing this:
1. Defining the 8- and 16-bit atomic types as 32-bit types and implementing their operations using the 32-bit interlocked variable access functions.
2. Defining the 8- and 16-bit atomic types as 8- and 16-bit types respectively, and implementing their operations using the 8- and 16-bit interlocked variable access functions.
I lean towards 1. as 2. would require dropping support for Windows Desktop 7/Windows Server 2008 and earlier. The disadvantage of approach 1. is that atomic_compare_exchange_* won't work for 8- and 16-bit types.