* [PATCH 0/4] Prevent more recursion into python based unwinders @ 2016-09-28 8:49 Kevin Buettner 2016-09-28 8:53 ` [PATCH 1/4] Distinguish sentinel frame from null frame Kevin Buettner ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Kevin Buettner @ 2016-09-28 8:49 UTC (permalink / raw) To: gdb-patches Pedro pushed a patch earlier this month which avoids recursion into a python based unwinder in some cases. The exact case that his patch fixed was when the unwinder attempted to call gdb.PendingFrame.read_register with either an actual or pseudo register. For reference, Pedro's patch is here: https://sourceware.org/ml/gdb-patches/2016-08/msg00239.html I recently posted a test case which exhibits problems with recursion in two other cases: 1) when gdb.parse_and_eval is called, and 2) when gdb.PendingFrame.read_register is called with a "user" register like "pc" on x86_64. My test case (extension) can be found here: https://sourceware.org/ml/gdb-patches/2016-09/msg00368.html This patch set fixes those two cases of recursion and adjusts one test, py-unwind-maint.exp, which was showing failures / regressions after my changes. I wish to thank Pedro for his collaboration on these patches. He suggested several of the key ideas that went into them. (That said, he hasn't seen them yet, so if I botched things, the fault is mine.) Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] Distinguish sentinel frame from null frame 2016-09-28 8:49 [PATCH 0/4] Prevent more recursion into python based unwinders Kevin Buettner @ 2016-09-28 8:53 ` Kevin Buettner 2016-10-12 13:07 ` Pedro Alves 2016-09-28 8:56 ` [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID Kevin Buettner ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Kevin Buettner @ 2016-09-28 8:53 UTC (permalink / raw) To: gdb-patches Distinguish sentinel frame from null frame. This patch replaces the `current_frame' static global in frame.c with `sentinel_frame'. It also makes the sentinel frame id unique and different from the null frame. By itself, there is not much point to this patch, but it makes the code cleaner for the VALUE_FRAME_ID changes in another patch. Since we now allow "navigation" to the sentinel frame, it removes the necessity of adding special cases to other parts of GDB. Note that a new function, get_next_frame_sentinel_okay, is introduced in this patch. It will be used by the VALUE_FRAME_ID changes that I've made. Thanks to Pedro Alves for this suggestion. gdb/ChangeLog: * frame.h (enum frame_id_stack_status): Add FID_STACK_SENTINEL. (struct frame_id): Increase number of bits required for storing stack status to 3 from 2. (sentinel_frame_id): New declaration. (get_next_frame_sentinel_okay): Declare. (frame_find_by_id_sentinel_okay): Declare. * frame.c (current_frame): Rename this static global to... (sentinel_frame): ...this static global, which has also been moved an earlier location in the file. (fprint_frame_id): Add case for sentinel frame id. (get_frame_id): Return early for sentinel frame. (sentinel_frame_id): Define. (frame_find_by_id): Add case for sentinel_frame_id. (create_sentinel_frame): Use sentinel_frame_id for this_id.value instead of null_frame_id. (get_current_frame): Add local declaration for `current_frame'. Remove local declaration for `sentinel_frame.' Add new_local, new_sentinel_p, and use it for knowing when to call get_frame_id(). (get_next_frame_sentinel_okay): New function. (reinit_frame_cache): Use `sentinel_frame' in place of `current_frame'. --- gdb/frame.c | 101 +++++++++++++++++++++++++++++++++++++++++------------------- gdb/frame.h | 12 +++++++- 2 files changed, 81 insertions(+), 32 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index d3f3056..3ca8ab2 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -43,6 +43,17 @@ #include "hashtab.h" #include "valprint.h" +/* The sentinel frame is the innermost frame. It generally does not + have any stack associated with it. Register values may be directly + determined by inspection with no unwinding necessary. + + The sentinel frame is constructed so that the next frame is a pointer + to the sentinel frame itself. + + The current frame can be found at sentinel_frame->prev. */ + +static struct frame_info *sentinel_frame; + static struct frame_info *get_prev_frame_raw (struct frame_info *this_frame); static const char *frame_stop_reason_symbol_string (enum unwind_stop_reason reason); @@ -65,7 +76,7 @@ enum cached_copy_status /* We keep a cache of stack frames, each of which is a "struct frame_info". The innermost one gets allocated (in - wait_for_inferior) each time the inferior stops; current_frame + wait_for_inferior) each time the inferior stops; sentinel_frame points to it. Additional frames get allocated (in get_prev_frame) as needed, and are chained through the next and prev fields. Any time that the frame cache becomes invalid (most notably when we @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id) fprintf_unfiltered (file, "!stack"); else if (id.stack_status == FID_STACK_UNAVAILABLE) fprintf_unfiltered (file, "stack=<unavailable>"); + else if (id.stack_status == FID_STACK_SENTINEL) + fprintf_unfiltered (file, "stack=<sentinel>"); else fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr)); fprintf_unfiltered (file, ","); @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi) if (fi == NULL) return null_frame_id; + if (fi == sentinel_frame) + return sentinel_frame_id; + if (!fi->this_id.p) { int stashed; @@ -562,6 +578,7 @@ frame_unwind_caller_id (struct frame_info *next_frame) } const struct frame_id null_frame_id = { 0 }; /* All zeros. */ +const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL, 0, 1, 0 }; const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_INVALID, 0, 1, 0 }; struct frame_id @@ -797,6 +814,10 @@ frame_find_by_id (struct frame_id id) if (!frame_id_p (id)) return NULL; + /* Check for the sentinel frame. */ + if (frame_id_eq (id, sentinel_frame_id)) + return sentinel_frame; + /* Try using the frame stash first. Finding it there removes the need to perform the search by looping over all frames, which can be very CPU-intensive if the number of frames is very high (the loop is O(n) @@ -1474,10 +1495,9 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache) /* Link this frame back to itself. The frame is self referential (the unwound PC is the same as the pc), so make it so. */ frame->next = frame; - /* Make the sentinel frame's ID valid, but invalid. That way all - comparisons with it should fail. */ + /* The sentinel frame has a special ID. */ frame->this_id.p = 1; - frame->this_id.value = null_frame_id; + frame->this_id.value = sentinel_frame_id; if (frame_debug) { fprintf_unfiltered (gdb_stdlog, "{ create_sentinel_frame (...) -> "); @@ -1487,10 +1507,6 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache) return frame; } -/* Info about the innermost stack frame (contents of FP register). */ - -static struct frame_info *current_frame; - /* Cache for frame addresses already read by gdb. Valid only while inferior is stopped. Control variables for the frame cache should be local to this module. */ @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame struct frame_info * get_current_frame (void) { + struct frame_info *current_frame; + int new_sentinel_p = 0; + /* First check, and report, the lack of registers. Having GDB report "No stack!" or "No memory" when the target doesn't even have registers is very confusing. Besides, "printcmd.exp" @@ -1526,30 +1545,37 @@ get_current_frame (void) if (get_traceframe_number () < 0) validate_registers_access (); - if (current_frame == NULL) + if (sentinel_frame == NULL) { - int stashed; - struct frame_info *sentinel_frame = + sentinel_frame = create_sentinel_frame (current_program_space, get_current_regcache ()); - - /* Set the current frame before computing the frame id, to avoid - recursion inside compute_frame_id, in case the frame's - unwinder decides to do a symbol lookup (which depends on the - selected frame's block). - - This call must always succeed. In particular, nothing inside - get_prev_frame_always_1 should try to unwind from the - sentinel frame, because that could fail/throw, and we always - want to leave with the current frame created and linked in -- - we should never end up with the sentinel frame as outermost - frame. */ - current_frame = get_prev_frame_always_1 (sentinel_frame); - gdb_assert (current_frame != NULL); - - /* No need to compute the frame id yet. That'll be done lazily - from inside get_frame_id instead. */ + new_sentinel_p = 1; } + /* Set the current frame before computing the frame id, to avoid + recursion inside compute_frame_id, in case the frame's + unwinder decides to do a symbol lookup (which depends on the + selected frame's block). + + This call must always succeed. In particular, nothing inside + get_prev_frame_always_1 should try to unwind from the + sentinel frame, because that could fail/throw, and we always + want to leave with the current frame created and linked in -- + we should never end up with the sentinel frame as outermost + frame. */ + current_frame = get_prev_frame_always_1 (sentinel_frame); + gdb_assert (current_frame != NULL); + + /* The call to get_frame_id, below, is not necessary when the stack is + well behaved. But when it's not well behaved, we want to ensure + that the frame id for the current frame is known (stashed) prior to + finding frame id values for any outer frames. + + The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error + in the "backtrace from stop_frame" test when we don't do this here. */ + if (new_sentinel_p) + get_frame_id (current_frame); + return current_frame; } @@ -1729,6 +1755,19 @@ get_next_frame (struct frame_info *this_frame) return NULL; } +/* Return the frame that THIS_FRAME calls. If THIS_FRAME is the innermost + frame, we return the sentinel frame. Thus, NULL will never be returned. */ + +struct frame_info * +get_next_frame_sentinel_okay (struct frame_info *this_frame) +{ + gdb_assert (this_frame != NULL); + + /* This always works, since the sentinel frame refers to itself via the + ``next'' field. */ + return this_frame->next; +} + /* Observer for the target_changed event. */ static void @@ -1745,7 +1784,7 @@ reinit_frame_cache (void) struct frame_info *fi; /* Tear down all frame caches. */ - for (fi = current_frame; fi != NULL; fi = fi->prev) + for (fi = sentinel_frame; fi != NULL; fi = fi->prev) { if (fi->prologue_cache && fi->unwind->dealloc_cache) fi->unwind->dealloc_cache (fi, fi->prologue_cache); @@ -1757,10 +1796,10 @@ reinit_frame_cache (void) obstack_free (&frame_cache_obstack, 0); obstack_init (&frame_cache_obstack); - if (current_frame != NULL) + if (sentinel_frame != NULL) annotate_frames_invalid (); - current_frame = NULL; /* Invalidate cache */ + sentinel_frame = NULL; /* Invalidate cache */ select_frame (NULL); frame_stash_invalidate (); if (frame_debug) diff --git a/gdb/frame.h b/gdb/frame.h index 5f21bb8..849a83d 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -89,6 +89,9 @@ enum frame_id_stack_status /* Stack address is valid, and is found in the stack_addr field. */ FID_STACK_VALID = 1, + /* Sentinel frame. Stack may or may not be valid. */ + FID_STACK_SENTINEL = 2, + /* Stack address is unavailable. I.e., there's a valid stack, but we don't know where it is (because memory or registers we'd compute it from were not collected). */ @@ -149,7 +152,7 @@ struct frame_id CORE_ADDR special_addr; /* Flags to indicate the above fields have valid contents. */ - ENUM_BITFIELD(frame_id_stack_status) stack_status : 2; + ENUM_BITFIELD(frame_id_stack_status) stack_status : 3; unsigned int code_addr_p : 1; unsigned int special_addr_p : 1; @@ -165,6 +168,9 @@ struct frame_id /* For convenience. All fields are zero. This means "there is no frame". */ extern const struct frame_id null_frame_id; +/* Sentinel frame. */ +extern const struct frame_id sentinel_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. */ @@ -309,6 +315,10 @@ extern void select_frame (struct frame_info *); extern struct frame_info *get_prev_frame (struct frame_info *); extern struct frame_info *get_next_frame (struct frame_info *); +/* Like get_next_frame(), but allows return of the sentinel frame. NULL + is never returned. */ +extern struct frame_info *get_next_frame_sentinel_okay (struct frame_info *); + /* Return a "struct frame_info" corresponding to the frame that called THIS_FRAME. Returns NULL if there is no such frame. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] Distinguish sentinel frame from null frame 2016-09-28 8:53 ` [PATCH 1/4] Distinguish sentinel frame from null frame Kevin Buettner @ 2016-10-12 13:07 ` Pedro Alves 2016-10-26 7:22 ` Kevin Buettner 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2016-10-12 13:07 UTC (permalink / raw) To: Kevin Buettner, gdb-patches Hi Kevin, Finally looking at this. On 09/28/2016 09:49 AM, Kevin Buettner wrote: > --- > gdb/frame.c | 101 +++++++++++++++++++++++++++++++++++++++++------------------- > gdb/frame.h | 12 +++++++- > 2 files changed, 81 insertions(+), 32 deletions(-) > > diff --git a/gdb/frame.c b/gdb/frame.c > index d3f3056..3ca8ab2 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -43,6 +43,17 @@ > #include "hashtab.h" > #include "valprint.h" > > +/* The sentinel frame is the innermost frame. It generally does not > + have any stack associated with it. Register values may be directly > + determined by inspection with no unwinding necessary. I think this comment is confusing. If you look at comments in sentinel-frame.h|c, you'll find it calling the sentinel frame the one past the inner-most instead of being _the_ innermost: [...] /* Implement the sentinel frame. The sentinel frame terminates the inner most end of the frame chain. If unwound, it returns the information need to construct an inner-most frame. */ [...] /* Pump prime the sentinel frame's cache. Since this needs the REGCACHE provide that here. */ /* The sentinel frame is used as a starting point for creating the previous (inner most) frame. That frame's THIS_ID method will be called to determine the inner most frame's ID. Not this one. */ [...] > It generally does not have any stack associated with it. Right, it should not ever be possible to ask the sentinel frame for its stack pointer, because we'd have to unwind that from frame level -2. :-) > Register values may be directly determined by inspection with > no unwinding necessary. This is true of the current frame. The reason we have a sentinel frame is that the unwind machinery always gets the THIS_FRAME's registers by unwinding them from the next frame. In order to keep that working without a special case for frame #0, we stuff the sentinel frame before (at frame level -1). That's why the only useful thing sentinel-frame.c does is implement prev_register. > + > + The sentinel frame is constructed so that the next frame is a pointer > + to the sentinel frame itself. > + > + The current frame can be found at sentinel_frame->prev. */ > + > +static struct frame_info *sentinel_frame; > @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id) > fprintf_unfiltered (file, "!stack"); > else if (id.stack_status == FID_STACK_UNAVAILABLE) > fprintf_unfiltered (file, "stack=<unavailable>"); > + else if (id.stack_status == FID_STACK_SENTINEL) > + fprintf_unfiltered (file, "stack=<sentinel>"); > else > fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr)); > fprintf_unfiltered (file, ","); > @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi) > if (fi == NULL) > return null_frame_id; > > + if (fi == sentinel_frame) > + return sentinel_frame_id; > + Why do you need this? When the sentinel frame is created, it's given a frame id already: /* The sentinel frame has a special ID. */ frame->this_id.p = 1; frame->this_id.value = sentinel_frame_id; > if (!fi->this_id.p) > { > int stashed; > -/* Info about the innermost stack frame (contents of FP register). */ > - > -static struct frame_info *current_frame; > - > /* Cache for frame addresses already read by gdb. Valid only while > inferior is stopped. Control variables for the frame cache should > be local to this module. */ > @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame > struct frame_info * > get_current_frame (void) > { > + struct frame_info *current_frame; > + int new_sentinel_p = 0; > + /* Set the current frame before computing the frame id, to avoid > + recursion inside compute_frame_id, in case the frame's > + unwinder decides to do a symbol lookup (which depends on the > + selected frame's block). > + > + This call must always succeed. In particular, nothing inside > + get_prev_frame_always_1 should try to unwind from the > + sentinel frame, because that could fail/throw, and we always > + want to leave with the current frame created and linked in -- > + we should never end up with the sentinel frame as outermost > + frame. */ > + current_frame = get_prev_frame_always_1 (sentinel_frame); > + gdb_assert (current_frame != NULL); > + > + /* The call to get_frame_id, below, is not necessary when the stack is > + well behaved. But when it's not well behaved, we want to ensure > + that the frame id for the current frame is known (stashed) prior to > + finding frame id values for any outer frames. > + > + The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error > + in the "backtrace from stop_frame" test when we don't do this here. */ > + if (new_sentinel_p) > + get_frame_id (current_frame); I don't understand this. I applied patch #1 locally with this bit removed, and I don't see said internal error. Oh, this only happens when the following patches are applied. IMO, it's better to move this hunk to the patch that actually creates the requirement for it, so that we have the whole logical change in one patch. Can you elaborate more on why the test causes the internal "backtrace from stop_frame" error without this here? > + > return current_frame; > } > > @@ -1729,6 +1755,19 @@ get_next_frame (struct frame_info *this_frame) > return NULL; > } > > +/* Return the frame that THIS_FRAME calls. If THIS_FRAME is the innermost > + frame, we return the sentinel frame. Thus, NULL will never be returned. */ This comment is a ambiguous too -- is innermost here talking about the sentinel frame or the real innermost frame (current frame) ? > + > +struct frame_info * > +get_next_frame_sentinel_okay (struct frame_info *this_frame) > +{ > + gdb_assert (this_frame != NULL); > + > + /* This always works, since the sentinel frame refers to itself via the > + ``next'' field. */ > + return this_frame->next; > +} Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] Distinguish sentinel frame from null frame 2016-10-12 13:07 ` Pedro Alves @ 2016-10-26 7:22 ` Kevin Buettner 2016-10-28 5:26 ` Kevin Buettner 0 siblings, 1 reply; 11+ messages in thread From: Kevin Buettner @ 2016-10-26 7:22 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves On Wed, 12 Oct 2016 14:07:13 +0100 Pedro Alves <palves@redhat.com> wrote: > > @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id) > > fprintf_unfiltered (file, "!stack"); > > else if (id.stack_status == FID_STACK_UNAVAILABLE) > > fprintf_unfiltered (file, "stack=<unavailable>"); > > + else if (id.stack_status == FID_STACK_SENTINEL) > > + fprintf_unfiltered (file, "stack=<sentinel>"); > > else > > fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr)); > > fprintf_unfiltered (file, ","); > > @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi) > > if (fi == NULL) > > return null_frame_id; > > > > + if (fi == sentinel_frame) > > + return sentinel_frame_id; > > + > > Why do you need this? When the sentinel frame is created, it's > given a frame id already: You're right - this isn't needed. I've removed it from a new version of the patch which I'm preparing. > > /* Cache for frame addresses already read by gdb. Valid only while > > inferior is stopped. Control variables for the frame cache should > > be local to this module. */ > > @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame > > struct frame_info * > > get_current_frame (void) > > { > > + struct frame_info *current_frame; > > + int new_sentinel_p = 0; > > + /* Set the current frame before computing the frame id, to avoid > > + recursion inside compute_frame_id, in case the frame's > > + unwinder decides to do a symbol lookup (which depends on the > > + selected frame's block). > > + > > + This call must always succeed. In particular, nothing inside > > + get_prev_frame_always_1 should try to unwind from the > > + sentinel frame, because that could fail/throw, and we always > > + want to leave with the current frame created and linked in -- > > + we should never end up with the sentinel frame as outermost > > + frame. */ > > + current_frame = get_prev_frame_always_1 (sentinel_frame); > > + gdb_assert (current_frame != NULL); > > + > > + /* The call to get_frame_id, below, is not necessary when the stack is > > + well behaved. But when it's not well behaved, we want to ensure > > + that the frame id for the current frame is known (stashed) prior to > > + finding frame id values for any outer frames. > > + > > + The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error > > + in the "backtrace from stop_frame" test when we don't do this here. */ > > + if (new_sentinel_p) > > + get_frame_id (current_frame); > > I don't understand this. I applied patch #1 locally with this bit > removed, and I don't see said internal error. > > Oh, this only happens when the following patches are applied. > > IMO, it's better to move this hunk to the patch that actually > creates the requirement for it, so that we have the whole > logical change in one patch. Can you elaborate more on > why the test causes the internal "backtrace from stop_frame" > error without this here? Here's the portion of the log file showing the failure/internal error: (gdb) break stop_frame Breakpoint 1 at 0x40059a: file dw2-dup-frame.c, line 22. (gdb) run Starting program: /mesquite2/sourceware-git/mesquite-native-python-unwind-2-split/bld/gdb/testsuite/outputs/gdb.dwarf2/dw2-dup-frame/dw2-dup-frame Breakpoint 1, stop_frame () at dw2-dup-frame.c:22 22 } (gdb) bt /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:544: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.dwarf2/dw2-dup-frame.exp: backtrace from stop_frame (GDB internal error) Here's a partial backtrace from the internal error, showing the frames which I think are relevant, plus several extra to provide context: #0 internal_error ( file=0x932b98 "/ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c", line=544, fmt=0x932b20 "%s: Assertion `%s' failed.") at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/common/errors.c:54 #1 0x000000000072207e in get_frame_id (fi=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:544 #2 0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:390 #3 0x00000000004ef5be in bootstrap_python_frame_filters (frame=0xe5a760, frame_low=0, frame_high=-1) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-framefilter.c:1453 #4 0x00000000004ef7a9 in gdbpy_apply_frame_filter ( extlang=0x8857e0 <extension_language_python>, frame=0xe5a760, flags=7, args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, frame_high=-1) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-framefilter.c:1548 #5 0x00000000005f2c5a in apply_ext_lang_frame_filter (frame=0xe5a760, flags=7, args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, frame_high=-1) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/extension.c:572 #6 0x00000000005ea896 in backtrace_command_1 (count_exp=0x0, show_locals=0, no_filters=0, from_tty=1) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/stack.c:1834 Examination of the code in frame_info_to_frame_object(), which is in python/py-frame.c, is key to understanding this problem: if (get_prev_frame (frame) == NULL && get_frame_unwind_stop_reason (frame) != UNWIND_NO_REASON && get_next_frame (frame) != NULL) { frame_obj->frame_id = get_frame_id (get_next_frame (frame)); frame_obj->frame_id_is_next = 1; } else { frame_obj->frame_id = get_frame_id (frame); frame_obj->frame_id_is_next = 0; } I will first note that the frame id for frame has not been computed yet. (This was verified by placing a breakpoint on compute_frame_id().) The call to get_prev_frame() causes the the frame id to (eventually) be computed for the previous frame. Here's a backtrace showing how we get there: #0 compute_frame_id (fi=0x10e2810) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:496 #1 0x0000000000724a67 in get_prev_frame_if_no_cycle (this_frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:1871 #2 0x0000000000725136 in get_prev_frame_always_1 (this_frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2045 #3 0x000000000072516b in get_prev_frame_always (this_frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2061 #4 0x000000000072570f in get_prev_frame (this_frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2303 #5 0x00000000004eb471 in frame_info_to_frame_object (frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:381 For this particular case, we end up in the else clause of the code above which calls get_frame_id (frame). It's at this point that the frame id for frame is computed. Again, here's a backtrace: #0 compute_frame_id (fi=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:496 #1 0x000000000072203d in get_frame_id (fi=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:539 #2 0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760) at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:390 The test in question, dw2-dup-frame.exp, deliberately creates a broken (cyclic) stack. So, in this instance, the frame id for the prev `frame' will be the same as that for `frame'. But that particular frame id ended up in the stash during the previous frame operation. When, just a few lines later, we compute the frame id for `frame', the id in question is already in the stash, thus triggering the assertion failure. An alternate "fix" (or perhaps band-aid) for this problem is as follows: diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c index 6bdac08..a1d305e 100644 --- a/gdb/python/py-frame.c +++ b/gdb/python/py-frame.c @@ -375,6 +375,10 @@ frame_info_to_frame_object (struct frame_info *frame) TRY { + /* Avoid case where the frame id for previous frame is computed + before that for the frame under consideration. */ + (void) get_frame_id (frame); + /* Try to get the previous frame, to determine if this is the last frame in a corrupt stack. If so, we need to store the frame_id of the next frame and not of this one (which is possibly invalid). */ I'm not especially fond of this solution though. If it's necessary to create frame ids in a certain sequence, this ordering should be imposed elsewhere. Also, I've caught just one case here. There may also be similar problems lurking which we haven't caught yet. I hope you that you (or someone else) can suggest a better solution... Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] Distinguish sentinel frame from null frame 2016-10-26 7:22 ` Kevin Buettner @ 2016-10-28 5:26 ` Kevin Buettner 0 siblings, 0 replies; 11+ messages in thread From: Kevin Buettner @ 2016-10-28 5:26 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Wed, 26 Oct 2016 00:21:53 -0700 Kevin Buettner <kevin@buettner.to> wrote: > > > /* Cache for frame addresses already read by gdb. Valid only while > > > inferior is stopped. Control variables for the frame cache should > > > be local to this module. */ > > > @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame > > > struct frame_info * > > > get_current_frame (void) > > > { > > > + struct frame_info *current_frame; > > > + int new_sentinel_p = 0; > > > + /* Set the current frame before computing the frame id, to avoid > > > + recursion inside compute_frame_id, in case the frame's > > > + unwinder decides to do a symbol lookup (which depends on the > > > + selected frame's block). > > > + > > > + This call must always succeed. In particular, nothing inside > > > + get_prev_frame_always_1 should try to unwind from the > > > + sentinel frame, because that could fail/throw, and we always > > > + want to leave with the current frame created and linked in -- > > > + we should never end up with the sentinel frame as outermost > > > + frame. */ > > > + current_frame = get_prev_frame_always_1 (sentinel_frame); > > > + gdb_assert (current_frame != NULL); > > > + > > > + /* The call to get_frame_id, below, is not necessary when the stack is > > > + well behaved. But when it's not well behaved, we want to ensure > > > + that the frame id for the current frame is known (stashed) prior to > > > + finding frame id values for any outer frames. > > > + > > > + The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error > > > + in the "backtrace from stop_frame" test when we don't do this here. */ > > > + if (new_sentinel_p) > > > + get_frame_id (current_frame); [...] > An alternate "fix" (or perhaps band-aid) for this problem is as > follows: > > diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c > index 6bdac08..a1d305e 100644 > --- a/gdb/python/py-frame.c > +++ b/gdb/python/py-frame.c > @@ -375,6 +375,10 @@ frame_info_to_frame_object (struct frame_info *frame) > TRY > { > > + /* Avoid case where the frame id for previous frame is computed > + before that for the frame under consideration. */ > + (void) get_frame_id (frame); > + > /* Try to get the previous frame, to determine if this is the last frame > in a corrupt stack. If so, we need to store the frame_id of the next > frame and not of this one (which is possibly invalid). */ > > I'm not especially fond of this solution though. If it's necessary to > create frame ids in a certain sequence, this ordering should be > imposed elsewhere. Also, I've caught just one case here. There may > also be similar problems lurking which we haven't caught yet. > > I hope you that you (or someone else) can suggest a better solution... FYI, I found a solution that I like better. I plan to submit this patch - or something like it - as part of a new patch series for preventing recursion in python based unwinders. So... no need to review it now, but (I hope that) there's also no need to spend time looking for the "better solution" requested in my earlier post. diff --git a/gdb/frame.c b/gdb/frame.c index bc42674..b19e782 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2217,6 +2217,18 @@ get_prev_frame (struct frame_info *this_frame) something should be calling get_selected_frame() or get_current_frame(). */ gdb_assert (this_frame != NULL); + + /* If this_frame is the current frame, then compute and stash + its frame id prior to fetching and computing the frame id of the + previous frame. Otherwise, the cycle detection code in + get_prev_frame_if_no_cycle() will not work correctly. When + get_frame_id() is called later on, an assertion error will + be triggered in the event of a cycle between the current + frame and its previous frame. */ + + if (this_frame->level == 0) + get_frame_id (this_frame); + frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc); /* tausq/2004-12-07: Dummy frames are skipped because it doesn't make much ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID 2016-09-28 8:49 [PATCH 0/4] Prevent more recursion into python based unwinders Kevin Buettner 2016-09-28 8:53 ` [PATCH 1/4] Distinguish sentinel frame from null frame Kevin Buettner @ 2016-09-28 8:56 ` Kevin Buettner 2016-10-12 13:08 ` Pedro Alves 2016-09-28 8:59 ` [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers Kevin Buettner 2016-09-28 12:41 ` [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import Kevin Buettner 3 siblings, 1 reply; 11+ messages in thread From: Kevin Buettner @ 2016-09-28 8:56 UTC (permalink / raw) To: gdb-patches Tweak meaning of VALUE_FRAME_ID. The VALUE_FRAME_ID macro provides access to a member in struct value that's used to hold the frame id that's used when determining a register's value or when assigning to a register. The underlying member has a long and obscure name. I won't refer to it here, but will simply refer to VALUE_FRAME_ID as if it's the struct value member instead of being a convenient macro. At the moment, without this patch in place, VALUE_FRAME_ID is set in value_of_register_lazy() and several other locations to hold the frame id of the frame passed to those functions. VALUE_FRAME_ID is used in the lval_register case of value_fetch_lazy(). To fetch the register's value, it calls get_frame_register_value() which, in turn, calls frame_unwind_register_value() with frame->next. A python based unwinder may wish to determine the value of a register or evaluate an expression containing a register. When it does this, value_fetch_lazy() will be called under some circumstances. It will attempt to determine the frame id associated with the frame passed to it. In so doing, it will end up back in the frame sniffer of the very same python unwinder that's attempting to learn the value of a register as part of the sniffing operation. This recursion is not desirable. As noted above, when value_fetch_lazy() wants to fetch a register's value, it does so (indirectly) by unwinding from frame->next. With this in mind, a solution suggests itself: Change VALUE_FRAME_ID to hold the frame id associated with the next frame. Then, when it comes time to obtain the value associated with the register, we can simply unwind from the frame corresponding to the frame id stored in VALUE_FRAME_ID. This neatly avoids the python unwinder recursion problem by changing when the "next" operation occurs. Instead of the "next" operation occuring when the register value is fetched, it occurs earlier on when assigning a frame id to VALUE_FRAME_ID. (Thanks to Pedro for this suggestion.) This patch implements this idea. It builds on the patch "Distinguish sentinel frame from null frame". Without that work in place, it's necessary to check for null_id at several places and then obtain the sentinel frame. In the course of developing this patch, I found that the lval_register case in value_assign() required some special care. It was passing the frame associated with VALUE_FRAME_ID (for the value being assigned) to put_frame_register_bytes(). But put_frame_register_bytes() calls put_frame_register(), which calls frame_register, which does frame_register_unwind() on frame->next. I briefly considered adjusting frame_register_unwind to work on the frame passed to it instead of frame->next, but this would have required more extensive changes throughout more of GDB. Instead, I opted for changing value_assign() so that frame->prev is passed to put_frame_register_bytes(). gdb/ChangeLog: * findvar.c (value_of_register_lazy): VALUE_FRAME_ID for register value now refers to the next frame. (default_value_from_register): Likewise. (value_from_register): Likewise. * frame_unwind.c (frame_unwind_got_optimized): Likewise. * sentinel-frame.c (sentinel_frame_prev_register): Likewise. * valops.c (value_assign): Obtain `prev' frame for passing to put_frame_register_bytes(). * value.c (value_fetch_lazy): Call frame_unwind_register_value() instead of get_frame_register_value(). --- gdb/findvar.c | 22 ++++++++++++++++++---- gdb/frame-unwind.c | 2 +- gdb/sentinel-frame.c | 2 +- gdb/valops.c | 11 +++++++++++ gdb/value.c | 2 +- 5 files changed, 32 insertions(+), 7 deletions(-) diff --git a/gdb/findvar.c b/gdb/findvar.c index 6e28a29..4d4ae49 100644 --- a/gdb/findvar.c +++ b/gdb/findvar.c @@ -283,17 +283,23 @@ value_of_register_lazy (struct frame_info *frame, int regnum) { struct gdbarch *gdbarch = get_frame_arch (frame); struct value *reg_val; + struct frame_info *next_frame; gdb_assert (regnum < (gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch))); - /* We should have a valid (i.e. non-sentinel) frame. */ - gdb_assert (frame_id_p (get_frame_id (frame))); + gdb_assert (frame != NULL); + + next_frame = get_next_frame_sentinel_okay (frame); + + /* We should have a valid next frame. */ + gdb_assert (frame_id_p (get_frame_id (next_frame))); reg_val = allocate_value_lazy (register_type (gdbarch, regnum)); VALUE_LVAL (reg_val) = lval_register; VALUE_REGNUM (reg_val) = regnum; - VALUE_FRAME_ID (reg_val) = get_frame_id (frame); + VALUE_FRAME_ID (reg_val) = get_frame_id (next_frame); + return reg_val; } @@ -815,8 +821,16 @@ default_value_from_register (struct gdbarch *gdbarch, struct type *type, { int len = TYPE_LENGTH (type); struct value *value = allocate_value (type); + struct frame_info *frame; VALUE_LVAL (value) = lval_register; + frame = frame_find_by_id (frame_id); + + if (frame == NULL) + frame_id = null_frame_id; + else + frame_id = get_frame_id (get_next_frame_sentinel_okay (frame)); + VALUE_FRAME_ID (value) = frame_id; VALUE_REGNUM (value) = regnum; @@ -902,7 +916,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame) including the location. */ v = allocate_value (type); VALUE_LVAL (v) = lval_register; - VALUE_FRAME_ID (v) = get_frame_id (frame); + VALUE_FRAME_ID (v) = get_frame_id (get_next_frame_sentinel_okay (frame)); VALUE_REGNUM (v) = regnum; ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1, value_contents_raw (v), &optim, diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c index 187e6c2..0dd98a2 100644 --- a/gdb/frame-unwind.c +++ b/gdb/frame-unwind.c @@ -210,7 +210,7 @@ frame_unwind_got_optimized (struct frame_info *frame, int regnum) mark_value_bytes_optimized_out (val, 0, TYPE_LENGTH (type)); VALUE_LVAL (val) = lval_register; VALUE_REGNUM (val) = regnum; - VALUE_FRAME_ID (val) = get_frame_id (frame); + VALUE_FRAME_ID (val) = get_frame_id (get_next_frame_sentinel_okay (frame)); return val; } diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c index 6cd1bc3..515650c 100644 --- a/gdb/sentinel-frame.c +++ b/gdb/sentinel-frame.c @@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame, struct value *value; value = regcache_cooked_read_value (cache->regcache, regnum); - VALUE_FRAME_ID (value) = get_frame_id (this_frame); + VALUE_FRAME_ID (value) = get_frame_id (get_next_frame_sentinel_okay (this_frame)); return value; } diff --git a/gdb/valops.c b/gdb/valops.c index 40392e8..c3305fa 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -1114,6 +1114,17 @@ value_assign (struct value *toval, struct value *fromval) /* Figure out which frame this is in currently. */ frame = frame_find_by_id (VALUE_FRAME_ID (toval)); + + /* value_of_register_lazy() stores what amounts to frame->next + in VALUE_FRAME_ID. But our eventual call to + put_frame_register_bytes() will do its own next. So, to + make things work out, we must pass it frame->prev instead + of just frame. Otherwise, it'll (essentially) be + frame->next->next. */ + + if (frame != NULL) + frame = get_prev_frame (frame); + value_reg = VALUE_REGNUM (toval); if (!frame) diff --git a/gdb/value.c b/gdb/value.c index b825aec..92afc45 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -4004,7 +4004,7 @@ value_fetch_lazy (struct value *val) gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame), regnum, type)); - new_val = get_frame_register_value (frame, regnum); + new_val = frame_unwind_register_value (frame, regnum); /* If we get another lazy lval_register value, it means the register is found by reading it from the next frame. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID 2016-09-28 8:56 ` [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID Kevin Buettner @ 2016-10-12 13:08 ` Pedro Alves 0 siblings, 0 replies; 11+ messages in thread From: Pedro Alves @ 2016-10-12 13:08 UTC (permalink / raw) To: Kevin Buettner, gdb-patches On 09/28/2016 09:52 AM, Kevin Buettner wrote: > Tweak meaning of VALUE_FRAME_ID. > > The VALUE_FRAME_ID macro provides access to a member in struct value > that's used to hold the frame id that's used when determining a > register's value or when assigning to a register. The underlying > member has a long and obscure name. I won't refer to it here, but > will simply refer to VALUE_FRAME_ID as if it's the struct value member > instead of being a convenient macro. > > At the moment, without this patch in place, VALUE_FRAME_ID is set in > value_of_register_lazy() and several other locations to hold the frame > id of the frame passed to those functions. > > VALUE_FRAME_ID is used in the lval_register case of > value_fetch_lazy(). To fetch the register's value, it calls > get_frame_register_value() which, in turn, calls > frame_unwind_register_value() with frame->next. > > A python based unwinder may wish to determine the value of a register > or evaluate an expression containing a register. When it does this, > value_fetch_lazy() will be called under some circumstances. It will > attempt to determine the frame id associated with the frame passed to > it. In so doing, it will end up back in the frame sniffer of the very > same python unwinder that's attempting to learn the value of a > register as part of the sniffing operation. This recursion is not > desirable. > > As noted above, when value_fetch_lazy() wants to fetch a register's > value, it does so (indirectly) by unwinding from frame->next. > > With this in mind, a solution suggests itself: Change VALUE_FRAME_ID > to hold the frame id associated with the next frame. Then, when it > comes time to obtain the value associated with the register, we can > simply unwind from the frame corresponding to the frame id stored in > VALUE_FRAME_ID. This neatly avoids the python unwinder recursion > problem by changing when the "next" operation occurs. Instead of the > "next" operation occuring when the register value is fetched, it > occurs earlier on when assigning a frame id to VALUE_FRAME_ID. > (Thanks to Pedro for this suggestion.) > At the very least, The VALUE_FRAME_ID macro's documentation and the value->frame_id's documentation must be updated to reflect the new meaning. Ideally, we'd rename these to VALUE_NEXT_FRAME_ID and value->frame_id. But that'd probably be best done as a separate follow up patch. > diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c > index 6cd1bc3..515650c 100644 > --- a/gdb/sentinel-frame.c > +++ b/gdb/sentinel-frame.c > @@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame, > struct value *value; > > value = regcache_cooked_read_value (cache->regcache, regnum); > - VALUE_FRAME_ID (value) = get_frame_id (this_frame); > + VALUE_FRAME_ID (value) = get_frame_id (get_next_frame_sentinel_okay (this_frame)); This looks a bit odd. I think it only works because getting the next of the sentinel frame returns back the sentinel frame again. How about we write directly: VALUE_FRAME_ID (value) = sentinel_frame_id; With that, maybe we could make it an internal error to call get_next_frame_sentinel_okay on the sentinel frame? Otherwise this look OK to me, but I'd like to understand better the need for get_frame_id call in the other patch. Maybe there's something else that we should be doing. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers 2016-09-28 8:49 [PATCH 0/4] Prevent more recursion into python based unwinders Kevin Buettner 2016-09-28 8:53 ` [PATCH 1/4] Distinguish sentinel frame from null frame Kevin Buettner 2016-09-28 8:56 ` [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID Kevin Buettner @ 2016-09-28 8:59 ` Kevin Buettner 2016-10-12 13:08 ` Pedro Alves 2016-09-28 12:41 ` [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import Kevin Buettner 3 siblings, 1 reply; 11+ messages in thread From: Kevin Buettner @ 2016-09-28 8:59 UTC (permalink / raw) To: gdb-patches Make gdb.PendingFrame.read_register handle "user" registers. The C function, pending_framepy_read_register(), which implements the python interface gdb.PendingFrame.read_register does not handle the so called "user" registers like "pc". An assertion error is triggered due to the user registers having numbers larger than or equal to gdbarch_num_regs(gdbarch). With the VALUE_FRAME_ID tweak in place, the call to get_frame_register_value() can simply be replaced by a call to value_of_register(), which handles both real registers as well as the user registers. gdb/ChangeLog: * python/py-unwind.c (pending_framepy_read_register): Use value_of_register() instead of get_frame_register_value(). --- gdb/python/py-unwind.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c index cc685ae..6740034 100644 --- a/gdb/python/py-unwind.c +++ b/gdb/python/py-unwind.c @@ -412,7 +412,12 @@ pending_framepy_read_register (PyObject *self, PyObject *args) TRY { - val = get_frame_register_value (pending_frame->frame_info, regnum); + /* Fetch the value associated with a register, whether it's + a real register or a so called "user" register, like "pc", + which maps to a real register. In the past, + get_frame_register_value() was used here, which did not + handle the user register case. */ + val = value_of_register (regnum, pending_frame->frame_info); if (val == NULL) PyErr_Format (PyExc_ValueError, "Cannot read register %d from frame.", ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers 2016-09-28 8:59 ` [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers Kevin Buettner @ 2016-10-12 13:08 ` Pedro Alves 0 siblings, 0 replies; 11+ messages in thread From: Pedro Alves @ 2016-10-12 13:08 UTC (permalink / raw) To: Kevin Buettner, gdb-patches On 09/28/2016 09:56 AM, Kevin Buettner wrote: > Make gdb.PendingFrame.read_register handle "user" registers. > > The C function, pending_framepy_read_register(), which implements > the python interface gdb.PendingFrame.read_register does not handle > the so called "user" registers like "pc". An assertion error is > triggered due to the user registers having numbers larger than or > equal to gdbarch_num_regs(gdbarch). > > With the VALUE_FRAME_ID tweak in place, the call to > get_frame_register_value() can simply be replaced by a call to > value_of_register(), which handles both real registers as well as the > user registers. > > gdb/ChangeLog: > > * python/py-unwind.c (pending_framepy_read_register): Use > value_of_register() instead of get_frame_register_value(). OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import 2016-09-28 8:49 [PATCH 0/4] Prevent more recursion into python based unwinders Kevin Buettner ` (2 preceding siblings ...) 2016-09-28 8:59 ` [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers Kevin Buettner @ 2016-09-28 12:41 ` Kevin Buettner 2016-10-12 13:10 ` Pedro Alves 3 siblings, 1 reply; 11+ messages in thread From: Kevin Buettner @ 2016-09-28 12:41 UTC (permalink / raw) To: gdb-patches py-unwind-maint.exp: Allow unwinders to be called during python import In his commit of "Fix PR19927: Avoid unwinder recursion if sniffer uses calls parse_and_eval", Pedro updated the py-unwind-maint.exp test to account for different behavior in GDB caused by his changes to frame.c. This patch changes py-unwind-maint.exp so that either behavior is acceptable. Regardless of which path is taken, the number of PASSes and the names of the tests are the same. gdb/testsuite/ChangeLog: * gdb.python/py-unwind-maint.exp: Adjust tests to allow for unwinders to be called as a side effect of "source" and "disable unwinder" commands. If these side effects don't occur, detect that behavior and run slightly different tests which are also considered to be correct behavior. --- gdb/testsuite/gdb.python/py-unwind-maint.exp | 59 ++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/gdb/testsuite/gdb.python/py-unwind-maint.exp b/gdb/testsuite/gdb.python/py-unwind-maint.exp index 1253057..91b1fdf 100644 --- a/gdb/testsuite/gdb.python/py-unwind-maint.exp +++ b/gdb/testsuite/gdb.python/py-unwind-maint.exp @@ -34,13 +34,49 @@ if ![runto_main ] then { return -1 } -gdb_test "source ${pyfile}" "Python script imported" \ - "import python scripts" -gdb_test_sequence "frame" "All unwinders enabled" { - "py_unwind_maint_ps_unwinder called" - "global_unwinder called" - "#0 main" +send_gdb "source ${pyfile}\n" +gdb_expect { + -re "Python script imported\r\n$gdb_prompt $" { + set unwinder_called_on_import false + } + -re ".*$gdb_prompt $" { + set unwinder_called_on_import true + } + timeout { + fail "Can't source python script" + return -1 + } +} + +clean_restart ${testfile} + +if ![runto_main ] then { + fail "Can't run to main" + return -1 +} + +if { $unwinder_called_on_import } { + gdb_test_sequence "source ${pyfile}" "import python scripts" { + "Python script imported" + "py_unwind_maint_ps_unwinder called" + "global_unwinder called" + } + # The unwinders were called above. We keep the name of the + # test the same so that it matches the case below and so that + # we have no greater or fewer passes regardless which path + # is taken. + gdb_test_sequence "frame" "All unwinders enabled" { + "#0 main" + } +} else { + gdb_test "source ${pyfile}" "Python script imported" \ + "import python scripts" + gdb_test_sequence "frame" "All unwinders enabled" { + "py_unwind_maint_ps_unwinder called" + "global_unwinder called" + "#0 main" + } } gdb_test_sequence "info unwinder" "Show all unwinders" { @@ -57,8 +93,15 @@ gdb_test_sequence "continue" "Unwinders called" { "global_unwinder called" } -gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" { - "1 unwinder disabled" +if { $unwinder_called_on_import } { + gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" { + "1 unwinder disabled" + "py_unwind_maint_ps_unwinder called" + } +} else { + gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" { + "1 unwinder disabled" + } } gdb_test_sequence "info unwinder" "Show with global unwinder disabled" { ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import 2016-09-28 12:41 ` [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import Kevin Buettner @ 2016-10-12 13:10 ` Pedro Alves 0 siblings, 0 replies; 11+ messages in thread From: Pedro Alves @ 2016-10-12 13:10 UTC (permalink / raw) To: Kevin Buettner, gdb-patches On 09/28/2016 09:59 AM, Kevin Buettner wrote: > py-unwind-maint.exp: Allow unwinders to be called during python import Looks like you have the subject here again. > > In his commit of "Fix PR19927: Avoid unwinder recursion if sniffer > uses calls parse_and_eval", Pedro updated the py-unwind-maint.exp > test to account for different behavior in GDB caused by his changes > to frame.c. > > This patch changes py-unwind-maint.exp so that either behavior is > acceptable. > > Regardless of which path is taken, the number of PASSes and the > names of the tests are the same. I'd like to understand better the need for this. BTW, should probably use gdb_test_multiple instead of send_gdb/gdb_expect. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-10-28 5:26 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-28 8:49 [PATCH 0/4] Prevent more recursion into python based unwinders Kevin Buettner 2016-09-28 8:53 ` [PATCH 1/4] Distinguish sentinel frame from null frame Kevin Buettner 2016-10-12 13:07 ` Pedro Alves 2016-10-26 7:22 ` Kevin Buettner 2016-10-28 5:26 ` Kevin Buettner 2016-09-28 8:56 ` [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID Kevin Buettner 2016-10-12 13:08 ` Pedro Alves 2016-09-28 8:59 ` [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers Kevin Buettner 2016-10-12 13:08 ` Pedro Alves 2016-09-28 12:41 ` [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import Kevin Buettner 2016-10-12 13:10 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).