[Bug libstdc++/108288] New: Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[Bug libstdc++/108288] New: Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[simon.marchi at polymtl dot ca]

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

            Bug ID: 108288
           Summary: Deadlock when using -fno-elide-constructor +
                    -D_GLIBCXX_DEBUG=1 + -std=c++11
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: simon.marchi at polymtl dot ca
  Target Milestone: ---

I built GDB with -fno-elide-constructor in order to chase / prove a bug in some
move constructor (which would get elided otherwise, hiding the problem).  I
also used -D_GLIBCXX_DEBUG=1 and -std=c++11, I think it's important here.  This
results in a deadlock in GDB, when trying to run a simple program:

(top-gdb) bt 15
#0  futex_wait (private=0, expected=2, futex_word=0x7ffff6e35f80
<__gnu_internal::get_mutex(unsigned char)::m>) at
../sysdeps/nptl/futex-internal.h:146
#1  __GI___lll_lock_wait (futex=futex@entry=0x7ffff6e35f80
<__gnu_internal::get_mutex(unsigned char)::m>, private=0) at lowlevellock.c:49
#2  0x00007ffff69bac12 in lll_mutex_lock_optimized (mutex=0x7ffff6e35f80
<__gnu_internal::get_mutex(unsigned char)::m>) at pthread_mutex_lock.c:48
#3  ___pthread_mutex_lock (mutex=0x7ffff6e35f80
<__gnu_internal::get_mutex(unsigned char)::m>) at pthread_mutex_lock.c:93
#4  0x00007ffff6cd2518 in __gthread_mutex_lock (__mutex=0x7ffff6e35f80
<__gnu_internal::get_mutex(unsigned char)::m>) at
/usr/src/debug/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:749#5
 __gnu_cxx::__mutex::lock (this=0x7ffff6e35f80
<__gnu_internal::get_mutex(unsigned char)::m>) at
/usr/src/debug/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/concurrence.h:149
#6  __gnu_cxx::__scoped_lock::__scoped_lock (__name=..., this=<synthetic
pointer>) at
/usr/src/debug/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/concurrence.h:241
#7  __gnu_debug::_Safe_iterator_base::_M_detach (this=0x7fffffffa5a8) at
/usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:408
#8  0x0000555557fd1608 in
__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<other_sections
const**, std::__cxx1998::vector<other_sections const*,
std::allocator<other_sections const*> > >, std::__debug::vector<other_sections
const*, std::allocator<other_sections const*> >,
std::forward_iterator_tag>::_Safe_iterator (this=0x7fffffffaab0, __x=...) at
/usr/include/c++/12.2.0/debug/safe_iterator.h:201
#9  0x0000555557fcf919 in
__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<other_sections
const**, std::__cxx1998::vector<other_sections const*,
std::allocator<other_sections const*> > >, std::__debug::vector<other_sections
const*, std::allocator<other_sections const*> >,
std::bidirectional_iterator_tag>::_Safe_iterator (this=0x7fffffffaab0) at
/usr/include/c++/12.2.0/debug/safe_iterator.h:550
#10 0x0000555557fcf93f in
__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<other_sections
const**, std::__cxx1998::vector<other_sections const*,
std::allocator<other_sections const*> > >, std::__debug::vector<other_sections
const*, std::allocator<other_sections const*> >,
std::random_access_iterator_tag>::_Safe_iterator (this=0x7fffffffaab0) at
/usr/include/c++/12.2.0/debug/safe_iterator.h:699
#11 0x0000555557fd2734 in
__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<other_sections
const**, std::__cxx1998::vector<other_sections const*,
std::allocator<other_sections const*> > >, std::__debug::vector<other_sections
const*, std::allocator<other_sections const*> >,
std::random_access_iterator_tag>::operator++ (this=0x7fffffffa7e0) at
/usr/include/c++/12.2.0/debug/safe_iterator.h:764
#12 0x0000555557fa6341 in addr_info_make_relative (addrs=0x7fffffffb8f0,
abfd=0x6120000ebfc0) at /home/simark/src/binutils-gdb/gdb/symfile.c:551
#13 0x0000555557fa8cee in syms_from_objfile_1 (objfile=0x614000007e40,
addrs=0x7fffffffb8f0, add_flags=...) at
/home/simark/src/binutils-gdb/gdb/symfile.c:957
#14 0x0000555557fa9106 in syms_from_objfile (objfile=0x614000007e40,
addrs=0x7fffffffb8f0, add_flags=...) at
/home/simark/src/binutils-gdb/gdb/symfile.c:985
(More stack frames follow...)

