Re: MI testsuite to use PTY for inferior

[Bob Rossi <bob@brasko.net>]

> I've looked at your patch and have some comments.  I'm not very familiar
> with expect so some of what I say might be wrong.

Nick, thanks for taking the time.

> > +# INFERIOR_PTY should be set to TRUE to have the inferior work with it's
> > +# own PTY. If set to FALSE, the inferior shares GDB's PTY.
> >  #
> >  # When running over NFS, particularly if running many simultaneous
> >  # tests on different hosts all using the same server, things can
> >  # get really slow.  Give gdb at least 3 minutes to start up.
> >  #
> > -proc mi_gdb_start { } {
> > +proc mi_gdb_start { args } {
> >      global verbose
> >      global GDB
> >      global GDBFLAGS
> > @@ -99,6 +105,10 @@
> >  
> >      gdb_stop_suppressing_tests;
> >  
> > +    if { [llength $args] == 1} {
> > +	set inferior_pty [lindex $args 0]
> > +    }
> > +
> >      # Start SID.
> >      if { [info procs sid_start] != "" } {
> >  	verbose "Spawning SID"
> > @@ -117,6 +127,16 @@
> >  	    exit 1
> >  	}
> >      }
> > +
> > +    # spawn off the new pty for the inferior process
> 
> Open a new pty for the inferior process? (spawn means new process?)

I used the verb "spawn" because in order to get a new pty you have to
call 'spawn pty'. If it is prefered to say "Create the new pty ...", I
can change that.

> > +    if { [ info exists inferior_pty ] } {
> > +	spawn -pty
> > +	global mi_inferior_spawn_id
> > +	global mi_inferior_tty_name
> > +	set mi_inferior_spawn_id $spawn_id
> > +	set mi_inferior_tty_name $spawn_out(slave,name)
> > +    }
> > +
> 
> This seems to try to open a new pty even if inferior_pty is false.  Shouldn't
> the condition, here and elsewhere, be:
> 
> if { $inferior_pty } {

I think this could be a style issue, but I'm not sure. I only create the
inferior_pty variable if mi_gdb_start's INFERIOR_PTY is true. Then I
check in later calls to see if the variable inferior_pty exists via
'info exists inferior_pty'. 

I'm pretty sure this is equivalent to always creating the variable
inferior_pty and always assigning it a true/false value based on
INFERIOR_PTY. Then I could do an if on it's value, instead of simply
it's existence.

Is one of these approaches prefered over the other in GDB's testsuite or
in TCL programming?

> > -# mi_gdb_test COMMAND PATTERN MESSAGE -- send a command to gdb; test the result.
> > +# mi_gdb_test COMMAND PATTERN IPATTERN MESSAGE -- send a command to gdb; test the result.
> >  #
> >  # COMMAND is the command to execute, send to GDB with send_gdb.  If
> >  #   this is the null string no command is sent.
> >  # PATTERN is the pattern to match for a PASS, and must NOT include
> >  #   the \r\n sequence immediately before the gdb prompt.
> > +# IPATTERN is the pattern to match for the inferior's output. This will not 
> > +#   produce a PASS if successfull, but will produce a FAIL if unsuccessful.
> >  # MESSAGE is an optional message to be printed.  If this is
> >  #   omitted, then the pass/fail messages use the command string as the
> >  #   message.  (If this is the empty string, then sometimes we don't
> > @@ -533,14 +568,20 @@
> 
> With these arguments, you can't discriminate between
> 
> mi_gdb_test COMMAND PATTERN MESSAGE
> 
> and
> 
> mi_gdb_test COMMAND PATTERN IPATTERN
> 
> (MESSAGE is optional).

Yeah, this does look like a problem. Thanks. Is there a good way to work
around this in TCL? Or should I make the IPATTERN parameter necessary?
Force the user to put "" for IPATTERN? 

Or rather, should I make the MESSAGE parameter necessary? I have a feeling 
that all of the MI tests use the MESSAGE parameter. If I do that, I can
make the IPATTERN parameter optional, and last.

> Re your ChangeLog entry,
> > +	* gdb.mi/mi-console.exp: Ditto.
> > +	(47-exec-next): Use mi_gdb_test to get GDB and Inferior output.
> 
> 47-exec-next isn't a procedure.  I would just do:
> 
> * gdb.mi/mi-console.exp: Use mi_gdb_test to get GDB and Inferior output.
> 
> I know that "Ditto." and "Likewise." are used often in GDB but I prefer:
> 
> 	* gdb.mi/gdb669.exp, gdb.mi/gdb680.exp, gdb.mi/gdb701.exp,
> 	gdb.mi/gdb792.exp, gdb.mi/mi-break.exp, gdb.mi/mi-disassemble.exp,
> 	gdb.mi/mi-eval.exp: Tell mi_gdb_start to use a PTY for inferior.
> 	Update Copyright.

I will gladly change my ChangeLog sytle if either Daneil or Eli agree
with your assessment. I have currently built up my ChangeLog style as a
repetitive response from Daneil and Eli in order to comply with there
style, in order to get faster patch review time.

Thanks,
Bob Rossi