new gdb arch routine FRAME_UNCHANGED

new gdb arch routine FRAME_UNCHANGED

[David Mosberger]

The patch below defines a new gdbarch routine called FRAME_UNCHANGED.
This is needed for gdb/ia64 because the ia64 architecture has two
separate stack pointers (one for memory stack and one for register stack).
Because of the two stacks, it's not possible to look just at the frame
pointer and PC to determine whether a frame changed.

If the patch looks OK, I'd appreciate if someone could apply it.

Note: the patch below doesn't include the ia64 version of
FRAME_UNCHANGED.  It is part of a larger update to add unwind library
support to gdb/ia64 and I'll submit that separately.  So, for now, the
patch is effectively a no-op for all platforms.

Also, it will be necessary to re-generate gdbarch.[ch] after applying
the patch.

Thanks,

	--david

ChangeLog

2002-05-08  David Mosberger-Tang  <David.Mosberger@acm.org>

	* gdbarch.sh (FRAME_UNCHANGED): New routine.

	* frame.h: Declare default_frame_unchanged() and
	generic_dummy_frame_chain().

	* blockframe.c (default_frame_unchanged): New function.
	(get_prev_frame): Use FRAME_UNCHANGED() to determine whether
	the new frame is different from the old one.

diff -u -r1.134 gdbarch.sh
--- gdbarch.sh	8 May 2002 20:43:04 -0000	1.134
+++ gdbarch.sh	9 May 2002 05:33:48 -0000
@@ -548,6 +548,7 @@
 f:2:BREAKPOINT_FROM_PC:const unsigned char *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr:::legacy_breakpoint_from_pc::0
 f:2:MEMORY_INSERT_BREAKPOINT:int:memory_insert_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_insert_breakpoint::0
 f:2:MEMORY_REMOVE_BREAKPOINT:int:memory_remove_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_remove_breakpoint::0
+f:2:FRAME_UNCHANGED:int:frame_unchanged:struct frame_info *prev, struct frame_info *next:prev, next::0:default_frame_unchanged::0
 v:2:DECR_PC_AFTER_BREAK:CORE_ADDR:decr_pc_after_break::::0:-1
 f::PREPARE_TO_PROCEED:int:prepare_to_proceed:int select_it:select_it::0:default_prepare_to_proceed::0
 v:2:FUNCTION_START_OFFSET:CORE_ADDR:function_start_offset::::0:-1
diff -u -r1.27 blockframe.c
--- blockframe.c	5 May 2002 01:15:12 -0000	1.27
+++ blockframe.c	9 May 2002 05:33:25 -0000
@@ -39,6 +39,16 @@
 
 void _initialize_blockframe (void);
 
+/* Default FRAME_UNCHANGED.  Returns true if the current and next
+   frame have the same frame address and the same PC.  This version
+   should work for most platforms.  One exception is ia64, since it
+   has separate memory and register stacks.  */
+int
+default_frame_unchanged (struct frame_info *prev, struct frame_info *next)
+{
+  return (prev->frame == next->frame && prev->pc == next->pc);
+}
+
 /* A default FRAME_CHAIN_VALID, in the form that is suitable for most
    targets.  If FRAME_CHAIN_VALID returns zero it means that the given
    frame is the outermost one and has no caller. */