My theory is:

- frame #11 has acquired the lock at 0x7ffff6e35f80:

  (top-gdb) frame 11
  #11 0x0000555557fd2734 in
__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<other_sections
const**, std::__cxx1998::vector<other_sections const*,
std::allocator<other_sections const*> > >, std::__debug::vector<other_sections
const*, std::allocator<other_sections const*> >,
std::random_access_iterator_tag>::operator++ (this=0x7fffffffa7e0) at
/usr/include/c++/12.2.0/debug/safe_iterator.h:764
  764                                   _Attach_single());
  (top-gdb) print __l._M_device
  $12 = (__gnu_cxx::__scoped_lock::__mutex_type &) @0x7ffff6e35f80: {

- frame #7 is trying to acquire the same lock:

  (top-gdb) frame 7
  #7  __gnu_debug::_Safe_iterator_base::_M_detach (this=0x7fffffffa5a8) at
/usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:408
  408     /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc: Bad file
descriptor.
  (top-gdb) frame 6
  #6  __gnu_cxx::__scoped_lock::__scoped_lock (__name=..., this=<synthetic
pointer>) at
/usr/src/debug/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/concurrence.h:241
  241    
/usr/src/debug/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/concurrence.h:
Bad file descriptor.
  (top-gdb) p __name
  $13 = (__gnu_cxx::__scoped_lock::__mutex_type &) @0x7ffff6e35f80: {

- This isn't normally seen because the move construction of _Safe_iterator
(frame 10) would normally get elided.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-01-05
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I've tried to reproduce this, but it depends on the addresses of the
_Safe_iterator objects that get created, because they'll use different mutexes
unless their addresses collide in the hash function.

Would you be able to test this patch?

--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -761,12 +761,9 @@ namespace __gnu_debug
       _Safe_iterator
       operator++(int) _GLIBCXX_NOEXCEPT
       {
-       _GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(),
-                             _M_message(__msg_bad_inc)
-                             ._M_iterator(*this, "this"));
-       __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
-       return _Safe_iterator(this->base()++, this->_M_sequence,
-                             _Attach_single());
+       _Safe_iterator __ret = *this;
+       ++*this;
+       return __ret;
       }

       // ------ Bidirectional iterator requirements ------
@@ -788,12 +785,9 @@ namespace __gnu_debug
       _Safe_iterator
       operator--(int) _GLIBCXX_NOEXCEPT
       {
-       _GLIBCXX_DEBUG_VERIFY(this->_M_decrementable(),
-                             _M_message(__msg_bad_dec)
-                             ._M_iterator(*this, "this"));
-       __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
-       return _Safe_iterator(this->base()--, this->_M_sequence,
-                             _Attach_single());
+       _Safe_iterator __ret = *this;
+       --*this;
+       return __ret;
       }

       // ------ Random access iterator requirements ------

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I reproduced it by reducing the size of the mutex pool to one, so they always
collide. And the patch above seems to fix it. I'll test it properly and push.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This was my reproducer, although it doesn't deadlock without the hack to use a
single mutex for all objects.

#define _GLIBCXX_DEBUG 1
#include <vector>

int main()
{
  std::vector<int> v{1,2,3};
  auto i = v.begin();
  return *i++;
}

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[simon.marchi at polymtl dot ca]

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

