[2/2] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close (PR gdb/28080)

Message ID 20210712193311.837489-3-pedro@palves.net
State New
Headers show
Series
  • Fix detach with target remote (PR gdb/28080)
Related show

Commit Message

Pedro Alves July 12, 2021, 7:33 p.m.
Before PR gdb/28080 was fixed by the previous patch, GDB was crashing
like this:

 (gdb) detach
 Detaching from program: target:/any/program, process 3671843
 Detaching from process 3671843
 Ending remote debugging.
 [Inferior 1 (process 3671843) detached]
 In main
 terminate called after throwing an instance of 'gdb_exception_error'
 Aborted (core dumped)

Here's the exception above being thrown:

 (top-gdb) bt
 #0  throw_error (error=TARGET_CLOSE_ERROR, fmt=0x555556035588 "Remote connection closed") at src/gdbsupport/common-exceptions.cc:222
 #1  0x0000555555bbaa46 in remote_target::readchar (this=0x555556a11040, timeout=10000) at src/gdb/remote.c:9440
 #2  0x0000555555bbb9e5 in remote_target::getpkt_or_notif_sane_1 (this=0x555556a11040, buf=0x555556a11058, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9928
 #3  0x0000555555bbbda9 in remote_target::getpkt_sane (this=0x555556a11040, buf=0x555556a11058, forever=0) at src/gdb/remote.c:10030
 #4  0x0000555555bc0e75 in remote_target::remote_hostio_send_command (this=0x555556a11040, command_bytes=13, which_packet=14, remote_errno=0x7fffffffcfd0, attachment=0x0, attachment_len=0x0) at src/gdb/remote.c:12137
 #5  0x0000555555bc1b6c in remote_target::remote_hostio_close (this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12455
 #6  0x0000555555bc1bb4 in remote_target::fileio_close (During symbol reading: .debug_line address at offset 0x64f417 is 0 [in module build/gdb/gdb]
 this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12462
 #7  0x0000555555c9274c in target_fileio_close (fd=3, target_errno=0x7fffffffcfd0) at src/gdb/target.c:3365
 #8  0x000055555595a19d in gdb_bfd_iovec_fileio_close (abfd=0x555556b9f8a0, stream=0x555556b11530) at src/gdb/gdb_bfd.c:439
 #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556b9f8a0) at src/bfd/opncls.c:599
 #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556b9f8a0) at src/bfd/opncls.c:847
 #11 0x0000555555e0a27a in bfd_close (abfd=0x555556b9f8a0) at src/bfd/opncls.c:814
 #12 0x000055555595a9d3 in gdb_bfd_close_or_warn (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:626
 #13 0x000055555595ad29 in gdb_bfd_unref (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:715
 #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573
 #15 0x0000555555ae955a in std::_Sp_counted_ptr<objfile*, (__gnu_cxx::_Lock_policy)2>::_M_dispose (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:377
 #16 0x000055555572b7c8 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:155
 #17 0x00005555557263c3 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x555556bf0588, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:730
 #18 0x0000555555ae745e in std::__shared_ptr<objfile, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:1169
 #19 0x0000555555ae747e in std::shared_ptr<objfile>::~shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr.h:103
 #20 0x0000555555b1c1dc in __gnu_cxx::new_allocator<std::_List_node<std::shared_ptr<objfile> > >::destroy<std::shared_ptr<objfile> > (this=0x5555564cdd60, __p=0x555556bf0580) at /usr/include/c++/9/ext/new_allocator.h:153
 #21 0x0000555555b1bb1d in std::allocator_traits<std::allocator<std::_List_node<std::shared_ptr<objfile> > > >::destroy<std::shared_ptr<objfile> > (__a=..., __p=0x555556bf0580) at /usr/include/c++/9/bits/alloc_traits.h:497
 #22 0x0000555555b1b73e in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::_M_erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/stl_list.h:1921
 #23 0x0000555555b1afeb in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/list.tcc:158
 #24 0x0000555555b19576 in program_space::remove_objfile (this=0x5555564cdd20, objfile=0x555556515540) at src/gdb/progspace.c:210
 #25 0x0000555555ae4502 in objfile::unlink (this=0x555556515540) at src/gdb/objfiles.c:487
 #26 0x0000555555ae5a12 in objfile_purge_solibs () at src/gdb/objfiles.c:875
 #27 0x0000555555c09686 in no_shared_libraries (ignored=0x0, from_tty=1) at src/gdb/solib.c:1236
 #28 0x00005555559e3f5f in detach_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:2769

Note frame #14:

 #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573

That's a dtor, thus noexcept.  That's the reason for the
std::terminate.

The previous patch fixed things such that the exception above isn't
thrown anymore.  However, it's possible that e.g., the remote
connection drops just while a user types "nosharedlibrary", or some
other reason that leads to objfile::~objfile, and then we end up the
same std::terminate problem.

Also notice that frames #9-#11 are BFD frames:

 #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556bc27e0) at src/bfd/opncls.c:599
 #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556bc27e0) at src/bfd/opncls.c:847
 #11 0x0000555555e0a27a in bfd_close (abfd=0x555556bc27e0) at src/bfd/opncls.c:814

BFD is written in C and thus throwing exceptions over such frames may
either not clean up properly, or, may abort if bfd is not compiled
with -​fasynchronous-unwind-tables (x86-64 defaults that on, but not
all GCC ports do).

Thus frame #8 sees like a good place to swallow exceptions.  More so
since in this spot we already close returning error.  That's what this
commit does.  Without the previous fix, we'd see:

 (gdb) detach
 Detaching from program: target:/any/program, process 2197701
 Ending remote debugging.
 [Inferior 1 (process 2197701) detached]
 warning: while closing file: Remote connection closed

Note it prints a warning, which would still be a regression compared
to GDB 10, if it weren't for the previous fix.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	PR gdb/28080
	* gdb_bfd.c (gdb_bfd_iovec_fileio_close): Wrap target_fileio_close
	in try/catch and print warning on exception.

Change-Id: Ic7a26ddba0a4444e3377b0e7c1c89934a84545d7
---
 gdb/gdb_bfd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
2.26.2

Comments

Tom de Vries via Gdb-patches July 13, 2021, 1:19 a.m. | #1
On 2021-07-12 3:33 p.m., Pedro Alves wrote:
> Before PR gdb/28080 was fixed by the previous patch, GDB was crashing

> like this:

> 

>  (gdb) detach

>  Detaching from program: target:/any/program, process 3671843

>  Detaching from process 3671843

>  Ending remote debugging.

>  [Inferior 1 (process 3671843) detached]

>  In main

>  terminate called after throwing an instance of 'gdb_exception_error'

>  Aborted (core dumped)

> 

> Here's the exception above being thrown:

> 

>  (top-gdb) bt

>  #0  throw_error (error=TARGET_CLOSE_ERROR, fmt=0x555556035588 "Remote connection closed") at src/gdbsupport/common-exceptions.cc:222

>  #1  0x0000555555bbaa46 in remote_target::readchar (this=0x555556a11040, timeout=10000) at src/gdb/remote.c:9440

>  #2  0x0000555555bbb9e5 in remote_target::getpkt_or_notif_sane_1 (this=0x555556a11040, buf=0x555556a11058, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9928

>  #3  0x0000555555bbbda9 in remote_target::getpkt_sane (this=0x555556a11040, buf=0x555556a11058, forever=0) at src/gdb/remote.c:10030

>  #4  0x0000555555bc0e75 in remote_target::remote_hostio_send_command (this=0x555556a11040, command_bytes=13, which_packet=14, remote_errno=0x7fffffffcfd0, attachment=0x0, attachment_len=0x0) at src/gdb/remote.c:12137

>  #5  0x0000555555bc1b6c in remote_target::remote_hostio_close (this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12455

>  #6  0x0000555555bc1bb4 in remote_target::fileio_close (During symbol reading: .debug_line address at offset 0x64f417 is 0 [in module build/gdb/gdb]

>  this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12462

>  #7  0x0000555555c9274c in target_fileio_close (fd=3, target_errno=0x7fffffffcfd0) at src/gdb/target.c:3365

>  #8  0x000055555595a19d in gdb_bfd_iovec_fileio_close (abfd=0x555556b9f8a0, stream=0x555556b11530) at src/gdb/gdb_bfd.c:439

>  #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556b9f8a0) at src/bfd/opncls.c:599

>  #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556b9f8a0) at src/bfd/opncls.c:847

>  #11 0x0000555555e0a27a in bfd_close (abfd=0x555556b9f8a0) at src/bfd/opncls.c:814

