[RFA/hppa] Fix pb in inferior function call

[RFA/hppa] Fix pb in inferior function call

[Joel Brobecker]

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

Hello,

I was trying to understand the source of the following problem
(extracted from call-rt-st.exp):

     (gdb) p print_struct_rep (*struct1)
     Contents of struct1: 
     
             22         0
     dummy-frame.c:304: internal-error: dummy_frame_prev_register: Assertion `dummy != NULL' failed.
     A problem internal to GDB has been detected,
     further debugging may prove unreliable.
     Quit this debugging session? (y or n) n
     
     dummy-frame.c:304: internal-error: dummy_frame_prev_register: Assertion `dummy != NULL' failed.
     A problem internal to GDB has been detected,
     further debugging may prove unreliable.
     Create a core file of GDB? (y or n) n
     
     dummy-frame.c:304: internal-error: dummy_frame_prev_register: Assertion `dummy != NULL' failed.
     A problem internal to GDB has been detected,
     further debugging may prove unreliable.
     Quit this debugging session? (y or n) n
     
     dummy-frame.c:304: internal-error: dummy_frame_prev_register: Assertion `dummy != NULL' failed.
     A problem internal to GDB has been detected,
     further debugging may prove unreliable.
     Create a core file of GDB? (y or n) n

The assertion fails because we fail to locate the dummy_frame
in our dummy_frame_stack. The reason for the failure is that
the TOS stored in the dummy_frame we saved is different from
the stack_addr of the frame_id we built for the dummy_frame.
It's off by a few bytes.

The stack_addr for the dummy frame is computed by reading the Stack
Pointer register. The TOS value is the value of SP after the dummy
frame has been pushed.

If I understand correctly how this is all supposed to work, I think
we simply forgot to update the value of the SP register. Because the
function doesn't read its parameters from the stack (the struct is
passed via 2 registers), we don't see any noticeable effect on the
execution of the function we called. However, when we reach our
end-of-inferior-function-call, the value of the SP is back to the
original value, which doesn't match the saved TOS.

2004-04-30  J. Brobecker  <brobecker@gnat.com>

        * hppa-tdep.c (hppa32_push_dummy_call): Set the Stack Pointer.
        (hppa64_push_dummy_call): Likewise.

The change has been tested on hppa32-hpux11.00, and it fixes roughly
500 regressions (yay! :-). It also brings the duration of the testsuite
run from several hours down to about 45 mins.

I didn't test the change for hppa64, but it seems pretty obvious if
the hppa32 one is correct.

OK to apply?

Thanks,
-- 
Joel

PS: My main objective is to get the frame code stable enough so that
    the patch I was working on to detect that we stopped inside a
    function call using frame IDs works without regressions on HP/UX.
    I didn't realize I would open such a can of worms when I first
    started on this path... :-/

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

Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.136
diff -u -p -r1.136 hppa-tdep.c
--- hppa-tdep.c	7 Mar 2004 19:58:27 -0000	1.136
+++ hppa-tdep.c	31 Mar 2004 03:57:30 -0000
@@ -911,6 +911,9 @@ hppa32_push_dummy_call (struct gdbarch *
   /* Set the return address.  */
   regcache_cooked_write_unsigned (regcache, RP_REGNUM, bp_addr);
 
+  /* Update the Stack Pointer.  */
+  regcache_cooked_write_unsigned (regcache, SP_REGNUM, param_end + 32);
+
   /* The stack will have 32 bytes of additional space for a frame marker.  */
   return param_end + 32;
 }
@@ -1031,6 +1034,9 @@ hppa64_push_dummy_call (struct gdbarch *
 
   /* Set the return address.  */
   regcache_cooked_write_unsigned (regcache, RP_REGNUM, bp_addr);
+
+  /* Update the Stack Pointer.  */
+  regcache_cooked_write_unsigned (regcache, SP_REGNUM, param_end + 64);
 
   /* The stack will have 32 bytes of additional space for a frame marker.  */
   return param_end + 64;

Re: [RFA/hppa] Fix pb in inferior function call

[Daniel Jacobowitz]

On Tue, Mar 30, 2004 at 08:18:48PM -0800, Joel Brobecker wrote:
> The assertion fails because we fail to locate the dummy_frame
> in our dummy_frame_stack. The reason for the failure is that
> the TOS stored in the dummy_frame we saved is different from
> the stack_addr of the frame_id we built for the dummy_frame.
> It's off by a few bytes.
> 
> The stack_addr for the dummy frame is computed by reading the Stack
> Pointer register. The TOS value is the value of SP after the dummy
> frame has been pushed.
> 
> If I understand correctly how this is all supposed to work, I think
> we simply forgot to update the value of the SP register. Because the
> function doesn't read its parameters from the stack (the struct is
> passed via 2 registers), we don't see any noticeable effect on the
> execution of the function we called. However, when we reach our
> end-of-inferior-function-call, the value of the SP is back to the
> original value, which doesn't match the saved TOS.
> 
> 2004-04-30  J. Brobecker  <brobecker@gnat.com>
> 
>         * hppa-tdep.c (hppa32_push_dummy_call): Set the Stack Pointer.
>         (hppa64_push_dummy_call): Likewise.
> 
> The change has been tested on hppa32-hpux11.00, and it fixes roughly
> 500 regressions (yay! :-). It also brings the duration of the testsuite
> run from several hours down to about 45 mins.
> 
> I didn't test the change for hppa64, but it seems pretty obvious if
> the hppa32 one is correct.
> 
> OK to apply?

