public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Tom de Vries <tdevries@suse.de>
To: gdb-patches@sourceware.org
Subject: [PATCH 1/2] [gdb] Fix segfault in for_each_block, part 1
Date: Sat,  4 Nov 2023 16:57:56 +0100	[thread overview]
Message-ID: <20231104155757.16649-2-tdevries@suse.de> (raw)
In-Reply-To: <20231104155757.16649-1-tdevries@suse.de>

When running test-case gdb.base/vfork-follow-parent.exp on powerpc64 (likewise
on s390x), I run into:
...
(gdb) PASS: gdb.base/vfork-follow-parent.exp: \
  exec_file=vfork-follow-parent-exit: target-non-stop=on: non-stop=off: \
  resolution_method=schedule-multiple: print unblock_parent = 1
continue^M
Continuing.^M
Reading symbols from vfork-follow-parent-exit...^M
^M
^M
Fatal signal: Segmentation fault^M
----- Backtrace -----^M
0x1027d3e7 gdb_internal_backtrace_1^M
        src/gdb/bt-utils.c:122^M
0x1027d54f _Z22gdb_internal_backtracev^M
        src/gdb/bt-utils.c:168^M
0x1057643f handle_fatal_signal^M
        src/gdb/event-top.c:889^M
0x10576677 handle_sigsegv^M
        src/gdb/event-top.c:962^M
0x3fffa7610477 ???^M
0x103f2144 for_each_block^M
        src/gdb/dcache.c:199^M
0x103f235b _Z17dcache_invalidateP13dcache_struct^M
        src/gdb/dcache.c:251^M
0x10bde8c7 _Z24target_dcache_invalidatev^M
        src/gdb/target-dcache.c:50^M
...
or similar.

The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it should always return false, given that we're running on a
regular linux system, but instead it returns first true, then false.

In more detail, the segmentation fault happens as follows:
- a program space with an address space is created
- a second program space is about to be created. maybe_new_address_space
  is called, and because linux_is_uclinux returns true, maybe_new_address_space
  returns false, and no new address space is created
- a second program space with the same address space is created
- a program space is deleted. Because linux_is_uclinux now returns false,
  gdbarch_has_shared_address_space (current_inferior ()->arch ()) returns
  false, and the address space is deleted
- when gdb uses the address space of the remaining program space, we run into
  the segfault, because the address space is deleted.

Hardcoding linux_is_uclinux to false makes the test-case pass.

We leave addressing the root cause for the following commit in this series.

For now, prevent the segmentation fault by making the address space a refcounted
object.

This was already suggested here [1]:
...
A better solution might be to have the address spaces be reference counted
...

Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.

PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547

[1] https://sourceware.org/pipermail/gdb-patches/2023-October/202928.html
---
 gdb/progspace.c | 37 +++++++++++++++++++++++++++----------
 gdb/progspace.h | 11 ++++++++++-
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/gdb/progspace.c b/gdb/progspace.c
index 839707e9d71..4fea21f0ca1 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -96,9 +96,9 @@ remove_program_space (program_space *pspace)
 /* See progspace.h.  */
 
 program_space::program_space (address_space *aspace_)
-  : num (++last_program_space_num),
-    aspace (aspace_)
+  : num (++last_program_space_num)
 {
+  set_aspace (aspace_);
   program_spaces.push_back (this);
   gdb::observers::new_program_space.notify (this);
 }
@@ -122,8 +122,7 @@ program_space::~program_space ()
   /* Defer breakpoint re-set because we don't want to create new
      locations for this pspace which we're tearing down.  */
   clear_symtab_users (SYMFILE_DEFER_BP_RESET);
-  if (!gdbarch_has_shared_address_space (current_inferior ()->arch ()))
-    delete this->aspace;
+  reset_aspace ();
 }
 
 /* See progspace.h.  */
@@ -409,20 +408,19 @@ update_address_spaces (void)
 
   init_address_spaces ();
 
+  for (struct program_space *pspace : program_spaces)
+    pspace->reset_aspace ();
+
   if (shared_aspace)
     {
       struct address_space *aspace = new address_space ();
 
-      delete current_program_space->aspace;
       for (struct program_space *pspace : program_spaces)
-	pspace->aspace = aspace;
+	pspace->set_aspace (aspace);
     }
   else
     for (struct program_space *pspace : program_spaces)
-      {
-	delete pspace->aspace;
-	pspace->aspace = new address_space ();
-      }
+      pspace->set_aspace (new address_space ());
 
   for (inferior *inf : all_inferiors ())
     if (gdbarch_has_global_solist (current_inferior ()->arch ()))
@@ -433,8 +431,27 @@ update_address_spaces (void)
 
 \f
 
+void
+program_space::set_aspace (struct address_space *aspace_)
+{
+  aspace = aspace_;
+
+  aspace->incref ();
+}
+
 /* See progspace.h.  */
 
+void
+program_space::reset_aspace ()
+{
+  aspace->decref ();
+
+  if (aspace->refcount () == 0)
+    delete aspace;
+
+  aspace = nullptr;
+}
+
 void
 program_space::clear_solib_cache ()
 {
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a22e427400e..065ca38e255 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -336,6 +336,10 @@ struct program_space
      make breakpoints global).  */
   struct address_space *aspace = NULL;
 
+  void set_aspace (struct address_space *aspace);
+
+  void reset_aspace ();
+
   /* True if this program space's section offsets don't yet represent
      the final offsets of the "live" address space (that is, the
      section addresses still require the relocation offsets to be
@@ -384,12 +388,17 @@ struct program_space
 /* An address space.  It is used for comparing if
    pspaces/inferior/threads see the same address space and for
    associating caches to each address space.  */
-struct address_space
+struct address_space : public refcounted_object
 {
   /* Create a new address space object, and add it to the list.  */
   address_space ();
   DISABLE_COPY_AND_ASSIGN (address_space);
 
+  ~address_space ()
+  {
+    gdb_assert (refcount () == 0);
+  }
+
   /* Returns the integer address space id of this address space.  */
   int num () const
   {
-- 
2.35.3


  reply	other threads:[~2023-11-04 15:56 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-04 15:57 [PATCH 0/2] [gdb] Fix segfault in for_each_block Tom de Vries
2023-11-04 15:57 ` Tom de Vries [this message]
2023-11-06 15:24   ` [PATCH 1/2] [gdb] Fix segfault in for_each_block, part 1 Andrew Burgess
2023-11-07 13:28     ` Tom de Vries
2023-11-06 17:05   ` Simon Marchi
2023-11-07 11:16     ` Andrew Burgess
2023-11-07 13:32     ` Tom de Vries
2023-11-04 15:57 ` [PATCH 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231104155757.16649-2-tdevries@suse.de \
    --to=tdevries@suse.de \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).