[PATCH][PR breakpoints/27009] s390: Fix BC instruction breakpoint handling

[PATCH][PR breakpoints/27009] s390: Fix BC instruction breakpoint handling

[Giancarlo Frix]

Hi all,

The following patch fixes a bug in the s390/s390x port (reported as
PR breakpoints/27009) that would sometimes crash the inferior when
displaced stepping was being used.

I'm not sure if my company has filed copyright papers yet, but
since the change is only one line and this is my (and my company's)
first contribution to an upstream GNU project, it shouldn't count as
legally significant for copyright, so they shouldn't be necessary.

Giancarlo Frix
------------------------------
From 003a877b1b706832c280e9534d08b34481985b78 Mon Sep 17 00:00:00 2001
From: Giancarlo Frix <gfrix@rocketsoftware.com>
Date: Thu, 3 Dec 2020 18:52:22 -0500
Subject: [PATCH] s390: Fix BC instruction breakpoint handling

This fixes a long-lived bug in the s390 port.

When trying to step over a breakpoint set on a BC (branch on condition)
instruction with displaced stepping on IBM Z, gdb would incorrectly
adjust the pc regardless of whether or not the branch was taken. Since
the branch target is an absolute address, this would cause the inferior
to jump around wildly whenever the branch was taken, either crashing it
or causing it to behave unpredictably.

It turns out that the logic to handle BC instructions correctly was in
the code, but that the enum value representing its opcode has always
been incorrect.

This patch corrects the enum value to the actual opcode, fixing the
stepping problem. The enum value is also used in the prologue analysis
code, so this also fixes a minor bug where more of the prologue would
be read than was necessary.

gdb/ChangeLog:

2020-12-03  Giancarlo Frix  <gfrix@rocketsoftware.com>

PR breakpoints/27009
* s390-tdep.h (op_bc): Correct BC opcode value.
---
 gdb/s390-tdep.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/s390-tdep.h b/gdb/s390-tdep.h
index c0ea9cf6dc..fc8918fe77 100644
--- a/gdb/s390-tdep.h
+++ b/gdb/s390-tdep.h
@@ -117,7 +117,7 @@ enum
   op_basr  = 0x0d,
   op_bas   = 0x4d,
   op_bcr   = 0x07,
-  op_bc    = 0x0d,
+  op_bc    = 0x47,
   op_bctr  = 0x06,
   op_bctgr = 0xb946,
   op_bct   = 0x46,
--
2.23.0

================================
Rocket Software, Inc. and subsidiaries ■ 77 Fourth Avenue, Waltham MA 02451 ■ Main Office Toll Free Number: +1 855.577.4323
Contact Customer Support: https://my.rocketsoftware.com/RocketCommunity/RCEmailSupport
Unsubscribe from Marketing Messages/Manage Your Subscription Preferences - http://www.rocketsoftware.com/manage-your-email-preferences
Privacy Policy - http://www.rocketsoftware.com/company/legal/privacy-policy
================================

This communication and any attachments may contain confidential information of Rocket Software, Inc. All unauthorized use, disclosure or distribution is prohibited. If you are not the intended recipient, please notify Rocket Software immediately and destroy all copies of this communication. Thank you.

Re: [PATCH][PR breakpoints/27009] s390: Fix BC instruction breakpoint handling

[Joel Brobecker]

Hello Giancarlo,

On Fri, Dec 04, 2020 at 01:55:09AM +0000, Giancarlo Frix via Gdb-patches wrote:
> Hi all,
> 
> The following patch fixes a bug in the s390/s390x port (reported as
> PR breakpoints/27009) that would sometimes crash the inferior when
> displaced stepping was being used.
> 
> I'm not sure if my company has filed copyright papers yet, but
> since the change is only one line and this is my (and my company's)
> first contribution to an upstream GNU project, it shouldn't count as
> legally significant for copyright, so they shouldn't be necessary.

Agreed. I did a quick search of the FSF records, and could not
find your company, but the patch is very small and completely
obvious.

What I did was, as a formality, double-checked that the opcode
is indeed correct (note also that 0xd was already used by op_basr
so I suspect you'll be seeing some improvements with the handling
of that operand too). From there, I pushed the patch to master
after having added an entry in gdb/ChangeLog. And since the fix
was obvious while the effect of the bug potentially very confusing,
and the fix already had a PR attached to it, I backported the patch
to the gdb-10-branch also. So the fix will be in the GDB 10.2 release.

Thanks for the patch!

> 
> Giancarlo Frix
> ------------------------------
> From 003a877b1b706832c280e9534d08b34481985b78 Mon Sep 17 00:00:00 2001
> From: Giancarlo Frix <gfrix@rocketsoftware.com>
> Date: Thu, 3 Dec 2020 18:52:22 -0500
> Subject: [PATCH] s390: Fix BC instruction breakpoint handling
> 
> This fixes a long-lived bug in the s390 port.
> 
> When trying to step over a breakpoint set on a BC (branch on condition)
> instruction with displaced stepping on IBM Z, gdb would incorrectly
> adjust the pc regardless of whether or not the branch was taken. Since
> the branch target is an absolute address, this would cause the inferior
> to jump around wildly whenever the branch was taken, either crashing it
> or causing it to behave unpredictably.
> 
> It turns out that the logic to handle BC instructions correctly was in
> the code, but that the enum value representing its opcode has always
> been incorrect.
> 
> This patch corrects the enum value to the actual opcode, fixing the
> stepping problem. The enum value is also used in the prologue analysis
> code, so this also fixes a minor bug where more of the prologue would
> be read than was necessary.
> 
> gdb/ChangeLog:
> 
> 2020-12-03  Giancarlo Frix  <gfrix@rocketsoftware.com>
> 
> PR breakpoints/27009
> * s390-tdep.h (op_bc): Correct BC opcode value.


> ---
>  gdb/s390-tdep.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/s390-tdep.h b/gdb/s390-tdep.h
> index c0ea9cf6dc..fc8918fe77 100644
> --- a/gdb/s390-tdep.h
> +++ b/gdb/s390-tdep.h
> @@ -117,7 +117,7 @@ enum
>    op_basr  = 0x0d,
>    op_bas   = 0x4d,
>    op_bcr   = 0x07,
> -  op_bc    = 0x0d,
> +  op_bc    = 0x47,
>    op_bctr  = 0x06,
>    op_bctgr = 0xb946,
>    op_bct   = 0x46,
> --
> 2.23.0
> 
> ================================
> Rocket Software, Inc. and subsidiaries ■ 77 Fourth Avenue, Waltham MA 02451 ■ Main Office Toll Free Number: +1 855.577.4323
> Contact Customer Support: https://my.rocketsoftware.com/RocketCommunity/RCEmailSupport
> Unsubscribe from Marketing Messages/Manage Your Subscription Preferences - http://www.rocketsoftware.com/manage-your-email-preferences
> Privacy Policy - http://www.rocketsoftware.com/company/legal/privacy-policy
> ================================
> 
> This communication and any attachments may contain confidential information of Rocket Software, Inc. All unauthorized use, disclosure or distribution is prohibited. If you are not the intended recipient, please notify Rocket Software immediately and destroy all copies of this communication. Thank you.

-- 
Joel