[patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Sandra Loosemore]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

This patch fixes a group of failures in gdb.base/shell.exp on Windows 
host due to assumptions that GDB is launching a POSIX-like shell.  On 
Windows, we get CMD.EXE instead.

There is a potential second group of failures on Windows host not 
addressed by this patch, due to the dependence of some of these tests on 
utilities like wc, sed, and grep.  These tests happen to work for me 
because the test harness connects to the remote host using Cygwin ssh 
and both GDB and CMD.EXE inherit the PATH setting from the parent Cygwin 
shell.  I don't know if disabling those tests on Windows host too is 
appropriate, or again if this is an instance of badly-designed tests 
that could be rewritten to avoid those dependencies.  WDYT?

-Sandra

[-- Attachment #2: windows-shell.patch --]
[-- Type: text/x-patch, Size: 4194 bytes --]

commit 6d3377fd7e040e1c6001af12f8d8b2dfa8b7b0c6
Author: Sandra Loosemore <sandra@codesourcery.com>
Date:   Wed Jun 24 17:32:16 2020 -0700

    Fix POSIX-isms in gdb.base/shell.exp
    
    Some recent tests added to gdb.base/shell.exp have been failing on
    Windows host due to assumptions that the shell is a POSIX variant.  On
    Windows, GDB uses CMD.EXE via the system() call to run shell commands
    instead.
    
    There seems to be no obvious CMD.EXE equivalent for "kill -2 $$" to
    signal the shell process, so this patch skips those tests on Windows
    host.  The second problem addressed here is that CMD.EXE only
    recognizes double quotes, not single quotes; that change can be made
    unconditionally since POSIX shells recognize double quotes as well.
    
    2020-06-24  Sandra Loosemore  <sandra@codesourcery.com>
    
    	* gdb.base/shell.exp: Skip pipe tests dependent on sh on Windows host.
    	Use double quotes instead of single quotes.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1b77459..aebfe8d 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-24  Sandra Loosemore  <sandra@codesourcery.com>
+
+	* gdb.base/shell.exp: Skip pipe tests dependent on sh on Windows host.
+	Use double quotes instead of single quotes.
+
 2020-06-24  Pedro Alves  <palves@redhat.com>
 
 	* gdb.arch/amd64-entry-value-paramref.exp: Use
diff --git a/gdb/testsuite/gdb.base/shell.exp b/gdb/testsuite/gdb.base/shell.exp
index f656077..ad36f6b 100644
--- a/gdb/testsuite/gdb.base/shell.exp
+++ b/gdb/testsuite/gdb.base/shell.exp
@@ -34,9 +34,12 @@ gdb_test_no_output "! exit 1"
 gdb_test "p \$_shell_exitcode" " = 1" "shell fail exitcode"
 gdb_test "p \$_shell_exitsignal" " = void" "shell fail exitsignal"
 
-gdb_test_no_output "! kill -2 $$"
-gdb_test "p \$_shell_exitcode" " = void" "shell interrupt exitcode"
-gdb_test "p \$_shell_exitsignal" " = 2" "shell interrupt exitsignal"
+# This test will not work when the shell is CMD.EXE.
+if { ! [ishost *-*-mingw*] } {
+    gdb_test_no_output "! kill -2 $$"
+    gdb_test "p \$_shell_exitcode" " = void" "shell interrupt exitcode"
+    gdb_test "p \$_shell_exitsignal" " = 2" "shell interrupt exitsignal"
+}
 
 # Define the user command "foo", used to test "pipe" command.
 gdb_test_multiple "define foo" "define foo" {
@@ -67,16 +70,16 @@ gdb_test "|foo|grep truc|wc -l" "1" "no space around pipe char"
 gdb_test "echo coucou\\n" "coucou" "echo coucou"
 gdb_test "||wc -l" "1" "repeat previous command"
 
-gdb_test "| -d ! echo this contains a | character\\n ! sed -e 's/|/PIPE/'" \
+gdb_test "| -d ! echo this contains a | character\\n ! sed -e \"s/|/PIPE/\"" \
     "this contains a PIPE character" "alternate 1char delim"
 
-gdb_test "|-d ! echo this contains a | character\\n!sed -e 's/|/PIPE/'" \
+gdb_test "|-d ! echo this contains a | character\\n!sed -e \"s/|/PIPE/\"" \
     "this contains a PIPE character" "alternate 1char delim, no space"
 
-gdb_test "| -d !!! echo this contains a | character\\n !!! sed -e 's/|/PIPE/'" \
+gdb_test "| -d !!! echo this contains a | character\\n !!! sed -e \"s/|/PIPE/\"" \
     "this contains a PIPE character" "alternate 3char delim"
 
-gdb_test "|-d !!! echo this contains a | character\\n!!!sed -e 's/|/PIPE/'" \
+gdb_test "|-d !!! echo this contains a | character\\n!!!sed -e \"s/|/PIPE/\"" \
     "this contains a PIPE character" "alternate 3char delim, no space"
 
 # Convenience variables with pipe command.
@@ -88,9 +91,12 @@ gdb_test "|p 123| exit 1" ""
 gdb_test "p \$_shell_exitcode" " = 1" "pipe fail exitcode"
 gdb_test "p \$_shell_exitsignal" " = void" "pipe fail exitsignal"
 
-gdb_test "|p 123| kill -2 $$" ""
-gdb_test "p \$_shell_exitcode" " = void" "pipe interrupt exitcode"
-gdb_test "p \$_shell_exitsignal" " = 2" "pipe interrupt exitsignal"
+# This test will not work when the shell is CMD.EXE.
+if { ! [ishost *-*-mingw*] } {
+    gdb_test "|p 123| kill -2 $$" ""
+    gdb_test "p \$_shell_exitcode" " = void" "pipe interrupt exitcode"
+    gdb_test "p \$_shell_exitsignal" " = 2" "pipe interrupt exitsignal"
+}
 
 # Error handling verifications.
 gdb_test "|" "Missing COMMAND" "all missing"

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Christian Biesinger]

On Wed, Jun 24, 2020 at 8:35 PM Sandra Loosemore
<sandra@codesourcery.com> wrote:
>
> This patch fixes a group of failures in gdb.base/shell.exp on Windows
> host due to assumptions that GDB is launching a POSIX-like shell.  On
> Windows, we get CMD.EXE instead.

Hmm, if I call runtest/make check from an msys2 shell, do I really get
cmd.exe here? I ask because i wonder how many people run this from
cmd.exe as opposed to msys's shell, so it may be better to check for
cmd more specifically somehow (maybe $SHELL?)


Christian

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Sandra Loosemore]

On 6/25/20 11:32 AM, Christian Biesinger wrote:
> On Wed, Jun 24, 2020 at 8:35 PM Sandra Loosemore
> <sandra@codesourcery.com> wrote:
>>
>> This patch fixes a group of failures in gdb.base/shell.exp on Windows
>> host due to assumptions that GDB is launching a POSIX-like shell.  On
>> Windows, we get CMD.EXE instead.
> 
> Hmm, if I call runtest/make check from an msys2 shell, do I really get
> cmd.exe here? I ask because i wonder how many people run this from
> cmd.exe as opposed to msys's shell, so it may be better to check for
> cmd more specifically somehow (maybe $SHELL?)

I'm not familiar with the msys2 shell, but....

On Windows host GDB uses the system() function provided by the host C 
library to launch a shell and does not consult $SHELL at all.  This is 
in shell_escape in gdb/cli/cli-cmds.c.

Here is the documentation I found via Google for system() in the Windows 
library:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/system-wsystem?view=vs-2019

"system uses the COMSPEC and PATH environment variables to locate the 
command-interpreter file CMD.exe."

I think the GDB manual doesn't accurately describe what GDB does in this 
situation, BTW.

-Sandra

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Eli Zaretskii]

> From: Sandra Loosemore <sandra@codesourcery.com>
> Date: Thu, 25 Jun 2020 11:57:08 -0600
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> Here is the documentation I found via Google for system() in the Windows 
> library:
> 
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/system-wsystem?view=vs-2019
> 
> "system uses the COMSPEC and PATH environment variables to locate the 
> command-interpreter file CMD.exe."
> 
> I think the GDB manual doesn't accurately describe what GDB does in this 
> situation, BTW.

Which part of the manual did you allude to here, and what is in your
opinion inaccurate there?

Thanks.

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Sandra Loosemore]

On 6/25/20 12:07 PM, Eli Zaretskii wrote:
>> From: Sandra Loosemore <sandra@codesourcery.com>
>> Date: Thu, 25 Jun 2020 11:57:08 -0600
>> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>>
>> Here is the documentation I found via Google for system() in the Windows
>> library:
>>
>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/system-wsystem?view=vs-2019
>>
>> "system uses the COMSPEC and PATH environment variables to locate the
>> command-interpreter file CMD.exe."
>>
>> I think the GDB manual doesn't accurately describe what GDB does in this
>> situation, BTW.
> 
> Which part of the manual did you allude to here, and what is in your
> opinion inaccurate there?

https://sourceware.org/gdb/current/onlinedocs/gdb/Shell-Commands.html#Shell-Commands

"If it exists, the environment variable SHELL determines which shell to 
run. Otherwise GDB uses the default shell (/bin/sh on Unix systems, 
COMMAND.COM on MS-DOS, etc.). "