>  #12 0x000055555595a9d3 in gdb_bfd_close_or_warn (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:626

>  #13 0x000055555595ad29 in gdb_bfd_unref (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:715

>  #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573

>  #15 0x0000555555ae955a in std::_Sp_counted_ptr<objfile*, (__gnu_cxx::_Lock_policy)2>::_M_dispose (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:377

>  #16 0x000055555572b7c8 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:155

>  #17 0x00005555557263c3 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x555556bf0588, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:730

>  #18 0x0000555555ae745e in std::__shared_ptr<objfile, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:1169

>  #19 0x0000555555ae747e in std::shared_ptr<objfile>::~shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr.h:103

>  #20 0x0000555555b1c1dc in __gnu_cxx::new_allocator<std::_List_node<std::shared_ptr<objfile> > >::destroy<std::shared_ptr<objfile> > (this=0x5555564cdd60, __p=0x555556bf0580) at /usr/include/c++/9/ext/new_allocator.h:153

>  #21 0x0000555555b1bb1d in std::allocator_traits<std::allocator<std::_List_node<std::shared_ptr<objfile> > > >::destroy<std::shared_ptr<objfile> > (__a=..., __p=0x555556bf0580) at /usr/include/c++/9/bits/alloc_traits.h:497

>  #22 0x0000555555b1b73e in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::_M_erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/stl_list.h:1921

>  #23 0x0000555555b1afeb in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/list.tcc:158

>  #24 0x0000555555b19576 in program_space::remove_objfile (this=0x5555564cdd20, objfile=0x555556515540) at src/gdb/progspace.c:210

>  #25 0x0000555555ae4502 in objfile::unlink (this=0x555556515540) at src/gdb/objfiles.c:487

>  #26 0x0000555555ae5a12 in objfile_purge_solibs () at src/gdb/objfiles.c:875

>  #27 0x0000555555c09686 in no_shared_libraries (ignored=0x0, from_tty=1) at src/gdb/solib.c:1236

>  #28 0x00005555559e3f5f in detach_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:2769

> 

> Note frame #14:

> 

>  #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573

> 

> That's a dtor, thus noexcept.  That's the reason for the

> std::terminate.

> 

> The previous patch fixed things such that the exception above isn't

> thrown anymore.  However, it's possible that e.g., the remote

> connection drops just while a user types "nosharedlibrary", or some

> other reason that leads to objfile::~objfile, and then we end up the

> same std::terminate problem.

> 

> Also notice that frames #9-#11 are BFD frames:

> 

>  #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556bc27e0) at src/bfd/opncls.c:599

>  #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556bc27e0) at src/bfd/opncls.c:847

>  #11 0x0000555555e0a27a in bfd_close (abfd=0x555556bc27e0) at src/bfd/opncls.c:814

> 

> BFD is written in C and thus throwing exceptions over such frames may

> either not clean up properly, or, may abort if bfd is not compiled

> with -​fasynchronous-unwind-tables (x86-64 defaults that on, but not


Weird characters on the line above, just before fasynchronous.

> all GCC ports do).

> 

> Thus frame #8 sees like a good place to swallow exceptions.  More so


sees -> seems

> since in this spot we already close returning error.  That's what this


I don't understand this sentence, "in this spot we already close
returning error".

> commit does.  Without the previous fix, we'd see:

> 

>  (gdb) detach

>  Detaching from program: target:/any/program, process 2197701

>  Ending remote debugging.

>  [Inferior 1 (process 2197701) detached]

>  warning: while closing file: Remote connection closed

> 

> Note it prints a warning, which would still be a regression compared

> to GDB 10, if it weren't for the previous fix.


Maybe the more correct thing to do would be to make
gdb_bfd_iovec_fileio_close return an error when it can't properly close
the file.  That error would be communicated as a bfd return status to
gdb_bfd_close_or_warn, which would warn.

If we need to catch exceptions for the close operations, I suppose we
need to do that as well for the other BFD iovec operations: open, pread
and stat?  A remote communication error could happen in any of those.

Simon
Pedro Alves July 13, 2021, 12:16 p.m. | #2
On 2021-07-13 2:19 a.m., Simon Marchi wrote:
> 

> On 2021-07-12 3:33 p.m., Pedro Alves wrote:

>> BFD is written in C and thus throwing exceptions over such frames may

>> either not clean up properly, or, may abort if bfd is not compiled

>> with -​fasynchronous-unwind-tables (x86-64 defaults that on, but not

> 

> Weird characters on the line above, just before fasynchronous.


Curious, must be the result of copy/paste from the browser, for I googled the
correct spelling.  They're not visible in emacs nor my Thunderbird, nor in the
archives:
 https://sourceware.org/pipermail/gdb-patches/2021-July/180849.html
but I see them in the raw email source.  Fixed.

> 

>> all GCC ports do).

>>

>> Thus frame #8 sees like a good place to swallow exceptions.  More so

> 

> sees -> seems


Fixed.

> 

>> since in this spot we already close returning error.  That's what this

> 

> I don't understand this sentence, "in this spot we already close

> returning error".


That's because the "ignore" word is missing... :-P

I've tweaked it to:

    Thus frame #8 seems like a good place to swallow exceptions.  More so
    since in this spot we already ignore target_fileio_close return
    errors.  That's what this commit does.  Without the previous fix, we'd
    see:

> 

>> commit does.  Without the previous fix, we'd see:

>>

>>  (gdb) detach

>>  Detaching from program: target:/any/program, process 2197701

>>  Ending remote debugging.

>>  [Inferior 1 (process 2197701) detached]

>>  warning: while closing file: Remote connection closed

>>

>> Note it prints a warning, which would still be a regression compared

>> to GDB 10, if it weren't for the previous fix.

> 

> Maybe the more correct thing to do would be to make

> gdb_bfd_iovec_fileio_close return an error when it can't properly close

> the file.  That error would be communicated as a bfd return status to

> gdb_bfd_close_or_warn, which would warn.


The problem with that is that it would require losing the exception
message, because the warning gdb_bfd_close_or_warn prints is just the
text version of a bfd error:

  if (!ret)
    warning (_("cannot close \"%s\": %s"),
	     name, bfd_errmsg (bfd_get_error ()));

Maybe we could add a new type of bfd error, a custom error, where the error
reason is given by callback (which gdb would install) or a string or some such, so
we could pass the gdb error through bfd.  Not that different from how
bfd_error_on_input is a special error code too.  But I'd rather not do this now.

I've tweaked the patch to ensure that the new warning looks exactly like the
one in gdb_bfd_close_or_warn, though.  See below.

> 

> If we need to catch exceptions for the close operations, I suppose we

> need to do that as well for the other BFD iovec operations: open, pread

> and stat?  A remote communication error could happen in any of those.


Yes, though I don't think they're as likely to be in dtor/noexcept paths,
and it isn't as obvious to me what the return code should be.  With close,
there's not much else we can do other than ignore the error.  I'd rather
not touch them for now, for time constraint reasons.  :-/

If you'd prefer, I can drop this patch for now.  The issue addressed by this
patch isn't a regression.

From 56f2e825c7800eb226a916bc33e3948e2c02fa27 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>

Date: Mon, 12 Jul 2021 18:03:22 +0100
Subject: [PATCH] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close
 (PR gdb/28080)

Before PR gdb/28080 was fixed by the previous patch, GDB was crashing
like this:

 (gdb) detach
 Detaching from program: target:/any/program, process 3671843
 Detaching from process 3671843
 Ending remote debugging.
 [Inferior 1 (process 3671843) detached]
 In main
 terminate called after throwing an instance of 'gdb_exception_error'
 Aborted (core dumped)

