public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits
@ 2023-08-19 19:06 comexk at gmail dot com
2023-08-19 19:36 ` [Bug c++/111077] " pinskia at gcc dot gnu.org
` (8 more replies)
0 siblings, 9 replies; 10+ messages in thread
From: comexk at gmail dot com @ 2023-08-19 19:06 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
Bug ID: 111077
Summary: atomic_ref compare_exchange_strong doesn't properly
ignore padding bits
Product: gcc
Version: unknown
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: libstdc++
Assignee: unassigned at gcc dot gnu.org
Reporter: comexk at gmail dot com
Target Milestone: ---
C++20 requires that the functions `std::atomic<T>::compare_exchange_strong` and
`std::atomic_ref<T>::compare_exchange_strong` ignore any padding bits that
exist in T. libstdc++ implements this by relying on the invariant that
`std::atomic<T>` values in memory always have zeroed padding bits. All methods
of `std::atomic<T>` that store values to memory (including the constructor)
zero the padding bits first. Then, `std::atomic<T>::compare_exchange_strong`
zeroes the padding bits of the `expected` value, allowing it to assume that the
native atomic compare-exchange won't fail due to padding bits.
However, this doesn't work correctly for `std::atomic_ref<T>`. All methods of
`std::atomic_ref<T>` that store to memory do zero the padding bits. But what
about the initial value of the object, before the first atomic_ref pointing to
it was created? That value could be anything.
As a result, this code (compiled with -std=c++20) incorrectly prints 0, even
though the only difference between `value` and `expected` is in a padding byte:
#include <atomic>
#include <stdio.h>
int main() {
struct HasPadding { char a; short b; };
HasPadding value = {};
((char *)&value)[1] = 0x42; // update padding byte
HasPadding expected = {};
printf("%d\n",
std::atomic_ref(value).compare_exchange_strong(
expected, expected));
}
This could be fixed by reimplementing
`std::atomic_ref<T>::compare_exchange_strong` to use a loop, as in this
pseudocode:
bool compare_exchange_strong(std::atomic_ref<T> ref, T& expected, T
desired) {
T orig = expected;
while (1) {
if (ref.compare_exchange_weak_including_padding(expected, desired))
{
return true;
}
if (!equal_ignoring_padding(orig, expected)) {
return false;
}
}
}
[See also:
https://github.com/rust-lang/unsafe-code-guidelines/issues/449#issuecomment-1677985851,
which has a comparison of different compilers.]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
@ 2023-08-19 19:36 ` pinskia at gcc dot gnu.org
2023-08-19 20:07 ` pinskia at gcc dot gnu.org
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-08-19 19:36 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Component|libstdc++ |c++
--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The calls for clearing the padding are there before eiline:
```
std::__atomic_impl::__clear_padding<main()::HasPadding> (__exp_28);
_4 = (int) __is_weak_30(D);
_5 = std::__atomic_impl::__clear_padding<main()::HasPadding> (__i_31(D));
_6 = VIEW_CONVERT_EXPR<unsigned int>(*_5);
_7 = std::__addressof<main()::HasPadding> (__val_33(D));
retval.11_37 = __atomic_compare_exchange_4 (_7, __exp_28, _6, _4, __s_35(D),
__f_16(D));
```
Oh we are not clearing the padding of the reference ...
Is this even well defined?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
2023-08-19 19:36 ` [Bug c++/111077] " pinskia at gcc dot gnu.org
@ 2023-08-19 20:07 ` pinskia at gcc dot gnu.org
2023-08-19 20:11 ` [Bug libstdc++/111077] " pinskia at gcc dot gnu.org
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-08-19 20:07 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://lists.llvm.org/pipermail/cfe-dev/2018-December/060537.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
2023-08-19 19:36 ` [Bug c++/111077] " pinskia at gcc dot gnu.org
2023-08-19 20:07 ` pinskia at gcc dot gnu.org
@ 2023-08-19 20:11 ` pinskia at gcc dot gnu.org
2023-08-19 20:14 ` pinskia at gcc dot gnu.org
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-08-19 20:11 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Component|c++ |libstdc++
--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://lists.llvm.org/pipermail/cfe-dev/2018-December/060605.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
` (2 preceding siblings ...)
2023-08-19 20:11 ` [Bug libstdc++/111077] " pinskia at gcc dot gnu.org
@ 2023-08-19 20:14 ` pinskia at gcc dot gnu.org
2023-08-19 20:19 ` pinskia at gcc dot gnu.org
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-08-19 20:14 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
reading through all of these discussions almost want to say the paper on
atomic_ref and padding bits of compare_and_exchange still missed the point of
this issue. Maybe this is undefined and maybe this is defect in the library
part of C++ standard even ...
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
` (3 preceding siblings ...)
2023-08-19 20:14 ` pinskia at gcc dot gnu.org
@ 2023-08-19 20:19 ` pinskia at gcc dot gnu.org
2023-08-20 7:16 ` redi at gcc dot gnu.org
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-08-19 20:19 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
See
https://gcc.gnu.org/pipermail/libstdc++/2022-October/054899.html
Also
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
` (4 preceding siblings ...)
2023-08-19 20:19 ` pinskia at gcc dot gnu.org
@ 2023-08-20 7:16 ` redi at gcc dot gnu.org
2023-08-20 8:35 ` redi at gcc dot gnu.org
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: redi at gcc dot gnu.org @ 2023-08-20 7:16 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #5)
> See
> https://gcc.gnu.org/pipermail/libstdc++/2022-October/054899.html
>
> Also
This is just a buggy test.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
` (5 preceding siblings ...)
2023-08-20 7:16 ` redi at gcc dot gnu.org
@ 2023-08-20 8:35 ` redi at gcc dot gnu.org
2023-09-01 15:02 ` cvs-commit at gcc dot gnu.org
2024-05-21 9:16 ` jakub at gcc dot gnu.org
8 siblings, 0 replies; 10+ messages in thread
From: redi at gcc dot gnu.org @ 2023-08-20 8:35 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
Jonathan Wakely <redi at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Assignee|unassigned at gcc dot gnu.org |redi at gcc dot gnu.org
Last reconfirmed| |2023-08-20
Ever confirmed|0 |1
Target Milestone|--- |13.3
Status|UNCONFIRMED |ASSIGNED
--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to comexk from comment #0)
> [See also:
> https://github.com/rust-lang/unsafe-code-guidelines/issues/449#issuecomment-
> 1677985851, which has a comparison of different compilers.]
"GCC does nothing to handle this" is incorrect. Only the case of an initial
value with non-zero padding is not handled, because atomic_ref doesn't control
the variable's initial value. atomic_ref::store and atomic_ref::exchange clear
padding bits, and so does a successful compare_exchange_{weak,strong}.
But we do have a bug here, because if that initial value has non-zero padding
bits even a compare_exchange_strong loop won't eventually settle, because we
keep zeroing the padding in the 'expected' value, which means it never matches.
We return the current value (with padding bits preserved), but then the next
call using that exact value will get the padding zeroed again, and fail to
match.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
` (6 preceding siblings ...)
2023-08-20 8:35 ` redi at gcc dot gnu.org
@ 2023-09-01 15:02 ` cvs-commit at gcc dot gnu.org
2024-05-21 9:16 ` jakub at gcc dot gnu.org
8 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-09-01 15:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:
https://gcc.gnu.org/g:dcbec954fcba42d97760c6bd98a4c5618473ec93
commit r14-3625-gdcbec954fcba42d97760c6bd98a4c5618473ec93
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Aug 23 12:23:37 2023 +0100
libstdc++: Use a loop in atomic_ref::compare_exchange_strong [PR111077]
We need to use a loop in std::atomic_ref::compare_exchange_strong in
order to properly implement the C++20 requirement that padding bits do
not participate when checking the value for equality. The variable being
modified by a std::atomic_ref might have an initial value with non-zero
padding bits, so when the __atomic_compare_exchange built-in returns
false we need to check whether that was only because of non-equal
padding bits that are not part of the value representation. If the value
bits differ, it's just a failed compare-exchange. If the value bits are
the same, we need to retry the __atomic_compare_exchange using the value
that was just read by the previous failed call. As noted in the
comments, it's possible for that second try to also fail due to another
thread storing the same value but with differences in padding.
Because it's undefined to access a variable directly while it's held by
a std::atomic_ref, and because std::atomic_ref will only ever store
values with zeroed padding, we know that padding bits will never go from
zero to non-zero during the lifetime of a std::atomic_ref. They can only
go from an initial non-zero state to zero. This means the loop will
terminate, rather than looping indefinitely as padding bits flicker on
and off. In theory users could call __atomic_store etc. directly and
write a value with non-zero padding bits, but we don't need to support
that. Users doing that should ensure they do not write non-zero padding,
to be compatibile with our std::atomic_ref's invariants.
This isn't a problem for std::atomic<T>::compare_exchange_strong because
the initial value (and all later stores to the variable) are performed
by the library, so we ensure that stored values always have padding bits
cleared. That means we can simply clear the padding bits of the
'expected' value and we will be comparing two values with equal padding
bits. This means we don't need the loop for std::atomic, so update the
__atomic_impl::__compare_exchange function to take a bool parameter that
says whether it's being used by std::atomic_ref. If not, we can use a
simpler, non-looping implementation.
libstdc++-v3/ChangeLog:
PR libstdc++/111077
* include/bits/atomic_base.h (__atomic_impl::__compare_exchange):
Add _AtomicRef non-type template parameter and use a loop if it
is true.
(__atomic_impl::compare_exchange_weak): Add _AtomicRef NTTP.
(__atomic_impl::compare_exchange_strong): Likewise.
(atomic_ref::compare_exchange_weak): Use true for NTTP.
(atomic_ref::compare_exchange_strong): Use true for NTTP.
* testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc:
Fix test to not rely on atomic_ref::load() to return an object
with padding preserved.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug libstdc++/111077] atomic_ref compare_exchange_strong doesn't properly ignore padding bits
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
` (7 preceding siblings ...)
2023-09-01 15:02 ` cvs-commit at gcc dot gnu.org
@ 2024-05-21 9:16 ` jakub at gcc dot gnu.org
8 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2024-05-21 9:16 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111077
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|13.3 |13.4
--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 13.3 is being released, retargeting bugs to GCC 13.4.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-21 9:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-19 19:06 [Bug libstdc++/111077] New: atomic_ref compare_exchange_strong doesn't properly ignore padding bits comexk at gmail dot com
2023-08-19 19:36 ` [Bug c++/111077] " pinskia at gcc dot gnu.org
2023-08-19 20:07 ` pinskia at gcc dot gnu.org
2023-08-19 20:11 ` [Bug libstdc++/111077] " pinskia at gcc dot gnu.org
2023-08-19 20:14 ` pinskia at gcc dot gnu.org
2023-08-19 20:19 ` pinskia at gcc dot gnu.org
2023-08-20 7:16 ` redi at gcc dot gnu.org
2023-08-20 8:35 ` redi at gcc dot gnu.org
2023-09-01 15:02 ` cvs-commit at gcc dot gnu.org
2024-05-21 9:16 ` jakub at gcc dot gnu.org
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).