[binutils-gdb] gdb/remote: iterate on pspace inferiors in remote_new_objfile

[binutils-gdb] gdb/remote: iterate on pspace inferiors in remote_new_objfile

[Simon Marchi]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=06c7226ea19c1880fc160b8890c3a5574d490709

commit 06c7226ea19c1880fc160b8890c3a5574d490709
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Fri Apr 29 23:21:14 2022 -0400

    gdb/remote: iterate on pspace inferiors in remote_new_objfile
    
    Commit 152a17495663 ("gdb: prune inferiors at end of
    fetch_inferior_event, fix intermittent failure of
    gdb.threads/fork-plus-threads.exp") broke some tests with the
    native-gdbserver board, such as:
    
        (gdb) PASS: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=child: break cond on target : vfork: break marker
        continue^M
        Continuing.^M
        terminate called after throwing an instance of 'gdb_exception_error'^M
    
    I can manually reproduce the issue by running (just the commands that
    the test does as a one liner):
    
        $ ./gdb -q --data-directory=data-directory \
              testsuite/outputs/gdb.base/step-over-syscall/step-over-vfork \
              -ex "tar rem | ../gdbserver/gdbserver - testsuite/outputs/gdb.base/step-over-syscall/step-over-vfork" \
              -ex "b main" \
              -ex c \
              -ex "d 1" \
              -ex "set displaced-stepping off" \
              -ex "b *0x7ffff7d7ac5a if main == 0" \
              -ex "set detach-on-fork off" \
              -ex "set follow-fork-mode child" \
              -ex c \
              -ex "inferior 1" \
              -ex "b marker" \
              -ex c
    
    ... where 0x7ffff7d7ac5a is the exact address of the vfork syscall
    (which can be found by looking at gdb.log).
    
    The important part of the above is that a vfork syscall creates inferior
    2, then inferior 2 executes until exit, then we switch back to inferior
    1 and try to resume it.
    
    The uncaught exception happens here:
    
        #4  0x00005596969d81a9 in error (fmt=0x559692da9e40 "Cannot execute this command while the target is running.\nUse the \"interrupt\" command to stop the target\nand then try again.")
            at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:43
        #5  0x0000559695af6f66 in remote_target::putpkt_binary (this=0x617000038080, buf=0x559692da4380 "qSymbol::", cnt=9) at /home/simark/src/binutils-gdb/gdb/remote.c:9560
        #6  0x0000559695af6aaf in remote_target::putpkt (this=0x617000038080, buf=0x559692da4380 "qSymbol::") at /home/simark/src/binutils-gdb/gdb/remote.c:9518
        #7  0x0000559695ab50dc in remote_target::remote_check_symbols (this=0x617000038080) at /home/simark/src/binutils-gdb/gdb/remote.c:5141
        #8  0x0000559695b3cccf in remote_new_objfile (objfile=0x0) at /home/simark/src/binutils-gdb/gdb/remote.c:14600
        #9  0x0000559693bc52a9 in std::__invoke_impl<void, void (*&)(objfile*), objfile*> (__f=@0x61b0000167f8: 0x559695b3cb1d <remote_new_objfile(objfile*)>) at /usr/include/c++/11.2.0/bits/invoke.h:61
        #10 0x0000559693bb2848 in std::__invoke_r<void, void (*&)(objfile*), objfile*> (__fn=@0x61b0000167f8: 0x559695b3cb1d <remote_new_objfile(objfile*)>) at /usr/include/c++/11.2.0/bits/invoke.h:111
        #11 0x0000559693b8dddf in std::_Function_handler<void (objfile*), void (*)(objfile*)>::_M_invoke(std::_Any_data const&, objfile*&&) (__functor=..., __args#0=@0x7ffe0bae0590: 0x0) at /usr/include/c++/11.2.0/bits/std_function.h:291
        #12 0x00005596956374b2 in std::function<void (objfile*)>::operator()(objfile*) const (this=0x61b0000167f8, __args#0=0x0) at /usr/include/c++/11.2.0/bits/std_function.h:560
        #13 0x0000559695633c64 in gdb::observers::observable<objfile*>::notify (this=0x55969ef5c480 <gdb::observers::new_objfile>, args#0=0x0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/observable.h:150
        #14 0x0000559695df6cc2 in clear_symtab_users (add_flags=...) at /home/simark/src/binutils-gdb/gdb/symfile.c:2873
        #15 0x000055969574c263 in program_space::~program_space (this=0x6120000c8a40, __in_chrg=<optimized out>) at /home/simark/src/binutils-gdb/gdb/progspace.c:154
        #16 0x0000559694fc086b in delete_inferior (inf=0x61700003bf80) at /home/simark/src/binutils-gdb/gdb/inferior.c:205
        #17 0x0000559694fc341f in prune_inferiors () at /home/simark/src/binutils-gdb/gdb/inferior.c:390
        #18 0x0000559695017ada in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4293
        #19 0x0000559694f629e6 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:41
        #20 0x0000559695b3b0e3 in remote_async_serial_handler (scb=0x6250001ef100, context=0x6170000380a8) at /home/simark/src/binutils-gdb/gdb/remote.c:14466
        #21 0x0000559695c59eb7 in run_async_handler_and_reschedule (scb=0x6250001ef100) at /home/simark/src/binutils-gdb/gdb/ser-base.c:138
        #22 0x0000559695c5a42a in fd_event (error=0, context=0x6250001ef100) at /home/simark/src/binutils-gdb/gdb/ser-base.c:189
        #23 0x00005596969d9ebf in handle_file_event (file_ptr=0x60700005af40, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:574
        #24 0x00005596969da7fa in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:700
        #25 0x00005596969d8539 in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
    
    If I enable "set debug infrun" just before the last continue, we see:
    
        (gdb) continue
        Continuing.
        [infrun] clear_proceed_status_thread: 965604.965604.0
        [infrun] proceed: enter
          [infrun] proceed: addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT
          [infrun] scoped_disable_commit_resumed: reason=proceeding
          [infrun] start_step_over: enter
            [infrun] start_step_over: stealing global queue of threads to step, length = 0
            [infrun] operator(): step-over queue now empty
          [infrun] start_step_over: exit
          [infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [965604.965604.0] at 0x7ffff7d7ac5c
          [infrun] do_target_resume: resume_ptid=965604.0.0, step=0, sig=GDB_SIGNAL_0
          [infrun] prepare_to_wait: prepare_to_wait
          [infrun] reset: reason=proceeding
          [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
          [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
        [infrun] proceed: exit
        [infrun] fetch_inferior_event: enter
          [infrun] scoped_disable_commit_resumed: reason=handling event
          [infrun] do_target_wait: Found 2 inferiors, starting at #1
          [infrun] random_pending_event_thread: None found.
          [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
          [infrun] print_target_wait_results:   965604.965604.0 [Thread 965604.965604],
          [infrun] print_target_wait_results:   status->kind = VFORK_DONE
          [infrun] handle_inferior_event: status->kind = VFORK_DONE
          [infrun] context_switch: Switching context from 0.0.0 to 965604.965604.0
          [infrun] handle_vfork_done: not waiting for a vfork-done event
          [infrun] start_step_over: enter
            [infrun] start_step_over: stealing global queue of threads to step, length = 0
            [infrun] operator(): step-over queue now empty
          [infrun] start_step_over: exit
          [infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [965604.965604.0] at 0x7ffff7d7ac5c
          [infrun] do_target_resume: resume_ptid=965604.0.0, step=0, sig=GDB_SIGNAL_0
          [infrun] prepare_to_wait: prepare_to_wait
          [infrun] reset: reason=handling event
          [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
          [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
        terminate called after throwing an instance of 'gdb_exception_error'
    
    What happens is:
    
     - After doing the "continue" on inferior 1, the remote target gives us
       a VFORK_DONE event.  The core ignores it and resumes inferior 1.
     - Since prune_inferiors is now called after each handled event, in
       fetch_inferior_event, it is called after we handled that VFORK_DONE
       event and resumed inferior 1.
     - Inferior 2 is pruned, which (see backtrace above) causes its program
       space to be deleted, which clears the symtabs for that program space,
       which calls the new_objfile observable and remote_new_objfile
       observer (with a nullptr objfile, to indicate that the previously
       loaded symbols have been discarded), which calls
       remote_check_symbols.
    
    remote_check_symbols is the function that sends the qSymbol packet, to
    let the remote side ask for symbol addresses.  The problem is that the
    remote target is working in all-stop / sync mode and is currently
    resumed.  It has sent a vCont packet to resume the target and is waiting
    for a stop reply.  It can't send any packets in the mean time.  That
    causes the exception to be thrown.
    
    This wasn't a problem before, when prune_inferiors was called in
    normal_stop, because it was always called at a time the target was not
    resumed.
    
    An important observation here is that the new_objfile observable is
    invoked for a change in inferior 2's program space (inferior 2's program
    space is the current program space).  Inferior 2 isn't bound to any
    process on the remote side (it has exited, that's why it's being
    pruned).  It doesn't make sense to try to send a qSymbol packet for a
    process that doesn't exist on the remote side.  remote_check_symbols
    actually attempts to avoid that:
    
       /* The remote side has no concept of inferiors that aren't running
         yet, it only knows about running processes.  If we're connected
         but our current inferior is not running, we should not invite the
         remote target to request symbol lookups related to its
         (unrelated) current process.  */
      if (!target_has_execution ())
        return;
    
    The problem here is that while inferior 2's program space is the current
    program space, inferior 1 is the current inferior.  So the check above
    passes, since inferior has execution.  We therefore try to send a
    qSymbol packet for inferior 1 in reaction to a change in inferior 2's
    program space, that's wrong.
    
    This exposes a conceptual flaw in remote_new_objfile.  The "new_objfile"
    event concerns a specific program space, which can concern multiple
    inferiors, as inferiors can share a program space.  We shouldn't
    consider the current inferior at all, but instead all inferiors bound to
    the affected program space.  Especially since the current inferior can
    be unrelated to the current program space at that point.
    
    To be clear, we are in this state because ~program_space sets itself as
    the current program space, but there is no more inferior having that
    program space to switch to, inferior 2 has already been unlinked.
    
    To fix this, make remote_new_objfile iterate on all inferiors bound to
    the affected program space.  Remove the target_has_execution check from
    remote_check_symbols, replace it with an assert.  All callers must
    ensure that the current inferior has execution before calling it.
    
    Change-Id: Ica643145bcc03115248290fd310cadab8ec8371c

Diff:
---
 gdb/remote.c | 87 +++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 28 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6914947c172..fde6df3f84d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1016,13 +1016,20 @@ struct stop_reply : public notif_event
   int core;
 };
 
+/* Return TARGET as a remote_target if it is one, else nullptr.  */
+
+static remote_target *
+as_remote_target (process_stratum_target *target)
+{
+  return dynamic_cast<remote_target *> (target);
+}
+
 /* See remote.h.  */
 
 bool
 is_remote_target (process_stratum_target *target)
 {
-  remote_target *rt = dynamic_cast<remote_target *> (target);
-  return rt != nullptr;
+  return as_remote_target (target) != nullptr;
 }
 
 /* Per-program-space data key.  */
@@ -5116,13 +5123,10 @@ remote_target::remote_check_symbols ()
   char *tmp;
   int end;
 
-  /* The remote side has no concept of inferiors that aren't running
-     yet, it only knows about running processes.  If we're connected
-     but our current inferior is not running, we should not invite the
-     remote target to request symbol lookups related to its
-     (unrelated) current process.  */
-  if (!target_has_execution ())
-    return;
+  /* It doesn't make sense to send a qSymbol packet for an inferior that
+     doesn't have execution, because the remote side doesn't know about
+     inferiors without execution.  */
+  gdb_assert (target_has_execution ());
 
   if (packet_support (PACKET_qSymbol) == PACKET_DISABLE)
     return;
@@ -14575,28 +14579,55 @@ show_remote_cmd (const char *args, int from_tty)
 static void
 remote_new_objfile (struct objfile *objfile)
 {
-  remote_target *remote = get_current_remote_target ();
+  /* The objfile change happened in that program space.  */
+  program_space *pspace = current_program_space;
 
-  /* First, check whether the current inferior's process target is a remote
-     target.  */
-  if (remote == nullptr)
-    return;
+  /* The affected program space is possibly shared by multiple inferiors.
+     Consider sending a qSymbol packet for each of the inferiors using that
+     program space.  */
+  for (inferior *inf : all_inferiors ())
+    {
+      if (inf->pspace != pspace)
+	continue;
 
-  /* When we are attaching or handling a fork child and the shared library
-     subsystem reads the list of loaded libraries, we receive new objfile
-     events in between each found library.  The libraries are read in an
-     undefined order, so if we gave the remote side a chance to look up
-     symbols between each objfile, we might give it an inconsistent picture
-     of the inferior.  It could appear that a library A appears loaded but
-     a library B does not, even though library A requires library B.  That
-     would present a state that couldn't normally exist in the inferior.
-
-     So, skip these events, we'll give the remote a chance to look up symbols
-     once all the loaded libraries and their symbols are known to GDB.  */
-  if (current_inferior ()->in_initial_library_scan)
-    return;
+      /* Check whether the inferior's process target is a remote target.  */
+      remote_target *remote = as_remote_target (inf->process_target ());
+      if (remote == nullptr)
+	continue;
+
+      /* When we are attaching or handling a fork child and the shared library
+	 subsystem reads the list of loaded libraries, we receive new objfile
+	 events in between each found library.  The libraries are read in an
+	 undefined order, so if we gave the remote side a chance to look up
+	 symbols between each objfile, we might give it an inconsistent picture
+	 of the inferior.  It could appear that a library A appears loaded but
+	 a library B does not, even though library A requires library B.  That
+	 would present a state that couldn't normally exist in the inferior.
+
+	 So, skip these events, we'll give the remote a chance to look up
+	 symbols once all the loaded libraries and their symbols are known to
+	 GDB.  */
+      if (inf->in_initial_library_scan)
+	continue;
+
+      if (!remote->has_execution (inf))
+	continue;
+
+      /* Need to switch to a specific thread, because remote_check_symbols will
+         set the general thread using INFERIOR_PTID.
 
-  remote->remote_check_symbols ();
+	 It's possible to have inferiors with no thread here, because we are
+	 called very early in the connection process, while the inferior is
+	 being set up, before threads are added.  Just skip it, start_remote_1
+	 also calls remote_check_symbols when it's done setting things up.  */
+      thread_info *thread = any_thread_of_inferior (inf);
+      if (thread != nullptr)
+	{
+	  scoped_restore_current_thread restore_thread;
+	  switch_to_thread (thread);
+	  remote->remote_check_symbols ();
+	}
+  }
 }
 
 /* Pull all the tracepoints defined on the target and create local