Here's the exception above being thrown:

 (top-gdb) bt
 #0  throw_error (error=TARGET_CLOSE_ERROR, fmt=0x555556035588 "Remote connection closed") at src/gdbsupport/common-exceptions.cc:222
 #1  0x0000555555bbaa46 in remote_target::readchar (this=0x555556a11040, timeout=10000) at src/gdb/remote.c:9440
 #2  0x0000555555bbb9e5 in remote_target::getpkt_or_notif_sane_1 (this=0x555556a11040, buf=0x555556a11058, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9928
 #3  0x0000555555bbbda9 in remote_target::getpkt_sane (this=0x555556a11040, buf=0x555556a11058, forever=0) at src/gdb/remote.c:10030
 #4  0x0000555555bc0e75 in remote_target::remote_hostio_send_command (this=0x555556a11040, command_bytes=13, which_packet=14, remote_errno=0x7fffffffcfd0, attachment=0x0, attachment_len=0x0) at src/gdb/remote.c:12137
 #5  0x0000555555bc1b6c in remote_target::remote_hostio_close (this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12455
 #6  0x0000555555bc1bb4 in remote_target::fileio_close (During symbol reading: .debug_line address at offset 0x64f417 is 0 [in module build/gdb/gdb]
 this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12462
 #7  0x0000555555c9274c in target_fileio_close (fd=3, target_errno=0x7fffffffcfd0) at src/gdb/target.c:3365
 #8  0x000055555595a19d in gdb_bfd_iovec_fileio_close (abfd=0x555556b9f8a0, stream=0x555556b11530) at src/gdb/gdb_bfd.c:439
 #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556b9f8a0) at src/bfd/opncls.c:599
 #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556b9f8a0) at src/bfd/opncls.c:847
 #11 0x0000555555e0a27a in bfd_close (abfd=0x555556b9f8a0) at src/bfd/opncls.c:814
 #12 0x000055555595a9d3 in gdb_bfd_close_or_warn (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:626
 #13 0x000055555595ad29 in gdb_bfd_unref (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:715
 #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573
 #15 0x0000555555ae955a in std::_Sp_counted_ptr<objfile*, (__gnu_cxx::_Lock_policy)2>::_M_dispose (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:377
 #16 0x000055555572b7c8 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:155
 #17 0x00005555557263c3 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x555556bf0588, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:730
 #18 0x0000555555ae745e in std::__shared_ptr<objfile, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:1169
 #19 0x0000555555ae747e in std::shared_ptr<objfile>::~shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr.h:103
 #20 0x0000555555b1c1dc in __gnu_cxx::new_allocator<std::_List_node<std::shared_ptr<objfile> > >::destroy<std::shared_ptr<objfile> > (this=0x5555564cdd60, __p=0x555556bf0580) at /usr/include/c++/9/ext/new_allocator.h:153
 #21 0x0000555555b1bb1d in std::allocator_traits<std::allocator<std::_List_node<std::shared_ptr<objfile> > > >::destroy<std::shared_ptr<objfile> > (__a=..., __p=0x555556bf0580) at /usr/include/c++/9/bits/alloc_traits.h:497
 #22 0x0000555555b1b73e in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::_M_erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/stl_list.h:1921
 #23 0x0000555555b1afeb in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/list.tcc:158
 #24 0x0000555555b19576 in program_space::remove_objfile (this=0x5555564cdd20, objfile=0x555556515540) at src/gdb/progspace.c:210
 #25 0x0000555555ae4502 in objfile::unlink (this=0x555556515540) at src/gdb/objfiles.c:487
 #26 0x0000555555ae5a12 in objfile_purge_solibs () at src/gdb/objfiles.c:875
 #27 0x0000555555c09686 in no_shared_libraries (ignored=0x0, from_tty=1) at src/gdb/solib.c:1236
 #28 0x00005555559e3f5f in detach_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:2769

Note frame #14:

 #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573

That's a dtor, thus noexcept.  That's the reason for the
std::terminate.

The previous patch fixed things such that the exception above isn't
thrown anymore.  However, it's possible that e.g., the remote
connection drops just while a user types "nosharedlibrary", or some
other reason that leads to objfile::~objfile, and then we end up the
same std::terminate problem.

Also notice that frames #9-#11 are BFD frames:

 #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556bc27e0) at src/bfd/opncls.c:599
 #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556bc27e0) at src/bfd/opncls.c:847
 #11 0x0000555555e0a27a in bfd_close (abfd=0x555556bc27e0) at src/bfd/opncls.c:814

BFD is written in C and thus throwing exceptions over such frames may
either not clean up properly, or, may abort if bfd is not compiled
with -fasynchronous-unwind-tables (x86-64 defaults that on, but not
all GCC ports do).

Thus frame #8 seems like a good place to swallow exceptions.  More so
since in this spot we already ignore target_fileio_close return
errors.  That's what this commit does.  Without the previous fix, we'd
see:

 (gdb) detach
 Detaching from program: target:/any/program, process 2197701
 Ending remote debugging.
 [Inferior 1 (process 2197701) detached]
 warning: cannot close "target:/lib64/ld-linux-x86-64.so.2": Remote connection closed

Note it prints a warning, which would still be a regression compared
to GDB 10, if it weren't for the previous fix.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	PR gdb/28080
	* gdb_bfd.c (gdb_bfd_close_warning): New.
	(gdb_bfd_iovec_fileio_close): Wrap target_fileio_close in
	try/catch and print warning on exception.
	(gdb_bfd_close_or_warn): Use gdb_bfd_close_warning.

