[downstream patch FYI] workaround stale frame_info * (PR 13866)

[downstream patch FYI] workaround stale frame_info * (PR 13866)

[Jan Kratochvil]

Hi,

I did not look at which commit caused this regression but apparently it was
introduced at least with multi-inferiors.

I understand this fix is not right fix of the crash; but in most GDB cases one
does not use multi-inferior so why to regress single-inferior by it.
Some more simple solutions still fix the single-inferior mode but they
regressed the multi-inferior mode
	gdb.threads/no-unwaited-for-left.exp
	gdb.multi/base.exp
so I had to put there that sorting magic.

With proper C++ sanity check of stale live frame_info references the testcase
would be simple without the "frame_garbage_collection" reproducer below.
It is also reproducible just with valgrind but regularly running the whole
testsuite under valgrind I did not find feasible.

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.


Thanks,
Jan


gdb/
2012-04-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Workaround PR backtrace/13866.
	* progspace.c (switch_to_program_space_and_thread): Try not to call
	switch_to_thread.

--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -481,17 +481,28 @@ save_current_space_and_thread (void)
 void
 switch_to_program_space_and_thread (struct program_space *pspace)
 {
-  struct inferior *inf;
+  struct inferior *inf = current_inferior ();
 
-  inf = find_inferior_for_program_space (pspace);
+  if (inf->pspace != pspace)
+    inf = find_inferior_for_program_space (pspace);
   if (inf != NULL)
     {
-      struct thread_info *tp;
+      struct thread_info *tp, *current_tp = NULL;
+
+      if (ptid_get_pid (inferior_ptid) == inf->pid)
+	current_tp = find_thread_ptid (inferior_ptid);
 
       tp = any_live_thread_of_process (inf->pid);
       if (tp != NULL)
 	{
-	  switch_to_thread (tp->ptid);
+	  /* Prefer primarily thread not THREAD_EXITED and secondarily thread
+	     not EXECUTING.  */
+	  if (current_tp == NULL
+	      || (tp->state != THREAD_EXITED
+		  && current_tp->state == THREAD_EXITED)
+	      || (!tp->executing && current_tp->executing))
+	    switch_to_thread (tp->ptid);
+
 	  /* Switching thread switches pspace implicitly.  We're
 	     done.  */
 	  return;


Reproducer with:
./gdb -nx ~/t/thread -ex 'b 24' -ex r -ex 'until 25'
Breakpoint 1, main () at /home/jkratoch/t/thread.c:24
24	  v++;
Segmentation fault (core dumped)

#include <pthread.h>
#include <assert.h>
#include <unistd.h>

static int v;

static void *start (void *arg)
{
  v++;
  v++;
  v++;
  v++;
  sleep (100);
  return arg;
}

int main (void)
{
  pthread_t thread1;
  int i;

  i = pthread_create (&thread1, NULL, start, NULL);
  assert (i == 0);
  v++;
  v++;
  v++;
  v++;
  i = pthread_join (thread1, NULL);
  assert (i == 0);

  return 0;
}
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1522,12 +1522,30 @@ frame_observer_target_changed (struct target_ops *target)
   reinit_frame_cache ();
 }
 
+typedef struct obstack obstack_s;
+DEF_VEC_O (obstack_s);
+static VEC (obstack_s) *frame_poison_vec;
+
+void frame_garbage_collection (void);
+void
+frame_garbage_collection (void)
+{
+  struct obstack *obstack_p;
+  int ix;
+
+  for (ix = 0; VEC_iterate (obstack_s, frame_poison_vec, ix, obstack_p); ix++)
+    obstack_free (obstack_p, 0);
+
+  VEC_free (obstack_s, frame_poison_vec);
+  frame_poison_vec = NULL;
+}
+
 /* Flush the entire frame cache.  */
 
 void
 reinit_frame_cache (void)
 {
-  struct frame_info *fi;
+  struct frame_info *fi, *fi_prev;
 
   /* Tear down all frame caches.  */
   for (fi = current_frame; fi != NULL; fi = fi->prev)
@@ -1538,8 +1556,14 @@ reinit_frame_cache (void)
 	fi->base->unwind->dealloc_cache (fi, fi->base_cache);
     }
 
+  for (fi = current_frame; fi != NULL; fi = fi_prev)
+    {
+      fi_prev = fi->prev;
+      memset (fi, 0, sizeof (*fi));
+    }
+  VEC_safe_push (obstack_s, frame_poison_vec, &frame_cache_obstack);
+
   /* Since we can't really be sure what the first object allocated was.  */
-  obstack_free (&frame_cache_obstack, 0);
   obstack_init (&frame_cache_obstack);
 
   if (current_frame != NULL)
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -359,6 +359,11 @@ prepare_execute_command (void)
   if (non_stop)
     target_dcache_invalidate ();
 
+  {
+    extern void frame_garbage_collection (void);
+    frame_garbage_collection ();
+  }
+
   return cleanup;
 }
 

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> With proper C++ sanity check of stale live frame_info references
Jan> the testcase would be simple without the "frame_garbage_collection"
Jan> reproducer below.  It is also reproducible just with valgrind but
Jan> regularly running the whole testsuite under valgrind I did not find
Jan> feasible.

I was thinking about this problem recently.

One idea would be to only let unwinders access struct frame_info, and
have all other code use struct frame_id, perhaps with some simple cache
so that repeated calls for a given frame_id will not cause too much
work.

This would make it much harder to cache information when not allowed.

On the minus side, there are 1300 uses of struct frame_info to audit.

Tom

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 05/18/2012 06:44 PM, Tom Tromey wrote:

>>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> With proper C++ sanity check of stale live frame_info references
> Jan> the testcase would be simple without the "frame_garbage_collection"
> Jan> reproducer below.  It is also reproducible just with valgrind but
> Jan> regularly running the whole testsuite under valgrind I did not find
> Jan> feasible.
> 
> I was thinking about this problem recently.
> 
> One idea would be to only let unwinders access struct frame_info, and
> have all other code use struct frame_id, perhaps with some simple cache
> so that repeated calls for a given frame_id will not cause too much
> work.
> 
> This would make it much harder to cache information when not allowed.
> 
> On the minus side, there are 1300 uses of struct frame_info to audit.


We invalidate the frame chain when something, anything, changes
memory or registers in the inferior, or we do something else that might
change our view of the frame stack (such as loading symbols).  But, setting a
breakpoint doesn't really change the inferior's memory from most of GDB code's
perspective, including the frame machinery, because reading back the memory we just
written to always gives back the contents as if the breakpoint wasn't planted in
the first place (due to shadowing).  So it's garanteed (minus bugs), that
the rebuilt cache will be a clone of the original cache.

We also only have one global cache, not a cache per thread (which I don't think
would be a good idea, considering inferiors with many many threads), as and
such we flush the cache whenever we change threads, even if for some internal
operation, like installing a breakpoint.  But, if we build a cache for thread A,
flip to thread B for a second, and then flip back to thread A, while having done
nothing that might have changed our view of the frame chain, the frame chain
built for thread A the second time should be the exact same as what
was build before flipping to thread B.

It seems to me that combining the both points, we should be able to get back
the old behavior, where calling a breakpoint insertion function wouldn't
invalidate the frame chain.

-- 
Pedro Alves

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Jan Kratochvil]

