[RFA] parse_frame_specification (stack.c)

[RFA] parse_frame_specification (stack.c)

[David Taylor]

[This is a revision of my previous patch.

For most processors (specifically, those that use the default identity
transformations for pointer -> address and address -> pointer), this
patch is a no op.]

In gdb, if you say:

    "frame <small-number>"
or
    "info frame <small-number>"

then you expect to either move up or down some number of frames or to
get information on the frame having the specified "index".

But, if for your gdb target, addresses and pointers are different,
then the current code in parse_frame_specification will treat the
number as a pointer and convert it to an address.

So, if you have a Harvard architecture processor where a pointer of 0
(say) corresponds to a text address of 0x2000000 and a data address of
0x1000000, and you say

    frame 0

then gdb will try to move to frame 0x1000000.

Assuming you don't have that many frames, it will then try to create a
frame at address 0x1000000.  Which, in my case, will generate an
error...

This fixes it so that

    frame 0

will do the right thing on such configurations.

ChangeLog entry:

	* stack.c (parse_frame_specification): For one argument case,
 	handle the situation where the argument is an integer, not an
 	address -- arguably the most common case.  This matters on
	targets where pointers and addresses are different.

Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.12
diff -c -r1.12 stack.c
*** stack.c	2001/02/10 12:01:11	1.12
--- stack.c	2001/03/05 16:47:52
***************
*** 704,709 ****
--- 704,710 ----
    int numargs = 0;
  #define	MAXARGS	4
    CORE_ADDR args[MAXARGS];
+   int level;
  
    if (frame_exp)
      {
***************
*** 723,730 ****
  	  addr_string = savestring (frame_exp, p - frame_exp);
  
  	  {
  	    tmp_cleanup = make_cleanup (xfree, addr_string);
! 	    args[numargs++] = parse_and_eval_address (addr_string);
  	    do_cleanups (tmp_cleanup);
  	  }
  
--- 724,738 ----
  	  addr_string = savestring (frame_exp, p - frame_exp);
  
  	  {
+ 	    value_ptr vp;
+ 
  	    tmp_cleanup = make_cleanup (xfree, addr_string);
! 
! 	    vp = parse_and_eval (addr_string);
! 	    if (numargs == 0)
! 	      level = value_as_long (vp);
! 
! 	    args[numargs++] = value_as_pointer (vp);
  	    do_cleanups (tmp_cleanup);
  	  }
  
***************
*** 744,750 ****
        /* NOTREACHED */
      case 1:
        {
- 	int level = args[0];
  	struct frame_info *fid =
  	find_relative_frame (get_current_frame (), &level);
  	struct frame_info *tfid;
--- 752,757 ----

Re: [RFA] parse_frame_specification (stack.c)

[Fernando Nasser]

David,

The real problem here is that there is an ambiguity in this command
argument specification.  If a frame is specified as an address, it
should be proceeded by a "*" as we do in the break command.

It seems that problems like this have been encountered before.  here is
the comment in the code that refers to s similar situation:

        /* If SETUP_ARBITRARY_FRAME is defined, then frame
specifications
           take at least 2 addresses.  It is important to detect this
case
           here so that "frame 100" does not give a confusing error
message
           like "frame specification requires two addresses".  This of
course
           does not solve the "frame 100" problem for machines on which
           a frame specification can be made with one address.  To solve
           that, we need a new syntax for a specifying a frame by
address.
           I think the cleanest syntax is $frame(0x45)
($frame(0x23,0x45) for
           two args, etc.), but people might think that is too much
typing,
           so I guess *0x23,0x45 would be a possible alternative (commas
           really should be used instead of spaces to delimit; using
spaces
           normally works in an expression).  */

Maybe we should start requiring the * for addresses and if not assuming
it is a stack level (small integer as you say) and update the manual
accordingly.

Fernando



David Taylor wrote:
> 
> [This is a revision of my previous patch.
> 
> For most processors (specifically, those that use the default identity
> transformations for pointer -> address and address -> pointer), this
> patch is a no op.]
> 
> In gdb, if you say:
> 
>     "frame <small-number>"
> or
>     "info frame <small-number>"
> 
> then you expect to either move up or down some number of frames or to
> get information on the frame having the specified "index".
> 
> But, if for your gdb target, addresses and pointers are different,
> then the current code in parse_frame_specification will treat the
> number as a pointer and convert it to an address.
> 
> So, if you have a Harvard architecture processor where a pointer of 0
> (say) corresponds to a text address of 0x2000000 and a data address of
> 0x1000000, and you say
> 
>     frame 0
> 
> then gdb will try to move to frame 0x1000000.
> 
> Assuming you don't have that many frames, it will then try to create a
> frame at address 0x1000000.  Which, in my case, will generate an
> error...
> 
> This fixes it so that
> 
>     frame 0
> 
> will do the right thing on such configurations.
> 
> ChangeLog entry:
> 
>         * stack.c (parse_frame_specification): For one argument case,
>         handle the situation where the argument is an integer, not an
>         address -- arguably the most common case.  This matters on
>         targets where pointers and addresses are different.
> 
> Index: stack.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/stack.c,v
> retrieving revision 1.12
> diff -c -r1.12 stack.c
> *** stack.c     2001/02/10 12:01:11     1.12
> --- stack.c     2001/03/05 16:47:52
> ***************
> *** 704,709 ****
> --- 704,710 ----
>     int numargs = 0;
>   #define       MAXARGS 4
>     CORE_ADDR args[MAXARGS];
> +   int level;
> 
>     if (frame_exp)
>       {
> ***************
> *** 723,730 ****
>           addr_string = savestring (frame_exp, p - frame_exp);
> 
>           {
>             tmp_cleanup = make_cleanup (xfree, addr_string);
> !           args[numargs++] = parse_and_eval_address (addr_string);
>             do_cleanups (tmp_cleanup);
>           }
> 
> --- 724,738 ----
>           addr_string = savestring (frame_exp, p - frame_exp);
> 
>           {
> +           value_ptr vp;
> +
>             tmp_cleanup = make_cleanup (xfree, addr_string);
> !
> !           vp = parse_and_eval (addr_string);
> !           if (numargs == 0)
> !             level = value_as_long (vp);
> !
> !           args[numargs++] = value_as_pointer (vp);
>             do_cleanups (tmp_cleanup);
>           }
> 
> ***************
> *** 744,750 ****
>         /* NOTREACHED */
>       case 1:
>         {
> -       int level = args[0];
>         struct frame_info *fid =
>         find_relative_frame (get_current_frame (), &level);
>         struct frame_info *tfid;
> --- 752,757 ----

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] parse_frame_specification (stack.c)

[Andrew Cagney]

Fernando Nasser wrote:
> 
> David,
> 
> The real problem here is that there is an ambiguity in this command
> argument specification.  If a frame is specified as an address, it
> should be proceeded by a "*" as we do in the break command.
> 
> It seems that problems like this have been encountered before.  here is
> the comment in the code that refers to s similar situation:

See:

http://sources.redhat.com/gdb/onlinedocs/gdb_7.html#SEC43

I believe David is preserving documented behavour.

	Andrew

Re: [RFA] parse_frame_specification (stack.c)

[Andrew Cagney]

David Taylor wrote:

> ChangeLog entry:
> 
>         * stack.c (parse_frame_specification): For one argument case,
>         handle the situation where the argument is an integer, not an
>         address -- arguably the most common case.  This matters on
>         targets where pointers and addresses are different.

Yes, fine, approved.  (Perhaphs just add a comment that the code is
careful to not evaluate the expression twice - avoiding sideeffects).

	Andrew

Re: [RFA] parse_frame_specification (stack.c)

[Fernando Nasser]

Andrew Cagney wrote:
> 
> Fernando Nasser wrote:
> >
> > David,
> >
> > The real problem here is that there is an ambiguity in this command
> > argument specification.  If a frame is specified as an address, it
> > should be proceeded by a "*" as we do in the break command.
> >
> > It seems that problems like this have been encountered before.  here is
> > the comment in the code that refers to s similar situation:
> 
> See:
> 
> http://sources.redhat.com/gdb/onlinedocs/gdb_7.html#SEC43
> 
> I believe David is preserving documented behavour.
> 

You got that right.  I mentioned that we could fix the syntax (manual
included) so it is not any longer ambiguous.

Instead of inventing a syntax, I suggested that we do as we already do
with breakpoints.  Numbers are breakpoints  *NNNNNNNN are addresses.  I
don't particularly like the breakpoints syntax.  I wish people had used
"#N" to indicate a breakpoint number or a stack level.  That would also
make things unambiguous.

But, anyway, frames at very low addresses are not very likely so I guess
we should just leave things as they are.



-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] parse_frame_specification (stack.c)

[Eli Zaretskii]

On Mon, 5 Mar 2001, Fernando Nasser wrote:

> Maybe we should start requiring the * for addresses and if not assuming
> it is a stack level (small integer as you say) and update the manual
> accordingly.

I agree.

Re: [RFA] parse_frame_specification (stack.c)

[Eli Zaretskii]

On Mon, 5 Mar 2001, Fernando Nasser wrote:

> But, anyway, frames at very low addresses are not very likely so I guess
> we should just leave things as they are.

How low is ``low''?

The lowest possible frame address is 0x1000, I guess (for a
hypothetical architecture which leaves only the null page uncommitted
and has its stack right after that).  Is it unreasonable to expect 4K
frames?  I don't think so; I once had to debug a program with infinite
recursion, where I needed to wade through 750K(!) frames.  As another
data point, Emacs during garbage collection routinely uses 10K or more
recursive invocations of mark_object function and its ilk.

Re: [RFA] parse_frame_specification (stack.c)

[Fernando Nasser]

My thoughts exactly.  I did not know of any examples with 4K frames though.
But we seem to have been outnumbered in the "*" approach :-)

We can adopt an heuristic that assumes "4" as a stack level and "0x4" as a frame address, thus given the user a chance to break the ambiguity (i.e. decimals are stack levels and hex numbers are addresses). We add this note to the manual and perhaps to the help.  But what would we do with computed values like "frame $var12"?  Conversely, "frame $var12" and "frame *$var12" are certainly different.

I still think the "*" is the only definitive solution.

Fernando  





Eli Zaretskii wrote:
> 
On Mon, 5 Mar 2001, Fernando Nasser wrote:

> Maybe we should start requiring the * for addresses and if not assuming
> it is a stack level (small integer as you say) and update the manual
> accordingly.

I agree.

> On Mon, 5 Mar 2001, Fernando Nasser wrote:
> 
> > Maybe we should start requiring the * for addresses and if not assuming
> > it is a stack level (small integer as you say) and update the manual
> > accordingly.
> 
> I agree.

On Mon, 5 Mar 2001, Fernando Nasser wrote:

> But, anyway, frames at very low addresses are not very likely so I guess
> we should just leave things as they are.

How low is ``low''?

The lowest possible frame address is 0x1000, I guess (for a
hypothetical architecture which leaves only the null page uncommitted
and has its stack right after that).  Is it unreasonable to expect 4K
frames?  I don't think so; I once had to debug a program with infinite
recursion, where I needed to wade through 750K(!) frames.  As another
data point, Emacs during garbage collection routinely uses 10K or more
recursive invocations of mark_object function and its ilk.


-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] parse_frame_specification (stack.c)

[David Taylor]

    Date: Mon, 05 Mar 2001 12:26:55 -0500
    From: Fernando Nasser <fnasser@redhat.com>

    David,

    The real problem here is that there is an ambiguity in this command
    argument specification.  If a frame is specified as an address, it
    should be proceeded by a "*" as we do in the break command.

    It seems that problems like this have been encountered before.  here is
    the comment in the code that refers to s similar situation:

I saw that comment; and, by the way, it isn't a new comment -- it was
put into the code in Jan 1994 -- over 7 years ago.  I wasn't about to
change the specification for such an entrenched item without there
first being discussion on gdb about it.

Regardless of the outcome of the discussion (assuming there is one) on
whether to change the interface to the frame/info frame commands, I'd
like to get this bug fix committed.

	                                                            To solve
	       that, we need a new syntax for a specifying a frame by address.
	       I think the cleanest syntax is $frame(0x45) ($frame(0x23,0x45) for
	       two args, etc.), but people might think that is too much typing,
	       so I guess *0x23,0x45 would be a possible alternative (commas
	       really should be used instead of spaces to delimit; using spaces
	       normally works in an expression).  */

I don't object to the $frame syntax on grounds of too much typing, but
rather because of what I think it would do to the expression
evaluation code -- you'd have $frame(...) where it has the syntax of a
builtin variable $frame, but it isn't a variable, and it has the
syntax of a function call (...), but it isn't a function call...

Bleh!  Extreme bleh!

    Maybe we should start requiring the * for addresses and if not assuming
    it is a stack level (small integer as you say) and update the manual
    accordingly.

My initial reaction would be to favor this change (though I might
change my mind after I think about it more).

I am not going to implement it, though, unless there is consensus that
such a change would be a good thing.

Would you like to lead the discussion?

If I understand your proposal:

123       -- integer
*123      -- address

foo(123)  -- call function foo with argument 123, treat the result as
             an integer

*foo(123) -- call function foo with argument 123, treat the result as
             a pointer and convert it to an address

123,*456,789,*1011 -- integer,address,integer,address

    Fernando

David