* [PATCH v2 0/2] [gdb] Fix segfault in for_each_block @ 2023-11-07 13:19 Tom de Vries 2023-11-07 13:19 ` [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries 2023-11-07 13:19 ` [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries 0 siblings, 2 replies; 8+ messages in thread From: Tom de Vries @ 2023-11-07 13:19 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 of them 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 second 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 added an assert that PTRACE_PEEKUSER doesn't fail (errno != 0) and ran into it in test-case gdb.base/access-mem-running.exp, during trying to "set a breakpoint while the process is running". It's clear that it's quite common for this to happen, and it's surprising that this doesn't cause more problems. I'll eventually file PRs for these two issues, 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). Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547 Tom de Vries (2): [gdb] Fix segfault in for_each_block, part 1 [gdb] Fix segfault in for_each_block, part 2 gdb/infrun.c | 12 +++++++++++- gdb/progspace.c | 37 +++++++++++++++++++++++++++---------- gdb/progspace.h | 11 ++++++++++- 3 files changed, 48 insertions(+), 12 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 | 28 +++++++++++++++++++--------- gdb/linux-nat.c | 2 +- gdb/process-stratum-target.c | 2 +- gdb/progspace.c | 29 +++++++++++++++++------------ gdb/progspace.h | 5 +++-- gdb/record-btrace.c | 2 +- gdb/regcache.c | 2 +- gdb/target-dcache.c | 11 ++++++----- 11 files changed, 68 insertions(+), 52 deletions(-) base-commit: d1bd9787f9ad8c49fb463d8d53411aeb5f9c179b -- 2.35.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 2023-11-07 13:19 [PATCH v2 0/2] [gdb] Fix segfault in for_each_block Tom de Vries @ 2023-11-07 13:19 ` Tom de Vries 2023-11-07 15:00 ` Simon Marchi 2023-11-07 13:19 ` [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries 1 sibling, 1 reply; 8+ messages in thread From: Tom de Vries @ 2023-11-07 13:19 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. 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 | 16 ++++++++-------- gdb/linux-nat.c | 2 +- gdb/process-stratum-target.c | 2 +- gdb/progspace.c | 29 +++++++++++++++++------------ gdb/progspace.h | 5 +++-- gdb/record-btrace.c | 2 +- gdb/regcache.c | 2 +- gdb/target-dcache.c | 11 ++++++----- 11 files changed, 57 insertions(+), 51 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..f80dced115c 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; + std::shared_ptr<address_space> aspace = NULL; /* The program space bound to this inferior. */ struct program_space *pspace = NULL; diff --git a/gdb/infrun.c b/gdb/infrun.c index 4fde96800fb..f0f35c23c9c 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,7 @@ handle_vfork_child_exec_or_exit (int exec) if (vfork_parent->pending_detach) { struct program_space *pspace; - struct address_space *aspace; + std::shared_ptr<address_space> aspace; /* follow-fork child, detach-on-fork on. */ @@ -5905,7 +5905,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..2e68c71bc21 100644 --- a/gdb/progspace.c +++ b/gdb/progspace.c @@ -55,7 +55,7 @@ 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 * +std::shared_ptr<address_space> maybe_new_address_space (void) { int shared_aspace @@ -67,7 +67,9 @@ maybe_new_address_space (void) return program_spaces[0]->aspace; } - return new address_space (); + std::shared_ptr<address_space> res; + res.reset (new address_space ()); + return std::move (res); } /* Start counting over from scratch. */ @@ -96,9 +98,17 @@ 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) { + aspace.reset (aspace_); + program_spaces.push_back (this); + gdb::observers::new_program_space.notify (this); +} + +program_space::program_space (std::shared_ptr<address_space> aspace_) + : num (++last_program_space_num) +{ + aspace = aspace_; program_spaces.push_back (this); gdb::observers::new_program_space.notify (this); } @@ -122,8 +132,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; + aspace = nullptr; } /* See progspace.h. */ @@ -413,16 +422,12 @@ update_address_spaces (void) { struct address_space *aspace = new address_space (); - delete current_program_space->aspace; for (struct program_space *pspace : program_spaces) - pspace->aspace = aspace; + pspace->aspace.reset (aspace); } else for (struct program_space *pspace : program_spaces) - { - delete pspace->aspace; - pspace->aspace = new address_space (); - } + pspace->aspace.reset (new address_space ()); for (inferior *inf : all_inferiors ()) if (gdbarch_has_global_solist (current_inferior ()->arch ())) diff --git a/gdb/progspace.h b/gdb/progspace.h index a22e427400e..c796329dd18 100644 --- a/gdb/progspace.h +++ b/gdb/progspace.h @@ -193,6 +193,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 (std::shared_ptr<address_space> aspace); /* Releases a program space, and all its contents (shared libraries, objfiles, and any other references to the program space in other @@ -334,7 +335,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; + std::shared_ptr<address_space> aspace = nullptr; /* True if this program space's section offsets don't yet represent the final offsets of the "live" address space (that is, the @@ -445,7 +446,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 std::shared_ptr<address_space> maybe_new_address_space (void); /* 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/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; -- 2.35.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 2023-11-07 13:19 ` [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries @ 2023-11-07 15:00 ` Simon Marchi 2023-11-09 15:07 ` Tom de Vries 0 siblings, 1 reply; 8+ messages in thread From: Simon Marchi @ 2023-11-07 15:00 UTC (permalink / raw) To: Tom de Vries, gdb-patches Thanks for the update. I noted a few comments on the use of shared_ptr. > 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; Does it work if you remove the pspace variable too and just do: inf->pspace = new program_space (maybe_new_address_space ()); > @@ -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; Same here, if it works. > @@ -584,7 +584,7 @@ class inferior : public refcounted_object, > bool removable = false; > > /* The address space bound to this inferior. */ > - struct address_space *aspace = NULL; > + std::shared_ptr<address_space> aspace = NULL; Remove `= NULL`. > @@ -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; I would suggest removing the program_space(address_space *) constructor, and replacing these calls with: new program_space (std::make_shared<address_space> ()) I think the `address_space *` is a bit dangerous. If you pass an address_space that is managed by a shared_ptr elsewhere, there will be two shared_ptr separately managing this address_space, which will result in a double free. > @@ -1029,7 +1029,7 @@ handle_vfork_child_exec_or_exit (int exec) > if (vfork_parent->pending_detach) > { > struct program_space *pspace; > - struct address_space *aspace; > + std::shared_ptr<address_space> aspace; Especially for non-trivially constructible objects, declare the variable where first used, to avoid constructing an empty object and then doing an assignment. In this case, we can even steal the reference from the inferior to avoid in incref + decref (not a big deal but heh, why not): std::shared_ptr<address_space> aspace = std::move (inf->aspace); After that call, inf->aspace is empty (contains nullptr), at least according to: https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr > @@ -67,7 +67,9 @@ maybe_new_address_space (void) > return program_spaces[0]->aspace; > } > > - return new address_space (); > + std::shared_ptr<address_space> res; > + res.reset (new address_space ()); > + return std::move (res); return std::make_shared<address_space> (); > @@ -96,9 +98,17 @@ 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) > { > + aspace.reset (aspace_); > + program_spaces.push_back (this); > + gdb::observers::new_program_space.notify (this); > +} > + > +program_space::program_space (std::shared_ptr<address_space> aspace_) > + : num (++last_program_space_num) > +{ > + aspace = aspace_; Initialize aspace in the initialization list, and std::move it while at it to avoid an incref/decref: program_space::program_space (std::shared_ptr<address_space> aspace_) : num (++last_program_space_num), aspace (std::move (aspace_)) > @@ -122,8 +132,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; > + aspace = nullptr; This should not be necessary, it will happen when the aspace shared_ptr gets destroyed (right after this). > @@ -413,16 +422,12 @@ update_address_spaces (void) > { > struct address_space *aspace = new address_space (); > > - delete current_program_space->aspace; > for (struct program_space *pspace : program_spaces) > - pspace->aspace = aspace; > + pspace->aspace.reset (aspace); I think this is a wrong use of shared_ptr, you tell separate shared_ptrs to start managing this address_space object. They'll all have a refcount of 1, and will all try to delete the address space later. Instead, create one shared_ptr like this: auto aspace = std::make_shared<address_space> (); And then assign to each (the loop doesn't need to change in fact): 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.reset (new address_space ()); This would work, but to be more robust and avoid the pitfalls I explained elsewhere, I would suggest creating address spaces only through std::make_shared. So: pspace->aspace = std::make_shared<address_space> (); There is also a slight performance advantage in using make_shared, explained here: https://ddanilov.me/shared-ptr-is-evil/ make_shared allocates the shared_ptr's control block (the shared thing that holds the refcount) and the actual object in the same buffer, whereas using reset + new allocates two separate buffers. > @@ -334,7 +335,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; > + std::shared_ptr<address_space> aspace = nullptr; No need to initialize with nullptr (just like unique_ptr) (and it gets initialized in the constructor anyway). Simon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 2023-11-07 15:00 ` Simon Marchi @ 2023-11-09 15:07 ` Tom de Vries 2023-11-09 16:26 ` Simon Marchi 0 siblings, 1 reply; 8+ messages in thread From: Tom de Vries @ 2023-11-09 15:07 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 11/7/23 16:00, Simon Marchi wrote: > Thanks for the update. I noted a few comments on the use of shared_ptr. > >> 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; > > Does it work if you remove the pspace variable too and just do: > > inf->pspace = new program_space (maybe_new_address_space ()); > >> @@ -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; > > Same here, if it works. > It does work, but the change is user visible, so I've factored that out into a separate patch in v3 ( https://sourceware.org/pipermail/gdb-patches/2023-November/203931.html ). >> @@ -584,7 +584,7 @@ class inferior : public refcounted_object, >> bool removable = false; >> >> /* The address space bound to this inferior. */ >> - struct address_space *aspace = NULL; >> + std::shared_ptr<address_space> aspace = NULL; > > Remove `= NULL`. > Done. >> @@ -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; > > I would suggest removing the program_space(address_space *) constructor, > and replacing these calls with: > > new program_space (std::make_shared<address_space> ()) > Ah, I was wondering about something like that, make sense, thanks. > I think the `address_space *` is a bit dangerous. If you pass an > address_space that is managed by a shared_ptr elsewhere, there will be > two shared_ptr separately managing this address_space, which will result > in a double free. > >> @@ -1029,7 +1029,7 @@ handle_vfork_child_exec_or_exit (int exec) >> if (vfork_parent->pending_detach) >> { >> struct program_space *pspace; >> - struct address_space *aspace; >> + std::shared_ptr<address_space> aspace; > > Especially for non-trivially constructible objects, declare the variable > where first used, to avoid constructing an empty object and then doing > an assignment. In this case, we can even steal the reference from the > inferior to avoid in incref + decref (not a big deal but heh, why not): > > std::shared_ptr<address_space> aspace = std::move (inf->aspace); > > After that call, inf->aspace is empty (contains nullptr), at least > according to: > > https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr > Done. >> @@ -67,7 +67,9 @@ maybe_new_address_space (void) >> return program_spaces[0]->aspace; >> } >> >> - return new address_space (); >> + std::shared_ptr<address_space> res; >> + res.reset (new address_space ()); >> + return std::move (res); > > return std::make_shared<address_space> (); > Done. >> @@ -96,9 +98,17 @@ 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) >> { >> + aspace.reset (aspace_); >> + program_spaces.push_back (this); >> + gdb::observers::new_program_space.notify (this); >> +} >> + >> +program_space::program_space (std::shared_ptr<address_space> aspace_) >> + : num (++last_program_space_num) >> +{ >> + aspace = aspace_; > > Initialize aspace in the initialization list, and std::move it while at > it to avoid an incref/decref: > Ack, done. > program_space::program_space (std::shared_ptr<address_space> aspace_) > : num (++last_program_space_num), > aspace (std::move (aspace_)) > >> @@ -122,8 +132,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; >> + aspace = nullptr; > > This should not be necessary, it will happen when the aspace shared_ptr > gets destroyed (right after this). > Ack, done. >> @@ -413,16 +422,12 @@ update_address_spaces (void) >> { >> struct address_space *aspace = new address_space (); >> >> - delete current_program_space->aspace; >> for (struct program_space *pspace : program_spaces) >> - pspace->aspace = aspace; >> + pspace->aspace.reset (aspace); > > I think this is a wrong use of shared_ptr, you tell separate > shared_ptrs to start managing this address_space object. They'll all > have a refcount of 1, and will all try to delete the address space > later. > Ouch. I wondered why this didn't surface in my testing, but I guess that's because I only tested native, and this is only exercised with remote. > Instead, create one shared_ptr like this: > > auto aspace = std::make_shared<address_space> (); > > And then assign to each (the loop doesn't need to change in fact): > > 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.reset (new address_space ()); > > This would work, but to be more robust and avoid the pitfalls I > explained elsewhere, I would suggest creating address spaces only > through std::make_shared. So: > > pspace->aspace = std::make_shared<address_space> (); > > > There is also a slight performance advantage in using make_shared, > explained here: > > https://ddanilov.me/shared-ptr-is-evil/ > > make_shared allocates the shared_ptr's control block (the shared thing > that holds the refcount) and the actual object in the same buffer, > whereas using reset + new allocates two separate buffers. > Done. >> @@ -334,7 +335,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; >> + std::shared_ptr<address_space> aspace = nullptr; > > No need to initialize with nullptr (just like unique_ptr) (and it gets > initialized in the constructor anyway). Done. V3 submitted here ( https://sourceware.org/pipermail/gdb-patches/2023-November/203934.html ). Thanks, - Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 2023-11-09 15:07 ` Tom de Vries @ 2023-11-09 16:26 ` Simon Marchi 0 siblings, 0 replies; 8+ messages in thread From: Simon Marchi @ 2023-11-09 16:26 UTC (permalink / raw) To: Tom de Vries, gdb-patches On 11/9/23 10:07, Tom de Vries wrote: > On 11/7/23 16:00, Simon Marchi wrote: >> Thanks for the update. I noted a few comments on the use of shared_ptr. >> >>> 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; >> >> Does it work if you remove the pspace variable too and just do: >> >> inf->pspace = new program_space (maybe_new_address_space ()); >> >>> @@ -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; >> >> Same here, if it works. >> > > It does work, but the change is user visible, so I've factored that out into a separate patch in v3 ( https://sourceware.org/pipermail/gdb-patches/2023-November/203931.html ). Oh, well if it changes the behavior of GDB, maybe that's not a good idea then. I suggested that just because it would make the code cleaner, but I don't think that's a good reason to justify changing the behavior. >>> @@ -413,16 +422,12 @@ update_address_spaces (void) >>> { >>> struct address_space *aspace = new address_space (); >>> - delete current_program_space->aspace; >>> for (struct program_space *pspace : program_spaces) >>> - pspace->aspace = aspace; >>> + pspace->aspace.reset (aspace); >> >> I think this is a wrong use of shared_ptr, you tell separate >> shared_ptrs to start managing this address_space object. They'll all >> have a refcount of 1, and will all try to delete the address space >> later. >> > > Ouch. I wondered why this didn't surface in my testing, but I guess that's because I only tested native, and this is only exercised with remote. I don't think any regular testing exercises this, you'd need to test against an uclinux target (an MMU-less Linux) to reach that code. Simon ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 2023-11-07 13:19 [PATCH v2 0/2] [gdb] Fix segfault in for_each_block Tom de Vries 2023-11-07 13:19 ` [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries @ 2023-11-07 13:19 ` Tom de Vries 2023-11-07 15:26 ` Simon Marchi 1 sibling, 1 reply; 8+ messages in thread From: Tom de Vries @ 2023-11-07 13:19 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. PR gdb/30547 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547 --- gdb/infrun.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index f0f35c23c9c..94a0924653f 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec) go ahead and create a new one for this exiting inferior. */ + std::shared_ptr<address_space> aspace; + { + /* Temporarily switch to the vfork parent, to facilitate ptrace + calls done during maybe_new_address_space. */ + scoped_restore save_inferior_ptid + = make_scoped_restore (&inferior_ptid); + inferior_ptid = ptid_t (vfork_parent->pid); + aspace = maybe_new_address_space (); + } + /* 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 ()); + inf->pspace = new program_space (aspace); inf->aspace = inf->pspace->aspace; set_current_program_space (inf->pspace); inf->removable = true; -- 2.35.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 2023-11-07 13:19 ` [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries @ 2023-11-07 15:26 ` Simon Marchi 2023-11-09 15:10 ` Tom de Vries 0 siblings, 1 reply; 8+ messages in thread From: Simon Marchi @ 2023-11-07 15:26 UTC (permalink / raw) To: Tom de Vries, gdb-patches On 11/7/23 08:19, Tom de Vries wrote: > 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. I would suggest adding some assertion: - assert that inferior_ptid != null_ptid in ppc_linux_nat_target::auxv_parse and s390_linux_nat_target::auxv_parse - assert that tid != 0 in ppc_linux_target_wordsize (and in s390_target_wordsize), though it could use a bit of refactoring to take tid as a parameter like ppc_linux_target_wordsize) > 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. > > PR gdb/30547 > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547 > --- > gdb/infrun.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index f0f35c23c9c..94a0924653f 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec) > go ahead and create a new one for this exiting > inferior. */ > > + std::shared_ptr<address_space> aspace; > + { > + /* Temporarily switch to the vfork parent, to facilitate ptrace > + calls done during maybe_new_address_space. */ > + scoped_restore save_inferior_ptid > + = make_scoped_restore (&inferior_ptid); > + inferior_ptid = ptid_t (vfork_parent->pid); > + aspace = maybe_new_address_space (); > + } I guess that's fine as a minimal patch, but it scares me when we just set part of the global state and inferior_ptid / current_inferior / current_program_space don't match. maybe_new_address_space calls gdbarch_has_shared_address_space, which calls linux_is_uclinux, which reads auxv. auxv can be read in different ways. It's hard to get convinced that only setting inferior_ptid is enough. Since we need a scoped_restore_current_thread in the current scope, can you move it a bit higher? Something like (untested): scoped_restore_current_thread restore_thread; /* 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)); std::shared_ptr<address_space> 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); Below, you can std::move the aspace when creating the new program_space: inf->pspace = new program_space (std::move (aspace)); Even better, I think, would be to pass the vfork parent inferior as a new `existing_inferior` parameter to maybe_new_address_space, and pass it all the way through linux_is_uclinux (and do some inferior switching there in order to do the target calls). The existing_inferior parameter would be an inferior that maybe_new_address_space can inspect to determine if a new address space is needed, and from which it can get an existing address space, if it determines that the address space should be shared between the existing and the new inferior. But I think it's something that can be done later, switching to the parent here is fine to keep the patch small and easily backportable. Simon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 2023-11-07 15:26 ` Simon Marchi @ 2023-11-09 15:10 ` Tom de Vries 0 siblings, 0 replies; 8+ messages in thread From: Tom de Vries @ 2023-11-09 15:10 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 11/7/23 16:26, Simon Marchi wrote: > On 11/7/23 08:19, Tom de Vries wrote: >> 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. > > I would suggest adding some assertion: > > - assert that inferior_ptid != null_ptid in > ppc_linux_nat_target::auxv_parse and > s390_linux_nat_target::auxv_parse > - assert that tid != 0 in ppc_linux_target_wordsize (and in > s390_target_wordsize), though it could use a bit of refactoring to > take tid as a parameter like ppc_linux_target_wordsize) > I've added these, though I haven't been able to test on s390. >> 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. >> >> PR gdb/30547 >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547 >> --- >> gdb/infrun.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index f0f35c23c9c..94a0924653f 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec) >> go ahead and create a new one for this exiting >> inferior. */ >> >> + std::shared_ptr<address_space> aspace; >> + { >> + /* Temporarily switch to the vfork parent, to facilitate ptrace >> + calls done during maybe_new_address_space. */ >> + scoped_restore save_inferior_ptid >> + = make_scoped_restore (&inferior_ptid); >> + inferior_ptid = ptid_t (vfork_parent->pid); >> + aspace = maybe_new_address_space (); >> + } > > I guess that's fine as a minimal patch, but it scares me when we just > set part of the global state and inferior_ptid / current_inferior / > current_program_space don't match. maybe_new_address_space calls > gdbarch_has_shared_address_space, which calls linux_is_uclinux, which > reads auxv. auxv can be read in different ways. It's hard to get > convinced that only setting inferior_ptid is enough. > > Since we need a scoped_restore_current_thread in the current scope, can > you move it a bit higher? Something like (untested): > > scoped_restore_current_thread restore_thread; > > /* 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)); > std::shared_ptr<address_space> 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); > > Below, you can std::move the aspace when creating the new program_space: > > inf->pspace = new program_space (std::move (aspace)); > Done, v3 available here ( https://sourceware.org/pipermail/gdb-patches/2023-November/203933.html ). Thanks, - Tom > Even better, I think, would be to pass the vfork parent inferior as a > new `existing_inferior` parameter to maybe_new_address_space, and pass > it all the way through linux_is_uclinux (and do some inferior switching > there in order to do the target calls). The existing_inferior parameter > would be an inferior that maybe_new_address_space can inspect to > determine if a new address space is needed, and from which it can get > an existing address space, if it determines that the address space > should be shared between the existing and the new inferior. But I think > it's something that can be done later, switching to the parent here is > fine to keep the patch small and easily backportable. > > Simon ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-09 16:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-07 13:19 [PATCH v2 0/2] [gdb] Fix segfault in for_each_block Tom de Vries 2023-11-07 13:19 ` [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 Tom de Vries 2023-11-07 15:00 ` Simon Marchi 2023-11-09 15:07 ` Tom de Vries 2023-11-09 16:26 ` Simon Marchi 2023-11-07 13:19 ` [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 Tom de Vries 2023-11-07 15:26 ` Simon Marchi 2023-11-09 15:10 ` 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).