The SHELL environment variable is not consulted on Windows hosts.  It 
just uses whatever shell the system() call uses -- look at the 
implementation of shell_escape in gdb/cli/cli-cmds.c.

The references to MS-DOS and COMMAND.COM are certainly bit-rotten. 
COMMAND.COM was replaced by CMD.EXE in Windows NT, 25+ years ago now. 
And do you really expect to be able to build a modern GDB to run on a 
16-bit MS-DOS host?  :-P

-Sandra

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Eli Zaretskii]

> CC: <cbiesinger@google.com>, <gdb-patches@sourceware.org>
> From: Sandra Loosemore <sandra@codesourcery.com>
> Date: Thu, 25 Jun 2020 13:35:45 -0600
> 
> >> I think the GDB manual doesn't accurately describe what GDB does in this
> >> situation, BTW.
> > 
> > Which part of the manual did you allude to here, and what is in your
> > opinion inaccurate there?
> 
> https://sourceware.org/gdb/current/onlinedocs/gdb/Shell-Commands.html#Shell-Commands
> 
> "If it exists, the environment variable SHELL determines which shell to 
> run. Otherwise GDB uses the default shell (/bin/sh on Unix systems, 
> COMMAND.COM on MS-DOS, etc.). "
> 
> The SHELL environment variable is not consulted on Windows hosts.  It 
> just uses whatever shell the system() call uses -- look at the 
> implementation of shell_escape in gdb/cli/cli-cmds.c.

It isn't factually wrong: $SHELL isn't supposed to be set on non-Posix
hosts.  However, I made the text more accurate with the patch below,
which I just installed.

> The references to MS-DOS and COMMAND.COM are certainly bit-rotten. 
> COMMAND.COM was replaced by CMD.EXE in Windows NT, 25+ years ago now. 
> And do you really expect to be able to build a modern GDB to run on a 
> 16-bit MS-DOS host?  :-P

You'd be surprised:

  ftp://ftp.delorie.com/pub/djgpp/current/v2gnu/gdb801b.zip
  https://groups.google.com/forum/#!msg/comp.os.msdos.djgpp/MIm6bIlJ8b8/yNuLXDoxBQAJ

The DJGPP project has MS-DOS ports of all the main GNU utilities, most
of them of fairly recent versions (the above is GDB 8.0.1).

As for the 16-bit nature of MS-DOS and the ability to run a modern GDB
on it, you remind me of a short dialogue between RMS and DJ Delorie,
which happened more than 30 years ago and started the DJGPP project;
see the first paragraphs on this page:

  http://www.delorie.com/djgpp/history.html

Here's the patch I installed on the master branch:

diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 4b1a4c0..c591934 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2020-06-26  Eli Zaretskii  <eliz@gnu.org>
+
+	* gdb.texinfo (Shell Commands): More accurate description of use
+	of $SHELL.  Reported by Sandra Loosemore <sandra@codesourcery.com>.
+
 2020-06-23  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.texinfo (Maintenance Commands): Document new 'maint print
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7f8c77a..fbe9f85 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -1451,9 +1451,10 @@
 @itemx !@var{command-string}
 Invoke a standard shell to execute @var{command-string}.
 Note that no space is needed between @code{!} and @var{command-string}.
-If it exists, the environment variable @code{SHELL} determines which
-shell to run.  Otherwise @value{GDBN} uses the default shell
-(@file{/bin/sh} on Unix systems, @file{COMMAND.COM} on MS-DOS, etc.).
+On GNU and Unix systems, the environment variable @code{SHELL}, if it
+exists, determines which shell to run.  Otherwise @value{GDBN} uses
+the default shell (@file{/bin/sh} on GNU and Unix systems,
+@file{cmd.exe} on MS-Windows, @file{COMMAND.COM} on MS-DOS, etc.).
 @end table
 
 The utility @code{make} is often needed in development environments.

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Sandra Loosemore]

