From d7cb0ef35b112e35a3e9b4dc30ffd800c9d0a4fe Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 12 Jul 2021 17:10:48 +0100 Subject: [PATCH] Fix detach with target remote (PR gdb/28080) Commit 408f66864a1a823591b26420410c982174c239a2 ("detach in all-stop with threads running") regressed "detach" with "target remote": (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=) at src/gdb/objfiles.c:573 #15 0x0000555555ae955a in std::_Sp_counted_ptr::_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=) at /usr/include/c++/9/bits/shared_ptr_base.h:730 #18 0x0000555555ae745e in std::__shared_ptr::~__shared_ptr (this=0x555556bf0580, __in_chrg=) at /usr/include/c++/9/bits/shared_ptr_base.h:1169 #19 0x0000555555ae747e in std::shared_ptr::~shared_ptr (this=0x555556bf0580, __in_chrg=) at /usr/include/c++/9/bits/shared_ptr.h:103 #20 0x0000555555b1c1dc in __gnu_cxx::new_allocator > >::destroy > (this=0x5555564cdd60, __p=0x555556bf0580) at /usr/include/c++/9/ext/new_allocator.h:153 #21 0x0000555555b1bb1d in std::allocator_traits > > >::destroy > (__a=..., __p=0x555556bf0580) at /usr/include/c++/9/bits/alloc_traits.h:497 #22 0x0000555555b1b73e in std::__cxx11::list, std::allocator > >::_M_erase (this=0x5555564cdd60, __position=std::shared_ptr (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/stl_list.h:1921 #23 0x0000555555b1afeb in std::__cxx11::list, std::allocator > >::erase (this=0x5555564cdd60, __position=std::shared_ptr (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 So frame #28 already detached the remote process, and then we're purging the shared libraries. GDB had opened remote shared libraries via the target: sysroot, so it tries closing them. GDBserver is tearing down already, so remote communication breaks down and we close the remote target and throw TARGET_CLOSE_ERROR. Note frame #14: #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=) at src/gdb/objfiles.c:573 That's a dtor, thus noexcept. That's the reason for the std::terminate. Stepping back a bit, why do we still have open remote files if we've managed to detach already, and, we're debugging with "target remote"? The reason is that commit 408f66864a1a823591b26420410c982174c239a2 makes detach_command hold a reference to the target, so the remote target won't be finally closed until frame #28 returns. It's closing the target that invalidates target file I/O handles. This commit fixes the issue by not relying on target_close to invalidate the target file I/O handles, instead invalidate them immediately in remote_unpush_target. So when GDB purges the solibs, and we end up in target_fileio_close (frame #7 above), there's nothing to do, and we don't try to talk with the remote target anymore. The regression isn't seen when testing with --target_board=native-gdbserver, because that does "set sysroot" to disable the "target:" sysroot, for test run speed reasons. So this commit adds a testcase that explicitly tests detach with "set sysroot target:". gdb/ChangeLog: yyyy-mm-dd Pedro Alves PR gdb/28080 * remote.c (remote_unpush_target): Invalidate file I/O target handles. * target.c (fileio_handles_invalidate_target): Make extern. * target.h (fileio_handles_invalidate_target): Declare. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves PR gdb/28080 * gdb.base/detach-sysroot-target.exp: New. * gdb.base/detach-sysroot-target.c: New. Reported-By: Jonah Graham Change-Id: I851234910172f42a1b30e731161376c344d2727d --- gdb/remote.c | 9 +++ gdb/target.c | 8 +-- gdb/target.h | 6 ++ .../gdb.base/detach-sysroot-target.c | 22 +++++++ .../gdb.base/detach-sysroot-target.exp | 59 +++++++++++++++++++ 5 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 gdb/testsuite/gdb.base/detach-sysroot-target.c create mode 100644 gdb/testsuite/gdb.base/detach-sysroot-target.exp diff --git a/gdb/remote.c b/gdb/remote.c index 6d674c8139f..96e2750cd32 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -5636,6 +5636,15 @@ remote_unpush_target (remote_target *target) pop_all_targets_at_and_above (process_stratum); generic_mourn_inferior (); } + + /* Don't rely on target_close doing this when the target is popped + from the last remote inferior above, because something may be + holding a reference to the target higher up on the stack, meaning + target_close won't be called yet. We lost the connection to the + target, so clear these now, otherwise we may later throw + TARGET_CLOSE_ERROR while trying to tell the remote target to + close the file. */ + fileio_handles_invalidate_target (target); } static void diff --git a/gdb/target.c b/gdb/target.c index 767685fbca3..78752898191 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -3121,13 +3121,9 @@ static std::vector fileio_fhandles; list each time a new file is opened. */ static int lowest_closed_fd; -/* Invalidate the target associated with open handles that were open - on target TARG, since we're about to close (and maybe destroy) the - target. The handles remain open from the client's perspective, but - trying to do anything with them other than closing them will fail - with EIO. */ +/* See target.h. */ -static void +void fileio_handles_invalidate_target (target_ops *targ) { for (fileio_fh_t &fh : fileio_fhandles) diff --git a/gdb/target.h b/gdb/target.h index e22f9038197..ddd4f3803cb 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -2230,6 +2230,12 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf, extern gdb::unique_xmalloc_ptr target_fileio_read_stralloc (struct inferior *inf, const char *filename); +/* Invalidate the target associated with open handles that were open + on target TARG, since we're about to close (and maybe destroy) the + target. The handles remain open from the client's perspective, but + trying to do anything with them other than closing them will fail + with EIO. */ +extern void fileio_handles_invalidate_target (target_ops *targ); /* Tracepoint-related operations. */ diff --git a/gdb/testsuite/gdb.base/detach-sysroot-target.c b/gdb/testsuite/gdb.base/detach-sysroot-target.c new file mode 100644 index 00000000000..9811b15f06d --- /dev/null +++ b/gdb/testsuite/gdb.base/detach-sysroot-target.c @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2021 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +int +main () +{ + return 0; +} diff --git a/gdb/testsuite/gdb.base/detach-sysroot-target.exp b/gdb/testsuite/gdb.base/detach-sysroot-target.exp new file mode 100644 index 00000000000..9ae1eb6277a --- /dev/null +++ b/gdb/testsuite/gdb.base/detach-sysroot-target.exp @@ -0,0 +1,59 @@ +# Copyright 2021 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . */ + +# Test running a program from the GDB prompt and then detaching it, +# with "set sysroot target:". Regression test for PR gdb/28080. +# +# It is assumed that it is only safe to tweak the sysroot on the +# current board if it is currently either empty or already "target:". +# If set to anything else, we skip the test. + +standard_testfile + +if {[prepare_for_testing "failed to prepare" ${binfile} ${srcfile}]} { + return +} + +gdb_test_multiple "show sysroot" "" { + -wrap -re "The current system root is \"\"\\." { + pass $gdb_test_name + + # Explicitly set target: sysroot. + gdb_test_no_output "set sysroot target:" + } + -wrap -re "The current system root is \"target:\"\\." { + pass $gdb_test_name + + # Nothing else to do. + } + -re "$gdb_prompt $" { + pass $gdb_test_name + + # If testing with any other sysroot, we probably should not + # mess with it. + unsupported "sysroot is set" + return + } +} + +if ![runto_main] { + fail "couldn't run to main" + return +} + +# With PR gdb/28080, this would crash GDB when testing with "target +# remote". +set escapedbinfile [string_to_regexp ${binfile}] +gdb_test "detach" "Detaching from program: .*$escapedbinfile, .*"