Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
home:MaxxedSUSE:Emulators
gdb
gdb-fix-segfault-in-for_each_block-part-1.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File gdb-fix-segfault-in-for_each_block-part-1.patch of Package gdb
From 75617e6d28b93814ac46ad85ad4fc2b133f61114 Mon Sep 17 00:00:00 2001 From: Tom de Vries <tdevries@suse.de> Date: Fri, 3 Nov 2023 16:25:33 +0100 Subject: [PATCH] [gdb] Fix segfault in for_each_block, part 1 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 | 20 +++++------ 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, 103 insertions(+), 80 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f8d19356828..f4acb4ea8c4 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1605,7 +1605,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; @@ -2278,7 +2278,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. @@ -2713,7 +2713,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 @@ -4276,7 +4276,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. */ @@ -4355,7 +4355,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; @@ -7153,10 +7153,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))); } @@ -7173,7 +7173,7 @@ breakpoint_location_address_range_overlap (struct bp_location *bl, CORE_ADDR addr, int len) { if (gdbarch_has_global_breakpoints (target_gdbarch ()) - || bl->pspace->aspace == aspace) + || bl->pspace->aspace.get () == aspace) { int bl_len = bl->length != 0 ? bl->length : 1; @@ -7230,8 +7230,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); } @@ -9297,8 +9299,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. */ @@ -11696,7 +11699,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 b0ecca8b63a..87c61eeafd7 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -775,15 +775,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; @@ -946,15 +944,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 4d001b0ad50..fa5c3c92eeb 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -541,7 +541,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 c078098a6f8..4073150d80c 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -501,8 +501,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); } @@ -573,8 +573,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 @@ -583,8 +583,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); @@ -938,7 +938,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. */ @@ -963,9 +962,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) { @@ -1019,7 +1017,7 @@ handle_vfork_child_exec_or_exit (int exec) /* 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 *aspace = maybe_new_address_space (); + 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 @@ -5639,7 +5637,7 @@ handle_inferior_event (struct execution_control_state *ecs) = get_thread_arch_aspace_regcache (parent_inf->process_target (), 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 2b206a4ec1e..474d3c7f945 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -4316,7 +4316,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 4722c5a0f28..e67012a8591 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 32bdfebcf7c..55df3b65dfe 100644 --- a/gdb/progspace.c +++ b/gdb/progspace.c @@ -54,8 +54,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 (target_gdbarch ()); @@ -65,7 +65,7 @@ maybe_new_address_space (void) return program_spaces[0]->aspace; } - return new address_space (); + return new_address_space (); } /* Start counting over from scratch. */ @@ -93,9 +93,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); } @@ -118,8 +118,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 (target_gdbarch ())) - delete this->aspace; } /* See progspace.h. */ @@ -389,18 +387,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 (target_gdbarch ())) @@ -437,5 +431,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 85215f0e2f1..07cca8c675c 100644 --- a/gdb/progspace.h +++ b/gdb/progspace.h @@ -28,6 +28,8 @@ #include "solist.h" #include "gdbsupport/next-iterator.h" #include "gdbsupport/safe-iterator.h" +#include "gdbsupport/refcounted-object.h" +#include "gdbsupport/gdb_ref_ptr.h" #include <list> #include <vector> @@ -42,6 +44,40 @@ struct so_list; 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 @@ -191,7 +227,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 @@ -332,7 +368,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 @@ -379,28 +415,6 @@ struct program_space target_section_table 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; @@ -443,7 +457,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 0daba893813..276cdcc03b8 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -2314,7 +2314,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 56b6d047874..8a0b57e67b8 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -1617,7 +1617,7 @@ get_thread_arch_aspace_regcache_and_check (process_stratum_target *target, the current inferior's gdbarch. Also use the current inferior's address space. */ gdbarch *arch = current_inferior ()->gdbarch; - address_space *aspace = current_inferior ()->aspace; + address_space *aspace = current_inferior ()->aspace.get (); regcache *regcache = get_thread_arch_aspace_regcache (target, ptid, arch, aspace); diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h index 9ad7ebf5f0c..5f25dc7ed6b 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 */ base-commit: c55a452eaf9390d5659d3205f762aa2cb84511e1 -- 2.35.3
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
