From: Andrew Burgess Date: Fri, 27 Mar 2026 11:29:07 +0000 (+0000) Subject: gdb: refactor core_target ::close and ::detach functions X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3780b9993c973a2b68b496b80eddb820c0932cc0;p=thirdparty%2Fbinutils-gdb.git gdb: refactor core_target ::close and ::detach functions This patch refactors the core_target ::close, ::detach, and ::clear_core functions so that the m_core_bfd is not cleared before the core_target is deleted. My motivation for this change is the get_inferior_core_bfd function. This function checks to see if an inferior has a core_target in its target stack, if it does then there is an assert that the core_target's m_core_bfd will not be NULL. Currently, this assert is mostly correct, but during a call to target_detach, the assert stops being true. Calling target_detach will call core_target::detach, which calls core_target::clear_core, which sets m_core_bfd to NULL. The core_target is not unpushed from the inferior's target stack until GDB returns from ::clear_core back to ::detach. This means that, for a short period of time, from the moment m_core_bfd is set to NULL in ::clear_core, until the unpush back in ::detach, the assertion in get_inferior_core_bfd is no longer valid. Within this window we trigger the core_file_changed observer. If any of the observers call get_inferior_core_bfd then the assertion will trigger. Currently, no observer calls get_inferior_core_bfd, the observer just clears some caches. However, in the next commit I'd like to add a new Python event API for when the core file is changed. User code attached to this event can call Inferior.corefile, which is implemented by a call to get_inferior_core_bfd, and in this case the assert will trigger. I could just delete the assertion, but I'd prefer to not do that. I think by restructuring the code we can leave the assertion in place. The first thing to understand is that a core_target is not shareable, see process_stratum_target::is_shareable. This means that a core_target will only appear within a single inferior's target stack. Next there are two ways that a core_target can be removed from an inferior's target stack. First is via target_detach, this is triggered either by the 'detach' command, or by the 'core-file' command without a core filename. In both these cases target_detach is called, which calls core_target::detach, this function unpushes the core_target from the inferior's target stack. As the core_target is not shareable the reference count will return to zero, at which point the core_target will be closed and deleted. The second way that a core_target can be removed from an inferior's target_stack is by direct replacement. If a user loads a different process_stratum_target, e.g. 'target remote ....' then this replaces the core_target in the target_stack. Doing this reduces the core_target's reference count to zero, which causes the target to be closed and deleted. These two approaches differ in that the first calls core_target::detach and then core_target::close, while the second avoids calling ::detach, and immediately calls ::close. My proposal is that we can defer calling the core_file_changed observer until core_target::close. By this point the core_target will have been removed from the inferior's target_stack, and so the assert in get_inferior_core_bfd will still hold. We already call the observer at this point for the process_stratum_target replacement case (e.g. when a user does 'target remote ...' to replace a core file target), this proposal would just mean that we always call the observer at this point, rather than potentially calling it earlier in the detach case. This commit does this change by making a number of changes: * The code to reset m_core_bfd to NULL, and to trigger the core_file_changed observer, is removed from core_target::clear, this only leaves the code relating to exiting and cleaning up after the inferior that was created for inspecting the core file. * To reflect this change of focus, core_target::clear_core is renamed to core_target::exit_core_file_inferior. * In core_target::detach, nothing really needs to change other than calling ::exit_core_file_inferior. I have added an assert that reflects the fact that ::detach cannot be called twice on the same core_target (after the first call the core_target will always be closed and deleted). * In core_target::close the call to ::exit_core_file_inferior needs to be conditional. As discussed above, in the replacement case, ::close can be called without first calling ::detach. But in the target_detach case, ::detach will have already been called, and as a result ::exit_core_file_inferior will have already been called. * Also in core_target::close, we now unconditionally trigger the core_target_changed observer. This commit is a refactor, and there should be no user observable changes. --- diff --git a/gdb/corelow.c b/gdb/corelow.c index c66ff50cbc2..d2e352fd5c1 100644 --- a/gdb/corelow.c +++ b/gdb/corelow.c @@ -286,7 +286,7 @@ public: private: /* per-core data */ /* Get rid of the core inferior. */ - void clear_core (); + void exit_core_file_inferior (); /* The core's section table. Note that these target sections are *not* mapped in the current address spaces' set of target @@ -615,24 +615,23 @@ core_target::build_file_mappings () /* An arbitrary identifier for the core inferior. */ #define CORELOW_PID 1 +/* See class declaration above. */ + void -core_target::clear_core () +core_target::exit_core_file_inferior () { - if (this->core_bfd () != nullptr) - { - switch_to_no_thread (); /* Avoid confusion from thread - stuff. */ - exit_inferior (current_inferior ()); + /* Opening a core file ensures that some thread, even if it's just a + "fake" thread, will have been selected. */ + gdb_assert (inferior_ptid != null_ptid); - /* Clear out solib state while the bfd is still open. See - comments in clear_solib in solib.c. */ - clear_solib (current_program_space); + /* Avoid confusion from thread stuff. */ + switch_to_no_thread (); - m_core_bfd.reset (nullptr); + exit_inferior (current_inferior ()); - /* Notify that the core file has changed. */ - gdb::observers::core_file_changed.notify (current_inferior ()); - } + /* Clear out solib state while the bfd is still open. See + comments in clear_solib in solib.c. */ + clear_solib (current_program_space); } /* Close the core target. */ @@ -640,11 +639,38 @@ core_target::clear_core () void core_target::close () { - clear_core (); + /* The core BFD is set when the core_target is created and attached to + the inferior. It is never explicitly cleared, instead m_core_bfd will + have its reference count reduced when the core_target is deleted. */ + gdb_assert (this->core_bfd () != nullptr); + + /* If we called ::detach before calling ::close then the inferior will + have already been exited. This will happen if the user clears the + core file with the 'core-file' or 'detach' commands. + + However, if the user just causes the core_target to be unpushed, by + pushing an alternative target, e.g. 'target remote ....', then we will + not call ::detach before calling ::close. + + In the former case we don't want to exit the inferior twice; this is + mostly harmless except it causes two 'exited' events to be emitted in + the Python API, which isn't ideal. + + As opening a core_target always ensures that some thread is selected, + then we can tell if exit_core_file_inferior has already been called by + checking if no thread is now selected. */ + if (inferior_ptid != null_ptid) + exit_core_file_inferior (); /* Core targets are heap-allocated (see core_target_open), so here we delete ourselves. */ delete this; + + /* Notify that the core file has changed. This is intentionally done + after the core_target is deleted as nothing in here depends on the + core_target itself, the core_target has already been removed from the + inferior's target stack by this point. */ + gdb::observers::core_file_changed.notify (current_inferior ()); } /* Look for sections whose names start with `.reg/' so that we can @@ -1242,17 +1268,18 @@ void core_target::detach (inferior *inf, int from_tty) { /* The core BFD is set when the core_target is created and attached to - the inferior. It is only cleared during detach or close. After - detaching the core target will be closed and deleted, so detach can - never be called twice. What this means is that detach will never be - called without the core BFD being set. */ + the inferior. It is never explicitly cleared, instead m_core_bfd will + have its reference count reduced when the core_target is deleted. */ gdb_assert (this->core_bfd () != nullptr); - /* Get rid of the core. Don't rely on core_target::close doing it, - because target_detach may be called with core_target's refcount > 1, - meaning core_target::close may not be called yet by the - unpush_target call below. */ - clear_core (); + /* Similarly, the inferior and thread are created when the core_target is + opened, and are only exited when this function, or ::close are called. + As calling ::close deletes the core_target, then when this function is + called, the inferior will still be live. */ + gdb_assert (inferior_ptid != null_ptid); + + /* Get rid of the core inferior. */ + exit_core_file_inferior (); /* This detach method should only be called from target_detach, which holds a reference to this core_target. As such, this core_target will