[v2] GDB/testsuite: Add a way to send multiple init commands

  On Fri, 20 Jun 2014, Pedro Alves wrote:

> > 2014-06-11  Maciej W. Rozycki  <macro@mips.com>
> >             Maciej W. Rozycki  <macro@codesourcery.com>
> > 
> > 	* lib/gdb.exp (gdb_run_cmd): Process `gdb_init_commands'.
> > 	* lib/mi-support.exp (mi_run_cmd): Process `gdb_init_commands'.
> > 	* README (Board Settings): Document `gdb_init_command' and 
> > 	`gdb_init_commands'.
> 
> I don't particularly see much need for this -- I do this in my
> boards instead:
> 
>  set GDBFLAGS ""
>  set GDBFLAGS "${GDBFLAGS} -ex \"set breakpoint always-inserted on\""
>  set GDBFLAGS "${GDBFLAGS} -ex \"set target-async 1\""
> 
> See:
> 
> https://sourceware.org/gdb/wiki/TestingGDB#Passing_an_option_to_GDB_.2BAC8_Running_the_whole_test_suite_in_a_non-default_mode
> 
> But, given gdb_init_command exists and this can be made
> non-intrusive, it's fine with me to add the new option.

 That and I think there are two issues with passing commands as 
command-line arguments:

1. They are always executed, perhaps unnecessarily whereas 
   `gdb_init_command' and consequently `gdb_init_commands' are only 
   interpreted when a target connection is about to be made (this is more 
   of an aesthetic matter, but still).

2. Some environments have a limit, maybe quite low, on the maximum length 
   of a command line or command-line arguments they accept (now that is 
   more real).

 BTW, in updating DejaGNU documentation that refers to `gdb_init_command' 
I've noticed it lists a command that pokes at a CPU register there -- has 
the semantics of the setting changed sometime, perhaps long ago?  Does 
anybody know/remember?

> Thought, I'd much prefer if this code that appears twice:
> 
> > +    set commands ""
> >      if [target_info exists gdb_init_command] {
> > -	send_gdb "[target_info gdb_init_command]\n"
> > +	lappend commands [target_info gdb_init_command]
> > +    }
> > +    if [target_info exists gdb_init_commands] {
> > +	set commands [concat $commands [target_info gdb_init_commands]]
> > +    }
> > +    foreach command $commands {
> 
> was factored out to a procedure that returns the command list.  Like:
> 
> # Comment here
> proc gdb_init_commands {} {
>     set commands {}
>     if [target_info exists gdb_init_command] {
> 	lappend commands [target_info gdb_init_command]
>     }
>     if [target_info exists gdb_init_commands] {
> 	set commands [concat $commands [target_info gdb_init_commands]]
>     }
>     return commands
> }
> 
> And then, both users can do
> 
>     foreach command [gdb_init_commands] {

 Done, as below, and retested.  Any other questions or comments?  
Otherwise OK to apply?

 Here's the original description repeated, for a reference:

--------------------------------------------------------------------------
 Right now we provide a board info entry, `gdb_init_command', that allows 
one to send a single command to GDB before the program to be debugged is 
started.  This is useful e.g. for slow remote targets to change the 
default "remotetimeout" setting.  Occasionally I found a need to send 
multiple commands instead, however this cannot be achieved with 
`gdb_init_command'.

 This change therefore extends the mechanism by adding a TCL list of GDB 
commands to send, via a board info entry called `gdb_init_commands'.  
There is no limit as to the number of commands put there.  The old 
`gdb_init_command' mechanism remains supported for compatibility with 
existing people's environments.  I have a separate change for DejaGNU too, 
being posted right away, that adds an `add_board_info' procedure that 
makes it easy to append entries there; it's not strictly needed here or 
anywhere in our testsuite though.
--------------------------------------------------------------------------

2014-07-10  Maciej W. Rozycki  <macro@mips.com>
            Maciej W. Rozycki  <macro@codesourcery.com>

	* lib/gdb-utils.exp: New file.
	* lib/gdb.exp (gdb_run_cmd): Call gdb_init_commands, replacing
	inline `gdb_init_command' processing.
	* lib/mi-support.exp (mi_run_cmd): Likewise.
	* README: Document `gdb_init_command' and `gdb_init_commands'.

  Maciej

gdb-init-commands.diff
  