OK.  You're right; push_dummy_call should update the stack pointer
itself.  I think one of the previous dummy call mechanisms wasn't
supposed to:
  if (DEPRECATED_DUMMY_WRITE_SP_P ())
    DEPRECATED_DUMMY_WRITE_SP (sp);
so it probably just got lost in a conversion.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA/hppa] Fix pb in inferior function call

[Andrew Cagney]

> I think one of the previous dummy call mechanisms wasn't
> supposed to:
>   if (DEPRECATED_DUMMY_WRITE_SP_P ())
>     DEPRECATED_DUMMY_WRITE_SP (sp);

It was never really pinned down, either using write_sp or using 
push_dummy_args (or what ever it was called), or ...

Now push_dummy_call is responsible for everything.

> so it probably just got lost in a conversion.

Yes.

Joel, do you think a pull up of the hppa stuff onto the 6.1 branch would 
be useful?  (it can only break HP/UX so is "mostly harmless").

Andrew

Re: [RFA/hppa] Fix pb in inferior function call

[Joel Brobecker]

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

> > 2004-04-30  J. Brobecker  <brobecker@gnat.com>
> > 
> >         * hppa-tdep.c (hppa32_push_dummy_call): Set the Stack Pointer.
> >         (hppa64_push_dummy_call): Likewise.
> > 
> > The change has been tested on hppa32-hpux11.00, and it fixes roughly
> > 500 regressions (yay! :-). It also brings the duration of the testsuite
> > run from several hours down to about 45 mins.
> > 
> > I didn't test the change for hppa64, but it seems pretty obvious if
> > the hppa32 one is correct.
> > 
> > OK to apply?
> 
> OK.  You're right; push_dummy_call should update the stack pointer
> itself.

Thanks. Checked in.

Andrew asked:
> Joel, do you think a pull up of the hppa stuff onto the 6.1 branch would
> be useful?  (it can only break HP/UX so is "mostly harmless").

Yes, I think so. I tested and then committed the attached patch for the
branch (there was one minor edit to make).

2004-04-31  J. Brobecker  <brobecker@gnat.com>
  
        * hppa-tdep.c (hppa32_push_dummy_call): Set the Stack Pointer.
        (hppa64_push_dummy_call): Likewise.

Thanks,
-- 
Joel

[-- Attachment #2: hppa-tdep-branch.diff --]
[-- Type: text/plain, Size: 1031 bytes --]

Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.129.2.1
diff -u -p -r1.129.2.1 hppa-tdep.c
--- hppa-tdep.c	10 Mar 2004 20:06:08 -0000	1.129.2.1
+++ hppa-tdep.c	31 Mar 2004 18:41:17 -0000
@@ -2269,6 +2269,9 @@ hppa32_push_dummy_call (struct gdbarch *
   /* Set the return address.  */
   regcache_cooked_write_unsigned (regcache, RP_REGNUM, bp_addr);
 
+  /* Update the Stack Pointer.  */
+  regcache_cooked_write_unsigned (regcache, SP_REGNUM, param_end + 32);
+
   /* The stack will have 32 bytes of additional space for a frame marker.  */
   return param_end + 32;
 }
@@ -2392,6 +2395,9 @@ hppa64_push_dummy_call (struct gdbarch *
   
   /* Set the return address.  */
   regcache_cooked_write_unsigned (regcache, RP_REGNUM, bp_addr);
+
+  /* Update the Stack Pointer.  */
+  regcache_cooked_write_unsigned (regcache, SP_REGNUM, sp + 64);
 
   /* The stack will have 64 bytes of additional space for a frame
      marker.  */

Re: [RFA/hppa] Fix pb in inferior function call

[Andrew Cagney]

> 
> Thanks. Checked in.
> 
> Andrew asked:
> 
>>> Joel, do you think a pull up of the hppa stuff onto the 6.1 branch would
>>> be useful?  (it can only break HP/UX so is "mostly harmless").
> 
> 
> Yes, I think so. I tested and then committed the attached patch for the
> branch (there was one minor edit to make).
> 
> 2004-04-31  J. Brobecker  <brobecker@gnat.com>
>   
>         * hppa-tdep.c (hppa32_push_dummy_call): Set the Stack Pointer.
>         (hppa64_push_dummy_call): Likewise.

Are there any other differences (such as in the config/pa/ directory?)

Andrew