Message ID | 20200226200542.746617-3-sergiodj@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 101570 invoked by alias); 26 Feb 2020 20:06:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 101376 invoked by uid 89); 26 Feb 2020 20:06:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=1999, controversial X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (207.211.31.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Feb 2020 20:06:28 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582747587; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IZty+NlgzqQ4NoVX1juu+3Q/p1oH0oXjEBJgNgFCKMA=; b=YkAQVYnW5+BQU1F/W6r/nCB9XNwOFIwgvoTaIhMMu79oYx29EPWIAzQ5gBUesJyBPyq7BB 0xazAFA4aII7yDgQVTgJWbKE/Dw2fVLMoZJ42yLplODKcXHBnvcKB7BlFdS+ErIJpj/Kw6 GJzwXmyUUf27JcZ/CZrqdlTeeJxktJ4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-237-A_2bKbIwN6GhfbodyW7qUA-1; Wed, 26 Feb 2020 15:06:20 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 55839192296D; Wed, 26 Feb 2020 20:06:19 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-54.yyz.redhat.com [10.15.17.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id AD8145DA76; Wed, 26 Feb 2020 20:06:18 +0000 (UTC) From: Sergio Durigan Junior <sergiodj@redhat.com> To: GDB Patches <gdb-patches@sourceware.org> Cc: Pedro Alves <palves@redhat.com>, Tom Tromey <tom@tromey.com>, Eli Zaretskii <eliz@gnu.org>, Ruslan Kabatsayev <b7.10110111@gmail.com>, Sergio Durigan Junior <sergiodj@redhat.com> Subject: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name' Date: Wed, 26 Feb 2020 15:05:38 -0500 Message-Id: <20200226200542.746617-3-sergiodj@redhat.com> In-Reply-To: <20200226200542.746617-1-sergiodj@redhat.com> References: <20190926042155.31481-1-sergiodj@redhat.com> <20200226200542.746617-1-sergiodj@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
Feb. 26, 2020, 8:05 p.m. UTC
Since this hunk may be a bit controversial, I decided to split it into a separate patch. This is going to be needed by the ptrace-error feature; GDB will need to be able to access the value of errno even after a call to our 'perror'-like functions. The idea here is that we actually *want* errno to propagate between our customized 'perror' calls. We currently have this code on 'throw_perror_with_name': /* I understand setting these is a matter of taste. Still, some people may clear errno but not know about bfd_error. Doing this here is not unreasonable. */ bfd_set_error (bfd_error_no_error); errno = 0; git blame tells me that this piece of code is pretty old; the commit that "introduced" it is: commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc Author: Stan Shebs <shebs@codesourcery.com> Date: Fri Apr 16 01:35:26 1999 +0000 Initial creation of sourceware repository so yeah... If we go to the POSIX specification for 'perror', it doesn't really say anything about whether errno should be preserved or not. It does, however, say that 'perror's messages should be the same as those returned by 'strerror', and 'strerror' is not supposed to alter errno if the call is successful. Maybe when our wrapper was written it was OK to modify errno, I don't know. But I'd like to propose that we stick to POSIX in this case. Another small hunk is the one that saves/restores errno on gdbserver's 'perror_with_name', but this one is pretty trivial, I think. gdb/ChangeLog: yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com> * utils.c (throw_perror_with_name): Don't reset errno/bfd_error. gdbserver/ChangeLog: yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com> * utils.cc (perror_with_name): Save/restore errno. --- gdb/utils.c | 6 ------ gdbserver/utils.cc | 2 ++ 2 files changed, 2 insertions(+), 6 deletions(-)
Comments
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
Sergio> Since this hunk may be a bit controversial, I decided to split it into
Sergio> a separate patch. This is going to be needed by the ptrace-error
Sergio> feature; GDB will need to be able to access the value of errno even
Sergio> after a call to our 'perror'-like functions.
I'm in favor of this. The existing code seems pretty ugly.
I'd imagine it's unlikely that any caller would rely on this.
If it tested cleanly then that is good enough for me.
Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
Sergio> 'perror_with_name', but this one is pretty trivial, I think.
I didn't understand why this one was needed.
Does safe_strerror reset errno? Maybe a comment would be in order.
Tom
On Friday, February 28 2020, Tom Tromey wrote: >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: > > Sergio> Since this hunk may be a bit controversial, I decided to split it into > Sergio> a separate patch. This is going to be needed by the ptrace-error > Sergio> feature; GDB will need to be able to access the value of errno even > Sergio> after a call to our 'perror'-like functions. > > I'm in favor of this. The existing code seems pretty ugly. > > I'd imagine it's unlikely that any caller would rely on this. > If it tested cleanly then that is good enough for me. As far as I have tested (buildbot and locally), everything is OK. > Sergio> Another small hunk is the one that saves/restores errno on gdbserver's > Sergio> 'perror_with_name', but this one is pretty trivial, I think. > > I didn't understand why this one was needed. > Does safe_strerror reset errno? Maybe a comment would be in order. Ah, no, safe_strerror doesn't reset errno. As I explained in the commit message, it is not allowed to do so. The reason I decided to explicitly save/restore errno is because I wanted to make sure that we (also explicitly) follow the same rules for both GDB and gdbserver perror functions. But now I'm left thinking that I should have proposed the same thing for 'throw_perror_with_name'... Anyway, I'm fine with removing the save/restore from gdbserver's perror. I'll also remove the reference to it in the commit message. Thanks,
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >> I didn't understand why this one was needed. >> Does safe_strerror reset errno? Maybe a comment would be in order. Sergio> Ah, no, safe_strerror doesn't reset errno. As I explained in the commit Sergio> message, it is not allowed to do so. The reason I decided to explicitly Sergio> save/restore errno is because I wanted to make sure that we (also Sergio> explicitly) follow the same rules for both GDB and gdbserver perror Sergio> functions. But now I'm left thinking that I should have proposed the Sergio> same thing for 'throw_perror_with_name'... Sergio> Anyway, I'm fine with removing the save/restore from gdbserver's Sergio> perror. I'll also remove the reference to it in the commit message. The main thing for me is that when someone sees this in 5 years, that there's some idea of why the line is there. Leaving the line out is just as good :) thanks, Tom
On 2/26/20 8:05 PM, Sergio Durigan Junior wrote: > > git blame tells me that this piece of code is pretty old; the commit > that "introduced" it is: > > commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc > Author: Stan Shebs <shebs@codesourcery.com> > Date: Fri Apr 16 01:35:26 1999 +0000 > > Initial creation of sourceware repository > > so yeah... This is not the oldest commit in the tree. Using git log starting at the hash, you should be able to find older commits which touch the file. The thing is that history around the time of that "initial creation" commit, is quite messy, because the CVS tree back then was deleted and recreated a number of times and merged in some odd ways. Thanks, Pedro Alves
On Friday, February 28 2020, Tom Tromey wrote: >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: > >>> I didn't understand why this one was needed. >>> Does safe_strerror reset errno? Maybe a comment would be in order. > > Sergio> Ah, no, safe_strerror doesn't reset errno. As I explained in the commit > Sergio> message, it is not allowed to do so. The reason I decided to explicitly > Sergio> save/restore errno is because I wanted to make sure that we (also > Sergio> explicitly) follow the same rules for both GDB and gdbserver perror > Sergio> functions. But now I'm left thinking that I should have proposed the > Sergio> same thing for 'throw_perror_with_name'... > > Sergio> Anyway, I'm fine with removing the save/restore from gdbserver's > Sergio> perror. I'll also remove the reference to it in the commit message. > > The main thing for me is that when someone sees this in 5 years, that > there's some idea of why the line is there. Leaving the line out is > just as good :) Right, that's an excellent argument. It's safe to leave the line out right now; the important thing is that we don't touch errno, anyway. Thanks,
On Friday, February 28 2020, Pedro Alves wrote: > On 2/26/20 8:05 PM, Sergio Durigan Junior wrote: >> >> git blame tells me that this piece of code is pretty old; the commit >> that "introduced" it is: >> >> commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc >> Author: Stan Shebs <shebs@codesourcery.com> >> Date: Fri Apr 16 01:35:26 1999 +0000 >> >> Initial creation of sourceware repository >> >> so yeah... > > This is not the oldest commit in the tree. Using git log > starting at the hash, you should be able to find older commits > which touch the file. The thing is that history around the time > of that "initial creation" commit, is quite messy, because the > CVS tree back then was deleted and recreated a number of times > and merged in some odd ways. Sorry, I didn't mean to say that this is the oldest commit in the tree. But even doing a bit more search, I could only find this commit: commit 8eec331072987d38064745a33ae89cc5d195029c Author: Steve Chamberlain <sac@cygnus> Date: Sat Mar 19 03:16:10 1994 +0000 * utils.c (prompt_for_continue): Call readline, not gdb_readline. which did: - bfd_error = no_error; + bfd_set_error (bfd_error_no_error); This means the code is really old (pre-1994), and I could not track when it was added. And I don't think it matters much at this point: even if I could find the exact commit that added it, I doubt I'd be able to find the rationale.
On 2/28/20 3:29 PM, Tom Tromey wrote: >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: > > Sergio> Since this hunk may be a bit controversial, I decided to split it into > Sergio> a separate patch. This is going to be needed by the ptrace-error > Sergio> feature; GDB will need to be able to access the value of errno even > Sergio> after a call to our 'perror'-like functions. > > I'm in favor of this. The existing code seems pretty ugly. I'm not sure in favor of relying on errno being preserved from throw site to catch site, with potentially multiple try/catch hops in between. Sergio, can you point out exactly how you're intending to use that? But, I'm in favor of removing this errno/bfd_error clearing too. We used to have more global state clearing around this area, but it's been gradually removed. E.g.: commit 0af679c6e0645a93d5a60ec936b94dc70a2f9e5c Author: Pedro Alves <palves@redhat.com> AuthorDate: Tue Apr 12 16:49:30 2016 +0100 Commit: Pedro Alves <palves@redhat.com> CommitDate: Tue Apr 12 16:55:35 2016 +0100 Don't call clear_quit_flag in prepare_to_throw_exception I think this is reminiscent of the time when a longjmp would always jump to the top level. Nowaways code that throw exceptions other than a quit, which may even be caught and handled without reaching the top level. Certainly such exceptions shouldn't clear an interrupt request... I suspect this perror_with_name errno/bfd_error clearing was also related to ancient direct longjmp to the top level. > I'd imagine it's unlikely that any caller would rely on this. > If it tested cleanly then that is good enough for me. > > Sergio> Another small hunk is the one that saves/restores errno on gdbserver's > Sergio> 'perror_with_name', but this one is pretty trivial, I think. > > I didn't understand why this one was needed. > Does safe_strerror reset errno? Maybe a comment would be in order. Thanks, Pedro Alves
On Friday, February 28 2020, Pedro Alves wrote: > On 2/28/20 3:29 PM, Tom Tromey wrote: >>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >> >> Sergio> Since this hunk may be a bit controversial, I decided to split it into >> Sergio> a separate patch. This is going to be needed by the ptrace-error >> Sergio> feature; GDB will need to be able to access the value of errno even >> Sergio> after a call to our 'perror'-like functions. >> >> I'm in favor of this. The existing code seems pretty ugly. > > I'm not sure in favor of relying on errno being preserved from > throw site to catch site, with potentially multiple try/catch hops > in between. Sergio, can you point out exactly how you're > intending to use that? Yeah. I caught this problem when I was testing to see if GDB would print the ptrace fail reason when trying (unsuccessfully) to attach to a process. The call stack looks like: linux_nat_target::attach | |--> inf_ptrace_target::attach # where ptrace fails | |--> throw_perror_with_name # where errno is set to 0 When 'throw_perror_with_name' calls 'error', 'linux_nat_target::attach' catches the exception and tries to print the reason: try { inf_ptrace_target::attach (args, from_tty); } catch (const gdb_exception_error &ex) { int saved_errno = errno; pid_t pid = parse_pid_to_attach (args); std::string reason = linux_ptrace_attach_fail_reason (pid, saved_errno); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ if (!reason.empty ()) throw_error (ex.error, "warning: %s\n%s", reason.c_str (), ex.what ()); else throw_error (ex.error, "%s", ex.what ()); } However, at this point errno is already 0, so the function can't determine the exact reason for the ptrace failure. In fact, because errno = 0, 'linux_ptrace_attach_fail_reason' doesn't print anything, because it thinks everything is OK! IMO, it doesn't make sense to have errno = 0 while you're handling an exception (which, in this case, was caused exactly because a syscall failed, and so we expect errno to be different than 0). Thanks,
On 2/28/20 8:35 PM, Sergio Durigan Junior wrote: > On Friday, February 28 2020, Pedro Alves wrote: > >> On 2/28/20 3:29 PM, Tom Tromey wrote: >>>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >>> >>> Sergio> Since this hunk may be a bit controversial, I decided to split it into >>> Sergio> a separate patch. This is going to be needed by the ptrace-error >>> Sergio> feature; GDB will need to be able to access the value of errno even >>> Sergio> after a call to our 'perror'-like functions. >>> >>> I'm in favor of this. The existing code seems pretty ugly. >> >> I'm not sure in favor of relying on errno being preserved from >> throw site to catch site, with potentially multiple try/catch hops >> in between. Sergio, can you point out exactly how you're >> intending to use that? > > Yeah. I caught this problem when I was testing to see if GDB would > print the ptrace fail reason when trying (unsuccessfully) to attach to a > process. > > The call stack looks like: > > linux_nat_target::attach > | > |--> inf_ptrace_target::attach # where ptrace fails > | > |--> throw_perror_with_name # where errno is set to 0 > > When 'throw_perror_with_name' calls 'error', 'linux_nat_target::attach' > catches the exception and tries to print the reason: > > try > { > inf_ptrace_target::attach (args, from_tty); > } > catch (const gdb_exception_error &ex) > { > int saved_errno = errno; > pid_t pid = parse_pid_to_attach (args); > std::string reason = linux_ptrace_attach_fail_reason (pid, saved_errno); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > if (!reason.empty ()) > throw_error (ex.error, "warning: %s\n%s", reason.c_str (), > ex.what ()); > else > throw_error (ex.error, "%s", ex.what ()); > } > > However, at this point errno is already 0, so the function can't > determine the exact reason for the ptrace failure. In fact, because > errno = 0, 'linux_ptrace_attach_fail_reason' doesn't print anything, > because it thinks everything is OK! > > IMO, it doesn't make sense to have errno = 0 while you're handling an > exception (which, in this case, was caused exactly because a syscall > failed, and so we expect errno to be different than 0). This is bad design. Exception objects should be self contained and not rely on global state to transfer information. E.g., it should be possible to do void my_function () { try { function_that_throws (); } catch (const gdb_exception &ex) { // call some function that potentially changes errno. function_that_changes_errno (); throw; // rethrow. } } and then: try { my_function (); } catch (const gdb_exception &ex) { // errno here is really unreliable. } It's not even guaranteed that the exception thrown from within inf_ptrace_target::attach is thrown after setting errno, or that errno is meaningful for the exception thrown. For example, this error: if (pid == getpid ()) error (_("I refuse to debug myself!")); I think the simpler thing to do here is to change inf_ptrace_target::attach to return the ptrace errno as the function's return. I.e., rename it and change it from: void inf_ptrace_target::attach (const char *args, int from_tty); to: int inf_ptrace_target::ptrace_attach (const char *args, int from_tty); And change the body like this: errno = 0; ptrace (PT_ATTACH, pid, (PTRACE_TYPE_ARG3)0, 0); if (errno != 0) - perror_with_name (("ptrace")); + return errno; with a return 0 added at the end. All other errors still throw like before. Then add a new "attach" method that wraps the old method, so that targets that inherit inf_ptrace_target and don't override attach continue working: void inf_ptrace_target::attach (const char *args, int from_tty) { errno = ptrace_attach (args, from_tty); if (errno != 0) perror_with_name (("ptrace")); } This avoids having to think about storing the errno number in the exception object. Thanks, Pedro Alves
On Friday, February 28 2020, Pedro Alves wrote: > On 2/28/20 8:35 PM, Sergio Durigan Junior wrote: >> On Friday, February 28 2020, Pedro Alves wrote: >> >>> On 2/28/20 3:29 PM, Tom Tromey wrote: >>>>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes: >>>> >>>> Sergio> Since this hunk may be a bit controversial, I decided to split it into >>>> Sergio> a separate patch. This is going to be needed by the ptrace-error >>>> Sergio> feature; GDB will need to be able to access the value of errno even >>>> Sergio> after a call to our 'perror'-like functions. >>>> >>>> I'm in favor of this. The existing code seems pretty ugly. >>> >>> I'm not sure in favor of relying on errno being preserved from >>> throw site to catch site, with potentially multiple try/catch hops >>> in between. Sergio, can you point out exactly how you're >>> intending to use that? >> >> Yeah. I caught this problem when I was testing to see if GDB would >> print the ptrace fail reason when trying (unsuccessfully) to attach to a >> process. >> >> The call stack looks like: >> >> linux_nat_target::attach >> | >> |--> inf_ptrace_target::attach # where ptrace fails >> | >> |--> throw_perror_with_name # where errno is set to 0 >> >> When 'throw_perror_with_name' calls 'error', 'linux_nat_target::attach' >> catches the exception and tries to print the reason: >> >> try >> { >> inf_ptrace_target::attach (args, from_tty); >> } >> catch (const gdb_exception_error &ex) >> { >> int saved_errno = errno; >> pid_t pid = parse_pid_to_attach (args); >> std::string reason = linux_ptrace_attach_fail_reason (pid, saved_errno); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> if (!reason.empty ()) >> throw_error (ex.error, "warning: %s\n%s", reason.c_str (), >> ex.what ()); >> else >> throw_error (ex.error, "%s", ex.what ()); >> } >> >> However, at this point errno is already 0, so the function can't >> determine the exact reason for the ptrace failure. In fact, because >> errno = 0, 'linux_ptrace_attach_fail_reason' doesn't print anything, >> because it thinks everything is OK! >> >> IMO, it doesn't make sense to have errno = 0 while you're handling an >> exception (which, in this case, was caused exactly because a syscall >> failed, and so we expect errno to be different than 0). > > This is bad design. Exception objects should be self contained > and not rely on global state to transfer information. OK. I implemented your idea, but I will wait until the other patches are reviewed so I can submit a v2 of the whole series. Thanks.
diff --git a/gdb/utils.c b/gdb/utils.c index 0b470120a2..df8add1afa 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -595,12 +595,6 @@ throw_perror_with_name (enum errors errcode, const char *string) { std::string combined = perror_string (string); - /* I understand setting these is a matter of taste. Still, some people - may clear errno but not know about bfd_error. Doing this here is not - unreasonable. */ - bfd_set_error (bfd_error_no_error); - errno = 0; - throw_error (errcode, _("%s."), combined.c_str ()); } diff --git a/gdbserver/utils.cc b/gdbserver/utils.cc index d88f4ac5ca..50ebba43ca 100644 --- a/gdbserver/utils.cc +++ b/gdbserver/utils.cc @@ -46,6 +46,7 @@ perror_with_name (const char *string) { const char *err; char *combined; + int saved_errno = errno; err = safe_strerror (errno); if (err == NULL) @@ -56,6 +57,7 @@ perror_with_name (const char *string) strcat (combined, ": "); strcat (combined, err); + errno = saved_errno; error ("%s.", combined); }