[RFA/mips(commit?)] Unwinding from noreturn function

[RFA/mips(commit?)] Unwinding from noreturn function

[Joel Brobecker]

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

Hello,

I'm starting to work on bringing mips-irix back to life. One of the issues
I've noticed is that unwinding is generally broken, particularly from
noreturn functions. I normally have commit priviledges for mips, but
in this case I wouldn't mind an extra opinion on the first issue I found.
Since I don't have access to a mips16 machine, I can't test the change
there...

Consider the following example:

        procedure A is
        begin
           raise Constraint_Error;
        end A;

Compile it with the following command:

        % gnatmake -g a

Then try the following:

        % gdb a
        (gdb) start
        (gdb) b __gnat_debug_raise_exception 
        Breakpoint 2 at 0x9947160: file s-except.adb, line 44.
        (gdb) cont
        Continuing.
        
        Breakpoint 2, <__gnat_debug_raise_exception> (e=0x9a33168)
        [...]
        (gdb) bt
        #0  <__gnat_debug_raise_exception> (e=0x9a33168) at s-except.adb:44
        #1  0x097b82e8 in <__gnat_raise_nodefer_with_msg> (e=0x9a33168)
            at a-except.adb:830
        #2  0x097b82e8 in <__gnat_raise_nodefer_with_msg> (e=0x9a33168)
            at a-except.adb:830
        Backtrace stopped: previous frame identical to this frame (corrupt stack?)

I actually found two issues:

  1. One major: mips_pc_is_mips16 that returns non-zero if bit 0
     of the address it is given is set.

        /* If bit 0 of the address is set, assume this is a MIPS16 address. */
        if (is_mips16_addr (memaddr))
          return 1;

     However, this doesn't work very well in our case, especially
     in this situation:

        static const struct frame_unwind *
        mips_insn16_frame_sniffer (struct frame_info *next_frame)
        {
          CORE_ADDR pc = frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
          if (mips_pc_is_mips16 (pc))
            return &mips_insn16_frame_unwind;
          return NULL;
        }

     In this case, frame_unwind_address_in_block will return the
     frame return address *minus one*, thus accidently triggering
     the check above.

     As a consequence, we ended up using the mips16 unwinder instead
     of the mips32 one.

     It seems to me that the above check is only an optimization,
     and I've spotted at least one instance where I cannot see an
     obvious guaranty that the address has not been decremented
     by one of the _in_block functions... So the decision I made
     was to remove that check.

  2. One minor: There was a confusion in the unwinder between
     the return address and the address of the instruction calling us.
     So I replaced frame_pc_unwind calls by their associated
     frame_unwind_address_in_block.

With the two fixes above, I managed to get the full backtrace:

        (gdb) bt
        #0  <__gnat_debug_raise_exception> (e=0x9a33168) at s-except.adb:44
        #1  0x097b82e8 in <__gnat_raise_nodefer_with_msg> (e=0x9a33168)
            at a-except.adb:830
        #2  0x097b8990 in ada.exceptions.raise_with_location_and_msg (
            e=0x9a33168, f=(system.address) 0xf, l=1717986919, 
            m=(system.address) 0x31) at a-except.adb:995
        #3  0x097b82a0 in <__gnat_raise_constraint_error_msg> (
            file=(system.address) 0x9a33168, line=161761434, 
            msg=(system.address) 0x31) at a-except.adb:795
        #4  0x097b8b58 in <__gnat_rcheck_04> (
            file=(system.address) 0x9a3b240, line=15) at a-except.adb:1043
        #5  0x100027f0 in a () at a.adb:3

2007-03-07  Joel Brobecker  <brobecker@adacore.com>

        * mips-tdep.c (mips_pc_is_mips16): Remove check for bit zero of
        the given address.
        (mips_insn16_frame_cache): Use the proper function to get the
        address of the caller instruction.
        (mips_insn32_frame_cache): Likewise.

