Merge ~liushuyu-011/ubuntu/+source/gdb:ubuntu/jammy-devel into ubuntu/+source/gdb:ubuntu/jammy-devel

[Zixing Liu]

This merge proposal backports several patches from GDB 13 to fix an issue on armhf where GDB will break the program executed due to incorrectly placed debug instructions in the process memory.

You can find the full details on this issue here: https://bugs.launchpad.net/gdb/+bug/2041396.

[Sergio Durigan Junior (sergiodj)]

Thanks for the MP, Zixing.

I've left some minor requests below. Overall the changes LGTM, but having a PPA where I could find the package with the fix ready to be test would have helped. Either way, I'm building it now and reserving an ARM64 machine to perform the test before sponsoring the upload.

The SRU text is also OK. I'll let you know for sure after I perform the tests.

Meanwhile, you can work on addressing the comments below. Thanks.

review: Needs Fixing

[Zixing Liu (liushuyu-011)]

Hi Sergio,

I do have a PPA build for this: https://launchpad.net/~liushuyu-011/+archive/ubuntu/misc/+build/26929623. Launchpad was having issues yesterday, so I did not post the links. Sorry for the inconvenience.

[Zixing Liu (liushuyu-011)]

Done. Comments addressed.

[Sergio Durigan Junior (sergiodj)]

Ah, thanks Zinxing. I see that the build has failed on all architectures but armhf. Could you retrigger those just to make sure that they're OK, please?

I'm still struggling to obtain access to an arm64 machine where I can test these changes. I might go ahead and upload it, though, because my previous experience with machine reservation on the internal MAAS is not great.

[Sergio Durigan Junior (sergiodj)]

Uploaded, thanks:

$ dput gdb_12.1-0ubuntu1~22.04.1_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/gdb/gdb_12.1-0ubuntu1~22.04.1_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/gdb/gdb_12.1-0ubuntu1~22.04.1.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading gdb_12.1-0ubuntu1~22.04.1.dsc: done.
  Uploading gdb_12.1-0ubuntu1~22.04.1.debian.tar.xz: done.
  Uploading gdb_12.1-0ubuntu1~22.04.1_source.buildinfo: done.
  Uploading gdb_12.1-0ubuntu1~22.04.1_source.changes: done.
Successfully uploaded packages.

review: Approve

[Zixing Liu (liushuyu-011)]

> Could you retrigger those just to make sure that they're OK, please?

Retrying. Launchpad has been under stress for the past few days, so it may take a few tries.

[Sergio Durigan Junior (sergiodj)]

On Friday, October 27 2023, Zixing Liu wrote:

>> Could you retrigger those just to make sure that they're OK, please?
>
> Retrying. Launchpad has been under stress in the pass few days, so it may take a few tries.

Thanks!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

diff --git a/debian/changelog b/debian/changelog
index 366bc0f..ce4d5a2 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+gdb (12.1-0ubuntu1~22.04.1) jammy; urgency=medium
+
+  * SRU: LP: #2041396: Fix SIGILL generated by GDB 12.1 on armhf by
+    backporting the fix from GDB 13.1 to 22.04 LTS.
+
+ -- Zixing Liu <[e-mail]>  Thu, 26 Oct 2023 12:04:57 -0600
+
 gdb (12.1-0ubuntu1~22.04) jammy-proposed; urgency=medium
 
   * SRU: LP: #1971474: Update to the final 12.1 release in 22.04 LTS.
diff --git a/debian/control b/debian/control
index fc692a2..283d41b 100644
--- a/debian/control
+++ b/debian/control
@@ -1,5 +1,6 @@
 Source: gdb
-Maintainer: Héctor Orón Martínez <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Héctor Orón Martínez <[e-mail]>
 Uploaders: Riku Voipio <[e-mail]>,
            Sergio Durigan Junior <[e-mail]>
 Section: devel
diff --git a/debian/control.in b/debian/control.in
index de505b6..1f6032a 100644
--- a/debian/control.in
+++ b/debian/control.in
@@ -1,5 +1,6 @@
 Source: gdb