@@ -447,8 +457,7 @@
      this can't be an architecture-independent check.  */
   if (next_frame != NULL)
     {
-      if (prev->frame == next_frame->frame
-	  && prev->pc == next_frame->pc)
+      if (FRAME_UNCHANGED (prev, next_frame))
 	{
 	  next_frame->prev = NULL;
 	  obstack_free (&frame_cache_obstack, prev);
diff -u -r1.17 frame.h
--- frame.h	5 May 2002 02:24:38 -0000	1.17
+++ frame.h	9 May 2002 05:33:38 -0000
@@ -160,6 +160,12 @@
 
 #define FRAME_FP(fi) ((fi)->frame)
 
+/* Define a default FRAME_UNCHANGED in the form that that is suitable
+   for most targets.  If FRAME_UNCHANGED returns zero it means that
+   the two frames are indistinguishable.  */
+
+extern int default_frame_unchanged (struct frame_info *, struct frame_info *);
+
 /* Level of the frame: 0 for innermost, 1 for its caller, ...; or -1
    for an invalid frame.  */
 

Re: new gdb arch routine FRAME_UNCHANGED

[Andrew Cagney]

> The patch below defines a new gdbarch routine called FRAME_UNCHANGED.
> This is needed for gdb/ia64 because the ia64 architecture has two
> separate stack pointers (one for memory stack and one for register stack).
> Because of the two stacks, it's not possible to look just at the frame
> pointer and PC to determine whether a frame changed.
> 
> If the patch looks OK, I'd appreciate if someone could apply it.
> 
> Note: the patch below doesn't include the ia64 version of
> FRAME_UNCHANGED.  It is part of a larger update to add unwind library
> support to gdb/ia64 and I'll submit that separately.  So, for now, the
> patch is effectively a no-op for all platforms.

Is the patch available somewhere?

When you say unwind library support do you mean dwarf2cfi or something 
else?  From memory CFI identifies a frame using CFA and PC, I'm 
wondering how things work here.

cf: http://sources.redhat.com/ml/gdb-patches/2002-04/msg00749.html
There is code through out GDB that relies on being able to re-find a 
frame in the frame chain.  The code typically relies on just the frame 
address (oops).  The referenced patch changes it to use frame/pc.  If I 
understand what you're saying correctly, still more information will 
need to be saved?

> Also, it will be necessary to re-generate gdbarch.[ch] after applying
> the patch.

(that's normal)

enjoy,
Andrew

Re: new gdb arch routine FRAME_UNCHANGED

[David Mosberger]

>>>>> On Fri, 10 May 2002 21:04:47 -0400, Andrew Cagney <ac131313@cygnus.com> said:

  Andrew> Is the patch available somewhere?

Not online, but if you want me to, I'm happy to send the complete
patch to gdb-patches so you can see where I'm going (or thought I was
going... ;-).

  Andrew> When you say unwind library support do you mean dwarf2cfi or
  Andrew> something else?  From memory CFI identifies a frame using
  Andrew> CFA and PC, I'm wondering how things work here.

The ia64 conventions come with their own unwind info.  The conventions
do not define a unique address/identifier for each frame.

  Andrew> cf:
  Andrew> http://sources.redhat.com/ml/gdb-patches/2002-04/msg00749.html
  Andrew> There is code through out GDB that relies on being able to
  Andrew> re-find a frame in the frame chain.  The code typically
  Andrew> relies on just the frame address (oops).  The referenced
  Andrew> patch changes it to use frame/pc.  If I understand what
  Andrew> you're saying correctly, still more information will need to
  Andrew> be saved?

Yes, this is an issue I was wondering about, too.  My goal was fairly
limited though: I just wanted to change gdb/ia64 to an unwind-info
based implementation that works at least as well as the old version of
gdb/ia64.  I didn't look into this specific issue.

It sounds like for ia64 we would need to add a another field to struct
frame_id to track the register stack address of a frame.  Otherwise, a
recursive function that only uses the register stack would lead to a
series of indistinguishable frames (a simple recursive factorial would
do that).

Is the idea to treat frame_id as an opaque structure?  If so, I could
add a routine to the unwind library API to obtain a unique frame-id
for a given frame.  That way, the ia64 issue could be hidden behind
the API.

	--david

Re: new gdb arch routine FRAME_UNCHANGED

[Andrew Cagney]

> On Fri, 10 May 2002 21:04:47 -0400, Andrew Cagney <ac131313@cygnus.com> said:
> 
> 
>   Andrew> Is the patch available somewhere?
> 
> Not online, but if you want me to, I'm happy to send the complete
> patch to gdb-patches so you can see where I'm going (or thought I was
> going... ;-).

Tossing out the work-in-progress will probably be useful - KevinB (ia64 
maintainer) would at very likely be interested in what is comming his 
way :-)  (I should also double check what HP's Assignement status is.)

>   Andrew> When you say unwind library support do you mean dwarf2cfi or
>   Andrew> something else?  From memory CFI identifies a frame using
>   Andrew> CFA and PC, I'm wondering how things work here.
> 
> The ia64 conventions come with their own unwind info.  The conventions
> do not define a unique address/identifier for each frame.

>   Andrew> cf:
>   Andrew> http://sources.redhat.com/ml/gdb-patches/2002-04/msg00749.html
>   Andrew> There is code through out GDB that relies on being able to
>   Andrew> re-find a frame in the frame chain.  The code typically
>   Andrew> relies on just the frame address (oops).  The referenced
>   Andrew> patch changes it to use frame/pc.  If I understand what
>   Andrew> you're saying correctly, still more information will need to
>   Andrew> be saved?
> 
> Yes, this is an issue I was wondering about, too.  My goal was fairly
> limited though: I just wanted to change gdb/ia64 to an unwind-info
> based implementation that works at least as well as the old version of
> gdb/ia64.  I didn't look into this specific issue.

> It sounds like for ia64 we would need to add a another field to struct
> frame_id to track the register stack address of a frame.  Otherwise, a
> recursive function that only uses the register stack would lead to a
> series of indistinguishable frames (a simple recursive factorial would
> do that).

Yep.

Hmm, is it possible to re-enter a function without changing the register 
stack pointer?  If it isn't then the register stack pointer could be 
used for frame->frame.

> Is the idea to treat frame_id as an opaque structure?  If so, I could
> add a routine to the unwind library API to obtain a unique frame-id
> for a given frame.  That way, the ia64 issue could be hidden behind
> the API.

The value can't be really opaque.  GUI code and MI are easier if they 
don't have to worry about malloc/free (frame_id being a lightweight) and 
MI needs to be able to pass the contents back to clients.

However, gdbarch methods to fill in and compare frame_id's could 
certainly be added.

Andrew

Re: new gdb arch routine FRAME_UNCHANGED

[David Mosberger]

>>>>> On Sat, 11 May 2002 23:52:33 -0400, Andrew Cagney <ac131313@cygnus.com> said:

  Andrew> Tossing out the work-in-progress will probably be useful -
  Andrew> KevinB (ia64 maintainer) would at very likely be interested
  Andrew> in what is comming his way :-)

