On Saturday 08 November 2008 15:04:31 Eli Zaretskii wrote:

> Finally, this chapter seems to be not on design of MI, but more about
> advice to frontend implementors.  So I think it should be renamed
> accordingly, and some introductory text added to its beginning saying
> this is the intent of the chapter.

I don't quite agree. These section describes the main building blocks and 
concepts of GDB/MI, and is necessary to understand anything in GDB/MI
docs. It is not some optional "best practices" document -- if read failed
to read what notifications are, or does not know how MI manages contexts,
he cannot create a working frontend.

> > +parts --- commands sent to @value{GDBN}, responses to those commands
> 
> It is best not to leave spaces around "---", the results look better
> in print.

Do you think you can summarize such style requirements for GDB manual somewhere?
(It appears that there's no universal opinion on this matter, so this is necessary
a GDB-local guideline).


> >                                  Status notifications are used to
> 
> Wouldn't it be better to make "Status notifications" a separate @item
> in this list?

I think they are very similar from frontend point of view -- in that frontend
does only minimal processing of those notification, and won't break if they
are not emitted.

> >                                                       Therefore, it is
> > +preferrable that each MI command explicitly specify which thread and
>    ^^^^^^^^^^^
> "preferable".  Also, perhaps rephrase as "Therefore, we recommend that
> each MI command ...".  This removes passive tense from the sentence,
> which makes the sentence more concise and clear.

I think it's not strong enough. How about:

	Therefore, each MI command should explicitly specify which thread and

(And then, I'd have to adjust the last sentence of the next paragraph to remove
weak "suggested", and say that omitting --thread and --frame is allowed only for 
backward compatibility)

> > +On some targets, @value{GDBN} is capable of processing MI commands
> > +even while the target is running.  This is called asynchronous command
> > +execution.
> 
> Whenever you first introduce a term, give it the @dfn markup, which
> will make stand out in the text.  In this case, since this term is
> explained elsewhere in the manual, it is also a good idea to add a
> @pxref to that other node.

Actually, we don't have a section for CLI async mode, we have a section
for CLI non-stop mode.

> > +that even commands that operate on global state (like global
> > +variables, or breakpoints), still access the target in the context of
> > +a specific thread
> 
> What do you mean by "global variables" here?  As written, the text
> seems to say that global variables and breakpoints are commands, or
> maybe global state, which doesn't sound right to me.  "Breakpoints"
> could be replaced with "breakpoint commands", but I don't know what
> replacement to suggest for "global variables".

global variables, and breakpoints, are examples of the "global state"
that GDB commands can operate on. The point here is that even some
command appears to access only global state -- for example a global
variable, or a breakpoint list, the access is still is done in context
of a specific thread, and will fail, on may targets, if that thread is running....

> 
> >                    so frontend should try perform such operations on a
> > +stopped thread.
> 
> I don't see how this conclusion follows from the fact that commands
> access the target in the context of some thread.  Why doing that "on a
> stopped thread" will solve whatever problem you are trying to explain
> here? and what does "operations ON a stopped thread" mean in this
> context, anyway?

... and with the explanation above, if doing operations in context of running
thread may fail even if that operation only accesses global state, the frontend
better pick a thread that is stopped.

> > +Since the set of allowed commands depends on the target, this
> > +documention does not list which commands are allowed. Also,
> > +@value{GDBN} does not try to simulate asynchronous execution of
> > +commands that fail -- in particular, it does not try to briefly
> > +interrupt an executing thread to execute a command, and it does not
> > +try to automatically pick a stopped thread and execute a command in
> > +the context of that thread.
> > +
> > +Two commands that are explicitly required to always work are
> > +@code{-exec-interrupt}, to stop a thread, and @code{-thread-info}, to
> > +find the state of each thread.
> 
> I fail to understand why these two paragraphs are useful, nor why they
> are in this particular place.  If the first sentence of the first
> paragraph is related to the second paragraph, let's put them together,
> and let's also make the first sentence less general ("this
> documentation", as written, seems to refer to the entire manual).
> 
> As for the second sentence of the first paragraph, I completely fail
> to grasp the significance of what you are saying, or to parse the
> "does not try to simulate asynchronous execution of commands that
> fail" part.  Can you help me understand what you are trying to say
> here?

