[PATCH 2/3] gdb: refactor core_target ::close and ::detach functions

[Andrew Burgess <aburgess@redhat.com>]

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.
---
 gdb/corelow.c | 75 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 24 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 216b4e70066..b4438990a7a 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -286,7 +286,7 @@ class core_target final : public process_stratum_target
 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
@@ -614,24 +614,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.  */
@@ -639,11 +638,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
@@ -1240,17 +1266,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 ();
 
   /* Note that 'this' may be dangling after this call.  unpush_target
      closes the target if the refcount reaches 0, and our close
-- 
2.25.4