From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15254 invoked by alias); 10 Sep 2009 02:29:07 -0000 Received: (qmail 14171 invoked by uid 22791); 10 Sep 2009 02:28:57 -0000 X-SWARE-Spam-Status: No, hits=0.3 required=5.0 tests=AWL,BAYES_00,KAM_STOCKTIP X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 10 Sep 2009 02:28:45 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 1663D2BABE8 for ; Wed, 9 Sep 2009 22:28:43 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id bQSWKXEWF9Dx for ; Wed, 9 Sep 2009 22:28:42 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7D2D72BAC01 for ; Wed, 9 Sep 2009 22:28:42 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C397DF589B; Wed, 9 Sep 2009 19:28:34 -0700 (PDT) Date: Thu, 10 Sep 2009 02:29:00 -0000 From: Joel Brobecker To: gdb-patches@sourceware.org Subject: [gdb-7.0] Re: RFC: Mark outer frames Message-ID: <20090910022834.GI20694@adacore.com> References: <20090828213237.GA9175@caradoc.them.org> <20090903225219.GA1266@adacore.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="f+W+jCU1fRNres8c" Content-Disposition: inline In-Reply-To: <20090903225219.GA1266@adacore.com> User-Agent: Mutt/1.5.18 (2008-05-17) 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 X-SW-Source: 2009-09/txt/msg00271.txt.bz2 --f+W+jCU1fRNres8c Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 3204 Hello, We need to make a decision. We have a failed-assertion on x86-linux, and it is a regression compared to gdb-6.8. We have decided that we had to fix this issue before starting the gdb-7.0 release. This failed assertion is one symptom of the same issue that manifests itself in several ways (I think we have at least a couple of PRs for that). Daniel sent a patch as an RFC, because it implements a temporary solution to a larger design shortcoming: > The magic outer frame ID is, IMO, a workaround. What we really want > is to have frame IDs wherever we can. Either "stack address > uncertain, but function known" or even "stack address and function > known, but outerness detected". This requires changes to the > unwinding interface, and additional changes to each affected unwinder > to build the best IDs we can and to mark the outer frame some other > way. The issue with the current patch is summarized: > The obvious pitfall is that the outer frame isn't a single consistent > frame. So there's an ugly bit in infrun that says if we set the stack > pointer while inside an outer frame, and suddenly we're in a frame we > think we can unwind from (mostly incorrectly at this point), then > we've not changed functions. Otherwise stepping through _start will > blow up on some platforms I tried. The "ugly" piece in infrun.c is the following hunk: + /* The outer_frame_id check is a heuristic to detect stepping + through startup code. If we step over an instruction which + sets the stack pointer from an invalid value to a valid value, + we may detect that as a subroutine call from the mythical + "outermost" function. This could be fixed by marking + outermost frames as !stack_p,code_p,special_p. Then the + initial outermost frame, before sp was valid, would + have code_addr == &_start. See the commend in frame_id_eq + for more. */ if (!frame_id_eq (get_stack_frame_id (frame), ecs->event_thread->step_stack_frame_id) - && frame_id_eq (frame_unwind_caller_id (frame), - ecs->event_thread->step_stack_frame_id)) + && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()), + ecs->event_thread->step_stack_frame_id) + && (!frame_id_eq (ecs->event_thread->step_stack_frame_id, + outer_frame_id) + || step_start_function != find_pc_function (stop_pc)))) Maybe the patch is not perfect, but we're a little under pressure if we want to release soon, and the patch itself is fairly localized. If we want to only fix the PR that blocks us, we can try the patch that I am attaching, which guards "info frame" against null frame IDs. It's not very invasive, but it is at least as ugly as Daniel finds his patch ugly, and only fixes the "info frame" command. However, if there are objections to Daniel's patch, then my hack allows us to get going for the relase without a major delay. Any objection to Daniel's patch? I'm going to be aggressive here, and apply on Friday unless someone objects. I am attaching Daniel's patch for easier reference. Mine is attached next. -- Joel --f+W+jCU1fRNres8c Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="outer-frame-id.diff" Content-length: 13088 2009-08-28 Daniel Jacobowitz * frame.c (get_frame_id): Default to outer_frame_id if the this_id method does not supply an ID. Assert that the result is not null_frame_id. (outer_frame_id): New. (frame_id_p): Accept outer_frame_id. (frame_id_eq): Allow outer_frame_id to be equal to itself. (frame_find_by_id): Revert previous local workarounds. (get_prev_frame_1): Adjust end-of-stack check to test outer_frame_id. * frame.h (null_frame_id, frame_id_p): Update comments. (outer_frame_id): Declare. * infrun.c (handle_inferior_event): Do not treat all steps from the outermost frame as subroutine calls. * libunwind-frame.c (libunwind_frame_this_id): Do not clear THIS_ID. * hppa-tdep.c (hppa_stub_frame_this_id): Likewise. * ia64-tdep.c (ia64_frame_this_id): Likewise. (ia64_libunwind_frame_this_id, ia64_libunwind_sigtramp_frame_this_id): Use outer_frame_id instead of null_frame_id. * amd64obsd-tdep.c (amd64obsd_trapframe_cache): Use outer_frame_id. * i386obsd-tdep.c (i386obsd_trapframe_cache): Likewise. * inline-frame.c (inline_frame_this_id): Refuse outer_frame_id. * thread.c (restore_selected_frame): Update comment and remove frame_id_p check. gdb/doc/ * gdbint.texinfo (Unwinding the Frame ID): Reference outer_frame_id. Index: amd64obsd-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/amd64obsd-tdep.c,v retrieving revision 1.29 diff -u -p -r1.29 amd64obsd-tdep.c --- amd64obsd-tdep.c 2 Jul 2009 17:25:52 -0000 1.29 +++ amd64obsd-tdep.c 28 Aug 2009 21:14:03 -0000 @@ -380,7 +380,7 @@ amd64obsd_trapframe_cache (struct frame_ if ((cs & I386_SEL_RPL) == I386_SEL_UPL) { /* Trap from user space; terminate backtrace. */ - trad_frame_set_id (cache, null_frame_id); + trad_frame_set_id (cache, outer_frame_id); } else { Index: frame.c =================================================================== RCS file: /cvs/src/src/gdb/frame.c,v retrieving revision 1.271 diff -u -p -r1.271 frame.c --- frame.c 2 Jul 2009 17:25:53 -0000 1.271 +++ frame.c 28 Aug 2009 21:14:03 -0000 @@ -291,7 +291,10 @@ get_frame_id (struct frame_info *fi) if (fi->unwind == NULL) fi->unwind = frame_unwind_find_by_frame (fi, &fi->prologue_cache); /* Find THIS frame's ID. */ + /* Default to outermost if no ID is found. */ + fi->this_id.value = outer_frame_id; fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value); + gdb_assert (frame_id_p (fi->this_id.value)); fi->this_id.p = 1; if (frame_debug) { @@ -328,6 +331,7 @@ frame_unwind_caller_id (struct frame_inf } const struct frame_id null_frame_id; /* All zeros. */ +const struct frame_id outer_frame_id = { 0, 0, 0, 0, 0, 1, 0 }; struct frame_id frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr, @@ -369,6 +373,9 @@ frame_id_p (struct frame_id l) int p; /* The frame is valid iff it has a valid stack address. */ p = l.stack_addr_p; + /* outer_frame_id is also valid. */ + if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0) + p = 1; if (frame_debug) { fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l="); @@ -391,7 +398,14 @@ int frame_id_eq (struct frame_id l, struct frame_id r) { int eq; - if (!l.stack_addr_p || !r.stack_addr_p) + if (!l.stack_addr_p && l.special_addr_p && !r.stack_addr_p && r.special_addr_p) + /* The outermost frame marker is equal to itself. This is the + dodgy think about outer_frame_id, since between execution steps + we might step into another function - from which we can't + unwind either. More thought required to get rid of + outer_frame_id. */ + eq = 1; + else if (!l.stack_addr_p || !r.stack_addr_p) /* Like a NaN, if either ID is invalid, the result is false. Note that a frame ID is invalid iff it is the null frame ID. */ eq = 0; @@ -1376,7 +1390,7 @@ get_prev_frame_1 (struct frame_info *thi unwind to the prev frame. Be careful to not apply this test to the sentinel frame. */ this_id = get_frame_id (this_frame); - if (this_frame->level >= 0 && !frame_id_p (this_id)) + if (this_frame->level >= 0 && frame_id_eq (this_id, outer_frame_id)) { if (frame_debug) { Index: frame.h =================================================================== RCS file: /cvs/src/src/gdb/frame.h,v retrieving revision 1.178 diff -u -p -r1.178 frame.h --- frame.h 2 Jul 2009 17:09:28 -0000 1.178 +++ frame.h 28 Aug 2009 21:14:04 -0000 @@ -142,9 +142,14 @@ struct frame_id /* Methods for constructing and comparing Frame IDs. */ -/* For convenience. All fields are zero. */ +/* For convenience. All fields are zero. This means "there is no frame". */ extern const struct frame_id null_frame_id; +/* This means "there is no frame ID, but there is a frame". It should be + replaced by best-effort frame IDs for the outermost frame, somehow. + The implementation is only special_addr_p set. */ +extern const struct frame_id outer_frame_id; + /* Flag to control debugging. */ extern int frame_debug; @@ -170,7 +175,8 @@ extern struct frame_id frame_id_build_sp extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr); /* Returns non-zero when L is a valid frame (a valid frame has a - non-zero .base). */ + non-zero .base). The outermost frame is valid even without an + ID. */ extern int frame_id_p (struct frame_id l); /* Returns non-zero when L is a valid frame representing an inlined Index: hppa-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/hppa-tdep.c,v retrieving revision 1.270 diff -u -p -r1.270 hppa-tdep.c --- hppa-tdep.c 2 Jul 2009 17:25:54 -0000 1.270 +++ hppa-tdep.c 28 Aug 2009 21:14:04 -0000 @@ -2427,8 +2427,6 @@ hppa_stub_frame_this_id (struct frame_in if (info) *this_id = frame_id_build (info->base, get_frame_func (this_frame)); - else - *this_id = null_frame_id; } static struct value * Index: i386obsd-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/i386obsd-tdep.c,v retrieving revision 1.37 diff -u -p -r1.37 i386obsd-tdep.c --- i386obsd-tdep.c 2 Jul 2009 17:25:54 -0000 1.37 +++ i386obsd-tdep.c 28 Aug 2009 21:14:04 -0000 @@ -376,7 +376,7 @@ i386obsd_trapframe_cache (struct frame_i if ((cs & I386_SEL_RPL) == I386_SEL_UPL) { /* Trap from user space; terminate backtrace. */ - trad_frame_set_id (cache, null_frame_id); + trad_frame_set_id (cache, outer_frame_id); } else { Index: ia64-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/ia64-tdep.c,v retrieving revision 1.196 diff -u -p -r1.196 ia64-tdep.c --- ia64-tdep.c 25 Aug 2009 14:06:47 -0000 1.196 +++ ia64-tdep.c 28 Aug 2009 21:14:05 -0000 @@ -1748,9 +1748,7 @@ ia64_frame_this_id (struct frame_info *t ia64_frame_cache (this_frame, this_cache); /* If outermost frame, mark with null frame id. */ - if (cache->base == 0) - (*this_id) = null_frame_id; - else + if (cache->base != 0) (*this_id) = frame_id_build_special (cache->base, cache->pc, cache->bsp); if (gdbarch_debug >= 1) fprintf_unfiltered (gdb_stdlog, @@ -2764,15 +2762,14 @@ ia64_libunwind_frame_this_id (struct fra { struct gdbarch *gdbarch = get_frame_arch (this_frame); enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); - struct frame_id id; + struct frame_id id = outer_frame_id; char buf[8]; CORE_ADDR bsp; - libunwind_frame_this_id (this_frame, this_cache, &id); - if (frame_id_eq (id, null_frame_id)) + if (frame_id_eq (id, outer_frame_id)) { - (*this_id) = null_frame_id; + (*this_id) = outer_frame_id; return; } @@ -2897,13 +2894,13 @@ ia64_libunwind_sigtramp_frame_this_id (s enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); char buf[8]; CORE_ADDR bsp; - struct frame_id id; + struct frame_id id = outer_frame_id; CORE_ADDR prev_ip; libunwind_frame_this_id (this_frame, this_cache, &id); - if (frame_id_eq (id, null_frame_id)) + if (frame_id_eq (id, outer_frame_id)) { - (*this_id) = null_frame_id; + (*this_id) = outer_frame_id; return; } Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.407 diff -u -p -r1.407 infrun.c --- infrun.c 21 Aug 2009 18:54:44 -0000 1.407 +++ infrun.c 28 Aug 2009 21:14:06 -0000 @@ -3796,10 +3796,22 @@ infrun: not switching back to stepped th NOTE: frame_id_eq will never report two invalid frame IDs as being equal, so to get into this block, both the current and previous frame must have valid frame IDs. */ + /* The outer_frame_id check is a heuristic to detect stepping + through startup code. If we step over an instruction which + sets the stack pointer from an invalid value to a valid value, + we may detect that as a subroutine call from the mythical + "outermost" function. This could be fixed by marking + outermost frames as !stack_p,code_p,special_p. Then the + initial outermost frame, before sp was valid, would + have code_addr == &_start. See the commend in frame_id_eq + for more. */ if (!frame_id_eq (get_stack_frame_id (frame), ecs->event_thread->step_stack_frame_id) - && frame_id_eq (frame_unwind_caller_id (frame), - ecs->event_thread->step_stack_frame_id)) + && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()), + ecs->event_thread->step_stack_frame_id) + && (!frame_id_eq (ecs->event_thread->step_stack_frame_id, + outer_frame_id) + || step_start_function != find_pc_function (stop_pc)))) { CORE_ADDR real_stop_pc; Index: inline-frame.c =================================================================== RCS file: /cvs/src/src/gdb/inline-frame.c,v retrieving revision 1.1 diff -u -p -r1.1 inline-frame.c --- inline-frame.c 28 Jun 2009 00:20:22 -0000 1.1 +++ inline-frame.c 28 Aug 2009 21:14:06 -0000 @@ -148,6 +148,10 @@ inline_frame_this_id (struct frame_info frame"). This will take work. */ gdb_assert (frame_id_p (*this_id)); + /* For now, require we don't match outer_frame_id either (see + comment above). */ + gdb_assert (!frame_id_eq (*this_id, outer_frame_id)); + /* Future work NOTE: Alexandre Oliva applied a patch to GCC 4.3 which generates DW_AT_entry_pc for inlined functions when possible. If this attribute is available, we should use it Index: libunwind-frame.c =================================================================== RCS file: /cvs/src/src/gdb/libunwind-frame.c,v retrieving revision 1.23 diff -u -p -r1.23 libunwind-frame.c --- libunwind-frame.c 3 Jan 2009 05:57:52 -0000 1.23 +++ libunwind-frame.c 28 Aug 2009 21:14:06 -0000 @@ -278,8 +278,6 @@ libunwind_frame_this_id (struct frame_in if (cache != NULL) (*this_id) = frame_id_build (cache->base, cache->func_addr); - else - (*this_id) = null_frame_id; } struct value * Index: thread.c =================================================================== RCS file: /cvs/src/src/gdb/thread.c,v retrieving revision 1.115 diff -u -p -r1.115 thread.c --- thread.c 2 Jul 2009 21:57:27 -0000 1.115 +++ thread.c 28 Aug 2009 21:14:06 -0000 @@ -885,15 +885,11 @@ restore_selected_frame (struct frame_id frame = find_relative_frame (get_current_frame (), &count); if (count == 0 && frame != NULL - /* Either the frame ids match, of they're both invalid. The - latter case is not failsafe, but since it's highly unlikely + /* The frame ids must match - either both valid or both outer_frame_id. + The latter case is not failsafe, but since it's highly unlikely the search by level finds the wrong frame, it's 99.9(9)% of the time (for all practical purposes) safe. */ - && (frame_id_eq (get_frame_id (frame), a_frame_id) - /* Note: could be better to check every frame_id - member for equality here. */ - || (!frame_id_p (get_frame_id (frame)) - && !frame_id_p (a_frame_id)))) + && frame_id_eq (get_frame_id (frame), a_frame_id)) { /* Cool, all is fine. */ select_frame (frame); Index: doc/gdbint.texinfo =================================================================== RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v retrieving revision 1.311 diff -u -p -r1.311 gdbint.texinfo --- doc/gdbint.texinfo 26 Aug 2009 04:16:38 -0000 1.311 +++ doc/gdbint.texinfo 28 Aug 2009 21:14:07 -0000 @@ -2042,7 +2042,7 @@ address as its caller. On some platform the ID to further disambiguate frames---for instance, on IA-64 the separate register stack address is included in the ID. -An invalid frame ID (@code{null_frame_id}) returned from the +An invalid frame ID (@code{outer_frame_id}) returned from the @code{this_id} method means to stop unwinding after this frame. @section Unwinding Registers --f+W+jCU1fRNres8c Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="info-frame.diff" Content-length: 3000 commit ddc49fb123fbca081091847a8f0f7a66475518b8 Author: Joel Brobecker Date: Wed Sep 9 22:09:53 2009 -0400 * stack.c (frame_info): Do not try to print the location of the saved registers for frames that have a NULL frame ID. diff --git a/gdb/stack.c b/gdb/stack.c index 1c37801..99459fe 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -986,6 +986,7 @@ static void frame_info (char *addr_exp, int from_tty) { struct frame_info *fi; + int outer_frame_p; struct symtab_and_line sal; struct symbol *func; struct symtab *s; @@ -1001,6 +1002,16 @@ frame_info (char *addr_exp, int from_tty) fi = parse_frame_specification_1 (addr_exp, "No stack.", &selected_frame_p); gdbarch = get_frame_arch (fi); + /* The outer frame may have a null frame ID. This can happen when + to a program that is stopped at the entry point, for instance. + We have to be careful of this case, because we cannot unwind registers + unless we have a valid frame ID. + + FIXME: brobecker/2009-09-09: This distinction is necessary only + because the unwinding machinery does not have a way to distinguish + null frames from outer frames. If we fix that, we should be able + to get rid of this check and all the associated code that uses it. */ + outer_frame_p = !frame_id_p (get_frame_id (fi)); /* Name of the value returned by get_frame_pc(). Per comments, "pc" is not a good name. */ if (gdbarch_pc_regnum (gdbarch) >= 0) @@ -1077,9 +1088,15 @@ frame_info (char *addr_exp, int from_tty) if (sal.symtab) printf_filtered (" (%s:%d)", sal.symtab->filename, sal.line); puts_filtered ("; "); - wrap_here (" "); - printf_filtered ("saved %s ", pc_regname); - fputs_filtered (paddress (gdbarch, frame_unwind_caller_pc (fi)), gdb_stdout); + if (!outer_frame_p) + { + /* This is the outer frame: None of the registers have been saved. + So do not try to print the address were the PC has been saved. */ + wrap_here (" "); + printf_filtered ("saved %s ", pc_regname); + fputs_filtered (paddress (gdbarch, frame_unwind_caller_pc (fi)), + gdb_stdout); + } printf_filtered ("\n"); if (calling_frame_info == NULL) @@ -1175,6 +1192,18 @@ frame_info (char *addr_exp, int from_tty) int i; int need_nl = 1; + /* We need to be careful with the outer frame which has a null + frame ID, as most of the register unwinding routines assume + a non-null frame ID. We know that there are no saved register + at this point of the execution anyway, so just report this to + the user and return. */ + if (outer_frame_p) + { + printf_filtered (" no saved registers.\n"); + do_cleanups (back_to); + return; + } + /* The sp is special; what's displayed isn't the save address, but the value of the previous frame's sp. This is a legacy thing, at one stage the frame cached the previous frame's SP instead --f+W+jCU1fRNres8c--