CRIS port; frame cleanup crash

CRIS port; frame cleanup crash

[Orjan Friberg]

After a long overdue update of my gdb cvs tree, I found that something 
broke late March/early April.  I don't quite understand what goes on, 
but it seems to happen the first time a frame allocated by 
deprecated_frame_xmalloc_with_cleanup is freed by do_cleanups (which 
happens in cris_skip_prologue_main).  gdb segfaults on a call to free 
with a pointer to that frame.  The arm-tdep.c file contains the same 
construct of:

   old_chain = make_cleanup (null_cleanup, NULL);
   frame = deprecated_frame_xmalloc_with_cleanup (..., ...)
   <do something with frame>
   do_cleanups (old_chain);

The only thing I found that looked suspicous was that the frame variable 
is allocated by a call to obstack_alloc, but free'd with a "normal" call 
to free.  I would have guessed it should be with obstack_free, but then 
again, my understanding of what happens is limited so far.

Any pointers or suggestions are appreciated.

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Orjan Friberg wrote:
> After a long overdue update of my gdb cvs tree, I found that something 
> broke late March/early April.  I don't quite understand what goes on, 
> but it seems to happen the first time a frame allocated by 
> deprecated_frame_xmalloc_with_cleanup is freed by do_cleanups (which 
> happens in cris_skip_prologue_main).  gdb segfaults on a call to free 
> with a pointer to that frame.

I hacked around that particular failure by not allocating a frame, and 
simply relying on symbol information.  With that out of the way, I'm 
able to run the testsuite, albeit with some more FAILs than stock a gdb 5.3.

However, poking around in all the stuff that has been deprecated, I'm 
sort of at a loss as to where to start.  Replacing the deprecated 
function/macros one at a time doesn't look feasible, since a lot of 
functionality is replaced by the same functions 
(push_dummy_code/push_dummy_call for example).  Is there a preferred way 
of doing it, or is it just a matter of diving right into it?  I'd prefer 
to do it in a structured way to be able to catch when things start to 
break.  I see there are a couple of targets that have made the 
transition (d10v and m68hc11 for example), so that should provide some help.

Also, is there any documentation for the frame handling machinery that I 
didn't find?  I checked the archives, and read a bunch of Andrew 
Cagney's posts related to this, but couldn't really get the big picture 
from that.  (The Internals manual has only a brief mention of frames in 
section 3.1.)

Thanks for any help.

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Andrew Cagney]

> After a long overdue update of my gdb cvs tree, I found that something broke late March/early April.  I don't quite understand what goes on, but it seems to happen the first time a frame allocated by deprecated_frame_xmalloc_with_cleanup is freed by do_cleanups (which happens in cris_skip_prologue_main).  gdb segfaults on a call to free with a pointer to that frame.  The arm-tdep.c file contains the same construct of:
> 
>   old_chain = make_cleanup (null_cleanup, NULL);
>   frame = deprecated_frame_xmalloc_with_cleanup (..., ...)
>   <do something with frame>
>   do_cleanups (old_chain);

Er, that points at a bug:

struct frame_info *
deprecated_frame_xmalloc (void)
{
   struct frame_info *frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
   frame->this_id.p = 1;
   return frame;
}

shouldn't be allocated from the frame obstack pool.  I'll commit a fix.

> However, poking around in all the stuff that has been deprecated, I'm sort of at a loss as to where to start.  Replacing the deprecated function/macros one at a time doesn't look feasible, since a lot of functionality is replaced by the same functions (push_dummy_code/push_dummy_call for example).  Is there a preferred way of doing it, or is it just a matter of diving right into it?  I'd prefer to do it in a structured way to be able to catch when things start to break.  I see there are a couple of targets that have made the transition (d10v and m68hc11 for example), so that should provide some help.

ia64, Arm, ... Some people are diving straight in, some are atacking the 
edges - updating any deprecated methods first.

Andrew

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Andrew Cagney wrote:
> 
> Er, that points at a bug:
> 
> struct frame_info *
> deprecated_frame_xmalloc (void)
> {
>   struct frame_info *frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
>   frame->this_id.p = 1;
>   return frame;
> }
> 
> shouldn't be allocated from the frame obstack pool.  I'll commit a fix.

Thanks a lot; it works fine now.

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Ok, take two on trying to update the CRIS port to the new frame handling 
mechanism.  I'm planning to hook in the DWARF CFI frame unwinder, but I 
don't know if that affects any of the other stuff I'm going to have to 
do.  (I'm going to have to update to the new dummy stuff later, but I 
was hoping I could do that separately.)

