| Message ID | A78C989F6D9628469189715575E55B233325F370@IRSMSX104.ger.corp.intel.com |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 44994 invoked by alias); 8 Feb 2016 08:14:15 -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 44974 invoked by uid 89); 8 Feb 2016 08:14:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=Sunday, unintentionally, brobecker, Joel X-HELO: mga04.intel.com Received: from mga04.intel.com (HELO mga04.intel.com) (192.55.52.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Feb 2016 08:14:12 +0000 Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP; 08 Feb 2016 00:14:11 -0800 X-ExtLoop1: 1 Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by orsmga003.jf.intel.com with ESMTP; 08 Feb 2016 00:14:10 -0800 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.210]) by IRSMSX102.ger.corp.intel.com ([169.254.2.97]) with mapi id 14.03.0248.002; Mon, 8 Feb 2016 08:14:09 +0000 From: "Metzger, Markus T" <markus.t.metzger@intel.com> To: Joel Brobecker <brobecker@adacore.com> CC: "palves@redhat.com" <palves@redhat.com>, "gdb-patches@sourceware.org" <gdb-patches@sourceware.org> Subject: RE: [PATCH v2 2/3] frame: use get_prev_frame_always in skip_tailcall_frames Date: Mon, 8 Feb 2016 08:14:09 +0000 Message-ID: <A78C989F6D9628469189715575E55B233325F370@IRSMSX104.ger.corp.intel.com> References: <1454681922-2228-1-git-send-email-markus.t.metzger@intel.com> <1454681922-2228-2-git-send-email-markus.t.metzger@intel.com> <20160207130057.GE20874@adacore.com> In-Reply-To: <20160207130057.GE20874@adacore.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes |
Commit Message
Metzger, Markus T
Feb. 8, 2016, 8:14 a.m. UTC
> -----Original Message----- > From: Joel Brobecker [mailto:brobecker@adacore.com] > Sent: Sunday, February 7, 2016 2:01 PM > To: Metzger, Markus T <markus.t.metzger@intel.com> > Cc: palves@redhat.com; gdb-patches@sourceware.org > Subject: Re: [PATCH v2 2/3] frame: use get_prev_frame_always in > skip_tailcall_frames Hi Joel, Thanks for your review. > > Following the practice in skip_artificial_frames, also use > > get_prev_frame_always instead of get_prev_frame in > > skip_tailcall_frames. > > > > 2016-02-05 Markus Metzger <markus.t.metzger@intel.com> > > > > gdb/ > > * frame.c (skip_tailcall_frames): Call get_prev_frame_always. > > This seems OK; but would you be able to create a testcase for this? I modified an existing test case to cover the changes. GDB dies with the modified test and without the changes to skip_tailcall_frames. This also showed another place where we want to use get_prev_frame_always. Here's the modified version of this patch: commit d426b734a965ef21d03bf7ed30f20be7d7ed31fa Author: Markus Metzger <markus.t.metzger@intel.com> Date: Fri Feb 5 09:39:05 2016 +0100 frame: use get_prev_frame_always in skip_tailcall_frames and finish_command Following the practice in skip_artificial_frames, also use get_prev_frame_always instead of get_prev_frame in skip_tailcall_frames and in finish_command. 2016-02-08 Markus Metzger <markus.t.metzger@intel.com> gdb/ * frame.c (skip_tailcall_frames): Call get_prev_frame_always. * infcmd.c (finish_command): Call get_prev_frame_always. testsuite/ * gdb.arch/amd64-tailcall-ret.exp: Set backtrace limit. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
Comments
On 02/08/2016 08:14 AM, Metzger, Markus T wrote: > I modified an existing test case to cover the changes. GDB dies with the modified > test and without the changes to skip_tailcall_frames. This also showed another place > where we want to use get_prev_frame_always. > > Here's the modified version of this patch: Thanks. Could you do the limiting test in e.g., gdb.base/finish.exp and gdb.base/return.exp, so that it'd be covered on all archs? Thanks, Pedro Alves
> > I modified an existing test case to cover the changes. GDB dies with the modified > > test and without the changes to skip_tailcall_frames. This also showed another place > > where we want to use get_prev_frame_always. > > > > Here's the modified version of this patch: > > Thanks. Could you do the limiting test in e.g., gdb.base/finish.exp > and gdb.base/return.exp, so that it'd be covered on all archs? I was going to ask the very same :-). The fact that adding your test showed we missed a spot raised the question as to how much of the initial patch we were testing :).
> -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Joel Brobecker > Sent: Tuesday, February 9, 2016 12:58 PM > To: Pedro Alves <palves@redhat.com> > Cc: Metzger, Markus T <markus.t.metzger@intel.com>; gdb- > patches@sourceware.org > Subject: Re: [PATCH v2 2/3] frame: use get_prev_frame_always in > skip_tailcall_frames > > > > I modified an existing test case to cover the changes. GDB dies > > > with the modified test and without the changes to > > > skip_tailcall_frames. This also showed another place where we want to > use get_prev_frame_always. > > > > > > Here's the modified version of this patch: > > > > Thanks. Could you do the limiting test in e.g., gdb.base/finish.exp > > and gdb.base/return.exp, so that it'd be covered on all archs? > > I was going to ask the very same :-). The fact that adding your test showed > we missed a spot raised the question as to how much of the initial patch we > were testing :). I don't get your comment. I'm beginning to wonder if not all-but-the-backtrace-command-related get_prev_frame calls should really be calling get_prev_frame_always. The _always extension isn't very intuitive, though, given that this should be the standard function to use. Should get_prev_frame maybe be renamed to something like get_prev_frame_within_limit and get_prev_frame_always to get_prev_frame? Regards, Markus. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
On 02/09/2016 02:25 PM, Metzger, Markus T wrote: > I'm beginning to wonder if not all-but-the-backtrace-command-related > get_prev_frame calls should really be calling get_prev_frame_always. > > The _always extension isn't very intuitive, though, given that this should be > the standard function to use. Should get_prev_frame maybe be renamed to > something like get_prev_frame_within_limit and get_prev_frame_always > to get_prev_frame? Maybe a good idea. See also: https://sourceware.org/bugzilla/show_bug.cgi?id=15558 I just noticed/remembered something -- the check for backtracing past main and the entry point is in get_prev_frame, get_prev_frame_always bypasses it. This means that with your change, I think gdb now allows "finish" in "main" or in "_start". Maybe not a bad change, but I though it'd call it out explicitly. Thanks, Pedro Alves
> > I was going to ask the very same :-). The fact that adding your test showed > > we missed a spot raised the question as to how much of the initial patch we > > were testing :). > > I don't get your comment. This is the logic behind it: Presumably, your initial patch did fix something. It would be nice to have that tested, hence the suggestion to add that. You then added a test, but I think it only partially overlaps with the situation your initial patch was trying to cover, because the test you added uncovered a spot that you didn't need to change before. That's why I think there is a strong chance that adding one more test would increase coverage of your patch. Or said differently, if we undid any hunk in your commit, would a test immediately regress? > I'm beginning to wonder if not all-but-the-backtrace-command-related > get_prev_frame calls should really be calling get_prev_frame_always. > > The _always extension isn't very intuitive, though, given that this should be > the standard function to use. Should get_prev_frame maybe be renamed to > something like get_prev_frame_within_limit and get_prev_frame_always > to get_prev_frame? (need more time to answer that question)
> -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Pedro Alves > Sent: Tuesday, February 9, 2016 3:41 PM > To: Metzger, Markus T <markus.t.metzger@intel.com>; Joel Brobecker > <brobecker@adacore.com> > Cc: gdb-patches@sourceware.org > Subject: Re: [PATCH v2 2/3] frame: use get_prev_frame_always in > skip_tailcall_frames > > On 02/09/2016 02:25 PM, Metzger, Markus T wrote: > > > I'm beginning to wonder if not all-but-the-backtrace-command-related > > get_prev_frame calls should really be calling get_prev_frame_always. > > > > The _always extension isn't very intuitive, though, given that this > > should be the standard function to use. Should get_prev_frame maybe > > be renamed to something like get_prev_frame_within_limit and > > get_prev_frame_always to get_prev_frame? > > Maybe a good idea. See also: > > https://sourceware.org/bugzilla/show_bug.cgi?id=15558 > > I just noticed/remembered something -- the check for backtracing past main > and the entry point is in get_prev_frame, get_prev_frame_always bypasses > it. > > This means that with your change, I think gdb now allows "finish" in "main" or > in "_start". > > Maybe not a bad change, but I though it'd call it out explicitly. There are 46 calls to get_prev_frame ignoring this patch. Each of them should maybe be modified to call get_prev_frame_always, instead. And none of this is currently covered by the test suite. The first step would probably be to look at all those calls and derive test cases for each of them. Then fix the failing tests one by one. This is unrelated to this patch series. Let me therefore drop this patch. Regards, Markus. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
> -----Original Message----- > From: Metzger, Markus T > Sent: Tuesday, February 9, 2016 4:20 PM > To: Pedro Alves <palves@redhat.com>; Joel Brobecker > <brobecker@adacore.com> > Cc: gdb-patches@sourceware.org > Subject: RE: [PATCH v2 2/3] frame: use get_prev_frame_always in > skip_tailcall_frames > > > I'm beginning to wonder if not all-but-the-backtrace-command-related > > > get_prev_frame calls should really be calling get_prev_frame_always. > > > > > > The _always extension isn't very intuitive, though, given that this > > > should be the standard function to use. Should get_prev_frame maybe > > > be renamed to something like get_prev_frame_within_limit and > > > get_prev_frame_always to get_prev_frame? > > > > Maybe a good idea. See also: > > > > https://sourceware.org/bugzilla/show_bug.cgi?id=15558 > > > > I just noticed/remembered something -- the check for backtracing past > > main and the entry point is in get_prev_frame, get_prev_frame_always > > bypasses it. > > > > This means that with your change, I think gdb now allows "finish" in > > "main" or in "_start". > > > > Maybe not a bad change, but I though it'd call it out explicitly. > > There are 46 calls to get_prev_frame ignoring this patch. Each of them > should maybe be modified to call get_prev_frame_always, instead. And > none of this is currently covered by the test suite. I'm wondering in which cases GDB should ignore the user-defined backtrace limit. And if GDB should ignore it at all. If the limit is set, some aspects of GDB may not function any longer. But that's to be expected, isn't it? GDB shouldn't crash, of course. But I'm not sure if it should ignore user settings in too many cases. Maybe we should even switch back to get_prev_frame in skip_artificial_frames and rely on handling the NULL return if we exceed the backtrace limit? Regards, Markus. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
On 02/15/2016 09:50 AM, Metzger, Markus T wrote: > I'm wondering in which cases GDB should ignore the user-defined backtrace > limit. And if GDB should ignore it at all. > > If the limit is set, some aspects of GDB may not function any longer. But that's > to be expected, isn't it? > > GDB shouldn't crash, of course. But I'm not sure if it should ignore user settings > in too many cases. I'm starting to think the same way. Want to give it a try and see what breaks? > Maybe we should even switch back to get_prev_frame in > skip_artificial_frames and rely on handling the NULL return if we exceed the > backtrace limit? If all places will end up throwing an error, it may be the better to make skip_artificial_frames itself throw. We'd have to take a deeper look at each case though. We need to also keep in mind that there may be cases where skip_artificial_frames might be used in internal-facing code, where it might still be necessary get past inline frames to reach the real stack frame. I guess sticking a "set backtrace limit 1" in some of the inline tests would expose this. Thanks, Pedro Alves
diff --git a/gdb/frame.c b/gdb/frame.c index b7832c7..6ab8834 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -443,8 +443,12 @@ skip_artificial_frames (struct frame_info *frame) struct frame_info * skip_tailcall_frames (struct frame_info *frame) { + /* Note we use get_prev_frame_always, and not get_prev_frame. The + latter will truncate the frame chain, leading to this function + unintentionally returning a null_frame_id (e.g., when the user + sets a backtrace limit). */ while (get_frame_type (frame) == TAILCALL_FRAME) - frame = get_prev_frame (frame); + frame = get_prev_frame_always (frame); return frame; } diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 930dc61..e98f6f5 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -1958,7 +1958,10 @@ finish_command (char *arg, int from_tty) /* Done with ARGS. */ do_cleanups (args_chain); - frame = get_prev_frame (get_selected_frame (_("No selected frame."))); + /* Note we use get_prev_frame_always, and not get_prev_frame. The latter + will truncate the frame chain, leading to this function unintentionally + throwing an error (e.g., when the user sets a backtrace limit). */ + frame = get_prev_frame_always (get_selected_frame (_("No selected frame."))); if (frame == 0) error (_("\"finish\" not meaningful in the outermost frame.")); diff --git a/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp b/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp index 2642cdd..0334b54 100644 --- a/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp +++ b/gdb/testsuite/gdb.arch/amd64-tailcall-ret.exp @@ -36,6 +36,11 @@ if ![runto_main] { gdb_breakpoint "g" gdb_continue_to_breakpoint "g" ".* v = 2;" +# Severely limit the back trace. The limit should be ignored for "return" +# and "finish" commands. +# +gdb_test "set backtrace limit 1" + gdb_test "return" { f \(\); /\* second \*/} "return" \ {Make g return now\? \(y or n\) } "y"