-Maintainer: Héctor Orón Martínez <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Héctor Orón Martínez <[e-mail]>
 Uploaders: Riku Voipio <[e-mail]>,
            Sergio Durigan Junior <[e-mail]>
 Section: devel
diff --git a/debian/patches/Make-sure-a-copy_insn_closure-is-available-when-we-h.patch b/debian/patches/Make-sure-a-copy_insn_closure-is-available-when-we-h.patch
new file mode 100644
index 0000000..95bf1fd
--- /dev/null
+++ b/debian/patches/Make-sure-a-copy_insn_closure-is-available-when-we-h.patch
@@ -0,0 +1,103 @@
+From da6edc2feb66d75b89f65834d8bccad45c47525c Mon Sep 17 00:00:00 2001
+From: Luis Machado <[e-mail]>
+Date: Tue, 25 Oct 2022 11:01:32 +0100
+Subject: [PATCH 1/2] Make sure a copy_insn_closure is available when we have a
+ match in copy_insn_closure_by_addr
+
+PR gdb/29272
+
+Investigating PR29272, it was mentioned a particular test used to work on
+GDB 10, but it started failing with GDB 11 onwards. I tracked it down to
+some displaced stepping improvements on commit
+187b041e2514827b9d86190ed2471c4c7a352874.
+
+In particular, one of the corner cases using copy_insn_closure_by_addr got
+silently broken. It is hard to spot because it doesn't have any good tests
+for it, and the situation is quite specific to the Arm target.
+
+Essentially, the change from the displaced stepping improvements made it so
+we could still invoke copy_insn_closure_by_addr correctly to return the
+pointer to a copy_insn_closure, but it always returned nullptr due to
+the order of the statements in displaced_step_buffer::prepare.
+
+The way it is now, we first write the address of the displaced step buffer
+to PC and then save the copy_insn_closure pointer.
+
+The problem is that writing to PC for the Arm target requires figuring
+out if the new PC is thumb mode or not.
+
+With no copy_insn_closure data, the logic to determine the thumb mode
+during displaced stepping doesn't work, and gives random results that
+are difficult to track (SIGILL, SIGSEGV etc).
+
+Fix this by reordering the PC write in displaced_step_buffer::prepare
+and, for safety, add an assertion to
+displaced_step_buffer::copy_insn_closure_by_addr so GDB stops right
+when it sees this invalid situation. If this gets broken again in the
+future, it will be easier to spot.
+
+Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29272
+Approved-By: Simon Marchi <[e-mail]>
+Origin: upstream
+Applied-Upstream: 13.1, https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679
+Last-Update: 2023-10-26
+---
+ gdb/displaced-stepping.c | 28 +++++++++++++++++++++++++---
+ 1 file changed, 25 insertions(+), 3 deletions(-)
+
+diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
+index e2afbe42f58..d73a875afbb 100644
+--- a/gdb/displaced-stepping.c
++++ b/gdb/displaced-stepping.c
+@@ -139,15 +139,33 @@ displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
+       return DISPLACED_STEP_PREPARE_STATUS_CANT;
+     }
+ 
+-  /* Resume execution at the copy.  */
+-  regcache_write_pc (regcache, buffer->addr);
+-
+   /* This marks the buffer as being in use.  */
+   buffer->current_thread = thread;
+ 
+   /* Save this, now that we know everything went fine.  */
+   buffer->copy_insn_closure = std::move (copy_insn_closure);
+ 
++  /* Reset the displaced step buffer state if we failed to write PC.
++     Otherwise we will prevent this buffer from being used, as it will
++     always have a thread in buffer->current_thread.  */
++  auto reset_buffer = make_scope_exit
++    ([buffer] ()
++      {
++	buffer->current_thread = nullptr;
++	buffer->copy_insn_closure.reset ();
++      });
++
++  /* Adjust the PC so it points to the displaced step buffer address that will
++     be used.  This needs to be done after we save the copy_insn_closure, as
++     some architectures (Arm, for one) need that information so they can adjust
++     other data as needed.  In particular, Arm needs to know if the instruction
++     being executed in the displaced step buffer is thumb or not.  Without that
++     information, things will be very wrong in a random way.  */
++  regcache_write_pc (regcache, buffer->addr);
++
++  /* PC update successful.  Discard the displaced step state rollback.  */
++  reset_buffer.release ();
++
+   /* Tell infrun not to try preparing a displaced step again for this inferior if
+      all buffers are taken.  */
+   thread->inf->displaced_step_state.unavailable = true;
+@@ -264,7 +282,11 @@ displaced_step_buffers::copy_insn_closure_by_addr (CORE_ADDR addr)
+   for (const displaced_step_buffer &buffer : m_buffers)
+     {
+       if (addr == buffer.addr)
++      {
++	/* The closure information should always be available. */
++	gdb_assert (buffer.copy_insn_closure.get () != nullptr);
+ 	return buffer.copy_insn_closure.get ();
++      }
+     }
+ 
+   return nullptr;
+-- 
+2.34.1
+
diff --git a/debian/patches/Only-allow-closure-lookup-by-address-if-there-are-th.patch b/debian/patches/Only-allow-closure-lookup-by-address-if-there-are-th.patch
new file mode 100644
index 0000000..dabba97
--- /dev/null
+++ b/debian/patches/Only-allow-closure-lookup-by-address-if-there-are-th.patch
@@ -0,0 +1,167 @@
+From a9734f5e1640f47a089a472ca9c3b7e0df5c77ee Mon Sep 17 00:00:00 2001
+From: Luis Machado <[e-mail]>
+Date: Thu, 28 Sep 2023 11:08:29 +0100
+Subject: [PATCH 2/2] Only allow closure lookup by address if there are threads
+ displaced-stepping
+
+Since commit 1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679, we have an assertion in
+displaced_step_buffers::copy_insn_closure_by_addr that makes sure a closure
+is available whenever we have a match between the provided address argument and
+the buffer address.
+
+That is fine, but the report in PR30872 shows this assertion triggering when
+it really shouldn't. After some investigation, here's what I found out.
+
+The 32-bit Arm architecture is the only one that calls
+gdbarch_displaced_step_copy_insn_closure_by_addr directly, and that's because
+32-bit Arm needs to figure out the thumb state of the original instruction
+that we displaced-stepped through the displaced-step buffer.
+
+Before the assertion was put in place by commit
+1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679, there was the possibility of
+getting nullptr back, which meant we were not doing a displaced-stepping
+operation.
+
+Now, with the assertion in place, this is running into issues.
+
+It looks like displaced_step_buffers::copy_insn_closure_by_addr is
+being used to return a couple different answers depending on the
+state we're in:
+
+1 - If we are actively displaced-stepping, then copy_insn_closure_by_addr
+is supposed to return a valid closure for us, so we can determine the
+thumb mode.
+
+2 - If we are not actively displaced-stepping, then copy_insn_closure_by_addr
+should return nullptr to signal that there isn't any displaced-step buffers
+in use, because we don't have a valid closure (but we should always have
+this).
+
+Since the displaced-step buffers are always allocated, but not always used,
+that means the buffers will always contain data. In particular, the buffer
+addr field cannot be used to determine if the buffer is active or not.
+
+For instance, we cannot set the buffer addr field to 0x0, as that can be a
+valid PC in some cases.
+
+My understanding is that the current_thread field should be a good candidate
+to signal that a particular displaced-step buffer is active or not. If it is
+nullptr, we have no threads using that buffer to displaced-step.  Otherwise,
+it is an active buffer in use by a particular thread.
+
+The following fix modifies the displaced_step_buffers::copy_insn_closure_by_addr
+function so we only attempt to return a closure if the buffer has an assigned
+current_thread and if the buffer address matches the address argument.
+
+Alternatively, I think we could use a function to answer the question of
+whether we're actively displaced-stepping (so we have an active buffer) or
+not.
+
+I've also added a testcase that exercises the problem. It should reproduce
+reliably on Arm, as that is the only architecture that faces this problem
+at the moment.
+
+Regression-tested on Ubuntu 20.04. OK?
+
+Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30872
+Approved-By: Simon Marchi <[e-mail]>
+Origin: upstream
+Applied-Upstream: 13.1, https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=5d4a870e05ac45e3f5a301c672a4079995b5db7a
+Last-Update: 2023-10-26
+---
+ gdb/displaced-stepping.c                      |  3 +-
+ .../gdb.base/displaced-step-closure.c         | 21 ++++++++++
+ .../gdb.base/displaced-step-closure.exp       | 39 +++++++++++++++++++
+ 3 files changed, 62 insertions(+), 1 deletion(-)
+ create mode 100644 gdb/testsuite/gdb.base/displaced-step-closure.c
+ create mode 100644 gdb/testsuite/gdb.base/displaced-step-closure.exp
+
+diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
+index d73a875afbb..006c3913098 100644
+--- a/gdb/displaced-stepping.c
++++ b/gdb/displaced-stepping.c
+@@ -281,7 +281,8 @@ displaced_step_buffers::copy_insn_closure_by_addr (CORE_ADDR addr)
+ {
+   for (const displaced_step_buffer &buffer : m_buffers)
+     {
+-      if (addr == buffer.addr)
++      /* Make sure we have active buffers to compare to.  */
++      if (buffer.current_thread != nullptr && addr == buffer.addr)
+       {
+ 	/* The closure information should always be available. */
+ 	gdb_assert (buffer.copy_insn_closure.get () != nullptr);
+diff --git a/gdb/testsuite/gdb.base/displaced-step-closure.c b/gdb/testsuite/gdb.base/displaced-step-closure.c
+new file mode 100644
+index 00000000000..8540538e915
+--- /dev/null
++++ b/gdb/testsuite/gdb.base/displaced-step-closure.c
+@@ -0,0 +1,21 @@
++/* This testcase is part of GDB, the GNU debugger.
++
++   Copyright 2023 Free Software Foundation, Inc.
++
++   This program is free software; you can redistribute it and/or modify
++   it under the terms of the GNU General Public License as published by
++   the Free Software Foundation; either version 3 of the License, or
++   (at your option) any later version.
++
++   This program is distributed in the hope that it will be useful,
++   but WITHOUT ANY WARRANTY; without even the implied warranty of
++   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++   GNU General Public License for more details.
++
++   You should have received a copy of the GNU General Public License
++   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
++
++int main (int argc, char **argv)
++{
++  return 0;
++}
+diff --git a/gdb/testsuite/gdb.base/displaced-step-closure.exp b/gdb/testsuite/gdb.base/displaced-step-closure.exp
+new file mode 100644
+index 00000000000..3389cd4f0de
+--- /dev/null
++++ b/gdb/testsuite/gdb.base/displaced-step-closure.exp
+@@ -0,0 +1,39 @@
++# Copyright 2023 Free Software Foundation, Inc.
++#
++# This program is free software; you can redistribute it and/or modify
++# it under the terms of the GNU General Public License as published by
++# the Free Software Foundation; either version 3 of the License, or
++# (at your option) any later version.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program.  If not, see <http://www.gnu.org/licenses/>.
++#
++# This file is part of the gdb testsuite.
++#
++# Test a displaced stepping closure management bug, where a closure lookup
++# by address returns a match even if no displaced stepping is currently
++# taking place.
++
++standard_testfile
++if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
++    return -1
++}
++
++if ![runto_main] {
++    return -1
++}
++
++# We have a breakpoint at the current pc (from stopping at main).  Step over
++# the breakpoint.
++gdb_test "stepi" ".*" "step-over breakpoint"
++
++# Now attempt to disassemble the entry point function, where the displaced
++# stepping buffer is.  With the bug, gdb will crash when we attempt to list
++# the PC that was used to displaced-step the previous instruction.
++gdb_test "disassemble _start" ".*End of assembler dump\." \
++	 "disassemble through displaced-step buffer"
+-- 
+2.34.1
+
diff --git a/debian/patches/series b/debian/patches/series
index e4521f4..8ef04e2 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -14,3 +14,7 @@ fork-inferior-fix
 # Ubuntu/Linaro
 gdb-strings.patch
 ptrace-error-verbosity.patch
+
+# Backported from GDB 13.1
+Make-sure-a-copy_insn_closure-is-available-when-we-h.patch
+Only-allow-closure-lookup-by-address-if-there-are-th.patch