Change-Id: Ic7a26ddba0a4444e3377b0e7c1c89934a84545d7
---
 gdb/gdb_bfd.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 3312197acd5..312442a466e 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -423,6 +423,15 @@ gdb_bfd_iovec_fileio_pread (struct bfd *abfd, void *stream, void *buf,
   return pos;
 }
 
+/* Warn that it wasn't possible to close a bfd for file NAME, because
+   of REASON.  */
+
+static void
+gdb_bfd_close_warning (const char *name, const char *reason)
+{
+  warning (_("cannot close \"%s\": %s"), name, reason);
+}
+
 /* Wrapper for target_fileio_close suitable for passing as the
    CLOSE_FUNC argument to gdb_bfd_openr_iovec.  */
 
@@ -436,7 +445,16 @@ gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream)
 
   /* Ignore errors on close.  These may happen with remote
      targets if the connection has already been torn down.  */
-  target_fileio_close (fd, &target_errno);
+  try
+    {
+      target_fileio_close (fd, &target_errno);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Also avoid crossing exceptions over bfd.  */
+      gdb_bfd_close_warning (bfd_get_filename (abfd),
+			     ex.message->c_str ());
+    }
 
   /* Zero means success.  */
   return 0;
@@ -626,8 +644,8 @@ gdb_bfd_close_or_warn (struct bfd *abfd)
   ret = bfd_close (abfd);
 
   if (!ret)
-    warning (_("cannot close \"%s\": %s"),
-	     name, bfd_errmsg (bfd_get_error ()));
+    gdb_bfd_close_warning (name,
+			   bfd_errmsg (bfd_get_error ()));
 
   return ret;
 }

base-commit: 6bbe1a929c69e141e63265c7013fcf7f80bc792e
-- 
2.26.2
Tom de Vries via Gdb-patches July 13, 2021, 1:13 p.m. | #3
>>> since in this spot we already close returning error.  That's what this

>>

>> I don't understand this sentence, "in this spot we already close

>> returning error".

> 

> That's because the "ignore" word is missing... :-P

> 

> I've tweaked it to:

> 

>     Thus frame #8 seems like a good place to swallow exceptions.  More so

>     since in this spot we already ignore target_fileio_close return

>     errors.  That's what this commit does.  Without the previous fix, we'd

>     see:


Ah, I see.  Well then it sounds like if target_fileio_close returns an
error, that error should be returned by gdb_bfd_iovec_fileio_close as
well (eventually, not in this patch).

>> Maybe the more correct thing to do would be to make

>> gdb_bfd_iovec_fileio_close return an error when it can't properly close

>> the file.  That error would be communicated as a bfd return status to

>> gdb_bfd_close_or_warn, which would warn.

> 

> The problem with that is that it would require losing the exception

> message, because the warning gdb_bfd_close_or_warn prints is just the

> text version of a bfd error:

> 

>   if (!ret)

>     warning (_("cannot close \"%s\": %s"),

> 	     name, bfd_errmsg (bfd_get_error ()));

> 

> Maybe we could add a new type of bfd error, a custom error, where the error

> reason is given by callback (which gdb would install) or a string or some such, so

> we could pass the gdb error through bfd.  Not that different from how

> bfd_error_on_input is a special error code too.  But I'd rather not do this now.


Ah, I see.  A proper solution involving BFD would be good, but we could
also bypass BFD and store the error string somewhere where we can pick
it up again in gdb_bfd_close_or_warn.

But yeah I'm fine with what you have, it's certainly not worse than the
current state.

> 

> I've tweaked the patch to ensure that the new warning looks exactly like the

> one in gdb_bfd_close_or_warn, though.  See below.

> 

>>

>> If we need to catch exceptions for the close operations, I suppose we

>> need to do that as well for the other BFD iovec operations: open, pread

>> and stat?  A remote communication error could happen in any of those.

> 

> Yes, though I don't think they're as likely to be in dtor/noexcept paths,

> and it isn't as obvious to me what the return code should be.  With close,

> there's not much else we can do other than ignore the error.  I'd rather

> not touch them for now, for time constraint reasons.  :-/


I thought an issue (which you talk about in your commit message) was for
the exception to go over BFD frames, which may not have been compiled
with -fasynchronous-unwind-tables.  That could happens with any of the
callbacks.

> If you'd prefer, I can drop this patch for now.  The issue addressed by this

> patch isn't a regression.


I'm fine with what you have, it makes the situation a bit better.  I
just wanted to know whether the other callbacks should eventually be
converted to do the same thing, to document the situation for future us.

Simon
Pedro Alves July 13, 2021, 1:25 p.m. | #4
On 2021-07-13 2:13 p.m., Simon Marchi wrote:
>>>> since in this spot we already close returning error.  That's what this

>>>

>>> I don't understand this sentence, "in this spot we already close

>>> returning error".

>>

>> That's because the "ignore" word is missing... :-P

>>

>> I've tweaked it to:

>>

>>     Thus frame #8 seems like a good place to swallow exceptions.  More so

>>     since in this spot we already ignore target_fileio_close return

>>     errors.  That's what this commit does.  Without the previous fix, we'd

>>     see:

> 

> Ah, I see.  Well then it sounds like if target_fileio_close returns an

> error, that error should be returned by gdb_bfd_iovec_fileio_close as

> well (eventually, not in this patch).

> 

>>> Maybe the more correct thing to do would be to make

>>> gdb_bfd_iovec_fileio_close return an error when it can't properly close

>>> the file.  That error would be communicated as a bfd return status to

>>> gdb_bfd_close_or_warn, which would warn.

>>

>> The problem with that is that it would require losing the exception

>> message, because the warning gdb_bfd_close_or_warn prints is just the

>> text version of a bfd error:

>>

>>   if (!ret)

>>     warning (_("cannot close \"%s\": %s"),

>> 	     name, bfd_errmsg (bfd_get_error ()));

>>

>> Maybe we could add a new type of bfd error, a custom error, where the error

>> reason is given by callback (which gdb would install) or a string or some such, so

>> we could pass the gdb error through bfd.  Not that different from how

>> bfd_error_on_input is a special error code too.  But I'd rather not do this now.

> 

> Ah, I see.  A proper solution involving BFD would be good, but we could

> also bypass BFD and store the error string somewhere where we can pick

> it up again in gdb_bfd_close_or_warn.


And just pick a bfd_error_xxx constant, like e.g., bfd_error_system_call?
Then, if the GDB global is set, we know it's really a GDB error.  That would
work, I think, yeah.  As long as BFD itself doesn't look at the error to
decide anything, I guess.

> 

> But yeah I'm fine with what you have, it's certainly not worse than the

> current state.

> 


OK, I'll go merge the patches then.

>>

>> I've tweaked the patch to ensure that the new warning looks exactly like the

>> one in gdb_bfd_close_or_warn, though.  See below.

>>

>>>

>>> If we need to catch exceptions for the close operations, I suppose we

>>> need to do that as well for the other BFD iovec operations: open, pread

>>> and stat?  A remote communication error could happen in any of those.

>>

>> Yes, though I don't think they're as likely to be in dtor/noexcept paths,

>> and it isn't as obvious to me what the return code should be.  With close,

>> there's not much else we can do other than ignore the error.  I'd rather

>> not touch them for now, for time constraint reasons.  :-/

> 

> I thought an issue (which you talk about in your commit message) was for

> the exception to go over BFD frames, which may not have been compiled

> with -fasynchronous-unwind-tables.  That could happens with any of the

> callbacks.


Yeah, there's that issue, and the issue with reaching the ~objfile dtor.
The BFD issue doesn't trigger on x86-64, while the dtor one triggers everywhere.

> 

>> If you'd prefer, I can drop this patch for now.  The issue addressed by this

>> patch isn't a regression.

> 

> I'm fine with what you have, it makes the situation a bit better.  I

> just wanted to know whether the other callbacks should eventually be

> converted to do the same thing, to document the situation for future us.

Patch

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 3312197acd5..267a53a0edc 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -436,7 +436,15 @@  gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream)
 
   /* Ignore errors on close.  These may happen with remote
      targets if the connection has already been torn down.  */
-  target_fileio_close (fd, &target_errno);
+  try
+    {
+      target_fileio_close (fd, &target_errno);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Also avoid crossing exceptions over bfd.  */
+      warning (_("while closing file: %s"), ex.message->c_str ());
+    }
 
   /* Zero means success.  */
   return 0;