From a45ae3ed061717e5a1538b1ac402cad93f81cb55 Mon Sep 17 00:00:00 2001 From: Ulrich Weigand Date: Tue, 26 Aug 2008 17:40:25 +0000 Subject: [PATCH] * 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. (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. --- gdb/ChangeLog | 21 +++++++++++++++ gdb/dummy-frame.c | 61 +++++++++++++++++++++++++++++------------- gdb/dummy-frame.h | 2 ++ gdb/frame.c | 68 ++++++++++++++++++++++++++++++++++++----------- gdb/frame.h | 25 ++--------------- gdb/i386-tdep.c | 4 +-- gdb/infrun.c | 27 ------------------- gdb/stack.c | 25 ++--------------- 8 files changed, 124 insertions(+), 109 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 0477f24f67c..2270a1c6edf 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,24 @@ +2008-08-26 Ulrich Weigand + + * 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. + (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. + 2008-08-26 Ulrich Weigand * breakpoint.c (remove_breakpoint): Do not fail if unable to remove diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c index 4c044ef3df4..a27de2e28f7 100644 --- a/gdb/dummy-frame.c +++ b/gdb/dummy-frame.c @@ -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 *caller_regcache, 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 --git a/gdb/dummy-frame.h b/gdb/dummy-frame.h index b299b6dd9c8..f7e05e6a1f4 100644 --- a/gdb/dummy-frame.h +++ b/gdb/dummy-frame.h @@ -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 --git a/gdb/frame.c b/gdb/frame.c index c4f85fe9c8a..55ded716825 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -368,7 +368,33 @@ frame_id_eq (struct frame_id l, struct frame_id r) 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_id l, struct frame_id r) 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 (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 *this_frame) /* 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 --git a/gdb/frame.h b/gdb/frame.h index 1441e122282..231aa32e355 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -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 --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 048454aae39..3e215fce294 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -1712,8 +1712,8 @@ i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function, (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 --git a/gdb/infrun.c b/gdb/infrun.c index d6c49ab1391..2d31219901d 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3323,33 +3323,6 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (!gdbarch_get_longjmp_target)\n"); 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 --git a/gdb/stack.c b/gdb/stack.c index d9c4f0a239a..61c799da526 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1844,29 +1844,8 @@ If you continue, the return value that you specified will be ignored.\n"; 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) -- 2.30.2