This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Daniel Jacobowitz wrote: > Well, here I'm not sure about. I don't consider 'set backtrace limit' > to be just a user interface change; we use it as a safety net. OK, I see. > The only place your patch switched to using frame_find_by_id instead > of frame_id_inner was the dummy frame check, though. So maybe the > right thing to do is just to remove that stale dummy frame check, and > clear the dummy list when we get a new inferior? And stick to > get_prev_frame, at least for the moment. Hmmm, from the comments at the frame_id_inner check it seems like this is only used in exceptional cases; I'd agree to defer those until we get a new inferior. However, in actual fact, this is *only* place that ever deletes dummy frames, even in the absence of longjmp or other exceptional situations. That seems simply a bug to me, however. When in the normal course of execution a dummy frame is popped, it's associated dummy-frame.c data should really be cleaned up as well. The patch below implements this in addition to your suggestion. > Other than that, your logic looks entirely right to me. I like > the clever corruption check. Thanks for checking! New version re-tested on powerpc-linux, powerpc64-linux, and spu-elf. Bye, Ulrich ChangeLog: * dummy-frame.h (dummy_frame_pop): Add prototype. * dummy-frame.c: Include "observer.h". (dummy_frame_push): Do not check for stale frames. (dummy_frame_pop): New function. (cleanup_dummy_frames): New function. (_initialize_dummy_frame): Install it as inferior_created observer. * frame.h (struct frame_id): Update comments. (frame_id_inner): Remove prototype. * frame.c (frame_id_inner): Make static. Add comments. (frame_find_by_id): Update frame_id_inner safety net check to avoid false positives for targets using non-contiguous stack ranges. Use get_prev_frame_1 instead of get_prev_frame. (get_prev_frame_1): Update frame_id_inner safety net check. (frame_pop): Call dummy_frame_pop when popping a dummy frame. * stack.c (return_command): Directly pop the selected frame. * infrun.c (handle_inferior_event): Remove dead code. * i386-tdep.c (i386_push_dummy_call): Update comment. diff -urNp gdb-orig/gdb/dummy-frame.c gdb-head/gdb/dummy-frame.c --- gdb-orig/gdb/dummy-frame.c 2008-08-21 23:01:33.488803000 +0200 +++ gdb-head/gdb/dummy-frame.c 2008-08-21 23:41:19.968722918 +0200 @@ -30,6 +30,7 @@ #include "command.h" #include "gdbcmd.h" #include "gdb_string.h" +#include "observer.h" /* Dummy frame. This saves the processor state just prior to setting up the inferior function call. Older targets save the registers @@ -87,26 +88,8 @@ void dummy_frame_push (struct regcache *caller_regcache, const struct frame_id *dummy_id) { - struct gdbarch *gdbarch = get_regcache_arch (caller_regcache); struct dummy_frame *dummy_frame; - /* Check to see if there are stale dummy frames, perhaps left over - from when a longjump took us out of a function that was called by - the debugger. */ - dummy_frame = dummy_frame_stack; - while (dummy_frame) - /* FIXME: cagney/2004-08-02: Should just test IDs. */ - if (frame_id_inner (gdbarch, dummy_frame->id, (*dummy_id))) - /* Stale -- destroy! */ - { - dummy_frame_stack = dummy_frame->next; - regcache_xfree (dummy_frame->regcache); - xfree (dummy_frame); - dummy_frame = dummy_frame_stack; - } - else - dummy_frame = dummy_frame->next; - dummy_frame = XZALLOC (struct dummy_frame); dummy_frame->regcache = caller_regcache; dummy_frame->id = (*dummy_id); @@ -114,6 +97,47 @@ dummy_frame_push (struct regcache *calle dummy_frame_stack = dummy_frame; } +/* Pop the dummy frame with ID dummy_id from the dummy-frame stack. */ + +void +dummy_frame_pop (struct frame_id dummy_id) +{ + struct dummy_frame **dummy_ptr; + + for (dummy_ptr = &dummy_frame_stack; + (*dummy_ptr) != NULL; + dummy_ptr = &(*dummy_ptr)->next) + { + struct dummy_frame *dummy = *dummy_ptr; + if (frame_id_eq (dummy->id, dummy_id)) + { + *dummy_ptr = dummy->next; + regcache_xfree (dummy->regcache); + xfree (dummy); + break; + } + } +} + +/* There may be stale dummy frames, perhaps left over from when a longjump took us + out of a function that was called by the debugger. Clean them up at least once + whenever we start a new inferior. */ + +static void +cleanup_dummy_frames (struct target_ops *target, int from_tty) +{ + struct dummy_frame *dummy, *next; + + for (dummy = dummy_frame_stack; dummy; dummy = next) + { + next = dummy->next; + regcache_xfree (dummy->regcache); + xfree (dummy); + } + + dummy_frame_stack = NULL; +} + /* Return the dummy frame cache, it contains both the ID, and a pointer to the regcache. */ struct dummy_frame_cache @@ -258,4 +282,5 @@ _initialize_dummy_frame (void) _("Print the contents of the internal dummy-frame stack."), &maintenanceprintlist); + observer_attach_inferior_created (cleanup_dummy_frames); } diff -urNp gdb-orig/gdb/dummy-frame.h gdb-head/gdb/dummy-frame.h --- gdb-orig/gdb/dummy-frame.h 2008-08-21 23:01:33.492803000 +0200 +++ gdb-head/gdb/dummy-frame.h 2008-08-21 23:41:19.972722344 +0200 @@ -41,6 +41,8 @@ struct frame_id; extern void dummy_frame_push (struct regcache *regcache, const struct frame_id *dummy_id); +extern void dummy_frame_pop (struct frame_id dummy_id); + /* If the PC falls in a dummy frame, return a dummy frame unwinder. */ diff -urNp gdb-orig/gdb/frame.c gdb-head/gdb/frame.c --- gdb-orig/gdb/frame.c 2008-08-21 23:01:33.499802000 +0200 +++ gdb-head/gdb/frame.c 2008-08-21 23:41:19.980721197 +0200 @@ -368,7 +368,33 @@ frame_id_eq (struct frame_id l, struct f return eq; } -int +/* Safety net to check whether frame ID L should be inner to + frame ID R, according to their stack addresses. + + This method cannot be used to compare arbitrary frames, as the + ranges of valid stack addresses may be discontiguous (e.g. due + to sigaltstack). + + However, it can be used as safety net to discover invalid frame + IDs in certain circumstances. + + * If frame NEXT is the immediate inner frame to THIS, and NEXT + is a NORMAL frame, then the stack address of NEXT must be + inner-than-or-equal to the stack address of THIS. + + Therefore, if frame_id_inner (THIS, NEXT) holds, some unwind + error has occurred. + + * If frame NEXT is the immediate inner frame to THIS, and NEXT + is a NORMAL frame, and NEXT and THIS have different stack + addresses, no other frame in the frame chain may have a stack + address in between. + + Therefore, if frame_id_inner (TEST, THIS) holds, but + frame_id_inner (TEST, NEXT) does not hold, TEST cannot refer + to a valid frame in the frame chain. */ + +static int frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r) { int inner; @@ -395,28 +421,34 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_info * frame_find_by_id (struct frame_id id) { - struct frame_info *frame; + struct frame_info *frame, *prev_frame; /* ZERO denotes the null frame, let the caller decide what to do about it. Should it instead return get_current_frame()? */ if (!frame_id_p (id)) return NULL; - for (frame = get_current_frame (); - frame != NULL; - frame = get_prev_frame (frame)) + for (frame = get_current_frame (); ; frame = prev_frame) { struct frame_id this = get_frame_id (frame); if (frame_id_eq (id, this)) /* An exact match. */ return frame; - if (frame_id_inner (get_frame_arch (frame), id, this)) - /* Gone to far. */ + + prev_frame = get_prev_frame_1 (frame); + if (!prev_frame) + return NULL; + + /* As a safety net to avoid unnecessary backtracing while trying + to find an invalid ID, we check for a common situation where + we can detect from comparing stack addresses that no other + frame in the current frame chain can have this ID. See the + comment at frame_id_inner for details. */ + if (get_frame_type (frame) == NORMAL_FRAME + && !frame_id_inner (get_frame_arch (frame), id, this) + && frame_id_inner (get_frame_arch (prev_frame), id, + get_frame_id (prev_frame))) return NULL; - /* Either we're not yet gone far enough out along the frame - chain (inner(this,id)), or we're comparing frameless functions - (same .base, different .func, no test available). Struggle - on until we've definitly gone to far. */ } return NULL; } @@ -517,6 +549,11 @@ frame_pop (struct frame_info *this_frame scratch = frame_save_as_regcache (prev_frame); cleanups = make_cleanup_regcache_xfree (scratch); + /* If we are popping a dummy frame, clean up the associated + data as well. */ + if (get_frame_type (this_frame) == DUMMY_FRAME) + dummy_frame_pop (get_frame_id (this_frame)); + /* FIXME: cagney/2003-03-16: It should be possible to tell the target's register cache that it is about to be hit with a burst register transfer and that the sequence of register writes should @@ -1207,11 +1244,10 @@ get_prev_frame_1 (struct frame_info *thi /* Check that this frame's ID isn't inner to (younger, below, next) the next frame. This happens when a frame unwind goes backwards. - Exclude signal trampolines (due to sigaltstack the frame ID can - go backwards) and sentinel frames (the test is meaningless). */ - if (this_frame->next->level >= 0 - && this_frame->next->unwind->type != SIGTRAMP_FRAME - && frame_id_inner (get_frame_arch (this_frame), this_id, + This check is valid only if the next frame is NORMAL. See the + comment at frame_id_inner for details. */ + if (this_frame->next->unwind->type == NORMAL_FRAME + && frame_id_inner (get_frame_arch (this_frame->next), this_id, get_frame_id (this_frame->next))) { if (frame_debug) diff -urNp gdb-orig/gdb/frame.h gdb-head/gdb/frame.h --- gdb-orig/gdb/frame.h 2008-08-21 22:11:47.007062000 +0200 +++ gdb-head/gdb/frame.h 2008-08-21 23:41:20.023715033 +0200 @@ -111,7 +111,7 @@ struct frame_id frames that do not change the stack but are still distinct and have some form of distinct identifier (e.g. the ia64 which uses a 2nd stack for registers). This field is treated as unordered - i.e. will - not be used in frame ordering comparisons such as frame_id_inner(). + not be used in frame ordering comparisons. This field is valid only if special_addr_p is true. Otherwise, this frame is considered to have a wildcard special address, i.e. one that @@ -124,22 +124,7 @@ struct frame_id unsigned int special_addr_p : 1; }; -/* Methods for constructing and comparing Frame IDs. - - NOTE: Given stackless functions A and B, where A calls B (and hence - B is inner-to A). The relationships: !eq(A,B); !eq(B,A); - !inner(A,B); !inner(B,A); all hold. - - This is because, while B is inner-to A, B is not strictly inner-to A. - Being stackless, they have an identical .stack_addr value, and differ - only by their unordered .code_addr and/or .special_addr values. - - Because frame_id_inner is only used as a safety net (e.g., - detect a corrupt stack) the lack of strictness is not a problem. - Code needing to determine an exact relationship between two frames - must instead use frame_id_eq and frame_id_unwind. For instance, - in the above, to determine that A stepped-into B, the equation - "A.id != B.id && A.id == id_unwind (B)" can be used. */ +/* Methods for constructing and comparing Frame IDs. */ /* For convenience. All fields are zero. */ extern const struct frame_id null_frame_id; @@ -176,12 +161,6 @@ extern int frame_id_p (struct frame_id l either L or R have a zero .func, then the same frame base. */ extern int frame_id_eq (struct frame_id l, struct frame_id r); -/* Returns non-zero when L is strictly inner-than R (they have - different frame .bases). Neither L, nor R can be `null'. See note - above about frameless functions. */ -extern int frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, - struct frame_id r); - /* Write the internal representation of a frame ID on the specified stream. */ extern void fprint_frame_id (struct ui_file *file, struct frame_id id); diff -urNp gdb-orig/gdb/i386-tdep.c gdb-head/gdb/i386-tdep.c --- gdb-orig/gdb/i386-tdep.c 2008-08-21 21:43:48.854745000 +0200 +++ gdb-head/gdb/i386-tdep.c 2008-08-21 23:41:20.032713743 +0200 @@ -1712,8 +1712,8 @@ i386_push_dummy_call (struct gdbarch *gd (i386_frame_this_id, i386_sigtramp_frame_this_id, i386_dummy_id). It's there, since all frame unwinders for a given target have to agree (within a certain margin) on the - definition of the stack address of a frame. Otherwise - frame_id_inner() won't work correctly. Since DWARF2/GCC uses the + definition of the stack address of a frame. Otherwise frame id + comparison might not work correctly. Since DWARF2/GCC uses the stack address *before* the function call as a frame's CFA. On the i386, when %ebp is used as a frame pointer, the offset between the contents %ebp and the CFA as defined by GCC. */ diff -urNp gdb-orig/gdb/infrun.c gdb-head/gdb/infrun.c --- gdb-orig/gdb/infrun.c 2008-08-21 21:44:32.081066000 +0200 +++ gdb-head/gdb/infrun.c 2008-08-21 23:41:20.082706575 +0200 @@ -3314,33 +3314,6 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME ( tss->current_line = stop_pc_sal.line; tss->current_symtab = stop_pc_sal.symtab; - /* In the case where we just stepped out of a function into the - middle of a line of the caller, continue stepping, but - step_frame_id must be modified to current frame */ -#if 0 - /* NOTE: cagney/2003-10-16: I think this frame ID inner test is too - generous. It will trigger on things like a step into a frameless - stackless leaf function. I think the logic should instead look - at the unwound frame ID has that should give a more robust - indication of what happened. */ - if (step - ID == current - ID) - still stepping in same function; - else if (step - ID == unwind (current - ID)) - stepped into a function; - else - stepped out of a function; - /* Of course this assumes that the frame ID unwind code is robust - and we're willing to introduce frame unwind logic into this - function. Fortunately, those days are nearly upon us. */ -#endif - { - struct frame_info *frame = get_current_frame (); - struct frame_id current_frame = get_frame_id (frame); - if (!(frame_id_inner (get_frame_arch (frame), current_frame, - step_frame_id))) - step_frame_id = current_frame; - } - if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: keep going\n"); keep_going (ecs); diff -urNp gdb-orig/gdb/stack.c gdb-head/gdb/stack.c --- gdb-orig/gdb/stack.c 2008-08-21 21:44:33.061060000 +0200 +++ gdb-head/gdb/stack.c 2008-08-21 23:41:20.125700410 +0200 @@ -1844,29 +1844,8 @@ If you continue, the return value that y error (_("Not confirmed")); } - /* NOTE: cagney/2003-01-18: Is this silly? Rather than pop each - frame in turn, should this code just go straight to the relevant - frame and pop that? */ - - /* First discard all frames inner-to the selected frame (making the - selected frame current). */ - { - struct frame_id selected_id = get_frame_id (get_selected_frame (NULL)); - while (!frame_id_eq (selected_id, get_frame_id (get_current_frame ()))) - { - struct frame_info *frame = get_current_frame (); - if (frame_id_inner (get_frame_arch (frame), selected_id, - get_frame_id (frame))) - /* Caught in the safety net, oops! We've gone way past the - selected frame. */ - error (_("Problem while popping stack frames (corrupt stack?)")); - frame_pop (get_current_frame ()); - } - } - - /* Second discard the selected frame (which is now also the current - frame). */ - frame_pop (get_current_frame ()); + /* Discard the selected frame and all frames inner-to it. */ + frame_pop (get_selected_frame (NULL)); /* Store RETURN_VALUE in the just-returned register set. */ if (return_value != NULL) -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |