[Bug libstdc++/109305] New: Allocator copy in basic_string::operator=

[Bug libstdc++/109305] New: Allocator copy in basic_string::operator=

[marc-andre.laverdiere at synopsys dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

            Bug ID: 109305
           Summary: Allocator copy in basic_string::operator=
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marc-andre.laverdiere at synopsys dot com
  Target Milestone: ---

Created attachment 54773
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54773&action=edit
Self-contained preprocessed reproducer

In basic_string& operator=(const basic_string& __str), I noticed this line:

auto __alloc = __str._M_get_allocator();

This creates a local copy of the allocator object. I doubt this is intentional,
but I could be wrong.

GCC version: 12.1.0 with the patch for 105562

Output of g++ -E -v -save-temps -c
Using built-in specs.
COLLECT_GCC=g++
OFFLOAD_TARGET_NAMES=nvptx-none:hsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu
9.4.0-1ubuntu1~20.04.1' --with-bugurl=file:///usr/share/doc/gcc-9/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,gm2 --prefix=/usr
--with-gcc-major-version-only --program-suffix=-9
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug
--enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new
--enable-gnu-unique-object --disable-vtable-verify --enable-plugin
--enable-default-pie --with-system-zlib --with-target-system-zlib=auto
--enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686
--with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib
--with-tune=generic
--enable-offload-targets=nvptx-none=/build/gcc-9-Av3uEd/gcc-9-9.4.0/debian/tmp-nvptx/usr,hsa
--without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.1)
COLLECT_GCC_OPTIONS='-E' '-v' '-save-temps' '-c' '-shared-libgcc'
'-mtune=generic' '-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/9/cc1plus -E -quiet -v -imultiarch
x86_64-linux-gnu -D_GNU_SOURCE reproducer.cpp -mtune=generic -march=x86-64
-fpch-preprocess -fasynchronous-unwind-tables -fstack-protector-strong -Wformat
-Wformat-security -fstack-clash-protection -fcf-protection
ignoring duplicate directory "/usr/include/x86_64-linux-gnu/c++/9"
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/9/include-fixed"
ignoring nonexistent directory
"/usr/lib/gcc/x86_64-linux-gnu/9/../../../../x86_64-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/include/c++/9
 /usr/include/x86_64-linux-gnu/c++/9
 /usr/include/c++/9/backward
 /usr/lib/gcc/x86_64-linux-gnu/9/include
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
COMPILER_PATH=/usr/lib/gcc/x86_64-linux-gnu/9/:/usr/lib/gcc/x86_64-linux-gnu/9/:/usr/lib/gcc/x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/9/:/usr/lib/gcc/x86_64-linux-gnu/
LIBRARY_PATH=/usr/lib/gcc/x86_64-linux-gnu/9/:/usr/lib/gcc/x86_64-linux-gnu/9/../../../x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/9/../../../../lib/:/lib/x86_64-linux-gnu/:/lib/../lib/:/usr/lib/x86_64-linux-gnu/:/usr/lib/../lib/:/usr/lib/gcc/x86_64-linux-gnu/9/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-E' '-v' '-save-temps' '-c' '-shared-libgcc'
'-mtune=generic' '-march=x86-64'

I'm attaching a self-contained reproducer. The statement of interest is at line
14824.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
LWG2579

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2579



This copy only happens with _S_propagate_on_copy_assign which is true even.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-03-27

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Marc-André Laverdière from comment #0)
> Created attachment 54773 [details]
> Self-contained preprocessed reproducer
> 
> In basic_string& operator=(const basic_string& __str), I noticed this line:
> 
> auto __alloc = __str._M_get_allocator();
> 
> This creates a local copy of the allocator object. I doubt this is
> intentional, but I could be wrong.

It's completely intentional. The function needs to allocate new storage, so it
needs a non-const allocator that compares equal to __str.get_allocator().

As the comment hints at, the order of the operations is important. We don't
want to update the allocator in *this until after we've allocated the storage,
because that could throw. So we make a copy of the allocator, use it to
allocate memory, then only if that was successful we replace the allocator in
*this.

