public inbox for newlib@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] Use global stdio streams for all configurations
@ 2022-06-08  9:43 Sebastian Huber
  2022-06-10 10:45 ` Corinna Vinschen
  0 siblings, 1 reply; 5+ messages in thread
From: Sebastian Huber @ 2022-06-08  9:43 UTC (permalink / raw)
  To: newlib

The _REENT_GLOBAL_STDIO_STREAMS was introduced by commit
668a4c8722090fffd10869dbb15b879651c1370d in 2017.  Since then it was enabled by
default for RTEMS.  Recently, the option was enabled for Cygwin which
previously used an alternative implementation to use global stdio streams.

In Newlib, the stdio streams are defined to thread-specific pointers
_reent::_stdin, _reent::_stdout and _reent::_stderr.  If the option is disabled
(the default for most systems), then these pointers are initialized to
thread-specific FILE objects which use file descriptors 0, 1, and 2,
respectively.  There are at least three problems with this:

(1) The thread-specific FILE objects are closed by _reclaim_reent().  This
    leads to problems with language run-time libraries that provide wrappers to
    the C/POSIX stdio streams (for example C++ and Ada), since they use the
    thread-specific FILE objects of the initialization thread.  In case the
    initialization thread is deleted, then they use freed memory.

(2) Since thread-specific FILE objects are used with a common output device via
    file descriptors 0, 1 and 2, the locking at FILE object level cannot ensure
    atomicity of the output, e.g. a call to printf().

(3) There are resource managment issues, see:

    https://sourceware.org/pipermail/newlib/2022/019558.html

    https://bugs.linaro.org/show_bug.cgi?id=5841

This patch enables the _REENT_GLOBAL_STDIO_STREAMS behaviour for all Newlib
configurations and removes the option.  This removes a couple of #ifdef blocks.
---
v2:

* Add missing ';' to _REENT_INIT_PTR_ZEROED().

* Rebase to Newlib master.

 newlib/README                         |   6 --
 newlib/configure.ac                   |  10 +--
 newlib/libc/include/sys/config.h      |   7 --
 newlib/libc/include/sys/reent.h       | 118 +++-----------------------
 newlib/libc/reent/impure.c            |   6 --
 newlib/libc/reent/reent.c             |  20 -----
 newlib/libc/stdio/fcloseall.c         |   4 -
 newlib/libc/stdio/findfp.c            |  53 +-----------
 newlib/libc/stdio/local.h             |  17 ----
 winsup/cygwin/include/cygwin/config.h |   1 -
 10 files changed, 14 insertions(+), 228 deletions(-)

diff --git a/newlib/README b/newlib/README
index f00e66019..dd4686eeb 100644
--- a/newlib/README
+++ b/newlib/README
@@ -294,12 +294,6 @@ One feature can be enabled by specifying `--enable-FEATURE=yes' or
      Disable dynamic allocation of atexit entries.
      Most hosts and targets have it enabled in configure.host.
 
-`--enable-newlib-global-stdio-streams'
-     Enable to move the stdio stream FILE objects out of struct _reent and make
-     them global.  The stdio stream pointers of struct _reent are initialized
-     to point to the global stdio FILE stream objects.
-     Disabled by default.
-
 `--enable-newlib-reent-small'
      Enable small reentrant struct support.
      Disabled by default.
diff --git a/newlib/configure.ac b/newlib/configure.ac
index 57f830960..1951aab0c 100644
--- a/newlib/configure.ac
+++ b/newlib/configure.ac
@@ -174,13 +174,13 @@ AC_ARG_ENABLE(newlib-reent-binary-compat,
  fi], [newlib_reent_binary_compat=no])dnl
 
 dnl Support --enable-newlib-global-stdio-streams
+dnl This is no longer optional.  It is enabled in all Newlib configurations.
 AC_ARG_ENABLE(newlib-global-stdio-streams,
 [  --enable-newlib-global-stdio-streams   enable global stdio streams],
 [case "${enableval}" in
   yes) newlib_global_stdio_streams=yes;;
-  no)  newlib_global_stdio_streams=no ;;
   *)   AC_MSG_ERROR(bad value ${enableval} for newlib-global-stdio-streams option) ;;
