From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8048 invoked by alias); 7 May 2003 18:37:24 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 8019 invoked from network); 7 May 2003 18:37:22 -0000 Received: from unknown (HELO touchme.toronto.redhat.com) (207.219.125.105) by sources.redhat.com with SMTP; 7 May 2003 18:37:22 -0000 Received: from redhat.com (toocool.toronto.redhat.com [172.16.14.72]) by touchme.toronto.redhat.com (Postfix) with ESMTP id 6D42B800030; Wed, 7 May 2003 14:37:21 -0400 (EDT) Message-ID: <3EB95261.1080204@redhat.com> Date: Wed, 07 May 2003 18:37:00 -0000 From: "J. Johnston" Organization: Red Hat Inc. User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Andrew Cagney Cc: gdb-patches@sources.redhat.com Subject: Re: RFC: patch to refresh prev_pc References: <3EB6CE0A.4070409@redhat.com> <3EB8114B.4010809@redhat.com> Content-Type: multipart/mixed; boundary="------------090603050301040807080707" X-SW-Source: 2003-05/txt/msg00092.txt.bz2 This is a multi-part message in MIME format. --------------090603050301040807080707 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2490 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 * infrun.c (prev_pc): Move declaration ahead of proceed(). (proceed): Refresh prev_pc value before resuming. (stop_stepping): Remove code to refresh prev_pc. --------------090603050301040807080707 Content-Type: text/plain; name="infrun.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="infrun.patch" Content-length: 3318 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; /* 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; } --------------090603050301040807080707--