From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 6953B3858C33 for ; Tue, 24 Oct 2023 08:50:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6953B3858C33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6953B3858C33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698137556; cv=none; b=Glnr7aZhG+NOGUC1KJM9NOszolU7rOjCuh1FwInQf5BtZ3KweAk6sW+xRFA/EwQpB1Z09gjWe6iBa6WN3QG+v+dVHWWZMSRVndxcuhIrqMyz44rh151WD5ut+2nSmn+YRMgJ1R3r38DQYGuF53M824HT+Kb6uiTIdGbyfTeQf5I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698137556; c=relaxed/simple; bh=Mxfsg/dO2RhTtHDpCoM/EKV/pb+Kkd502ODuHcGqdB0=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=XC+TCyenaYHVUliOfOUuzMfAc4ZpDdtW/7AU3SHfGCaP/WynJV9O8VsjkJKDynIClfrOCzOY7F6RaSaOPb/huYgEGMJy3AN7DWPyGkKc+FFY9y6Wxgf7UOWKqReYEjhYF32pBTVTIYBv7eWOSGe4xXf+HzYQzSVWRx3yuCrETJM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698137444; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=MNiurnf7N7PXnwvrEPhB5J7JzOy9Uonyp/CUkh6QixI=; b=N39c+31TvnnIITFGgy9LuvFUAZDZvuGC0RW8a+r+FJAVi8jkhaqIL+IxLBIeRBPsl0BaM+ 2YnP6iAa331AlPpBp+uwlRE9GGDeaiQP1x4Tf08eoOqz8jccbp/s1mAqFEGnD1dhXntGGh hD+Zra6wnnHldwOO974necVg05KJ8Ik= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-380-htgFOokYPH6RCdRYdzGi-Q-1; Tue, 24 Oct 2023 04:50:42 -0400 X-MC-Unique: htgFOokYPH6RCdRYdzGi-Q-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-9a681c3470fso255723066b.1 for ; Tue, 24 Oct 2023 01:50:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698137441; x=1698742241; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MNiurnf7N7PXnwvrEPhB5J7JzOy9Uonyp/CUkh6QixI=; b=p1b7FXy17cPDrnvgiXODHIX+HpeL6DzJnt96NxPY2DqZ+P3BThNOiHbkw13v9aOali FqsHIFLSaePKfJrPmY2RnZ9JACzlkLaEQU7MMTjdiuBFEI47c1uWpCdrO07lEY74B1Jj NF2JldF9dWcxhO2MOmOIriREaENNKUKC1Alkn3omfPgHlUMopwW5fGW3+zJtVEjJzVUk Zf4N4K7Ds0wb3kDFNB1Sa3u5QcoyCKXJTa7mSOFRWkgIooGEJMN1rF6Pw7J3/sg2qIK+ vA91jWgxD7wJIVJ76PTqMzBD2DZ0PGD67PzsJqjjgqUcV4ES2FaEzFCuAL2ndZSAUyJv 9bsA== X-Gm-Message-State: AOJu0YzwffhHDlzq/cbdjF4yjV6XrERNrzqt4O4rc+/YVt8SxkpbHE9s 7vUScioiwHMtQwrz5XcSuPMufxu/WtT2mFGxGNOzVfsuuLyTe6jg6cQhdVVP7i5bNx8NfU0x/2P zAYEBkRMP+NXHc5PZxOufbQ7PW457gg== X-Received: by 2002:a17:907:2cd4:b0:9ca:e7ce:8e5f with SMTP id hg20-20020a1709072cd400b009cae7ce8e5fmr2315361ejc.25.1698137441110; Tue, 24 Oct 2023 01:50:41 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGCzXiS8FiSheigegOtUaoK3+swxoCari3u9tGbjQfAIEzmSxMPUaulPs9Hf+GNdhW78GpJXQ== X-Received: by 2002:a17:907:2cd4:b0:9ca:e7ce:8e5f with SMTP id hg20-20020a1709072cd400b009cae7ce8e5fmr2315344ejc.25.1698137440645; Tue, 24 Oct 2023 01:50:40 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id m16-20020a1709066d1000b009a1a5a7ebacsm7758913ejr.201.2023.10.24.01.50.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Oct 2023 01:50:40 -0700 (PDT) From: Andrew Burgess To: Carl Love , gdb-patches@sourceware.org, UlrichWeigand Cc: cel@us.ibm.com Subject: RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp In-Reply-To: <240a25512793955b83d82d90cd6d4f0ce09c564e.camel@us.ibm.com> References: <6f9c8fa20962129048384d74f6f15d1b37d255ed.camel@us.ibm.com> <76b8ed7b93608d40ab42b0538319f78eaf7d621c.camel@us.ibm.com> <87bkcyhc5g.fsf@redhat.com> <240a25512793955b83d82d90cd6d4f0ce09c564e.camel@us.ibm.com> Date: Tue, 24 Oct 2023 09:50:38 +0100 Message-ID: <87y1fs2yld.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,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: Carl Love writes: > Andrew: > > Thanks for the review and comments. See responses below. Carl, Sorry for my slow reply. > > 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. I'm now even more confused. The commit message for the first patch suggests that it contains two separate fixes, but the commit message, even your updated commit message for 'ver2' only mentions store.exp. To be more specific, in patch 1/2, the changes in ppc-linux-tdep.c seem unrelated to the changes in infrun.c. And I'm confused about how the changes in infrun.c can fix anything in the store.exp test script -- the comments relating to that part of the patch specifically talk about 'nexti' and signal handlers, but I don't see either of these being used in store.exp (we do use 'next' though, so maybe that's what you mean?) But I'm not clear if a signal is invoked in that test at all. In the text above you do mention step.exp, so maybe that's what the infrun.c changes are fixing? I'll grab a PPC machine and have a play, but your 'ver 2' commit message has no mention of step.exp, so if that test is important you might need to update the text. Usually in GDB when we talk about each patch containing a single fix we're talking about changes to GDB functionality, not fixing a test script. So it's not: 'Patch 1/1 fixes all of store.exp' but rather, 'Patch 1/2 fixes PPC DWARF register mapping' and 'Patch 2/2 fixes nexti blah blah signal frames'. >> >> > 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. Thanks. > > >> >> > 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. I took a quick look. There are three callers: 1 in dwarf2/loc.c, and 2 in jit.c. One of the callers in jit.c does fail to check the return value, but this looks like a bug to me. The other callers do check for a -1 return value. The comment in gdbarch-gen.h is also explicit that the right thing to do is return -1. Coupled with the fact that the comment, in its current location just makes no sense I think the best idea would be to drop it from your new function. Cleaning up the older code in the future would be nice, but is not required. But I don't think we need to introduce more incorrect comments just for consistency. Thanks, Andrew