Message ID | 3508.1416603484@usendtaylorx2l |
---|---|
State | New, archived |
Headers |
Received: (qmail 1974 invoked by alias); 21 Nov 2014 20:58:24 -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 1959 invoked by uid 89); 21 Nov 2014 20:58:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mailuogwhop.emc.com Received: from mailuogwhop.emc.com (HELO mailuogwhop.emc.com) (168.159.213.141) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 21 Nov 2014 20:58:22 +0000 Received: from maildlpprd01.lss.emc.com (maildlpprd01.lss.emc.com [10.253.24.33]) by mailuogwprd03.lss.emc.com (Sentrion-MTA-4.3.1/Sentrion-MTA-4.3.0) with ESMTP id sALKwKTa031822 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for <gdb-patches@sourceware.org>; Fri, 21 Nov 2014 15:58:20 -0500 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd03.lss.emc.com sALKwKTa031822 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd03.lss.emc.com sALKwKTa031822 Received: from mailhub.lss.emc.com (mailhubhoprd06.lss.emc.com [10.254.222.130]) by maildlpprd01.lss.emc.com (RSA Interceptor) for <gdb-patches@sourceware.org>; Fri, 21 Nov 2014 15:57:37 -0500 Received: from usendtaylorx2l.lss.emc.com (usendtaylorx2l.lss.emc.com [10.243.10.188]) by mailhub.lss.emc.com (Switch-3.4.3/Switch-3.4.3) with ESMTP id sALKw5bc011015 for <gdb-patches@sourceware.org>; Fri, 21 Nov 2014 15:58:06 -0500 Received: by usendtaylorx2l.lss.emc.com (Postfix, from userid 26043) id B20E75DAF18; Fri, 21 Nov 2014 15:58:04 -0500 (EST) Received: from usendtaylorx2l (localhost [127.0.0.1]) by usendtaylorx2l.lss.emc.com (Postfix) with ESMTP id 110E55DAF17 for <gdb-patches@sourceware.org>; Fri, 21 Nov 2014 15:58:04 -0500 (EST) From: David Taylor <dtaylor@emc.com> To: gdb-patches@sourceware.org Subject: RFA bug fix -- x86-64 stabs and deprecated fp register Date: Fri, 21 Nov 2014 15:58:04 -0500 Message-ID: <3508.1416603484@usendtaylorx2l> X-EMM-MHVC: 1 X-RSA-Classifications: public X-Sentrion-Hostname: mailuogwprd03.lss.emc.com X-IsSubscribed: yes |
Commit Message
David Taylor
Nov. 21, 2014, 8:58 p.m. UTC
Sometimes when using STABS on x86-64 GNU/Linux, GDB does not know which register to use for the frame pointer and as a result offsets from the frame pointer are treated as absolute addresses rather than as offsets... This patch provides a default for when the debug information doesn't specify which register to use. We have seen this problem when debugging problems with a previous release of our software (I believe it was built with GCC 4.5.x, if that matters). There were no regressions on x86-64 GNU/Linux. 2014-11-21 David Taylor <dtaylor@emc.com> * amd64-tdep.c (amd64_init_abi): Set default frame pointer. The preferred debugging format for all known AMD64 targets is
Comments
David, On Fri, Nov 21, 2014 at 03:58:04PM -0500, David Taylor wrote: > Sometimes when using STABS on x86-64 GNU/Linux, GDB does not know which > register to use for the frame pointer and as a result offsets from the > frame pointer are treated as absolute addresses rather than as > offsets... > > This patch provides a default for when the debug information doesn't > specify which register to use. > > We have seen this problem when debugging problems with a previous > release of our software (I believe it was built with GCC 4.5.x, if that > matters). > > There were no regressions on x86-64 GNU/Linux. > > 2014-11-21 David Taylor <dtaylor@emc.com> > > * amd64-tdep.c (amd64_init_abi): Set default frame pointer. I don't think your patch is correct, as it is going to affect the outcome of... (gdb) print $fp ... for frameless functions compiled with DWARF2. See std-regs.c:value_of_builtin_frame_fp_reg. If we were to apply your patch, it would unconditionally print the contents of the %rbp register as the result, which is not what we've been printing so far. I wish I could suggest another approach but I haven't had to touch stabs support in several years, now. If you provided a bit more details as to what actually happened in the debugger and where, I might be able to help you better. > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index e69da01..5a68c33 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -3006,6 +3006,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch * > gdbarch) > set_gdbarch_ps_regnum (gdbarch, AMD64_EFLAGS_REGNUM); /* %eflags */ > set_gdbarch_fp0_regnum (gdbarch, AMD64_ST0_REGNUM); /* %st(0) */ > > + set_gdbarch_deprecated_fp_regnum (gdbarch, AMD64_RBP_REGNUM); /* %rbp */ > + > /* The "default" register numbering scheme for AMD64 is referred to > as the "DWARF Register Number Mapping" in the System V psABI. > The preferred debugging format for all known AMD64 targets is
> Date: Sun, 30 Nov 2014 19:20:36 +0400 > From: Joel Brobecker <brobecker@adacore.com> > > David, > > On Fri, Nov 21, 2014 at 03:58:04PM -0500, David Taylor wrote: > > Sometimes when using STABS on x86-64 GNU/Linux, GDB does not know which > > register to use for the frame pointer and as a result offsets from the > > frame pointer are treated as absolute addresses rather than as > > offsets... > > > > This patch provides a default for when the debug information doesn't > > specify which register to use. > > > > We have seen this problem when debugging problems with a previous > > release of our software (I believe it was built with GCC 4.5.x, if that > > matters). > > > > There were no regressions on x86-64 GNU/Linux. > > > > 2014-11-21 David Taylor <dtaylor@emc.com> > > > > * amd64-tdep.c (amd64_init_abi): Set default frame pointer. > > I don't think your patch is correct, as it is going to affect > the outcome of... > > (gdb) print $fp > > ... for frameless functions compiled with DWARF2. See > std-regs.c:value_of_builtin_frame_fp_reg. > > If we were to apply your patch, it would unconditionally print > the contents of the %rbp register as the result, which is not > what we've been printing so far. > > I wish I could suggest another approach but I haven't had to touch > stabs support in several years, now. If you provided a bit more details > as to what actually happened in the debugger and where, I might be > able to help you better. But any effort is much better spent on switching to DWARF2. STABS support ws already considered obsolete when AMD64 support was added to GDB more than a decade ago.
Joel Brobecker <brobecker@adacore.com> wrote: > David, > > On Fri, Nov 21, 2014 at 03:58:04PM -0500, David Taylor wrote: > > Sometimes when using STABS on x86-64 GNU/Linux, GDB does not know which > > register to use for the frame pointer and as a result offsets from the > > frame pointer are treated as absolute addresses rather than as > > offsets... > > > > This patch provides a default for when the debug information doesn't > > specify which register to use. > > > > We have seen this problem when debugging problems with a previous > > release of our software (I believe it was built with GCC 4.5.x, if that > > matters). > > > > There were no regressions on x86-64 GNU/Linux. > > > > 2014-11-21 David Taylor <dtaylor@emc.com> > > > > * amd64-tdep.c (amd64_init_abi): Set default frame pointer. > > I don't think your patch is correct, as it is going to affect > the outcome of... > > (gdb) print $fp > > ... for frameless functions compiled with DWARF2. See > std-regs.c:value_of_builtin_frame_fp_reg. > > If we were to apply your patch, it would unconditionally print > the contents of the %rbp register as the result, which is not > what we've been printing so far. For a frameless function, I would expect that $fp would be either <unavailable> or would be the frame pointer for the most recent function that had a frame or would just be %rbp. Since a frameless function has no frame, I would consider any of the three to be reasonable. I would also argue that it is an error to ask for the frame pointer of a frameless function, but that an error should not be thrown as the user shouldn't be expected to know that the function is frameless. But, my case is different from print $fp . It's more like: print foo where the debug information says foo lives at $fp - 24. There are functions where the debug information (STABS) specifies an offset from the frame pointer, but does not specify which register is the frame pointer. GDB currently decides to not use any register. It treats the offset as an offset from zero -- put another way, it treats it as an absolute address. Trying to find a local function variable at address -32 or -28 or -24 does not work. It is not mapped and even if it was, it wouldn't be the right address. Now, $rbp - 32 or $rbp - 28 is another matter entirely. GDB is currently saying ``I've been asked for the value of $fp, but I don't know which register is the frame pointer, so I don't have a value for $fp, and I'll just use 0''. The one line fix fixed real problems we had with STABS on our legacy systems. And has caused no problems with DWARF on our current systems. > I wish I could suggest another approach but I haven't had to touch > stabs support in several years, now. If you provided a bit more details > as to what actually happened in the debugger and where, I might be > able to help you better. When I first posted this bug report and fix months ago, it drew zero responses. (I noticed recently that it wasn't in so I sent another message.) At that time I could have told you more. I remember the symptom but not the details. For selected functions if you typed: print <name_local_variable> you would get an error because GDB thought that the variable had an address of -32 or -28 or -24 or similar (depending upon which local variable you selected). The ELF file was using STABS and when I debugged it, GDB said that the variable's address was a small offset from the frame pointer. It didn't know what to use for the frame pointer and ended up using zero. The fix merely decides what to do if you need a frame pointer and haven't been able to otherwise figure out what to use for the frame pointer -- use the traditional default frame pointer register %rbp. If you already have a frame pointer or do not need a frame pointer, nothing is changed. > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > > index e69da01..5a68c33 100644 > > --- a/gdb/amd64-tdep.c > > +++ b/gdb/amd64-tdep.c > > @@ -3006,6 +3006,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch * > > gdbarch) > > set_gdbarch_ps_regnum (gdbarch, AMD64_EFLAGS_REGNUM); /* %eflags */ > > set_gdbarch_fp0_regnum (gdbarch, AMD64_ST0_REGNUM); /* %st(0) */ > > > > + set_gdbarch_deprecated_fp_regnum (gdbarch, AMD64_RBP_REGNUM); /* %rbp */ > > + > > /* The "default" register numbering scheme for AMD64 is referred to > > as the "DWARF Register Number Mapping" in the System V psABI. > > The preferred debugging format for all known AMD64 targets is > > -- > Joel
Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > But any effort is much better spent on switching to DWARF2. STABS > support ws already considered obsolete when AMD64 support was added to > GDB more than a decade ago. For current systems we have earlier this year switched to DWARF, but we will need to support STABS for legacy systems for a few years. As far as it being obsolete, I asked about its status in January 2013 on the gdb list. My take away from that discussion was that it was not deprecated nor obsolete and that it was being maintained; but, that new development was focused on DWARF. [As far as DWARF goes, we use DWARF 3+/4 and GCC 4.7+ as we use -g3 to get macro debug information and want the strings to be consolidated.]
> For current systems we have earlier this year switched to DWARF, but we > will need to support STABS for legacy systems for a few years. Can you say which systems? I am just curious to see what systems out there still use stabs. Also, do you have an idea of long you might have to continue supporting stabs (it's ok if you do not know or do not want to say). > As far as it being obsolete, I asked about its status in January 2013 on > the gdb list. My take away from that discussion was that it was not > deprecated nor obsolete and that it was being maintained; but, that new > development was focused on DWARF. I think that's a fair understanding, although I don't know of any maintainer that still uses DWARF, so maintenance is likely to remain limited to integrating patch submissions such as yours.
On Fri, Nov 21, 2014 at 12:58 PM, David Taylor <dtaylor@emc.com> wrote: > Sometimes when using STABS on x86-64 GNU/Linux, GDB does not know which > register to use for the frame pointer and as a result offsets from the > frame pointer are treated as absolute addresses rather than as > offsets... > > This patch provides a default for when the debug information doesn't > specify which register to use. > > We have seen this problem when debugging problems with a previous > release of our software (I believe it was built with GCC 4.5.x, if that > matters). > > There were no regressions on x86-64 GNU/Linux. > > 2014-11-21 David Taylor <dtaylor@emc.com> > > * amd64-tdep.c (amd64_init_abi): Set default frame pointer. > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index e69da01..5a68c33 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -3006,6 +3006,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch * > gdbarch) > set_gdbarch_ps_regnum (gdbarch, AMD64_EFLAGS_REGNUM); /* %eflags */ > set_gdbarch_fp0_regnum (gdbarch, AMD64_ST0_REGNUM); /* %st(0) */ > > + set_gdbarch_deprecated_fp_regnum (gdbarch, AMD64_RBP_REGNUM); /* %rbp */ > + > /* The "default" register numbering scheme for AMD64 is referred to > as the "DWARF Register Number Mapping" in the System V psABI. > The preferred debugging format for all known AMD64 targets is Hi. I haven't dug too deep to understand whether this is a good fix or not, but I have a request. Given the discussion, I think it's reasonable to assume a future reader of the code will ask "Why is this code here?" One of my pet peeves is having to spend too much time answering that question, and while I would much rather have such questions answered in the code, I also don't want to have to read more than a few sentences (in general - certainly there are cases where a paragraph or two in the code can be invaluable). At any rate ... I'll let you decide whether to add something to the code or to the commit message (or both), but at a minimum please add a full description to the commit message that explains things. Thanks!
> For a frameless function, I would expect that $fp would be either > <unavailable> or would be the frame pointer for the most recent function > that had a frame or would just be %rbp. > > Since a frameless function has no frame, I would consider any of the > three to be reasonable. I would also argue that it is an error to ask > for the frame pointer of a frameless function, but that an error should > not be thrown as the user shouldn't be expected to know that the > function is frameless. I don't know if it's me who used the wrong technical term, or if the terminology is misleading, but for me, a frameless function was meant to mean a function which does not use the frame pointer. But, to me, functions always have a frame, even if maybe in some case perhaps it is possible for their frame to have zero data in it. But in any case, there is always a frame address, and that's what $fp means to me. > When I first posted this bug report and fix months ago, it drew zero > responses. (I noticed recently that it wasn't in so I sent another > message.) At that time I could have told you more. Sorry about that, but I personally have been the busiest I have ever been at work for the last few months. And I have a feeling that others have been too. The only tip I can offer to prevent this from happening is to ping us. Typical protocol we recommend is 2 weeks after submission and every week thereafter. It's sad that it has to be this way, but we have to remember that we are all volunteers trying to do their best. At the moment, I am taking time away from work just to research further and answer you. > There are functions where the debug information (STABS) specifies an > offset from the frame pointer, but does not specify which register is > the frame pointer. GDB currently decides to not use any register. It > treats the offset as an offset from zero -- put another way, it treats > it as an absolute address. [...] > I remember the symptom but not the details. For selected functions if > you typed: > > print <name_local_variable> > > you would get an error because GDB thought that the variable had an > address of -32 or -28 or -24 or similar (depending upon which local > variable you selected). > > The ELF file was using STABS and when I debugged it, GDB said that the > variable's address was a small offset from the frame pointer. It didn't > know what to use for the frame pointer and ended up using zero. > > The fix merely decides what to do if you need a frame pointer and > haven't been able to otherwise figure out what to use for the frame > pointer -- use the traditional default frame pointer register %rbp. > > If you already have a frame pointer or do not need a frame pointer, > nothing is changed. It's actually not a question of having a register or no register. We were able to make this work without your patch before, and I just tested again today and it worked for me. I invite you to first take a look at findvar.c:default_read_var_value, which eventually calls frame.c:get_frame_locals_address. This function then "sniff"es the code to determine which base unwinder to use. And normally in your case, it should probably find amd64-tdep.c:amd64_frame_base. That's the vector which is going to use to compute the "frame_id" object corresponding to your function's frame. And of particular interest in your case is the "this_local" field which is expected to query the frame to return the frame's base address. In your case, it should probably be amd64_frame_base_address which, slightly simplified, returns the frame->cache->base. And going back further, you'll find that the frame cache is computed by amd64_frame_cache, which is a wrapper around amd64_frame_cache_1, which uses prologue parsing to determine the frame's base address. I am guessing that this is where you need to look.
On Tue, Dec 2, 2014 at 7:50 PM, Joel Brobecker <brobecker@adacore.com> wrote: >> When I first posted this bug report and fix months ago, it drew zero >> responses. (I noticed recently that it wasn't in so I sent another >> message.) At that time I could have told you more. > > Sorry about that, but I personally have been the busiest I have ever > been at work for the last few months. And I have a feeling that others > have been too. Yep! :-) > The only tip I can offer to prevent this from happening > is to ping us. Typical protocol we recommend is 2 weeks after submission s/2 weeks/1 week/, right? The ContributionChecklist page in the wiki says "Please ping and keep pinging the patch on a weekly basis." https://sourceware.org/gdb/wiki/ContributionChecklist Also, pings can get buried, at least in my inbox, as all I scan is the subject line. We could add to the protocol the inclusion of "[PING]" in the subject line or some such. That will raise the visibility to me anyway. > and every week thereafter. It's sad that it has to be this way, but > we have to remember that we are all volunteers trying to do their best. > At the moment, I am taking time away from work just to research further > and answer you. Yeah. Anyone should feel free to ping once per week. That still doesn't guarantee a timely response, but at least it'll raise visibility.
> > The only tip I can offer to prevent this from happening > > is to ping us. Typical protocol we recommend is 2 weeks after submission > > s/2 weeks/1 week/, right? > The ContributionChecklist page in the wiki says > "Please ping and keep pinging the patch on a weekly basis." > https://sourceware.org/gdb/wiki/ContributionChecklist I personally feel that 1 week after patch submission is a bit fast for the first ping, but if that's the consensus, then I stand corrected. Thanks for pointing this out!
> Date: Mon, 01 Dec 2014 11:30:30 -0500 > From: David Taylor <dtaylor@usendtaylorx2l.lss.emc.com> > > Joel Brobecker <brobecker@adacore.com> wrote: > > > David, > > > > On Fri, Nov 21, 2014 at 03:58:04PM -0500, David Taylor wrote: > > > Sometimes when using STABS on x86-64 GNU/Linux, GDB does not know which > > > register to use for the frame pointer and as a result offsets from the > > > frame pointer are treated as absolute addresses rather than as > > > offsets... > > > > > > This patch provides a default for when the debug information doesn't > > > specify which register to use. > > > > > > We have seen this problem when debugging problems with a previous > > > release of our software (I believe it was built with GCC 4.5.x, if that > > > matters). > > > > > > There were no regressions on x86-64 GNU/Linux. > > > > > > 2014-11-21 David Taylor <dtaylor@emc.com> > > > > > > * amd64-tdep.c (amd64_init_abi): Set default frame pointer. > > > > I don't think your patch is correct, as it is going to affect > > the outcome of... > > > > (gdb) print $fp > > > > ... for frameless functions compiled with DWARF2. See > > std-regs.c:value_of_builtin_frame_fp_reg. > > > > If we were to apply your patch, it would unconditionally print > > the contents of the %rbp register as the result, which is not > > what we've been printing so far. > > For a frameless function, I would expect that $fp would be either > <unavailable> or would be the frame pointer for the most recent function > that had a frame or would just be %rbp. We deprecated the concept of a frame pointer "register" more than a decade ago. There really isn't such a register on many architectures. On amd64 %rbp is available for the compiler to use for whatever purpose it sees fit. Reintroducing a real frame pointer register in the amd64-tdep.c code would be a mistake. The set_gdbarch_deprecated_fp_regnum() function has "deprecated" in its name for a reason. > Since a frameless function has no frame, I would consider any of the > three to be reasonable. I would also argue that it is an error to ask > for the frame pointer of a frameless function, but that an error should > not be thrown as the user shouldn't be expected to know that the > function is frameless. In GDB there always is a frame. A frameless function is a function that doesn't set up its own frame but re-uses the frame set up by its parent. So that matches "would be the frame pointer for the most recent function that had a frame". > But, my case is different from > > print $fp > > . It's more like: > > print foo > > where the debug information says foo lives at $fp - 24. The question here is how your debug format actually defines $fp here. For DWARF2 this is reasonably well defined. It has the concept of a call frame address CFA and provides enough information to reconstruct that address. For STABS this isn't well defined. Hopefully it matches GDB's idea of "frame base", which is what $fp represents. Few of us have really looked at the STABS code in GDB in the last decae. The mainstream Open Source world moved to DWARF2 long ago, and there is a substantial amount of bitrot in the STABS-related code, both on the producer (GCC) and consumer (GDB) side. But judging from what you write here: > There are functions where the debug information (STABS) specifies an > offset from the frame pointer, but does not specify which register is > the frame pointer. GDB currently decides to not use any register. It > treats the offset as an offset from zero -- put another way, it treats > it as an absolute address. This seems to suggest that the real bug is in GDB's STABS code. It should probably encode the address of your variable as being LOC_LOCAL, in which case the address would be calculated as relative to the "frame locals" address which for most architectures (including amd64) is the same as "frame base". ... > When I first posted this bug report and fix months ago, it drew zero > responses. (I noticed recently that it wasn't in so I sent another > message.) At that time I could have told you more. The problem here is that by choosing to use STABS on amd64 for your platform you're now so far behind the curve that there's basically nobody around with enough knowledge to help you out. All I can say to you is that your suggested fix isn't the right approach.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index e69da01..5a68c33 100644 --- a/gdb/amd64-tdep.c +++ b/gdb/amd64-tdep.c @@ -3006,6 +3006,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch * gdbarch) set_gdbarch_ps_regnum (gdbarch, AMD64_EFLAGS_REGNUM); /* %eflags */ set_gdbarch_fp0_regnum (gdbarch, AMD64_ST0_REGNUM); /* %st(0) */ + set_gdbarch_deprecated_fp_regnum (gdbarch, AMD64_RBP_REGNUM); /* %rbp */ + /* The "default" register numbering scheme for AMD64 is referred to as the "DWARF Register Number Mapping" in the System V psABI.