From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 961373858D1E for ; Mon, 20 Feb 2023 16:34:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 961373858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=us.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=us.ibm.com Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 31KF3B9T024157; Mon, 20 Feb 2023 16:34:22 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : subject : from : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding; s=pp1; bh=uiHsMB2dabdTjbeTsthSR/wDAbww0ig0JQ7Av/ZkQN8=; b=C1EiZKh0lb5z/JtTQFJ8+FmLpVwcPaUsD3k2LRbE8zADexkxP4ysu33ksl9HCfWxPhXA lqlzL0KVRXwYi9XhsEok13Zl0FmIdV0M3GBhbAxKVudF6ZWV1P7pESqZwxgNLZPRftEX xCQsy2gR7e0AChh5wVEb6JwpPlDeep9vyaLnEve4bVl5YmJzGfE6o3A/XjhfAwPR/MUO htmac2lSR+tri18RzPlcs09wXHAs/qizvug8A+WOc20wfIKNDf1HBG+Fv1OptDePv8oQ AV964KpLffg1+55X4QCz3f1JLcIrg/dddICaOkancso3xrwgxDIWlTpSC3Y6KI5/jdtT PQ== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3nv2at88as-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 20 Feb 2023 16:34:21 +0000 Received: from m0098416.ppops.net (m0098416.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 31KEwtnt010534; Mon, 20 Feb 2023 16:34:21 GMT Received: from ppma03dal.us.ibm.com (b.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.11]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3nv2at88a9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 20 Feb 2023 16:34:21 +0000 Received: from pps.filterd (ppma03dal.us.ibm.com [127.0.0.1]) by ppma03dal.us.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 31KFJ5Hm005297; Mon, 20 Feb 2023 16:34:20 GMT Received: from smtprelay07.wdc07v.mail.ibm.com ([9.208.129.116]) by ppma03dal.us.ibm.com (PPS) with ESMTPS id 3ntpa6snce-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 20 Feb 2023 16:34:20 +0000 Received: from smtpav02.dal12v.mail.ibm.com (smtpav02.dal12v.mail.ibm.com [10.241.53.101]) by smtprelay07.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 31KGYI8463504880 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 20 Feb 2023 16:34:19 GMT Received: from smtpav02.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B3B445805A; Mon, 20 Feb 2023 16:34:18 +0000 (GMT) Received: from smtpav02.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3E0D95805C; Mon, 20 Feb 2023 16:34:18 +0000 (GMT) Received: from li-e362e14c-2378-11b2-a85c-87d605f3c641.ibm.com (unknown [9.163.93.208]) by smtpav02.dal12v.mail.ibm.com (Postfix) with ESMTP; Mon, 20 Feb 2023 16:34:18 +0000 (GMT) Message-ID: <971cb4e330db3bd8bd728f2f2c3036ee0ec8a26d.camel@us.ibm.com> Subject: Re: [PATCH ] PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp From: Carl Love To: Ulrich Weigand , "gdb-patches@sourceware.org" , Bruno Larsen , "tdevries@suse.de" , "pedro@palves.net" Cc: cel@us.ibm.com Date: Mon, 20 Feb 2023 08:34:17 -0800 In-Reply-To: <041f62e9f26fd4a536bc90c34f072985582e6237.camel@de.ibm.com> References: <071f24ecf9b3a2bbbe8fee7db77492eb55c5f3ff.camel@us.ibm.com> <1d9b21914354bef6a290ac30673741e722e11757.camel@de.ibm.com> <3e3c9c40f07ab01c79fe10915e76ffa187c42ad9.camel@us.ibm.com> <122f5d2d3db9ef1979b0f8da927d005f32bba82c.camel@us.ibm.com> <011768e8-2b76-f8ed-1174-fbaa020b15e7@redhat.com> <78b464a1-e32e-c3da-85e4-7bfc322cc29f@redhat.com> <7848e9858b54e33e399b871774ffc0b5058c1736.camel@us.ibm.com> <65d44121-65f7-a212-79ec-07ce53c15ecb@suse.de> <9fe94c0979cb40979b0dea7693a901c2d9f66164.camel@us.ibm.com> <59417813-eb4a-baf8-4e5d-e225d6732f71@suse.de> <7a494157-494f-6adf-d533-bf373b0f054f@redhat.com> <71aa635593df0677811afb85409aa190bcfa4f6a.camel@us.ibm.com> <15864a6b87b25c93e99a28149f23138267735f2a.camel@us.ibm.com> <041f62e9f26fd4a536bc90c34f072985582e6237.camel@de.ibm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-18.el8) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: wkGWogXyuIX4U6IIj-NpRNZPSXtH0UOp X-Proofpoint-ORIG-GUID: IffkdJrDfGgt4TEo9Q3mz3Ojp8KvBrgX X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.170.22 definitions=2023-02-20_13,2023-02-20_02,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 malwarescore=0 suspectscore=0 mlxlogscore=999 adultscore=0 phishscore=0 mlxscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 spamscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2302200151 X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 2023-02-17 at 12:24 +0000, Ulrich Weigand wrote: > Carl Love wrote: > > This looks generally OK to me, except for two minor issues > in the code base, and one problem in the test suite: > > > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > > index 77206fcbfe8..a65cc700fc9 100644 > > --- a/gdb/infcmd.c > > +++ b/gdb/infcmd.c > > @@ -1728,28 +1728,41 @@ finish_backward (struct finish_command_fsm > > *sm) > > no way that a function up the stack can have a return address > > that's equal to its entry point. */ > > > > - if (sal.pc != pc) > > - { > > - frame_info_ptr frame = get_selected_frame (nullptr); > > - struct gdbarch *gdbarch = get_frame_arch (frame); > > + CORE_ADDR alt_entry_point = sal.pc; > > + CORE_ADDR entry_point = alt_entry_point; > > + frame_info_ptr frame = get_selected_frame (nullptr); > > + struct gdbarch *gdbarch = get_frame_arch (frame); > > > > - /* Set a step-resume at the function's entry point. Once > > that's > > - hit, we'll do one more step backwards. */ > > - symtab_and_line sr_sal; > > - sr_sal.pc = sal.pc; > > - sr_sal.pspace = get_frame_program_space (frame); > > - insert_step_resume_breakpoint_at_sal (gdbarch, > > - sr_sal, null_frame_id); > > + if (gdbarch_skip_entrypoint_p (gdbarch)) > > + /* Some architectures, like PowerPC use local and global entry > > points. > > + There is only one Entry Point (GEP = LEP) for other > > architectures. > > + The GEP is an alternate entry point. The LEP is the normal > > entry point. > > + The value of entry_point was initialized to the alternate > > entry point > > + (GEP). It will be adjusted to the normal entry point if > > the function > > + has two entry points. */ > > + entry_point = gdbarch_skip_entrypoint (gdbarch, sal.pc); > > + > > + if ((pc >= alt_entry_point) && (pc <= entry_point)) > > + /* We are either at one of the entry points or between the > > entry points. > > + If we are not at the alt_entry point, go back to the > > alt_entry_point > > + If we at the normal entry point step back one instruction, > > when we > > + stop we will determine if we entered via the entry point or > > the > > + alternate entry point. If we are at the alternate entry > > point, > > + single step back to the function call. */ > > + tp->control.step_range_start = tp->control.step_range_end = 1; > > > > - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); > > - } > > else > > { > > - /* We're almost there -- we just need to back up by one more > > - single-step. */ > > - tp->control.step_range_start = tp->control.step_range_end = > > 1; > > - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); > > + /* We are in the body of the function. Set a breakpoint to > > backup to > > + the normal entry point. */ > > + symtab_and_line sr_sal; > > + sr_sal.pc = entry_point; > > + sr_sal.pspace = get_frame_program_space (frame); > > + insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal, > > + null_frame_id); > > } > > + > > + proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); > > } > > This change looks much larger than it actually is, because you've > swapped the order of the if vs. else blocks (the orignal code sets > the breakpoint in the if path and the single-step range in the > else path, while you've swapped this). Can you swap this back to > shorten the diff? OK, swapped them. > > > + if (execution_direction == EXEC_REVERSE > > + && ecs->event_thread->control.proceed_to_finish) > > + { > > + /* We are executing the reverse-finish command. */ > > + if (ecs->event_thread->stop_pc () >= ecs- > > >stop_func_alt_start > > + && ecs->event_thread->stop_pc () < ecs->stop_func_start > > + && ecs->stop_func_alt_start != ecs->stop_func_start) > > This third condition seems redundant: if stop_pc is >= func_alt_start > *and* < func_start, then func_alt_start cannot be equal func_start. Yes, on Powerpc the third condition is redundant. However, on other platforms, specifically X86, the third condition is always false which ensures the if statement is false. This is important on the tests gdb.btrace/tailcall-only.exp and gdb.btrace/tailcall.exp which only run on X86. The tests call the up command which will satisfy conditions 1 and 2 and thus do an "extra" step command resulting in a test failure. The third condition ensures that we never execute an additional reverse-step unless the platform supports multiple entry points and we are at the alternate entry point. I can see where this is not obvious at first glance. I added a comment to the if statement explaining the need for the apparent redundancy. Also, the if statement is nested inside of the if reverse if statement. I combined the two if statements into a single if statement. > > diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp > > b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp > > index 94292d5eb9b..dc5cf097511 100644 > > --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp > > +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp > > @@ -36,39 +36,6 @@ if { ![runto_main] } { > > return -1 > > } > > > > -# Do repeated stepping COMMANDs in order to reach TARGET from > > CURRENT > > -# > > -# COMMAND is a stepping command > > -# CURRENT is a string matching the current location > > -# TARGET is a string matching the target location > > -# TEST is the test name > > -# > > -# The function issues repeated COMMANDs as long as the location > > matches > > -# CURRENT up to a maximum of 100 steps. > > -# > > -# TEST passes if the resulting location matches TARGET and fails > > -# otherwise. > > -# > > -proc step_until { command current target test } { > > - global gdb_prompt > > - > > - set count 0 > > - gdb_test_multiple "$command" "$test" { > > - -re "$current.*$gdb_prompt $" { > > - incr count > > - if { $count < 100 } { > > - send_gdb "$command\n" > > - exp_continue > > - } else { > > - fail "$test" > > - } > > - } > > - -re "$target.*$gdb_prompt $" { > > - pass "$test" > > - } > > - } > > -} > > - > > gdb_test_no_output "record" > > gdb_test "next" ".*" "record trace" > > How is it OK to just remove this procedure? This is still used > in the rest of the step-indirect-call-thunk.exp test case ... Yes, looks like my fix up of the call to step_until in step-indirect- call-thunk.exp got lost somewhere along the line. I remember making the change. I went back to see why the regression testing didn't catch the error. On PowerPC, the test doesn't run as the compile options "-mindirect- branch=thunk" and "-mfunction-return=thunk" are not supported on PowerPC, so the test fails anyways. It looks like the test didn't run on my X86 either due to the regression test failing before getting to the gdb tests. Looks like I need to run the regression test in subdirectory gdb to make sure all of the gdb tests get run. I have run the updated patch, on X86, manually and in the regression test to make sure there are no regressions with this test. I checked out the base and regression runs on X86 to make sure the regression tests are all be run. > > I get why you may want to reduce duplication, but then you'd > have to update current users of "step_until" as well. Yes, those updates got lost rebasing the patch at some point. Fixed. > > Also, talking about duplication, should the (already separate) > gdb_step_until command in lib/gdb.exp now just be a variant > of the new repeat_cmd_until ? OK, I extended the new repeat_cmd_until proceedure to take the additional input max_steps. I created a new version of gdb_step_until that calls repeat_cmd_until. I added an optional argument, current, to gdb_step_until proceedure for the current line. By default, current is set to "\}" which is needed by the call to repeat_cmd_until proceedure. The new gdb_step_until proceedure works in the existing regression tests without changing the gdb_step_until call in the existing tests. Thanks for reviewing the patch. I will post an updated patch shortly. Carl