On 07/10/2014 01:17 AM, Maciej W. Rozycki wrote:
> On Fri, 20 Jun 2014, Pedro Alves wrote:
> 
>>> 2014-06-11  Maciej W. Rozycki  <macro@mips.com>
>>>             Maciej W. Rozycki  <macro@codesourcery.com>
>>>
>>> 	* lib/gdb.exp (gdb_run_cmd): Process `gdb_init_commands'.
>>> 	* lib/mi-support.exp (mi_run_cmd): Process `gdb_init_commands'.
>>> 	* README (Board Settings): Document `gdb_init_command' and 
>>> 	`gdb_init_commands'.
>>
>> I don't particularly see much need for this -- I do this in my
>> boards instead:
>>
>>  set GDBFLAGS ""
>>  set GDBFLAGS "${GDBFLAGS} -ex \"set breakpoint always-inserted on\""
>>  set GDBFLAGS "${GDBFLAGS} -ex \"set target-async 1\""
>>
>> See:
>>
>> https://sourceware.org/gdb/wiki/TestingGDB#Passing_an_option_to_GDB_.2BAC8_Running_the_whole_test_suite_in_a_non-default_mode
>>
>> But, given gdb_init_command exists and this can be made
>> non-intrusive, it's fine with me to add the new option.
> 
>  That and I think there are two issues with passing commands as 
> command-line arguments:
> 
> 1. They are always executed, perhaps unnecessarily whereas 
>    `gdb_init_command' and consequently `gdb_init_commands' are only 
>    interpreted when a target connection is about to be made (this is more 
>    of an aesthetic matter, but still).

Which I'll guess is an historic accident (not by design) given the
option's name.  I'd think hooking gdb_reload/gdb_load would do just
as well.

> 
> 2. Some environments have a limit, maybe quite low, on the maximum length 
>    of a command line or command-line arguments they accept (now that is 
>    more real).

Not really an issue, as you can just put the commands in a GDB script
and do:

  set GDBFLAGS "${GDBFLAGS} -x \"/path/to/script.gdb\""

Very much like a response file.

> 
>  BTW, in updating DejaGNU documentation that refers to `gdb_init_command' 
> I've noticed it lists a command that pokes at a CPU register there -- has 
> the semantics of the setting changed sometime, perhaps long ago?  Does 
> anybody know/remember?

I don't know the history, but I'd guess it's related to
this in config/gdb-comm.exp (found in dejagnu itself, not
gdb):

#
# gdb_comm_load -- load the program and execute it
#
# PROG is a full pathname to the file to load, no arguments.
# Result is "untested", "pass", "fail", etc.
#
proc gdb_comm_load { dest prog args } {
...
    remote_send host "target $protocol $targetname\n"
    remote_expect host 60 {
...
    if {[target_info exists gdb_init_command]} {
	remote_send host "[target_info gdb_init_command]\n"
	remote_expect host 10 {
	    -re ".*$gdb_prompt $" { }
	    default {
		gdb_comm_leave
		return [list "fail" ""]
	    }
	}
    }
...
}

So in that board, gdb_init_command runs after connecting, and
is used to configure the board after connecting.  Clearly there's
a usage conflict here...

(that's a ${board}_load override, note.)

I'd guess most of what's in that file predates all the equivalent
infrastructure we have, even.  Maybe gdb_init_command started out
there before in appeared in gdb's codebase.  But that's just
guesswork.  I wasn't around then.  :-)  I have no idea if gdb-comm.exp
is still used.

>  Done, as below, and retested.  Any other questions or comments?  
> Otherwise OK to apply?

This version looks good to me.

Thanks,
  

On Thu, 10 Jul 2014, Pedro Alves wrote:

> >>> 2014-06-11  Maciej W. Rozycki  <macro@mips.com>
> >>>             Maciej W. Rozycki  <macro@codesourcery.com>
> >>>
> >>> 	* lib/gdb.exp (gdb_run_cmd): Process `gdb_init_commands'.
> >>> 	* lib/mi-support.exp (mi_run_cmd): Process `gdb_init_commands'.
> >>> 	* README (Board Settings): Document `gdb_init_command' and 
> >>> 	`gdb_init_commands'.
> >>
> >> I don't particularly see much need for this -- I do this in my
> >> boards instead:
> >>
> >>  set GDBFLAGS ""
> >>  set GDBFLAGS "${GDBFLAGS} -ex \"set breakpoint always-inserted on\""
> >>  set GDBFLAGS "${GDBFLAGS} -ex \"set target-async 1\""
> >>
> >> See:
> >>
> >> https://sourceware.org/gdb/wiki/TestingGDB#Passing_an_option_to_GDB_.2BAC8_Running_the_whole_test_suite_in_a_non-default_mode
> >>
> >> But, given gdb_init_command exists and this can be made
> >> non-intrusive, it's fine with me to add the new option.
> > 
> >  That and I think there are two issues with passing commands as 
> > command-line arguments:
> > 
> > 1. They are always executed, perhaps unnecessarily whereas 
> >    `gdb_init_command' and consequently `gdb_init_commands' are only 
> >    interpreted when a target connection is about to be made (this is more 
> >    of an aesthetic matter, but still).
> 
> Which I'll guess is an historic accident (not by design) given the
> option's name.  I'd think hooking gdb_reload/gdb_load would do just
> as well.

 That I have a separate patch for. ;)  The thing is some actions can only 