On 6/26/20 1:08 AM, Eli Zaretskii wrote:
> [snip]
> 
> Here's the patch I installed on the master branch:
> 
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index 4b1a4c0..c591934 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,8 @@
> +2020-06-26  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* gdb.texinfo (Shell Commands): More accurate description of use
> +	of $SHELL.  Reported by Sandra Loosemore <sandra@codesourcery.com>.
> +
>   2020-06-23  Andrew Burgess  <andrew.burgess@embecosm.com>
>   
>   	* gdb.texinfo (Maintenance Commands): Document new 'maint print
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 7f8c77a..fbe9f85 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -1451,9 +1451,10 @@
>   @itemx !@var{command-string}
>   Invoke a standard shell to execute @var{command-string}.
>   Note that no space is needed between @code{!} and @var{command-string}.
> -If it exists, the environment variable @code{SHELL} determines which
> -shell to run.  Otherwise @value{GDBN} uses the default shell
> -(@file{/bin/sh} on Unix systems, @file{COMMAND.COM} on MS-DOS, etc.).
> +On GNU and Unix systems, the environment variable @code{SHELL}, if it
> +exists, determines which shell to run.  Otherwise @value{GDBN} uses
> +the default shell (@file{/bin/sh} on GNU and Unix systems,
> +@file{cmd.exe} on MS-Windows, @file{COMMAND.COM} on MS-DOS, etc.).
>   @end table
>   
>   The utility @code{make} is often needed in development environments.
> 

This looks fine to me -- thanks.

-Sandra

Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Christian Biesinger]

On Thu, Jun 25, 2020 at 12:57 PM Sandra Loosemore
<sandra@codesourcery.com> wrote:
>
> On 6/25/20 11:32 AM, Christian Biesinger wrote:
> > On Wed, Jun 24, 2020 at 8:35 PM Sandra Loosemore
> > <sandra@codesourcery.com> wrote:
> >>
> >> This patch fixes a group of failures in gdb.base/shell.exp on Windows
> >> host due to assumptions that GDB is launching a POSIX-like shell.  On
> >> Windows, we get CMD.EXE instead.
> >
> > Hmm, if I call runtest/make check from an msys2 shell, do I really get
> > cmd.exe here? I ask because i wonder how many people run this from
> > cmd.exe as opposed to msys's shell, so it may be better to check for
> > cmd more specifically somehow (maybe $SHELL?)
>
> I'm not familiar with the msys2 shell, but....
>
> On Windows host GDB uses the system() function provided by the host C
> library to launch a shell and does not consult $SHELL at all.  This is
> in shell_escape in gdb/cli/cli-cmds.c.

Ah sorry, I didn't realize that this shell use came from GDB, I had
assumed this was from dejagnu. This (unofficially) looks good then.

Christian

[ping] Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Sandra Loosemore]

Ping?  I'd like to get this patch into the GDB 10 branch.

https://sourceware.org/pipermail/gdb-patches/2020-June/169865.html

On 6/24/20 7:35 PM, Sandra Loosemore wrote:
> This patch fixes a group of failures in gdb.base/shell.exp on Windows 
> host due to assumptions that GDB is launching a POSIX-like shell.  On 
> Windows, we get CMD.EXE instead.
> 
> There is a potential second group of failures on Windows host not 
> addressed by this patch, due to the dependence of some of these tests on 
> utilities like wc, sed, and grep.  These tests happen to work for me 
> because the test harness connects to the remote host using Cygwin ssh 
> and both GDB and CMD.EXE inherit the PATH setting from the parent Cygwin 
> shell.  I don't know if disabling those tests on Windows host too is 
> appropriate, or again if this is an instance of badly-designed tests 
> that could be rewritten to avoid those dependencies.  WDYT?
> 
> -Sandra

Re: [ping] Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Simon Marchi]

On 2020-07-15 3:53 p.m., Sandra Loosemore wrote:
> Ping?  I'd like to get this patch into the GDB 10 branch.
> 
> https://sourceware.org/pipermail/gdb-patches/2020-June/169865.html
> 
> On 6/24/20 7:35 PM, Sandra Loosemore wrote:
>> This patch fixes a group of failures in gdb.base/shell.exp on Windows 
>> host due to assumptions that GDB is launching a POSIX-like shell.  On 
>> Windows, we get CMD.EXE instead.
>>
>> There is a potential second group of failures on Windows host not 
>> addressed by this patch, due to the dependence of some of these tests on 
>> utilities like wc, sed, and grep.  These tests happen to work for me 
>> because the test harness connects to the remote host using Cygwin ssh 
>> and both GDB and CMD.EXE inherit the PATH setting from the parent Cygwin 
>> shell.  I don't know if disabling those tests on Windows host too is 
>> appropriate, or again if this is an instance of badly-designed tests 
>> that could be rewritten to avoid those dependencies.  WDYT?
>>
>> -Sandra
> 

The patch looks raisonnable to me... but Eli, as the de facto Windows specialist,
could you give your OK?

Simon

Re: [ping] Re: [patch] gdb/testsuite: Fix POSIX-isms in gdb.base/shell.exp

[Eli Zaretskii]

> From: Simon Marchi <simark@simark.ca>
> Date: Wed, 15 Jul 2020 22:41:59 -0400
> 
> The patch looks raisonnable to me... but Eli, as the de facto Windows specialist,
> could you give your OK?

Yes, OK.