Why do you think this local copy is a problem?

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[marc-andre.laverdiere at synopsys dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #4 from Marc-André Laverdière <marc-andre.laverdiere at synopsys dot com> ---
The comment is "If this allocation throws there are no effects:" and I didn't
understand the implications. Thanks for you spelled it out the logic behind it.
May I encourage you to update the comment?

As to why a local copy could be a problem, it's an efficiency (rather than
correctness) issue here. Removing unnecessary copies is always a plus. From the
explanations I got, that copy is necessary - or at least, removing it would be
complex.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|WAITING                     |RESOLVED

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Removing it would make the code less efficient and more complex.

If your allocator is expensive to copy, your allocator is bad and should be
fixed. They need to be cheap to copy.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
For your reproducer, the allocator is std::allocator<char> which is an empty
class and copying is a no-op. There is no efficiency concern whatsoever.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[chgros at synopsys dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #7 from Charles-Henri Gros <chgros at synopsys dot com> ---
For context, we're trying to detect cases where using "auto" unintentionally
creates a copy (it's regrettably common).
Here the copy is necessary to get a non-const value; that's definitely
something we could consider. Thank you for your feedback.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> Removing it would make the code less efficient and more complex.

In fact, I don't even see how it would be possible, except by making *another*
copy, e.g.

--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -1588,20 +1588,33 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
                    _M_destroy(_M_allocated_capacity);
                    _M_data(_M_use_local_data());
                    _M_set_length(0);
+                   std::__alloc_on_copy(_M_get_allocator(),
+                                        __str._M_get_allocator());
                  }
                else
                  {
+                   auto __backup_alloc = _M_get_allocator();
+                   std::__alloc_on_copy(_M_get_allocator(),
+                                        __str._M_get_allocator());
                    const auto __len = __str.size();
-                   auto __alloc = __str._M_get_allocator();
-                   // If this allocation throws there are no effects:
-                   auto __ptr = _Alloc_traits::allocate(__alloc, __len + 1);
-                   _M_destroy(_M_allocated_capacity);
+                   pointer __ptr;
+                   __try {
+                     __ptr = _Alloc_traits::allocate(_M_get_allocator(),
+                                                     __len + 1);
+                   } __catch (...) {
+                     std::__alloc_on_copy(_M_get_allocator(), __backup_alloc);
+                     __throw_exception_again;
+                   }
+                   _Alloc_traits::deallocate(__backup_alloc, _M_data(),
+                                             _M_allocated_capacity + 1);
                    _M_data(__ptr);
                    _M_capacity(__len);
                    _M_set_length(__len);
                  }
              }
-           std::__alloc_on_copy(_M_get_allocator(), __str._M_get_allocator());
+           else
+             std::__alloc_on_copy(_M_get_allocator(),
+                                  __str._M_get_allocator());
          }
 #endif
        this->_M_assign(__str);

This makes the code much worse, and slower, and still has a copy (the
__backup_alloc) variable.

[Bug libstdc++/109305] Allocator copy in basic_string::operator=

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109305

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Charles-Henri Gros from comment #7)
> For context, we're trying to detect cases where using "auto" unintentionally
> creates a copy (it's regrettably common).
> Here the copy is necessary to get a non-const value; that's definitely
> something we could consider. Thank you for your feedback.

Yes, it's necessary. If the copy was replaced with a reference to __str's
existing allocator, the next line of code would not compile:

include/bits/basic_string.h:1597:57: error: binding reference of type
‘std::allocator_traits<std::allocator<char> >::allocator_type&’ {aka
‘std::allocator<char>&’} to ‘const std::allocator<char>’ discards qualifiers
 1597 |                     auto __ptr = _Alloc_traits::allocate(__alloc, __len
+ 1);
      |                                 
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

Maybe you can use that to detect unnecessary copies: would changing auto to
auto&& still compile?

(That's still incomplete, it might compile, but behave incorrectly, but at
least checking if it compiles would be a start!)