Oh, I already discussed the complete patch with KevinB.

  Andrew> Hmm, is it possible to re-enter a function without changing
  Andrew> the register stack pointer?

Yes.

  Andrew> If it isn't then the register
  Andrew> stack pointer could be used for frame->frame.

That doesn't work.  I think you might be able to get away with using
the memory stack pointer *OR* the register stack pointer as a unique
id for each frame, but that seems rather ugly and I suspect there are
cases where you might run into ambiguities.

  >> Is the idea to treat frame_id as an opaque structure?  If so, I
  >> could add a routine to the unwind library API to obtain a unique
  >> frame-id for a given frame.  That way, the ia64 issue could be
  >> hidden behind the API.

  Andrew> The value can't be really opaque.  GUI code and MI are
  Andrew> easier if they don't have to worry about malloc/free
  Andrew> (frame_id being a lightweight) and MI needs to be able to
  Andrew> pass the contents back to clients.

Oh, by "opaque" I didn't mean a "void *".  Just an object of a fixed
size with an undefined structure.  I agree that it's preferable to be
able to declare automatic variables of this type.  I was thinking of
having an array of "void *" inside an struct, with the array size
being architecture-specific.

  Andrew> However, gdbarch methods to fill in and compare frame_id's
  Andrew> could certainly be added.

OK, that would probably do it.  Do you see any need to calculate a
hash or is comparison for equality sufficient?

	--david

Re: new gdb arch routine FRAME_UNCHANGED

[Andrew Cagney]

> Oh, by "opaque" I didn't mean a "void *".  Just an object of a fixed
> size with an undefined structure.  I agree that it's preferable to be
> able to declare automatic variables of this type.  I was thinking of
> having an array of "void *" inside an struct, with the array size
> being architecture-specific.

