From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16827 invoked by alias); 28 Aug 2009 21:32:53 -0000 Received: (qmail 16819 invoked by uid 22791); 28 Aug 2009 21:32:51 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 28 Aug 2009 21:32:43 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id D211D7C09A; Fri, 28 Aug 2009 21:32:40 +0000 (GMT) Received: from caradoc.them.org (209.195.188.212.nauticom.net [209.195.188.212]) by nan.false.org (Postfix) with ESMTP id 3CD44108C6; Fri, 28 Aug 2009 21:32:40 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1Mh93l-0002l8-J8; Fri, 28 Aug 2009 17:32:37 -0400 Date: Fri, 28 Aug 2009 22:16:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sourceware.org, Joel Brobecker Subject: RFC: Mark outer frames Message-ID: <20090828213237.GA9175@caradoc.them.org> Mail-Followup-To: gdb-patches@sourceware.org, Joel Brobecker MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) X-IsSubscribed: yes 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-08/txt/msg00525.txt.bz2 This patch is a very lightly tested HEAD port of a patch I've been using for several months. I'm posting this because, as Joel reminded me, it might help out with PR/9711 (Quadratic slowdown for "where" command). It might not, too :-) I haven't even tried. This patch was originally for an entirely different problem, namely, the dreaded "Value is not active" error. What happens with that is that we connect to some target, and we're in the middle of its startup code before there's a stack frame. No unwinder thinks we have a way out of this frame, so nothing returns a valid this_id. So the frame id is null_frame_id - which is overloaded to mean 'unknown id' and 'no frame at all'. The value checks are using it one way and the unwinder is using it the other. So this patch breaks it in half. There's null_frame_id, and a new outer_frame_id which is assigned to any frame that no unwinder has an ID for. 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 reason this might help with 9711 is that we'll have a valid frame ID for the register values of the outermost frame. Previously we had nothing. I'm not entirely sure, now, why I was convinced that would help. Thoughts? Does this actually help with 9711? Is it too ugly to live? -- Daniel Jacobowitz CodeSourcery 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