From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44529 invoked by alias); 28 Feb 2020 21:11:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 44521 invoked by uid 89); 28 Feb 2020 21:11:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=H*f:sk:87k146p, H*MI:sk:87k146p, H*i:sk:87k146p X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Feb 2020 21:11:33 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582924291; 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=fcZT4JhzXt2Lf90xWqvhheHI0g7DOkrgC9CVnsD22Vo=; b=XcrMGfigohjq9YmE+oQhiVVk+AeXlHbWxO2lEHjZQVpNl61rotRs85r/tiD3v34UtzIS9c MMo+Ui2VfuKkN09PHIMnhhu2iEWR7CEMyiGcGGndr6LbexF92E7W6OGiePisOl7x0GEG9q wmUUMuEiU9JRP28hYsZ2HRv5zWhMGPY= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-418-9Kpmj3ucM1qnQ_6QucNkPw-1; Fri, 28 Feb 2020 16:11:27 -0500 Received: by mail-wr1-f69.google.com with SMTP id z15so1886162wrw.0 for ; Fri, 28 Feb 2020 13:11:27 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id i8sm8220208wrq.10.2020.02.28.13.11.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 28 Feb 2020 13:11:24 -0800 (PST) Subject: Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name' To: Sergio Durigan Junior References: <20190926042155.31481-1-sergiodj@redhat.com> <20200226200542.746617-1-sergiodj@redhat.com> <20200226200542.746617-3-sergiodj@redhat.com> <87wo86ivbu.fsf@tromey.com> <87k146pi13.fsf@redhat.com> Cc: Tom Tromey , GDB Patches , Eli Zaretskii , Ruslan Kabatsayev From: Pedro Alves Message-ID: <89e47bf5-d5c8-849d-e345-9b79306418f8@redhat.com> Date: Fri, 28 Feb 2020 21:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <87k146pi13.fsf@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-02/txt/msg01076.txt.bz2 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 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