From patchwork Wed Aug 3 15:42:44 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 14285 Received: (qmail 25158 invoked by alias); 3 Aug 2016 15:43:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 25148 invoked by uid 89); 3 Aug 2016 15:43:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-pf0-f195.google.com Received: from mail-pf0-f195.google.com (HELO mail-pf0-f195.google.com) (209.85.192.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 03 Aug 2016 15:42:53 +0000 Received: by mail-pf0-f195.google.com with SMTP id g202so14908725pfb.1 for ; Wed, 03 Aug 2016 08:42:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=NntFVFdb6/qABms7OskVUjPHK5M0mQNfA1FJBOT+0E4=; b=f8+jNPOZNrVYqXqKxdQFbozGPNGILXXE2F0qrLne5Wbm3SCMNxs1NxXp7seKgbk/nI gQ2JIkfL4JzG3xn9x7z9GQRFH62BgSGcamYsDBrKtJ7ZVoXJLwrtLLeKDVyHswSvF/7n Ua/p8RJcTiRnhaj4W6BiQCO50BtfGkZh2TqyofraIOg7cw/kJpYrq/txvEsuceFDFUh9 T4L9OdymlLGfkTsUrcjKTy9WLo85IpHWWU96O0xkG+R6lqrarSN8SU0/wYZRWS/Jl3Ra sXNg5AOm8N+73iXnaszKh50EuCzaB104Bjl4PJMoO2656M28+1I0gsf3KoyxhCdxy+r2 NUsg== X-Gm-Message-State: AEkooutN5Gv3BpLb7AqTR30vEMc+U6IY1HM+2KF85Pp22SEdBp0kHLctNQR8bSOKZqqqjA== X-Received: by 10.98.16.75 with SMTP id y72mr116910345pfi.50.1470238971342; Wed, 03 Aug 2016 08:42:51 -0700 (PDT) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id h1sm13655399pay.48.2016.08.03.08.42.47 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 03 Aug 2016 08:42:50 -0700 (PDT) From: Yao Qi To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers References: <1470230379-6790-1-git-send-email-yao.qi@linaro.org> Date: Wed, 03 Aug 2016 16:42:44 +0100 In-Reply-To: (Pedro Alves's message of "Wed, 3 Aug 2016 15:21:50 +0100") Message-ID: <86ziotdfkb.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Pedro Alves writes: > Throwing is useful when the exception needs to cross several layers. > Originally that was added because the exceptions had to cross through > multiple layers, including inf-ptrace.c and the regcache. > > In cases where we have the ptrace errno code at hand, we can simply > check for ESRCH. That is, in fact what regsets_store_inferior_registers > does. > > else if (errno == ESRCH) > { > /* At this point, ESRCH should mean the process is > already gone, in which case we simply ignore attempts > to change its registers. See also the related > comment in linux_resume_one_lwp. */ > free (buf); > return 0; > } > > > (store_register checks ESRCH too instead of throwing, as well > as linux_detach_one_lwp and attach_proc_task_lwp_callback.) The reason I choose throw exception is that regsets_fetch_inferior_registers is called by linux_resume_one_lwp_throw which throws exception when process is gone. Even if regsets_fetch_inferior_registers handles ESRCH quietly, the exception will be thrown out somewhere else in the callees of linux_resume_one_lwp_throw. It is better to throw exception earlier to avoid some unnecessary operations, for example, in linux_resume_one_lwp_throw, struct regcache *regcache = get_thread_regcache (current_thread, 1); lwp->stop_pc = (*the_low_target.get_pc) (regcache); if (debug_threads) { debug_printf (" %s from pc 0x%lx\n", step ? "step" : "continue", (long) lwp->stop_pc); } if we don't throw exception in regsets_fetch_inferior_registers, lwp->stop_pc will have the garbage value, unless we check the status of pc register in regcache in each backend, see whether its status is REG_UNAVAILABLE or not. Maybe, since lwp is gone, the value lwp->stop_pc doesn't matter too much. > > However, I don't think swallowing a register/write error is always the > best to do. > > If we're resuming the program, then yes, we should swallow the error, > because now that the thread is resumed, we'll collect the exit > status shortly. > > But if the thread is _not_ resumed, the user has the now-dead thread > selected, and does "info registers" or "p $somereg = 0xf00" -- then I > think we should report back the error all the way back to the user, > not swallow it and display random garbage for register contents, or > pretend the write succeeded. nowadays, GDBserver does this in get_thread_regcache, /* Invalidate all registers, to prevent stale left-overs. */ memset (regcache->register_status, REG_UNAVAILABLE, regcache->tdesc->num_registers); fetch_inferior_registers (regcache, -1); if the thread is gone, the status of registers is REG_UNAVAILABLE. > > So if we go the throw route, I think the catch should be somewhere > higher level, in the code that is reading/writing registers because > it is resuming the thread. See how linux-nat.c:resume_stopped_resumed_lwps's > TRY block encloses more than one call that might throw. > > It may be that in this case the register read is coming from gdb > directly (really no idea), which would mean that it's gdb that > would have to ignore the error, which would complicate things. Yes, I agree. > > But until that is gone, I see no reason to prefer a throw/catch > instead of simply checking for ESRCH, mirroring > regsets_store_inferior_registers? OK, how about the patch below? diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index e251ac4..1839f99 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -5405,6 +5405,12 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info, not "active". This can happen in normal operation, so suppress the warning in this case. */ } + else if (errno == ESRCH) + { + /* At this point, ESRCH should mean the process is + already gone, in which case we simply ignore attempts + to read its registers. */ + } else { char s[256];