[RFA/RFC] ARM : one-line prologue analysis

[RFA/RFC] ARM : one-line prologue analysis

[Jerome Guitton]

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


The support for unwinding through functions which do not build a frame
pointer seems broken for arm-elf. Consider the attached example. It is
basically main -> c -> b -> a (X '->' Y meaning "X calls Y"). the
calls to printf are here only to avoid sibcall optimization. With gcc
3.2, we will get a one-line prologue for a, b, c and main :


a:
        str     lr, [sp, #-4]!
	[...]

Alas, getting a backtrace from 'a' will not work properly:

(gdb) file d
Reading symbols from d...done.
(gdb) target sim
Connected to the simulator.
(gdb) load d
[...]
(gdb) b a
Breakpoint 1 at 0x8228: file a.c, line 5.
(gdb) r
Starting program: /cardiff.a/guitton/fsf/gdb-head-merge/tmp/test/d 

Breakpoint 1, a () at a.c:5
5         printf ("HERE I AM J.H.");
(gdb) bt
#0  a () at a.c:5
#1  0x00824000 in ?? ()

There is a 'address minus one' thing going on. Indeed, 0x00824000 is not a
valid address, it should be 0x00008240:

(gdb) x/i 0x00008240
0x8240 <b+8>:   ldr     r0, [pc, #8]    ; 0x8250 <b+24>

This problem is in arm_make_prologue_cache. Indeed, for testing if a
register has been saved on the stack, arm_make_prologue_cache should
compare the field addr of the saved reg table to -1, according to
trad-frame.h.  It compare it to 0! That's where the 'address minus -1'
thing comes from.

The first patch is a fix for this bug (obv.dif). It seems straight forward
to me. I run the gdb testsuite on a arm-elf simulator, it fixes 150 failures.
No regression.

Still, there is another bug:

(gdb) bt
#0  a () at a.c:5
#1  0x00008240 in b () at b.c:4
#2  0x00008240 in b () at b.c:4

For some reason that I don't understand completely (and that's the
[RFC] part of this email), in arm_scan_prologue the choice has been
made not to get lr from the stack if the prologue is "str lr, [sp,
#4]!". I just want to point out that getting it from the stack (my
second patch, lr.dif) fixes the problem:

(gdb) bt
#0  a () at a.c:5
#1  0x00008240 in b () at b.c:4
#2  0x0000825c in c () at c.c:4
#3  0x00008278 in main () at d.c:2

I don't thing that's a good idea to consider that these one-line-prologue
functions are frameless, even if they don't build a frame pointer.
Opinions/ideas?

Tested against the testsuite on the arm simulator, I get no regression and
no fix. (the baseline debugger includes my first patch)

-- 
Jerome

