Re: CYGWIN file input redirection

[Pedro Alves <palves@redhat.com>]

On 10/22/2016 10:30 AM, Eli Zaretskii wrote:
> Ping!  Is this OK to push to master, please?

I think this should have a NEWS entry.  

Since on Windows, programs are expected to handle redirection
themselves, isn't there a chance that this will make it a bit
harder to debug such redirection code in inferiors that
do it themselves?  E.g., imagine if you're debugging cmd.exe's
redirection code.  Or maybe even debugging this new code
in gdb itself?  Do we need a knob to be able to disable
this feature?

I didn't bother to try to understand the patch completely.
I just trust that it works.  It's fine with me to put it in.

It'd be nice to add comments mentioning what syntax works and doesn't
work.  Is there something users should know about syntax, that
should be added to the manual?

Ideally some test would "prove" this all works, which would
also make it possible to more confidently change the implementation
later on if we find it necessary.  It's been years since I'd tried to
run the testsuite for mingw gdb (under cygwin/msys/msys2 of course)
and I have no idea whether people are doing that nowadays.  I have
the impression that maybe no one is..  And then, I can't seem to
find any existing test that exercises redirection, even
on Unix...  :-/  Oh well.

Please see below.

>> 2016-10-15  Eli Zaretskii  <eliz@gnu.org>
>>
>> 	Support command-line redirection for Windows native debugging
>>
>> 	* windows-nat.c (redir_open, redir_set_redirection)
>> 	(redirect_inferior_handles) [!__CYGWIN__]: New functions for
>> 	redirecting standard handles of the inferior.
>> 	(windows_create_inferior) [!__CYGWIN__]: Use them to redirect
>> 	standard handles of the inferior based on redirection symbols on
>> 	the command line.
>>
>> --- gdb/windows-nat.c~1	2016-10-09 12:37:04.538125000 +0300
>> +++ gdb/windows-nat.c	2016-10-15 14:27:51.966125000 +0300
>> @@ -2054,6 +2054,166 @@ clear_win32_environment (char **env)
>>  }
>>  #endif
>>  

>> +  fname++;	/* skip the separator space */
>> +  fd = _open (fname, mode, _S_IREAD | _S_IWRITE);
>> +  if (fd < 0)
>> +    return -1;
>> +  /* _open just sets a flag for O_APPEND, which won't be passed to the
>> +     inferior, so we need to actually move the file pointer.  */
>> +  if ((mode & O_APPEND) != 0)
>> +    _lseek (fd, 0L, SEEK_END);
>> +  *hdl = (HANDLE)_get_osfhandle (fd);

GDB puts a space after casts:

  *hdl = (HANDLE) _get_osfhandle (fd);


>> +  return 0;
>> +}
>> +
>> +static int
>> +redir_set_redirection (const char *s, HANDLE *inp, HANDLE *out, HANDLE *err)

It'd be good to have an intro comment for every new function describing
the interface.  What does this return, for example.

>> +#else  /* !__CYGWIN__ */
>> +  allargs_len = strlen (allargs);
>> +  allargs_copy = strcpy ((char *)alloca (allargs_len + 1), allargs);

Space after cast.

We should really get rid of all unbounded allocas, but it's
preexisting, so you get a pass. :-)

Nit, we already know the length after the strlen call, so
instead of strcpy it could be memcpy.


>> +  if (strpbrk (allargs_copy, "<>"))

      if (strpbrk (allargs_copy, "<>") != NULL)


>> +    {
>> +      int e = errno;
>> +      errno = 0;
>> +      redirected =
>> +	redirect_inferior_handles (allargs, allargs_copy,
>> +				   &inf_stdin, &inf_stdout, &inf_stderr);
>> +      if (errno)
>> +	warning (_("Error in redirection: %s."), strerror (errno));
>> +      else
>> +	errno = e;
>> +      allargs_len = strlen (allargs_copy);
>> +    }
>> +  if (inferior_io_terminal
>> +      && !(inf_stdin != INVALID_HANDLE_VALUE
>> +	   && inf_stdout != INVALID_HANDLE_VALUE
>> +	   && inf_stderr != INVALID_HANDLE_VALUE))

I find these double-negatives hard to read.  I'd suggest:

  if (inferior_io_terminal
      && (inf_stdin == INVALID_HANDLE_VALUE
	  || inf_stdout == INVALID_HANDLE_VALUE
	  || inf_stderr == INVALID_HANDLE_VALUE))

Thanks,
Pedro Alves