Change win32 compat atomic_compare_exchange_strong to match C11 spec
#57
Conversation
* Change `atomic_compare_exchange_strong` to macro, so it may take an atomic first argument and non-atomic second argument, as in the C11 specification * Remove OS-dependent code in `libavcodec/vcc_thread.c`
compat/atomics/win32/stdatomic.h
Outdated
| return *expected == old; | ||
| } | ||
| #define atomic_compare_exchange_strong(object, expected, desired) \ | ||
| (InterlockedCompareExchangePointer((PVOID *) object, (PVOID) desired, *((PVOID *) expected)) == *((PVOID *) expected)) |
There was a problem hiding this comment.
This will overwrite the *object.
int is 32 bits on most platforms. PVOID is 64 bits on 64 bits platform.
this function will write 64 bits data(PVOID) to a 32 bits (int) location.
only vlc can do the right thing:
https://code.videolan.org/videolan/vlc/-/blob/8ef254bbae16f3df8b760c4d89fc518610a2f40a/include/vlc_atomic.h#L331
There was a problem hiding this comment.
👍 This is what I was worried about without using the fixed-size types. I tried implementing using a switch statement but this was a bit funny with the macros. I will try the way VLC is doing it with the telescoping macros.
There was a problem hiding this comment.
dabc841 has gotten rid of the error but I'm still not sure is what we want.
atomic_int is not necessarily (will be on 32 bit) the same size as int when using the win32 atomic compatibility header. At the moment, on a 64-bit system object will be a 64-bit intptr_t so expected and desired will be overread. The spec says each integer type and its corresponding atomic type should have the same size, but I assume there's some reason intptr_t is being used here?
There was a problem hiding this comment.
Looks like the definitions of all atomic types as intptr_t (initially in c91e72e) is simply taken from an old version of VLC: https://code.videolan.org/videolan/vlc/-/commit/fd82bdcba3903bfe377d25c0cbadd960958d37ea
@nuomi2021
Shall I change them all to the corresponding non-atomic types? This is what VLC has now, seems more in line with the C11 specification and will fix the issue we have here.
There was a problem hiding this comment.
Please send your current patch to the mailing list and ask other maintainers for review.
the project will continue to rebase against upstream. Once upstream accepted your patch. we will get it too.
|
let's close this. since you will send patch to upstream. |
As a preface,
vvc_thread.cis the only place in the entire FFmpeg codebase which usesatomic_compare_*so hopefully this isn't too risky a change – it feels like a daunting place to make one. The change tocompat/atomics/win32/stdatomic.hprobably belongs upstream but I thought I'd get it checked here first considering this is the only place where it will matter at the moment.Previously, OS-dependent code was needed to use
atomic_compare_exchange_strong. This was causing issues with cross-compilation. This was necessary because the definition of the function in the win32 atomic compatibility header did not match the C11 specification. The C11 spec describes the function aswhere
Ais an atomic typeCis its non-atomic counterpartIn FFmpeg's win32 atomic compatibility header, all atomic types are
intptr_ts.atomic_compare_exchange_strongwas defined as an inline function with the prototypewhich only matches the C11 specification for
atomic_intptr_t- bothobjectandexpectedare essentiallyA.The new prototype is much more relaxed, it uses a macro to take any pointer
objectandexpectedand anydesired. This is more relaxed than the C11 specification - it makes no assertions thatAis atomic nor thatAandCare counterparts. I figure this is better than the previous prototype though as it doesn't require OS-specific code to use the function every time and any mistakes will still be flagged when compiling with gcc for Linux (which is much more common than Windows/MSVC during development I imagine).Its implementation uses
InterlockedCompareExchangePointer), which works with 64-bit operands on a 64-bit system and 32-bit operands on a 32-bit system. This is not the same as the C11atomic_compare_exchange_strong– this takes arbitrarily-sized arguments. For this reason, the function currently produces a build warning:Should this instead be implemented with a switch statement on the size of the type to the Window's API's sized arguments e.g.
InterlockedCompareExchange64?I believe this PR will supersede #56 and close #37.