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

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/ce150f3849cebe97bc7fc028674d3c7f8c73f64d/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.
* There is nowhere in the FFmpeg codebase which currently uses 8- or 16-bit atomic types

This patch:
* 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.
* Dispatches Windows API functions based on operand size to avoid overwrites. Now there are a variety of sizes of atomic types, the correct Windows API function must be selected based on the object's sizes. Feedback is appreciated on how best to handle the failure case in the atomic_helper macro.
* Removes 8- and 16-bit types not supported by all versions of winnt.h. None of these were being used anywhere in FFmpeg. As these cannot be operated on atomically, they have been removed. Alternatives to this recommended.

(PS: This is my first submitted patch, please be nice although I'm sure you will)

Signed-off-by: Frank Plowman <post@frankplowman.com>

---
 compat/atomics/win32/stdatomic.h | 104 +++++++++++--------------------
 1 file changed, 36 insertions(+), 68 deletions(-)

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.

> * 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.

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).

> On 9 Apr 2023, at 09:59, Rémi Denis-Courmont <remi@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.