be requested before the target is opened and some actions can only be made 
afterwards.  Consider setting `remotetimeout' for a crawler target and 
poking at a control register required to set the correct mode of execution 
as two representative examples.

 If you think it would be a worthwhile addition, then I can reorder the 
change to the fromt of my queue; otherwise it'll have to wait for its turn 
as that's for a change stuff we haven't used recently and therefore I 
don't consider it very important.

> > 2. Some environments have a limit, maybe quite low, on the maximum length 
> >    of a command line or command-line arguments they accept (now that is 
> >    more real).
> 
> Not really an issue, as you can just put the commands in a GDB script
> and do:
> 
>   set GDBFLAGS "${GDBFLAGS} -x \"/path/to/script.gdb\""
> 
> Very much like a response file.

 Fair enough.

> >  BTW, in updating DejaGNU documentation that refers to `gdb_init_command' 
> > I've noticed it lists a command that pokes at a CPU register there -- has 
> > the semantics of the setting changed sometime, perhaps long ago?  Does 
> > anybody know/remember?
> 
> I don't know the history, but I'd guess it's related to
> this in config/gdb-comm.exp (found in dejagnu itself, not
> gdb):
> 
> #
> # gdb_comm_load -- load the program and execute it
> #
> # PROG is a full pathname to the file to load, no arguments.
> # Result is "untested", "pass", "fail", etc.
> #
> proc gdb_comm_load { dest prog args } {
> ...
>     remote_send host "target $protocol $targetname\n"
>     remote_expect host 60 {
> ...
>     if {[target_info exists gdb_init_command]} {
> 	remote_send host "[target_info gdb_init_command]\n"
> 	remote_expect host 10 {
> 	    -re ".*$gdb_prompt $" { }
> 	    default {
> 		gdb_comm_leave
> 		return [list "fail" ""]
> 	    }
> 	}
>     }
> ...
> }
> 
> So in that board, gdb_init_command runs after connecting, and
> is used to configure the board after connecting.  Clearly there's
> a usage conflict here...
> 
> (that's a ${board}_load override, note.)
> 
> I'd guess most of what's in that file predates all the equivalent
> infrastructure we have, even.  Maybe gdb_init_command started out
> there before in appeared in gdb's codebase.  But that's just
> guesswork.  I wasn't around then.  :-)  I have no idea if gdb-comm.exp
> is still used.

 Thanks for the pointer, I didn't expect it was a place to look there.  
There's this comment at the top of gdb-comm.exp:

# Note: some of this was cribbed from the gdb testsuite since we need
# to use some pretty standard gdb features (breakpoints in particular).

so I suppose some of that stuff was somehow forked off our infrastructure 
sometime, probably long ago.  There's a bunch of board description files 
in DejaGNU that pull this gdb-comm.exp harness; many actually don't do 
anything beyond.

 It may need some further investigation and maybe synchronising things 
between DejaGNU and our stuff.  I'll talk to the DejaGNU maintainer, he 
seems to have been around for a while and may remember what this all is 
about.

> >  Done, as below, and retested.  Any other questions or comments?  
> > Otherwise OK to apply?
> 
> This version looks good to me.

 Committed now, thanks for your review.

  Maciej