From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 158323858032 for ; Mon, 16 Oct 2023 15:51:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 158323858032 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 ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 158323858032 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.158.5 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697471526; cv=none; b=oFj0YQi2sNAthaN9YWUNbxFgqrA2XLEGkiAA5EGsRUyGwA5Xei8cLJyVywb0QOfwO2zOt3vr+/SY694kyAZs4FPgFnCpofBWuJkskihdCXHEig/5prxZSttfNIeMxfHMYnvveahUtuxI7RWhtiOuusUlS0rz6nEVRcXbQ/0J42c= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697471526; c=relaxed/simple; bh=QihP44fG11ZXkQCH+Gr285vmn2jsD6e9nq9Gd9oOPVc=; h=DKIM-Signature:Message-ID:From:To:Date:Mime-Version:Subject; b=edjZiEXzR9jbXltXe/ACmj0lkQyOu1BIQztmY02+77sV6t1kGFIPsMEmQpMBfFlYektbl8idCAg/8SkUeVIZRV23tNTVd5y83OlHm5HgsasuoWAVQuZKX0mRObBG9uAG4K7f/8giDk5zWCcTQlbWOZwMX9BZF3Us99QNtNksxOA= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0353722.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 39GFj6lb009986 for ; Mon, 16 Oct 2023 15:51:55 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : from : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding : subject; s=pp1; bh=VVyhIiXHAtcy7uIkHVUy9styyPybNXm7c9Ljk8uWp+I=; b=pIvjY4vtsVCNh3tB1i6w0exuADwlOwZjqTEMnHhBFVgb2fuC2AMtUIdwjCa6dxv3yxHI XprdQ05ThLIBvo5xasF3+QsAVsWPtjKvLy4uCHyLVh79MH9pXfsPnJ59z31rlJwMeg3H tPSBMspgiqvkvMD+MmMFlBZOYV8Q37rpThgbxRfiruU1vvVeTroZGypn+sP7zbcKPi0l pvDc5T0d2zI7gh78hE1mlBLgJfJfUdJK+ky45eH4m8vdGHby27aFWWlAbfMr+npuiWb6 +OtAEc27w17Duw5YXO+v8x+QBewJ/4kbNr02rA6NWREbDabe5mmEj24jEK7JP+Klu+9v 9w== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3ts836g9uc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 16 Oct 2023 15:51:55 +0000 Received: from m0353722.ppops.net (m0353722.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 39GFjA8x010938 for ; Mon, 16 Oct 2023 15:51:54 GMT Received: from ppma13.dal12v.mail.ibm.com (dd.9e.1632.ip4.static.sl-reverse.com [50.22.158.221]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3ts836g9ty-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 16 Oct 2023 15:51:54 +0000 Received: from pps.filterd (ppma13.dal12v.mail.ibm.com [127.0.0.1]) by ppma13.dal12v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 39GDjoWD030738; Mon, 16 Oct 2023 15:51:53 GMT Received: from smtprelay04.wdc07v.mail.ibm.com ([172.16.1.71]) by ppma13.dal12v.mail.ibm.com (PPS) with ESMTPS id 3tr7hj9er3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 16 Oct 2023 15:51:53 +0000 Received: from smtpav01.wdc07v.mail.ibm.com (smtpav01.wdc07v.mail.ibm.com [10.39.53.228]) by smtprelay04.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 39GFpq9W45220202 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 16 Oct 2023 15:51:52 GMT Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 1BC8858055; Mon, 16 Oct 2023 15:51:52 +0000 (GMT) Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3D0ED5805B; Mon, 16 Oct 2023 15:51:51 +0000 (GMT) Received: from wecm-9-67-110-146.wecm.ibm.com (unknown [9.67.110.146]) by smtpav01.wdc07v.mail.ibm.com (Postfix) with ESMTP; Mon, 16 Oct 2023 15:51:51 +0000 (GMT) Message-ID: <240a25512793955b83d82d90cd6d4f0ce09c564e.camel@us.ibm.com> From: Carl Love To: Andrew Burgess , gdb-patches@sourceware.org, UlrichWeigand Cc: cel@us.ibm.com Date: Mon, 16 Oct 2023 08:51:50 -0700 In-Reply-To: <87bkcyhc5g.fsf@redhat.com> References: <6f9c8fa20962129048384d74f6f15d1b37d255ed.camel@us.ibm.com> <76b8ed7b93608d40ab42b0538319f78eaf7d621c.camel@us.ibm.com> <87bkcyhc5g.fsf@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-22.el8) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: ePNqDmT7ad5R_DvzZEymEcAWGDsYLpW6 X-Proofpoint-ORIG-GUID: N5k4MjGiErD2Yh3Oc4z2TqxXJmH6fg-A Subject: RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-16_09,2023-10-12_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 mlxlogscore=737 spamscore=0 priorityscore=1501 phishscore=0 malwarescore=0 mlxscore=0 suspectscore=0 clxscore=1015 impostorscore=0 adultscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000 definitions=main-2310160136 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,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: Andrew: Thanks for the review and comments. See responses below. On Mon, 2023-10-16 at 15:31 +0100, Andrew Burgess wrote: > Carl Love writes: > > > GDB maintainers: > > > > This is the first patch in the series which fixes the DWWARF > > register > > mapping and signal handling issues on PowerPC. > > > > Carl > > > > ----------------------------------------------- > > > > rs6000, Fix Linux DWARF register mapping > > > > The PowerPC DWARF register mapping is the same for the .eh_frame > > and > > .debug_frame on Linux. PowerPC uses different mapping for > > .eh_frame and > > .debug_frame on other operating systems. The current GDB support > > for > > mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum > > and > > rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct > > for Linux. > > The files have some legacy mappings for spe_acc, spefscr, EV which > > was > > removed from GCC in 2017. > > > > This patch adds a two new functions > > rs6000_linux_dwarf2_reg_to_regnum, > > and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c > > to handle > > the DWARF register mappings on Linux. Function > > rs6000_linux_dwarf2_reg_to_regnum is installed for both > > gdb_dwarf_to_regnum > > and gdbarch_stab_reg_to_regnum since the mappings are the same. > > > > The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated > > to > > call set_gdbarch_dwarf2_reg_to_regnum map the new function > > rs6000_linux_dwarf2_reg_to_regnum for the architecture. Similarly, > > dwarf2_frame_set_adjust_regnum is called to map > > rs6000_linux_adjust_frame_regnum into the architecture. > > > > The second issue fixed by this patch is the check for subroutine > > process_event_stop_test. Need to make sure the frame is not the > > SIGTRAMP_FRAME. The sequence of events on most platforms is: > > Usually for GDB we avoid bundling unrelated changes into a single > commit. Each commit should address one self contained issue (as far > as > possible). > > I really struggling to see any connection between the two fixes you > have > here. As mentioned in the commit log, the first patch which fixes the DWARF register mapping fixes two of the 5 failures seen in store.exp. Specifically the patch fixes the issue of GDB not stopping in the handler which results in the step.exp test failing. The second patch then fixes the rest of the issues with the step.exp test. > > > 1) Some code is running when a signal arrives. > > 2) The kernel handles the signal and dispatches to the handler. > > ... > > > > However on PowerPC the sequence of events is: > > > > 1) Some code is running when a signal arrives. > > 2) The kernel handles the signal and dispatches to the > > trampoline. > > 3) The trampoline performs a normal function call to the handler. > > ... > > > > We want "nexti" to step into, not over, signal handlers invoked > > by the kernel. This is the case most platforms as the kernel puts > > a > > signal trampoline frame onto the stack to handle proper return > > after the > > handler. However, on some platforms such as PowerPC, the kernel > > actually > > uses a trampoline to handle *invocation* of the handler. > > > > The issue is fixed in function process_event_stop_test by adding a > > check > > that the frame is not a SIGTRAMP_FRAME to the if statement to stop > > in > > a subroutine call. This prevents GDB from erroneously detecting > > the > > trampoline invocation as a subroutine call. > > > > This patch fixes two regression test failures in > > gdb.base/store.exp. It > > also fixes two regression failures in gdb.python/py-thread- > > exited.exp. > > On the one random PPC box I tried this patch on, I'm not seeing any > failures in gdb.python/py-thread-exited.exp either before, or after > this > commit. > > Which tests in gdb.python/py-thread-exited.exp are you seeing as > broken? > And which of the two fixes in this commit fix the problems you're > seeing? The comment about the py-thread-exited.exp should have been removed. I found that sometimes that test fails and sometime passes. I have yet to dig into it and figure out why the test is inconsistent. The inconsistent behavior exists with and without these patches. I will remove the comment about the gdb.python/py-thread-exited.exp fixes as that is erroneous. > > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 > > with no > > new regressions. > > --- > > gdb/infrun.c | 13 ++++++++++ > > gdb/ppc-linux-tdep.c | 56 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 69 insertions(+) > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c > > index 4730d290442..922d8a6913d 100644 > > --- a/gdb/infrun.c > > +++ b/gdb/infrun.c > > @@ -7334,8 +7334,21 @@ process_event_stop_test (struct > > execution_control_state *ecs) > > initial outermost frame, before sp was valid, would > > have code_addr == &_start. See the comment in > > frame_id::operator== > > for more. */ > > + > > + /* We want "nexti" to step into, not over, signal handlers > > invoked > > + by the kernel, therefore this subroutine check should not > > trigger > > + for a signal handler invocation. On most platforms, this is > > already > > + not the case, as the kernel puts a signal trampoline frame > > onto the > > + stack to handle proper return after the handler, and > > therefore at this > > + point, the current frame is a grandchild of the step frame, > > not a > > + child. However, on some platforms, the kernel actually uses > > a > > + trampoline to handle *invocation* of the handler. In that > > case, > > + when executing the first instruction of the trampoline, this > > check > > + would erroneously detect the trampoline invocation as a > > subroutine > > + call. Fix this by checking for SIGTRAMP_FRAME. */ > > if ((get_stack_frame_id (frame) > > != ecs->event_thread->control.step_stack_frame_id) > > + && get_frame_type (frame) != SIGTRAMP_FRAME > > && ((frame_unwind_caller_id (get_current_frame ()) > > == ecs->event_thread->control.step_stack_frame_id) > > && ((ecs->event_thread->control.step_stack_frame_id > > diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c > > index 784dafa59db..7fb90799dff 100644 > > --- a/gdb/ppc-linux-tdep.c > > +++ b/gdb/ppc-linux-tdep.c > > @@ -83,6 +83,7 @@ > > #include "features/rs6000/powerpc-isa207-vsx64l.c" > > #include "features/rs6000/powerpc-isa207-htm-vsx64l.c" > > #include "features/rs6000/powerpc-e500l.c" > > +#include "dwarf2/frame.h" > > > > /* Shared library operations for PowerPC-Linux. */ > > static struct target_so_ops powerpc_so_ops; > > @@ -2088,6 +2089,52 @@ ppc_linux_displaced_step_prepare (gdbarch > > *arch, thread_info *thread, > > return per_inferior->disp_step_buf->prepare (thread, > > displaced_pc); > > } > > > > +/* Convert a Dwarf 2 register number to a GDB register number for > > Linux. */ > > +static int > > +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int > > num) > > +{ > > + ppc_gdbarch_tdep *tdep = > > gdbarch_tdep(gdbarch); > > + > > + if (0 <= num && num <= 31) > > + return tdep->ppc_gp0_regnum + num; > > + else if (32 <= num && num <= 63) > > + /* FIXME: jimb/2004-05-05: What should we do when the debug > > info > > + specifies registers the architecture doesn't have? Our > > + callers don't check the value we return. */ > > I see this comment was just copied from else where, but isn't the > answer > just: return -1 ? > > The comment about the 'return -1' at the trail of this function seems > to > suggest that would be the correct thing to do. > > I guess I'm asking: do we need to add another copy of this (I think > out > of date) fixme? Yes, the function is based on rs6000_dwarf2_reg_to_regnum with the specific changes for the Linux DWARF mappings. The comment looked out of date to me, but I left it for consistency with the other two functions that have the same comment. It would probably be best to investigate the comment further and update it in all places in a separate patch. Would it be acceptable to leave the comment as is for now, for consistency sake, and I will work on a separate cleanup patch to address removing the comment in the original two places and this additional place? I will want to look into it a bit more but I think we can just remove the comment. But I do want to verify that the return value is never used first. Carl