Message ID | 20210226155937.621324-1-thiago.macieira@intel.com |
---|---|
State | New |
Headers | show |
Series | [1/5] std::latch: reduce internal implementation from ptrdiff_t to int | expand |
On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > > private: > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > + alignas(__alignof__(int)) int _M_a; Futexes must be aligned to 4 bytes. Andreas.
On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > > > private: > > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > + alignas(__alignof__(int)) int _M_a; > > Futexes must be aligned to 4 bytes. Agreed, but doesn't this accomplish that?
On Feb 26 2021, Thiago Macieira wrote: > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: >> > @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> > } >> > >> > private: >> > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; >> > + alignas(__alignof__(int)) int _M_a; >> >> Futexes must be aligned to 4 bytes. > > Agreed, but doesn't this accomplish that? No. It uses whatever alignment the type already has, and is an elaborate no-op. Andreas.
On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > On Feb 26 2021, Thiago Macieira wrote: > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > >> > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > >> > + alignas(__alignof__(int)) int _M_a; > >> > >> Futexes must be aligned to 4 bytes. > > > > Agreed, but doesn't this accomplish that? > > No. It uses whatever alignment the type already has, and is an > elaborate no-op. I thought so too when I read the original line. But I expected it was written like that for a reason, especially since the same pattern appears in other places. I can change to "alignas(4)" (which is a GCC extension, I believe). Is that the correct solution?
On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: > On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > > On Feb 26 2021, Thiago Macieira wrote: > > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > >> > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > >> > + alignas(__alignof__(int)) int _M_a; > > >> > > >> Futexes must be aligned to 4 bytes. > > > > > > Agreed, but doesn't this accomplish that? > > > > No. It uses whatever alignment the type already has, and is an > > elaborate no-op. > > I thought so too when I read the original line. But I expected it was written > like that for a reason, especially since the same pattern appears in other > places. > > I can change to "alignas(4)" (which is a GCC extension, I believe). Is that > the correct solution? IMNSHO make use of the corresponding atomic type. Then there'd be no need for separate what's-the-right-align-curse games. brgds, H-P
On Sun, Feb 28, 2021 at 10:53 PM Hans-Peter Nilsson <hp@bitrange.com> wrote: > > > > On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: > > > On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > > > On Feb 26 2021, Thiago Macieira wrote: > > > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > > > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > > >> > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > > >> > + alignas(__alignof__(int)) int _M_a; > > > >> > > > >> Futexes must be aligned to 4 bytes. > > > > > > > > Agreed, but doesn't this accomplish that? > > > > > > No. It uses whatever alignment the type already has, and is an > > > elaborate no-op. > > > > I thought so too when I read the original line. But I expected it was written > > like that for a reason, especially since the same pattern appears in other > > places. > > > > I can change to "alignas(4)" (which is a GCC extension, I believe). Is that > > the correct solution? > > IMNSHO make use of the corresponding atomic type. Then there'd > be no need for separate what's-the-right-align-curse games. Or align as the corresponding atomic type (in case using an actual std::atomic<int> is undesirable). OTOH the proposed code isn't any more bogus than the previous ... Richard. > brgds, H-P
On 2021-02-28 13:31, Hans-Peter Nilsson wrote: >> On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: >> >> On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: On Feb >> 26 2021, Thiago Macieira wrote: On Friday, 26 February 2021 10:14:42 >> PST Andreas Schwab wrote: On Feb 26 2021, Thiago Macieira via >> Gcc-patches wrote: - alignas(__alignof__(ptrdiff_t)) ptrdiff_t >> _M_a; >> + alignas(__alignof__(int)) int _M_a; >> Futexes must be aligned to 4 bytes. >> >> Agreed, but doesn't this accomplish that? > No. It uses whatever alignment the type already has, and is an > elaborate no-op. > I thought so too when I read the original line. But I expected it was > written > like that for a reason, especially since the same pattern appears in > other > places. > I can change to "alignas(4)" (which is a GCC extension, I believe). Is > that > the correct solution? > IMNSHO make use of the corresponding atomic type. Then there'd > be no need for separate what's-the-right-align-curse games. There is no predicate wait on atomic<T>. > brgds, H-P
On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote: >ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't. Yes, we should do this for GCC 11. > libstdc++-v3/include/std/latch | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/libstdc++-v3/include/std/latch b/libstdc++-v3/include/std/latch >index ef8c301e5e9..156aea5c5e5 100644 >--- a/libstdc++-v3/include/std/latch >+++ b/libstdc++-v3/include/std/latch >@@ -48,7 +48,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > public: > static constexpr ptrdiff_t > max() noexcept >- { return __gnu_cxx::__int_traits<ptrdiff_t>::__max; } >+ { return __gnu_cxx::__int_traits<int>::__max; } > > constexpr explicit latch(ptrdiff_t __expected) noexcept > : _M_a(__expected) { } >@@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > > private: >- alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; >+ alignas(__alignof__(int)) int _M_a; > }; > _GLIBCXX_END_NAMESPACE_VERSION > } // namespace >-- >2.30.1 >
On 01/03/21 09:56 +0100, Richard Biener via Libstdc++ wrote: >On Sun, Feb 28, 2021 at 10:53 PM Hans-Peter Nilsson <hp@bitrange.com> wrote: >> >> >> >> On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: >> >> > On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: >> > > On Feb 26 2021, Thiago Macieira wrote: >> > > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: >> > > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: >> > > >> > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; >> > > >> > + alignas(__alignof__(int)) int _M_a; >> > > >> >> > > >> Futexes must be aligned to 4 bytes. >> > > > >> > > > Agreed, but doesn't this accomplish that? >> > > >> > > No. It uses whatever alignment the type already has, and is an >> > > elaborate no-op. >> > >> > I thought so too when I read the original line. But I expected it was written >> > like that for a reason, especially since the same pattern appears in other >> > places. >> > >> > I can change to "alignas(4)" (which is a GCC extension, I believe). Is that >> > the correct solution? >> >> IMNSHO make use of the corresponding atomic type. Then there'd >> be no need for separate what's-the-right-align-curse games. That won't work though, because we need direct access to the integer object, not to a std::atomic<int> which contains it. >Or align as the corresponding atomic type (in case using an actual >std::atomic<int> is undesirable). OTOH the proposed code isn't >any more bogus than the previous ... The previous code accounts for the fact that ptrdiff_t is a typedef for an unspecified type, and that some ABIs allow struct members to have weaker alignment than they would have otherwise. e.g. __alignof__(long long) != alignof(long long) on x86. Yes, I know ptrdiff_t isn't long long on x86, but since it's a typedef for some target-specific type, it's still possible that alignof != __alignof__. So alignas(__alignof__(T)) is not necessarily a no-op. So not bogus. For int, there shouldn't be any need to force the alignment. I don't think any ABI supported by GCC allows int members to be aligned to less than __alignof__(int). Users could break it by compiling with #pragma pack, but I have no sympathy for such silliness.
On Mär 03 2021, Jonathan Wakely wrote: > For int, there shouldn't be any need to force the alignment. I don't > think any ABI supported by GCC allows int members to be aligned to > less than __alignof__(int). There is no requirement that __alignof__(int) is big enough. Andreas.
On 03/03/21 14:56 +0000, Jonathan Wakely wrote: >On 01/03/21 09:56 +0100, Richard Biener via Libstdc++ wrote: >>On Sun, Feb 28, 2021 at 10:53 PM Hans-Peter Nilsson <hp@bitrange.com> wrote: >>> >>> >>> >>>On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: >>> >>>> On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: >>>> > On Feb 26 2021, Thiago Macieira wrote: >>>> > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: >>>> > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: >>>> > >> > - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; >>>> > >> > + alignas(__alignof__(int)) int _M_a; >>>> > >> >>>> > >> Futexes must be aligned to 4 bytes. >>>> > > >>>> > > Agreed, but doesn't this accomplish that? >>>> > >>>> > No. It uses whatever alignment the type already has, and is an >>>> > elaborate no-op. >>>> >>>> I thought so too when I read the original line. But I expected it was written >>>> like that for a reason, especially since the same pattern appears in other >>>> places. >>>> >>>> I can change to "alignas(4)" (which is a GCC extension, I believe). Is that >>>> the correct solution? It's not a GCC extensions. You're thinking of alignas(obj) which is a GCC extension. >>>IMNSHO make use of the corresponding atomic type. Then there'd >>>be no need for separate what's-the-right-align-curse games. > >That won't work though, because we need direct access to the integer >object, not to a std::atomic<int> which contains it. > >>Or align as the corresponding atomic type (in case using an actual >>std::atomic<int> is undesirable). OTOH the proposed code isn't >>any more bogus than the previous ... > >The previous code accounts for the fact that ptrdiff_t is a typedef >for an unspecified type, and that some ABIs allow struct members to have >weaker alignment than they would have otherwise. > >e.g. __alignof__(long long) != alignof(long long) on x86. > >Yes, I know ptrdiff_t isn't long long on x86, but since it's a typedef >for some target-specific type, it's still possible that >alignof != __alignof__. So alignas(__alignof__(T)) is not necessarily >a no-op. So not bogus. > >For int, there shouldn't be any need to force the alignment. I don't >think any ABI supported by GCC allows int members to be aligned to >less than __alignof__(int). Users could break it by compiling with >#pragma pack, but I have no sympathy for such silliness. Jakub said on IRC that m68k might have alignof(int) == 2, so we'd need to increase that alignment to use it as a futex. N.B. std::atomic and std::atomic_ref don't use alignas(__alignof__(T)) they do this instead: static_assert(is_trivially_copyable_v<_Tp>); // 1/2/4/8/16-byte types must be aligned to at least their size. static constexpr int _S_min_alignment = (sizeof(_Tp) & (sizeof(_Tp) - 1)) || sizeof(_Tp) > 16 ? 0 : sizeof(_Tp); public: static constexpr size_t required_alignment = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp); Using std::atomic_ref<T>::required_alignment would give that value. For something being used as a futex we should just use alignas(4) though, since that's what the kernel requires.
On Wed, 3 Mar 2021, Jonathan Wakely wrote: > For int, there shouldn't be any need to force the alignment. I don't > think any ABI supported by GCC allows int members to be aligned to > less than __alignof__(int). (sizeof(int) last) The CRIS ABI does as in default packed, and ISTR there was some other gcc port too, but I don't recall whether that (too) had no (current) Linux port. brgds, H-P
On Wednesday, 3 March 2021 06:34:26 PST Jonathan Wakely wrote: > On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote: > >ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't. > > Yes, we should do this for GCC 11. Want me to re-submit this one alone, with the "alignas(4)" with a commend indicating it's what the kernel requires?
On 03/03/21 09:14 -0800, Thiago Macieira via Libstdc++ wrote: >On Wednesday, 3 March 2021 06:34:26 PST Jonathan Wakely wrote: >> On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote: >> >ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't. >> >> Yes, we should do this for GCC 11. > >Want me to re-submit this one alone, with the "alignas(4)" with a commend >indicating it's what the kernel requires? Yes please.
diff --git a/libstdc++-v3/include/std/latch b/libstdc++-v3/include/std/latch index ef8c301e5e9..156aea5c5e5 100644 --- a/libstdc++-v3/include/std/latch +++ b/libstdc++-v3/include/std/latch @@ -48,7 +48,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION public: static constexpr ptrdiff_t max() noexcept - { return __gnu_cxx::__int_traits<ptrdiff_t>::__max; } + { return __gnu_cxx::__int_traits<int>::__max; } constexpr explicit latch(ptrdiff_t __expected) noexcept : _M_a(__expected) { } @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } private: - alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; + alignas(__alignof__(int)) int _M_a; }; _GLIBCXX_END_NAMESPACE_VERSION } // namespace