On Fri, 18 May 2012 20:00:39 +0200, Pedro Alves wrote:
> It seems to me that combining the both points, we should be able to get back
> the old behavior, where calling a breakpoint insertion function wouldn't
> invalidate the frame chain.

This is not well debuggable compared to the Tom's compile-time verification
proposal.  There may be workforce to mechanically replace frame_info for
frame_id as long as it gets reviewed.


Regards,
Jan

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> It seems to me that combining the both points, we should be able
Pedro> to get back the old behavior, where calling a breakpoint
Pedro> insertion function wouldn't invalidate the frame chain.

That would definitely be an improvement.

The thinking behind my approach was that we would replace crashing bugs
involving a stale cache with performance bugs (and maybe not even
those).  I think this would be friendlier for users.

Tom

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 04/04/2012 08:14 PM, Jan Kratochvil wrote:

> Hi,
> 
> I did not look at which commit caused this regression but apparently it was
> introduced at least with multi-inferiors.
> 
> I understand this fix is not right fix of the crash; but in most GDB cases one
> does not use multi-inferior so why to regress single-inferior by it.
> Some more simple solutions still fix the single-inferior mode but they
> regressed the multi-inferior mode
> 	gdb.threads/no-unwaited-for-left.exp
> 	gdb.multi/base.exp
> so I had to put there that sorting magic.
> 
> With proper C++ sanity check of stale live frame_info references the testcase
> would be simple without the "frame_garbage_collection" reproducer below.
> It is also reproducible just with valgrind but regularly running the whole
> testsuite under valgrind I did not find feasible.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
> 


