[PATCH/RFA] Fix busted logic in find_saved_register()

[PATCH/RFA] Fix busted logic in find_saved_register()

[Jason R Thorpe]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

find_saved_register() is used by mips_get_saved_register() and
the alpha_get_saved_register() in my (updated) multi-arch changes
for the Alpha target.

While investigating some testsuite failures, it appeared that
there is no way thjat find_saved_register() could possibly work
on either MIPS or Alpha, since the first thing it does on either
of those platforms is dereference a NULL pointer (said pointer is
initlaized to NULL at the top of the function).

I believe the following patch makes find_saved_register() actually
implement the logic it claims to.  It certainly fixes the problem
I had with GDB dumping core, and fixes the relevant testsuite failures.

OK to commit?

	* frame.c (find_saved_register): Avoid a NULL pointer
	dereference and actually walk the frame list.

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

[-- Attachment #2: frame-patch --]
[-- Type: text/plain, Size: 798 bytes --]

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.7
diff -c -r1.7 frame.c
*** frame.c	17 Apr 2002 21:55:12 -0000	1.7
--- frame.c	20 Apr 2002 21:30:25 -0000
***************
*** 83,91 ****
    while (1)
      {
        QUIT;
!       frame1 = get_next_frame (frame1);
!       if (frame1 == 0 || frame1 == frame)
  	break;
        FRAME_INIT_SAVED_REGS (frame1);
        if (frame1->saved_regs[regnum])
  	addr = frame1->saved_regs[regnum];
--- 83,92 ----
    while (1)
      {
        QUIT;
!       frame1 = get_next_frame (frame);
!       if (frame1 == 0)
  	break;
+       frame = frame1;
        FRAME_INIT_SAVED_REGS (frame1);
        if (frame1->saved_regs[regnum])
  	addr = frame1->saved_regs[regnum];

Re: [PATCH/RFA] Fix busted logic in find_saved_register()

[Andrew Cagney]

> find_saved_register() is used by mips_get_saved_register() and
> the alpha_get_saved_register() in my (updated) multi-arch changes
> for the Alpha target.
> 
> While investigating some testsuite failures, it appeared that
> there is no way thjat find_saved_register() could possibly work
> on either MIPS or Alpha, since the first thing it does on either
> of those platforms is dereference a NULL pointer (said pointer is
> initlaized to NULL at the top of the function).
> 
> I believe the following patch makes find_saved_register() actually
> implement the logic it claims to.  It certainly fixes the problem
> I had with GDB dumping core, and fixes the relevant testsuite failures.
> 
> OK to commit?
> 
> 	* frame.c (find_saved_register): Avoid a NULL pointer
> 	dereference and actually walk the frame list.

Yes.  Looks like my:

2002-04-17  Andrew Cagney  <ac131313@redhat.com>

* frame.c (find_saved_register): Find saved registers in the next
not prev frame.
Fix PR gdb/365.

flushed out another problem.

You mention that the alpha is calling that function.  Is that directly 
or indirectly?  I'm going to eliminate the MIPS direct call.

Andrew

Re: [PATCH/RFA] Fix busted logic in find_saved_register()

[Jason R Thorpe]

On Sat, Apr 20, 2002 at 07:08:03PM -0400, Andrew Cagney wrote:

 > Yes.  Looks like my:
 > 
 > 2002-04-17  Andrew Cagney  <ac131313@redhat.com>
 > 
 > * frame.c (find_saved_register): Find saved registers in the next
 > not prev frame.
 > Fix PR gdb/365.
 > 
 > flushed out another problem.

Ok, I will commit the fix.

 > You mention that the alpha is calling that function.  Is that directly 
 > or indirectly?  I'm going to eliminate the MIPS direct call.

Directly.  The alpha_get_saved_register() is nearly identical to the
mips_get_saved_register().

How are you changing the MIPS target?

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

Re: [PATCH/RFA] Fix busted logic in find_saved_register()

[Andrew Cagney]

> Ok, I will commit the fix.
> 
>  > You mention that the alpha is calling that function.  Is that directly 
>  > or indirectly?  I'm going to eliminate the MIPS direct call.
> 
> Directly.  The alpha_get_saved_register() is nearly identical to the
> mips_get_saved_register().

If you've implemented INIT_SAVED_REGS() do you need a custom 
get_saved_register()?

> How are you changing the MIPS target?

Things will affect the MIPS at two levels.

Ref:
Andrew Cagney - [rfc] Frame based register cache / frame->unwind
http://sources.redhat.com/ml/gdb/2002-04/msg00245.html

I'm looking to change the way frames are unwound.  I'd ignore that patch 
as posted - I've managed to greatly simplify things.  Main thing to note 
that the new code doesn't use find_saved_register().  Instead it uses a 
recursive frame->register_unwind() method.

If/when this change goes through, any code using the existing 
find_saved_register() would be handed a local copy.

--

At a more theoretical [sp] level, I'm looking to rewrite the entire 
function.  Much of the MIPS complexity comes about because its REGNUM's 
are overloaded.  For instance, the hardware fp0 register might be 64 
bits but when a program (via debug info) refers to it, it is only 32 bits.

I think the way to handle this is to map debug info REGNUMs onto the 
pseudo address space and then use register_{read,write} to map them onto 
the corresponding hardware register.

This one is more theoretical as the SH5 is the first port to try to get 
this working and (apparently) it flushed out a few teething problems.

enjoy,
Andrew

Re: [PATCH/RFA] Fix busted logic in find_saved_register()

[Jason R Thorpe]

On Sat, Apr 20, 2002 at 08:00:06PM -0400, Andrew Cagney wrote:

 > > Directly.  The alpha_get_saved_register() is nearly identical to the
 > > mips_get_saved_register().
 > 
 > If you've implemented INIT_SAVED_REGS() do you need a custom 
 > get_saved_register()?

Ok, this is where my understanding of the call_dummy stuff starts to get
muddled :-)

The Alpha target doesn't use the generic call_dummy frame stuff, but
rather its own.  I suspect it could be converted to use the generic
stuff, but an explanation of how it's supposed to work would really help :-)

Would it be terribly evil to commit the alpha multi-arch stuff that I
have now (which would meet your condition for committing my alpha-netbsd
configuration :-), and then take a stab at converting it to generic dummy
frames?

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

Re: [PATCH/RFA] Fix busted logic in find_saved_register()

[Andrew Cagney]

> On Sat, Apr 20, 2002 at 08:00:06PM -0400, Andrew Cagney wrote:
> 
>  > > Directly.  The alpha_get_saved_register() is nearly identical to the
>  > > mips_get_saved_register().
>  > > If you've implemented INIT_SAVED_REGS() do you need a custom 
>  > get_saved_register()?
> 
> Ok, this is where my understanding of the call_dummy stuff starts to get
> muddled :-)
> 
> The Alpha target doesn't use the generic call_dummy frame stuff, but
> rather its own.  I suspect it could be converted to use the generic
> stuff, but an explanation of how it's supposed to work would really help :-)

Traditional dummy frames were implemented by creating a fake frame on 
the stack that contained all the registers.

Generic dummy frames instead, don't bother.  They create a fake, local 
to gdb, frame that contains the saved registers, and then calls the 
inferior function directly.  On the stack they appear like a special 
frameless function.  On paper, getting them to work is fairly easy.  See 
xstormy16-elf, cris-elf, or any new/obscure target.  Suggest grepping 
for the word ``dummy'' and copying the corresponding settings.

> Would it be terribly evil to commit the alpha multi-arch stuff that I
> have now (which would meet your condition for committing my alpha-netbsd
> configuration :-), and then take a stab at converting it to generic dummy
> frames?

Getting it multi-arched has a higher priority.

Andrew