[-- Attachment #2: obv.dif --]
[-- Type: text/plain, Size: 924 bytes --]

2003-09-25  Jerome Guitton  <guitton@act-europe.fr>

	* arm-tdep.c (arm_make_prologue_cache): Compare the field addr of
	the saved regs record with -1 to test if the register has been saved
	on the stack.

*** arm-tdep.c.1	Tue Sep 23 20:58:12 2003
--- arm-tdep.c	Tue Sep 23 20:58:45 2003
*************** arm_make_prologue_cache (struct frame_in
*** 967,973 ****
    /* Calculate actual addresses of saved registers using offsets
       determined by arm_scan_prologue.  */
    for (reg = 0; reg < NUM_REGS; reg++)
!     if (cache->saved_regs[reg].addr != 0)
        cache->saved_regs[reg].addr += cache->prev_sp;
  
    return cache;
--- 967,973 ----
    /* Calculate actual addresses of saved registers using offsets
       determined by arm_scan_prologue.  */
    for (reg = 0; reg < NUM_REGS; reg++)
!     if (cache->saved_regs[reg].addr != -1)
        cache->saved_regs[reg].addr += cache->prev_sp;
  
    return cache;

[-- Attachment #3: lr.dif --]
[-- Type: text/plain, Size: 788 bytes --]

2003-09-25  Jerome Guitton  <guitton@act-europe.fr>

	* arm-tdep.c (arm_scan_prologue_cache): When analysing the instruction
	"str lr, [sp, #-4]", save the address where lr has been stored.

*** arm-tdep.c.2	Tue Sep 23 20:59:52 2003
--- arm-tdep.c	Tue Sep 23 21:06:07 2003
*************** arm_scan_prologue (struct frame_info *ne
*** 845,851 ****
  	}
        else if (insn == 0xe52de004)	/* str lr, [sp, #-4]! */
  	{
! 	  /* Function is frameless: extra_info defaults OK?  */
  	  continue;
  	}
        else if ((insn & 0xffff0000) == 0xe92d0000)
--- 845,852 ----
  	}
        else if (insn == 0xe52de004)	/* str lr, [sp, #-4]! */
  	{
! 	  sp_offset -= 4;
! 	  cache->saved_regs[ARM_LR_REGNUM].addr = sp_offset;
  	  continue;
  	}
        else if ((insn & 0xffff0000) == 0xe92d0000)

[-- Attachment #4: example.txt --]
[-- Type: text/plain, Size: 498 bytes --]


/* a.c */


extern void abort ();

void a () {
  printf ("HERE I AM J.H.");
}

/* b.c */

void b () {
  int i = 2;

  a ();
  printf ("END B %d \n", i);
}

/* c.c */

void c () {
  int i = 3;

  b ();
  printf ("END C %d \n", i);
}

/* d.c */

int main () {
  c ();
  printf ("END D\n");
}

#Makefile

CFLAGS=-O2 -save-temps -g
ARMCFLAGS=-mno-apcs-frame

.SUFFIXES: .c

.c.o:
	arm-elf-gcc -c $(CFLAGS) $(ARMCFLAGS) $<

d: a.o b.o c.o d.o
	arm-elf-gcc a.o b.o c.o d.o -o d

all: d

clean:
	-rm *.o

Re: [RFA/RFC] ARM : one-line prologue analysis

[Andrew Cagney]

>     for (reg = 0; reg < NUM_REGS; reg++)
> !     if (cache->saved_regs[reg].addr != -1)
>         cache->saved_regs[reg].addr += cache->prev_sp;

Would trad_frame_addr_p() be better?

Andrew

Re: [RFA/RFC] ARM : one-line prologue analysis

[Richard Earnshaw]

> >     for (reg = 0; reg < NUM_REGS; reg++)
> > !     if (cache->saved_regs[reg].addr != -1)
> >         cache->saved_regs[reg].addr += cache->prev_sp;
> 
> Would trad_frame_addr_p() be better?
> 
> Andrew
> 
> 

I think so.  However, arm_scan_prologue (which is run just before this 
loop) abuses the .addr field and puts an offset into it.  That offset can 
be (often is) negative at that point; the loop Jerome is fixing is the one 
where that offset is turned back into a real address.

Fortunately, the stack will always be word aligned (your application is 
really broken if it isn't), and registers are always at least four bytes 
in size, so I don't think it's possible for -1 to ever get written in 
there by the code itself.

So approved with Andrew's suggested change.

R.

PS Andrew,

We seem to be horribly overloading and then exposing these special values 
in the trad_frame interface.  Wouldn't it make more sense to add more 
routines to get and update the values, then we can hide the special 
meanings entirely (and maybe even change the way they are recorded if we 
need to).

R.

Re: [RFA/RFC] ARM : one-line prologue analysis

[Jerome Guitton]

Richard Earnshaw (rearnsha@arm.com):

> > Would trad_frame_addr_p() be better?

Agreed.

> So approved with Andrew's suggested change.

OK, I will check it in ASAP. Just to be sure that there is no
misunderstanding, this approval is for the first patch only, right?

-- 
Jerome

Re: [RFA/RFC] ARM : one-line prologue analysis

[Richard Earnshaw]

> Richard Earnshaw (rearnsha@arm.com):
> 
> > > Would trad_frame_addr_p() be better?
> 
> Agreed.
> 
> > So approved with Andrew's suggested change.
> 
> OK, I will check it in ASAP. Just to be sure that there is no
> misunderstanding, this approval is for the first patch only, right?

Actually, I think both patches are OK.

R.

Re: [RFA/RFC] ARM : one-line prologue analysis

[Jerome Guitton]

Richard Earnshaw (rearnsha@arm.com):

> > OK, I will check it in ASAP. Just to be sure that there is no
> > misunderstanding, this approval is for the first patch only, right?
> 
> Actually, I think both patches are OK.

There was a misunderstanding :-)
Thank you for your approval.

-- 
Jerome

Re: [RFA/RFC] ARM : one-line prologue analysis

[Andrew Cagney]

> PS Andrew,
> 
> We seem to be horribly overloading and then exposing these special values 
> in the trad_frame interface.  Wouldn't it make more sense to add more 
> routines to get and update the values, then we can hide the special 
> meanings entirely (and maybe even change the way they are recorded if we 
> need to).

Yes.  The last iteration of that added frame_p.  Can you leave a 
dropping somewhere (either a comment in trad_frame*) or a bug report 
pointing this out.

Andrew

[commit] ARM : one-line prologue analysis

[Jerome Guitton]

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

I merged the two patches, and committed them.

-- 
Jerome

[-- Attachment #2: arm.dif --]
[-- Type: text/plain, Size: 1727 bytes --]

2003-09-29  Jerome Guitton  <guitton@act-europe.fr>

	* arm-tdep.c (arm_make_prologue_cache): Use trad_frame_addr_p to
	test if the register has been saved on the stack.
	(arm_scan_prologue_cache): When analysing the instruction
	"str lr, [sp, #-4]", save the address where lr has been stored.

Index: arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.151
diff -c -p -r1.151 arm-tdep.c
*** arm-tdep.c	25 Sep 2003 14:21:00 -0000	1.151
--- arm-tdep.c	29 Sep 2003 13:11:51 -0000
*************** arm_scan_prologue (struct frame_info *ne
*** 845,851 ****
  	}
        else if (insn == 0xe52de004)	/* str lr, [sp, #-4]! */
  	{
! 	  /* Function is frameless: extra_info defaults OK?  */
  	  continue;
  	}
        else if ((insn & 0xffff0000) == 0xe92d0000)
--- 845,852 ----
  	}
        else if (insn == 0xe52de004)	/* str lr, [sp, #-4]! */
  	{
! 	  sp_offset -= 4;
! 	  cache->saved_regs[ARM_LR_REGNUM].addr = sp_offset;
  	  continue;
  	}
        else if ((insn & 0xffff0000) == 0xe92d0000)
*************** arm_make_prologue_cache (struct frame_in
*** 967,973 ****
    /* Calculate actual addresses of saved registers using offsets
       determined by arm_scan_prologue.  */
    for (reg = 0; reg < NUM_REGS; reg++)
!     if (cache->saved_regs[reg].addr != 0)
        cache->saved_regs[reg].addr += cache->prev_sp;
  
    return cache;
--- 968,974 ----
    /* Calculate actual addresses of saved registers using offsets
       determined by arm_scan_prologue.  */
    for (reg = 0; reg < NUM_REGS; reg++)
!     if (trad_frame_addr_p (cache->saved_regs, reg))
        cache->saved_regs[reg].addr += cache->prev_sp;
  
    return cache;