Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8

[Doug Evans <dje at google dot com>]

[Ugh, bad To address on first try.]

Hi.

While looking into removing lwp_list from gdb (akin to what I did for
gdbserver) I found that several bits of target code are calling
init_thread_list
(grep init_thread_list *.c).
Maybe there's the odd case where target code would need to do this,
but normally when should target code *ever* do this?

[
side notes:
- having to deal with target code doing common operations (for lack of
a better phrase) is something I have to fix in another context
(prologue skipping), so it's now a bit of a pet peeve of mine
- it's likely there are more such cases (e.g. init_wait_for_inferior),
but for the nonce I'm picking init_thread_list as a basis for
discussion
- these issues are not specific to or introduced by the commit
specified in the subject line, it's just a concrete base from which to
phrase my questions
]

To try to assist you with getting a handle on my confusion, consider
remote.c:extended_remote_create_inferior_1 from gdb 6.8:

  /* Clean up from the last time we were running.  */
  init_thread_list ();
  init_wait_for_inferior ();

Now look at what's there today:

  if (!have_inferiors ())
    {
      /* Clean up from the last time we ran, before we mark the target
         running again.  This will mark breakpoints uninserted, and
         get_offsets may insert breakpoints.  */
      init_thread_list ();
      init_wait_for_inferior ();
    }

I think(!) there may be multiple ways to look at all of this as being
wrong, so pick your favorite, but here's one way: What does it matter
whether there are other inferiors as to whether
remote.c:extended_remote_create_inferior has to "clean up from the
last time we were running"?

Obviously we can't call init_thread_list if there are other inferiors,
but why are we calling init_thread_list here at all?  Why isn't this
state being managed by gdb itself (inf*.c or some such)?  I can
imagine one can look at this as just being still a work in progress on
the way to proper multi-target support.  It's stil a bit odd though to
have taken this step this way, so I'm hoping for some clarification.

Another related question I have is: Why does remote-sim.c:gdbsim_create_inferior
call init_wait_for_inferior unconditionally whereas the above code
conditions the call on !have_inferiors()?  Maybe it's a simple
oversight, but I think (emphasis on THINK, I could certainly be
missing something) we need to take a step back and ask why this code
is there at all.  Putting this code in target routines gives us a lot
of flexibility, but the cost is more mental load (for lack of a better
phrase) and more touch points when trying to fix/improve core gdb, and
I'm getting the impression that the pendulum has swung too far towards
putting general housekeeping operations in target code.