* [PATCH v4 0/2] [gdb] Fix segfault in for_each_block
@ 2023-11-10 8:09 Tom de Vries
2023-11-10 8:09 ` [PATCH v4 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries
2023-11-10 8:09 ` [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries
0 siblings, 2 replies; 6+ messages in thread
From: Tom de Vries @ 2023-11-10 8:09 UTC (permalink / raw)
To: gdb-patches
This patch series fixes PR gdb/30547, a segfault when running test-case
gdb.base/vfork-follow-parent.exp on powerpc64 (likewise on s390x).
There are two patches, each one by themselves sufficient to no
longer trigger the segfault.
The root cause of the problem is that linux_is_uclinux, and consequently
gdbarch_has_shared_address_space returns an incorrect value.
The first patch makes gdb more robust against gdbarch_has_shared_address_space
returning incorrect values, by eliminating a call to it.
The third patch addresses the root cause.
Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.
[ I used gdb-14-branch for ppc64-linux, because I can't build trunk anymore
with system gcc 4.8.5 (CentOS-7), due to the recent c++17 requirement (and
just before that, some gcc bug in atomic support), and that's all I have
readily available on that machine. ]
There is still scope to fix things further.
When I started to investigate, I noticed that I only ran into the segfault on
ppc64 and s390x, two big-endian architectures, so I sort of expected to find an
endian-related problem.
Instead, the problem was ppc_linux_target_wordsize returning 4 instead of 8,
which causes gdb to interpret the 8-byte entry auxv vector using 4-byte
words, causing an incorrect linux_is_uclinux == true.
The same problem happens on ppc64le (ppc_linux_target_wordsize returns 4),
it's just that the incorrect word size doesn't change the outcome of:
- target_auxv_search (AT_NULL, &dummy) == 1, and
- target_auxv_search (AT_PAGESZ, &dummy) == 1
so linux_is_uclinux returns false, as it should.
This suggest a too forgiving parsing of the auxv vector, which should be made
more strict.
Finally, it should be fixed that ppc_linux_target_wordsize returns 4 in a
process with wordsize == 8.
I've file PRs for these two issues:
- [gdb] Make auxv parsing more strict
https://sourceware.org/bugzilla/show_bug.cgi?id=31040
- [gdb/tdep] ppc_linux_target_wordsize silently returns wrong result
https://sourceware.org/bugzilla/show_bug.cgi?id=31038
For now my interest is to backport at least one, possibly both patches from
this series to fix this PR on the gdb 14 release branch (and the 13.2 based
distro packages I maintain).
Submission history:
v4:
- drop patch "[gdb] Eliminate local var pspace in inferior.c"
- revert back to using refcounted_object, integrating Simon's
proposed patch
- add Simon as co-author on both patches
v3:
- fix std::shared_ptr usage issues after review by Simon
- add patch "[gdb] Eliminate local var pspace in inferior.c"
following up on comments by Simon
- update third patch according to comments by Simon
v2:
- use std::shared_ptr as suggested by Andrew and Simon
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
Tom de Vries (3):
[gdb] Fix segfault in for_each_block, part 1
[gdb] Eliminate local var pspace in inferior.c
[gdb] Fix segfault in for_each_block, part 2
gdb/breakpoint.c | 29 ++++++++-------
gdb/inferior.c | 24 ++++--------
gdb/inferior.h | 2 +-
gdb/infrun.c | 37 +++++++++++--------
gdb/linux-nat.c | 2 +-
gdb/nat/ppc-linux.c | 2 +
gdb/ppc-linux-nat.c | 2 +
gdb/process-stratum-target.c | 2 +-
gdb/progspace.c | 21 ++++-------
gdb/progspace.h | 6 +--
gdb/record-btrace.c | 2 +-
gdb/regcache.c | 2 +-
gdb/s390-linux-nat.c | 5 ++-
gdb/scoped-mock-context.h | 2 +-
gdb/target-dcache.c | 11 +++---
.../gdb.python/py-progspace-events.exp | 4 +-
16 files changed, 78 insertions(+), 75 deletions(-)
--
2.35.3
Tom de Vries (2):
[gdb] Fix segfault in for_each_block, part 1
[gdb] Fix segfault in for_each_block, part 2
gdb/breakpoint.c | 29 ++++++++-------
gdb/inferior.c | 8 ++---
gdb/inferior.h | 2 +-
gdb/infrun.c | 34 ++++++++++--------
gdb/linux-nat.c | 2 +-
gdb/nat/ppc-linux.c | 2 ++
gdb/ppc-linux-nat.c | 2 ++
gdb/process-stratum-target.c | 2 +-
gdb/progspace.c | 22 +++++-------
gdb/progspace.h | 64 +++++++++++++++++++++-------------
gdb/record-btrace.c | 2 +-
gdb/regcache.c | 2 +-
gdb/s390-linux-nat.c | 5 ++-
gdb/scoped-mock-context.h | 2 +-
gdb/target-dcache.c | 11 +++---
gdbsupport/refcounted-object.h | 17 +++++++++
16 files changed, 121 insertions(+), 85 deletions(-)
base-commit: e0446214a07dd4e6709c63d1eb3d764181264246
--
2.35.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] [gdb] Fix segfault in for_each_block, part 1
2023-11-10 8:09 [PATCH v4 0/2] [gdb] Fix segfault in for_each_block Tom de Vries
@ 2023-11-10 8:09 ` Tom de Vries
2023-11-28 9:52 ` Tom de Vries
2023-11-10 8:09 ` [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries
1 sibling, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2023-11-10 8:09 UTC (permalink / raw)
To: gdb-patches
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.
Co-Authored-By: Simon Marchi <simon.marchi@polymtl.ca>
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/breakpoint.c | 29 ++++++++-------
gdb/inferior.c | 8 ++---
gdb/inferior.h | 2 +-
gdb/infrun.c | 18 +++++-----
gdb/linux-nat.c | 2 +-
gdb/process-stratum-target.c | 2 +-
gdb/progspace.c | 22 +++++-------
gdb/progspace.h | 64 +++++++++++++++++++++-------------
gdb/record-btrace.c | 2 +-
gdb/regcache.c | 2 +-
gdb/scoped-mock-context.h | 2 +-
gdb/target-dcache.c | 11 +++---
gdbsupport/refcounted-object.h | 17 +++++++++
13 files changed, 102 insertions(+), 79 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fe09dbcbeee..0b086fbeab3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1723,7 +1723,7 @@ one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
int bptoffset = 0;
if (!breakpoint_address_match (target_info->placed_address_space, 0,
- current_program_space->aspace, 0))
+ current_program_space->aspace.get (), 0))
{
/* The breakpoint is inserted in a different address space. */
return;
@@ -2409,7 +2409,7 @@ should_be_inserted (struct bp_location *bl)
a breakpoint. */
if ((bl->loc_type == bp_loc_software_breakpoint
|| bl->loc_type == bp_loc_hardware_breakpoint)
- && stepping_past_instruction_at (bl->pspace->aspace,
+ && stepping_past_instruction_at (bl->pspace->aspace.get (),
bl->address)
/* The single-step breakpoint may be inserted at the location
we're trying to step if the instruction branches to itself.
@@ -2847,7 +2847,7 @@ insert_bp_location (struct bp_location *bl,
read the breakpoint instead of returning the data saved in
the breakpoint location's shadow contents. */
bl->target_info.reqstd_address = bl->address;
- bl->target_info.placed_address_space = bl->pspace->aspace;
+ bl->target_info.placed_address_space = bl->pspace->aspace.get ();
bl->target_info.length = bl->length;
/* When working with target-side conditions, we must pass all the conditions
@@ -4429,7 +4429,7 @@ bp_location_inserted_here_p (const struct bp_location *bl,
const address_space *aspace, CORE_ADDR pc)
{
if (bl->inserted
- && breakpoint_address_match (bl->pspace->aspace, bl->address,
+ && breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
aspace, pc))
{
/* An unmapped overlay can't be a match. */
@@ -4508,7 +4508,7 @@ hardware_watchpoint_inserted_in_range (const address_space *aspace,
continue;
for (bp_location &loc : bpt.locations ())
- if (loc.pspace->aspace == aspace && loc.inserted)
+ if (loc.pspace->aspace.get () == aspace && loc.inserted)
{
CORE_ADDR l, h;
@@ -7330,10 +7330,10 @@ breakpoint_location_address_match (struct bp_location *bl,
const address_space *aspace,
CORE_ADDR addr)
{
- return (breakpoint_address_match (bl->pspace->aspace, bl->address,
+ return (breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
aspace, addr)
|| (bl->length
- && breakpoint_address_match_range (bl->pspace->aspace,
+ && breakpoint_address_match_range (bl->pspace->aspace.get (),
bl->address, bl->length,
aspace, addr)));
}
@@ -7350,7 +7350,7 @@ breakpoint_location_address_range_overlap (struct bp_location *bl,
CORE_ADDR addr, int len)
{
if (gdbarch_has_global_breakpoints (current_inferior ()->arch ())
- || bl->pspace->aspace == aspace)
+ || bl->pspace->aspace.get () == aspace)
{
int bl_len = bl->length != 0 ? bl->length : 1;
@@ -7407,8 +7407,10 @@ breakpoint_locations_match (const struct bp_location *loc1,
/* We compare bp_location.length in order to cover ranged
breakpoints. Keep this in sync with
bp_location_is_less_than. */
- return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
- loc2->pspace->aspace, loc2->address)
+ return (breakpoint_address_match (loc1->pspace->aspace.get (),
+ loc1->address,
+ loc2->pspace->aspace.get (),
+ loc2->address)
&& (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
&& loc1->length == loc2->length);
}
@@ -9568,8 +9570,9 @@ ranged_breakpoint::breakpoint_hit (const struct bp_location *bl,
|| ws.sig () != GDB_SIGNAL_TRAP)
return 0;
- return breakpoint_address_match_range (bl->pspace->aspace, bl->address,
- bl->length, aspace, bp_addr);
+ return breakpoint_address_match_range (bl->pspace->aspace.get (),
+ bl->address, bl->length, aspace,
+ bp_addr);
}
/* Implement the "resources_needed" method for ranged breakpoints. */
@@ -12015,7 +12018,7 @@ code_breakpoint::breakpoint_hit (const struct bp_location *bl,
|| ws.sig () != GDB_SIGNAL_TRAP)
return 0;
- if (!breakpoint_address_match (bl->pspace->aspace, bl->address,
+ if (!breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
aspace, bp_addr))
return 0;
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 1778723863e..37b4fad6e44 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -843,15 +843,13 @@ remove_inferior_command (const char *args, int from_tty)
struct inferior *
add_inferior_with_spaces (void)
{
- struct address_space *aspace;
struct program_space *pspace;
struct inferior *inf;
/* If all inferiors share an address space on this system, this
doesn't really return a new address space; otherwise, it
really does. */
- aspace = maybe_new_address_space ();
- pspace = new program_space (aspace);
+ pspace = new program_space (maybe_new_address_space ());
inf = add_inferior (0);
inf->pspace = pspace;
inf->aspace = pspace->aspace;
@@ -1014,15 +1012,13 @@ clone_inferior_command (const char *args, int from_tty)
for (i = 0; i < copies; ++i)
{
- struct address_space *aspace;
struct program_space *pspace;
struct inferior *inf;
/* If all inferiors share an address space on this system, this
doesn't really return a new address space; otherwise, it
really does. */
- aspace = maybe_new_address_space ();
- pspace = new program_space (aspace);
+ pspace = new program_space (maybe_new_address_space ());
inf = add_inferior (0);
inf->pspace = pspace;
inf->aspace = pspace->aspace;
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 33eff7a9141..3f74cb5d8d4 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -584,7 +584,7 @@ class inferior : public refcounted_object,
bool removable = false;
/* The address space bound to this inferior. */
- struct address_space *aspace = NULL;
+ address_space_ref_ptr aspace;
/* The program space bound to this inferior. */
struct program_space *pspace = NULL;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4c7eb9be792..95620d479d8 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -529,8 +529,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
}
else
{
- child_inf->aspace = new address_space ();
- child_inf->pspace = new program_space (child_inf->aspace);
+ child_inf->pspace = new program_space (new_address_space ());
+ child_inf->aspace = child_inf->pspace->aspace;
child_inf->removable = true;
clone_program_space (child_inf->pspace, parent_inf->pspace);
}
@@ -608,8 +608,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
child_inf->aspace = parent_inf->aspace;
child_inf->pspace = parent_inf->pspace;
- parent_inf->aspace = new address_space ();
- parent_inf->pspace = new program_space (parent_inf->aspace);
+ parent_inf->pspace = new program_space (new_address_space ());
+ parent_inf->aspace = parent_inf->pspace->aspace;
clone_program_space (parent_inf->pspace, child_inf->pspace);
/* The parent inferior is still the current one, so keep things
@@ -618,8 +618,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
}
else
{
- child_inf->aspace = new address_space ();
- child_inf->pspace = new program_space (child_inf->aspace);
+ child_inf->pspace = new program_space (new_address_space ());
+ child_inf->aspace = child_inf->pspace->aspace;
child_inf->removable = true;
child_inf->symfile_flags = SYMFILE_NO_READ;
clone_program_space (child_inf->pspace, parent_inf->pspace);
@@ -1029,7 +1029,6 @@ handle_vfork_child_exec_or_exit (int exec)
if (vfork_parent->pending_detach)
{
struct program_space *pspace;
- struct address_space *aspace;
/* follow-fork child, detach-on-fork on. */
@@ -1054,9 +1053,8 @@ handle_vfork_child_exec_or_exit (int exec)
of" a hack. */
pspace = inf->pspace;
- aspace = inf->aspace;
- inf->aspace = nullptr;
inf->pspace = nullptr;
+ address_space_ref_ptr aspace = std::move (inf->aspace);
if (print_inferior_events)
{
@@ -5905,7 +5903,7 @@ handle_inferior_event (struct execution_control_state *ecs)
= get_thread_arch_aspace_regcache (parent_inf,
ecs->ws.child_ptid (),
gdbarch,
- parent_inf->aspace);
+ parent_inf->aspace.get ());
/* Read PC value of parent process. */
parent_pc = regcache_read_pc (regcache);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 1c9756c18bd..ca6e57cdaf9 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4361,7 +4361,7 @@ linux_nat_target::thread_address_space (ptid_t ptid)
inf = find_inferior_pid (this, pid);
gdb_assert (inf != NULL);
- return inf->aspace;
+ return inf->aspace.get ();
}
/* Return the cached value of the processor core for thread PTID. */
diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index 173c3ecad13..ebd47bb9282 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -37,7 +37,7 @@ process_stratum_target::thread_address_space (ptid_t ptid)
"address space of thread %s\n"),
target_pid_to_str (ptid).c_str ());
- return inf->aspace;
+ return inf->aspace.get ();
}
struct gdbarch *
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 839707e9d71..0fc1fdd57ab 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -55,8 +55,8 @@ address_space::address_space ()
return a pointer to an existing address space, in case inferiors
share an address space on this target system. */
-struct address_space *
-maybe_new_address_space (void)
+address_space_ref_ptr
+maybe_new_address_space ()
{
int shared_aspace
= gdbarch_has_shared_address_space (current_inferior ()->arch ());
@@ -67,7 +67,7 @@ maybe_new_address_space (void)
return program_spaces[0]->aspace;
}
- return new address_space ();
+ return new_address_space ();
}
/* Start counting over from scratch. */
@@ -95,9 +95,9 @@ remove_program_space (program_space *pspace)
/* See progspace.h. */
-program_space::program_space (address_space *aspace_)
+program_space::program_space (address_space_ref_ptr aspace_)
: num (++last_program_space_num),
- aspace (aspace_)
+ aspace (std::move (aspace_))
{
program_spaces.push_back (this);
gdb::observers::new_program_space.notify (this);
@@ -122,8 +122,6 @@ 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;
}
/* See progspace.h. */
@@ -411,18 +409,14 @@ update_address_spaces (void)
if (shared_aspace)
{
- struct address_space *aspace = new address_space ();
+ address_space_ref_ptr aspace = new_address_space ();
- delete current_program_space->aspace;
for (struct program_space *pspace : program_spaces)
pspace->aspace = aspace;
}
else
for (struct program_space *pspace : program_spaces)
- {
- delete pspace->aspace;
- pspace->aspace = new address_space ();
- }
+ pspace->aspace = new_address_space ();
for (inferior *inf : all_inferiors ())
if (gdbarch_has_global_solist (current_inferior ()->arch ()))
@@ -459,5 +453,5 @@ initialize_progspace (void)
modules have done that. Do this before
initialize_current_architecture, because that accesses the ebfd
of current_program_space. */
- current_program_space = new program_space (new address_space ());
+ current_program_space = new program_space (new_address_space ());
}
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a22e427400e..163cbf6a0f8 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -29,6 +29,8 @@
#include "gdbsupport/next-iterator.h"
#include "gdbsupport/safe-iterator.h"
#include "gdbsupport/intrusive_list.h"
+#include "gdbsupport/refcounted-object.h"
+#include "gdbsupport/gdb_ref_ptr.h"
#include <list>
#include <vector>
@@ -43,6 +45,40 @@ struct shobj;
typedef std::list<std::unique_ptr<objfile>> objfile_list;
+/* 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 : public refcounted_object
+{
+ /* Create a new address space object, and add it to the list. */
+ address_space ();
+ DISABLE_COPY_AND_ASSIGN (address_space);
+
+ /* Returns the integer address space id of this address space. */
+ int num () const
+ {
+ return m_num;
+ }
+
+ /* Per aspace data-pointers required by other GDB modules. */
+ registry<address_space> registry_fields;
+
+private:
+ int m_num;
+};
+
+using address_space_ref_ptr
+ = gdb::ref_ptr<address_space,
+ refcounted_object_delete_ref_policy<address_space>>;
+
+/* Create a new address space. */
+
+static inline address_space_ref_ptr
+new_address_space ()
+{
+ return address_space_ref_ptr::new_reference (new address_space);
+}
+
/* An iterator that wraps an iterator over std::unique_ptr<objfile>,
and dereferences the returned object. This is useful for iterating
over a list of shared pointers and returning raw pointers -- which
@@ -192,7 +228,7 @@ struct program_space
{
/* Constructs a new empty program space, binds it to ASPACE, and
adds it to the program space list. */
- explicit program_space (address_space *aspace);
+ explicit program_space (address_space_ref_ptr aspace);
/* Releases a program space, and all its contents (shared libraries,
objfiles, and any other references to the program space in other
@@ -334,7 +370,7 @@ struct program_space
are global, then this field is ignored (we don't currently
support inferiors sharing a program space if the target doesn't
make breakpoints global). */
- struct address_space *aspace = NULL;
+ address_space_ref_ptr aspace;
/* True if this program space's section offsets don't yet represent
the final offsets of the "live" address space (that is, the
@@ -381,28 +417,6 @@ struct program_space
std::vector<target_section> m_target_sections;
};
-/* 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
-{
- /* Create a new address space object, and add it to the list. */
- address_space ();
- DISABLE_COPY_AND_ASSIGN (address_space);
-
- /* Returns the integer address space id of this address space. */
- int num () const
- {
- return m_num;
- }
-
- /* Per aspace data-pointers required by other GDB modules. */
- registry<address_space> registry_fields;
-
-private:
- int m_num;
-};
-
/* The list of all program spaces. There's always at least one. */
extern std::vector<struct program_space *>program_spaces;
@@ -445,7 +459,7 @@ class scoped_restore_current_program_space
/* Maybe create a new address space object, and add it to the list, or
return a pointer to an existing address space, in case inferiors
share an address space. */
-extern struct address_space *maybe_new_address_space (void);
+extern address_space_ref_ptr maybe_new_address_space ();
/* Update all program spaces matching to address spaces. The user may
have created several program spaces, and loaded executables into
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index abab79f3132..4a1ecdd1ad6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2321,7 +2321,7 @@ record_btrace_replay_at_breakpoint (struct thread_info *tp)
if (insn == NULL)
return 0;
- return record_check_stopped_by_breakpoint (tp->inf->aspace, insn->pc,
+ return record_check_stopped_by_breakpoint (tp->inf->aspace.get (), insn->pc,
&btinfo->stop_reason);
}
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 2e48c025809..239656d1987 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1628,7 +1628,7 @@ get_thread_arch_aspace_regcache_and_check (inferior *inf_for_target_calls,
the current inferior's gdbarch. Also use the current inferior's address
space. */
gdbarch *arch = inf_for_target_calls->arch ();
- address_space *aspace = inf_for_target_calls->aspace;
+ address_space *aspace = inf_for_target_calls->aspace.get ();
regcache *regcache = get_thread_arch_aspace_regcache (inf_for_target_calls,
ptid, arch, aspace);
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index 5b5f46df7ba..70b913b949b 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -38,7 +38,7 @@ struct scoped_mock_context
Target mock_target;
ptid_t mock_ptid {1, 1};
- program_space mock_pspace {new address_space ()};
+ program_space mock_pspace {new_address_space ()};
inferior mock_inferior {mock_ptid.pid ()};
thread_info mock_thread {&mock_inferior, mock_ptid};
diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
index 13c2888e7ea..b1b772ab76e 100644
--- a/gdb/target-dcache.c
+++ b/gdb/target-dcache.c
@@ -33,7 +33,7 @@ int
target_dcache_init_p (void)
{
DCACHE *dcache
- = target_dcache_aspace_key.get (current_program_space->aspace);
+ = target_dcache_aspace_key.get (current_program_space->aspace.get ());
return (dcache != NULL);
}
@@ -44,7 +44,7 @@ void
target_dcache_invalidate (void)
{
DCACHE *dcache
- = target_dcache_aspace_key.get (current_program_space->aspace);
+ = target_dcache_aspace_key.get (current_program_space->aspace.get ());
if (dcache != NULL)
dcache_invalidate (dcache);
@@ -56,7 +56,7 @@ target_dcache_invalidate (void)
DCACHE *
target_dcache_get (void)
{
- return target_dcache_aspace_key.get (current_program_space->aspace);
+ return target_dcache_aspace_key.get (current_program_space->aspace.get ());
}
/* Return the target dcache. If it is not initialized yet, initialize
@@ -66,12 +66,13 @@ DCACHE *
target_dcache_get_or_init (void)
{
DCACHE *dcache
- = target_dcache_aspace_key.get (current_program_space->aspace);
+ = target_dcache_aspace_key.get (current_program_space->aspace.get ());
if (dcache == NULL)
{
dcache = dcache_init ();
- target_dcache_aspace_key.set (current_program_space->aspace, dcache);
+ target_dcache_aspace_key.set (current_program_space->aspace.get (),
+ dcache);
}
return dcache;
diff --git a/gdbsupport/refcounted-object.h b/gdbsupport/refcounted-object.h
index d8fdb950043..294fd873df1 100644
--- a/gdbsupport/refcounted-object.h
+++ b/gdbsupport/refcounted-object.h
@@ -67,4 +67,21 @@ struct refcounted_object_ref_policy
}
};
+/* A policy class to interface gdb::ref_ptr with a refcounted_object, that
+ deletes the object once the refcount reaches 0.. */
+
+template<typename T>
+struct refcounted_object_delete_ref_policy
+{
+ static void incref (T *obj)
+ { obj->incref (); }
+
+ static void decref (T *obj)
+ {
+ obj->decref ();
+ if (obj->refcount () == 0)
+ delete obj;
+ }
+};
+
#endif /* COMMON_REFCOUNTED_OBJECT_H */
--
2.35.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2
2023-11-10 8:09 [PATCH v4 0/2] [gdb] Fix segfault in for_each_block Tom de Vries
2023-11-10 8:09 ` [PATCH v4 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries
@ 2023-11-10 8:09 ` Tom de Vries
2023-11-10 8:24 ` Hannes Domani
1 sibling, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2023-11-10 8:09 UTC (permalink / raw)
To: gdb-patches
The previous commit describes PR gdb/30547, a segfault when running test-case
gdb.base/vfork-follow-parent.exp on powerpc64 (likewise on s390x).
The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it returns true instead of false.
So, why does linux_is_uclinux:
...
int
linux_is_uclinux (void)
{
CORE_ADDR dummy;
return (target_auxv_search (AT_NULL, &dummy) > 0
&& target_auxv_search (AT_PAGESZ, &dummy) == 0);
...
return true?
This is because ppc_linux_target_wordsize returns 4 instead of 8, causing
ppc_linux_nat_target::auxv_parse to misinterpret the auxv vector.
So, why does ppc_linux_target_wordsize:
...
int
ppc_linux_target_wordsize (int tid)
{
int wordsize = 4;
/* Check for 64-bit inferior process. This is the case when the host is
64-bit, and in addition the top bit of the MSR register is set. */
long msr;
errno = 0;
msr = (long) ptrace (PTRACE_PEEKUSER, tid, PT_MSR * 8, 0);
if (errno == 0 && ppc64_64bit_inferior_p (msr))
wordsize = 8;
return wordsize;
}
...
return 4?
Specifically, we get this result because because tid == 0, so we get
errno == ESRCH.
The tid == 0 is caused by the switch_to_no_thread in
handle_vfork_child_exec_or_exit:
...
/* Switch to no-thread while running clone_program_space, so
that clone_program_space doesn't want to read the
selected frame of a dead process. */
scoped_restore_current_thread restore_thread;
switch_to_no_thread ();
inf->pspace = new program_space (maybe_new_address_space ());
...
but moving the maybe_new_address_space call to before that gives us the
same result. The tid is no longer 0, but we still get ESRCH because the
thread has exited.
Fix this in handle_vfork_child_exec_or_exit by doing the
maybe_new_address_space call in the context of the vfork parent.
Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.
Co-Authored-By: Simon Marchi <simon.marchi@polymtl.ca>
PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
---
gdb/infrun.c | 16 +++++++++++-----
gdb/nat/ppc-linux.c | 2 ++
gdb/ppc-linux-nat.c | 2 ++
gdb/s390-linux-nat.c | 5 ++++-
4 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 95620d479d8..7266bbe5960 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1103,13 +1103,19 @@ handle_vfork_child_exec_or_exit (int exec)
go ahead and create a new one for this exiting
inferior. */
- /* Switch to no-thread while running clone_program_space, so
- that clone_program_space doesn't want to read the
- selected frame of a dead process. */
scoped_restore_current_thread restore_thread;
- switch_to_no_thread ();
- inf->pspace = new program_space (maybe_new_address_space ());
+ /* Temporarily switch to the vfork parent, to facilitate ptrace
+ calls done during maybe_new_address_space. */
+ switch_to_thread (any_live_thread_of_inferior (vfork_parent));
+ address_space_ref_ptr aspace = maybe_new_address_space ();
+
+ /* Switch back to the vfork child inferior. Switch to no-thread
+ while running clone_program_space, so that clone_program_space
+ doesn't want to read the selected frame of a dead process. */
+ switch_to_inferior_no_thread (inf);
+
+ inf->pspace = new program_space (std::move (aspace));
inf->aspace = inf->pspace->aspace;
set_current_program_space (inf->pspace);
inf->removable = true;
diff --git a/gdb/nat/ppc-linux.c b/gdb/nat/ppc-linux.c
index 0957d1b58a7..74549754806 100644
--- a/gdb/nat/ppc-linux.c
+++ b/gdb/nat/ppc-linux.c
@@ -78,6 +78,8 @@ ppc64_64bit_inferior_p (long msr)
int
ppc_linux_target_wordsize (int tid)
{
+ gdb_assert (tid != 0);
+
int wordsize = 4;
/* Check for 64-bit inferior process. This is the case when the host is
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index a0205119f00..70dd20ad963 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1914,6 +1914,8 @@ ppc_linux_nat_target::auxv_parse (const gdb_byte **readptr,
const gdb_byte *endptr, CORE_ADDR *typep,
CORE_ADDR *valp)
{
+ gdb_assert (inferior_ptid != null_ptid);
+
int tid = inferior_ptid.lwp ();
if (tid == 0)
tid = inferior_ptid.pid ();
diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 0aa3edea8aa..1a94ce487c3 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -949,10 +949,12 @@ s390_target_wordsize (void)
/* Check for 64-bit inferior process. This is the case when the host is
64-bit, and in addition bit 32 of the PSW mask is set. */
#ifdef __s390x__
+ int tid = s390_inferior_tid ();
+ gdb_assert (tid == 0);
long pswm;
errno = 0;
- pswm = (long) ptrace (PTRACE_PEEKUSER, s390_inferior_tid (), PT_PSWMASK, 0);
+ pswm = (long) ptrace (PTRACE_PEEKUSER, tid, PT_PSWMASK, 0);
if (errno == 0 && (pswm & 0x100000000ul) != 0)
wordsize = 8;
#endif
@@ -965,6 +967,7 @@ s390_linux_nat_target::auxv_parse (const gdb_byte **readptr,
const gdb_byte *endptr, CORE_ADDR *typep,
CORE_ADDR *valp)
{
+ gdb_assert (inferior_ptid != null_ptid);
int sizeof_auxv_field = s390_target_wordsize ();
bfd_endian byte_order = gdbarch_byte_order (current_inferior ()->arch ());
const gdb_byte *ptr = *readptr;
--
2.35.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2
2023-11-10 8:09 ` [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries
@ 2023-11-10 8:24 ` Hannes Domani
2023-11-28 9:47 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Domani @ 2023-11-10 8:24 UTC (permalink / raw)
To: gdb-patches, Tom de Vries
Am Freitag, 10. November 2023 um 09:08:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
> --- a/gdb/s390-linux-nat.c
> +++ b/gdb/s390-linux-nat.c
> @@ -949,10 +949,12 @@ s390_target_wordsize (void)
> /* Check for 64-bit inferior process. This is the case when the host is
> 64-bit, and in addition bit 32 of the PSW mask is set. */
> #ifdef __s390x__
> + int tid = s390_inferior_tid ();
> + gdb_assert (tid == 0);
Did you not mean `tid != 0` here?
Regards
Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2
2023-11-10 8:24 ` Hannes Domani
@ 2023-11-28 9:47 ` Tom de Vries
0 siblings, 0 replies; 6+ messages in thread
From: Tom de Vries @ 2023-11-28 9:47 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
On 11/10/23 09:24, Hannes Domani wrote:
> Am Freitag, 10. November 2023 um 09:08:11 MEZ hat Tom de Vries <tdevries@suse.de> Folgendes geschrieben:
>
>> --- a/gdb/s390-linux-nat.c
>> +++ b/gdb/s390-linux-nat.c
>> @@ -949,10 +949,12 @@ s390_target_wordsize (void)
>> /* Check for 64-bit inferior process. This is the case when the host is
>> 64-bit, and in addition bit 32 of the PSW mask is set. */
>> #ifdef __s390x__
>> + int tid = s390_inferior_tid ();
>> + gdb_assert (tid == 0);
>
> Did you not mean `tid != 0` here?
>
I did, thanks for spotting that.
I hadn't been able to test on s390x yet, by now I have.
Fixed in the v5 version I've pushed (
https://sourceware.org/pipermail/gdb-patches/2023-November/204569.html ).
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] [gdb] Fix segfault in for_each_block, part 1
2023-11-10 8:09 ` [PATCH v4 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries
@ 2023-11-28 9:52 ` Tom de Vries
0 siblings, 0 replies; 6+ messages in thread
From: Tom de Vries @ 2023-11-28 9:52 UTC (permalink / raw)
To: gdb-patches
On 11/10/23 09:09, Tom de Vries wrote:
> 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.
>
> Co-Authored-By: Simon Marchi <simon.marchi@polymtl.ca>
>
With no further comments, I decided to commit. The rebase was not
entirely trivial, so I posted this as v5 (
https://sourceware.org/pipermail/gdb-patches/2023-November/204570.html )
before pushing.
Thanks,
- Tom
> 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/breakpoint.c | 29 ++++++++-------
> gdb/inferior.c | 8 ++---
> gdb/inferior.h | 2 +-
> gdb/infrun.c | 18 +++++-----
> gdb/linux-nat.c | 2 +-
> gdb/process-stratum-target.c | 2 +-
> gdb/progspace.c | 22 +++++-------
> gdb/progspace.h | 64 +++++++++++++++++++++-------------
> gdb/record-btrace.c | 2 +-
> gdb/regcache.c | 2 +-
> gdb/scoped-mock-context.h | 2 +-
> gdb/target-dcache.c | 11 +++---
> gdbsupport/refcounted-object.h | 17 +++++++++
> 13 files changed, 102 insertions(+), 79 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index fe09dbcbeee..0b086fbeab3 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1723,7 +1723,7 @@ one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
> int bptoffset = 0;
>
> if (!breakpoint_address_match (target_info->placed_address_space, 0,
> - current_program_space->aspace, 0))
> + current_program_space->aspace.get (), 0))
> {
> /* The breakpoint is inserted in a different address space. */
> return;
> @@ -2409,7 +2409,7 @@ should_be_inserted (struct bp_location *bl)
> a breakpoint. */
> if ((bl->loc_type == bp_loc_software_breakpoint
> || bl->loc_type == bp_loc_hardware_breakpoint)
> - && stepping_past_instruction_at (bl->pspace->aspace,
> + && stepping_past_instruction_at (bl->pspace->aspace.get (),
> bl->address)
> /* The single-step breakpoint may be inserted at the location
> we're trying to step if the instruction branches to itself.
> @@ -2847,7 +2847,7 @@ insert_bp_location (struct bp_location *bl,
> read the breakpoint instead of returning the data saved in
> the breakpoint location's shadow contents. */
> bl->target_info.reqstd_address = bl->address;
> - bl->target_info.placed_address_space = bl->pspace->aspace;
> + bl->target_info.placed_address_space = bl->pspace->aspace.get ();
> bl->target_info.length = bl->length;
>
> /* When working with target-side conditions, we must pass all the conditions
> @@ -4429,7 +4429,7 @@ bp_location_inserted_here_p (const struct bp_location *bl,
> const address_space *aspace, CORE_ADDR pc)
> {
> if (bl->inserted
> - && breakpoint_address_match (bl->pspace->aspace, bl->address,
> + && breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
> aspace, pc))
> {
> /* An unmapped overlay can't be a match. */
> @@ -4508,7 +4508,7 @@ hardware_watchpoint_inserted_in_range (const address_space *aspace,
> continue;
>
> for (bp_location &loc : bpt.locations ())
> - if (loc.pspace->aspace == aspace && loc.inserted)
> + if (loc.pspace->aspace.get () == aspace && loc.inserted)
> {
> CORE_ADDR l, h;
>
> @@ -7330,10 +7330,10 @@ breakpoint_location_address_match (struct bp_location *bl,
> const address_space *aspace,
> CORE_ADDR addr)
> {
> - return (breakpoint_address_match (bl->pspace->aspace, bl->address,
> + return (breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
> aspace, addr)
> || (bl->length
> - && breakpoint_address_match_range (bl->pspace->aspace,
> + && breakpoint_address_match_range (bl->pspace->aspace.get (),
> bl->address, bl->length,
> aspace, addr)));
> }
> @@ -7350,7 +7350,7 @@ breakpoint_location_address_range_overlap (struct bp_location *bl,
> CORE_ADDR addr, int len)
> {
> if (gdbarch_has_global_breakpoints (current_inferior ()->arch ())
> - || bl->pspace->aspace == aspace)
> + || bl->pspace->aspace.get () == aspace)
> {
> int bl_len = bl->length != 0 ? bl->length : 1;
>
> @@ -7407,8 +7407,10 @@ breakpoint_locations_match (const struct bp_location *loc1,
> /* We compare bp_location.length in order to cover ranged
> breakpoints. Keep this in sync with
> bp_location_is_less_than. */
> - return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
> - loc2->pspace->aspace, loc2->address)
> + return (breakpoint_address_match (loc1->pspace->aspace.get (),
> + loc1->address,
> + loc2->pspace->aspace.get (),
> + loc2->address)
> && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
> && loc1->length == loc2->length);
> }
> @@ -9568,8 +9570,9 @@ ranged_breakpoint::breakpoint_hit (const struct bp_location *bl,
> || ws.sig () != GDB_SIGNAL_TRAP)
> return 0;
>
> - return breakpoint_address_match_range (bl->pspace->aspace, bl->address,
> - bl->length, aspace, bp_addr);
> + return breakpoint_address_match_range (bl->pspace->aspace.get (),
> + bl->address, bl->length, aspace,
> + bp_addr);
> }
>
> /* Implement the "resources_needed" method for ranged breakpoints. */
> @@ -12015,7 +12018,7 @@ code_breakpoint::breakpoint_hit (const struct bp_location *bl,
> || ws.sig () != GDB_SIGNAL_TRAP)
> return 0;
>
> - if (!breakpoint_address_match (bl->pspace->aspace, bl->address,
> + if (!breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
> aspace, bp_addr))
> return 0;
>
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 1778723863e..37b4fad6e44 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -843,15 +843,13 @@ remove_inferior_command (const char *args, int from_tty)
> struct inferior *
> add_inferior_with_spaces (void)
> {
> - struct address_space *aspace;
> struct program_space *pspace;
> struct inferior *inf;
>
> /* If all inferiors share an address space on this system, this
> doesn't really return a new address space; otherwise, it
> really does. */
> - aspace = maybe_new_address_space ();
> - pspace = new program_space (aspace);
> + pspace = new program_space (maybe_new_address_space ());
> inf = add_inferior (0);
> inf->pspace = pspace;
> inf->aspace = pspace->aspace;
> @@ -1014,15 +1012,13 @@ clone_inferior_command (const char *args, int from_tty)
>
> for (i = 0; i < copies; ++i)
> {
> - struct address_space *aspace;
> struct program_space *pspace;
> struct inferior *inf;
>
> /* If all inferiors share an address space on this system, this
> doesn't really return a new address space; otherwise, it
> really does. */
> - aspace = maybe_new_address_space ();
> - pspace = new program_space (aspace);
> + pspace = new program_space (maybe_new_address_space ());
> inf = add_inferior (0);
> inf->pspace = pspace;
> inf->aspace = pspace->aspace;
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 33eff7a9141..3f74cb5d8d4 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -584,7 +584,7 @@ class inferior : public refcounted_object,
> bool removable = false;
>
> /* The address space bound to this inferior. */
> - struct address_space *aspace = NULL;
> + address_space_ref_ptr aspace;
>
> /* The program space bound to this inferior. */
> struct program_space *pspace = NULL;
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 4c7eb9be792..95620d479d8 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -529,8 +529,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
> }
> else
> {
> - child_inf->aspace = new address_space ();
> - child_inf->pspace = new program_space (child_inf->aspace);
> + child_inf->pspace = new program_space (new_address_space ());
> + child_inf->aspace = child_inf->pspace->aspace;
> child_inf->removable = true;
> clone_program_space (child_inf->pspace, parent_inf->pspace);
> }
> @@ -608,8 +608,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
>
> child_inf->aspace = parent_inf->aspace;
> child_inf->pspace = parent_inf->pspace;
> - parent_inf->aspace = new address_space ();
> - parent_inf->pspace = new program_space (parent_inf->aspace);
> + parent_inf->pspace = new program_space (new_address_space ());
> + parent_inf->aspace = parent_inf->pspace->aspace;
> clone_program_space (parent_inf->pspace, child_inf->pspace);
>
> /* The parent inferior is still the current one, so keep things
> @@ -618,8 +618,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
> }
> else
> {
> - child_inf->aspace = new address_space ();
> - child_inf->pspace = new program_space (child_inf->aspace);
> + child_inf->pspace = new program_space (new_address_space ());
> + child_inf->aspace = child_inf->pspace->aspace;
> child_inf->removable = true;
> child_inf->symfile_flags = SYMFILE_NO_READ;
> clone_program_space (child_inf->pspace, parent_inf->pspace);
> @@ -1029,7 +1029,6 @@ handle_vfork_child_exec_or_exit (int exec)
> if (vfork_parent->pending_detach)
> {
> struct program_space *pspace;
> - struct address_space *aspace;
>
> /* follow-fork child, detach-on-fork on. */
>
> @@ -1054,9 +1053,8 @@ handle_vfork_child_exec_or_exit (int exec)
> of" a hack. */
>
> pspace = inf->pspace;
> - aspace = inf->aspace;
> - inf->aspace = nullptr;
> inf->pspace = nullptr;
> + address_space_ref_ptr aspace = std::move (inf->aspace);
>
> if (print_inferior_events)
> {
> @@ -5905,7 +5903,7 @@ handle_inferior_event (struct execution_control_state *ecs)
> = get_thread_arch_aspace_regcache (parent_inf,
> ecs->ws.child_ptid (),
> gdbarch,
> - parent_inf->aspace);
> + parent_inf->aspace.get ());
> /* Read PC value of parent process. */
> parent_pc = regcache_read_pc (regcache);
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 1c9756c18bd..ca6e57cdaf9 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4361,7 +4361,7 @@ linux_nat_target::thread_address_space (ptid_t ptid)
>
> inf = find_inferior_pid (this, pid);
> gdb_assert (inf != NULL);
> - return inf->aspace;
> + return inf->aspace.get ();
> }
>
> /* Return the cached value of the processor core for thread PTID. */
> diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
> index 173c3ecad13..ebd47bb9282 100644
> --- a/gdb/process-stratum-target.c
> +++ b/gdb/process-stratum-target.c
> @@ -37,7 +37,7 @@ process_stratum_target::thread_address_space (ptid_t ptid)
> "address space of thread %s\n"),
> target_pid_to_str (ptid).c_str ());
>
> - return inf->aspace;
> + return inf->aspace.get ();
> }
>
> struct gdbarch *
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 839707e9d71..0fc1fdd57ab 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -55,8 +55,8 @@ address_space::address_space ()
> return a pointer to an existing address space, in case inferiors
> share an address space on this target system. */
>
> -struct address_space *
> -maybe_new_address_space (void)
> +address_space_ref_ptr
> +maybe_new_address_space ()
> {
> int shared_aspace
> = gdbarch_has_shared_address_space (current_inferior ()->arch ());
> @@ -67,7 +67,7 @@ maybe_new_address_space (void)
> return program_spaces[0]->aspace;
> }
>
> - return new address_space ();
> + return new_address_space ();
> }
>
> /* Start counting over from scratch. */
> @@ -95,9 +95,9 @@ remove_program_space (program_space *pspace)
>
> /* See progspace.h. */
>
> -program_space::program_space (address_space *aspace_)
> +program_space::program_space (address_space_ref_ptr aspace_)
> : num (++last_program_space_num),
> - aspace (aspace_)
> + aspace (std::move (aspace_))
> {
> program_spaces.push_back (this);
> gdb::observers::new_program_space.notify (this);
> @@ -122,8 +122,6 @@ 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;
> }
>
> /* See progspace.h. */
> @@ -411,18 +409,14 @@ update_address_spaces (void)
>
> if (shared_aspace)
> {
> - struct address_space *aspace = new address_space ();
> + address_space_ref_ptr aspace = new_address_space ();
>
> - delete current_program_space->aspace;
> for (struct program_space *pspace : program_spaces)
> pspace->aspace = aspace;
> }
> else
> for (struct program_space *pspace : program_spaces)
> - {
> - delete pspace->aspace;
> - pspace->aspace = new address_space ();
> - }
> + pspace->aspace = new_address_space ();
>
> for (inferior *inf : all_inferiors ())
> if (gdbarch_has_global_solist (current_inferior ()->arch ()))
> @@ -459,5 +453,5 @@ initialize_progspace (void)
> modules have done that. Do this before
> initialize_current_architecture, because that accesses the ebfd
> of current_program_space. */
> - current_program_space = new program_space (new address_space ());
> + current_program_space = new program_space (new_address_space ());
> }
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index a22e427400e..163cbf6a0f8 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -29,6 +29,8 @@
> #include "gdbsupport/next-iterator.h"
> #include "gdbsupport/safe-iterator.h"
> #include "gdbsupport/intrusive_list.h"
> +#include "gdbsupport/refcounted-object.h"
> +#include "gdbsupport/gdb_ref_ptr.h"
> #include <list>
> #include <vector>
>
> @@ -43,6 +45,40 @@ struct shobj;
>
> typedef std::list<std::unique_ptr<objfile>> objfile_list;
>
> +/* 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 : public refcounted_object
> +{
> + /* Create a new address space object, and add it to the list. */
> + address_space ();
> + DISABLE_COPY_AND_ASSIGN (address_space);
> +
> + /* Returns the integer address space id of this address space. */
> + int num () const
> + {
> + return m_num;
> + }
> +
> + /* Per aspace data-pointers required by other GDB modules. */
> + registry<address_space> registry_fields;
> +
> +private:
> + int m_num;
> +};
> +
> +using address_space_ref_ptr
> + = gdb::ref_ptr<address_space,
> + refcounted_object_delete_ref_policy<address_space>>;
> +
> +/* Create a new address space. */
> +
> +static inline address_space_ref_ptr
> +new_address_space ()
> +{
> + return address_space_ref_ptr::new_reference (new address_space);
> +}
> +
> /* An iterator that wraps an iterator over std::unique_ptr<objfile>,
> and dereferences the returned object. This is useful for iterating
> over a list of shared pointers and returning raw pointers -- which
> @@ -192,7 +228,7 @@ struct program_space
> {
> /* Constructs a new empty program space, binds it to ASPACE, and
> adds it to the program space list. */
> - explicit program_space (address_space *aspace);
> + explicit program_space (address_space_ref_ptr aspace);
>
> /* Releases a program space, and all its contents (shared libraries,
> objfiles, and any other references to the program space in other
> @@ -334,7 +370,7 @@ struct program_space
> are global, then this field is ignored (we don't currently
> support inferiors sharing a program space if the target doesn't
> make breakpoints global). */
> - struct address_space *aspace = NULL;
> + address_space_ref_ptr aspace;
>
> /* True if this program space's section offsets don't yet represent
> the final offsets of the "live" address space (that is, the
> @@ -381,28 +417,6 @@ struct program_space
> std::vector<target_section> m_target_sections;
> };
>
> -/* 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
> -{
> - /* Create a new address space object, and add it to the list. */
> - address_space ();
> - DISABLE_COPY_AND_ASSIGN (address_space);
> -
> - /* Returns the integer address space id of this address space. */
> - int num () const
> - {
> - return m_num;
> - }
> -
> - /* Per aspace data-pointers required by other GDB modules. */
> - registry<address_space> registry_fields;
> -
> -private:
> - int m_num;
> -};
> -
> /* The list of all program spaces. There's always at least one. */
> extern std::vector<struct program_space *>program_spaces;
>
> @@ -445,7 +459,7 @@ class scoped_restore_current_program_space
> /* Maybe create a new address space object, and add it to the list, or
> return a pointer to an existing address space, in case inferiors
> share an address space. */
> -extern struct address_space *maybe_new_address_space (void);
> +extern address_space_ref_ptr maybe_new_address_space ();
>
> /* Update all program spaces matching to address spaces. The user may
> have created several program spaces, and loaded executables into
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index abab79f3132..4a1ecdd1ad6 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -2321,7 +2321,7 @@ record_btrace_replay_at_breakpoint (struct thread_info *tp)
> if (insn == NULL)
> return 0;
>
> - return record_check_stopped_by_breakpoint (tp->inf->aspace, insn->pc,
> + return record_check_stopped_by_breakpoint (tp->inf->aspace.get (), insn->pc,
> &btinfo->stop_reason);
> }
>
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 2e48c025809..239656d1987 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1628,7 +1628,7 @@ get_thread_arch_aspace_regcache_and_check (inferior *inf_for_target_calls,
> the current inferior's gdbarch. Also use the current inferior's address
> space. */
> gdbarch *arch = inf_for_target_calls->arch ();
> - address_space *aspace = inf_for_target_calls->aspace;
> + address_space *aspace = inf_for_target_calls->aspace.get ();
> regcache *regcache = get_thread_arch_aspace_regcache (inf_for_target_calls,
> ptid, arch, aspace);
>
> diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
> index 5b5f46df7ba..70b913b949b 100644
> --- a/gdb/scoped-mock-context.h
> +++ b/gdb/scoped-mock-context.h
> @@ -38,7 +38,7 @@ struct scoped_mock_context
>
> Target mock_target;
> ptid_t mock_ptid {1, 1};
> - program_space mock_pspace {new address_space ()};
> + program_space mock_pspace {new_address_space ()};
> inferior mock_inferior {mock_ptid.pid ()};
> thread_info mock_thread {&mock_inferior, mock_ptid};
>
> diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
> index 13c2888e7ea..b1b772ab76e 100644
> --- a/gdb/target-dcache.c
> +++ b/gdb/target-dcache.c
> @@ -33,7 +33,7 @@ int
> target_dcache_init_p (void)
> {
> DCACHE *dcache
> - = target_dcache_aspace_key.get (current_program_space->aspace);
> + = target_dcache_aspace_key.get (current_program_space->aspace.get ());
>
> return (dcache != NULL);
> }
> @@ -44,7 +44,7 @@ void
> target_dcache_invalidate (void)
> {
> DCACHE *dcache
> - = target_dcache_aspace_key.get (current_program_space->aspace);
> + = target_dcache_aspace_key.get (current_program_space->aspace.get ());
>
> if (dcache != NULL)
> dcache_invalidate (dcache);
> @@ -56,7 +56,7 @@ target_dcache_invalidate (void)
> DCACHE *
> target_dcache_get (void)
> {
> - return target_dcache_aspace_key.get (current_program_space->aspace);
> + return target_dcache_aspace_key.get (current_program_space->aspace.get ());
> }
>
> /* Return the target dcache. If it is not initialized yet, initialize
> @@ -66,12 +66,13 @@ DCACHE *
> target_dcache_get_or_init (void)
> {
> DCACHE *dcache
> - = target_dcache_aspace_key.get (current_program_space->aspace);
> + = target_dcache_aspace_key.get (current_program_space->aspace.get ());
>
> if (dcache == NULL)
> {
> dcache = dcache_init ();
> - target_dcache_aspace_key.set (current_program_space->aspace, dcache);
> + target_dcache_aspace_key.set (current_program_space->aspace.get (),
> + dcache);
> }
>
> return dcache;
> diff --git a/gdbsupport/refcounted-object.h b/gdbsupport/refcounted-object.h
> index d8fdb950043..294fd873df1 100644
> --- a/gdbsupport/refcounted-object.h
> +++ b/gdbsupport/refcounted-object.h
> @@ -67,4 +67,21 @@ struct refcounted_object_ref_policy
> }
> };
>
> +/* A policy class to interface gdb::ref_ptr with a refcounted_object, that
> + deletes the object once the refcount reaches 0.. */
> +
> +template<typename T>
> +struct refcounted_object_delete_ref_policy
> +{
> + static void incref (T *obj)
> + { obj->incref (); }
> +
> + static void decref (T *obj)
> + {
> + obj->decref ();
> + if (obj->refcount () == 0)
> + delete obj;
> + }
> +};
> +
> #endif /* COMMON_REFCOUNTED_OBJECT_H */
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-28 9:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-10 8:09 [PATCH v4 0/2] [gdb] Fix segfault in for_each_block Tom de Vries
2023-11-10 8:09 ` [PATCH v4 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries
2023-11-28 9:52 ` Tom de Vries
2023-11-10 8:09 ` [PATCH v4 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries
2023-11-10 8:24 ` Hannes Domani
2023-11-28 9:47 ` Tom de Vries
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).