I've used this frame validation patch, based on yours, which sprinkles
validate_frame calls around frame.c to catch uses of already dead frame_info
objects.  It makes the "until" bug 100% reproducible (with an internal_error) without
valgrind.  I've then ran the whole testsuite with this on, and that didn't catch
any other problem.  You mention in PR13866 many situations with stale
frame_info; did you have some other way to catch those, or was that just a hunch?

---

 gdb/frame.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/top.c   |    5 ++++
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index e012f2d..285b60d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -129,6 +129,14 @@ struct frame_info
   enum unwind_stop_reason stop_reason;
 };

+#define DEAD_FRAME_LEVEL (-55)
+
+static void
+validate_frame (struct frame_info *frame)
+{
+  gdb_assert (frame->level != DEAD_FRAME_LEVEL);
+}
+
 /* A frame stash used to speed up frame lookups.  */

 /* We currently only stash one frame at a time, as this seems to be
@@ -324,6 +332,8 @@ get_frame_id (struct frame_info *fi)
   if (fi == NULL)
     return null_frame_id;

+  validate_frame (fi);
+
   if (!fi->this_id.p)
     {
       if (frame_debug)
@@ -621,6 +631,8 @@ frame_find_by_id (struct frame_id id)
 static int
 frame_unwind_pc_if_available (struct frame_info *this_frame, CORE_ADDR *pc)
 {
+  validate_frame (this_frame);
+
   if (!this_frame->prev_pc.p)
     {
       if (gdbarch_unwind_pc_p (frame_unwind_arch (this_frame)))
@@ -1522,12 +1534,30 @@ frame_observer_target_changed (struct target_ops *target)
   reinit_frame_cache ();
 }

+typedef struct obstack obstack_s;
+DEF_VEC_O (obstack_s);
+static VEC (obstack_s) *frame_poison_vec;
+
+void frame_garbage_collection (void);
+void
+frame_garbage_collection (void)
+{
+  struct obstack *obstack_p;
+  int ix;
+
+  for (ix = 0; VEC_iterate (obstack_s, frame_poison_vec, ix, obstack_p); ix++)
+    obstack_free (obstack_p, 0);
+
+  VEC_free (obstack_s, frame_poison_vec);
+  frame_poison_vec = NULL;
+}
+
 /* Flush the entire frame cache.  */

 void
 reinit_frame_cache (void)
 {
-  struct frame_info *fi;
+  struct frame_info *fi, *fi_prev;

   /* Tear down all frame caches.  */
   for (fi = current_frame; fi != NULL; fi = fi->prev)
@@ -1538,8 +1568,15 @@ reinit_frame_cache (void)
 	fi->base->unwind->dealloc_cache (fi, fi->base_cache);
     }

+  for (fi = current_frame; fi != NULL; fi = fi_prev)
+    {
+      fi_prev = fi->prev;
+      memset (fi, 0, sizeof (*fi));
+      fi->level = DEAD_FRAME_LEVEL;
+    }
+  VEC_safe_push (obstack_s, frame_poison_vec, &frame_cache_obstack);
+
   /* Since we can't really be sure what the first object allocated was.  */
-  obstack_free (&frame_cache_obstack, 0);
   obstack_init (&frame_cache_obstack);

   if (current_frame != NULL)
