Hi Jonathan, Rahl, On Sat, Mar 01, 2025 at 11:59:35AM +0000, Jonathan Wakely wrote: > On Sat, 1 Mar 2025 at 11:36, rahl <rahl@xxxxxxxxxx> wrote: > > > > On 28 February 2025 20:57:50 UTC, Alejandro Colomar <alx@xxxxxxxxxx> wrote: > > >On Fri, Feb 28, 2025 at 10:13:04AM +0000, rahl wrote: > > >> I noticed that 'const' is used for both 'one' and 'zero' variables in > > >> the Examples section demo code of manpage 'futex(2)'. > > >> > > >> The variables are both used in calls to > > >> 'atomic_compare_exchange_strong()' where the 'const' is discarded as > > >> it may write to the 'expected' parameter during a "failure" case. > > > > > >I don't understand what that function is. It doesn't have a manual > > >page, and it's neither in /usr/include. It doesn't appear in the GCC > > >manual either. And it's not described in ISO C. > > > > > >What is that function? > > > > > >I'd like to understand what we're calling to be able to understand how > > >the calling code is wrong. > > > > atomic_compare_exchange_XXX() are defined in stdatomic.h Ahh, it's a compiler library. Now I can find it. alx@debian:~$ find /usr/lib/ 2>/dev/null | grep stdatomic.h /usr/lib/gcc/x86_64-linux-gnu/14/include/stdatomic.h /usr/lib/llvm-19/lib/clang/19/include/stdatomic.h alx@debian:~$ find /usr/lib/ 2>/dev/null | grep stdatomic.h | xargs grepc -n -B1 atomic_compare_exchange_strong /usr/lib/gcc/x86_64-linux-gnu/14/include/stdatomic.h-178- /usr/lib/gcc/x86_64-linux-gnu/14/include/stdatomic.h:179:#define atomic_compare_exchange_strong(PTR, VAL, DES) \ atomic_compare_exchange_strong_explicit (PTR, VAL, DES, __ATOMIC_SEQ_CST, \ __ATOMIC_SEQ_CST) -- /usr/lib/llvm-19/lib/clang/19/include/stdatomic.h-149- /usr/lib/llvm-19/lib/clang/19/include/stdatomic.h:150:#define atomic_compare_exchange_strong(object, expected, desired) __c11_atomic_compare_exchange_strong(object, expected, desired, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) > > and were introduced in C11 (ISO/IEC 9899:2011 I believe). And indeed were introduced in C11 (I don't remember how I searched the other day that I didn't find it). $ stdc C11 atomic_compare_exchange_strong _Bool atomic_compare_exchange_strong(volatile A *object, C *expected, C desired); > > The main online documentation for these that I'm currently aware of is at cppreference. > > <https://en.cppreference.com/w/c/atomic/atomic_compare_exchange> > > > > >BTW, there seem to be other important bugs in that example program, > > >which I don't understand either. Would you mind having a look at those > > >(I'm assuming that you seem familiar with these atomic APIs)? See: > > > > You're right, there are more bugs. I'm however not so familiar with these functions, but I do have some help. > > > > The errors in question relate to a missing _Atomic qualifier for several variables and function parameters. > > The futex.2 examples used to use the non-standard > __sync_bool_compare_and_swap, which was correct. It was changed to use > atomic_compare_exchange_strong by: > > commit 915c4ba36f9f71db88e7e7913b845d046996f485 > Author: Benjamin Peterson <benjamin@xxxxxxxxxx> > Date: Tue Nov 13 22:53:41 2018 -0800 > > futex.2: Make the example use C11 atomics rather than GCC builtins > > That change was wrong and broke the examples, because C11 atomics are > only usable on _Atomic variables. __sync_bool_compare_and_swap and > __atomic_compare_exchange are usable with plain non-_Atomic types. Thanks! Should we just revert that commit, or do you think we can fix the current program? Do you think the C11 APIs are worth it? Could you please review rahl's patch? > > The documentation linked above should clarify this as well. > > > > It turns out these calls could be replaced with a compiler built-in (__atomic_compare_exchange_n) both for clang and gcc, which wouldn't have the above problem, and would also allow for the removal of 'stdatomic.h'. However, this didn't feel too in keeping with manpage example code. > > > > The gcc docs are here: > > <https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html> > > > > >> Attached is the trivial patch to remove the offending qualifiers. > > > > > >Please provide a complete patch, with a commit message. See the files > > >under <CONTRIBUTING.d/>. > > > > Rather than submit a separate email, I have attached a file > > containing the full output from git-format-patch, Yes, that's what I needed (I don't know why people strip the output of git-format-patch(1); it's easier to just send it as is, I think). :) > > with the message being in what I believe is the expected format Yes. Please add to it the following tags: Fixes: 915c4ba36f9f (2018-11-17; "futex.2: Make the example use C11 atomics rather than GCC builtins") Cc: Jonathan Wakely <jwakely.gcc@xxxxxxxxx> > > - having read that an attachment is also an acceptable approach, > > albeit perhaps not in email form. Yes, it is. What you sent this time is just fine. Do you think I could improve that documentation that would have prevented you from sending the first email with the stripped patch? Do you think anything there is unclear and could be clarified? > > If for some reason you feel it should be submitted as a separate > > email I can do so. > > This way seemed more prudent for now as this is my first such patch > > and I'd like to get it right before having multiple threads in the ML. You could still send as a separate email in the same thread with git-format-patch(1)'s --in-reply-to flag (or git-send-email(1), which has the same flag), which would have been fine, but this is okay too. > > > > Let me know if there are any further issues. > > Cheers, > > > > rahl Have a lovely day! Alex -- <https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
