[Bug libstdc++/56166] New: std::string::clear() can allocate memory despite being marked noexcept

[Bug libstdc++/56166] New: std::string::clear() can allocate memory despite being marked noexcept

[zackw at panix dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

             Bug #: 56166
           Summary: std::string::clear() can allocate memory despite being
                    marked noexcept
    Classification: Unclassified
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: zackw@panix.com


The attached test case (which is regrettably large, but I haven't found a
shorter way to trigger the problem) will call terminate() when compiled with
-std=c++11, despite there being no point at which an exception should escape
main().  The problem is deep inside std::basic_string:

/usr/include/c++/4.7/bits/basic_string.h
    796       /**
    797        *  Erases the string, making it empty.
    798        */
    799       void
    800       clear() _GLIBCXX_NOEXCEPT
    801       { _M_mutate(0, this->size(), 0); }

_M_mutate can allocate memory even when it's being asked to erase the string
(presumably due to internal reference-counting -- if you take 's2' out of the
test case the crash goes away), and thus can throw bad_alloc.  In C++11 mode,
clear() is marked 'noexcept', so we wind up in terminate().

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[zackw at panix dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #1 from Zack Weinberg <zackw at panix dot com> 2013-01-31 18:21:49 UTC ---
Created attachment 29320
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29320
test case

In case you're wondering, this was an attempt to do at least *some* testing of
a library's robustness in the face of memory allocation failure.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-01-31
     Ever Confirmed|0                           |1

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2013-01-31 19:25:27 UTC ---
(In reply to comment #0)
> _M_mutate can allocate memory even when it's being asked to erase the string
> (presumably due to internal reference-counting

If another string shares the same representation then you can't just modify the
representation, you need to create your own clone first, *then* clear it.

For the default configuration we could just point to the shared empty rep, I
wonder why we don't do that.  For the _GLIBCXX_FULLY_DYNAMIC_STRING case we
should probably remove noexcept

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> 2013-01-31 22:24:21 UTC ---
I suppose ext/vstring is already fine. As regards the current
std::basic_string, if Jon has a quick fix, great, but I suggest not spending
much time on it.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[zackw at panix dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #4 from Zack Weinberg <zackw at panix dot com> 2013-02-01 00:48:43 UTC ---
Is vstring going to be promoted to std::string in the *near* future? it doesn't
seem done to me, eg there is no stringstream for it, and it appears to generate
bulkier code. (not scientifically benchmarked)

that said, it doesn't fail this test case, but I haven't checked how its
clear() works, so I'm not sure it has no bug here.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2013-02-01 08:41:59 UTC ---
4.9, yes. Of course wrt the C++11 requirements the current implementation is
non conforming for many other reasons (reference counting is out ruled) + there
are long standing bugs (in partixular alignment related and related to the
shared zero rep for some targets). It goes without saying that when ext/vstring
is promoted it also gets automatically all the iostream overloads, etc, and
further improvements are definitely welcome.

That said, for sure I don't mean to unconditionally oppose fixes to the current
implementation, eh Jon's idea seems fine to me in principle, if only he could
prototype it, would be great, my point was that it doesn't seem to make much
sense doing a *lot* of that *now*.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 2013-02-01 11:25:33 UTC ---
(In reply to comment #5)
> That said, for sure I don't mean to unconditionally oppose fixes to the current
> implementation, eh Jon's idea seems fine to me in principle, if only he could
> prototype it, would be great, my point was that it doesn't seem to make much
> sense doing a *lot* of that *now*.

Yeah I can take a quick look at that over the weekend

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[fasdfasdas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

Tolga <fasdfasdas at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fasdfasdas at gmail dot com

--- Comment #7 from Tolga <fasdfasdas at gmail dot com> ---
I can confirm this on g++ 4.8.1.

I think I can also confirm that it is about reference counting, thanks to this
report, I was able to work around it though.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[fasdfasdas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #8 from Tolga <fasdfasdas at gmail dot com> ---
Correction, removing the copy, therefore reference counting effect, feels like
has decreased the error but I found out std::string::clear() still throws
std::length_error, although I use no reference.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[fasdfasdas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #9 from Tolga <fasdfasdas at gmail dot com> ---
It was a bug in my code that called clear from one thread while other inserts
something.

Yet, it throws.

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #10 from Marc Glisse <glisse at gcc dot gnu.org> ---
Author: glisse
Date: Fri Sep 20 15:50:09 2013
New Revision: 202781

URL: http://gcc.gnu.org/viewcvs?rev=202781&root=gcc&view=rev
Log:
2013-09-20  Marc Glisse  <marc.glisse@inria.fr>

    PR libstdc++/58338
    * include/bits/allocator.h (__alloc_swap::_S_do_it,
    __shrink_to_fit_aux::_S_do_it): Mark as noexcept.
    * include/bits/basic_string.h (basic_string::_Rep) [_S_empty_rep,
    _M_is_leaked, _M_is_shared, _M_set_leaked, _M_set_sharable,
    _M_set_length_and_sharable, _M_dispose]: Likewise.
    (basic_string::_Alloc_hider::_Alloc_hider): Likewise.
    (basic_string) [_M_data, _M_rep, _M_ibegin, _M_iend, _M_limit,
    _M_disjunct, _M_copy, _M_move, _M_assign, _S_copy_chars, _S_compare,
    _S_empty_rep, shrink_to_fit, operator[] const, front const, back const]:
    Likewise.
    [clear]: Link to PR 56166.
    [swap]: Link to PR 58265.
    * include/bits/stl_deque.h (_Deque_iterator) [_S_buffer_size,
    _Deque_iterator, _M_const_cast, operator*, operator->, operator++,
    operator--, operator+=, operator+, operator-=, operator-, operator[],
    _M_set_node]: Mark as noexcept.
    (operator==(const _Deque_iterator&, const _Deque_iterator&),
    operator!=(const _Deque_iterator&, const _Deque_iterator&),
    operator<(const _Deque_iterator&, const _Deque_iterator&),
    operator>(const _Deque_iterator&, const _Deque_iterator&),
    operator<=(const _Deque_iterator&, const _Deque_iterator&),
    operator>=(const _Deque_iterator&, const _Deque_iterator&),
    operator-(const _Deque_iterator&, const _Deque_iterator&),
    operator+(ptrdiff_t, const _Deque_iterator&)): Likewise.
    (_Deque_base) [_Deque_base(const allocator_type&)]: Add missing call to
    _M_initialize_map.
    [~_Deque_base, _M_deallocate_node, _M_deallocate_map, _M_destroy_nodes]:
    Mark as noexcept.
    (_Deque_base::_Deque_impl) [_Deque_impl(const _Tp_alloc_type&),
    _Deque_impl(_Tp_alloc_type&&)]: Likewise.
    (deque) [_S_buffer_size, operator=(deque&&), shrink_to_fit, operator[],
    front, back, pop_front, pop_back, swap]: Likewise.
    [deque(), deque(const allocator_type&)]: Merge.
    * include/debug/deque (deque) [operator=(deque&&), shrink_to_fit,
    operator[], front, back, pop_front, pop_back, swap]: Mark as noexcept.
    * include/profile/deque (deque) [operator=(deque&&), operator[], front,
    back, pop_front, pop_back, swap]: Likewise.
    * testsuite/23_containers/deque/requirements/dr438/assign_neg.cc:
    Adjust line number.
    * testsuite/23_containers/deque/requirements/dr438/constructor_1_neg.cc:
    Likewise.
    * testsuite/23_containers/deque/requirements/dr438/constructor_2_neg.cc:
    Likewise.
    * testsuite/23_containers/deque/requirements/dr438/insert_neg.cc:
    Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/allocator.h
    trunk/libstdc++-v3/include/bits/basic_string.h
    trunk/libstdc++-v3/include/bits/stl_deque.h
    trunk/libstdc++-v3/include/debug/deque
    trunk/libstdc++-v3/include/profile/deque
   
trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/assign_neg.cc
   
trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_1_neg.cc
   
trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_2_neg.cc
   
trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/insert_neg.cc

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56166

--- Comment #11 from Marc Glisse <glisse at gcc dot gnu.org> ---
Author: glisse
Date: Tue Sep 24 10:07:32 2013
New Revision: 202861

URL: http://gcc.gnu.org/viewcvs?rev=202861&root=gcc&view=rev
Log:
2013-09-24  Marc Glisse  <marc.glisse@inria.fr>

    PR libstdc++/58338
    PR libstdc++/56166
    * include/bits/basic_string.h (basic_string)
    [basic_string(basic_string&&)]: Make the noexcept conditional.
    [operator=(basic_string&&), assign(basic_string&&)]: Link to PR 58265.
    [begin(), end(), rbegin(), rend(), clear]: Remove noexcept.
    [pop_back]: Comment on the lack of noexcept.
    * include/debug/string (basic_string) [basic_string(const _Allocator&),
    basic_string(basic_string&&), begin(), end(), rbegin(), rend(), clear,
    operator[](size_type), pop_back]: Comment out noexcept, until vstring
    replaces basic_string.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/basic_string.h
    trunk/libstdc++-v3/include/debug/string

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[redi at gcc dot gnu.org]

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

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think __gnu_cxx::__rc_string has the same problem

There's a patch at https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00278.html

[Bug libstdc++/56166] std::string::clear() can throw despite being marked noexcept

[redi at gcc dot gnu.org]

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

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
N.B. this is fixed when using the new std::__cxx11::basic_string in GCC 5