diff mbox series

libstdc++: Fix constexpr basic_string union member [PR113294]

Message ID DB9PR01MB7466F182400680C1ACC93EA1A4482@DB9PR01MB7466.eurprd01.prod.exchangelabs.com
State New
Headers show
Series libstdc++: Fix constexpr basic_string union member [PR113294] | expand

Commit Message

Paul Keir Feb. 12, 2024, 6:15 p.m. UTC 
A call to `basic_string::clear()` in the std::string move assignment operator leads to a constexpr error from an access of inactive union member `_M_local_buf` in the added test (`test_move()`). Changing `__str._M_local_buf` to `__str._M_use_local_data()` in `operator=(basic_string&& __str)` fixes this.

Running `make check RUNTESTFLAGS="conformance.exp=21_strings/basic_string/*"` on x86_64-pc-linux-gnu gave unchanged summary results (# of expected passes 570 # of expected failures 4 # of unsupported tests 4).

I don't have write access.

Signed-off-by: Paul Keir <paul.keir@uws.ac.uk>

--


Please consider the environment and think before you print.

The University of the West of Scotland is a registered Scottish charity. Charity number SC002520.

This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender.

Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the University of the West of Scotland.

As a public body, the University of the West of Scotland may be required to make available emails as well as other written forms of information as a result of a request made under the Freedom of Information (Scotland) Act 2002.

Comments

Patrick Palka Feb. 12, 2024, 10:15 p.m. UTC | #1 
On Mon, 12 Feb 2024, Paul Keir wrote:

> A call to `basic_string::clear()` in the std::string move assignment operator leads to a constexpr error from an access of inactive union member `_M_local_buf` in the added test (`test_move()`). Changing `__str._M_local_buf` to `__str._M_use_local_data()` in `operator=(basic_string&& __str)` fixes this.
> 
> Running `make check RUNTESTFLAGS="conformance.exp=21_strings/basic_string/*"` on x86_64-pc-linux-gnu gave unchanged summary results (# of expected passes 570 # of expected failures 4 # of unsupported tests 4).
> 
> I don't have write access.
> 
> Signed-off-by: Paul Keir <paul.keir@uws.ac.uk>

LGTM!  A ChangeLog entry is missing from the commit message,
so here's one that we can use for this commit:

	PR libstdc++/113294

libstdc++-v3/ChangeLog:

	* include/bits/basic_string.h (basic_string::operator=): Use
	_M_use_local_data() instead of _M_local_buf on the moved-from
	string.
	* testsuite/21_strings/basic_string/modifiers/constexpr.cc
	(test_move): New test.

> 
> diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> index 43efc99bea9..8a695a494ef 100644
> --- a/libstdc++-v3/include/bits/basic_string.h
> +++ b/libstdc++-v3/include/bits/basic_string.h
> @@ -909,7 +909,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>                 __str._M_capacity(__capacity);
>               }
>             else
> -             __str._M_data(__str._M_local_buf);
> +             __str._M_data(__str._M_use_local_data());
>           }
>         else // Need to do a deep copy
>           assign(__str);
> diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> index 0e28a6d4487..ad7548f3236 100644
> --- a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> +++ b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> @@ -50,3 +50,16 @@ test_erasure()
>  }
> 
>  static_assert( test_erasure() );
> +
> +constexpr bool
> +test_move()
> +{
> +  std::string s1;
> +  std::string s2 = "1234567890123456"; // 16 chars: more than _S_local_capacity
> +  s1 = std::move(s2);
> +  VERIFY( s1 == "1234567890123456" );
> +
> +  return true;
> +}
> +
> +static_assert( test_move() );
> --
> 
> 
> Please consider the environment and think before you print.
> 
> The University of the West of Scotland is a registered Scottish charity. Charity number SC002520.
> 
> This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender.
> 
> Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the University of the West of Scotland.
> 
> As a public body, the University of the West of Scotland may be required to make available emails as well as other written forms of information as a result of a request made under the Freedom of Information (Scotland) Act 2002.
> 
>
Jonathan Wakely Feb. 12, 2024, 10:23 p.m. UTC | #2 
On Mon, 12 Feb 2024 at 22:17, Patrick Palka <ppalka@redhat.com> wrote:

> On Mon, 12 Feb 2024, Paul Keir wrote:
>
> > A call to `basic_string::clear()` in the std::string move assignment
> operator leads to a constexpr error from an access of inactive union member
> `_M_local_buf` in the added test (`test_move()`). Changing
> `__str._M_local_buf` to `__str._M_use_local_data()` in
> `operator=(basic_string&& __str)` fixes this.
> >
> > Running `make check
> RUNTESTFLAGS="conformance.exp=21_strings/basic_string/*"` on
> x86_64-pc-linux-gnu gave unchanged summary results (# of expected passes
> 570 # of expected failures 4 # of unsupported tests 4).
> >
> > I don't have write access.
> >
> > Signed-off-by: Paul Keir <paul.keir@uws.ac.uk>
>
> LGTM!  A ChangeLog entry is missing from the commit message,
> so here's one that we can use for this commit:
>
>         PR libstdc++/113294
>
> libstdc++-v3/ChangeLog:
>
>         * include/bits/basic_string.h (basic_string::operator=): Use
>         _M_use_local_data() instead of _M_local_buf on the moved-from
>         string.
>         * testsuite/21_strings/basic_string/modifiers/constexpr.cc
>         (test_move): New test.
>

The patch is OK for trunk and gcc-13 with that commit msg. Patrick, could
you push it for Paul?

Thank you both.



> >
> > diff --git a/libstdc++-v3/include/bits/basic_string.h
> b/libstdc++-v3/include/bits/basic_string.h
> > index 43efc99bea9..8a695a494ef 100644
> > --- a/libstdc++-v3/include/bits/basic_string.h
> > +++ b/libstdc++-v3/include/bits/basic_string.h
> > @@ -909,7 +909,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> >                 __str._M_capacity(__capacity);
> >               }
> >             else
> > -             __str._M_data(__str._M_local_buf);
> > +             __str._M_data(__str._M_use_local_data());
> >           }
> >         else // Need to do a deep copy
> >           assign(__str);
> > diff --git
> a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> > index 0e28a6d4487..ad7548f3236 100644
> > ---
> a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> > +++
> b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
> > @@ -50,3 +50,16 @@ test_erasure()
> >  }
> >
> >  static_assert( test_erasure() );
> > +
> > +constexpr bool
> > +test_move()
> > +{
> > +  std::string s1;
> > +  std::string s2 = "1234567890123456"; // 16 chars: more than
> _S_local_capacity
> > +  s1 = std::move(s2);
> > +  VERIFY( s1 == "1234567890123456" );
> > +
> > +  return true;
> > +}
> > +
> > +static_assert( test_move() );
> > --
> >
> >
> > Please consider the environment and think before you print.
> >
> > The University of the West of Scotland is a registered Scottish charity.
> Charity number SC002520.
> >
> > This e-mail and any attachment is for authorised use by the intended
> recipient(s) only. It may contain proprietary material, confidential
> information and/or be subject to legal privilege. It should not be copied,
> disclosed to, retained or used by, any other party. If you are not an
> intended recipient then please promptly delete this e-mail and any
> attachment and all copies and inform the sender.
> >
> > Please note that any views or opinions presented in this email are
> solely those of the author and do not necessarily represent those of the
> University of the West of Scotland.
> >
> > As a public body, the University of the West of Scotland may be required
> to make available emails as well as other written forms of information as a
> result of a request made under the Freedom of Information (Scotland) Act
> 2002.
> >
> >
>
>
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 43efc99bea9..8a695a494ef 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -909,7 +909,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
                __str._M_capacity(__capacity);
              }
            else
-             __str._M_data(__str._M_local_buf);
+             __str._M_data(__str._M_use_local_data());
          }
        else // Need to do a deep copy
          assign(__str);
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
index 0e28a6d4487..ad7548f3236 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/constexpr.cc
@@ -50,3 +50,16 @@  test_erasure()
 }

 static_assert( test_erasure() );
+
+constexpr bool
+test_move()
+{
+  std::string s1;
+  std::string s2 = "1234567890123456"; // 16 chars: more than _S_local_capacity
+  s1 = std::move(s2);
+  VERIFY( s1 == "1234567890123456" );
+
+  return true;
+}
+
+static_assert( test_move() );