On Thu, Mar 24, 2022 at 11:01:30AM +0100, Tom de Vries wrote:
> > 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.
Ah, sorry, I certainly meant
return (woldval & ((UTYPE) -1 << shift)) != 0;
or
return (woldval & ((UTYPE) ~(UTYPE) 0 << shift)) != 0;
i.e. more portable ways of
return (woldval & (0xff << shift)) != 0;
which don't hardcode that UTYPE is 8-bit unsigned char.
If one uses just __atomic_test_and_set and __atomic_clear, then I think
it makes no difference.
But testing whether the old byte was non-zero more matches the previous
intent in case the previous value is neither 0 nor
__GCC_ATOMIC_TEST_AND_SET_TRUEVAL
and treats it as "set" as well.
I think we don't need to change the loop, woldval | wval even for woldval
byte containing say 42 the or will make it still non-zero.
The documentation argues against using those atomics on types other than
bool and {,{un,}signed }char but libatomic still supports those, I believe
when one doesn't have hw specific support for these, __atomic_clear will
clear the entire UTYPE.
Jakub
