Re: [v6] multi-executable support

[Pedro Alves <pedro at codesourcery dot com>]

On Monday 05 October 2009 22:55:09, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Mon, 5 Oct 2009 16:59:20 +0100
> > 
> > Eli, could you please take a new look at the docs bits?
> > I may have missed something, but I hope nothing big.
> 
> Ack.  I also spotted a few typos in the comments, see below.

Thanks.

> 
> > +/* These functions concerns about actual breakpoints inserted in the
> 
> "These functions concerns" doesn't sound right.

Fixed.

> 
> > +/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
> > +   same breakpoint location.  In most targets, this is will be true if
>                                                        ^^
> I think "is" is redundant here.

Fixed.

> 
> > +    /* The program had previously vforked, and now the child is done
> > +       with the shared memory region, because it exec'ed or exited.
> > +       Note that the event if reported to the vfork parent.  This is
>                               ^^
> Did you mean "is"?

Yes.

> 
> > +	  /* If this is a vfork child exiting, then the pspace and
> > +	     aspaces where shared with the parent.  Since we're
>                      ^^^^^
> "were", right?

Right.

> 
> > Index: src/gdb/doc/gdb.texinfo
> 
> This is okay, but I have a few comments:
> 
> > +In addition, below each inferior line, @value{GDBN} prints extra
> > +information that isn't suitable to display in tabular form.  For
> > +example, extra vfork parent/child relationships.
> 
> How about showing some of that in the example?  Otherwise, this note
> sounds too mysterious, IMO.

The idea was to not distract the user here much with a use case
that isn't that common, and have she follow the xref
to see an example of that.  It seems I posted the example in the "Forks"
section, but didn't finish what I wanted to with it :-/  Will fix.

> 
> > +Many commands will work the same with multiple programs as with a
> > +single program: E.g., @code{print myglobal} will simply display the
>                    ^^^^
> "e.g.", in lower case.

Fixed.

> 
> > +Ocasionaly, when debugging @value{GDBN} itself, it may be useful to
>    ^^^^^^^^^^
> "Occasionally"
> 
> > +get more info about the relationship or inferiors, programs, address
>                                         ^^
> "of", I presume

Fixed and yes.

> 
> > +@smallexample
> > +(@value{GDBP}) maint info program-spaces
> > +  Id   Executable
> > +  2    goodbye
> > +        Bound inferiors: ID 1 (process 21561)
> > +* 1    hello
> > +@end smallexample
> > +@end table
> > +
> > +Here we can see that no inferior is running the program @code{hello},
> > +while @code{process 21561} is running the program @code{goodbye}.  On
> > +some targets, it is possible that multiple inferiors are bound to the
> > +same program space.  The most common example is that of debugging both
> > +the parent and child processes of a @code{vfork}
> > +call. @xref{Forks,,Debugging Forks}.
> 
> Since this is about program spaces, not about inferiors, I'd suggest
> to show an example that includes multiple inferiors in the same
> program space.  Otherwise, this looks awfully similar to "info
> inferiors" and the purpose of this command remains unclear.

Good idea, will do that.

> 
> > +@code{follow-exec-mode} can be:
> > +
> > +
> > +  new - the debugger creates a new inferior and rebinds the process \n\
> > +to this new inferior.  The program the process was running before\n\
> > +the exec call can be restarted afterwards by restarting the original\n\
> > +inferior.\n\
> > +\n\
> > +  same - the debugger keeps the process bound to the same inferior.\n\
> > +The new executable image replaces the previous executable loaded in\n\
> > +the inferior.  Restarting the inferior after the exec call restarts\n\
> > +the executable the process was running after the exec call.\n\
> > +\n\
> > +By default, the debugger will use the same inferior."),
> 
> This looks like copy/paste from the doc strings in the source.  It
> should be removed, I think.

Bah, of course.

> 
> > +@value{GDBN} creates a new inferior and rebinds the process to this
> > +new inferior.  The program the process was running before the exec
>                                                                  ^^^^
> "exec" should be in @code.
> 

Fixed.

> > +inferior.  Restarting the inferior after the exec call restarts the
> > +executable the process was running after the exec call.  This is the
> > +default mode.
> 
> Same here.
> 
> > +(@value{GDBP}) info sspaces
> 
> Should be "maint info program-spaces", right?

That would be the direct mapping, but "info inferiors" would
be better.  I add adjusted the "new" variant but missed
the "same" variant.  Thanks for catching.

> 
> > Index: src/gdb/NEWS
> 
> This is okay, with a couple of comments:
> 
> > +maint info program-spaces
> > +  List the program spaces loaded into GDB.
> 
> This should probably be after the *-inferior commands, not before them.

Hmm, bah, we missed something important here.  Since this feature missed
7.0, I get to add a "Changes since GDB 7.0" section, and rewire these
entries there.

> 
> > +  When the program execs, request to keep the previous executable'
>                                                           ^^^^^^^^^^^
> "executable's", right?

Right.

> 
> > +  symbol loaded, and load the new executable in a new symbol-space; or
> > +  request GDB to reuse the symbol-space and replace the previous
> > +  executable's symbols with the new executable.
> 
> I think you want to replace "symbol space" with "program space".

Yeah.  I think I should rewrite that entry a bit.  This text
predates the "always-an-inferior" change and the corresponding
docs/help entry of the setting doesn't describe the exact same
thing.

> 
> > +/* Add a new empty program space to the program space list, and binds it
> > +   to ASPACE.  Returns the pointer to the new object.  */
> 
> Either "add", "bind", and "return", or "adds", "binds", and "returns".

Fixed.

> 
> > +/* If ARGS is NULL or empty, print information about all symbol
> > +   spaces.  Otherwise, ARGS is a text representation of a LONG
> 
> "program spaces".

Clearly my grepping failed to consider lines breaks.  Fixed.

> > +/* All known objfiles are kept in a linked list.  This points to the
> > +   root of this list. */
> 
> I believe "root" is used for trees.  Lists have a "head".

I'm sure I could plant some species of lists.  :-P  Thanks, fixed.

I'll post a new patch later, and I'll re-quote the bits that need
review in particular.  Thanks much, I couldn't spot these issues even
after staring at the patch for long.

-- 
Pedro Alves