--- Comment #4 from Simon Marchi <simon.marchi at polymtl dot ca> ---
Thanks for looking into this so quickly.  I'll try to test the patch against my
use case, but I might need some guidance.  I know how to build gcc and install
it in some non-default prefix.  But then, do I need to do something special
when building my program, to ensure that this libstdc++.so will be used,
instead of the system one?  Just set LD_LIBRARY_PATH appropriately, like for
any other lib?

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The patch doesn't change libstdc++.so, it only changes the headers.

You don't even need to rebuild GCC. You could just put a patched copy of
safe_iterator.h in /tmp/x/debug and then compile with -I/tmp/x so it finds the
patched header.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[simon.marchi at polymtl dot ca]

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

--- Comment #6 from Simon Marchi <simon.marchi at polymtl dot ca> ---
Because some code trying to acquire the lock (see frame #7 in my backtrace) is
in debug.cc, I thought it would maybe need to be changed too?  But I don't
really understand any of this.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
__gnu_debug::_Safe_iterator_base::_M_detach in debug.cc isn't affected by the
patch though.

The fix is to the post-inc and post-dec members, so only the code that calls
those needs to be recompiled.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[simon.marchi at polymtl dot ca]

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

--- Comment #8 from Simon Marchi <simon.marchi at polymtl dot ca> ---
I tested with just patching my /usr/include, and it looks like it works fine,
I'm able to run a program under my GDB.  Removing the fix, I get back the hang.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:b9479ddc7a28fb672ca67304a67d66524d8200a4

commit r13-5044-gb9479ddc7a28fb672ca67304a67d66524d8200a4
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jan 5 16:23:51 2023 +0000

    libstdc++: Fix deadlock in debug iterator increment [PR108288]

    With -fno-elide-constructors the debug iterator post-increment and
    post-decrement operators are susceptible to deadlock. They take a mutex
    lock and then return a temporary, which also attempts to take a lock to
    attach itself to the sequence. If the return value and *this happen to
    collide and use the same mutex from the pool, then you get a deadlock
    trying to lock a mutex that is already held by the current thread.

    The solution is to construct the return value before taking the lock.
    The copy constructor and pre-inc/pre-dec operators already manage locks
    correctly, without deadlock, so just implement post-inc/post-dec in the
    conventional way, taking a copy then modifying *this, then returning the
    copy.

    libstdc++-v3/ChangeLog:

            PR libstdc++/108288
            * include/debug/safe_iterator.h (_Safe_iterator::operator++(int))
            (_Safe_iterator::operator--(int)): Do not hold lock around
            construction of return value.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.5

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk, but it should be backported too.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Francois Dumont <fdumont@gcc.gnu.org>:

https://gcc.gnu.org/g:5c9833d878602de20043a5b74e4c6acd33d5fb3e

commit r13-5180-g5c9833d878602de20043a5b74e4c6acd33d5fb3e
Author: François Dumont <fdumont@gcc.gnu.org>
Date:   Mon Jan 9 18:48:30 2023 +0100

    libstdc++: [_GLIBCXX_DEBUG] Complete deadlock fix on safe iterators
[PR108288]

    Complete fix on all _Safe_iterator post-increment and post-decrement
implementations
    and on _Safe_local_iterator.

    libstdc++-v3/ChangeLog:

            PR libstdc++/108288
            * include/debug/safe_iterator.h
(_Safe_iterator<>::operator++(int)): Extend deadlock
            fix to other iterator category.
            (_Safe_iterator<>::operator--(int)): Likewise.
            * include/debug/safe_local_iterator.h
(_Safe_local_iterator<>::operator++(int)):
            Fix deadlock.
            * testsuite/util/debug/unordered_checks.h
(invalid_local_iterator_pre_increment): New.
            (invalid_local_iterator_post_increment): New.
            *
testsuite/23_containers/unordered_map/debug/invalid_local_iterator_post_increment_neg.cc:
            New test.
            *
testsuite/23_containers/unordered_map/debug/invalid_local_iterator_pre_increment_neg.cc:
            New test.

[Bug libstdc++/108288] Deadlock when using -fno-elide-constructor + -D_GLIBCXX_DEBUG=1 + -std=c++11

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5