[commit] Rename frame_pc_unwind and frame_unwind_id

[commit] Rename frame_pc_unwind and frame_unwind_id

[Daniel Jacobowitz]

This patch is the first non-trivial change in inlining support, and
readily separable.

The users of frame_unwind_id and frame_pc_unwind are all either
inferior control, trying to find the caller / return address of a new
function, or trampoline handling.  I audited all of the uses, and the
right behavior in every one of them is to ignore any inlined functions
at the current location.  A future patch, the one adding inlined
frames, will make the corresponding change to frame_unwind_caller_id
and frame_unwind_caller_pc.  For now, I've just renamed them to
indicate the correct expectations.

Tested on x86_64-linux and committed.

-- 
Daniel Jacobowitz
CodeSourcery

2008-07-15  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame.c (frame_unwind_id): Renamed to ...
	(frame_unwind_caller_id): ... this.  All callers updated.
	(frame_pc_unwind): Renamed to ...
	(frame_unwind_caller_pc): ... this.  All callers updated.
	* frame.h: Document frame_unwind_caller_WHAT functions.
	(frame_unwind_id): Renamed to ...
	(frame_unwind_caller_id): ... this.
	(frame_pc_unwind): Renamed to ...
	(frame_unwind_caller_pc): ... this.
	* hppa-tdep.c (hppa_find_unwind_entry_in_block): Correct comment.
	* stack.c (parse_frame_specification_1): Do not rely on
	frame_unwind_id.

---
 gdb/frame.c           |    8 ++++----
 gdb/frame.h           |    9 +++++++--
 gdb/glibc-tdep.c      |    2 +-
 gdb/hppa-tdep.c       |    2 +-
 gdb/infrun.c          |   14 ++++++++------
 gdb/mips-linux-tdep.c |    2 +-
 gdb/obsd-tdep.c       |    2 +-
 gdb/sol2-tdep.c       |    2 +-
 gdb/stack.c           |   14 +++++++++++---
 9 files changed, 35 insertions(+), 20 deletions(-)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2008-07-02 15:27:52.000000000 -0400
+++ src/gdb/frame.c	2008-07-02 15:44:32.000000000 -0400
@@ -270,7 +270,7 @@ get_frame_id (struct frame_info *fi)
 }
 
 struct frame_id
