From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 3F5AC3850432 for ; Tue, 13 Jul 2021 13:13:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3F5AC3850432 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 16DDDh4P019880 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 13 Jul 2021 09:13:48 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 16DDDh4P019880 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8C5E91E01B; Tue, 13 Jul 2021 09:13:43 -0400 (EDT) Subject: Re: [PATCH 2/2] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close (PR gdb/28080) To: Pedro Alves , gdb-patches@sourceware.org References: <20210712193311.837489-1-pedro@palves.net> <20210712193311.837489-3-pedro@palves.net> <5235b93c-d76e-9220-ee3d-5af35e7af463@palves.net> From: Simon Marchi Message-ID: <3a5dc58e-776c-1202-9cf1-cc3403e49895@polymtl.ca> Date: Tue, 13 Jul 2021 09:13:43 -0400 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: <5235b93c-d76e-9220-ee3d-5af35e7af463@palves.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 13 Jul 2021 13:13:43 +0000 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP 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 13:13:52 -0000 >>> 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