Re: [PATCH 6/6] Implement proper "startup-with-shell" support on gdbserver

[Sergio Durigan Junior <sergiodj@redhat.com>]

Thanks for the review, Eli.

Comments below.

On Friday, December 23 2016, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: palves@redhat.com, Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Thu, 22 Dec 2016 22:39:21 -0500
>> 
>> gdb/doc/ChangeLog:
>> 2016-12-22  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* gdb.texinfo (startup-with-shell): Add note mentioning that the
>> 	setting is also used by remote targets.
>> 	(set remote startup-shell): New item.
>
> The text in parentheses should be the name of the node where you make
> the change.  If you want to refer to specific symbols within the node,
> either mention them as part of the text or put them in <..>, like
> this:
>
>   * gdb.texinfo (Starting) <startup-with-shell>: Add note ...

Ops, thanks for pointing that out.  I'll fix it.

>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index a0de7d1..3250cae 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -2174,6 +2174,10 @@ initialization file---such as @file{.cshrc} for C-shell,
>>  $@file{.zshenv} for the Z shell, or the file specified in the
>>  @samp{BASH_ENV} environment variable for BASH.
>>  
>> +This setting is also used by @code{target extended-remote} to
>> +determine whether the target system will start the inferior using the
>> +shell (@pxref{set remote startup-shell}).
>
> I would add here a qualification: only on Unix-like target systems.
> (Is it true that this is a necessary condition for the applicability
> of that feature?)

The target must be able to start the inferior using a shell.  Of all the
non-UNIX-like systems that GDB supports, I am not aware of any that can
do it, so I think this is a fair qualification.

>> +@item set remote startup-shell @var{shell}
>> +@itemx show remote startup-shell
>> +@anchor{set remote startup-shell}
>> +@cindex startup shell, for remote target
>> +Set the shell that is going to be used to start the inferior with
>> +@code{target extended-remote}.  This should be set to a valid shell on
>> +the target system, and is only effective when
>> +@code{startup-with-shell} is on.  If it is not set, the target system
>> +will use the shell pointed by the @code{SHELL} environment variable.
>
> Three comments on this:
>
>   . What is a "valid shell" in this context?  Is the DOS command.com a
>     valid shell, for example (probably not)?  I think we should be
>     more explicit about which shells are acceptable.

Right.  "Valid shells" are shells that are present on the target system
(i.e., they are installed there), and that can be used to start the
inferior.  I know almost nothing about command.com, but ISTR that it
cannot be used to start programs.

I will explain it better in the text.

>   . What exactly is the value supposed to be?  Is it a full absolute
>     file name of the shell executable?  We should say so explicitly,
>     IMO.

It all depends on what is in the $PATH.  But you're right, I'll mention
that explicitly.

>   . Does it really make sense to require that both this option and
>     startup-with-shell be on?

They don't need to be both on.  The only setting that needs to be on is
the startup-with-shell.  The 'remote startup-shell' option only needs to
be set if the user wants to explicitly specify which shell to use.

>     Why cannot the user set only this
>     option to achieve that effect, with some special value standing
>     for "use $SHELL"?

The "special value", in this case, is an empty value.  I.e., if the user
doesn't touch the 'remote startup-shell' option and the
startup-with-shell option is on, the target will use $SHELL to start the
inferior.

Now, it is questionable whether to 

>     Also, is it possible that a user will want
>     remote targets to use a shell, while avoiding that for native
>     debugging, in the same session?

Yeah, I thought about that.  My first thought was to add a 'set/show
remote startup-with-shell' option, but I thought this was going to be
too much for the user.  Maybe not.

With this new setting, we'd have:

- startup-with-shell: Used only locally, default on.  Instructs GDB to
  start the inferior using the shell.

- remote startup-with-shell: Used only remotely, default on.  Instructs
  gdbserver to start the inferior using the shell.

- remote startup-shell: Used only remotely (if remote startup-with-shell
  is on), default "" (empty).  Tells which shell to be used by gdbserver
  to start the inferior.  If empty, use $SHELL.

WDYT?

> A minor nit: environment variables should have the @env markup, not
> @code.

Will fix.

> I think this warrants a NEWS entry.

Will write.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/