Earlier we've said that GDB commands *may* fail is executed in context of a
running thread. The immediate question is whether GDB/MI documentation has
more accurate information what commands will fail in what cases. This
paragraph states that such information is highly dependent on the target,
and GDB/MI cannot promise anything. It further tries to state that if target
cannot perform some operation in the context of a running thread, GDB will
not try to simulate that, for example, by stopping thread, doing the operation,
and resuming thread.

> 
> > +In future, support for debugging several hardware systems, each with
>    ^^^^^^^^^
> "In the future"
> 
> > +several cores, each with different processes, is likely.
> 
> "is likely" is too far from its subject, which makes this sentence
> awkward to read.  Suggest to rearrange this sentence:
> 
>   In the future, it is likely that @value{GDBN} will support debugging
>   of several hardware systems, each one having several cores with
>   several different processes running on each core.
> 
> Btw, talking about "the future" in a manual is problematic, because
> "the future" turns out to be present at some point, and then the text
> sounds obsolete for no good reason.  How about replacing "in the
> future" with "on some platforms" (and getting rid of "will" and the
> future tense)?

Ok, did so.

> 
> > +                    Every command that accepts the @samp{--thread}
> > +does not care of the thread's position in the target hierarchy and
> 
> What is "the thread's position in the target hierarchy"? what
> hierarchy is being referenced here?

I've ended up rewriting entire section about thread groups -- I could
not find a way to address your comments with local edits.

> > +@item =thread-created,id="@var{id}",group-id="@var{gid}"
> > +@itemx =thread-exited,id="@var{id}",group-id="@var{gid}"
> >  A thread either was created, or has exited.  The @var{id} field
> > -contains the @value{GDBN} identifier of the thread.
> > +contains the @value{GDBN} identifier of the thread.  The @var{gid}
> > +field identifies the thread group this thread belongs to.
> 
> Do we have the description of what exactly is a thread group id,
> anywhere in the manual?  If so, please add here an xref to that
> place.  If we don't have such a description, we should add one.
 
Well, we don't have description for thread id, either -- it's just
a string.

> >  Resumes the execution of the inferior program until a breakpoint is
> > -encountered, or until the inferior exits.
> > +encountered, or until the inferior exits.  In all-stop mode, may
> 
> A @pxref to the node that describes the all-stop mode would be useful
> here.

Done.

> > +resume only one thread, or all threads, depending on the value of the
> > +@samp{scheduler-mode} variable.
> 
> You mean scheduler-locking, not scheduler-mode, right?  If so, please
> add a reference to the node that describes it.

It's the same node as for All-Stop Mode, so it's already referred to.

> > +If an expression specified when creating a fixed variable object
> > +refers to a local variable, the variable object becomes bound to the 
> > +frame the local variable belongs to.
> 
> What is the meaning of a local variable _belonging_ to a frame?  Do
> you mean the frame in whose scope it exists at the time of specifying
> the expression for a fixed variable?

Yes. Or, rather, my model is that local variables do not exist independently,
but rather we have a set of stack frames, and each stack frame has a number
of local variables. So a single local variable at compilation time may correspond
to multiple local variable, in different threads/frames, at runtime.

> 
> >                                       For multithreaded programs, the
> > +variable object also becomes bound to the thread.
> 
> Again, what thread is that? how is it determined?

It might be best to reword the last two sentences thusly:

	If an expression specified when creating a fixed variable object
	refers to a local variable, the variable object becomes bound to the
	thread and frame in which the variable object is created.

> > +@smallexample
> > +(gdb)
> 
> This should be @value{GDBP}, I think.

OOC, what practical differences or benefits that will have?

> 
> > +-list-thread-groups
> > +^done,threads=[],groups=[@{id="17",type="process",pid="yyy",num_children="2"@}]
> 
> The command description says it will report thread groups, but the
> output shown in the example also includes "threads=[]".  What is that
> part?

I've revised this.


Thanks for feedback. I attach both the revised version of the patch, and the delta
relatively to the previous version.

Thanks,
Volodya