Thanks in advance for any answers to questions, or comments on what I've 
understood or misunderstood amongst all of this (or even pointers to 
information I might have missed).

First of all, what does "unwind" mean in the frame context?  I know it 
sounds silly, but I've been trying unsuccessfully to wrap my head around 
that word.  Is there some fundamental thing I may have missed concerning 
the new frame handling, or can I just replace "unwind" with "dig out" in 
my head?

About the struct unwind_cache: looking at the other architectures, I'm 
still not sure what I need in this struct.  (I'm guessing stuff from the 
old struct frame_extra_info should go here if still needed.)  Is there a 
recommended starting point for this struct?  Some of the common members 
between architectures' unwind caches seem to be:

prev_sp: Most comments say "The previous frame's inner most stack 
address.  Used as this frame ID's stack_addr."  So would that be the 
what the stack pointer was when the current frame was entered?

base: Is this "base" as in "base for local variables" or does it refer 
to something else?  Most architectures seem to set this to the frame's 
frame pointer.

sp_offset and size I think I understand: how much the stack pointer has 
been changed so far in the frame, and how much stack space was allocated 
in this frame (absolute value of sp_offset) so far.


Thanks,
Orjan

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Andrew Cagney]

> Ok, take two on trying to update the CRIS port to the new frame handling mechanism.  I'm planning to hook in the DWARF CFI frame unwinder, but I don't know if that affects any of the other stuff I'm going to have to do.  (I'm going to have to update to the new dummy stuff later, but I was hoping I could do that separately.)

CRIS is already using generic dummy frames so it should be in good 
shape.  The only potential got-ya is with unwind_dummy_id - you'll need 
to check that the correct ID is returned.

> Thanks in advance for any answers to questions, or comments on what I've understood or misunderstood amongst all of this (or even pointers to information I might have missed).
> 
> First of all, what does "unwind" mean in the frame context?  I know it sounds silly, but I've been trying unsuccessfully to wrap my head around that word.  Is there some fundamental thing I may have missed concerning the new frame handling, or can I just replace "unwind" with "dig out" in my head?

The term "unwind" is used by the dwarf-2 specification. 
http://www.eagercon.com/dwarf/dwarf3std.htm
it includes a working example in the appendix.

The important thing is to "dig out" the register from the correct frame. 
  frame_unwind_register (next_frame, "pc") will "dig out" the PC from 
the next frame (often found in next frame's link register) returning the 
value as it should be in "this_frame".

> About the struct unwind_cache: looking at the other architectures, I'm still not sure what I need in this struct.  (I'm guessing stuff from the old struct frame_extra_info should go here if still needed.)  Is there a recommended starting point for this struct?  Some of the common members between architectures' unwind caches seem to be:

Yes, frame_extra_info is a very good starting point.  "init_saved_regs" 
is also a very a good starting point for the new cache init code.

> prev_sp: Most comments say "The previous frame's inner most stack address.  Used as this frame ID's stack_addr."  So would that be the what the stack pointer was when the current frame was entered?

Probably :-(

The i386 call instruction adjusts the SP leading to:
   /* Now that we have the base address for the stack frame we can
      calculate the value of %esp in the calling frame.  */
   cache->saved_sp = cache->base + 8;

> base: Is this "base" as in "base for local variables" or does it refer to something else?  Most architectures seem to set this to the frame's frame pointer.

Thats correct.

> sp_offset and size I think I understand: how much the stack pointer has been changed so far in the frame, and how much stack space was allocated in this frame (absolute value of sp_offset) so far.

that's correct

Also:
    struct trad_frame_saved_reg *saved_regs;
(from mips et.al.) which can help simplify the implementation of 
register unwind.

Andrew

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Andrew Cagney wrote:
> 
> The term "unwind" is used by the dwarf-2 specification. 
> http://www.eagercon.com/dwarf/dwarf3std.htm
> it includes a working example in the appendix.

Excellent, thanks a lot.  Section 6.4 regarding Call Frame Information 
cleared up some of the confusion regarding unwinding.

> The important thing is to "dig out" the register from the correct frame. 
>  frame_unwind_register (next_frame, "pc") will "dig out" the PC from the 
> next frame (often found in next frame's link register) returning the 
> value as it should be in "this_frame".

This explanation has me slightly puzzled.  I gather from the code that 
"PC" refers to the current program location within a frame, and not an 
actual CPU register.  Is "next" synonymous with "outer" in your 
explanation above?  (Perhaps a stupid question; maybe unwind by 
definition works on the outer frame/caller.)  And "link register" I 
assume is the equivalent of a subroutine return pointer.

Approaching it from another direction, what would be a good test to see 
if the unwind code works correctly?  At present, step (into function 
calls), next (over function calls), finish, and backtrace seem to work 
ok for both leaf- and non-leaf-functions (for CRIS the prologue differs 
slightly as the SRP isn't pushed in case of a leaf function).  Is there 
any particular existing testcase that would be good at detecting errors 
in the frame code?

Another question: should I hook in the dwarf-2 frame sniffer from the 
very beginning?  Or wait until the other stuff seems to be working ok?

Thanks,
Orjan

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Andrew Cagney]

[Joel, MichaelC, FYI]

> Andrew Cagney wrote:
> 
>> The term "unwind" is used by the dwarf-2 specification. http://www.eagercon.com/dwarf/dwarf3std.htm
>> it includes a working example in the appendix.

> 
> Excellent, thanks a lot.  Section 6.4 regarding Call Frame Information cleared up some of the confusion regarding unwinding.



>> The important thing is to "dig out" the register from the correct frame.  frame_unwind_register (next_frame, "pc") will "dig out" the PC from the next frame (often found in next frame's link register) returning the value as it should be in "this_frame".

> 
> This explanation has me slightly puzzled.  I gather from the code that "PC" refers to the current program location within a frame, and not an actual CPU register.  Is "next" synonymous with "outer" in your explanation above?  (Perhaps a stupid question; maybe unwind by definition works on the outer frame/caller.)  And "link register" I assume is the equivalent of a subroutine return pointer.

Yes, "pc" is a misnomer, within GDB it doesn't mean the "pc register", 
"resume address" is closer.  Check the comments in frame.c:struct 
frame_info for definitions of: "next", "inner", "newer" and separatly 
"prev", "outer", "older".  Yes, link register is the return address 
register.

This frame (the caller) calls next frame (the callee).  As part of that 
the callers resume addresses ends up in the callee's return-address 
register.  The callee (next) then saves the return-address register on 
the stack.  Hence to get the callers (this) PC, it needs to be dug out 
of the callers (next) frame's stack.  Anyway, sounds like you've got it 
right.

> Approaching it from another direction, what would be a good test to see if the unwind code works correctly?  At present, step (into function calls), next (over function calls), finish, and backtrace seem to work ok for both leaf- and non-leaf-functions (for CRIS the prologue differs slightly as the SRP isn't pushed in case of a leaf function).  Is there any particular existing testcase that would be good at detecting errors in the frame code?

Exactly that.

Also "advance.exp", in particular the sequence:

./gdb advance
(gdb) break func
(gdb) run
(gdb) advance func3
--- should break in main and not func3

is a good check of the frame ID.

And callfuncs.exp, and a sequence like:

./gdb callfuncs
(gdb) break add
(gdb) break main
(gdb) run
(gdb) print add(1,2)
(gdb) bt
(gdb) print add(3,4)
(gdb) bt
add(3,4)
<dummy frame>
add (1,2)
<dummy frame>
main ()
(gdb)

is a good check of dummy frames

> Another question: should I hook in the dwarf-2 frame sniffer from the very beginning?  Or wait until the other stuff seems to be working ok?

Towards the end.  It's good to first get the old code working reasonably 
well.

Andrew

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Andrew Cagney wrote:
> 
> This frame (the caller) calls next frame (the callee).  As part of that 
> the callers resume addresses ends up in the callee's return-address 
> register.  The callee (next) then saves the return-address register on 
> the stack.  Hence to get the callers (this) PC, it needs to be dug out 
> of the callers (next) frame's stack.  Anyway, sounds like you've got it 
> right.

Thanks a lot for the clarification.  I think I'm less confused now.

> Also "advance.exp", in particular the sequence:
> 
> ./gdb advance
> (gdb) break func
> (gdb) run
> (gdb) advance func3
> --- should break in main and not func3
> 
> is a good check of the frame ID.

Thanks for the tip.  Running advance.exp results in all PASS.  (When 
issuing the command sequence above manually on the CRIS target and 
comparing with my Linux host, I end up at different locations in main 
(at marker1() and func (c), respectively), but advance.exp seems to 
indicate that either is ok.)  So, at least it's not completely broken.

I'll get to callfuncs.exp next for the dummy frames implementation.

Thanks,
Orjan

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Andrew Cagney wrote:
> 
> And callfuncs.exp, and a sequence like:
> 
> ./gdb callfuncs
> (gdb) break add
> (gdb) break main
> (gdb) run
> (gdb) print add(1,2)
> (gdb) bt
> (gdb) print add(3,4)
> (gdb) bt
> add(3,4)
> <dummy frame>
> add (1,2)
> <dummy frame>
> main ()
> (gdb)
> 
> is a good check of dummy frames

Ok, backtrace doesn't work at all.  (find_dummy_frame() isn't able to 
locate the dummy frame because fp doesn't match dummyframe->top.)  A 
couple of questions regarding this:

frame_align(): this function is only concerned with architecture issues, 
and not ABI issues, right?  What I mean is that we're not mimicking 
anything the compiler would do the way we do when we set up arguments 
for a function call.  If the architecture has no alignment restrictions 
on the stack, then we shouldn't have to do any stack alignment, although 
the compiler might align it for performance reasons.

push_dummy_call(): the CRIS code allocates more space on the stack than 
is actually needed; is this going to cause me problems?  Since, as I 
understand it, the SP returned from push_dummy_call() must match the SP 
unwound when unwinding the dummy id I'm thinking I'll either have to get 
rid of the over-allocation on the stack (preferred) or compensate for it 
when unwinding the dummy id.  Is this correct?

FWIW, everything up to and including the cmp10(...) test in 
callfuncs.exp works, so at least the argument setup seems ok.

Thanks,
Orjan

-- 
Orjan Friberg
Axis Communications

Re: CRIS port; frame cleanup crash

[Andrew Cagney]

> Andrew Cagney wrote:
> 
> And callfuncs.exp, and a sequence like:
> 
> ./gdb callfuncs
> (gdb) break add
> (gdb) break main
> (gdb) run
> (gdb) print add(1,2)
> (gdb) bt
> (gdb) print add(3,4)
> (gdb) bt
> add(3,4)
> <dummy frame>
> add (1,2)
> <dummy frame>
> main ()
> (gdb)
> 
> is a good check of dummy frames
> 
> Ok, backtrace doesn't work at all.  (find_dummy_frame() isn't able to locate the dummy frame because fp doesn't match dummyframe->top.)  A couple of questions regarding this:

Ok, you've tripped over a bit of screwed up GDB history.  'till the 
frame rewrite the choice of dummyframe->top was very arbitrary cf:

           /* An older target that hasn't explicitly or implicitly
              saved the dummy frame's top-of-stack.  Try matching the
              FP against the saved SP and FP.  NOTE: If you're trying
              to fix a problem with GDB not correctly finding a dummy
              frame, check the comments that go with FRAME_ALIGN() and
              UNWIND_DUMMY_ID().  */

> frame_align(): this function is only concerned with architecture issues, and not ABI issues, right?  What I mean is that we're not mimicking anything the compiler would do the way we do when we set up arguments for a function call.  If the architecture has no alignment restrictions on the stack, then we shouldn't have to do any stack alignment, although the compiler might align it for performance reasons.

It's more an ABI issue.

Frame alighment is oftem more strict than ISA alignment.  For instance, 
a 32-bit machine may require 4-byte alignment, but the ABI might specify 
that a stack must be 8 or even 16-byte aligned when entering a function.

> push_dummy_call(): the CRIS code allocates more space on the stack than is actually needed; is this going to cause me problems?  Since, as I understand it, the SP returned from push_dummy_call() must match the SP unwound when unwinding the dummy id I'm thinking I'll either have to get rid of the over-allocation on the stack (preferred) or compensate for it when unwinding the dummy id.  Is this correct?

That is correct.

       /* Sanity.  The exact same SP value is returned by
          PUSH_DUMMY_CALL, saved as the dummy-frame TOS, and used by
          unwind_dummy_id to form the frame ID's stack address.  */

As for which of the two choices is prefered, which ever makes your life 
easier.

> FWIW, everything up to and including the cmp10(...) test in callfuncs.exp works, so at least the argument setup seems ok.

ya!

Andrew

Re: CRIS port; frame cleanup crash

[Orjan Friberg]

Andrew Cagney wrote:
>> FWIW, everything up to and including the cmp10(...) test in 
>> callfuncs.exp works, so at least the argument setup seems ok.
> 
> 
> ya!

... and it gets even better!  Running advance.exp, callfuncs.exp, 
finish.exp, funcargs.exp, restore.exp, scope.exp, and store.exp now 
results in:

# of expected passes            731
# of unsupported tests          1

structs.exp still gives me 30+ FAILs, but OTOH I haven't implemented a 
return_value yet (still using store/extract_return_value and 
use_struct_convention).

There's still some cleaning up to do before I submit a patch, but it 
does looks promising.  Andrew, thanks for all your help.

-- 
Orjan Friberg
Axis Communications