Re: [PATCH][libatomic] Fix return value in libat_test_and_set

[Tom de Vries <tdevries@suse.de>]

On 3/24/22 10:02, Jakub Jelinek wrote:
> On Thu, Mar 24, 2022 at 09:28:15AM +0100, Tom de Vries via Gcc-patches wrote:
>> Hi,
>>
>> On nvptx (using a Quadro K2000 with driver 470.103.01) I ran into this:
>> ...
>> FAIL: gcc.dg/atomic/stdatomic-flag-2.c -O1 execution test
>> ...
>> which mimimized to:
>> ...
>>    #include <stdatomic.h>
>>    atomic_flag a = ATOMIC_FLAG_INIT;
>>    int main () {
>>      if ((atomic_flag_test_and_set) (&a))
>>        __builtin_abort ();
>>      return 0;
>>    }
>> ...
>>
>> The atomic_flag_test_and_set is implemented using __atomic_test_and_set_1,
>> which corresponds to the "word-sized compare-and-swap loop" version of
>> libat_test_and_set in libatomic/tas_n.c.
>>
>> The semantics of a test-and-set is that the return value is "true if and only
>> if the previous contents were 'set'".
>>
>> But the code uses:
>> ...
>>    return woldval != 0;
>> ...
>> which means it doesn't look only at the byte that was either set or not set,
>> but at the entire word.
>>
>> Fix this by using instead:
>> ...
>>    return (woldval & wval) == wval;
> 
> Shouldn't that be instead
>    return (woldval & ((UWORD) -1 << shift)) != 0;
> or
>    return (woldval & ((UWORD) ~(UWORD) 0 << shift)) != 0;
> ?

Well, I used '(woldval & wval) == wval' based on the fact that the set 
operation uses a bitor:
...
   wval = (UWORD)__GCC_ATOMIC_TEST_AND_SET_TRUEVAL << shift;
   woldval = __atomic_load_n (wptr, __ATOMIC_RELAXED);
   do
     {
       t = woldval | wval;
...
so apparently we do not care here about bits not in 
__GCC_ATOMIC_TEST_AND_SET_TRUEVAL (or alternatively, we care but assume 
that they're 0).

AFAIU, it would have been more precise to compare the entire byte with 
__GCC_ATOMIC_TEST_AND_SET_TRUEVAL, but then it would have made sense to 
set the entire byte in the set part as well.

Anyway, that doesn't seem to be what you're proposing.  During 
investigation of the failure I found that the address used is 
word-aligned, so shift becomes 0 in that case.  AFAICT, the fix you're 
proposing is a nop for shift == 0, and indeed, it doesn't fix the 
failure I'm observing.

> The exact __GCC_ATOMIC_TEST_AND_SET_TRUEVAL varies (the most usual
> value is 1, but sparc uses 0xff and m68k/sh use 0x80), falseval is
> always 0 though and (woldval & wval) == wval
> is testing whether some bits of the oldval are all set rather than
> whether the old byte was 0.
> Say for trueval 1 it tests whether the least significant bit is set,
> for 0x80 if the most significant bit of the byte is set, for
> 0xff whether all bits are set.

Yes, I noticed that.

AFAIU, the proposed patch ddrt under the assumption that we don't care 
about bits not set in __GCC_ATOMIC_TEST_AND_SET_TRUEVAL.

If that's not acceptable, I can submit a patch that doesn't have that 
assumption, and tests the entire byte (but should I also fix the set 
operation then?).

Thanks,
- Tom