Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Fri, 29 Sept 2023 at 17:46, Jonathan Wakely wrote: > > On Fri, 29 Sept 2023 at 17:29, Nathaniel Shead > wrote: > > > > On Fri, Sep 29, 2023 at 04:06:33PM +0100, Jonathan Wakely wrote: > > > On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely wrote: > > > > > Thanks for the comments, here's an updated version of the patch. > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > > > Great, I'll get this committed today - thanks! > > > > > > That's done now. > > > > > > > Thanks! > > > > > > > > > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > > > > already used only for their side effects without a cast to void, e.g. > > > > > > > > > > /** > > > > >* @brief Default constructor creates an empty string. > > > > >*/ > > > > > _GLIBCXX20_CONSTEXPR > > > > > basic_string() > > > > > > > > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > > > : _M_dataplus(_M_local_data()) > > > > > { > > > > > _M_use_local_data(); > > > > > _M_set_length(0); > > > > > } > > > > > > > > > > I haven't updated these, but should this be changed for consistency? > > > > > > > > Yes, good idea. I can do that. > > > > > > I started to do that, and decided it made more sense to split out the > > > constexpr loop from _M_use_local_data() into a separate function, > > > _M_init_local_buf(). Then we can use that for all the places where we > > > don't care about the return value. That avoids the overhead of using > > > pointer_traits::pointer_to when we don't need the return value (which > > > is admittedly only going to be an issue for -O0 code, but I think it's > > > cleaner this way anyway). > > > > > > Please see the attached patch and let me know what you think. > > > > I agree, and it also looks clearer to me what is happening. > > Good, I'll make this change next week then. > > > > > > > > > > > Thanks again for fixing these. I think this might fix some bug reports > > > > about clang rejecting our std::string in constant expressions, so I'll > > > > check those. > > > > > > Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 > > > (so we should backport it to gcc-13 and gcc-12 too). > > > > > commit 2668979d3206ff6c039ac0165aae29377a15666c > > > Author: Jonathan Wakely > > > Date: Fri Sep 29 12:12:22 2023 > > > > > > libstdc++: Split std::basic_string::_M_use_local_data into two > > > functions > > > > > > This splits out the activate-the-union-member-for-constexpr logic from > > > _M_use_local_data, so that it can be used separately in cases that > > > don't > > > need to use std::pointer_traits::pointer_to to obtain the > > > return value. > > > > > > This leaves only three uses of _M_use_local_data() which are all the > > > same form: > > > > > > __s._M_data(_M_use_local_data()); > > > __s._M_set_length(0); > > > > > > We could remove _M_use_local_data() and change those three places to > > > use > > > a new _M_reset() function that does: > > > > > > _M_init_local_buf(); > > > _M_data(_M_local_data()); > > > _M_set_length(0); > > > > > > This is left for a future change. > > > > > > libstdc++-v3/ChangeLog: > > > > > > * include/bits/basic_string.h (_M_init_local_buf()): New > > > function. > > > (_M_use_local_data()): Use _M_init_local_buf. > > > (basic_string(), basic_string(const Alloc&)) > > > (basic_string(basic_string&&)) > > > (basic_string(basic_string&&, const Alloc&)): Use > > > _M_init_local_buf instead of _M_use_local_data(). > > > * include/bits/basic_string.tcc (swap(basic_string&)) > > > (_M_construct(InIter, InIter, forward_iterator_tag)) > > > (_M_construct(size_type, CharT), reserve()): Likewise. > > > (_M_construct(InIter, InIter, input_iterator_tag)): Remove > > > call > > > to _M_use_local_data() and initialize the local buffer > > > directly. > > > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h > > > b/libstdc++-v3/include/bits/basic_string.h > > > index 4f94cd967cf..18a19b8dcbc 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.h > > > +++ b/libstdc++-v3/include/bits/basic_string.h > > > @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > >// Ensure that _M_local_buf is the active member of the union. > > >__attribute__((__always_inline__)) > > >_GLIBCXX14_CONSTEXPR > > > - pointer > > > - _M_use_local_data() _GLIBCXX_NOEXCEPT > > > + void > > > + _M_init_local_buf() _GLIBCXX_NOEXCEPT > > >{ > > > #if __cpp_lib_is_constant_evaluated > > > if (std::is_constant_evaluated()) > > > for (size_type __i = 0; __i <= _S_local_capacity; ++__i) > > > _M_local_buf[__i] = _CharT(); > > > +#endif > > > + } > > > + > > > +
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Fri, 29 Sept 2023 at 17:29, Nathaniel Shead wrote: > > On Fri, Sep 29, 2023 at 04:06:33PM +0100, Jonathan Wakely wrote: > > On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely wrote: > > > > Thanks for the comments, here's an updated version of the patch. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > Great, I'll get this committed today - thanks! > > > > That's done now. > > > > Thanks! > > > > > > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > > > already used only for their side effects without a cast to void, e.g. > > > > > > > > /** > > > >* @brief Default constructor creates an empty string. > > > >*/ > > > > _GLIBCXX20_CONSTEXPR > > > > basic_string() > > > > > > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > > : _M_dataplus(_M_local_data()) > > > > { > > > > _M_use_local_data(); > > > > _M_set_length(0); > > > > } > > > > > > > > I haven't updated these, but should this be changed for consistency? > > > > > > Yes, good idea. I can do that. > > > > I started to do that, and decided it made more sense to split out the > > constexpr loop from _M_use_local_data() into a separate function, > > _M_init_local_buf(). Then we can use that for all the places where we > > don't care about the return value. That avoids the overhead of using > > pointer_traits::pointer_to when we don't need the return value (which > > is admittedly only going to be an issue for -O0 code, but I think it's > > cleaner this way anyway). > > > > Please see the attached patch and let me know what you think. > > I agree, and it also looks clearer to me what is happening. Good, I'll make this change next week then. > > > > > > Thanks again for fixing these. I think this might fix some bug reports > > > about clang rejecting our std::string in constant expressions, so I'll > > > check those. > > > > Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 > > (so we should backport it to gcc-13 and gcc-12 too). > > > commit 2668979d3206ff6c039ac0165aae29377a15666c > > Author: Jonathan Wakely > > Date: Fri Sep 29 12:12:22 2023 > > > > libstdc++: Split std::basic_string::_M_use_local_data into two functions > > > > This splits out the activate-the-union-member-for-constexpr logic from > > _M_use_local_data, so that it can be used separately in cases that don't > > need to use std::pointer_traits::pointer_to to obtain the > > return value. > > > > This leaves only three uses of _M_use_local_data() which are all the > > same form: > > > > __s._M_data(_M_use_local_data()); > > __s._M_set_length(0); > > > > We could remove _M_use_local_data() and change those three places to use > > a new _M_reset() function that does: > > > > _M_init_local_buf(); > > _M_data(_M_local_data()); > > _M_set_length(0); > > > > This is left for a future change. > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/basic_string.h (_M_init_local_buf()): New > > function. > > (_M_use_local_data()): Use _M_init_local_buf. > > (basic_string(), basic_string(const Alloc&)) > > (basic_string(basic_string&&)) > > (basic_string(basic_string&&, const Alloc&)): Use > > _M_init_local_buf instead of _M_use_local_data(). > > * include/bits/basic_string.tcc (swap(basic_string&)) > > (_M_construct(InIter, InIter, forward_iterator_tag)) > > (_M_construct(size_type, CharT), reserve()): Likewise. > > (_M_construct(InIter, InIter, input_iterator_tag)): Remove call > > to _M_use_local_data() and initialize the local buffer directly. > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h > > b/libstdc++-v3/include/bits/basic_string.h > > index 4f94cd967cf..18a19b8dcbc 100644 > > --- a/libstdc++-v3/include/bits/basic_string.h > > +++ b/libstdc++-v3/include/bits/basic_string.h > > @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > >// Ensure that _M_local_buf is the active member of the union. > >__attribute__((__always_inline__)) > >_GLIBCXX14_CONSTEXPR > > - pointer > > - _M_use_local_data() _GLIBCXX_NOEXCEPT > > + void > > + _M_init_local_buf() _GLIBCXX_NOEXCEPT > >{ > > #if __cpp_lib_is_constant_evaluated > > if (std::is_constant_evaluated()) > > for (size_type __i = 0; __i <= _S_local_capacity; ++__i) > > _M_local_buf[__i] = _CharT(); > > +#endif > > + } > > + > > + __attribute__((__always_inline__)) > > + _GLIBCXX14_CONSTEXPR > > + pointer > > + _M_use_local_data() _GLIBCXX_NOEXCEPT > > + { > > +#if __glibcxx_is_constant_evaluated > > + _M_init_local_buf(); > > #endif > > return _M_local_data(); > >} > > What's the difference between __cpp_lib_is_con
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Fri, Sep 29, 2023 at 04:06:33PM +0100, Jonathan Wakely wrote: > On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely wrote: > > > Thanks for the comments, here's an updated version of the patch. > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > Great, I'll get this committed today - thanks! > > That's done now. > Thanks! > > > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > > already used only for their side effects without a cast to void, e.g. > > > > > > /** > > >* @brief Default constructor creates an empty string. > > >*/ > > > _GLIBCXX20_CONSTEXPR > > > basic_string() > > > > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > : _M_dataplus(_M_local_data()) > > > { > > > _M_use_local_data(); > > > _M_set_length(0); > > > } > > > > > > I haven't updated these, but should this be changed for consistency? > > > > Yes, good idea. I can do that. > > I started to do that, and decided it made more sense to split out the > constexpr loop from _M_use_local_data() into a separate function, > _M_init_local_buf(). Then we can use that for all the places where we > don't care about the return value. That avoids the overhead of using > pointer_traits::pointer_to when we don't need the return value (which > is admittedly only going to be an issue for -O0 code, but I think it's > cleaner this way anyway). > > Please see the attached patch and let me know what you think. I agree, and it also looks clearer to me what is happening. > > > Thanks again for fixing these. I think this might fix some bug reports > > about clang rejecting our std::string in constant expressions, so I'll > > check those. > > Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 > (so we should backport it to gcc-13 and gcc-12 too). > commit 2668979d3206ff6c039ac0165aae29377a15666c > Author: Jonathan Wakely > Date: Fri Sep 29 12:12:22 2023 > > libstdc++: Split std::basic_string::_M_use_local_data into two functions > > This splits out the activate-the-union-member-for-constexpr logic from > _M_use_local_data, so that it can be used separately in cases that don't > need to use std::pointer_traits::pointer_to to obtain the > return value. > > This leaves only three uses of _M_use_local_data() which are all the > same form: > > __s._M_data(_M_use_local_data()); > __s._M_set_length(0); > > We could remove _M_use_local_data() and change those three places to use > a new _M_reset() function that does: > > _M_init_local_buf(); > _M_data(_M_local_data()); > _M_set_length(0); > > This is left for a future change. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h (_M_init_local_buf()): New > function. > (_M_use_local_data()): Use _M_init_local_buf. > (basic_string(), basic_string(const Alloc&)) > (basic_string(basic_string&&)) > (basic_string(basic_string&&, const Alloc&)): Use > _M_init_local_buf instead of _M_use_local_data(). > * include/bits/basic_string.tcc (swap(basic_string&)) > (_M_construct(InIter, InIter, forward_iterator_tag)) > (_M_construct(size_type, CharT), reserve()): Likewise. > (_M_construct(InIter, InIter, input_iterator_tag)): Remove call > to _M_use_local_data() and initialize the local buffer directly. > > diff --git a/libstdc++-v3/include/bits/basic_string.h > b/libstdc++-v3/include/bits/basic_string.h > index 4f94cd967cf..18a19b8dcbc 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >// Ensure that _M_local_buf is the active member of the union. >__attribute__((__always_inline__)) >_GLIBCXX14_CONSTEXPR > - pointer > - _M_use_local_data() _GLIBCXX_NOEXCEPT > + void > + _M_init_local_buf() _GLIBCXX_NOEXCEPT >{ > #if __cpp_lib_is_constant_evaluated > if (std::is_constant_evaluated()) > for (size_type __i = 0; __i <= _S_local_capacity; ++__i) > _M_local_buf[__i] = _CharT(); > +#endif > + } > + > + __attribute__((__always_inline__)) > + _GLIBCXX14_CONSTEXPR > + pointer > + _M_use_local_data() _GLIBCXX_NOEXCEPT > + { > +#if __glibcxx_is_constant_evaluated > + _M_init_local_buf(); > #endif > return _M_local_data(); >} What's the difference between __cpp_lib_is_constant_evaluated and __glibcxx_is_constant_evaluated? Should these lines be using the same macro here? > @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >_GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) >: _M_dataplus(_M_local_data()) >{ > - _M_use_loc
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely wrote: > > Thanks for the comments, here's an updated version of the patch. > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > Great, I'll get this committed today - thanks! That's done now. > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > already used only for their side effects without a cast to void, e.g. > > > > /** > >* @brief Default constructor creates an empty string. > >*/ > > _GLIBCXX20_CONSTEXPR > > basic_string() > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > : _M_dataplus(_M_local_data()) > > { > > _M_use_local_data(); > > _M_set_length(0); > > } > > > > I haven't updated these, but should this be changed for consistency? > > Yes, good idea. I can do that. I started to do that, and decided it made more sense to split out the constexpr loop from _M_use_local_data() into a separate function, _M_init_local_buf(). Then we can use that for all the places where we don't care about the return value. That avoids the overhead of using pointer_traits::pointer_to when we don't need the return value (which is admittedly only going to be an issue for -O0 code, but I think it's cleaner this way anyway). Please see the attached patch and let me know what you think. > Thanks again for fixing these. I think this might fix some bug reports > about clang rejecting our std::string in constant expressions, so I'll > check those. Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 (so we should backport it to gcc-13 and gcc-12 too). commit 2668979d3206ff6c039ac0165aae29377a15666c Author: Jonathan Wakely Date: Fri Sep 29 12:12:22 2023 libstdc++: Split std::basic_string::_M_use_local_data into two functions This splits out the activate-the-union-member-for-constexpr logic from _M_use_local_data, so that it can be used separately in cases that don't need to use std::pointer_traits::pointer_to to obtain the return value. This leaves only three uses of _M_use_local_data() which are all the same form: __s._M_data(_M_use_local_data()); __s._M_set_length(0); We could remove _M_use_local_data() and change those three places to use a new _M_reset() function that does: _M_init_local_buf(); _M_data(_M_local_data()); _M_set_length(0); This is left for a future change. libstdc++-v3/ChangeLog: * include/bits/basic_string.h (_M_init_local_buf()): New function. (_M_use_local_data()): Use _M_init_local_buf. (basic_string(), basic_string(const Alloc&)) (basic_string(basic_string&&)) (basic_string(basic_string&&, const Alloc&)): Use _M_init_local_buf instead of _M_use_local_data(). * include/bits/basic_string.tcc (swap(basic_string&)) (_M_construct(InIter, InIter, forward_iterator_tag)) (_M_construct(size_type, CharT), reserve()): Likewise. (_M_construct(InIter, InIter, input_iterator_tag)): Remove call to _M_use_local_data() and initialize the local buffer directly. diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 4f94cd967cf..18a19b8dcbc 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 // Ensure that _M_local_buf is the active member of the union. __attribute__((__always_inline__)) _GLIBCXX14_CONSTEXPR - pointer - _M_use_local_data() _GLIBCXX_NOEXCEPT + void + _M_init_local_buf() _GLIBCXX_NOEXCEPT { #if __cpp_lib_is_constant_evaluated if (std::is_constant_evaluated()) for (size_type __i = 0; __i <= _S_local_capacity; ++__i) _M_local_buf[__i] = _CharT(); +#endif + } + + __attribute__((__always_inline__)) + _GLIBCXX14_CONSTEXPR + pointer + _M_use_local_data() _GLIBCXX_NOEXCEPT + { +#if __glibcxx_is_constant_evaluated + _M_init_local_buf(); #endif return _M_local_data(); } @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) : _M_dataplus(_M_local_data()) { - _M_use_local_data(); + _M_init_local_buf(); _M_set_length(0); } @@ -534,7 +544,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT : _M_dataplus(_M_local_data(), __a) { - _M_use_local_data(); + _M_init_local_buf(); _M_set_length(0); } @@ -678,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); tra
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Fri, 29 Sept 2023 at 00:25, Nathaniel Shead wrote: > > On Wed, Sep 27, 2023 at 03:13:35PM +0100, Jonathan Wakely wrote: > > On Sat, 23 Sept 2023 at 08:30, Nathaniel Shead via Libstdc++ > > wrote: > > > > > > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > > > libstd...@gcc.gnu.org> wrote: > > > > > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > > > following libstdc++ tests: > > > > > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > > > errors) > > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > > > errors) > > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess > > > > > errors) > > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess > > > > > errors) > > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 > > > > > (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 > > > > > (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 > > > > > (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 > > > > > (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > > -std=gnu++20 (test for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > > -std=gnu++26 (test for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > > > (test for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > > > (test for excess errors) > > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > > > errors) > > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 > > > > > compilation > > > > > failed to produce executable > > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > > > errors) > > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 > > > > > compilation > > > > > failed to produce executable > > > > > > > > > > On investigation though it looks like the issue might be with > > > > > libstdc++ > > > > > rather than the patch itself; running the failing tests using clang > > > > > with > > > > > libstdc++ also produces similar errors, and my reading of the code > > > > > suggests that this is correct. > > > > > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > > > the libstdc++ issues before resubmitting this patch for the C++ > > > > > frontend? Or should I submit a version of this patch without the > > > > > `std::construct_at` changes and wait till libstdc++ gets fixed for > > > > > that? > > > > > > > > > > > > > I think we should fix libstdc++. There are probably only a few places > > > > that > > > > need a fix, which cause all those failures. > > > > > > > > I can help with those fixes. I'll look into it after the weekend. > > > > > > > > > > Thanks. I did end up getting a chance to look at it earlier today, and > > > with the following patch I had no regressions when applying the frontend > > > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > -- >8 -- > > > > > > This patch ensures that the union members for std::string and > > > std::variant are always properly set when a change occurs. > > > > > > libstdc++-v3/ChangeLog: > > > > > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > > > Activate _M_local_buf when needed. > > > (basic_string(basic_string&&, const _Alloc&)): Likewise. > > > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > > > * include/std/variant: (__detail::__variant::__construct_n): New. > > > (__detail::_variant::__emplace): Use __construct_n. > > > > > > Signed-off-by: Nathaniel Shead > > > --- > > > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > > > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > > > libstdc++-v3/include/std/variant | 32 -- > > > 3 files changed, 38 insertions(+), 9 deletions(-) > > > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h > > > b/libstdc++-v3/include/bits/basic_string.h > > > index 09fd62afa66..7c342879827 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.h > > > +++ b/libstdc++-v3/include/bits/basic_string.h > > > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > >{ > > > if (__str._M_is_local()) > > > { > > > - traits_type::copy(_M_local_buf, __str._M_local_buf, > > > + traits_type::copy(_M_use_local_data(), __str._M_local
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Wed, Sep 27, 2023 at 03:13:35PM +0100, Jonathan Wakely wrote: > On Sat, 23 Sept 2023 at 08:30, Nathaniel Shead via Libstdc++ > wrote: > > > > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > > libstd...@gcc.gnu.org> wrote: > > > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > > following libstdc++ tests: > > > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > > errors) > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > > errors) > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 > > > > (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 > > > > (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > -std=gnu++20 (test for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > -std=gnu++26 (test for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > > (test for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > > (test for excess errors) > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > > errors) > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > > failed to produce executable > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > > errors) > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > > failed to produce executable > > > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > > rather than the patch itself; running the failing tests using clang with > > > > libstdc++ also produces similar errors, and my reading of the code > > > > suggests that this is correct. > > > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > > the libstdc++ issues before resubmitting this patch for the C++ > > > > frontend? Or should I submit a version of this patch without the > > > > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > > > > > > > > > > I think we should fix libstdc++. There are probably only a few places that > > > need a fix, which cause all those failures. > > > > > > I can help with those fixes. I'll look into it after the weekend. > > > > > > > Thanks. I did end up getting a chance to look at it earlier today, and > > with the following patch I had no regressions when applying the frontend > > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > -- >8 -- > > > > This patch ensures that the union members for std::string and > > std::variant are always properly set when a change occurs. > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > > Activate _M_local_buf when needed. > > (basic_string(basic_string&&, const _Alloc&)): Likewise. > > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > > * include/std/variant: (__detail::__variant::__construct_n): New. > > (__detail::_variant::__emplace): Use __construct_n. > > > > Signed-off-by: Nathaniel Shead > > --- > > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > > libstdc++-v3/include/std/variant | 32 -- > > 3 files changed, 38 insertions(+), 9 deletions(-) > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h > > b/libstdc++-v3/include/bits/basic_string.h > > index 09fd62afa66..7c342879827 100644 > > --- a/libstdc++-v3/include/bits/basic_string.h > > +++ b/libstdc++-v3/include/bits/basic_string.h > > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > >{ > > if (__str._M_is_local()) > > { > > - traits_type::copy(_M_local_buf, __str._M_local_buf, > > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > > __str.length() + 1); > > } > > else > > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > // basic_stringbuf relies on writing into unallocated capacity so > > // we mess up the contents if we put a '\0' in the string. > > _M_length(__str.length()); > > - __str._M_dat
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Sat, 23 Sept 2023 at 08:30, Nathaniel Shead via Libstdc++ wrote: > > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > libstd...@gcc.gnu.org> wrote: > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > following libstdc++ tests: > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > errors) > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++20 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++26 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > (test for excess errors) > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > failed to produce executable > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > failed to produce executable > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > rather than the patch itself; running the failing tests using clang with > > > libstdc++ also produces similar errors, and my reading of the code > > > suggests that this is correct. > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > the libstdc++ issues before resubmitting this patch for the C++ > > > frontend? Or should I submit a version of this patch without the > > > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > > > > > > > I think we should fix libstdc++. There are probably only a few places that > > need a fix, which cause all those failures. > > > > I can help with those fixes. I'll look into it after the weekend. > > > > Thanks. I did end up getting a chance to look at it earlier today, and > with the following patch I had no regressions when applying the frontend > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > > -- >8 -- > > This patch ensures that the union members for std::string and > std::variant are always properly set when a change occurs. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > Activate _M_local_buf when needed. > (basic_string(basic_string&&, const _Alloc&)): Likewise. > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > * include/std/variant: (__detail::__variant::__construct_n): New. > (__detail::_variant::__emplace): Use __construct_n. > > Signed-off-by: Nathaniel Shead > --- > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > libstdc++-v3/include/std/variant | 32 -- > 3 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/libstdc++-v3/include/bits/basic_string.h > b/libstdc++-v3/include/bits/basic_string.h > index 09fd62afa66..7c342879827 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >{ > if (__str._M_is_local()) > { > - traits_type::copy(_M_local_buf, __str._M_local_buf, > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > __str.length() + 1); > } > else > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > // basic_stringbuf relies on writing into unallocated capacity so > // we mess up the contents if we put a '\0' in the string. > _M_length(__str.length()); > - __str._M_data(__str._M_local_data()); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); >} > > @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >{ > if (__str._M_is_local()) > { > + _M_use_local_data(); Lets add a cast
Re: [PATCH] libstdc++: Ensure active union member is correctly set
On Sat, 23 Sept 2023, 08:30 Nathaniel Shead, wrote: > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > libstd...@gcc.gnu.org> wrote: > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > following libstdc++ tests: > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > errors) > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess > errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess > errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 > (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 > (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++20 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++26 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > (test for excess errors) > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > failed to produce executable > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > failed to produce executable > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > rather than the patch itself; running the failing tests using clang > with > > > libstdc++ also produces similar errors, and my reading of the code > > > suggests that this is correct. > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > the libstdc++ issues before resubmitting this patch for the C++ > > > frontend? Or should I submit a version of this patch without the > > > `std::construct_at` changes and wait till libstdc++ gets fixed for > that? > > > > > > > I think we should fix libstdc++. There are probably only a few places > that > > need a fix, which cause all those failures. > > > > I can help with those fixes. I'll look into it after the weekend. > > > > Thanks. I did end up getting a chance to look at it earlier today, and > with the following patch I had no regressions when applying the frontend > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > Nice, thanks! This looks good at first glance, I'll review it properly ASAP. > -- >8 -- > > This patch ensures that the union members for std::string and > std::variant are always properly set when a change occurs. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > Activate _M_local_buf when needed. > (basic_string(basic_string&&, const _Alloc&)): Likewise. > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > * include/std/variant: (__detail::__variant::__construct_n): New. > (__detail::_variant::__emplace): Use __construct_n. > > Signed-off-by: Nathaniel Shead > --- > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > libstdc++-v3/include/std/variant | 32 -- > 3 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/libstdc++-v3/include/bits/basic_string.h > b/libstdc++-v3/include/bits/basic_string.h > index 09fd62afa66..7c342879827 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >{ > if (__str._M_is_local()) > { > - traits_type::copy(_M_local_buf, __str._M_local_buf, > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > __str.length() + 1); > } > else > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > // basic_stringbuf relies on writing into unallocated capacity so > // we mess up the contents if we put a '\0' in the string. > _M_length(__str.length()); > - __str._M_data(__str._M_local_data()); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); >} > > @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >{ > if (__str._M_