-frame_unwind_id (struct frame_info *next_frame)
+frame_unwind_caller_id (struct frame_info *next_frame)
 {
   /* Use prev_frame, and not get_prev_frame.  The latter will truncate
      the frame chain, leading to this function unintentionally
@@ -421,7 +421,7 @@ frame_find_by_id (struct frame_id id)
 }
 
 CORE_ADDR
-frame_pc_unwind (struct frame_info *this_frame)
+frame_unwind_caller_pc (struct frame_info *this_frame)
 {
   if (!this_frame->prev_pc.p)
     {
@@ -452,7 +452,7 @@ frame_pc_unwind (struct frame_info *this
       this_frame->prev_pc.p = 1;
       if (frame_debug)
 	fprintf_unfiltered (gdb_stdlog,
-			    "{ frame_pc_unwind (this_frame=%d) -> 0x%s }\n",
+			    "{ frame_unwind_caller_pc (this_frame=%d) -> 0x%s }\n",
 			    this_frame->level,
 			    paddr_nz (this_frame->prev_pc.value));
     }
@@ -1517,7 +1517,7 @@ CORE_ADDR
 get_frame_pc (struct frame_info *frame)
 {
   gdb_assert (frame->next != NULL);
-  return frame_pc_unwind (frame->next);
+  return frame_unwind_caller_pc (frame->next);
 }
 
 /* Return an address that falls within THIS_FRAME's code block.  */
Index: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2008-07-02 15:27:52.000000000 -0400
+++ src/gdb/frame.h	2008-07-02 15:42:01.000000000 -0400
@@ -34,6 +34,11 @@
    frame_unwind_WHAT...(): Unwind THIS frame's WHAT from the NEXT
    frame.
 
+   frame_unwind_caller_WHAT...(): Unwind WHAT for NEXT stack frame's
+   real caller.  Any inlined functions in NEXT's stack frame are
+   skipped.  Use these to ignore any potentially inlined functions,
+   e.g. inlined into the first instruction of a library trampoline.
+
    put_frame_WHAT...(): Put a value into this frame (unsafe, need to
    invalidate the frame / regcache afterwards) (better name more
    strongly hinting at its unsafeness)
@@ -361,7 +366,7 @@ extern CORE_ADDR get_frame_base (struct 
 
    instead, since that avoids the bug.  */
 extern struct frame_id get_frame_id (struct frame_info *fi);
-extern struct frame_id frame_unwind_id (struct frame_info *next_frame);
+extern struct frame_id frame_unwind_caller_id (struct frame_info *next_frame);
 
 /* Assuming that a frame is `normal', return its base-address, or 0 if
    the information isn't available.  NOTE: This address is really only
@@ -515,7 +520,7 @@ extern const char *frame_map_regnum_to_n
    calling frame.  For GDB, `pc' is the resume address and not a
    specific register.  */
 
-extern CORE_ADDR frame_pc_unwind (struct frame_info *frame);
+extern CORE_ADDR frame_unwind_caller_pc (struct frame_info *frame);
 
 /* Discard the specified frame.  Restoring the registers to the state
    of the caller.  */
Index: src/gdb/glibc-tdep.c
===================================================================
--- src.orig/gdb/glibc-tdep.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/glibc-tdep.c	2008-07-02 15:44:14.000000000 -0400
@@ -97,7 +97,7 @@ glibc_skip_solib_resolver (struct gdbarc
         fixup = lookup_minimal_symbol ("fixup", NULL, objfile);
 
       if (fixup && SYMBOL_VALUE_ADDRESS (fixup) == pc)
-	return frame_pc_unwind (get_current_frame ());
+	return frame_unwind_caller_pc (get_current_frame ());
     }
 
   return 0;
Index: src/gdb/hppa-tdep.c
===================================================================
--- src.orig/gdb/hppa-tdep.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/hppa-tdep.c	2008-07-02 15:44:08.000000000 -0400
@@ -1794,7 +1794,7 @@ hppa_find_unwind_entry_in_block (struct 
   /* FIXME drow/20070101: Calling gdbarch_addr_bits_remove on the
      result of get_frame_address_in_block implies a problem.
      The bits should have been removed earlier, before the return
-     value of frame_pc_unwind.  That might be happening already;
+     value of gdbarch_unwind_pc.  That might be happening already;
      if it isn't, it should be fixed.  Then this call can be
      removed.  */
   pc = gdbarch_addr_bits_remove (get_frame_arch (this_frame), pc);
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/infrun.c	2008-07-02 15:43:37.000000000 -0400
@@ -2917,7 +2917,8 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
      being equal, so to get into this block, both the current and
      previous frame must have valid frame IDs.  */
   if (!frame_id_eq (get_frame_id (get_current_frame ()), step_frame_id)
-      && frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
+      && frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
+		      step_frame_id))
     {
       CORE_ADDR real_stop_pc;
 
@@ -3070,7 +3071,7 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
          set step-mode) or we no longer know how to get back
          to the call site.  */
       if (step_stop_if_no_debug
-	  || !frame_id_p (frame_unwind_id (get_current_frame ())))
+	  || !frame_id_p (frame_unwind_caller_id (get_current_frame ())))
 	{
 	  /* If we have no line number and the step-stop-if-no-debug
 	     is set, we stop the step so that the user has a chance to
@@ -3319,7 +3320,7 @@ insert_step_resume_breakpoint_at_frame (
    This is a separate function rather than reusing
    insert_step_resume_breakpoint_at_frame in order to avoid
    get_prev_frame, which may stop prematurely (see the implementation
-   of frame_unwind_id for an example).  */
+   of frame_unwind_caller_id for an example).  */
 
 static void
 insert_step_resume_breakpoint_at_caller (struct frame_info *next_frame)
@@ -3328,15 +3329,16 @@ insert_step_resume_breakpoint_at_caller 
 
   /* We shouldn't have gotten here if we don't know where the call site
      is.  */
-  gdb_assert (frame_id_p (frame_unwind_id (next_frame)));
+  gdb_assert (frame_id_p (frame_unwind_caller_id (next_frame)));
 
   init_sal (&sr_sal);		/* initialize to zeros */
 
   sr_sal.pc = gdbarch_addr_bits_remove
-		(current_gdbarch, frame_pc_unwind (next_frame));
+		(current_gdbarch, frame_unwind_caller_pc (next_frame));
   sr_sal.section = find_pc_overlay (sr_sal.pc);
 
-  insert_step_resume_breakpoint_at_sal (sr_sal, frame_unwind_id (next_frame));
+  insert_step_resume_breakpoint_at_sal (sr_sal,
+					frame_unwind_caller_id (next_frame));
 }
 
 /* Insert a "longjmp-resume" breakpoint at PC.  This is used to set a
Index: src/gdb/mips-linux-tdep.c
===================================================================
--- src.orig/gdb/mips-linux-tdep.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/mips-linux-tdep.c	2008-07-02 15:43:24.000000000 -0400
@@ -701,7 +701,7 @@ mips_linux_skip_resolver (struct gdbarch
   resolver = lookup_minimal_symbol ("__dl_runtime_resolve", NULL, NULL);
 
   if (resolver && SYMBOL_VALUE_ADDRESS (resolver) == pc)
-    return frame_pc_unwind (get_current_frame ());
+    return frame_unwind_caller_pc (get_current_frame ());
 
   return 0;
 }
Index: src/gdb/obsd-tdep.c
===================================================================
--- src.orig/gdb/obsd-tdep.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/obsd-tdep.c	2008-07-02 15:43:07.000000000 -0400
@@ -30,7 +30,7 @@ obsd_skip_solib_resolver (struct gdbarch
 
   msym = lookup_minimal_symbol("_dl_bind", NULL, NULL);
   if (msym && SYMBOL_VALUE_ADDRESS (msym) == pc)
-    return frame_pc_unwind (get_current_frame ());
+    return frame_unwind_caller_pc (get_current_frame ());
   else
     return find_solib_trampoline_target (get_current_frame (), pc);
 }
Index: src/gdb/sol2-tdep.c
===================================================================
--- src.orig/gdb/sol2-tdep.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/sol2-tdep.c	2008-07-02 15:43:01.000000000 -0400
@@ -30,7 +30,7 @@ sol2_skip_solib_resolver (struct gdbarch
 
   msym = lookup_minimal_symbol("elf_bndr", NULL, NULL);
   if (msym && SYMBOL_VALUE_ADDRESS (msym) == pc)
-    return frame_pc_unwind (get_current_frame ());
+    return frame_unwind_caller_pc (get_current_frame ());
 
   return 0;
 }
Index: src/gdb/stack.c
===================================================================
--- src.orig/gdb/stack.c	2008-07-02 15:27:53.000000000 -0400
+++ src/gdb/stack.c	2008-07-02 15:43:14.000000000 -0400
@@ -854,8 +854,16 @@ parse_frame_specification_1 (const char 
 	{
 	  if (frame_id_eq (id, get_frame_id (fid)))
 	    {
-	      while (frame_id_eq (id, frame_unwind_id (fid)))
-		fid = get_prev_frame (fid);
+	      struct frame_info *prev_frame;
+
+	      while (1)
+		{
+		  prev_frame = get_prev_frame (fid);
+		  if (!prev_frame
+		      || !frame_id_eq (id, get_frame_id (prev_frame)))
+		    break;
+		  fid = prev_frame;
+		}
 	      return fid;
 	    }
 	}
@@ -984,7 +992,7 @@ frame_info (char *addr_exp, int from_tty
   puts_filtered ("; ");
   wrap_here ("    ");
   printf_filtered ("saved %s ", pc_regname);
-  fputs_filtered (paddress (frame_pc_unwind (fi)), gdb_stdout);
+  fputs_filtered (paddress (frame_unwind_caller_pc (fi)), gdb_stdout);
   printf_filtered ("\n");
 
   if (calling_frame_info == NULL)

Re: [commit] Rename frame_pc_unwind and frame_unwind_id

[Mark Kettenis]

> Date: Tue, 15 Jul 2008 15:01:25 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> This patch is the first non-trivial change in inlining support, and
> readily separable.

Non-trivial...

> The users of frame_unwind_id and frame_pc_unwind are all either
> inferior control, trying to find the caller / return address of a new
> function, or trampoline handling.  I audited all of the uses, and the
> right behavior in every one of them is to ignore any inlined functions
> at the current location.  A future patch, the one adding inlined
> frames, will make the corresponding change to frame_unwind_caller_id
> and frame_unwind_caller_pc.  For now, I've just renamed them to
> indicate the correct expectations.

...so cann't we discuss this first please?

Re: [commit] Rename frame_pc_unwind and frame_unwind_id

[Daniel Jacobowitz]

On Tue, Jul 15, 2008 at 09:08:34PM +0200, Mark Kettenis wrote:
> > Date: Tue, 15 Jul 2008 15:01:25 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > This patch is the first non-trivial change in inlining support, and
> > readily separable.
> 
> Non-trivial...
> 
> > The users of frame_unwind_id and frame_pc_unwind are all either
> > inferior control, trying to find the caller / return address of a new
> > function, or trampoline handling.  I audited all of the uses, and the
> > right behavior in every one of them is to ignore any inlined functions
> > at the current location.  A future patch, the one adding inlined
> > frames, will make the corresponding change to frame_unwind_caller_id
> > and frame_unwind_caller_pc.  For now, I've just renamed them to
> > indicate the correct expectations.
> 
> ...so cann't we discuss this first please?

Sorry.  Want me to back it out?  I'm not going to commit anything
further; I've just posted the meat of the patch, which is harder to
separate.

I'm interested in your comments (about this patch or the larger one).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [commit] Rename frame_pc_unwind and frame_unwind_id

[Mark Kettenis]

> Date: Tue, 15 Jul 2008 15:22:13 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Tue, Jul 15, 2008 at 09:08:34PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 15 Jul 2008 15:01:25 -0400
> > > From: Daniel Jacobowitz <drow@false.org>
> > > 
> > > This patch is the first non-trivial change in inlining support, and
> > > readily separable.
> > 
> > Non-trivial...
> > 
> > > The users of frame_unwind_id and frame_pc_unwind are all either
> > > inferior control, trying to find the caller / return address of a new
> > > function, or trampoline handling.  I audited all of the uses, and the
> > > right behavior in every one of them is to ignore any inlined functions
> > > at the current location.  A future patch, the one adding inlined
> > > frames, will make the corresponding change to frame_unwind_caller_id
> > > and frame_unwind_caller_pc.  For now, I've just renamed them to
> > > indicate the correct expectations.
> > 
> > ...so cann't we discuss this first please?
> 
> Sorry.  Want me to back it out?  I'm not going to commit anything
> further; I've just posted the meat of the patch, which is harder to
> separate.

If it's easy for you to back it out, I'd appreciate it.  I really
don't think the new names are an improvement, and they are longer
makeing the code slightly less readable...

> I'm interested in your comments (about this patch or the larger one).

...but my main concern is that this diff and the larger one change the
meaning of a frame.  It seems it gets us further away of what I
consider to be frame.  Please give me a day or so to study the diff a
bit more, before I give a more detailed reaction.

Sorry that I didn't look closely enough at your earlier mails about
the inlining support.  I thought they were mainly dealing with the
debug info side of things and didn't really affect the unwinders.

Re: [commit] Rename frame_pc_unwind and frame_unwind_id

[Daniel Jacobowitz]

On Wed, Jul 16, 2008 at 12:41:12AM +0200, Mark Kettenis wrote:
> If it's easy for you to back it out, I'd appreciate it.  I really
> don't think the new names are an improvement, and they are longer
> makeing the code slightly less readable...

No trouble, so I backed it out.

They're not an improvement in the current case; without the
distinction between inlined and non-inlined frames, there's no need to
distinguish.  They only make sense in the context of inlined frames...

> > I'm interested in your comments (about this patch or the larger one).
> 
> ...but my main concern is that this diff and the larger one change the
> meaning of a frame.  It seems it gets us further away of what I
> consider to be frame.  Please give me a day or so to study the diff a
> bit more, before I give a more detailed reaction.

... which brings us over here.  Thanks for looking at it - I really
appreciate it!

I tried a couple of approaches when I was putting this together, and
frankly, I couldn't find any sensible way to not put the inlined
functions into the normal frame chain.  They have to be frames, or
else everything that looks at a frame has to look at some other
object; we need them most places that we have a frame.  If they're not
in the frame chain, then all the obvious uses of get_prev_frame /
get_next_frame have questionable semantics.

I've already ripped out the ugliest bits of the frame design in my
most recent reworking of this patch; I couldn't see how to get rid of
them the first time but a year later it was clear.  So maybe another
perspective will be able to clean this up further.  Let me know if you
have any questions on the patch; I don't have time right now to write
as much internals documentation for it as I'd like, but I'll explain
any bit that isn't clear.

-- 
Daniel Jacobowitz
CodeSourcery