[PATCH master + 11] gdb: don't set Linux-specific displaced stepping methods in s390_gdbarch_init

[PATCH master + 11] gdb: don't set Linux-specific displaced stepping methods in s390_gdbarch_init

[Simon Marchi]

According to bug 28056, running an s390x binary gives:

    (gdb) run
    Starting program: /usr/bin/ls
    /home/ubuntu/tmp/gdb-11.0.90.20210705/gdb/linux-tdep.c:2550: internal-error: displaced_step_prepare_status linux_displaced_step_prepare(gdbarch*, thread_info*, CORE_ADDR&): Assertion `gdbarch_data->num_disp_step_buffers > 0' failed.

This is because the s390 architecture registers some Linux-specific
displaced stepping callbacks in the OS-agnostic s390_gdbarch_init:

    set_gdbarch_displaced_step_prepare (gdbarch, linux_displaced_step_prepare);
    set_gdbarch_displaced_step_finish (gdbarch, linux_displaced_step_finish);
    set_gdbarch_displaced_step_restore_all_in_ptid
      (gdbarch, linux_displaced_step_restore_all_in_ptid);

But then the Linux-specific s390_linux_init_abi_any passes
num_disp_step_buffers=0 to linux_init_abi:

    linux_init_abi (info, gdbarch, 0);

The problem happens when linux_displaced_step_prepare is called for the
first time.  It tries to allocate the displaced stepping buffers, but
sees that the number of displaced stepping buffers for that architecture
is 0, which is unexpected / invalid.

s390_gdbarch_init should not register the linux_* callbacks, that is
expected to be done by linux_init_abi.  If debugging a bare-metal s390
program, or an s390 program on another OS GDB doesn't know about, we
wouldn't want to use them.  We would either register no callbacks, if
displaced stepping isn't supported, or register a different set of
callbacks if we wanted to support displaced stepping in those cases.

The commit that refactored the displaced stepping machinery and
introduced these set_gdbarch_displaced_step_* calls is 187b041e2514
("gdb: move displaced stepping logic to gdbarch, allow starting
concurrent displaced steps").  However, even before that,
s390_gdbarch_init did:

  set_gdbarch_displaced_step_location (gdbarch, linux_displaced_step_location);

... which already seemed wrong.  The Linux-specific callback was used
even for non-Linux system.  Maybe that was on purpose, because it would
also happen to work in some other non-Linux case, or maybe it was simply
a mistake.  I'll assume that this was a small mistake when
s390-tdep.{h,c} where factored out of s390-linux-tdep.c, in d6e589456475
("s390: Split up s390-linux-tdep.c into two files").

Fix this by removing the setting of these displaced step callbacks from
s390_gdbarch_init.  Instead, pass num_disp_step_buffers=1 to
linux_init_abi, in s390_linux_init_abi_any.  Doing so will cause
linux_init_abi to register these same callbacks.  It will also mean that
when debugging a bare-metal s390 executable or an executable on another
OS that GDB doesn't know about, gdbarch_displaced_step_prepare won't be
set, so displaced stepping won't be used.

This patch will need to be merged in the gdb-11-branch, since this is a
GDB 11 regression, so here's the ChangeLog entry:

gdb/ChangeLog:

	* s390-linux-tdep.c (s390_linux_init_abi_any): Pass 1 (number
	of displaced stepping buffers to linux_init_abi.
	* s390-tdep.c (s390_gdbarch_init): Don't set the Linux-specific
	displaced-stepping gdbarch callbacks.

Change-Id: Ieab2f8990c78fde845ce7378d6fd4ee2833800d5
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28056
---
 gdb/s390-linux-tdep.c | 2 +-
 gdb/s390-tdep.c       | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 04a4ff8df34a..1f40e10e3d38 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -1120,7 +1120,7 @@ s390_linux_init_abi_any (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   tdep->s390_syscall_record = s390_linux_syscall_record;
 
-  linux_init_abi (info, gdbarch, 0);
+  linux_init_abi (info, gdbarch, 1);
 
   /* Register handling.  */
   set_gdbarch_core_read_description (gdbarch, s390_core_read_description);
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 2579ee82b203..d3bb2bacbb40 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -7050,10 +7050,6 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_copy_insn (gdbarch,
 					s390_displaced_step_copy_insn);
   set_gdbarch_displaced_step_fixup (gdbarch, s390_displaced_step_fixup);
-  set_gdbarch_displaced_step_prepare (gdbarch, linux_displaced_step_prepare);
-  set_gdbarch_displaced_step_finish (gdbarch, linux_displaced_step_finish);
-  set_gdbarch_displaced_step_restore_all_in_ptid
-    (gdbarch, linux_displaced_step_restore_all_in_ptid);
   set_gdbarch_displaced_step_hw_singlestep (gdbarch, s390_displaced_step_hw_singlestep);
   set_gdbarch_software_single_step (gdbarch, s390_software_single_step);
   set_gdbarch_max_insn_length (gdbarch, S390_MAX_INSTR_SIZE);
-- 
2.32.0

Re: [PATCH master + 11] gdb: don't set Linux-specific displaced stepping methods in s390_gdbarch_init

[Simon Marchi]

On 2021-07-07 8:57 a.m., Simon Marchi wrote:
> According to bug 28056, running an s390x binary gives:
> 
>     (gdb) run
>     Starting program: /usr/bin/ls
>     /home/ubuntu/tmp/gdb-11.0.90.20210705/gdb/linux-tdep.c:2550: internal-error: displaced_step_prepare_status linux_displaced_step_prepare(gdbarch*, thread_info*, CORE_ADDR&): Assertion `gdbarch_data->num_disp_step_buffers > 0' failed.
> 
> This is because the s390 architecture registers some Linux-specific
> displaced stepping callbacks in the OS-agnostic s390_gdbarch_init:
> 
>     set_gdbarch_displaced_step_prepare (gdbarch, linux_displaced_step_prepare);
>     set_gdbarch_displaced_step_finish (gdbarch, linux_displaced_step_finish);
>     set_gdbarch_displaced_step_restore_all_in_ptid
>       (gdbarch, linux_displaced_step_restore_all_in_ptid);
> 
> But then the Linux-specific s390_linux_init_abi_any passes
> num_disp_step_buffers=0 to linux_init_abi:
> 
>     linux_init_abi (info, gdbarch, 0);
> 
> The problem happens when linux_displaced_step_prepare is called for the
> first time.  It tries to allocate the displaced stepping buffers, but
> sees that the number of displaced stepping buffers for that architecture
> is 0, which is unexpected / invalid.
> 
> s390_gdbarch_init should not register the linux_* callbacks, that is
> expected to be done by linux_init_abi.  If debugging a bare-metal s390
> program, or an s390 program on another OS GDB doesn't know about, we
> wouldn't want to use them.  We would either register no callbacks, if
> displaced stepping isn't supported, or register a different set of
> callbacks if we wanted to support displaced stepping in those cases.
> 
> The commit that refactored the displaced stepping machinery and
> introduced these set_gdbarch_displaced_step_* calls is 187b041e2514
> ("gdb: move displaced stepping logic to gdbarch, allow starting
> concurrent displaced steps").  However, even before that,
> s390_gdbarch_init did:
> 
>   set_gdbarch_displaced_step_location (gdbarch, linux_displaced_step_location);
> 
> ... which already seemed wrong.  The Linux-specific callback was used
> even for non-Linux system.  Maybe that was on purpose, because it would
> also happen to work in some other non-Linux case, or maybe it was simply
> a mistake.  I'll assume that this was a small mistake when
> s390-tdep.{h,c} where factored out of s390-linux-tdep.c, in d6e589456475
> ("s390: Split up s390-linux-tdep.c into two files").
> 
> Fix this by removing the setting of these displaced step callbacks from
> s390_gdbarch_init.  Instead, pass num_disp_step_buffers=1 to
> linux_init_abi, in s390_linux_init_abi_any.  Doing so will cause
> linux_init_abi to register these same callbacks.  It will also mean that
> when debugging a bare-metal s390 executable or an executable on another
> OS that GDB doesn't know about, gdbarch_displaced_step_prepare won't be
> set, so displaced stepping won't be used.
> 
> This patch will need to be merged in the gdb-11-branch, since this is a
> GDB 11 regression, so here's the ChangeLog entry:
> 
> gdb/ChangeLog:
> 
> 	* s390-linux-tdep.c (s390_linux_init_abi_any): Pass 1 (number
> 	of displaced stepping buffers to linux_init_abi.
> 	* s390-tdep.c (s390_gdbarch_init): Don't set the Linux-specific
> 	displaced-stepping gdbarch callbacks.

Hi Philipp,

Since you did the s390-tdep.c / s390-linux-tdep.c split, could you
please take a look at this patch, see if it looks correct to you?

Thanks!

Simon

Re: [PATCH master + 11] gdb: don't set Linux-specific displaced stepping methods in s390_gdbarch_init

[Andreas Arnez]

On Wed, Jul 07 2021, Simon Marchi via Gdb-patches wrote:

[...]

> The Linux-specific callback was used
> even for non-Linux system.  Maybe that was on purpose, because it would
> also happen to work in some other non-Linux case, or maybe it was simply
> a mistake.  I'll assume that this was a small mistake when
> s390-tdep.{h,c} where factored out of s390-linux-tdep.c, in d6e589456475
> ("s390: Split up s390-linux-tdep.c into two files").

I agree that this is probably a mistake.  It didn't cause any trouble
right away, because GDB doesn't really support any other s390 OSs.  One
possible additional "OS" would have been the Linux kernel runtime, but
that hasn't manifested yet.

> Fix this by removing the setting of these displaced step callbacks from
> s390_gdbarch_init.  Instead, pass num_disp_step_buffers=1 to
> linux_init_abi, in s390_linux_init_abi_any.  Doing so will cause
> linux_init_abi to register these same callbacks.  It will also mean that
> when debugging a bare-metal s390 executable or an executable on another
> OS that GDB doesn't know about, gdbarch_displaced_step_prepare won't be
> set, so displaced stepping won't be used.

Sounds good to me, and the patch looks good as well.  Thanks!

--
Andreas

Re: [PATCH master + 11] gdb: don't set Linux-specific displaced stepping methods in s390_gdbarch_init

[Simon Marchi]

On 2021-07-07 11:08 a.m., Andreas Arnez wrote:
> On Wed, Jul 07 2021, Simon Marchi via Gdb-patches wrote:
> 
> [...]
> 
>> The Linux-specific callback was used
>> even for non-Linux system.  Maybe that was on purpose, because it would
>> also happen to work in some other non-Linux case, or maybe it was simply
>> a mistake.  I'll assume that this was a small mistake when
>> s390-tdep.{h,c} where factored out of s390-linux-tdep.c, in d6e589456475
>> ("s390: Split up s390-linux-tdep.c into two files").
> 
> I agree that this is probably a mistake.  It didn't cause any trouble
> right away, because GDB doesn't really support any other s390 OSs.  One
> possible additional "OS" would have been the Linux kernel runtime, but
> that hasn't manifested yet.
> 
>> Fix this by removing the setting of these displaced step callbacks from
>> s390_gdbarch_init.  Instead, pass num_disp_step_buffers=1 to
>> linux_init_abi, in s390_linux_init_abi_any.  Doing so will cause
>> linux_init_abi to register these same callbacks.  It will also mean that
>> when debugging a bare-metal s390 executable or an executable on another
>> OS that GDB doesn't know about, gdbarch_displaced_step_prepare won't be
>> set, so displaced stepping won't be used.
> 
> Sounds good to me, and the patch looks good as well.  Thanks!

Thanks for checking, I pushed it to both branches!

Simon