From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by sourceware.org (Postfix) with ESMTPS id 7798E395440D for ; Fri, 6 May 2022 16:16:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7798E395440D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f41.google.com with SMTP id a14-20020a7bc1ce000000b00393fb52a386so7128276wmj.1 for ; Fri, 06 May 2022 09:16:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=gYUFOZBabW/HOTosTncHX+Og9NrCNcNDcfrARIcSZAU=; b=3pq0aAtcS/ARH31FSz6tD35irEH37qq3c/13uyafMhIC6tq9Q98fPMV88egI9bPt/Y WmnuatqVrE2WrcM7+64wYxCtwzAu0egAMRmxLKk7I6D+LOtwhFUedF0MLmPyWLGcV/3/ CFnk5lZHoMJEvmIW/bt4jcy11JQTTG8UTnkVAlk0aPTE7vT32psdE8A+WBrq943JipvO 6Qc9hdP62nh7QfReoSRLacqZ5J18Rhmq4OYsG7ceZbNFJXIQn4aCTqomU3KZRmriBtZk Of3T6scd9LrfBKr1RvNx8JlWSz+RB4tIXMoOX74Xd/QS5Sh5o1Zrl5aTUWrn9Z82j7F4 VLnQ== X-Gm-Message-State: AOAM530xOuG68Y72BV+qQBuWn3dg0xbrs5L/d4Ys2Hi2+HNBql/YjdFM KArjoyQk3XupA1ypzU5rWWI= X-Google-Smtp-Source: ABdhPJwoZ4jSOV17M0+yts9kcF6QxHKEPVlKNBT0KmTg+bZAMB21T0IyzXbKq5TisFeJ7MbO/9NoVw== X-Received: by 2002:a05:600c:1d9f:b0:394:7a51:cb8b with SMTP id p31-20020a05600c1d9f00b003947a51cb8bmr3594195wms.163.1651853818322; Fri, 06 May 2022 09:16:58 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id e13-20020a5d530d000000b0020c5253d8fasm4028218wrv.70.2022.05.06.09.16.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 06 May 2022 09:16:57 -0700 (PDT) Message-ID: Date: Fri, 6 May 2022 17:16:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH] PowerPC: bp-permanent.exp, kill-after-signal fix Content-Language: en-US To: Carl Love , gdb-patches@sourceware.org Cc: Ulrich Weigand , Rogerio Alves , Will Schmidt References: <28b5ef68-951f-00db-ae0f-9acdbf12f382@palves.net> From: Pedro Alves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LOTSOFHASH, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 May 2022 16:17:01 -0000 On 2022-05-03 21:10, Carl Love wrote: > The updated patch has been tested on Power 9 and Power 10 with linux > 5.14 or newer kernels to verify the patch works and does not generate > regressions on PowerPC. The patch was also tested on Intel to ensure > the patch didn't create any unintended regressions on that platform. > > Please let me know if the updated patch is acceptable. Thanks. > > Carl Love > > --------------------------------------------------- > > PowerPC: bp-permanent.exp, kill-after-signal fix > > The break point after the stepi on Intel is the entry point of the user > signal handler function test_signal_handler. The code at the break point > looks like: > > 0x : endbr64 > > On PowerPC with a Linux 5.9 kernel or latter, the address where gdb stops > after the stepi is in the vdso code inserted by the kernel. The code at the > breakpoint looks like: > > 0x <__kernel_start_sigtramp_rt64>: bctrl > > This is different from other architectures. As dicussed below, recent kernel dicussed -> discussed > changes involving the vdso for PowerPC have been made changes to the signal > handler code flow. PowerPC is now stoping in function stoping -> stopping > __kernel_start_sigtramp_rt64. Powerpc now requires an additional stepi to Powerpc -> PowerPC > reach the user signal handler unlike other architectures. > > The bp-permanent.exp and kill-after-signal tests run fine on PowerPC with an > kernel that is older than Linux 5.9. > > The Powerpc 64 signal handler was updated by the Linux kernel 5.9-rc1: Powerpc -> PowerPC > > commit id: 0138ba5783ae0dcc799ad401a1e8ac8333790df9 > powerpc/64/signal: Balance return predictor stack in signal trampoline > > An additional change to the Powerpc 64 signal handler was made in Linux kernel > version 5.11-rc7 : Powerpc -> PowerPC > > commit id: 24321ac668e452a4942598533d267805f291fdc9 > powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics > > The first kernel change, puts code into the user space signal handler (in the > vdso) as a performance optimization to prevent the call/return stack from > getting out of balance. The patch ensures that the entire user/kernel/vdso > cycle is balanced with the addition of the "brctl" instruction. > > The second patch, fixes the semantics of __kernel_sigtramp_rt64(). A new > symbol is introduced to serve as the jump target from the kernel to the > trampoline which now consists of two parts. > > The above changes for PowerPC signal handler, causes gdb to stop in the kernel > code not the user signal handler as expected. The kernel dispatches to the > vdso code which in turn calls into the signal handler. PowerPC is special in > that the kernel is using a vdso instruction (bctrl) to enter the signal handler. > > I do not have access to a system with the first patch but not the second. I did > test on Power 9 with the Linux 5.15.0-27-generic kernel. Both tests fail on > this Power 9 system. The two tests also fail on Power 10 with the Linux > 5.14.0-70.9.1.el9_0.ppc64le kernel. > > The following patch fixes the issue by checking if gdb stoped at "signal handler stoped -> stopped > called". If gdb stopped there, the tests verifies gdb is in the kernel function > __kernel_start_sigtramp_rt64 then does an additional stepi to reach the user > signal handler. With the patch below, the tests run without erros on both the > Power 9 and Power 10 systems with out any failures. erros -> errors > --- > gdb/testsuite/gdb.base/bp-permanent.exp | 25 +++++++++++++++ > gdb/testsuite/gdb.base/kill-after-signal.exp | 33 +++++++++++++++++++- > 2 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp > index 8be46e96238..11f5562fcc6 100644 > --- a/gdb/testsuite/gdb.base/bp-permanent.exp > +++ b/gdb/testsuite/gdb.base/bp-permanent.exp > @@ -260,6 +260,31 @@ proc test {always_inserted sw_watchpoint} { > -re "Program received signal SIGTRAP.*$gdb_prompt $" { > fail $test > } > + -re ".*signal handler called.*$gdb_prompt $" { > + # PowerPC Linux kernel patchs: patchs -> patch Maybe say "after", and "commit", as in: # After PowerPC Linux kernel commit: # # commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9 ... > + # commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9 > + # powerpc/64/signal: Balance return predictor > + # stack in signal trampoline. > + # > + # the kernel places an additional brctl instruction > + # in the vdso to call the user hadler. hadler -> handler > + # > + # commit 24321ac668e452a4942598533d267805f291fdc9 > + # powerpc/64/signal: Fix regression in > + # __kernel_sigtramp_rt64() semantics > + # > + # Updates the semantics of __kernel_sigtramp_rt64(). Say: # And then this commit: # # commit 24321ac668e452a4942598533d267805f291fdc9 # powerpc/64/signal: Fix regression in # __kernel_sigtramp_rt64() semantics # # updated the semantics of __kernel_sigtramp_rt64(). (note lowercase "updated") > + # It adds a new symbol to serve as a jump target from "It adds" -> "It added" > + # the kernel to the trampoline. > + # > + # The net result of these changes is that gdb stops > + # at __kernel_start_sigtramp_rt64. Need to do one > + # more stepi to reach the expected location in the user > + # signal handler. > + gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" \ > + "in kernel code" FYI, leading ".*" is never needed with gdb_test or gdb_test_multiple. You can just write: gdb_test "p \$pc" "__kernel_start_sigtramp_rt64.*" \ "in kernel code" > + gdb_test "stepi" "handler .*" $test > + } > -re "handler .*$gdb_prompt $" { > pass $test > } > diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp > index 09f5cbc39a6..1bf5a3e5789 100644 > --- a/gdb/testsuite/gdb.base/kill-after-signal.exp > +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp > @@ -36,7 +36,38 @@ if ![runto_main] { > } > > gdb_test "continue" "Program received signal SIGUSR1, .*" > -gdb_test "stepi" "\r\nhandler .*" > + > +set test "handler" > +gdb_test_multiple "stepi" $test { > + -re "\r\nhandler .*" { > + pass $test > + } > + -re ".*signal handler called.*$gdb_prompt $" { No need for leading ".*". Note -wrap would be a little shorter: -re -wrap "signal handler called.*" { > + # PowerPC Linux kernel patchs: > + # commit: 0138ba5783ae0dcc799ad401a1e8ac8333790df9 > + # powerpc/64/signal: Balance return predictor > + # stack in signal trampoline. > + # > + # the kernel places an additional brctl instruction > + # in the vdso to call the user hadler. > + # > + # commit 24321ac668e452a4942598533d267805f291fdc9 > + # powerpc/64/signal: Fix regression in > + # __kernel_sigtramp_rt64() semantics > + # > + # Updates the semantics of __kernel_sigtramp_rt64(). > + # It adds a new symbol to serve as a jump target from > + # the kernel to the trampoline. > + # > + # The net result of these changes is that gdb stops > + # at __kernel_start_sigtramp_rt64. Need to do one > + # more stepi to reach the expected location in the user > + # signal handler. Please update this comment too per comments above. > + gdb_test "p \$pc" ".*__kernel_start_sigtramp_rt64.*" "in kernel code" > + gdb_test "stepi" "\r\nhandler .*" $test > + } > +} > + OK with those changes. Thanks.