From: Matt Joyce <matthew.joyce@embedded-brains.de> Hello Corinna, Per your comments, please see version 2. 1) stdio_atexit is now called stdio_exit_handler 2) declarations for stdio_exit_handler() and _fwalk_sglue() are moved to sys/reent.h Thanks! Matt Matt Joyce (9): Remove duplicate stdio initializations Remove duplicate sglue initializations Add two __sglue initialization macros Remove __sinit_locks / __sinit_recursive_mutex Move __sglue initializations to __sfp() Add CLEANUP_FILE define Add stdio_exit_handler() Add global __sglue object for all configurations Remove __sglue member for one configuration Sebastian Huber (2): Declare global __sf[] only once stdio: Replace _fwalk_reent() with _fwalk_sglue() newlib/libc/include/sys/reent.h | 26 ++++++-- newlib/libc/reent/reent.c | 6 +- newlib/libc/stdio/fcloseall.c | 9 ++- newlib/libc/stdio/fflush.c | 2 +- newlib/libc/stdio/findfp.c | 112 +++++++++++++++++--------------- newlib/libc/stdio/fwalk.c | 16 ++--- newlib/libc/stdio/local.h | 3 - newlib/libc/stdio/refill.c | 4 +- newlib/libc/stdlib/exit.c | 5 +- winsup/cygwin/signal.cc | 5 +- winsup/cygwin/syscalls.cc | 6 +- 11 files changed, 110 insertions(+), 84 deletions(-) -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Removed duplicate stdio initializations from __sinit(). These are already initialized in the _REENT_INIT macro in sys/reent.h. This simplification enables the reduction of _GLOBAL_REENT dependency in a follow-on patch. --- newlib/libc/stdio/findfp.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 1370b63b8..ab3c6a55d 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -259,10 +259,6 @@ __sinit (struct _reent *s) s->_stdin = __sfp(s); s->_stdout = __sfp(s); s->_stderr = __sfp(s); -# else /* _REENT_GLOBAL_STDIO_STREAMS */ - s->_stdin = &__sf[0]; - s->_stdout = &__sf[1]; - s->_stderr = &__sf[2]; # endif /* _REENT_GLOBAL_STDIO_STREAMS */ #endif -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Removed duplicate sglue initializations from __sinit(). These are already initialized in the _REENT_INIT macro in sys/reent.h. This simplification enables the reduction of _GLOBAL_REENT dependency in a follow-on patch. --- newlib/libc/stdio/findfp.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index ab3c6a55d..8327b1992 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -246,15 +246,12 @@ __sinit (struct _reent *s) /* make sure we clean up on exit */ s->__cleanup = cleanup_stdio; /* conservative */ - s->__sglue._next = NULL; #ifndef _REENT_SMALL # ifndef _REENT_GLOBAL_STDIO_STREAMS s->__sglue._niobs = 3; s->__sglue._iobs = &s->__sf[0]; # endif /* _REENT_GLOBAL_STDIO_STREAMS */ #else - s->__sglue._niobs = 0; - s->__sglue._iobs = NULL; # ifndef _REENT_GLOBAL_STDIO_STREAMS s->_stdin = __sfp(s); s->_stdout = __sfp(s); -- 2.31.1
From: Sebastian Huber <sebastian.huber@embedded-brains.de> Reduced number of global __sf[] declarations from two to one, simplifying initializations in sys/reent.h. --- newlib/libc/include/sys/reent.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h index 4bc78a447..e0b0ccffb 100644 --- a/newlib/libc/include/sys/reent.h +++ b/newlib/libc/include/sys/reent.h @@ -293,6 +293,10 @@ typedef struct __sFILE __FILE; #endif /* __LARGE64_FILES */ #endif /* !__CUSTOM_FILE_IO__ */ +#ifdef _REENT_GLOBAL_STDIO_STREAMS +extern __FILE __sf[3]; +#endif + struct _glue { struct _glue *_next; @@ -426,7 +430,6 @@ struct _reent }; #ifdef _REENT_GLOBAL_STDIO_STREAMS -extern __FILE __sf[3]; # define _REENT_INIT(var) \ { 0, \ @@ -698,7 +701,6 @@ struct _reent }; #ifdef _REENT_GLOBAL_STDIO_STREAMS -extern __FILE __sf[3]; #define _REENT_STDIO_STREAM(var, index) &__sf[index] #else #define _REENT_STDIO_STREAM(var, index) &(var)->__sf[index] -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Added _REENT_INIT_SGLUE and _REENT_INIT_SGLUE_ZEROED macros to initialize __sglue member of struct _reent. This allows further simplification of __sinit() and facilitates the removal of __sglue as a member of struct _reent for certain configurations in a follow-on patch. --- newlib/libc/include/sys/reent.h | 9 ++++++++- newlib/libc/stdio/findfp.c | 7 +------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h index e0b0ccffb..4b0d68610 100644 --- a/newlib/libc/include/sys/reent.h +++ b/newlib/libc/include/sys/reent.h @@ -702,8 +702,14 @@ struct _reent #ifdef _REENT_GLOBAL_STDIO_STREAMS #define _REENT_STDIO_STREAM(var, index) &__sf[index] +#define _REENT_INIT_SGLUE(_ptr) { _NULL, 0, _NULL } +#define _REENT_INIT_SGLUE_ZEROED(_ptr) /* nothing to set */ #else #define _REENT_STDIO_STREAM(var, index) &(var)->__sf[index] +#define _REENT_INIT_SGLUE(_ptr) { _NULL, 3, &(_ptr)->__sf[0] } +#define _REENT_INIT_SGLUE_ZEROED(_ptr) \ + (_ptr)->__sglue._niobs = 3; \ + (_ptr)->__sglue._iobs = &(_ptr)->__sf[0]; #endif #define _REENT_INIT(var) \ @@ -751,7 +757,7 @@ struct _reent }, \ _REENT_INIT_ATEXIT \ _NULL, \ - {_NULL, 0, _NULL} \ + _REENT_INIT_SGLUE(&(var)) \ } #define _REENT_INIT_PTR_ZEROED(var) \ @@ -766,6 +772,7 @@ struct _reent (var)->_new._reent._r48._mult[1] = _RAND48_MULT_1; \ (var)->_new._reent._r48._mult[2] = _RAND48_MULT_2; \ (var)->_new._reent._r48._add = _RAND48_ADD; \ + _REENT_INIT_SGLUE_ZEROED(var) \ } #define _REENT_CHECK_RAND48(ptr) /* nothing */ diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 8327b1992..66867e664 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -246,12 +246,7 @@ __sinit (struct _reent *s) /* make sure we clean up on exit */ s->__cleanup = cleanup_stdio; /* conservative */ -#ifndef _REENT_SMALL -# ifndef _REENT_GLOBAL_STDIO_STREAMS - s->__sglue._niobs = 3; - s->__sglue._iobs = &s->__sf[0]; -# endif /* _REENT_GLOBAL_STDIO_STREAMS */ -#else +#ifdef _REENT_SMALL # ifndef _REENT_GLOBAL_STDIO_STREAMS s->_stdin = __sfp(s); s->_stdout = __sfp(s); -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Removed __sinit_lock_acquire() and __sinit_lock_release(). Replace these with __sfp_lock_acquire() and __sfp_lock_release(), respectively. This eliminates a potential deadlock issue between __sinit() and __sfp(). Removed now unused __sinit_recursive_mutex. --- newlib/libc/stdio/findfp.c | 19 +++---------------- newlib/libc/stdio/local.h | 2 -- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 66867e664..afbdad9b1 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -235,11 +235,11 @@ cleanup_stdio (struct _reent *ptr) void __sinit (struct _reent *s) { - __sinit_lock_acquire (); + __sfp_lock_acquire (); if (s->__cleanup) { - __sinit_lock_release (); + __sfp_lock_release (); return; } @@ -268,13 +268,12 @@ __sinit (struct _reent *s) stderr_init (s->_stderr); #endif /* _REENT_GLOBAL_STDIO_STREAMS */ - __sinit_lock_release (); + __sfp_lock_release (); } #ifndef __SINGLE_THREAD__ __LOCK_INIT_RECURSIVE(static, __sfp_recursive_mutex); -__LOCK_INIT_RECURSIVE(static, __sinit_recursive_mutex); void __sfp_lock_acquire (void) @@ -288,18 +287,6 @@ __sfp_lock_release (void) __lock_release_recursive (__sfp_recursive_mutex); } -void -__sinit_lock_acquire (void) -{ - __lock_acquire_recursive (__sinit_recursive_mutex); -} - -void -__sinit_lock_release (void) -{ - __lock_release_recursive (__sinit_recursive_mutex); -} - /* Walkable file locking routine. */ static int __fp_lock (struct _reent * ptr __unused, FILE * fp) diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h index 30c534dcd..9c6f63fdb 100644 --- a/newlib/libc/stdio/local.h +++ b/newlib/libc/stdio/local.h @@ -287,8 +287,6 @@ char *_llicvt (char *, long long, char); #else void __sfp_lock_acquire (void); void __sfp_lock_release (void); -void __sinit_lock_acquire (void); -void __sinit_lock_release (void); #endif /* Types used in positional argument support in vfprinf/vfwprintf. -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Moved last remaining __sglue initializations from __sinit() to __sfp(). The move better encapsulates access to __sglue and facilitates its decoupling from struct _reent in a follow-on patch. --- newlib/libc/stdio/findfp.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index afbdad9b1..7434c343c 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -151,8 +151,14 @@ __sfp (struct _reent *d) _newlib_sfp_lock_start (); - if (_GLOBAL_REENT->__cleanup == NULL) + if (_GLOBAL_REENT->__cleanup == NULL) { +#ifdef _REENT_GLOBAL_STDIO_STREAMS + _GLOBAL_REENT->__sglue._niobs = 3; + _GLOBAL_REENT->__sglue._iobs = &__sf[0]; +#endif __sinit (_GLOBAL_REENT); + } + for (g = &_GLOBAL_REENT->__sglue;; g = g->_next) { for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++) @@ -256,8 +262,6 @@ __sinit (struct _reent *s) #ifdef _REENT_GLOBAL_STDIO_STREAMS if (__sf[0]._cookie == NULL) { - _GLOBAL_REENT->__sglue._niobs = 3; - _GLOBAL_REENT->__sglue._iobs = &__sf[0]; stdin_init (&__sf[0]); stdout_init (&__sf[1]); stderr_init (&__sf[2]); -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Define the configuration-dependent constant CLEANUP_FILE for use in cleanup_stdio(). This will reduce duplicate code during the addition of a dedicated stdio atexit handler in a follow-on patch. --- newlib/libc/stdio/findfp.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 7434c343c..5b0402ac1 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -39,6 +39,21 @@ const struct __sFILE_fake __sf_fake_stderr = __FILE __sf[3]; #endif +#ifdef _STDIO_BSD_SEMANTICS + /* BSD and Glibc systems only flush streams which have been written to + at exit time. Calling flush rather than close for speed, as on + the aforementioned systems. */ +#define CLEANUP_FILE __sflushw_r +#else + /* Otherwise close files and flush read streams, too. + Note we call flush directly if "--enable-lite-exit" is in effect. */ +#ifdef _LITE_EXIT +#define CLEANUP_FILE _fflush_r +#else +#define CLEANUP_FILE _fclose_r +#endif +#endif + #if (defined (__OPTIMIZE_SIZE__) || defined (PREFER_SIZE_OVER_SPEED)) _NOINLINE_STATIC void #else @@ -208,30 +223,15 @@ found: static void cleanup_stdio (struct _reent *ptr) { - int (*cleanup_func) (struct _reent *, FILE *); -#ifdef _STDIO_BSD_SEMANTICS - /* BSD and Glibc systems only flush streams which have been written to - at exit time. Calling flush rather than close for speed, as on - the aforementioned systems. */ - cleanup_func = __sflushw_r; -#else - /* Otherwise close files and flush read streams, too. - Note we call flush directly if "--enable-lite-exit" is in effect. */ -#ifdef _LITE_EXIT - cleanup_func = _fflush_r; -#else - cleanup_func = _fclose_r; -#endif -#endif #ifdef _REENT_GLOBAL_STDIO_STREAMS if (ptr->_stdin != &__sf[0]) - (*cleanup_func) (ptr, ptr->_stdin); + CLEANUP_FILE (ptr, ptr->_stdin); if (ptr->_stdout != &__sf[1]) - (*cleanup_func) (ptr, ptr->_stdout); + CLEANUP_FILE (ptr, ptr->_stdout); if (ptr->_stderr != &__sf[2]) - (*cleanup_func) (ptr, ptr->_stderr); + CLEANUP_FILE (ptr, ptr->_stderr); #endif - (void) _fwalk_reent (ptr, cleanup_func); + (void) _fwalk_reent (ptr, CLEANUP_FILE); } /* -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Added a dedicated stdio exit handler to avoid using _GLOBAL_REENT in exit(). --- newlib/libc/include/sys/reent.h | 2 ++ newlib/libc/stdio/findfp.c | 11 ++++++++++- newlib/libc/stdlib/exit.c | 5 +++-- winsup/cygwin/signal.cc | 5 +++-- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h index 4b0d68610..067294dc7 100644 --- a/newlib/libc/include/sys/reent.h +++ b/newlib/libc/include/sys/reent.h @@ -837,6 +837,8 @@ extern struct _reent *_impure_ptr __ATTRIBUTE_IMPURE_PTR__; extern struct _reent _impure_data __ATTRIBUTE_IMPURE_DATA__; +extern void (*__stdio_exit_handler) (void); + void _reclaim_reent (struct _reent *); /* #define _REENT_ONLY define this to get only reentrant routines */ diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 5b0402ac1..719886e27 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -26,6 +26,8 @@ #include <sys/lock.h> #include "local.h" +void (*__stdio_exit_handler) (void); + #if defined(_REENT_SMALL) && !defined(_REENT_GLOBAL_STDIO_STREAMS) const struct __sFILE_fake __sf_fake_stdin = {_NULL, 0, 0, 0, 0, {_NULL, 0}, 0, _NULL}; @@ -153,6 +155,12 @@ sfmoreglue (struct _reent *d, int n) return &g->glue; } +static void +stdio_exit_handler (void) +{ + (void) _fwalk_reent (_GLOBAL_REENT, CLEANUP_FILE); +} + /* * Find a free FILE for fopen et al. */ @@ -166,12 +174,13 @@ __sfp (struct _reent *d) _newlib_sfp_lock_start (); - if (_GLOBAL_REENT->__cleanup == NULL) { + if (__stdio_exit_handler == NULL) { #ifdef _REENT_GLOBAL_STDIO_STREAMS _GLOBAL_REENT->__sglue._niobs = 3; _GLOBAL_REENT->__sglue._iobs = &__sf[0]; #endif __sinit (_GLOBAL_REENT); + __stdio_exit_handler = stdio_exit_handler; } for (g = &_GLOBAL_REENT->__sglue;; g = g->_next) diff --git a/newlib/libc/stdlib/exit.c b/newlib/libc/stdlib/exit.c index 3e618914e..9b7bd518b 100644 --- a/newlib/libc/stdlib/exit.c +++ b/newlib/libc/stdlib/exit.c @@ -59,7 +59,8 @@ exit (int code) #endif __call_exitprocs (code, NULL); - if (_GLOBAL_REENT->__cleanup) - (*_GLOBAL_REENT->__cleanup) (_GLOBAL_REENT); + if (__stdio_exit_handler != NULL) + (*__stdio_exit_handler) (); + _exit (code); } diff --git a/winsup/cygwin/signal.cc b/winsup/cygwin/signal.cc index 9b6c2509d..f5f2efdc7 100644 --- a/winsup/cygwin/signal.cc +++ b/winsup/cygwin/signal.cc @@ -13,6 +13,7 @@ details. */ #include <stdlib.h> #include <sys/cygwin.h> #include <sys/signalfd.h> +#include <sys/reent.h> /* needed for __stdio_exit_handler declaration */ #include "pinfo.h" #include "sigproc.h" #include "cygtls.h" @@ -408,8 +409,8 @@ abort (void) _my_tls.call_signal_handler (); /* Call any signal handler */ /* Flush all streams as per SUSv2. */ - if (_GLOBAL_REENT->__cleanup) - _GLOBAL_REENT->__cleanup (_GLOBAL_REENT); + if (__stdio_exit_handler != NULL) + (*__stdio_exit_handler) (); do_exit (SIGABRT); /* signal handler didn't exit. Goodbye. */ } -- 2.31.1
From: Sebastian Huber <sebastian.huber@embedded-brains.de> Replaced _fwalk_reent() with _fwalk_sglue(). The change adds an extra __sglue object as a parameter, which will allow the passing of a global __sglue object separate from the __sglue member of struct _reent. The global __sglue object will be added in a follow-on patch. --- newlib/libc/include/sys/reent.h | 3 +++ newlib/libc/stdio/fcloseall.c | 2 +- newlib/libc/stdio/fflush.c | 2 +- newlib/libc/stdio/findfp.c | 14 ++++++++++---- newlib/libc/stdio/fwalk.c | 16 ++++++++-------- newlib/libc/stdio/local.h | 1 - newlib/libc/stdio/refill.c | 4 ++-- winsup/cygwin/syscalls.cc | 6 ++---- 8 files changed, 27 insertions(+), 21 deletions(-) diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h index 067294dc7..6e420d0c8 100644 --- a/newlib/libc/include/sys/reent.h +++ b/newlib/libc/include/sys/reent.h @@ -841,6 +841,9 @@ extern void (*__stdio_exit_handler) (void); void _reclaim_reent (struct _reent *); +extern int _fwalk_sglue (struct _reent *, int (*)(struct _reent *, __FILE *), + struct _glue *); + /* #define _REENT_ONLY define this to get only reentrant routines */ #if defined(__DYNAMIC_REENT__) && !defined(__SINGLE_THREAD__) diff --git a/newlib/libc/stdio/fcloseall.c b/newlib/libc/stdio/fcloseall.c index 014c451cd..4d4e3554b 100644 --- a/newlib/libc/stdio/fcloseall.c +++ b/newlib/libc/stdio/fcloseall.c @@ -59,7 +59,7 @@ Required OS subroutines: <<close>>, <<fstat>>, <<isatty>>, <<lseek>>, int _fcloseall_r (struct _reent *ptr) { - return _fwalk_reent (ptr, _fclose_r); + return _fwalk_sglue (ptr, _fclose_r, &ptr->__sglue); } #ifndef _REENT_ONLY diff --git a/newlib/libc/stdio/fflush.c b/newlib/libc/stdio/fflush.c index 2b5f13bff..6e946da7b 100644 --- a/newlib/libc/stdio/fflush.c +++ b/newlib/libc/stdio/fflush.c @@ -286,7 +286,7 @@ int fflush (register FILE * fp) { if (fp == NULL) - return _fwalk_reent (_GLOBAL_REENT, _fflush_r); + return _fwalk_sglue (_GLOBAL_REENT, _fflush_r, &_GLOBAL_REENT->__sglue); return _fflush_r (_REENT, fp); } diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 719886e27..1627032a3 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -158,7 +158,7 @@ sfmoreglue (struct _reent *d, int n) static void stdio_exit_handler (void) { - (void) _fwalk_reent (_GLOBAL_REENT, CLEANUP_FILE); + (void) _fwalk_sglue (_GLOBAL_REENT, CLEANUP_FILE, &_GLOBAL_REENT->__sglue); } /* @@ -240,7 +240,7 @@ cleanup_stdio (struct _reent *ptr) if (ptr->_stderr != &__sf[2]) CLEANUP_FILE (ptr, ptr->_stderr); #endif - (void) _fwalk_reent (ptr, CLEANUP_FILE); + (void) _fwalk_sglue (ptr, CLEANUP_FILE, &ptr->__sglue); } /* @@ -323,15 +323,21 @@ __fp_unlock (struct _reent * ptr __unused, FILE * fp) void __fp_lock_all (void) { + struct _reent *ptr; + __sfp_lock_acquire (); - (void) _fwalk_reent (_REENT, __fp_lock); + ptr = _REENT; + (void) _fwalk_sglue (ptr, __fp_lock, &ptr->__sglue); } void __fp_unlock_all (void) { - (void) _fwalk_reent (_REENT, __fp_unlock); + struct _reent *ptr; + + ptr = _REENT; + (void) _fwalk_sglue (ptr, __fp_unlock, &ptr->__sglue); __sfp_lock_release (); } diff --git a/newlib/libc/stdio/fwalk.c b/newlib/libc/stdio/fwalk.c index 2cefcc40e..b1284c6ea 100644 --- a/newlib/libc/stdio/fwalk.c +++ b/newlib/libc/stdio/fwalk.c @@ -25,15 +25,13 @@ static char sccsid[] = "%W% (Berkeley) %G%"; #include <stdio.h> #include <stdlib.h> #include <errno.h> -#include "local.h" int -_fwalk_reent (struct _reent *ptr, - register int (*reent_function) (struct _reent *, FILE *)) +_fwalk_sglue (struct _reent *ptr, int (*func) (struct _reent *, FILE *), + struct _glue *g) { - register FILE *fp; - register int n, ret = 0; - register struct _glue *g; + FILE *fp; + int n, ret = 0; /* * It should be safe to walk the list without locking it; @@ -43,10 +41,12 @@ _fwalk_reent (struct _reent *ptr, * Avoid locking this list while walking it or else you will * introduce a potential deadlock in [at least] refill.c. */ - for (g = &ptr->__sglue; g != NULL; g = g->_next) + do { for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++) if (fp->_flags != 0 && fp->_flags != 1 && fp->_file != -1) - ret |= (*reent_function) (ptr, fp); + ret |= (*func) (ptr, fp); + g = g->_next; + } while (g != NULL); return ret; } diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h index 9c6f63fdb..e245fdb4e 100644 --- a/newlib/libc/stdio/local.h +++ b/newlib/libc/stdio/local.h @@ -182,7 +182,6 @@ extern int __stextmode (int); extern void __sinit (struct _reent *); extern void __smakebuf_r (struct _reent *, FILE *); extern int __swhatbuf_r (struct _reent *, FILE *, size_t *, int *); -extern int _fwalk_reent (struct _reent *, int (*)(struct _reent *, FILE *)); extern int __submore (struct _reent *, FILE *); #ifdef __LARGE64_FILES diff --git a/newlib/libc/stdio/refill.c b/newlib/libc/stdio/refill.c index 31665bcd9..4084d7250 100644 --- a/newlib/libc/stdio/refill.c +++ b/newlib/libc/stdio/refill.c @@ -102,10 +102,10 @@ __srefill_r (struct _reent * ptr, */ if (fp->_flags & (__SLBF | __SNBF)) { - /* Ignore this file in _fwalk_reent to avoid potential deadlock. */ + /* Ignore this file in _fwalk_sglue to avoid potential deadlock. */ short orig_flags = fp->_flags; fp->_flags = 1; - (void) _fwalk_reent (_GLOBAL_REENT, lflush); + (void) _fwalk_sglue (_GLOBAL_REENT, lflush, &_GLOBAL_REENT->__sglue); fp->_flags = orig_flags; /* Now flush this file without locking it. */ diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index 1cecaa017..fb994ac41 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -63,6 +63,7 @@ details. */ #include "sync.h" #include "child_info.h" #include <cygwin/fs.h> /* needed for RENAME_NOREPLACE */ +#include <sys/reent.h> /* needed for _fwalk_sglue() declaration */ #undef _close #undef _lseek @@ -3057,9 +3058,6 @@ _cygwin_istext_for_stdio (int fd) return 1; } -/* internal newlib function */ -extern "C" int _fwalk_reent (struct _reent *ptr, int (*function) (struct _reent *, FILE *)); - static int setmode_helper (struct _reent *ptr __unused, FILE *f) { @@ -3137,7 +3135,7 @@ cygwin_setmode (int fd, int mode) _my_tls.locals.setmode_mode = O_TEXT; else _my_tls.locals.setmode_mode = O_BINARY; - _fwalk_reent (_GLOBAL_REENT, setmode_helper); + _fwalk_sglue (_GLOBAL_REENT, setmode_helper, &_GLOBAL_REENT->__sglue); } return res; } -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Added a new global __sglue object for all configurations. Decouples the global file object list from the _GLOBAL_REENT structure by using this new object instead of the __sglue member of _GLOBAL_REENT in __sfp() and _fwalk_sglue(). --- newlib/libc/include/sys/reent.h | 2 ++ newlib/libc/stdio/fcloseall.c | 2 +- newlib/libc/stdio/fflush.c | 2 +- newlib/libc/stdio/findfp.c | 15 +++++++++------ newlib/libc/stdio/refill.c | 2 +- winsup/cygwin/syscalls.cc | 2 +- 6 files changed, 15 insertions(+), 10 deletions(-) diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h index 6e420d0c8..2c0598a5d 100644 --- a/newlib/libc/include/sys/reent.h +++ b/newlib/libc/include/sys/reent.h @@ -304,6 +304,8 @@ struct _glue __FILE *_iobs; }; +extern struct _glue __sglue; + /* * rand48 family support * diff --git a/newlib/libc/stdio/fcloseall.c b/newlib/libc/stdio/fcloseall.c index 4d4e3554b..f14c28d34 100644 --- a/newlib/libc/stdio/fcloseall.c +++ b/newlib/libc/stdio/fcloseall.c @@ -67,7 +67,7 @@ _fcloseall_r (struct _reent *ptr) int fcloseall (void) { - return _fcloseall_r (_GLOBAL_REENT); + return _fwalk_sglue (_GLOBAL_REENT, _fclose_r, &__sglue); } #endif diff --git a/newlib/libc/stdio/fflush.c b/newlib/libc/stdio/fflush.c index 6e946da7b..bbec4a19b 100644 --- a/newlib/libc/stdio/fflush.c +++ b/newlib/libc/stdio/fflush.c @@ -286,7 +286,7 @@ int fflush (register FILE * fp) { if (fp == NULL) - return _fwalk_sglue (_GLOBAL_REENT, _fflush_r, &_GLOBAL_REENT->__sglue); + return _fwalk_sglue (_GLOBAL_REENT, _fflush_r, &__sglue); return _fflush_r (_REENT, fp); } diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 1627032a3..bf471369c 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -39,6 +39,13 @@ const struct __sFILE_fake __sf_fake_stderr = #ifdef _REENT_GLOBAL_STDIO_STREAMS __FILE __sf[3]; +struct _glue __sglue = {NULL, 3, &__sf[0]}; +#else +#ifndef _REENT_SMALL +struct _glue __sglue = {NULL, 3, &_GLOBAL_REENT->__sf[0]}; +#else +struct _glue __sglue = {NULL, 0, NULL}; +#endif #endif #ifdef _STDIO_BSD_SEMANTICS @@ -158,7 +165,7 @@ sfmoreglue (struct _reent *d, int n) static void stdio_exit_handler (void) { - (void) _fwalk_sglue (_GLOBAL_REENT, CLEANUP_FILE, &_GLOBAL_REENT->__sglue); + (void) _fwalk_sglue (_GLOBAL_REENT, CLEANUP_FILE, &__sglue); } /* @@ -175,15 +182,11 @@ __sfp (struct _reent *d) _newlib_sfp_lock_start (); if (__stdio_exit_handler == NULL) { -#ifdef _REENT_GLOBAL_STDIO_STREAMS - _GLOBAL_REENT->__sglue._niobs = 3; - _GLOBAL_REENT->__sglue._iobs = &__sf[0]; -#endif __sinit (_GLOBAL_REENT); __stdio_exit_handler = stdio_exit_handler; } - for (g = &_GLOBAL_REENT->__sglue;; g = g->_next) + for (g = &__sglue;; g = g->_next) { for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++) if (fp->_flags == 0) diff --git a/newlib/libc/stdio/refill.c b/newlib/libc/stdio/refill.c index 4084d7250..8516d8576 100644 --- a/newlib/libc/stdio/refill.c +++ b/newlib/libc/stdio/refill.c @@ -105,7 +105,7 @@ __srefill_r (struct _reent * ptr, /* Ignore this file in _fwalk_sglue to avoid potential deadlock. */ short orig_flags = fp->_flags; fp->_flags = 1; - (void) _fwalk_sglue (_GLOBAL_REENT, lflush, &_GLOBAL_REENT->__sglue); + (void) _fwalk_sglue (_GLOBAL_REENT, lflush, &__sglue); fp->_flags = orig_flags; /* Now flush this file without locking it. */ diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index fb994ac41..bb9df76cf 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -3135,7 +3135,7 @@ cygwin_setmode (int fd, int mode) _my_tls.locals.setmode_mode = O_TEXT; else _my_tls.locals.setmode_mode = O_BINARY; - _fwalk_sglue (_GLOBAL_REENT, setmode_helper, &_GLOBAL_REENT->__sglue); + _fwalk_sglue (_GLOBAL_REENT, setmode_helper, &__sglue); } return res; } -- 2.31.1
From: Matt Joyce <matthew.joyce@embedded-brains.de> Removed __sglue member of struct reent when _REENT_GLOBAL_STDIO_STREAMS is defined. --- newlib/libc/include/sys/reent.h | 8 ++++---- newlib/libc/reent/reent.c | 6 +++++- newlib/libc/stdio/fcloseall.c | 5 +++++ newlib/libc/stdio/findfp.c | 9 ++++++++- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h index 2c0598a5d..5cdc67f68 100644 --- a/newlib/libc/include/sys/reent.h +++ b/newlib/libc/include/sys/reent.h @@ -696,19 +696,19 @@ struct _reent /* These are here last so that __FILE can grow without changing the offsets of the above members (on the off chance that future binary compatibility would be broken otherwise). */ - struct _glue __sglue; /* root of glue chain */ # ifndef _REENT_GLOBAL_STDIO_STREAMS + struct _glue __sglue; /* root of glue chain */ __FILE __sf[3]; /* first three file descriptors */ # endif }; #ifdef _REENT_GLOBAL_STDIO_STREAMS #define _REENT_STDIO_STREAM(var, index) &__sf[index] -#define _REENT_INIT_SGLUE(_ptr) { _NULL, 0, _NULL } +#define _REENT_INIT_SGLUE(_ptr) /* nothing to initialize */ #define _REENT_INIT_SGLUE_ZEROED(_ptr) /* nothing to set */ #else #define _REENT_STDIO_STREAM(var, index) &(var)->__sf[index] -#define _REENT_INIT_SGLUE(_ptr) { _NULL, 3, &(_ptr)->__sf[0] } +#define _REENT_INIT_SGLUE(_ptr) , { _NULL, 3, &(_ptr)->__sf[0] } #define _REENT_INIT_SGLUE_ZEROED(_ptr) \ (_ptr)->__sglue._niobs = 3; \ (_ptr)->__sglue._iobs = &(_ptr)->__sf[0]; @@ -758,7 +758,7 @@ struct _reent } \ }, \ _REENT_INIT_ATEXIT \ - _NULL, \ + _NULL \ _REENT_INIT_SGLUE(&(var)) \ } diff --git a/newlib/libc/reent/reent.c b/newlib/libc/reent/reent.c index 5a04fcf62..70f1c5f45 100644 --- a/newlib/libc/reent/reent.c +++ b/newlib/libc/reent/reent.c @@ -27,9 +27,10 @@ int errno; #endif +#ifndef _REENT_GLOBAL_STDIO_STREAMS /* Interim cleanup code */ -void +static void cleanup_glue (struct _reent *ptr, struct _glue *glue) { @@ -39,6 +40,7 @@ cleanup_glue (struct _reent *ptr, _free_r (ptr, glue); } +#endif void _reclaim_reent (struct _reent *ptr) @@ -124,8 +126,10 @@ _reclaim_reent (struct _reent *ptr) before the program exits, and who wants to wait for that? */ ptr->__cleanup (ptr); +#ifndef _REENT_GLOBAL_STDIO_STREAMS if (ptr->__sglue._next) cleanup_glue (ptr, ptr->__sglue._next); +#endif } /* Malloc memory not reclaimed; no good way to return memory anyway. */ diff --git a/newlib/libc/stdio/fcloseall.c b/newlib/libc/stdio/fcloseall.c index f14c28d34..642dc7d94 100644 --- a/newlib/libc/stdio/fcloseall.c +++ b/newlib/libc/stdio/fcloseall.c @@ -59,7 +59,12 @@ Required OS subroutines: <<close>>, <<fstat>>, <<isatty>>, <<lseek>>, int _fcloseall_r (struct _reent *ptr) { +#ifdef _REENT_GLOBAL_STDIO_STREAMS + /* There are no thread-specific FILE objects */ + return 0; +#else return _fwalk_sglue (ptr, _fclose_r, &ptr->__sglue); +#endif } #ifndef _REENT_ONLY diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index bf471369c..cad50d4e1 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -242,8 +242,9 @@ cleanup_stdio (struct _reent *ptr) CLEANUP_FILE (ptr, ptr->_stdout); if (ptr->_stderr != &__sf[2]) CLEANUP_FILE (ptr, ptr->_stderr); -#endif +#else (void) _fwalk_sglue (ptr, CLEANUP_FILE, &ptr->__sglue); +#endif } /* @@ -326,21 +327,27 @@ __fp_unlock (struct _reent * ptr __unused, FILE * fp) void __fp_lock_all (void) { +#ifndef _REENT_GLOBAL_STDIO_STREAMS struct _reent *ptr; +#endif __sfp_lock_acquire (); +#ifndef _REENT_GLOBAL_STDIO_STREAMS ptr = _REENT; (void) _fwalk_sglue (ptr, __fp_lock, &ptr->__sglue); +#endif } void __fp_unlock_all (void) { +#ifndef _REENT_GLOBAL_STDIO_STREAMS struct _reent *ptr; ptr = _REENT; (void) _fwalk_sglue (ptr, __fp_unlock, &ptr->__sglue); +#endif __sfp_lock_release (); } -- 2.31.1
Hello, I'm not against this change, but I have some questions related to the change... What about the retargetable locking scheme? Should the mutex __sinit_recursive_mutex be removed from newlib/libc/misc/lock.c too? Without removing the mutex, I suppose there would be a mutex that is never used in the system. Removing the mutex would pose an API break as I assume that the functions defined in newlib/libc/include/sys/lock.h (and implemented in newlib/libc/misc/lock.c) are to be considered API as it's the functions that the application needs to provide in order to ensure newlib is thread safe. Does this change force newlib to do a major release? Kind regards, Torbjörn -----Original Message----- From: Newlib <newlib-bounces+torbjorn.svensson=st.com@sourceware.org> On Behalf Of Matthew Joyce Sent: den 12 maj 2022 14:12 To: newlib@sourceware.org Subject: [PATCH v2 05/11] Remove __sinit_locks / __sinit_recursive_mutex From: Matt Joyce <matthew.joyce@embedded-brains.de> Removed __sinit_lock_acquire() and __sinit_lock_release(). Replace these with __sfp_lock_acquire() and __sfp_lock_release(), respectively. This eliminates a potential deadlock issue between __sinit() and __sfp(). Removed now unused __sinit_recursive_mutex. --- newlib/libc/stdio/findfp.c | 19 +++---------------- newlib/libc/stdio/local.h | 2 -- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 66867e664..afbdad9b1 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -235,11 +235,11 @@ cleanup_stdio (struct _reent *ptr) void __sinit (struct _reent *s) { - __sinit_lock_acquire (); + __sfp_lock_acquire (); if (s->__cleanup) { - __sinit_lock_release (); + __sfp_lock_release (); return; } @@ -268,13 +268,12 @@ __sinit (struct _reent *s) stderr_init (s->_stderr); #endif /* _REENT_GLOBAL_STDIO_STREAMS */ - __sinit_lock_release (); + __sfp_lock_release (); } #ifndef __SINGLE_THREAD__ __LOCK_INIT_RECURSIVE(static, __sfp_recursive_mutex); -__LOCK_INIT_RECURSIVE(static, __sinit_recursive_mutex); void __sfp_lock_acquire (void) @@ -288,18 +287,6 @@ __sfp_lock_release (void) __lock_release_recursive (__sfp_recursive_mutex); } -void -__sinit_lock_acquire (void) -{ - __lock_acquire_recursive (__sinit_recursive_mutex); -} - -void -__sinit_lock_release (void) -{ - __lock_release_recursive (__sinit_recursive_mutex); -} - /* Walkable file locking routine. */ static int __fp_lock (struct _reent * ptr __unused, FILE * fp) diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h index 30c534dcd..9c6f63fdb 100644 --- a/newlib/libc/stdio/local.h +++ b/newlib/libc/stdio/local.h @@ -287,8 +287,6 @@ char *_llicvt (char *, long long, char); #else void __sfp_lock_acquire (void); void __sfp_lock_release (void); -void __sinit_lock_acquire (void); -void __sinit_lock_release (void); #endif /* Types used in positional argument support in vfprinf/vfwprintf. -- 2.31.1
On 12/05/2022 15:25, Torbjorn SVENSSON via Newlib wrote: > What about the retargetable locking scheme? > Should the mutex __sinit_recursive_mutex be removed from newlib/libc/misc/lock.c too? Yes, we should this dummy mutex. > Without removing the mutex, I suppose there would be a mutex that is never used in the system. > Removing the mutex would pose an API break as I assume that the functions defined in newlib/libc/include/sys/lock.h (and implemented in newlib/libc/misc/lock.c) are to be considered API as it's the functions that the application needs to provide in order to ensure newlib is thread safe. It shouldn't hurt if a lock implementer provides an unused mutex. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.huber@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/
On May 12 14:11, Matthew Joyce wrote: > From: Matt Joyce <matthew.joyce@embedded-brains.de> > > Removed __sglue member of struct reent when > _REENT_GLOBAL_STDIO_STREAMS is defined. > --- > newlib/libc/include/sys/reent.h | 8 ++++---- > newlib/libc/reent/reent.c | 6 +++++- > newlib/libc/stdio/fcloseall.c | 5 +++++ > newlib/libc/stdio/findfp.c | 9 ++++++++- > 4 files changed, 22 insertions(+), 6 deletions(-) This patch requires another change in Cygwin. For some reason, entirely lost in the mist of time, Cygwin exports the cleanup_glue function. It already did so when winsup and newlib sources have been imported into the sourceware CVS repo, back in 2000. At this point I'd usually say that we can't change that because backward compatibility and yada yada, but it just doesn't make any sense to export cleanup_glue, and never did. At all. So, instead of reverting cleanup_glue to non-static, let's pull this through and let's drop it from the list of exported functions. That means, this patch needs a tiny extension and the commit message should mention that it's removing cleanup_glue from the list of exported symbols. Here's the diff of the required change, please add it to this patch: diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din index 63fa09af2553..751ab3754421 100644 --- a/winsup/cygwin/common.din +++ b/winsup/cygwin/common.din @@ -316,7 +316,6 @@ chroot SIGFE cimag NOSIGFE cimagf NOSIGFE cimagl NOSIGFE -cleanup_glue NOSIGFE clearenv SIGFE clearerr SIGFE clearerr_unlocked SIGFE diff --git a/winsup/cygwin/include/cygwin/version.h b/winsup/cygwin/include/cygwin/version.h index ad7fab43f458..6f65a12994bc 100644 --- a/winsup/cygwin/include/cygwin/version.h +++ b/winsup/cygwin/include/cygwin/version.h @@ -516,12 +516,13 @@ details. */ 341: Export pthread_cond_clockwait, pthread_mutex_clocklock, pthread_rwlock_clockrdlock, pthread_rwlock_clockwrlock, sem_clockwait, sig2str, str2sig. + 342: Remove cleanup_glue. Note that we forgot to bump the api for ualarm, strtoll, strtoull, sigaltstack, sethostname. */ #define CYGWIN_VERSION_API_MAJOR 0 -#define CYGWIN_VERSION_API_MINOR 341 +#define CYGWIN_VERSION_API_MINOR 342 /* There is also a compatibity version number associated with the shared memory regions. It is incremented when incompatible changes are made to the shared Thanks, Corinna
On May 12 14:11, Matthew Joyce wrote:
> From: Matt Joyce <matthew.joyce@embedded-brains.de>
>
> Added a dedicated stdio exit handler to avoid using _GLOBAL_REENT
> in exit().
> ---
> newlib/libc/include/sys/reent.h | 2 ++
> newlib/libc/stdio/findfp.c | 11 ++++++++++-
> newlib/libc/stdlib/exit.c | 5 +++--
> winsup/cygwin/signal.cc | 5 +++--
> 4 files changed, 18 insertions(+), 5 deletions(-)
Just FYI, the winsup/cygwin/signal.cc snippet doesn't apply cleanly
anymore due to Sebastian's patch. It's just a minor conflict,
nothing to worry about.
Corinna
Hi Matt,
On May 12 14:11, Matthew Joyce wrote:
> From: Matt Joyce <matthew.joyce@embedded-brains.de>
>
> Hello Corinna,
>
> Per your comments, please see version 2.
>
> 1) stdio_atexit is now called stdio_exit_handler
> 2) declarations for stdio_exit_handler() and _fwalk_sglue() are moved
> to sys/reent.h
Please see my comments in terms of patch 11, which needs an additional
tweak. With this change, the patchset is GTG.
Thanks,
Corinna
On May 12 21:14, Corinna Vinschen wrote: > Hi Matt, > > On May 12 14:11, Matthew Joyce wrote: > > From: Matt Joyce <matthew.joyce@embedded-brains.de> > > > > Hello Corinna, > > > > Per your comments, please see version 2. > > > > 1) stdio_atexit is now called stdio_exit_handler > > 2) declarations for stdio_exit_handler() and _fwalk_sglue() are moved > > to sys/reent.h > > Please see my comments in terms of patch 11, which needs an additional > tweak. With this change, the patchset is GTG. Btw., it looks like Cygwin can switch to _REENT_GLOBAL_STDIO_STREAMS with only minor changes. I'll test this a bit more before pushing, but it's quite neat so far... diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc index c8352adf9f25..0fd5509d2306 100644 --- a/winsup/cygwin/cygtls.cc +++ b/winsup/cygwin/cygtls.cc @@ -55,16 +55,8 @@ _cygtls::init_thread (void *x, DWORD (*func) (void *, void *)) _REENT_INIT_PTR (&local_clib); stackptr = stack; altstack.ss_flags = SS_DISABLE; - if (_GLOBAL_REENT) - { - local_clib._stdin = _GLOBAL_REENT->_stdin; - local_clib._stdout = _GLOBAL_REENT->_stdout; - local_clib._stderr = _GLOBAL_REENT->_stderr; - if (_GLOBAL_REENT->__cleanup) - local_clib.__cleanup = _cygtls::cleanup_early; - local_clib.__sglue._niobs = 3; - local_clib.__sglue._iobs = &_GLOBAL_REENT->__sf[0]; - } + if (_GLOBAL_REENT && _GLOBAL_REENT->__cleanup) + local_clib.__cleanup = _cygtls::cleanup_early; } thread_id = GetCurrentThreadId (); diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc index 5f460d8a5c78..6b816763e002 100644 --- a/winsup/cygwin/dcrt0.cc +++ b/winsup/cygwin/dcrt0.cc @@ -757,9 +757,7 @@ dll_crt0_0 () lock_process::init (); _impure_ptr = _GLOBAL_REENT; - _impure_ptr->_stdin = &_impure_ptr->__sf[0]; - _impure_ptr->_stdout = &_impure_ptr->__sf[1]; - _impure_ptr->_stderr = &_impure_ptr->__sf[2]; + _REENT_INIT_PTR_ZEROED (_GLOBAL_REENT); user_data->impure_ptr = _impure_ptr; user_data->impure_ptr_ptr = &_impure_ptr; diff --git a/winsup/cygwin/include/cygwin/config.h b/winsup/cygwin/include/cygwin/config.h index 71a216fbd16d..56fc326bd43e 100644 --- a/winsup/cygwin/include/cygwin/config.h +++ b/winsup/cygwin/include/cygwin/config.h @@ -66,6 +66,7 @@ extern inline struct _reent *__getreent (void) /* The following block of macros is required to build newlib correctly for Cygwin. Changing them in applications has no or not the desired effect. Just leave them alone. */ +#define _REENT_GLOBAL_STDIO_STREAMS 1 #define _READ_WRITE_RETURN_TYPE _ssize_t #define _READ_WRITE_BUFSIZE_TYPE size_t #define __LARGE64_FILES 1 diff --git a/winsup/cygwin/tlsoffsets.h b/winsup/cygwin/tlsoffsets.h [...skipped, just autogenerated data...] diff --git a/winsup/cygwin/tlsoffsets64.h b/winsup/cygwin/tlsoffsets64.h [...skipped, just autogenerated data...] Corinna
On 12/05/2022 21:44, Corinna Vinschen wrote: > diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc > index c8352adf9f25..0fd5509d2306 100644 > --- a/winsup/cygwin/cygtls.cc > +++ b/winsup/cygwin/cygtls.cc > @@ -55,16 +55,8 @@ _cygtls::init_thread (void *x, DWORD (*func) (void *, void *)) > _REENT_INIT_PTR (&local_clib); > stackptr = stack; > altstack.ss_flags = SS_DISABLE; > - if (_GLOBAL_REENT) > - { > - local_clib._stdin = _GLOBAL_REENT->_stdin; > - local_clib._stdout = _GLOBAL_REENT->_stdout; > - local_clib._stderr = _GLOBAL_REENT->_stderr; > - if (_GLOBAL_REENT->__cleanup) > - local_clib.__cleanup = _cygtls::cleanup_early; > - local_clib.__sglue._niobs = 3; > - local_clib.__sglue._iobs = &_GLOBAL_REENT->__sf[0]; > - } > + if (_GLOBAL_REENT && _GLOBAL_REENT->__cleanup) > + local_clib.__cleanup = _cygtls::cleanup_early; We have: #define _GLOBAL_REENT (&_impure_data) So, the NULL pointer check is superfluous. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.huber@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/
On May 13 07:44, Sebastian Huber wrote:
> On 12/05/2022 21:44, Corinna Vinschen wrote:
> > diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc
> > index c8352adf9f25..0fd5509d2306 100644
> > --- a/winsup/cygwin/cygtls.cc
> > +++ b/winsup/cygwin/cygtls.cc
> > @@ -55,16 +55,8 @@ _cygtls::init_thread (void *x, DWORD (*func) (void *, void *))
> > _REENT_INIT_PTR (&local_clib);
> > stackptr = stack;
> > altstack.ss_flags = SS_DISABLE;
> > - if (_GLOBAL_REENT)
> > - {
> > - local_clib._stdin = _GLOBAL_REENT->_stdin;
> > - local_clib._stdout = _GLOBAL_REENT->_stdout;
> > - local_clib._stderr = _GLOBAL_REENT->_stderr;
> > - if (_GLOBAL_REENT->__cleanup)
> > - local_clib.__cleanup = _cygtls::cleanup_early;
> > - local_clib.__sglue._niobs = 3;
> > - local_clib.__sglue._iobs = &_GLOBAL_REENT->__sf[0];
> > - }
> > + if (_GLOBAL_REENT && _GLOBAL_REENT->__cleanup)
> > + local_clib.__cleanup = _cygtls::cleanup_early;
>
> We have:
>
> #define _GLOBAL_REENT (&_impure_data)
>
> So, the NULL pointer check is superfluous.
Oh, right, thanks!
Corinna
Hi Sebastian, Hi Matt,
On May 12 21:14, Corinna Vinschen wrote:
> Hi Matt,
>
> On May 12 14:11, Matthew Joyce wrote:
> > From: Matt Joyce <matthew.joyce@embedded-brains.de>
> >
> > Hello Corinna,
> >
> > Per your comments, please see version 2.
> >
> > 1) stdio_atexit is now called stdio_exit_handler
> > 2) declarations for stdio_exit_handler() and _fwalk_sglue() are moved
> > to sys/reent.h
>
> Please see my comments in terms of patch 11, which needs an additional
> tweak. With this change, the patchset is GTG.
I forgot to mention... you don't have to send a v3, just push the
patchset with this additional tweak (and talk about it in the commit
message).
Thanks,
Corinna
On 12/05/2022 21:44, Corinna Vinschen wrote: > diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc > index 5f460d8a5c78..6b816763e002 100644 > --- a/winsup/cygwin/dcrt0.cc > +++ b/winsup/cygwin/dcrt0.cc > @@ -757,9 +757,7 @@ dll_crt0_0 () > > lock_process::init (); > _impure_ptr = _GLOBAL_REENT; > - _impure_ptr->_stdin = &_impure_ptr->__sf[0]; > - _impure_ptr->_stdout = &_impure_ptr->__sf[1]; > - _impure_ptr->_stderr = &_impure_ptr->__sf[2]; > + _REENT_INIT_PTR_ZEROED (_GLOBAL_REENT); > user_data->impure_ptr = _impure_ptr; > user_data->impure_ptr_ptr = &_impure_ptr; > The _GLOBAL_REENT == (&_impure_data) is statically initialized in impure.c: struct _reent __ATTRIBUTE_IMPURE_DATA__ _impure_data = _REENT_INIT (_impure_data); I guess you don't need the _REENT_INIT_PTR_ZEROED (_GLOBAL_REENT). -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.huber@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/
On May 13 12:54, Sebastian Huber wrote:
> On 12/05/2022 21:44, Corinna Vinschen wrote:
> > diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
> > index 5f460d8a5c78..6b816763e002 100644
> > --- a/winsup/cygwin/dcrt0.cc
> > +++ b/winsup/cygwin/dcrt0.cc
> > @@ -757,9 +757,7 @@ dll_crt0_0 ()
> > lock_process::init ();
> > _impure_ptr = _GLOBAL_REENT;
> > - _impure_ptr->_stdin = &_impure_ptr->__sf[0];
> > - _impure_ptr->_stdout = &_impure_ptr->__sf[1];
> > - _impure_ptr->_stderr = &_impure_ptr->__sf[2];
> > + _REENT_INIT_PTR_ZEROED (_GLOBAL_REENT);
> > user_data->impure_ptr = _impure_ptr;
> > user_data->impure_ptr_ptr = &_impure_ptr;
>
> The _GLOBAL_REENT == (&_impure_data) is statically initialized in impure.c:
>
> struct _reent __ATTRIBUTE_IMPURE_DATA__ _impure_data = _REENT_INIT
> (_impure_data);
>
> I guess you don't need the _REENT_INIT_PTR_ZEROED (_GLOBAL_REENT).
You're right, and more: Given that impure.c also sets _impure_ptr to
&_impure_data statically, setting _impure_ptr to _GLOBAL_REENT is
redundant as well. Nice.
Thanks,
Corinna
On 13/05/2022 10:03, Corinna Vinschen wrote: > Hi Sebastian, Hi Matt, > > On May 12 21:14, Corinna Vinschen wrote: >> Hi Matt, >> >> On May 12 14:11, Matthew Joyce wrote: >>> From: Matt Joyce <matthew.joyce@embedded-brains.de> >>> >>> Hello Corinna, >>> >>> Per your comments, please see version 2. >>> >>> 1) stdio_atexit is now called stdio_exit_handler >>> 2) declarations for stdio_exit_handler() and _fwalk_sglue() are moved >>> to sys/reent.h >> >> Please see my comments in terms of patch 11, which needs an additional >> tweak. With this change, the patchset is GTG. > > I forgot to mention... you don't have to send a v3, just push the > patchset with this additional tweak (and talk about it in the commit > message). Thanks for the review. I checked it in with hopefully the right additional tweaks. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.huber@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/
On May 13 13:20, Sebastian Huber wrote:
> On 13/05/2022 10:03, Corinna Vinschen wrote:
> > Hi Sebastian, Hi Matt,
> >
> > On May 12 21:14, Corinna Vinschen wrote:
> > > Hi Matt,
> > >
> > > On May 12 14:11, Matthew Joyce wrote:
> > > > From: Matt Joyce <matthew.joyce@embedded-brains.de>
> > > >
> > > > Hello Corinna,
> > > >
> > > > Per your comments, please see version 2.
> > > >
> > > > 1) stdio_atexit is now called stdio_exit_handler
> > > > 2) declarations for stdio_exit_handler() and _fwalk_sglue() are moved
> > > > to sys/reent.h
> > >
> > > Please see my comments in terms of patch 11, which needs an additional
> > > tweak. With this change, the patchset is GTG.
> >
> > I forgot to mention... you don't have to send a v3, just push the
> > patchset with this additional tweak (and talk about it in the commit
> > message).
>
> Thanks for the review. I checked it in with hopefully the right additional
> tweaks.
LGTM.
Thanks,
Corinna
On Thu, 12 May 2022 14:11:40 +0200 Matthew Joyce wrote: > From: Matt Joyce <matthew.joyce@embedded-brains.de> > > Added a dedicated stdio exit handler to avoid using _GLOBAL_REENT > in exit(). > --- > newlib/libc/include/sys/reent.h | 2 ++ > newlib/libc/stdio/findfp.c | 11 ++++++++++- > newlib/libc/stdlib/exit.c | 5 +++-- > winsup/cygwin/signal.cc | 5 +++-- > 4 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h > index 4b0d68610..067294dc7 100644 > --- a/newlib/libc/include/sys/reent.h > +++ b/newlib/libc/include/sys/reent.h > @@ -837,6 +837,8 @@ extern struct _reent *_impure_ptr __ATTRIBUTE_IMPURE_PTR__; > > extern struct _reent _impure_data __ATTRIBUTE_IMPURE_DATA__; > > +extern void (*__stdio_exit_handler) (void); > + > void _reclaim_reent (struct _reent *); > > /* #define _REENT_ONLY define this to get only reentrant routines */ > diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c > index 5b0402ac1..719886e27 100644 > --- a/newlib/libc/stdio/findfp.c > +++ b/newlib/libc/stdio/findfp.c > @@ -26,6 +26,8 @@ > #include <sys/lock.h> > #include "local.h" > > +void (*__stdio_exit_handler) (void); > + > #if defined(_REENT_SMALL) && !defined(_REENT_GLOBAL_STDIO_STREAMS) > const struct __sFILE_fake __sf_fake_stdin = > {_NULL, 0, 0, 0, 0, {_NULL, 0}, 0, _NULL}; > @@ -153,6 +155,12 @@ sfmoreglue (struct _reent *d, int n) > return &g->glue; > } > > +static void > +stdio_exit_handler (void) > +{ > + (void) _fwalk_reent (_GLOBAL_REENT, CLEANUP_FILE); > +} > + > /* > * Find a free FILE for fopen et al. > */ > @@ -166,12 +174,13 @@ __sfp (struct _reent *d) > > _newlib_sfp_lock_start (); > > - if (_GLOBAL_REENT->__cleanup == NULL) { > + if (__stdio_exit_handler == NULL) { > #ifdef _REENT_GLOBAL_STDIO_STREAMS > _GLOBAL_REENT->__sglue._niobs = 3; > _GLOBAL_REENT->__sglue._iobs = &__sf[0]; > #endif > __sinit (_GLOBAL_REENT); > + __stdio_exit_handler = stdio_exit_handler; > } > > for (g = &_GLOBAL_REENT->__sglue;; g = g->_next) > diff --git a/newlib/libc/stdlib/exit.c b/newlib/libc/stdlib/exit.c > index 3e618914e..9b7bd518b 100644 > --- a/newlib/libc/stdlib/exit.c > +++ b/newlib/libc/stdlib/exit.c > @@ -59,7 +59,8 @@ exit (int code) > #endif > __call_exitprocs (code, NULL); > > - if (_GLOBAL_REENT->__cleanup) > - (*_GLOBAL_REENT->__cleanup) (_GLOBAL_REENT); > + if (__stdio_exit_handler != NULL) > + (*__stdio_exit_handler) (); > + > _exit (code); > } > diff --git a/winsup/cygwin/signal.cc b/winsup/cygwin/signal.cc > index 9b6c2509d..f5f2efdc7 100644 > --- a/winsup/cygwin/signal.cc > +++ b/winsup/cygwin/signal.cc > @@ -13,6 +13,7 @@ details. */ > #include <stdlib.h> > #include <sys/cygwin.h> > #include <sys/signalfd.h> > +#include <sys/reent.h> /* needed for __stdio_exit_handler declaration */ > #include "pinfo.h" > #include "sigproc.h" > #include "cygtls.h" > @@ -408,8 +409,8 @@ abort (void) > _my_tls.call_signal_handler (); /* Call any signal handler */ > > /* Flush all streams as per SUSv2. */ > - if (_GLOBAL_REENT->__cleanup) > - _GLOBAL_REENT->__cleanup (_GLOBAL_REENT); > + if (__stdio_exit_handler != NULL) > + (*__stdio_exit_handler) (); > do_exit (SIGABRT); /* signal handler didn't exit. Goodbye. */ > } > > -- > 2.31.1 > After this commit, "ps | cat" outputs nothing in cygwin. However, just "ps" and "ls | cat" works. "stdbuf -o 0 ps | cat" also works. I am not sure this is the right thing, however, I found the following patch solves the issue. It seems that initializing __stdio_exit_handler is missing. diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c index 19952d4e0..e759b5402 100644 --- a/newlib/libc/stdio/findfp.c +++ b/newlib/libc/stdio/findfp.c @@ -63,6 +63,8 @@ struct _glue __sglue = {NULL, 3, &_GLOBAL_REENT->__sf[0]}; #endif #endif +static void stdio_exit_handler (void); + #if (defined (__OPTIMIZE_SIZE__) || defined (PREFER_SIZE_OVER_SPEED)) _NOINLINE_STATIC void #else @@ -109,6 +111,11 @@ std (FILE *ptr, if (__stextmode (ptr->_file)) ptr->_flags |= __SCLE; #endif + + if (__stdio_exit_handler == NULL) { + __sinit (_GLOBAL_REENT); + __stdio_exit_handler = stdio_exit_handler; + } } static inline void Could you please have a look? -- Takashi Yano <takashi.yano@nifty.ne.jp>
On 17/05/2022 10:36, Takashi Yano wrote: [...] > After this commit, "ps | cat" outputs nothing in cygwin. However, > just "ps" and "ls | cat" works. "stdbuf -o 0 ps | cat" also works. > > I am not sure this is the right thing, however, I found the following > patch solves the issue. It seems that initializing __stdio_exit_handler > is missing. > > diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c > index 19952d4e0..e759b5402 100644 > --- a/newlib/libc/stdio/findfp.c > +++ b/newlib/libc/stdio/findfp.c > @@ -63,6 +63,8 @@ struct _glue __sglue = {NULL, 3, &_GLOBAL_REENT->__sf[0]}; > #endif > #endif > > +static void stdio_exit_handler (void); > + > #if (defined (__OPTIMIZE_SIZE__) || defined (PREFER_SIZE_OVER_SPEED)) > _NOINLINE_STATIC void > #else > @@ -109,6 +111,11 @@ std (FILE *ptr, > if (__stextmode (ptr->_file)) > ptr->_flags |= __SCLE; > #endif > + > + if (__stdio_exit_handler == NULL) { > + __sinit (_GLOBAL_REENT); > + __stdio_exit_handler = stdio_exit_handler; > + } > } > > static inline void > > Could you please have a look? Could you please check if this patch fixes the issue: https://sourceware.org/pipermail/newlib/2022/019713.html -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.huber@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/