@@ -2070,6 +2107,8 @@ get_frame_address_in_block_if_available (struct frame_info *this_frame,
 {
   volatile struct gdb_exception ex;

+  validate_frame (this_frame);
+
   TRY_CATCH (ex, RETURN_MASK_ERROR)
     {
       *pc = get_frame_address_in_block (this_frame);
@@ -2089,6 +2128,8 @@ find_frame_sal (struct frame_info *frame, struct symtab_and_line *sal)
   int notcurrent;
   CORE_ADDR pc;

+  validate_frame (frame);
+
   /* If the next frame represents an inlined function call, this frame's
      sal is the "call site" of that inlined function, which can not
      be inferred from get_frame_pc.  */
@@ -2145,6 +2186,8 @@ find_frame_sal (struct frame_info *frame, struct symtab_and_line *sal)
 CORE_ADDR
 get_frame_base (struct frame_info *fi)
 {
+  validate_frame (fi);
+
   return get_frame_id (fi).stack_addr;
 }

@@ -2220,6 +2263,8 @@ frame_relative_level (struct frame_info *fi)
 enum frame_type
 get_frame_type (struct frame_info *frame)
 {
+  validate_frame (frame);
+
   if (frame->unwind == NULL)
     /* Initialize the frame's unwinder because that's what
        provides the frame's type.  */
@@ -2230,6 +2275,8 @@ get_frame_type (struct frame_info *frame)
 struct program_space *
 get_frame_program_space (struct frame_info *frame)
 {
+  validate_frame (frame);
+
   return frame->pspace;
 }

@@ -2238,6 +2285,8 @@ frame_unwind_program_space (struct frame_info *this_frame)
 {
   gdb_assert (this_frame);

+  validate_frame (this_frame);
+
   /* This is really a placeholder to keep the API consistent --- we
      assume for now that we don't have frame chains crossing
      spaces.  */
@@ -2247,6 +2296,8 @@ frame_unwind_program_space (struct frame_info *this_frame)
 struct address_space *
 get_frame_address_space (struct frame_info *frame)
 {
+  validate_frame (frame);
+
   return frame->aspace;
 }

@@ -2256,6 +2307,8 @@ void
 get_frame_memory (struct frame_info *this_frame, CORE_ADDR addr,
 		  gdb_byte *buf, int len)
 {
+  validate_frame (this_frame);
+
   read_memory (addr, buf, len);
 }

@@ -2266,6 +2319,8 @@ get_frame_memory_signed (struct frame_info *this_frame, CORE_ADDR addr,
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

+  validate_frame (this_frame);
+
   return read_memory_integer (addr, len, byte_order);
 }

@@ -2276,6 +2331,8 @@ get_frame_memory_unsigned (struct frame_info *this_frame, CORE_ADDR addr,
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

+  validate_frame (this_frame);
+
   return read_memory_unsigned_integer (addr, len, byte_order);
 }

@@ -2283,6 +2340,8 @@ int
 safe_frame_unwind_memory (struct frame_info *this_frame,
 			  CORE_ADDR addr, gdb_byte *buf, int len)
 {
+  validate_frame (this_frame);
+
   /* NOTE: target_read_memory returns zero on success!  */
   return !target_read_memory (addr, buf, len);
 }
@@ -2292,12 +2351,15 @@ safe_frame_unwind_memory (struct frame_info *this_frame,
 struct gdbarch *
 get_frame_arch (struct frame_info *this_frame)
 {
+  validate_frame (this_frame);
   return frame_unwind_arch (this_frame->next);
 }

 struct gdbarch *
 frame_unwind_arch (struct frame_info *next_frame)
 {
+  validate_frame (next_frame);
+
   if (!next_frame->prev_arch.p)
     {
       struct gdbarch *arch;
@@ -2326,6 +2388,8 @@ frame_unwind_arch (struct frame_info *next_frame)
 struct gdbarch *
 frame_unwind_caller_arch (struct frame_info *next_frame)
 {
+  validate_frame (next_frame);
+
   return frame_unwind_arch (skip_inlined_frames (next_frame));
 }

@@ -2336,6 +2400,8 @@ get_frame_sp (struct frame_info *this_frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);

+  validate_frame (this_frame);
+
   /* Normality - an architecture that provides a way of obtaining any
      frame inner-most address.  */
   if (gdbarch_unwind_sp_p (gdbarch))
@@ -2355,6 +2421,8 @@ get_frame_sp (struct frame_info *this_frame)
 enum unwind_stop_reason
 get_frame_unwind_stop_reason (struct frame_info *frame)
 {
+  validate_frame (frame);
+
   /* If we haven't tried to unwind past this point yet, then assume
      that unwinding would succeed.  */
   if (frame->prev_p == 0)
diff --git a/gdb/top.c b/gdb/top.c
index 061ad48..d797aff 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -360,6 +360,11 @@ prepare_execute_command (void)
   if (non_stop)
     target_dcache_invalidate ();

+  {
+    extern void frame_garbage_collection (void);
+    frame_garbage_collection ();
+  }
+
   return cleanup;
 }

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Jan Kratochvil]

On Tue, 05 Jun 2012 21:16:36 +0200, Pedro Alves wrote:
> I've then ran the whole testsuite with this on, and that didn't catch
> any other problem.  You mention in PR13866 many situations with stale
> frame_info; did you have some other way to catch those,

No.  This single PR13866 crash and GDB code in general gives me enough
reasoning to believe:

(a) There exist other such crashes in GDB, just not tested by the testsuite.
(b) More of such crashes will regress by changes in the future.

This is just my personal opinion you may not agree with.


> or was that just a hunch?

I appreciate not using this tone.


Regards,
Jan

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 06/05/2012 08:39 PM, Jan Kratochvil wrote:

>> or was that just a hunch?
> 
> I appreciate not using this tone.


Sorry, I did not mean to imply any tone!  It was a legitimate question.

-- 
Pedro Alves

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 06/05/2012 08:39 PM, Jan Kratochvil wrote:

> On Tue, 05 Jun 2012 21:16:36 +0200, Pedro Alves wrote:
>> I've then ran the whole testsuite with this on, and that didn't catch
>> any other problem.  You mention in PR13866 many situations with stale
>> frame_info; did you have some other way to catch those,
> 
> No. 


Okay, then I'll put the "until" fix in.  Thanks.

-- 
Pedro Alves

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Tom Tromey]

Pedro> I've then ran the whole testsuite with this on, and that didn't catch
Pedro> any other problem.  You mention in PR13866 many situations with stale
Pedro> frame_info; did you have some other way to catch those,

Jan> No.  This single PR13866 crash and GDB code in general gives me enough
Jan> reasoning to believe:

Jan> (a) There exist other such crashes in GDB, just not tested by the
Jan> testsuite.
Jan> (b) More of such crashes will regress by changes in the future.

Jan> This is just my personal opinion you may not agree with.

Despite the difficulties perhaps we should try to write a static
analyzer for this.  I am not sure if it could be made reliable enough,
but maybe it could.  The problem cases are when the analyzer must
account for cleanups -- but in this case we might be able to get away
with ignoring them.

Another idea is to simply get rid of frame_info and have only a
frame_id-based API.

Tom

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 06/06/2012 08:38 PM, Tom Tromey wrote:

> Despite the difficulties perhaps we should try to write a static
> analyzer for this.  I am not sure if it could be made reliable enough,
> but maybe it could.  The problem cases are when the analyzer must
> account for cleanups -- but in this case we might be able to get away
> with ignoring them.


If that doesn't work, or if it's too much trouble, we could consider putting
in a variant of my validation patch.  Instead of garbage collecting dead frames
at some specific points, keep say, a FIFO of the last N dead frames (don't free
them, but mark them dead, catching uses of those).  N could be low.  That'd
keep the extra memory bounded.  If this is considered expensive (unlikely),
we could guard it behind the new --development flag (kind of like
checking in gcc).

> Another idea is to simply get rid of frame_info and have only a
> frame_id-based API.

Most, a large majority of frame_info objects I see (when not a transient
use like get_current_frame passed directly to some frame.c function) of what I
see is in the unwinders, where that'd unnecessary, and wouldn't really work.
Most other places tend to already be careful with frame_info lifetime.  There's
execution commands (of which "until" was particularly bad), and a handful of
other things.  There are some paths in handle_inferior_event on software
single-step targets that might be hitting stale frame_info (hey, where
else would problems be, right?), but in all honesty, I don't think there
are that many related lurking bugs to worry that much about.

-- 
Pedro Alves

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 04/04/2012 08:14 PM, Jan Kratochvil wrote:

> 
> Reproducer with:
> ./gdb -nx ~/t/thread -ex 'b 24' -ex r -ex 'until 25'
> Breakpoint 1, main () at /home/jkratoch/t/thread.c:24
> 24	  v++;
> Segmentation fault (core dumped)
> 
> #include <pthread.h>
> #include <assert.h>
> #include <unistd.h>
> 
> static int v;
> 
> static void *start (void *arg)
> {
>   v++;
>   v++;
>   v++;
>   v++;
>   sleep (100);
>   return arg;
> }
> 
> int main (void)
> {
>   pthread_t thread1;
>   int i;
> 
>   i = pthread_create (&thread1, NULL, start, NULL);
>   assert (i == 0);
>   v++;
>   v++;
>   v++;
>   v++;
>   i = pthread_join (thread1, NULL);
>   assert (i == 0);
> 
>   return 0;
> }


This fixes the crash.

2012-06-05  Pedro Alves  <palves@redhat.com>

	PR backtrace/13866

	* breakpoint.c (until_break_command): Only fetch the selected
	frame after decode_line_1.
---

 gdb/breakpoint.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5cc1f64..9757d0d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10815,10 +10815,10 @@ 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 frame_info *frame;
+  struct gdbarch *frame_gdbarch;
+  struct frame_id stack_frame_id;
+  struct frame_id caller_frame_id;
   struct breakpoint *breakpoint;
   struct breakpoint *breakpoint2 = NULL;
   struct cleanup *old_chain;
@@ -10854,6 +10854,11 @@ until_break_command (char *arg, int from_tty, int anywhere)

   old_chain = make_cleanup (null_cleanup, NULL);

+  frame = get_selected_frame (NULL);
+  frame_gdbarch = get_frame_arch (frame);
+  stack_frame_id = get_stack_frame_id (frame);
+  caller_frame_id = frame_unwind_caller_id (frame);
+
   /* Installing a breakpoint invalidates the frame chain (as it may
      need to switch threads), so do any frame handling first.  */

Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

[Pedro Alves]

On 06/05/2012 08:24 PM, Pedro Alves wrote:

> This fixes the crash.

I actually posted an outdated version before...  This version adds a little
comment.  I've applied it.

2012-06-05  Pedro Alves  <palves@redhat.com>

	PR backtrace/13866

	* breakpoint.c (until_break_command): Only fetch the selected
	frame after decode_line_1.
---
 gdb/breakpoint.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5cc1f64..12db39b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10815,10 +10815,10 @@ 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 frame_info *frame;
+  struct gdbarch *frame_gdbarch;
+  struct frame_id stack_frame_id;
+  struct frame_id caller_frame_id;
   struct breakpoint *breakpoint;
   struct breakpoint *breakpoint2 = NULL;
   struct cleanup *old_chain;
@@ -10854,8 +10854,15 @@ until_break_command (char *arg, int from_tty, int anywhere)
 
   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.  */
+  /* Note linespec handling above invalidates the frame chain.
+     Installing a breakpoint also invalidates the frame chain (as it
+     may need to switch threads), so do any frame handling before
+     that.  */
+
+  frame = get_selected_frame (NULL);
+  frame_gdbarch = get_frame_arch (frame);
+  stack_frame_id = get_stack_frame_id (frame);
+  caller_frame_id = frame_unwind_caller_id (frame);
 
   /* Keep within the current frame, or in frames called by the current
      one.  */