From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by sourceware.org (Postfix) with ESMTPS id 3BA563953C05 for ; Tue, 13 Jul 2021 12:16:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3BA563953C05 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f50.google.com with SMTP id l4-20020a05600c4f04b0290220f8455631so2200279wmq.1 for ; Tue, 13 Jul 2021 05:16:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=hKUg5hLmdOxKy4EZmQN/Zx7NljAO3QDF04sL2kUH93E=; b=BFfN3xrtofWERr1BEGRxZPg1V/TOf5l97JkKXLtORRTTerhxxF5v9JxT3etBJwSZFd gXTAyaz8d/6/H5O/t0yCOEGSZP4xLYa7AnikEUQha/LXNENNmucBAg6+6QV9h1wb4Iev 9MUpNNU90+o6aPA5sEkWIFcQkxORkgdNd16pwUHSYF9fXynMebklshcQV7jxnfd0P+iV bndK8XwmdunBjcbwCehGZT7b/AWHLCo4MjJIXEsMY2dr/DZw5RQnS43sqTmBFLuNoVxM GF1pKb4cSHspXgUJvvtrKQIyXjbaeXzx6tRFE1zIoW97yvmuqZapZd2KvwdM4PfHEICh 3eWw== X-Gm-Message-State: AOAM531auq6GbaPw+n/mLABl1Tbq4ri+BukKAlmYFA/w92adpdxbwzdu E+RGrk7m4ExIVfO0oKg+ZPuN3W5LQWyHTg== X-Google-Smtp-Source: ABdhPJyucWLKXgfQ9cl1KkFaymheiFJ5O3i+QsRsquk1V9B239eEY7IOlM+Dg/DRHsxB8fwumP4RXQ== X-Received: by 2002:a1c:f206:: with SMTP id s6mr4675933wmc.102.1626178569356; Tue, 13 Jul 2021 05:16:09 -0700 (PDT) Received: from ?IPv6:2001:8a0:f932:6a00:46bc:d03b:7b3a:2227? ([2001:8a0:f932:6a00:46bc:d03b:7b3a:2227]) by smtp.gmail.com with ESMTPSA id a12sm18689870wrh.26.2021.07.13.05.16.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 13 Jul 2021 05:16:08 -0700 (PDT) Subject: Re: [PATCH 2/2] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close (PR gdb/28080) From: Pedro Alves To: Simon Marchi , gdb-patches@sourceware.org References: <20210712193311.837489-1-pedro@palves.net> <20210712193311.837489-3-pedro@palves.net> Message-ID: <5235b93c-d76e-9220-ee3d-5af35e7af463@palves.net> Date: Tue, 13 Jul 2021 13:16:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jul 2021 12:16:13 -0000 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 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=) 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 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. 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 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