Message ID | 1454853751-18455-1-git-send-email-koriakin@0x04.net |
---|---|
State | New, archived |
Headers |
Received: (qmail 66838 invoked by alias); 7 Feb 2016 14:02:35 -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 66818 invoked by uid 89); 7 Feb 2016 14:02:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=HContent-Transfer-Encoding:8bit X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 07 Feb 2016 14:02:34 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id B72433FE0F for <gdb-patches@sourceware.org>; Sun, 7 Feb 2016 15:03:20 +0100 (CET) Received: by hogfather.0x04.net (Postfix, from userid 1000) id DA5D258008E; Sun, 7 Feb 2016 15:02:31 +0100 (CET) From: =?UTF-8?q?Marcin=20Ko=C5=9Bcielnicki?= <koriakin@0x04.net> To: gdb-patches@sourceware.org Cc: =?UTF-8?q?Marcin=20Ko=C5=9Bcielnicki?= <koriakin@0x04.net> Subject: [PATCH 4/8] gdb/s390: Fill gen_return_address hook. Date: Sun, 7 Feb 2016 15:02:31 +0100 Message-Id: <1454853751-18455-1-git-send-email-koriakin@0x04.net> In-Reply-To: <1453637529-26972-5-git-send-email-koriakin@0x04.net> References: <1453637529-26972-5-git-send-email-koriakin@0x04.net> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Marcin Kościelnicki
Feb. 7, 2016, 2:02 p.m. UTC
gdb/ChangeLog: * s390-linux-tdep.c (s390_gen_return_address): New function. (s390_gdbarch_init): Fill gen_return_address hook. --- Added missing comment. gdb/ChangeLog | 5 +++++ gdb/s390-linux-tdep.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)
Comments
Ping. On 07/02/16 15:02, Marcin Kościelnicki wrote: > gdb/ChangeLog: > > * s390-linux-tdep.c (s390_gen_return_address): New function. > (s390_gdbarch_init): Fill gen_return_address hook. > --- > Added missing comment. > > gdb/ChangeLog | 5 +++++ > gdb/s390-linux-tdep.c | 13 +++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 6260040..0cf8bfc 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,5 +1,10 @@ > 2016-02-07 Marcin Kościelnicki <koriakin@0x04.net> > > + * s390-linux-tdep.c (s390_gen_return_address): New function. > + (s390_gdbarch_init): Fill gen_return_address hook. > + > +2016-02-07 Marcin Kościelnicki <koriakin@0x04.net> > + > * s390-linux-tdep.c (s390_ax_pseudo_register_collect): New function. > (s390_ax_pseudo_register_push_stack): New function. > (s390_gdbarch_init): Fill ax_pseudo_register_collect and > diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c > index 97bd564..0b91ed1 100644 > --- a/gdb/s390-linux-tdep.c > +++ b/gdb/s390-linux-tdep.c > @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, > return 0; > } > > +/* The "gen_return_address" gdbarch method. */ > + > +static void > +s390_gen_return_address (struct gdbarch *gdbarch, > + struct agent_expr *ax, struct axs_value *value, > + CORE_ADDR scope) > +{ > + value->type = register_type (gdbarch, S390_R14_REGNUM); > + value->kind = axs_lvalue_register; > + value->u.reg = S390_R14_REGNUM; > +} > + > > /* A helper for s390_software_single_step, decides if an instruction > is a partial-execution instruction that needs to be executed until > @@ -7970,6 +7982,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > s390_ax_pseudo_register_collect); > set_gdbarch_ax_pseudo_register_push_stack > (gdbarch, s390_ax_pseudo_register_push_stack); > + set_gdbarch_gen_return_address (gdbarch, s390_gen_return_address); > tdesc_use_registers (gdbarch, tdesc, tdesc_data); > set_gdbarch_register_name (gdbarch, s390_register_name); > >
Ping. On 25/02/16 20:23, Marcin Kościelnicki wrote: > Ping. > > On 07/02/16 15:02, Marcin Kościelnicki wrote: >> gdb/ChangeLog: >> >> * s390-linux-tdep.c (s390_gen_return_address): New function. >> (s390_gdbarch_init): Fill gen_return_address hook. >> --- >> Added missing comment. >> >> gdb/ChangeLog | 5 +++++ >> gdb/s390-linux-tdep.c | 13 +++++++++++++ >> 2 files changed, 18 insertions(+) >> >> diff --git a/gdb/ChangeLog b/gdb/ChangeLog >> index 6260040..0cf8bfc 100644 >> --- a/gdb/ChangeLog >> +++ b/gdb/ChangeLog >> @@ -1,5 +1,10 @@ >> 2016-02-07 Marcin Kościelnicki <koriakin@0x04.net> >> >> + * s390-linux-tdep.c (s390_gen_return_address): New function. >> + (s390_gdbarch_init): Fill gen_return_address hook. >> + >> +2016-02-07 Marcin Kościelnicki <koriakin@0x04.net> >> + >> * s390-linux-tdep.c (s390_ax_pseudo_register_collect): New >> function. >> (s390_ax_pseudo_register_push_stack): New function. >> (s390_gdbarch_init): Fill ax_pseudo_register_collect and >> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c >> index 97bd564..0b91ed1 100644 >> --- a/gdb/s390-linux-tdep.c >> +++ b/gdb/s390-linux-tdep.c >> @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct >> gdbarch *gdbarch, >> return 0; >> } >> >> +/* The "gen_return_address" gdbarch method. */ >> + >> +static void >> +s390_gen_return_address (struct gdbarch *gdbarch, >> + struct agent_expr *ax, struct axs_value *value, >> + CORE_ADDR scope) >> +{ >> + value->type = register_type (gdbarch, S390_R14_REGNUM); >> + value->kind = axs_lvalue_register; >> + value->u.reg = S390_R14_REGNUM; >> +} >> + >> >> /* A helper for s390_software_single_step, decides if an instruction >> is a partial-execution instruction that needs to be executed until >> @@ -7970,6 +7982,7 @@ s390_gdbarch_init (struct gdbarch_info info, >> struct gdbarch_list *arches) >> s390_ax_pseudo_register_collect); >> set_gdbarch_ax_pseudo_register_push_stack >> (gdbarch, s390_ax_pseudo_register_push_stack); >> + set_gdbarch_gen_return_address (gdbarch, s390_gen_return_address); >> tdesc_use_registers (gdbarch, tdesc, tdesc_data); >> set_gdbarch_register_name (gdbarch, s390_register_name); >> >> >
On Sun, Feb 07 2016, Marcin Kościelnicki wrote: > diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c > index 97bd564..0b91ed1 100644 > --- a/gdb/s390-linux-tdep.c > +++ b/gdb/s390-linux-tdep.c > @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, > return 0; > } > > +/* The "gen_return_address" gdbarch method. */ > + > +static void > +s390_gen_return_address (struct gdbarch *gdbarch, > + struct agent_expr *ax, struct axs_value *value, > + CORE_ADDR scope) > +{ > + value->type = register_type (gdbarch, S390_R14_REGNUM); > + value->kind = axs_lvalue_register; > + value->u.reg = S390_R14_REGNUM; > +} Under which circumstances is this supposed to work? And how reliable does it need to be? The ABI only guarantees that r14 holds the return address at function entry. Anywhere else it likely doesn't. -- Andreas
On 11/03/16 12:20, Andreas Arnez wrote: > On Sun, Feb 07 2016, Marcin Kościelnicki wrote: > >> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c >> index 97bd564..0b91ed1 100644 >> --- a/gdb/s390-linux-tdep.c >> +++ b/gdb/s390-linux-tdep.c >> @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, >> return 0; >> } >> >> +/* The "gen_return_address" gdbarch method. */ >> + >> +static void >> +s390_gen_return_address (struct gdbarch *gdbarch, >> + struct agent_expr *ax, struct axs_value *value, >> + CORE_ADDR scope) >> +{ >> + value->type = register_type (gdbarch, S390_R14_REGNUM); >> + value->kind = axs_lvalue_register; >> + value->u.reg = S390_R14_REGNUM; >> +} > > Under which circumstances is this supposed to work? And how reliable > does it need to be? The ABI only guarantees that r14 holds the return > address at function entry. Anywhere else it likely doesn't. > > -- > Andreas > Quoting gdbarch.sh: # Generate bytecodes to collect the return address in a frame. # Since the bytecodes run on the target, possibly with GDB not even # connected, the full unwinding machinery is not available, and # typically this function will issue bytecodes for one or more likely # places that the return address may be found. m:void:gen_return_address:struct agent_expr *ax, struct axs_value *value, CORE_ADDR scope:ax, value, scope::default_gen_return_address::0 ie. it's supposed to collect some value that will likely help the unwinder - if it collects the wrong thing, the unwinder (knowing the right place for sure) will simply consider the previous frame PC to be unavailable. We could also try to collect 14*<wordsize>(%r11), hoping that's the save slot for %r14, but the interface unfortunately doesn't support collecting multiple values (no matter what the comment above says). Unfortunately, this interface is just not very well-designed - both x86 and aarch64 just take a shot in the dark like this patch. A better way would be to reuse the existing unwinders and remove this hook altogether, or (for while-stepping, where we can't predict the PC) actually allow multiple values and aim at a few likely locations. But IMO that's not in scope for this patchset.
On Fri, Mar 11 2016, Marcin Kościelnicki wrote: > We could also try to collect 14*<wordsize>(%r11), hoping that's the > save slot for %r14, but the interface unfortunately doesn't support > collecting multiple values (no matter what the comment above says). Nah, that doesn't help either, since most functions don't use r11 as a frame pointer. There is just no way to locate the return address unless we have call frame information or perform code analysis. > Unfortunately, this interface is just not very well-designed - both > x86 and aarch64 just take a shot in the dark like this patch. A > better way would be to reuse the existing unwinders and remove this > hook altogether, or (for while-stepping, where we can't predict the > PC) actually allow multiple values and aim at a few likely locations. > But IMO that's not in scope for this patchset. The point I was trying to make is that r14 is fairly *unlikely* to contain the return address, unless we're near function entry. If we just called a function, then r14 contains an address within our own function. Otherwise r14 can also contain something else entirely. Is there a way to admit that we don't know the return address? What if we always return garbage? E.g., maybe it's better to always return 0?
On 11/03/16 13:18, Andreas Arnez wrote: > On Fri, Mar 11 2016, Marcin Kościelnicki wrote: > >> We could also try to collect 14*<wordsize>(%r11), hoping that's the >> save slot for %r14, but the interface unfortunately doesn't support >> collecting multiple values (no matter what the comment above says). > > Nah, that doesn't help either, since most functions don't use r11 as a > frame pointer. There is just no way to locate the return address unless > we have call frame information or perform code analysis. > >> Unfortunately, this interface is just not very well-designed - both >> x86 and aarch64 just take a shot in the dark like this patch. A >> better way would be to reuse the existing unwinders and remove this >> hook altogether, or (for while-stepping, where we can't predict the >> PC) actually allow multiple values and aim at a few likely locations. >> But IMO that's not in scope for this patchset. > > The point I was trying to make is that r14 is fairly *unlikely* to > contain the return address, unless we're near function entry. If we > just called a function, then r14 contains an address within our own > function. Otherwise r14 can also contain something else entirely. Well, it works for leaf functions... not much, but not totally useless either. > > Is there a way to admit that we don't know the return address? What if > we always return garbage? E.g., maybe it's better to always return 0? > We can always error() in there (and KFAIL the testcase in gdb.trace that exercises it). However, returning garbage here doesn't result in garbage backtrace - this only collects data, if the unwinder actually doing the work later determines it should look for the return address on the stack, it'll just ignore our collected $r14 and consider the return address unavailable (unless another collect rule happened to match it).
On Fri, Mar 11 2016, Marcin Kościelnicki wrote: > We can always error() in there (and KFAIL the testcase in gdb.trace > that exercises it). However, returning garbage here doesn't result in > garbage backtrace - this only collects data, if the unwinder actually > doing the work later determines it should look for the return address > on the stack, it'll just ignore our collected $r14 and consider the > return address unavailable (unless another collect rule happened to > match it). Well, from that test case it appears that `$_ret' is generally not expected to work very reliably. Since r14 usually does work near function entry, this may be sufficient for now. So I'm OK with the patch. Please add a small comment stating that this is a best-can-do approach that usually works near function entry and may yield wrong results otherwise.
On 03/11/2016 03:31 PM, Andreas Arnez wrote: > So I'm OK with the patch. Please add a small comment stating that this > is a best-can-do approach that usually works near function entry and may > yield wrong results otherwise. I think that should be put in the manual, even. Users will also trip on this, not just our tests. Thanks, Pedro Alves
On 11/03/16 16:31, Andreas Arnez wrote: > On Fri, Mar 11 2016, Marcin Kościelnicki wrote: > >> We can always error() in there (and KFAIL the testcase in gdb.trace >> that exercises it). However, returning garbage here doesn't result in >> garbage backtrace - this only collects data, if the unwinder actually >> doing the work later determines it should look for the return address >> on the stack, it'll just ignore our collected $r14 and consider the >> return address unavailable (unless another collect rule happened to >> match it). > > Well, from that test case it appears that `$_ret' is generally not > expected to work very reliably. Since r14 usually does work near > function entry, this may be sufficient for now. > > So I'm OK with the patch. Please add a small comment stating that this > is a best-can-do approach that usually works near function entry and may > yield wrong results otherwise. > Thanks, pushed with this comment: /* The "gen_return_address" gdbarch method. Since this is supposed to be just a best-effort method, and we don't really have the means to run the full unwinder here, just collect the link register. */
On Sun, Mar 13 2016, Marcin Kościelnicki wrote: > On 11/03/16 16:31, Andreas Arnez wrote: >> On Fri, Mar 11 2016, Marcin Kościelnicki wrote: >> >>> We can always error() in there (and KFAIL the testcase in gdb.trace >>> that exercises it). However, returning garbage here doesn't result in >>> garbage backtrace - this only collects data, if the unwinder actually >>> doing the work later determines it should look for the return address >>> on the stack, it'll just ignore our collected $r14 and consider the >>> return address unavailable (unless another collect rule happened to >>> match it). >> >> Well, from that test case it appears that `$_ret' is generally not >> expected to work very reliably. Since r14 usually does work near >> function entry, this may be sufficient for now. >> >> So I'm OK with the patch. Please add a small comment stating that this >> is a best-can-do approach that usually works near function entry and may >> yield wrong results otherwise. >> > > Thanks, pushed with this comment: > > /* The "gen_return_address" gdbarch method. Since this is supposed to be > just a best-effort method, and we don't really have the means to run > the full unwinder here, just collect the link register. */ Very good, thanks.
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 6260040..0cf8bfc 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,10 @@ 2016-02-07 Marcin Kościelnicki <koriakin@0x04.net> + * s390-linux-tdep.c (s390_gen_return_address): New function. + (s390_gdbarch_init): Fill gen_return_address hook. + +2016-02-07 Marcin Kościelnicki <koriakin@0x04.net> + * s390-linux-tdep.c (s390_ax_pseudo_register_collect): New function. (s390_ax_pseudo_register_push_stack): New function. (s390_gdbarch_init): Fill ax_pseudo_register_collect and diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c index 97bd564..0b91ed1 100644 --- a/gdb/s390-linux-tdep.c +++ b/gdb/s390-linux-tdep.c @@ -627,6 +627,18 @@ s390_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, return 0; } +/* The "gen_return_address" gdbarch method. */ + +static void +s390_gen_return_address (struct gdbarch *gdbarch, + struct agent_expr *ax, struct axs_value *value, + CORE_ADDR scope) +{ + value->type = register_type (gdbarch, S390_R14_REGNUM); + value->kind = axs_lvalue_register; + value->u.reg = S390_R14_REGNUM; +} + /* A helper for s390_software_single_step, decides if an instruction is a partial-execution instruction that needs to be executed until @@ -7970,6 +7982,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) s390_ax_pseudo_register_collect); set_gdbarch_ax_pseudo_register_push_stack (gdbarch, s390_ax_pseudo_register_push_stack); + set_gdbarch_gen_return_address (gdbarch, s390_gen_return_address); tdesc_use_registers (gdbarch, tdesc, tdesc_data); set_gdbarch_register_name (gdbarch, s390_register_name);