- esac], [newlib_global_stdio_streams=])dnl
+ esac], [newlib_global_stdio_streams=yes])dnl
  
 dnl Support --disable-newlib-fvwrite-in-streamio
 AC_ARG_ENABLE(newlib-fvwrite-in-streamio,
@@ -435,12 +435,6 @@ if test "${newlib_reent_binary_compat}" = "yes"; then
   AC_DEFINE(_WANT_REENT_BACKWARD_BINARY_COMPAT, 1, [Define to enable backward binary compatibility for struct _reent.])
 fi
 
-if test "${newlib_global_stdio_streams}" = "yes"; then
-  AC_DEFINE(_WANT_REENT_GLOBAL_STDIO_STREAMS, 1,
-	    [Define to move the stdio stream FILE objects out of struct _reent and make them global.
-	     The stdio stream pointers of struct _reent are initialized to point to the global stdio FILE stream objects.])
-fi
-
 _mb_len_max=1
 if test "${newlib_mb}" = "yes"; then
   AC_DEFINE(_MB_CAPABLE, 1, [Multibyte supported.])
diff --git a/newlib/libc/include/sys/config.h b/newlib/libc/include/sys/config.h
index c40bb51c2..4bce10de3 100644
--- a/newlib/libc/include/sys/config.h
+++ b/newlib/libc/include/sys/config.h
@@ -242,7 +242,6 @@
 #define __FILENAME_MAX__ 255
 #define _READ_WRITE_RETURN_TYPE _ssize_t
 #define __DYNAMIC_REENT__
-#define _REENT_GLOBAL_STDIO_STREAMS
 #endif
 
 #ifndef __EXPORT
@@ -280,12 +279,6 @@
 #endif
 #endif
 
-#ifdef _WANT_REENT_GLOBAL_STDIO_STREAMS
-#ifndef _REENT_GLOBAL_STDIO_STREAMS
-#define _REENT_GLOBAL_STDIO_STREAMS
-#endif
-#endif
-
 #ifdef _WANT_USE_LONG_TIME_T
 #ifndef _USE_LONG_TIME_T
 #define _USE_LONG_TIME_T
diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index 64d76c27c..a900dc0fc 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -142,39 +142,7 @@ struct __sbuf {
  * _ub._base!=NULL) and _up and _ur save the current values of _p and _r.
  */
 
-#if defined(_REENT_SMALL) && !defined(_REENT_GLOBAL_STDIO_STREAMS)
-/*
- * struct __sFILE_fake is the start of a struct __sFILE, with only the
- * minimal fields allocated.  In __sinit() we really allocate the 3
- * standard streams, etc., and point away from this fake.
- */
-struct __sFILE_fake {
-  unsigned char *_p;	/* current position in (some) buffer */
-  int	_r;		/* read space left for getc() */
-  int	_w;		/* write space left for putc() */
-  short	_flags;		/* flags, below; this FILE is free if 0 */
-  short	_file;		/* fileno, if Unix descriptor, else -1 */
-  struct __sbuf _bf;	/* the buffer (at least 1 byte, if !NULL) */
-  int	_lbfsize;	/* 0 or -_bf._size, for inline putc */
-
-  struct _reent *_data;
-};
-
-/* Following is needed both in libc/stdio and libc/stdlib so we put it
- * here instead of libc/stdio/local.h where it was previously. */
-
-extern void   __sinit (struct _reent *);
-
-# define _REENT_SMALL_CHECK_INIT(ptr)		\
-  do						\
-    {						\
-      if ((ptr) && !(ptr)->__cleanup)		\
-	__sinit (ptr);				\
-    }						\
-  while (0)
-#else /* _REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS */
-# define _REENT_SMALL_CHECK_INIT(ptr) /* nothing */
-#endif /* _REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS */
+#define _REENT_SMALL_CHECK_INIT(ptr) /* nothing */
 
 struct __sFILE {
   unsigned char *_p;	/* current position in (some) buffer */
@@ -286,9 +254,7 @@ typedef struct __sFILE   __FILE;
 #endif /* __LARGE64_FILES */
 #endif /* !__CUSTOM_FILE_IO__ */
 
-#ifdef _REENT_GLOBAL_STDIO_STREAMS
 extern __FILE __sf[3];
-#endif
 
 struct _glue
 {
@@ -340,11 +306,13 @@ struct _rand48 {
 #define _REENT_INIT_RESERVED_1 0,
 #define _REENT_INIT_RESERVED_2 0,
 #define _REENT_INIT_RESERVED_6_7 _NULL, _ATEXIT_INIT,
+#define _REENT_INIT_RESERVED_8 {_NULL, 0, _NULL},
 #else
 #define _REENT_INIT_RESERVED_0 /* Nothing to initialize */
 #define _REENT_INIT_RESERVED_1 /* Nothing to initialize */
 #define _REENT_INIT_RESERVED_2 /* Nothing to initialize */
 #define _REENT_INIT_RESERVED_6_7 /* Nothing to initialize */
+#define _REENT_INIT_RESERVED_8 /* Nothing to initialize */
 #endif
 
 /*
@@ -426,16 +394,14 @@ struct _reent
 #ifdef _REENT_BACKWARD_BINARY_COMPAT
   struct _atexit *_reserved_6;
   struct _atexit _reserved_7;
+  struct _glue _reserved_8;
 #endif
 
-  struct _glue __sglue;			/* root of glue chain */
   __FILE *__sf;			        /* file descriptors */
   struct _misc_reent *_misc;            /* strtok, multibyte states */
   char *_signal_buf;                    /* strsignal */
 };
 
-#ifdef _REENT_GLOBAL_STDIO_STREAMS
-
 # define _REENT_INIT(var) \
   { 0, \
     &__sf[0], \
@@ -456,7 +422,7 @@ struct _reent
     _NULL, \
     _NULL, \
     _REENT_INIT_RESERVED_6_7 \
-    {_NULL, 0, _NULL}, \
+    _REENT_INIT_RESERVED_8 \
     _NULL, \
     _NULL, \
     _NULL \
@@ -468,46 +434,6 @@ struct _reent
     (var)->_stderr = &__sf[2]; \
   }
 
-#else /* _REENT_GLOBAL_STDIO_STREAMS */
-
-extern const struct __sFILE_fake __sf_fake_stdin;
-extern const struct __sFILE_fake __sf_fake_stdout;
-extern const struct __sFILE_fake __sf_fake_stderr;
-
-# define _REENT_INIT(var) \
-  { 0, \
-    (__FILE *)&__sf_fake_stdin, \
-    (__FILE *)&__sf_fake_stdout, \
-    (__FILE *)&__sf_fake_stderr, \
-    0, \
-    _NULL, \
-    _REENT_INIT_RESERVED_0 \
-    _REENT_INIT_RESERVED_1 \
-    _NULL, \
-    _NULL, \
-    _NULL, \
-    0, \
-    0, \
-    _NULL, \
-    _NULL, \
-    _NULL, \
-    _NULL, \
-    _NULL, \
-    _REENT_INIT_RESERVED_6_7 \
-    {_NULL, 0, _NULL}, \
-    _NULL, \
-    _NULL, \
-    _NULL \
-  }
-
-#define _REENT_INIT_PTR_ZEROED(var) \
-  { (var)->_stdin = (__FILE *)&__sf_fake_stdin; \
-    (var)->_stdout = (__FILE *)&__sf_fake_stdout; \
-    (var)->_stderr = (__FILE *)&__sf_fake_stderr; \
-  }
-
-#endif /* _REENT_GLOBAL_STDIO_STREAMS */
-
 /* Specify how to handle reent_check malloc failures. */
 #ifdef _REENT_CHECK_VERIFY
 #include <assert.h>
@@ -695,33 +621,13 @@ struct _reent
 
   /* signal info */
   void (**_sig_func)(int);
-
-  /* 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).  */
-# 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) /* 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_ZEROED(_ptr) \
-  (_ptr)->__sglue._niobs = 3; \
-  (_ptr)->__sglue._iobs = &(_ptr)->__sf[0];
-#endif
-
 #define _REENT_INIT(var) \
   { 0, \
-    _REENT_STDIO_STREAM(&(var), 0), \
-    _REENT_STDIO_STREAM(&(var), 1), \
-    _REENT_STDIO_STREAM(&(var), 2), \
+    &__sf[0], \
+    &__sf[1], \
+    &__sf[2], \
     0, \
     "", \
     _REENT_INIT_RESERVED_1 \
@@ -762,13 +668,12 @@ struct _reent
     }, \
     _REENT_INIT_RESERVED_6_7 \
     _NULL \
-    _REENT_INIT_SGLUE(&(var)) \
   }
 
 #define _REENT_INIT_PTR_ZEROED(var) \
-  { (var)->_stdin = _REENT_STDIO_STREAM(var, 0); \
-    (var)->_stdout = _REENT_STDIO_STREAM(var, 1); \
-    (var)->_stderr = _REENT_STDIO_STREAM(var, 2); \
+  { (var)->_stdin = &__sf[0]; \
+    (var)->_stdout = &__sf[1]; \
+    (var)->_stderr = &__sf[2]; \
     (var)->_new._reent._rand_next = 1; \
     (var)->_new._reent._r48._seed[0] = _RAND48_SEED_0; \
     (var)->_new._reent._r48._seed[1] = _RAND48_SEED_1; \
@@ -777,7 +682,6 @@ 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/reent/impure.c b/newlib/libc/reent/impure.c
index 643a511c6..9290c9cd5 100644
--- a/newlib/libc/reent/impure.c
+++ b/newlib/libc/reent/impure.c
@@ -6,13 +6,7 @@
    important to reduce image size for targets with very small amounts
    of memory.  */
 #ifdef _REENT_SMALL
-#ifdef _REENT_GLOBAL_STDIO_STREAMS
 extern __FILE __sf[3] _ATTRIBUTE ((weak));
-#else
-extern const struct __sFILE_fake __sf_fake_stdin _ATTRIBUTE ((weak));
-extern const struct __sFILE_fake __sf_fake_stdout _ATTRIBUTE ((weak));
-extern const struct __sFILE_fake __sf_fake_stderr _ATTRIBUTE ((weak));
-#endif
 #endif
 
 struct _reent __ATTRIBUTE_IMPURE_DATA__ _impure_data = _REENT_INIT (_impure_data);
diff --git a/newlib/libc/reent/reent.c b/newlib/libc/reent/reent.c
index 04942ce4d..d61415901 100644
--- a/newlib/libc/reent/reent.c
+++ b/newlib/libc/reent/reent.c
@@ -27,21 +27,6 @@ int errno;
 
 #endif
 
-#ifndef _REENT_GLOBAL_STDIO_STREAMS
-/* Interim cleanup code */
-
-static void
-cleanup_glue (struct _reent *ptr,
-     struct _glue *glue)
-{
-  /* Have to reclaim these in reverse order: */
-  if (glue->_next)
-    cleanup_glue (ptr, glue->_next);
-
-  _free_r (ptr, glue);
-}
-#endif
-
 void
 _reclaim_reent (struct _reent *ptr)
 {
@@ -106,11 +91,6 @@ _reclaim_reent (struct _reent *ptr)
 	  /* cleanup won't reclaim memory 'coz usually it's run
 	     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 642dc7d94..e840af27c 100644
--- a/newlib/libc/stdio/fcloseall.c
+++ b/newlib/libc/stdio/fcloseall.c
@@ -59,12 +59,8 @@ 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 858c09ee3..ee991ed24 100644
--- a/newlib/libc/stdio/findfp.c
+++ b/newlib/libc/stdio/findfp.c
@@ -28,21 +28,9 @@
 
 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};
-const struct __sFILE_fake __sf_fake_stdout =
-    {_NULL, 0, 0, 0, 0, {_NULL, 0}, 0, _NULL};
-const struct __sFILE_fake __sf_fake_stderr =
-    {_NULL, 0, 0, 0, 0, {_NULL, 0}, 0, _NULL};
-#endif
-
-#ifdef _REENT_GLOBAL_STDIO_STREAMS
 __FILE __sf[3];
+
 struct _glue __sglue = {NULL, 3, &__sf[0]};
-#else
-struct _glue __sglue = {NULL, 0, NULL};
-#endif
 
 #ifdef _STDIO_BSD_SEMANTICS
   /* BSD and Glibc systems only flush streams which have been written to
@@ -161,11 +149,9 @@ global_stdio_init (void)
 {
   if (__stdio_exit_handler == NULL) {
     __stdio_exit_handler = stdio_exit_handler;
-#ifdef _REENT_GLOBAL_STDIO_STREAMS
     stdin_init (&__sf[0]);
     stdout_init (&__sf[1]);
     stderr_init (&__sf[2]);
-#endif
   }
 }
 
@@ -232,16 +218,12 @@ found:
 static void
 cleanup_stdio (struct _reent *ptr)
 {
-#ifdef _REENT_GLOBAL_STDIO_STREAMS
   if (ptr->_stdin != &__sf[0])
     CLEANUP_FILE (ptr, ptr->_stdin);
   if (ptr->_stdout != &__sf[1])
     CLEANUP_FILE (ptr, ptr->_stdout);
   if (ptr->_stderr != &__sf[2])
     CLEANUP_FILE (ptr, ptr->_stderr);
-#else
-  (void) _fwalk_sglue (ptr, CLEANUP_FILE, &ptr->__sglue);
-#endif
 }
 
 /*
@@ -262,22 +244,7 @@ __sinit (struct _reent *s)
   /* make sure we clean up on exit */
   s->__cleanup = cleanup_stdio;	/* conservative */
 
-#ifdef _REENT_SMALL
-# ifndef _REENT_GLOBAL_STDIO_STREAMS
-  s->_stdin = __sfp(s);
-  s->_stdout = __sfp(s);
-  s->_stderr = __sfp(s);
-# endif /* _REENT_GLOBAL_STDIO_STREAMS */
-#endif
-
   global_stdio_init ();
-
-#ifndef _REENT_GLOBAL_STDIO_STREAMS
-  stdin_init (s->_stdin);
-  stdout_init (s->_stdout);
-  stderr_init (s->_stderr);
-#endif /* _REENT_GLOBAL_STDIO_STREAMS */
-
   __sfp_lock_release ();
 }
 
@@ -320,32 +287,14 @@ __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);
-#else
   (void) _fwalk_sglue (NULL, __fp_lock, &__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);
-#else
   (void) _fwalk_sglue (NULL, __fp_unlock, &__sglue);
-#endif
-
   __sfp_lock_release ();
 }
 #endif
diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h
index e245fdb4e..9b355e3ac 100644
--- a/newlib/libc/stdio/local.h
+++ b/newlib/libc/stdio/local.h
@@ -193,31 +193,14 @@ extern _READ_WRITE_RETURN_TYPE __swrite64 (struct _reent *, void *,
 
 /* Called by the main entry point fns to ensure stdio has been initialized.  */
 
-#if defined(_REENT_SMALL) && !defined(_REENT_GLOBAL_STDIO_STREAMS)
 #define CHECK_INIT(ptr, fp) \
   do								\
     {								\
       struct _reent *_check_init_ptr = (ptr);			\
       if ((_check_init_ptr) && !(_check_init_ptr)->__cleanup)	\
 	__sinit (_check_init_ptr);				\
-      if ((fp) == (FILE *)&__sf_fake_stdin)			\
-	(fp) = _stdin_r(_check_init_ptr);			\
-      else if ((fp) == (FILE *)&__sf_fake_stdout)		\
-	(fp) = _stdout_r(_check_init_ptr);			\
-      else if ((fp) == (FILE *)&__sf_fake_stderr)		\
-	(fp) = _stderr_r(_check_init_ptr);			\
     }								\
   while (0)
-#else /* !_REENT_SMALL || _REENT_GLOBAL_STDIO_STREAMS */
-#define CHECK_INIT(ptr, fp) \
-  do								\
-    {								\
-      struct _reent *_check_init_ptr = (ptr);			\
-      if ((_check_init_ptr) && !(_check_init_ptr)->__cleanup)	\
-	__sinit (_check_init_ptr);				\
-    }								\
-  while (0)
-#endif /* !_REENT_SMALL || _REENT_GLOBAL_STDIO_STREAMS */
 
 /* Return true and set errno and stream error flag iff the given FILE
    cannot be written now.  */
diff --git a/winsup/cygwin/include/cygwin/config.h b/winsup/cygwin/include/cygwin/config.h
index fd3093755..f6d1b68f0 100644
--- a/winsup/cygwin/include/cygwin/config.h
+++ b/winsup/cygwin/include/cygwin/config.h
@@ -48,7 +48,6 @@ 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
-- 
2.35.3


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Use global stdio streams for all configurations
  2022-06-08  9:43 [PATCH v2] Use global stdio streams for all configurations Sebastian Huber
@ 2022-06-10 10:45 ` Corinna Vinschen
  2022-06-10 11:41   ` Sebastian Huber
  2022-06-17 11:33   ` Volodymyr Medvid
  0 siblings, 2 replies; 5+ messages in thread
From: Corinna Vinschen @ 2022-06-10 10:45 UTC (permalink / raw)
  To: newlib

On Jun  8 11:43, Sebastian Huber wrote:
> The _REENT_GLOBAL_STDIO_STREAMS was introduced by commit
> 668a4c8722090fffd10869dbb15b879651c1370d in 2017.  Since then it was enabled by
> default for RTEMS.  Recently, the option was enabled for Cygwin which
> previously used an alternative implementation to use global stdio streams.
> 
> In Newlib, the stdio streams are defined to thread-specific pointers
> _reent::_stdin, _reent::_stdout and _reent::_stderr.  If the option is disabled
> (the default for most systems), then these pointers are initialized to
> thread-specific FILE objects which use file descriptors 0, 1, and 2,
> respectively.  There are at least three problems with this:
> 
> (1) The thread-specific FILE objects are closed by _reclaim_reent().  This
>     leads to problems with language run-time libraries that provide wrappers to
>     the C/POSIX stdio streams (for example C++ and Ada), since they use the
>     thread-specific FILE objects of the initialization thread.  In case the
>     initialization thread is deleted, then they use freed memory.
> 
> (2) Since thread-specific FILE objects are used with a common output device via
>     file descriptors 0, 1 and 2, the locking at FILE object level cannot ensure
>     atomicity of the output, e.g. a call to printf().
> 
> (3) There are resource managment issues, see:
> 
>     https://sourceware.org/pipermail/newlib/2022/019558.html
> 
>     https://bugs.linaro.org/show_bug.cgi?id=5841
> 
> This patch enables the _REENT_GLOBAL_STDIO_STREAMS behaviour for all Newlib
> configurations and removes the option.  This removes a couple of #ifdef blocks.
> ---
> v2:
> 
> * Add missing ';' to _REENT_INIT_PTR_ZEROED().
> 
> * Rebase to Newlib master.
> 
>  newlib/README                         |   6 --
>  newlib/configure.ac                   |  10 +--
>  newlib/libc/include/sys/config.h      |   7 --
>  newlib/libc/include/sys/reent.h       | 118 +++-----------------------
>  newlib/libc/reent/impure.c            |   6 --
>  newlib/libc/reent/reent.c             |  20 -----
>  newlib/libc/stdio/fcloseall.c         |   4 -
>  newlib/libc/stdio/findfp.c            |  53 +-----------
>  newlib/libc/stdio/local.h             |  17 ----
>  winsup/cygwin/include/cygwin/config.h |   1 -
>  10 files changed, 14 insertions(+), 228 deletions(-)

LGTM together with your "Fix __fp_lock_all() and __fp_unlock_all()" patch.

Thx,
Corinna


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Use global stdio streams for all configurations
  2022-06-10 10:45 ` Corinna Vinschen
@ 2022-06-10 11:41   ` Sebastian Huber
  2022-06-10 11:52     ` Corinna Vinschen
  2022-06-17 11:33   ` Volodymyr Medvid
  1 sibling, 1 reply; 5+ messages in thread
From: Sebastian Huber @ 2022-06-10 11:41 UTC (permalink / raw)
  To: newlib

On 10.06.22 12:45, Corinna Vinschen wrote:
> On Jun  8 11:43, Sebastian Huber wrote:
>> The _REENT_GLOBAL_STDIO_STREAMS was introduced by commit
>> 668a4c8722090fffd10869dbb15b879651c1370d in 2017.  Since then it was enabled by
>> default for RTEMS.  Recently, the option was enabled for Cygwin which
>> previously used an alternative implementation to use global stdio streams.
>>
>> In Newlib, the stdio streams are defined to thread-specific pointers
>> _reent::_stdin, _reent::_stdout and _reent::_stderr.  If the option is disabled
>> (the default for most systems), then these pointers are initialized to
>> thread-specific FILE objects which use file descriptors 0, 1, and 2,
>> respectively.  There are at least three problems with this:
>>
>> (1) The thread-specific FILE objects are closed by _reclaim_reent().  This
>>      leads to problems with language run-time libraries that provide wrappers to
>>      the C/POSIX stdio streams (for example C++ and Ada), since they use the
>>      thread-specific FILE objects of the initialization thread.  In case the
>>      initialization thread is deleted, then they use freed memory.
>>
>> (2) Since thread-specific FILE objects are used with a common output device via
>>      file descriptors 0, 1 and 2, the locking at FILE object level cannot ensure
>>      atomicity of the output, e.g. a call to printf().
>>
>> (3) There are resource managment issues, see:
>>
>>      https://sourceware.org/pipermail/newlib/2022/019558.html
>>
>>      https://bugs.linaro.org/show_bug.cgi?id=5841
>>
>> This patch enables the _REENT_GLOBAL_STDIO_STREAMS behaviour for all Newlib
>> configurations and removes the option.  This removes a couple of #ifdef blocks.
>> ---
>> v2:
>>
>> * Add missing ';' to _REENT_INIT_PTR_ZEROED().
>>
>> * Rebase to Newlib master.
>>
>>   newlib/README                         |   6 --
>>   newlib/configure.ac                   |  10 +--
>>   newlib/libc/include/sys/config.h      |   7 --
>>   newlib/libc/include/sys/reent.h       | 118 +++-----------------------
>>   newlib/libc/reent/impure.c            |   6 --
>>   newlib/libc/reent/reent.c             |  20 -----
>>   newlib/libc/stdio/fcloseall.c         |   4 -
>>   newlib/libc/stdio/findfp.c            |  53 +-----------
>>   newlib/libc/stdio/local.h             |  17 ----
>>   winsup/cygwin/include/cygwin/config.h |   1 -
>>   10 files changed, 14 insertions(+), 228 deletions(-)
> LGTM together with your "Fix __fp_lock_all() and __fp_unlock_all()" patch.

Thanks for having a look at it. I am on holidays next week. Should I 
still check it in today?

-- 
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/

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Use global stdio streams for all configurations
  2022-06-10 11:41   ` Sebastian Huber
@ 2022-06-10 11:52     ` Corinna Vinschen
  0 siblings, 0 replies; 5+ messages in thread
From: Corinna Vinschen @ 2022-06-10 11:52 UTC (permalink / raw)
  To: newlib

On Jun 10 13:41, Sebastian Huber wrote:
> On 10.06.22 12:45, Corinna Vinschen wrote:
> > On Jun  8 11:43, Sebastian Huber wrote:
> > > The _REENT_GLOBAL_STDIO_STREAMS was introduced by commit
> > > 668a4c8722090fffd10869dbb15b879651c1370d in 2017.  Since then it was enabled by
> > > [...]
> > >   newlib/README                         |   6 --
> > >   newlib/configure.ac                   |  10 +--
> > >   newlib/libc/include/sys/config.h      |   7 --
> > >   newlib/libc/include/sys/reent.h       | 118 +++-----------------------
> > >   newlib/libc/reent/impure.c            |   6 --
> > >   newlib/libc/reent/reent.c             |  20 -----
> > >   newlib/libc/stdio/fcloseall.c         |   4 -
> > >   newlib/libc/stdio/findfp.c            |  53 +-----------
> > >   newlib/libc/stdio/local.h             |  17 ----
> > >   winsup/cygwin/include/cygwin/config.h |   1 -
> > >   10 files changed, 14 insertions(+), 228 deletions(-)
> > LGTM together with your "Fix __fp_lock_all() and __fp_unlock_all()" patch.
> 
> Thanks for having a look at it. I am on holidays next week. Should I still
> check it in today?

Sure, no worries.

Thx,
Corinna


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Use global stdio streams for all configurations
  2022-06-10 10:45 ` Corinna Vinschen
  2022-06-10 11:41   ` Sebastian Huber
@ 2022-06-17 11:33   ` Volodymyr Medvid
  1 sibling, 0 replies; 5+ messages in thread
From: Volodymyr Medvid @ 2022-06-17 11:33 UTC (permalink / raw)
  To: newlib

On Fri, Jun 10, 2022 at 1:45 PM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> On Jun  8 11:43, Sebastian Huber wrote:
> > The _REENT_GLOBAL_STDIO_STREAMS was introduced by commit
> > 668a4c8722090fffd10869dbb15b879651c1370d in 2017.  Since then it was enabled by
> > default for RTEMS.  Recently, the option was enabled for Cygwin which
> > previously used an alternative implementation to use global stdio streams.
> >
> > In Newlib, the stdio streams are defined to thread-specific pointers
> > _reent::_stdin, _reent::_stdout and _reent::_stderr.  If the option is disabled
> > (the default for most systems), then these pointers are initialized to
> > thread-specific FILE objects which use file descriptors 0, 1, and 2,
> > respectively.  There are at least three problems with this:
> >
> > (1) The thread-specific FILE objects are closed by _reclaim_reent().  This
> >     leads to problems with language run-time libraries that provide wrappers to
> >     the C/POSIX stdio streams (for example C++ and Ada), since they use the
> >     thread-specific FILE objects of the initialization thread.  In case the
> >     initialization thread is deleted, then they use freed memory.
> >
> > (2) Since thread-specific FILE objects are used with a common output device via
> >     file descriptors 0, 1 and 2, the locking at FILE object level cannot ensure
> >     atomicity of the output, e.g. a call to printf().
> >
> > (3) There are resource managment issues, see:
> >
> >     https://sourceware.org/pipermail/newlib/2022/019558.html
> >
> >     https://bugs.linaro.org/show_bug.cgi?id=5841
> >
> > This patch enables the _REENT_GLOBAL_STDIO_STREAMS behaviour for all Newlib
> > configurations and removes the option.  This removes a couple of #ifdef blocks.
> > ---
> > v2:
> >
> > * Add missing ';' to _REENT_INIT_PTR_ZEROED().
> >
> > * Rebase to Newlib master.
> >
> >  newlib/README                         |   6 --
> >  newlib/configure.ac                   |  10 +--
> >  newlib/libc/include/sys/config.h      |   7 --
> >  newlib/libc/include/sys/reent.h       | 118 +++-----------------------
> >  newlib/libc/reent/impure.c            |   6 --
> >  newlib/libc/reent/reent.c             |  20 -----
> >  newlib/libc/stdio/fcloseall.c         |   4 -
> >  newlib/libc/stdio/findfp.c            |  53 +-----------
> >  newlib/libc/stdio/local.h             |  17 ----
> >  winsup/cygwin/include/cygwin/config.h |   1 -
> >  10 files changed, 14 insertions(+), 228 deletions(-)
>
> LGTM together with your "Fix __fp_lock_all() and __fp_unlock_all()" patch.
>
> Thx,
> Corinna
>

Hi, I confirmed the memory leak reported as
https://sourceware.org/pipermail/newlib/2022/019558.html and
https://bugs.linaro.org/show_bug.cgi?id=5841 is no longer observed
with this patch - many thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-06-17 11:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-08  9:43 [PATCH v2] Use global stdio streams for all configurations Sebastian Huber
2022-06-10 10:45 ` Corinna Vinschen
2022-06-10 11:41   ` Sebastian Huber
2022-06-10 11:52     ` Corinna Vinschen
2022-06-17 11:33   ` Volodymyr Medvid

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).