* [patch] Fix another stale frame_info *
@ 2012-02-23 8:38 Jan Kratochvil
2012-02-24 15:37 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kratochvil @ 2012-02-23 8:38 UTC (permalink / raw)
To: gdb-patches
Hi,
ISTM one cannot safely initialize FRAME_CALLER_PC unconditionally.
No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
I will check it in.
Thanks,
Jan
gdb/
2012-02-23 Jan Kratochvil <jan.kratochvil@redhat.com>
* breakpoint.c (until_break_command): New variables frame_caller_id,
frame_caller_pc and frame_caller_arch. Use them after
set_momentary_breakpoint.
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9853,12 +9853,19 @@ until_break_command (char *arg, int from_tty, int anywhere)
struct symtabs_and_lines sals;
struct symtab_and_line sal;
struct frame_info *frame = get_selected_frame (NULL);
+ struct frame_id frame_caller_id = frame_unwind_caller_id (frame);
+ /* Initialize it just to avoid a GCC false warning. */
+ CORE_ADDR frame_caller_pc = 0;
+ struct gdbarch *frame_caller_arch = frame_unwind_caller_arch (frame);
struct breakpoint *breakpoint;
struct breakpoint *breakpoint2 = NULL;
struct cleanup *old_chain;
int thread;
struct thread_info *tp;
+ if (frame_id_p (frame_caller_id))
+ frame_caller_pc = frame_unwind_caller_pc (frame);
+
clear_proceed_status ();
/* Set a breakpoint where the user wants it and at return from
@@ -9903,17 +9912,15 @@ until_break_command (char *arg, int from_tty, int anywhere)
/* Keep within the current frame, or in frames called by the current
one. */
- if (frame_id_p (frame_unwind_caller_id (frame)))
+ if (frame_id_p (frame_caller_id))
{
- sal = find_pc_line (frame_unwind_caller_pc (frame), 0);
- sal.pc = frame_unwind_caller_pc (frame);
- breakpoint2 = set_momentary_breakpoint (frame_unwind_caller_arch (frame),
- sal,
- frame_unwind_caller_id (frame),
- bp_until);
+ sal = find_pc_line (frame_caller_pc, 0);
+ sal.pc = frame_caller_pc;
+ breakpoint2 = set_momentary_breakpoint (frame_caller_arch, sal,
+ frame_caller_id, bp_until);
make_cleanup_delete_breakpoint (breakpoint2);
- set_longjmp_breakpoint (tp, frame_unwind_caller_id (frame));
+ set_longjmp_breakpoint (tp, frame_caller_id);
make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] Fix another stale frame_info * 2012-02-23 8:38 [patch] Fix another stale frame_info * Jan Kratochvil @ 2012-02-24 15:37 ` Pedro Alves 2012-02-24 15:41 ` Jan Kratochvil 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2012-02-24 15:37 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches On 02/23/2012 08:37 AM, Jan Kratochvil wrote: > + if (frame_id_p (frame_caller_id)) > + frame_caller_pc = frame_unwind_caller_pc (frame); > + Yeah. If we swap the order we do things, we can avoid needing this bit. WDYT? It seems slightly cleaner to me. Tested on x86_64 Fedora 16. 2012-02-24 Jan Kratochvil <jan.kratochvil@redhat.com> Pedro Alves <palves@redhat.com> * breakpoint.c (until_break_command): Install breakpoints after all frame manipulations. --- gdb/breakpoint.c | 41 ++++++++++++++++++++++++----------------- 1 files changed, 24 insertions(+), 17 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 3740157..fa09238 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9853,6 +9853,8 @@ until_break_command (char *arg, int from_tty, int anywhere) struct symtabs_and_lines sals; struct symtab_and_line sal; struct frame_info *frame = get_selected_frame (NULL); + struct gdbarch *gdbarch = get_frame_arch (frame); + struct frame_id stack_frame_id = get_stack_frame_id (frame); struct breakpoint *breakpoint; struct breakpoint *breakpoint2 = NULL; struct cleanup *old_chain; @@ -9883,32 +9885,25 @@ until_break_command (char *arg, int from_tty, int anywhere) resolve_sal_pc (&sal); - if (anywhere) - /* If the user told us to continue until a specified location, - we don't specify a frame at which we need to stop. */ - breakpoint = set_momentary_breakpoint (get_frame_arch (frame), sal, - null_frame_id, bp_until); - else - /* Otherwise, specify the selected frame, because we want to stop - only at the very same frame. */ - breakpoint = set_momentary_breakpoint (get_frame_arch (frame), sal, - get_stack_frame_id (frame), - bp_until); - - old_chain = make_cleanup_delete_breakpoint (breakpoint); - tp = inferior_thread (); thread = tp->num; + old_chain = make_cleanup (null_cleanup, NULL); + + /* Installing a breakpoint invalidates the frame chain (as it may + need to switch threads), so do any frame handling first. */ + /* Keep within the current frame, or in frames called by the current one. */ if (frame_id_p (frame_unwind_caller_id (frame))) { - sal = find_pc_line (frame_unwind_caller_pc (frame), 0); - sal.pc = frame_unwind_caller_pc (frame); + struct symtab_and_line sal2; + + sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0); + sal2.pc = frame_unwind_caller_pc (frame); breakpoint2 = set_momentary_breakpoint (frame_unwind_caller_arch (frame), - sal, + sal2, frame_unwind_caller_id (frame), bp_until); make_cleanup_delete_breakpoint (breakpoint2); @@ -9917,6 +9912,18 @@ until_break_command (char *arg, int from_tty, int anywhere) make_cleanup (delete_longjmp_breakpoint_cleanup, &thread); } + if (anywhere) + /* If the user told us to continue until a specified location, + we don't specify a frame at which we need to stop. */ + breakpoint = set_momentary_breakpoint (gdbarch, sal, + null_frame_id, bp_until); + else + /* Otherwise, specify the selected frame, because we want to stop + only at the very same frame. */ + breakpoint = set_momentary_breakpoint (gdbarch, sal, + stack_frame_id, bp_until); + make_cleanup_delete_breakpoint (breakpoint); + proceed (-1, TARGET_SIGNAL_DEFAULT, 0); /* If we are running asynchronously, and proceed call above has ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix another stale frame_info * 2012-02-24 15:37 ` Pedro Alves @ 2012-02-24 15:41 ` Jan Kratochvil 2012-02-24 15:54 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Jan Kratochvil @ 2012-02-24 15:41 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Fri, 24 Feb 2012 16:28:04 +0100, Pedro Alves wrote: > Yeah. If we swap the order we do things, we can avoid needing this > bit. WDYT? It seems slightly cleaner to me. Tested on x86_64 Fedora 16. In general yes just this patch is not there yet. > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c [...] > if (frame_id_p (frame_unwind_caller_id (frame))) > { > - sal = find_pc_line (frame_unwind_caller_pc (frame), 0); > - sal.pc = frame_unwind_caller_pc (frame); > + struct symtab_and_line sal2; > + > + sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0); > + sal2.pc = frame_unwind_caller_pc (frame); > breakpoint2 = set_momentary_breakpoint (frame_unwind_caller_arch (frame), > - sal, > + sal2, > frame_unwind_caller_id (frame), > bp_until); > make_cleanup_delete_breakpoint (breakpoint2); Here is still in the code: set_longjmp_breakpoint (tp, frame_unwind_caller_id (frame)); Thanks, Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix another stale frame_info * 2012-02-24 15:41 ` Jan Kratochvil @ 2012-02-24 15:54 ` Pedro Alves 2012-02-24 16:05 ` Jan Kratochvil 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2012-02-24 15:54 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches On 02/24/2012 03:37 PM, Jan Kratochvil wrote: > On Fri, 24 Feb 2012 16:28:04 +0100, Pedro Alves wrote: >> Yeah. If we swap the order we do things, we can avoid needing this >> bit. WDYT? It seems slightly cleaner to me. Tested on x86_64 Fedora 16. > > In general yes just this patch is not there yet. > > >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c > [...] >> if (frame_id_p (frame_unwind_caller_id (frame))) >> { >> - sal = find_pc_line (frame_unwind_caller_pc (frame), 0); >> - sal.pc = frame_unwind_caller_pc (frame); >> + struct symtab_and_line sal2; >> + >> + sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0); >> + sal2.pc = frame_unwind_caller_pc (frame); >> breakpoint2 = set_momentary_breakpoint (frame_unwind_caller_arch (frame), >> - sal, >> + sal2, >> frame_unwind_caller_id (frame), >> bp_until); >> make_cleanup_delete_breakpoint (breakpoint2); > > Here is still in the code: > set_longjmp_breakpoint (tp, frame_unwind_caller_id (frame)); Whoops. Re-tested on x86_64 Fedora 16. 2012-02-24 Jan Kratochvil <jan.kratochvil@redhat.com> Pedro Alves <palves@redhat.com> * breakpoint.c (until_break_command): Install breakpoints after all frame manipulations. --- gdb/breakpoint.c | 48 ++++++++++++++++++++++++++++-------------------- 1 files changed, 28 insertions(+), 20 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 3740157..1d5f069 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9853,6 +9853,9 @@ until_break_command (char *arg, int from_tty, int anywhere) struct symtabs_and_lines sals; struct symtab_and_line sal; struct frame_info *frame = get_selected_frame (NULL); + struct gdbarch *gdbarch = get_frame_arch (frame); + struct frame_id stack_frame_id = get_stack_frame_id (frame); + struct frame_id caller_frame_id = frame_unwind_caller_id (frame); struct breakpoint *breakpoint; struct breakpoint *breakpoint2 = NULL; struct cleanup *old_chain; @@ -9883,40 +9886,45 @@ until_break_command (char *arg, int from_tty, int anywhere) resolve_sal_pc (&sal); - if (anywhere) - /* If the user told us to continue until a specified location, - we don't specify a frame at which we need to stop. */ - breakpoint = set_momentary_breakpoint (get_frame_arch (frame), sal, - null_frame_id, bp_until); - else - /* Otherwise, specify the selected frame, because we want to stop - only at the very same frame. */ - breakpoint = set_momentary_breakpoint (get_frame_arch (frame), sal, - get_stack_frame_id (frame), - bp_until); - - old_chain = make_cleanup_delete_breakpoint (breakpoint); - tp = inferior_thread (); thread = tp->num; + old_chain = make_cleanup (null_cleanup, NULL); + + /* Installing a breakpoint invalidates the frame chain (as it may + need to switch threads), so do any frame handling first. */ + /* Keep within the current frame, or in frames called by the current one. */ - if (frame_id_p (frame_unwind_caller_id (frame))) + if (frame_id_p (caller_frame_id)) { - sal = find_pc_line (frame_unwind_caller_pc (frame), 0); - sal.pc = frame_unwind_caller_pc (frame); + struct symtab_and_line sal2; + + sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0); + sal2.pc = frame_unwind_caller_pc (frame); breakpoint2 = set_momentary_breakpoint (frame_unwind_caller_arch (frame), - sal, - frame_unwind_caller_id (frame), + sal2, + caller_frame_id, bp_until); make_cleanup_delete_breakpoint (breakpoint2); - set_longjmp_breakpoint (tp, frame_unwind_caller_id (frame)); + set_longjmp_breakpoint (tp, caller_frame_id); make_cleanup (delete_longjmp_breakpoint_cleanup, &thread); } + if (anywhere) + /* If the user told us to continue until a specified location, + we don't specify a frame at which we need to stop. */ + breakpoint = set_momentary_breakpoint (gdbarch, sal, + null_frame_id, bp_until); + else + /* Otherwise, specify the selected frame, because we want to stop + only at the very same frame. */ + breakpoint = set_momentary_breakpoint (gdbarch, sal, + stack_frame_id, bp_until); + make_cleanup_delete_breakpoint (breakpoint); + proceed (-1, TARGET_SIGNAL_DEFAULT, 0); /* If we are running asynchronously, and proceed call above has ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix another stale frame_info * 2012-02-24 15:54 ` Pedro Alves @ 2012-02-24 16:05 ` Jan Kratochvil 2012-02-24 16:32 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Jan Kratochvil @ 2012-02-24 16:05 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Fri, 24 Feb 2012 16:52:01 +0100, Pedro Alves wrote: > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -9853,6 +9853,9 @@ until_break_command (char *arg, int from_tty, int anywhere) > struct symtabs_and_lines sals; > struct symtab_and_line sal; > struct frame_info *frame = get_selected_frame (NULL); > + struct gdbarch *gdbarch = get_frame_arch (frame); > + struct frame_id stack_frame_id = get_stack_frame_id (frame); > + struct frame_id caller_frame_id = frame_unwind_caller_id (frame); OK with it. 'gdbarch' should be called 'frame_gdbarch' as this function deals both with FRAME and previous-of-FRAME (caller_frame_id). Thanks, Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Fix another stale frame_info * 2012-02-24 16:05 ` Jan Kratochvil @ 2012-02-24 16:32 ` Pedro Alves 0 siblings, 0 replies; 6+ messages in thread From: Pedro Alves @ 2012-02-24 16:32 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches On 02/24/2012 04:00 PM, Jan Kratochvil wrote: > OK with it. > > 'gdbarch' should be called 'frame_gdbarch' as this function deals both with > FRAME and previous-of-FRAME (caller_frame_id). Agreed. Applied with that rename. Thanks. 2012-02-24 Jan Kratochvil <jan.kratochvil@redhat.com> Pedro Alves <palves@redhat.com> * breakpoint.c (until_break_command): Install breakpoints after all frame manipulations. --- gdb/breakpoint.c | 48 ++++++++++++++++++++++++++++-------------------- 1 files changed, 28 insertions(+), 20 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 3740157..b5c30ee 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9853,6 +9853,9 @@ until_break_command (char *arg, int from_tty, int anywhere) struct symtabs_and_lines sals; struct symtab_and_line sal; struct frame_info *frame = get_selected_frame (NULL); + struct gdbarch *frame_gdbarch = get_frame_arch (frame); + struct frame_id stack_frame_id = get_stack_frame_id (frame); + struct frame_id caller_frame_id = frame_unwind_caller_id (frame); struct breakpoint *breakpoint; struct breakpoint *breakpoint2 = NULL; struct cleanup *old_chain; @@ -9883,40 +9886,45 @@ until_break_command (char *arg, int from_tty, int anywhere) resolve_sal_pc (&sal); - if (anywhere) - /* If the user told us to continue until a specified location, - we don't specify a frame at which we need to stop. */ - breakpoint = set_momentary_breakpoint (get_frame_arch (frame), sal, - null_frame_id, bp_until); - else - /* Otherwise, specify the selected frame, because we want to stop - only at the very same frame. */ - breakpoint = set_momentary_breakpoint (get_frame_arch (frame), sal, - get_stack_frame_id (frame), - bp_until); - - old_chain = make_cleanup_delete_breakpoint (breakpoint); - tp = inferior_thread (); thread = tp->num; + old_chain = make_cleanup (null_cleanup, NULL); + + /* Installing a breakpoint invalidates the frame chain (as it may + need to switch threads), so do any frame handling first. */ + /* Keep within the current frame, or in frames called by the current one. */ - if (frame_id_p (frame_unwind_caller_id (frame))) + if (frame_id_p (caller_frame_id)) { - sal = find_pc_line (frame_unwind_caller_pc (frame), 0); - sal.pc = frame_unwind_caller_pc (frame); + struct symtab_and_line sal2; + + sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0); + sal2.pc = frame_unwind_caller_pc (frame); breakpoint2 = set_momentary_breakpoint (frame_unwind_caller_arch (frame), - sal, - frame_unwind_caller_id (frame), + sal2, + caller_frame_id, bp_until); make_cleanup_delete_breakpoint (breakpoint2); - set_longjmp_breakpoint (tp, frame_unwind_caller_id (frame)); + set_longjmp_breakpoint (tp, caller_frame_id); make_cleanup (delete_longjmp_breakpoint_cleanup, &thread); } + if (anywhere) + /* If the user told us to continue until a specified location, + we don't specify a frame at which we need to stop. */ + breakpoint = set_momentary_breakpoint (frame_gdbarch, sal, + null_frame_id, bp_until); + else + /* Otherwise, specify the selected frame, because we want to stop + only at the very same frame. */ + breakpoint = set_momentary_breakpoint (frame_gdbarch, sal, + stack_frame_id, bp_until); + make_cleanup_delete_breakpoint (breakpoint); + proceed (-1, TARGET_SIGNAL_DEFAULT, 0); /* If we are running asynchronously, and proceed call above has ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-02-24 16:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-02-23 8:38 [patch] Fix another stale frame_info * Jan Kratochvil 2012-02-24 15:37 ` Pedro Alves 2012-02-24 15:41 ` Jan Kratochvil 2012-02-24 15:54 ` Pedro Alves 2012-02-24 16:05 ` Jan Kratochvil 2012-02-24 16:32 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox