Re: RFC: patch to refresh prev_pc

["J. Johnston" <jjohnstn@redhat.com>]

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

Andrew Cagney wrote:
>> The following patch solves a problem on the ia64.  The problem
>> exists because of a generic problem to reset the prev_pc value
>> after an inferior function call or after a return command.
>>
>> Because the value is not properly set, the line number used to
>> initialize the ecs is incorrect.  On the ia64 this causes a problem
>> because there are extraneous linetable entries generated by the compiler
>> that are within the line (i.e. they don't change the line number).
>> When we apply "next" logic which uses the ecs line number, we end up 
>> stopping
>> at the first line table entry past our start position.  This often ends
>> up being just a few insns farther in the same line.  A specific example
>> of this problem is the next to 1237 test inside call-ar-st.exp.  An
>> inferior call is made on line 1236 and upon return we issue a next.
>>
>> I discussed this topic on the gdb forum and a number of attempts were
>> made to ensure the prev_pc value was up to date in 
>> init_execution_control_state()
>> in infrun.c.  Those attempts failed because the inferior was not 
>> guaranteed to
>> be stopped and so we weren't guaranteed that a ptrace to fetch the pc 
>> would work.
>>
>> This patch attempts to refresh the prev_pc value just before resuming 
>> in proceed().
>> It works for the ia64 problems cited above and also I have tested it 
>> on the x86.
>>
>> Is this patch ok?
> 
> 
> Yes, definitly a better strategy.  Two some tweaks:
> 
> - That new single line assignment needs some sort of big jucy 
> stand-alone comment that explains the rationale for the change, mention 
> where it was before, and where else was tried (and why both failed). The 
> more details the better, but something based on the above would do the 
> trick.
> 
> - the old (now redundant) code in stop_stepping vis:
> 
>   if (target_has_execution)
>     {
>       /* Assuming the inferior still exists, set these up for next
>          time, just like we did above if we didn't break out of the
>          loop.  */
>       prev_pc = read_pc ();
>     }
> 
> should be deleted (but check that it really does still work).
> 
> With those changes made, consider it approved.
> 

Revised patch checked in.  Thanks.

-- Jeff J.

2003-05-07  Jeff Johnston  <jjohnstn@redhat.com>

         * infrun.c (prev_pc): Move declaration ahead of proceed().
         (proceed): Refresh prev_pc value before resuming.
         (stop_stepping): Remove code to refresh prev_pc.


[-- Attachment #2: infrun.patch --]
[-- Type: text/plain, Size: 3318 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.108
diff -u -p -r1.108 infrun.c
--- infrun.c	5 May 2003 00:27:07 -0000	1.108
+++ infrun.c	7 May 2003 18:35:16 -0000
@@ -667,6 +667,12 @@ clear_proceed_status (void)
   bpstat_clear (&stop_bpstat);
 }
 
+
+/* Record the pc of the program the last time it stopped.  This is
+   just used internally by wait_for_inferior, but need to be preserved
+   over calls to it and cleared when the inferior is started.  */
+static CORE_ADDR prev_pc;
+
 /* Basic routine for continuing the program in various fashions.
 
    ADDR is the address to resume at, or -1 for resume where stopped.
@@ -772,6 +778,30 @@ proceed (CORE_ADDR addr, enum target_sig
      inferior.  */
   gdb_flush (gdb_stdout);
 
+  /* Refresh prev_pc value just prior to resuming.  This used to be
+     done in stop_stepping, however, setting prev_pc there did not handle
+     scenarios such as inferior function calls or returning from
+     a function via the return command.  In those cases, the prev_pc
+     value was not set properly for subsequent commands.  The prev_pc value 
+     is used to initialize the starting line number in the ecs.  With an 
+     invalid value, the gdb next command ends up stopping at the position
+     represented by the next line table entry past our start position.
+     On platforms that generate one line table entry per line, this
+     is not a problem.  However, on the ia64, the compiler generates
+     extraneous line table entries that do not increase the line number.
+     When we issue the gdb next command on the ia64 after an inferior call
+     or a return command, we often end up a few instructions forward, still 
+     within the original line we started.
+
+     An attempt was made to have init_execution_control_state () refresh
+     the prev_pc value before calculating the line number.  This approach
+     did not work because on platforms that use ptrace, the pc register
+     cannot be read unless the inferior is stopped.  At that point, we
+     are not guaranteed the inferior is stopped and so the read_pc ()
+     call can fail.  Setting the prev_pc value here ensures the value is 
+     updated correctly when the inferior is stopped.  */  
+  prev_pc = read_pc ();
+
   /* Resume inferior.  */
   resume (oneproc || step || bpstat_should_step (), stop_signal);
 
@@ -785,11 +815,6 @@ proceed (CORE_ADDR addr, enum target_sig
       normal_stop ();
     }
 }
-
-/* Record the pc of the program the last time it stopped.  This is
-   just used internally by wait_for_inferior, but need to be preserved
-   over calls to it and cleared when the inferior is started.  */
-static CORE_ADDR prev_pc;
 \f
 
 /* Start remote-debugging of a machine over a serial link.  */
@@ -2757,14 +2782,6 @@ step_over_function (struct execution_con
 static void
 stop_stepping (struct execution_control_state *ecs)
 {
-  if (target_has_execution)
-    {
-      /* Assuming the inferior still exists, set these up for next
-         time, just like we did above if we didn't break out of the
-         loop.  */
-      prev_pc = read_pc ();
-    }
-
   /* Let callers know we don't want to wait for the inferior anymore.  */
   ecs->wait_some_more = 0;
 }