Tested on mips-irix. Fixes many many many tests (about 500 or so).
OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: mips16.diff --]
[-- Type: text/plain, Size: 1382 bytes --]

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.404
diff -u -p -r1.404 mips-tdep.c
--- mips-tdep.c	27 Feb 2007 20:17:19 -0000	1.404
+++ mips-tdep.c	7 Mar 2007 04:02:06 -0000
@@ -808,10 +808,6 @@ mips_pc_is_mips16 (CORE_ADDR memaddr)
 {
   struct minimal_symbol *sym;
 
-  /* If bit 0 of the address is set, assume this is a MIPS16 address. */
-  if (is_mips16_addr (memaddr))
-    return 1;
-
   /* A flag indicating that this is a MIPS16 function is stored by elfread.c in
      the high bit of the info field.  Use this to decide if the function is
      MIPS16 or normal MIPS.  */
@@ -1640,7 +1636,8 @@ mips_insn16_frame_cache (struct frame_in
 
   /* Analyze the function prologue.  */
   {
-    const CORE_ADDR pc = frame_pc_unwind (next_frame);
+    const CORE_ADDR pc =
+      frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
     CORE_ADDR start_addr;
 
     find_pc_partial_function (pc, NULL, &start_addr, NULL);
@@ -1961,7 +1958,8 @@ mips_insn32_frame_cache (struct frame_in
 
   /* Analyze the function prologue.  */
   {
-    const CORE_ADDR pc = frame_pc_unwind (next_frame);
+    const CORE_ADDR pc =
+      frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
     CORE_ADDR start_addr;
 
     find_pc_partial_function (pc, NULL, &start_addr, NULL);

Re: [RFA/mips(commit?)] Unwinding from noreturn function

[Daniel Jacobowitz]

On Tue, Mar 06, 2007 at 08:16:43PM -0800, Joel Brobecker wrote:
>      However, this doesn't work very well in our case, especially
>      in this situation:
> 
>         static const struct frame_unwind *
>         mips_insn16_frame_sniffer (struct frame_info *next_frame)
>         {
>           CORE_ADDR pc = frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
>           if (mips_pc_is_mips16 (pc))
>             return &mips_insn16_frame_unwind;
>           return NULL;
>         }

I Am Dumb.  Check CVS history, but I think I changed that just a
couple of weeks ago; I audited all the sniffers looking for what ought
to use the unwound PC and what ought to use the unwound block address.
Here, I'm pretty sure I made the wrong choice.

I would recommend you revert my changes to this function and
mips_insn32_frame_sniffer instead.

>      It seems to me that the above check is only an optimization,
>      and I've spotted at least one instance where I cannot see an
>      obvious guaranty that the address has not been decremented
>      by one of the _in_block functions... So the decision I made
>      was to remove that check.

No, it's not just an optimization.  Especially with limited debug
info, it's important.

> 
>   2. One minor: There was a confusion in the unwinder between
>      the return address and the address of the instruction calling us.
>      So I replaced frame_pc_unwind calls by their associated
>      frame_unwind_address_in_block.

This half looks right.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/mips(commit?)] Unwinding from noreturn function

[Joel Brobecker]

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

> I Am Dumb.  Check CVS history, but I think I changed that just a
> couple of weeks ago; I audited all the sniffers looking for what ought
> to use the unwound PC and what ought to use the unwound block address.
> Here, I'm pretty sure I made the wrong choice.

Yep, I remembered. However, I also thought that your choice made sense.
I really think it does, but given:

> I would recommend you revert my changes to this function and
> mips_insn32_frame_sniffer instead.

And:

> >      It seems to me that the above check is only an optimization,
> >      and I've spotted at least one instance where I cannot see an
> >      obvious guaranty that the address has not been decremented
> >      by one of the _in_block functions... So the decision I made
> >      was to remove that check.
> 
> No, it's not just an optimization.  Especially with limited debug
> info, it's important.
        ^^^^^^^^^^^^^^
I decided to revert your changes for now.

> >   2. One minor: There was a confusion in the unwinder between
> >      the return address and the address of the instruction calling us.
> >      So I replaced frame_pc_unwind calls by their associated
> >      frame_unwind_address_in_block.
> 
> This half looks right.

Thanks.

So here is what I ended up checkin in:

2007-03-07  Joel Brobecker  <brobecker@adacore.com>

        * mips-tdep.c (mips_insn16_frame_cache, mips_insn32_frame_sniffer):
        Revert the previous change that had some unexpected side-effects
        on mips32.
        (mips_insn16_frame_cache, mips_insn32_frame_cache): Use the proper
        function to get the address of the calling instruction.

Re-tested on mips-irix, just to be sure. Same results as before
(meaning about 500 less FAILs).

I'm also sad to report that I have been told to put off work on
mips-irix for a while. That was a personal initiative on my side,
but I'm lacking time at work, so this was the first task that got cut.
I hope someone else can find the time to bring it back to life...

Thanks again, Daniel.
-- 
Joel

[-- Attachment #2: mips-tdep.c.diff --]
[-- Type: text/plain, Size: 1716 bytes --]

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.404
diff -u -p -r1.404 mips-tdep.c
--- mips-tdep.c	27 Feb 2007 20:17:19 -0000	1.404
+++ mips-tdep.c	7 Mar 2007 21:26:32 -0000
@@ -1640,7 +1640,8 @@ mips_insn16_frame_cache (struct frame_in
 
   /* Analyze the function prologue.  */
   {
-    const CORE_ADDR pc = frame_pc_unwind (next_frame);
+    const CORE_ADDR pc =
+      frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
     CORE_ADDR start_addr;
 
     find_pc_partial_function (pc, NULL, &start_addr, NULL);
@@ -1693,7 +1694,7 @@ static const struct frame_unwind mips_in
 static const struct frame_unwind *
 mips_insn16_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
+  CORE_ADDR pc = frame_pc_unwind (next_frame);
   if (mips_pc_is_mips16 (pc))
     return &mips_insn16_frame_unwind;
   return NULL;
@@ -1961,7 +1962,8 @@ mips_insn32_frame_cache (struct frame_in
 
   /* Analyze the function prologue.  */
   {
-    const CORE_ADDR pc = frame_pc_unwind (next_frame);
+    const CORE_ADDR pc =
+      frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
     CORE_ADDR start_addr;
 
     find_pc_partial_function (pc, NULL, &start_addr, NULL);
@@ -2014,7 +2016,7 @@ static const struct frame_unwind mips_in
 static const struct frame_unwind *
 mips_insn32_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
+  CORE_ADDR pc = frame_pc_unwind (next_frame);
   if (! mips_pc_is_mips16 (pc))
     return &mips_insn32_frame_unwind;
   return NULL;

Re: [RFA/mips(commit?)] Unwinding from noreturn function

[Daniel Jacobowitz]

On Wed, Mar 07, 2007 at 01:42:28PM -0800, Joel Brobecker wrote:
> Yep, I remembered. However, I also thought that your choice made sense.
> I really think it does, but given:

Yes, you're right - you could argue this either way.  I suspect that
to get it quite right you'd need both the in-block address and the
PC.

> I decided to revert your changes for now.

Thanks.  Looks good.

> I'm also sad to report that I have been told to put off work on
> mips-irix for a while. That was a personal initiative on my side,
> but I'm lacking time at work, so this was the first task that got cut.
> I hope someone else can find the time to bring it back to life...

Oh, well.  Thanks for your efforts - I certainly sympathize :-)

-- 
Daniel Jacobowitz
CodeSourcery