Ah, ok. I guess `struct { CORE_ADDR x[3]; }'.  If someone finds they 
need more entries then we can just keep making it bigger.  Trailing 
unused entries are zero.

The reason for advising against pointers or variable size struct is that 
these objects are not well managed in terms of malloc/delete.  The ptid 
is another example of the same thing of the same thing - no need to 
worry about memory leaks.

>   Andrew> However, gdbarch methods to fill in and compare frame_id's
>   Andrew> could certainly be added.
> 
> OK, that would probably do it.  Do you see any need to calculate a
> hash or is comparison for equality sufficient?

Some existing code wants to compare based on just frame.  See 
stack.c:parse_frame_specification.  Stop on first trailing zero?

Also, code wants to determine which code is more INNER_THAN().  With two 
stacks, that starts to get really weird!

Andrew

Re: new gdb arch routine FRAME_UNCHANGED

[Kevin Buettner]

On May 13,  6:46pm, Andrew Cagney wrote:

> Also, code wants to determine which code is more INNER_THAN().  With two 
> stacks, that starts to get really weird!

Yes.  On IA-64, the meaning of INNER_THAN is different for the two stacks.
(I.e, the stacks grow in different directions.)

Kevin

Re: new gdb arch routine FRAME_UNCHANGED

[Andrew Cagney]

> On May 13,  6:46pm, Andrew Cagney wrote:
> 
> 
>> Also, code wants to determine which code is more INNER_THAN().  With two 
>> stacks, that starts to get really weird!
> 
> 
> Yes.  On IA-64, the meaning of INNER_THAN is different for the two stacks.
> (I.e, the stacks grow in different directions.)
> 
> Kevin

My brain hurts!  This is increasingly like floating point with +0 != -0. 
  We have a situtation where for:

	frame_id a, b; // contain both SP's and PC

the test:

	!FRAME_ID_INNER_THAN(a,b) && !FRAME_INNER_THAN(b,a)

does not imply

	FRAME_ID_SAME(a,b)

(or what ever the functions are called).

Andrew

Re: new gdb arch routine FRAME_UNCHANGED

[David Mosberger]

>>>>> On Mon, 13 May 2002 19:14:32 -0400, Andrew Cagney <ac131313@cygnus.com> said:

  Andrew> My brain hurts!  This is increasingly like floating point
  Andrew> with +0 != -0.  We have a situtation where for:

  Andrew> 	frame_id a, b; // contain both SP's and PC

  Andrew> the test:

  Andrew> 	!FRAME_ID_INNER_THAN(a,b) && !FRAME_INNER_THAN(b,a)

  Andrew> does not imply

  Andrew> 	FRAME_ID_SAME(a,b)

  Andrew> (or what ever the functions are called).

I'm not sure I'm following you here.  Why is there both
FRAME_ID_INNER_THAN() and FRAME_INNER_THAN()?

	--david

Re: new gdb arch routine FRAME_UNCHANGED

[Andrew Cagney]

>   Andrew> the test:
> 
>   Andrew> 	!FRAME_ID_INNER_THAN(a,b) && !FRAME_INNER_THAN(b,a)
> 
>   Andrew> does not imply
> 
>   Andrew> 	FRAME_ID_SAME(a,b)
> 
>   Andrew> (or what ever the functions are called).
> 
> I'm not sure I'm following you here.  Why is there both
> FRAME_ID_INNER_THAN() and FRAME_INNER_THAN()?
> 

Typo :-(

Re: new gdb arch routine FRAME_UNCHANGED

[David Mosberger]

>>>>> On Mon, 13 May 2002 19:14:32 -0400, Andrew Cagney <ac131313@cygnus.com> said:

  Andrew> My brain hurts!  This is increasingly like floating point
  Andrew> with +0 != -0.  We have a situtation where for:

  Andrew> 	frame_id a, b; // contain both SP's and PC

  Andrew> the test:

  Andrew> 	!FRAME_ID_INNER_THAN(a,b) && !FRAME_INNER_THAN(b,a)

  Andrew> does not imply

  Andrew> 	FRAME_ID_SAME(a,b)

  Andrew> (or what ever the functions are called).

If you define FRAME_ID_INNER_THAN(a,b) as:

	((a.bsp > b.bsp) || (a.sp < b.sp))

then the relationship should still hold (assuming "bsp" represents the
register stack pointer and sp the memory stack pointer).

	--david

Re: new gdb arch routine FRAME_UNCHANGED

[Andrew Cagney]

> The patch below defines a new gdbarch routine called FRAME_UNCHANGED.
> This is needed for gdb/ia64 because the ia64 architecture has two
> separate stack pointers (one for memory stack and one for register stack).
> Because of the two stacks, it's not possible to look just at the frame
> pointer and PC to determine whether a frame changed.
> 
> If the patch looks OK, I'd appreciate if someone could apply it.
> 
> Note: the patch below doesn't include the ia64 version of
> FRAME_UNCHANGED.  It is part of a larger update to add unwind library
> support to gdb/ia64 and I'll submit that separately.  So, for now, the
> patch is effectively a no-op for all platforms.
> 
> Also, it will be necessary to re-generate gdbarch.[ch] after applying
> the patch.

Just to follow this up.  The frame_id code has been committed.  I think 
this has the potential of affecting the way you did your patch.  For 
instance, it could compare frame_id's for either sameness or more 
inner_than.

Andrew