* [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX @ 2016-03-30 5:22 Kylie McClain 2016-03-31 20:37 ` Paul Eggert 0 siblings, 1 reply; 8+ messages in thread From: Kylie McClain @ 2016-03-30 5:22 UTC (permalink / raw) To: emacs-devel; +Cc: Kylie McClain, somasissounds From: Kylie McClain <somasis@exherbo.org> Not all Linux libcs (ex. musl libc) support the functionality used under this #ifdef. --- src/print.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/print.c b/src/print.c index 2b53d75..331f3c4 100644 --- a/src/print.c +++ b/src/print.c @@ -775,7 +775,7 @@ debug_output_compilation_hack (bool x) print_output_debug_flag = x; } -#if defined (GNU_LINUX) +#if defined (__GLIBC__) /* This functionality is not vitally important in general, so we rely on non-portable ability to use stderr as lvalue. */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-03-30 5:22 [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX Kylie McClain @ 2016-03-31 20:37 ` Paul Eggert 2016-03-31 21:26 ` Kylie McClain 2016-04-01 6:43 ` Eli Zaretskii 0 siblings, 2 replies; 8+ messages in thread From: Paul Eggert @ 2016-03-31 20:37 UTC (permalink / raw) To: Kylie McClain, emacs-devel; +Cc: Kylie McClain Thanks for reporting that. I filed a bug report with a proposed patch here: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23173 Instead of disabling redirect-debugging-output, this patch ports it to systems like musl where stderr is not an lvalue. Please give it a try if you have the time. it's against master commit 750e1e19429cd781e2e60b462d19ef827d4da943. I haven't installed it into 'master' yet, to give Eli a heads-up about the impending change. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-03-31 20:37 ` Paul Eggert @ 2016-03-31 21:26 ` Kylie McClain 2016-04-01 6:43 ` Eli Zaretskii 1 sibling, 0 replies; 8+ messages in thread From: Kylie McClain @ 2016-03-31 21:26 UTC (permalink / raw) To: Paul Eggert; +Cc: Kylie McClain, emacs-devel On Thu, Mar 31, 2016 at 4:37 PM, Paul Eggert <eggert@cs.ucla.edu> wrote: > Thanks for reporting that. I filed a bug report with a proposed patch here: > > http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23173 > > Instead of disabling redirect-debugging-output, this patch ports it to > systems like musl where stderr is not an lvalue. Please give it a try if you > have the time. it's against master commit > 750e1e19429cd781e2e60b462d19ef827d4da943. > > I haven't installed it into 'master' yet, to give Eli a heads-up about the > impending change. Works just fine! :) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-03-31 20:37 ` Paul Eggert 2016-03-31 21:26 ` Kylie McClain @ 2016-04-01 6:43 ` Eli Zaretskii 2016-04-01 7:46 ` Paul Eggert 1 sibling, 1 reply; 8+ messages in thread From: Eli Zaretskii @ 2016-04-01 6:43 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel, somasis, somasissounds > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Thu, 31 Mar 2016 13:37:31 -0700 > Cc: Kylie McClain <somasis@exherbo.org> > > I haven't installed it into 'master' yet, to give Eli a heads-up about > the impending change. Thanks. I needed the additional patch below to get the patch to compile (how do other ports get F_DUPFD_CLOEXEC?). After that, temacs seems to hang during loadup. Looks like it hangs inside the libc function 'close'. I don't have enough free time now to do anything serious on master. Hopefully, someone else will be able to look into this, find out why it hangs, and suggest a remedy. --- src/print.c~0 2016-04-01 09:32:53.060500000 +0300 +++ src/print.c 2016-04-01 09:36:24.591750000 +0300 @@ -38,6 +38,10 @@ along with GNU Emacs. If not, see <http #include <float.h> #include <ftoastr.h> +#ifdef WINDOWSNT +#include <sys/socket.h> /* for F_DUPFD_CLOEXEC */ +#endif + struct terminal; /* Avoid actual stack overflow in print. */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-04-01 6:43 ` Eli Zaretskii @ 2016-04-01 7:46 ` Paul Eggert 2016-04-02 8:35 ` Eli Zaretskii 0 siblings, 1 reply; 8+ messages in thread From: Paul Eggert @ 2016-04-01 7:46 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel, somasis, somasissounds Eli Zaretskii wrote: > Thanks. I needed the additional patch below to get the patch to > compile (how do other ports get F_DUPFD_CLOEXEC?). <fcntl.h> defines it, perhaps via the gnulib replacement sourced in lib/fcntl.in.h. > After that, temacs seems to hang during loadup. Looks like it hangs > inside the libc function 'close'. I don't have enough free time now > to do anything serious on master. No rush. When you get time, if you happen to know who's calling 'close' I could ifdef that call out on MS-Windows. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-04-01 7:46 ` Paul Eggert @ 2016-04-02 8:35 ` Eli Zaretskii 2016-04-03 8:24 ` Paul Eggert 2016-04-04 16:45 ` Paul Eggert 0 siblings, 2 replies; 8+ messages in thread From: Eli Zaretskii @ 2016-04-02 8:35 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel, somasis, somasissounds > Cc: somasissounds@gmail.com, emacs-devel@gnu.org, somasis@exherbo.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Fri, 1 Apr 2016 00:46:52 -0700 > > > After that, temacs seems to hang during loadup. Looks like it hangs > > inside the libc function 'close'. I don't have enough free time now > > to do anything serious on master. > > No rush. When you get time, if you happen to know who's calling 'close' I could > ifdef that call out on MS-Windows. It turns out the call to dup2 with 2 identical arguments has a strange side effect with MS implementation: the subsequent attempt to fclose the corresponding stdio stream hangs. I was unable to understand why that happens, since according to my references, such a dup2 call should just validate the original file descriptor and do nothing else. But since we already have a sys_dup2 function that wraps the MS dup2, I added a workaround there, see the patch below. For the record, what init_standard_fds does is unnecessary on MS-Windows, since we already have the equivalent code in init_ntproc (for reasons explained in the comments there). So an alternative would be to make init_standard_fds a no-op for WINDOWSNT. I preferred to have less #ifdef's in mainline Emacs code, but if you think the alternative is better, it's fine with me. Btw, are changes like the one below safe? /* Manipulate tty. */ if (hide_char) { - etty_valid = emacs_get_tty (fileno (stdin), &etty) == 0; + etty_valid = emacs_get_tty (STDIN_FILENO, &etty) == 0; if (etty_valid) - set_binary_mode (fileno (stdin), O_BINARY); - suppress_echo_on_tty (fileno (stdin)); + set_binary_mode (STDIN_FILENO, O_BINARY); + suppress_echo_on_tty (STDIN_FILENO); } Are we sure fileno(stdin) will necessarily return 0, what with all the redirections and stuff? Why not keep the original code? So to sum it up, these additional changes are required for MS-Windows: diff --git a/src/print.c b/src/print.c index 2b53d75..ee60f57 100644 --- a/src/print.c +++ b/src/print.c @@ -38,6 +38,10 @@ along with GNU Emacs. If not, see <http://www.gnu.org/licenses/>. */ #include <float.h> #include <ftoastr.h> +#ifdef WINDOWSNT +#include <sys/socket.h> /* for F_DUPFD_CLOEXEC */ +#endif + struct terminal; /* Avoid actual stack overflow in print. */ diff --git a/src/w32.c b/src/w32.c index 3f4ac88..94aa7d8 100644 --- a/src/w32.c +++ b/src/w32.c @@ -8181,17 +8181,33 @@ sys_dup2 (int src, int dst) return -1; } - /* make sure we close the destination first if it's a pipe or socket */ - if (src != dst && fd_info[dst].flags != 0) + /* MS _dup2 seems to have weird side effect when invoked with 2 + identical arguments: an attempt to fclose the corresponding stdio + stream after that hangs (we do close standard streams in + init_ntproc). Attempt to avoid that by not calling _dup2 that + way: if SRC is valid, we know that dup2 should be a no-op, so do + nothing and return DST. */ + if (src == dst) + { + if ((HANDLE)_get_osfhandle (src) == INVALID_HANDLE_VALUE) + { + errno = EBADF; + return -1; + } + return dst; + } + + /* Make sure we close the destination first if it's a pipe or socket. */ + if (fd_info[dst].flags != 0) sys_close (dst); rc = _dup2 (src, dst); if (rc == 0) { - /* duplicate our internal info as well */ + /* Duplicate our internal info as well. */ fd_info[dst] = fd_info[src]; } - return rc; + return rc == 0 ? dst : rc; } int ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-04-02 8:35 ` Eli Zaretskii @ 2016-04-03 8:24 ` Paul Eggert 2016-04-04 16:45 ` Paul Eggert 1 sibling, 0 replies; 8+ messages in thread From: Paul Eggert @ 2016-04-03 8:24 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel, somasis, somasissounds Eli Zaretskii wrote: > Are we sure fileno(stdin) will necessarily return 0, what with all the > redirections and stuff? Yes. The redirections in Emacs don't change the file descriptor numbers associated with stdin, stdout, and stderr. They will always be 0, 1, and 2. > Why not keep the original code? It's a bit slower, and (more important) it leads the reader to wonder whether fileno (stdin) might not equal STDIN_FILENO. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX 2016-04-02 8:35 ` Eli Zaretskii 2016-04-03 8:24 ` Paul Eggert @ 2016-04-04 16:45 ` Paul Eggert 1 sibling, 0 replies; 8+ messages in thread From: Paul Eggert @ 2016-04-04 16:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel, somasis, somasissounds On 04/02/2016 01:35 AM, Eli Zaretskii wrote: > I preferred > to have less #ifdef's in mainline Emacs code, but if you think the > alternative is better, it's fine with me. No, I also like keeping the mainline code simpler. I installed the patch with your additions; thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-04-04 16:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-30 5:22 [PATCH] src/print.c: Check for __GLIBC__ rather than GNU_LINUX Kylie McClain 2016-03-31 20:37 ` Paul Eggert 2016-03-31 21:26 ` Kylie McClain 2016-04-01 6:43 ` Eli Zaretskii 2016-04-01 7:46 ` Paul Eggert 2016-04-02 8:35 ` Eli Zaretskii 2016-04-03 8:24 ` Paul Eggert 2016-04-04 16:45 ` Paul Eggert
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).