From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 129933 invoked by alias); 24 Oct 2016 12:36:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 129079 invoked by uid 89); 24 Oct 2016 12:36:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Ping! X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 24 Oct 2016 12:36:37 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9E792135A4; Mon, 24 Oct 2016 12:36:36 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9OCaYRM007702; Mon, 24 Oct 2016 08:36:34 -0400 Subject: Re: CYGWIN file input redirection To: Eli Zaretskii , gdb-patches@sourceware.org References: <83twcxn3ne.fsf@gnu.org> <8360ot3kzq.fsf@gnu.org> <83zim523ch.fsf@gnu.org> <83eg38rarq.fsf@gnu.org> Cc: samsurfer117@gmail.com From: Pedro Alves Message-ID: Date: Mon, 24 Oct 2016 12:36:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <83eg38rarq.fsf@gnu.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00663.txt.bz2 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 >> >> 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