Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8

[Doug Evans <dje at google dot com>]

Pedro Alves writes:
 > On 09/20/2014 07:39 PM, Doug Evans wrote:
 > > [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?
 > 
 > > 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.
 > 
 > Really not sure what sort of answer you're looking for.

The most succinct way of expressing what I'm looking for that I can
think of is that I want to understand this code, and I don't.
There are several instances of the !have_inferiors() check, I picked
this one because it seemed like as good a choice as any.

 > I don't really recall history on that level of detail,
 > but, before 6c95b8df, the only kind of multi-process GDB supported,
 > was the model used by DICOS.  On that platform, we didn't use
 > "run" at all, only "attach" and all programs shared the program
 > space.  Well, before 6c95b8df, GDB didn't even have the
 > concept of a program space, but it didn't matter; even now that
 > it does, a single program space is the model that best fits.
 > 
 > So when we got to making GDB's multi-process fit for Linux,
 > which was the point of that patch, we must have stumbled on that
 > init_thread_list call in remote.c:extended_remote_create_inferior_1,
 > as soon as we did two "run"s in a row.  So the change in that
 > patch just looks like the conservative change.

If this is just a temp hack to play it safe, then that explains it.
["this" being the wrapping of the calls to init_thread_list
and init_wait_for_inferior with "if (!have_inferiors ())"]
I read what I could from the thread when the patch was submitted
and I couldn't find anything. I don't know this code well enough
to be comfortable with any assumptions.

It's too bad FIXMEs are frowned on in gdb-land. I think this is a good
site for one.  Assuming this is just a work-in-progress, the code really
is confusing. For example, what if have_inferiors() returns TRUE?
That means that we won't "Clean up from the last time we ran, ...".
The reader is left with questions like
"Then what? Is the system left in a broken state because
we haven't cleaned up? Or is the cleanup done elsewhere? If so, where?
Or do things just not need to be cleaned up in this case? If so, why?"

OTOH, if a FIXME isn't appropriate here, and the code is ok as-is
(I'm not making a value judgement here, I still have to understand the code
first), I'd still like to understand it so I can add a comment to help the next
reader (which if it's me a year from now I may well have forgotten this
conversation :-)).

 > The comment change you show wasn't done by that patch.
 > A trivial git blame 6c95b8df^ points at:
 > 
 > commit 45280a5259f209ba74ed8255674a3fd345307a55
 > Author:     Daniel Jacobowitz <drow@false.org>
 > AuthorDate: Thu May 8 16:08:10 2008 +0000
 > 
 >         * remote.c (extended_remote_create_inferior_1): Clean up
 >         before marking the target running.
 > 
 > I didn't dig deeper than that, but git blame further will
 > probably point out something ancient.

Yeah, I went back a bit in the history, but it didn't help me
understand the code that's there today.

 > > 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()? 
 > 
 > Most probably because nobody has ever tried making remote sim work
 > with multiple processes at the same time, so nobody ever stumbled
 > on that.
 >
 > > 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.
 > 
 > Huh?  I think you're getting this backwards.  You make it sound like
 > we've been adding more of this stuff in target code ("putting"),
 > while instead these are _ancient_ code that over the years we've
 > been cleaning up.

Well, yes we have been adding non-target-specific code to target files.
E.g., I don't see how the trustability of compilers to emit good
data for prologues varies by target, and yet we're doing different things
in amd64_skip_prologue vs arm_skip_prologue w.r.t. gcc vs clang.
I could certainly be wrong here, but I don't think so.
Can gcc get this right for ARM enough that we accept the risk of the
odd bug whereas with AMD64 we've chosen to not accept this risk?
Here's a case where some cleanup is needed, but this is relatively
freshly added code.
[From one perspective *-tdep files are different than
target files like remote.c. But it's still a case of balancing
target-specific and non-target-specific code.]

Plus, pending an understanding of the !have_inferiors() check above (and
elsewhere), it's not clear to me we have always been cleaning up. I can't
know until I understand why things are the way they are. And if we haven't
always been cleaning up maybe more awareness is needed. OTOH, maybe we have,
so I'm asking.

So in addition to understanding the !have_inferiors() checks,
I want to bring this issue up to help raise awareness.
Not with anyone in particular but in general.
I only put you and Stan in the To line because I
think you guys can shed light on the use of !have_inferiors().

 > Why not just git blame on that one?  It shows:
 > 
 > 40b92220 (Jim Kingdon          1993-09-17 17:27:43 +0000  467)
 > 8501c742 (Stu Grossman         1996-08-13 00:01:37 +0000  468)   gdbsim_kill ();
 > 40b92220 (Jim Kingdon          1993-09-17 17:27:43 +0000  469)   remove_breakpoints ();
 > ec25d19b (Steve Chamberlain    1993-01-03 22:36:04 +0000  470)   init_wait_for_inferior ();
 > ec25d19b (Steve Chamberlain    1993-01-03 22:36:04 +0000  471)

Yeah. Alas this data didn't help with my question.