[PATCH v3 4/5] Enable _REENT_GLOBAL_STDIO_STREAMS for RTEMS

[Sebastian Huber]

Signed-off-by: Sebastian Huber <***@embedded-brains.de>
---
newlib/libc/include/sys/config.h | 1 +
newlib/libc/stdio/local.h | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/newlib/libc/include/sys/config.h b/newlib/libc/include/sys/config.h
index 555239f8b..ae8caff7b 100644
--- a/newlib/libc/include/sys/config.h
+++ b/newlib/libc/include/sys/config.h
@@ -238,6 +238,7 @@
#define _READ_WRITE_RETURN_TYPE _ssize_t
#define __DYNAMIC_REENT__
#define _REENT_GLOBAL_ATEXIT
+#define _REENT_GLOBAL_STDIO_STREAMS
#endif

#ifndef __EXPORT
diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h
index 5f6995501..511e5e35f 100644
--- a/newlib/libc/stdio/local.h
+++ b/newlib/libc/stdio/local.h
@@ -38,7 +38,7 @@
case _STDIO_CLOSE_PER_REENT_STD_STREAMS is defined these file descriptors
will be closed via close() provided the owner of the reent structure
triggerd the on demand reent initilization, see CHECK_INIT(). */
-#if !defined(__rtems__) && !defined(__tirtos__)
+#if !defined(__tirtos__)
#define _STDIO_CLOSE_PER_REENT_STD_STREAMS
#endif

[Sebastian Huber]

Signed-off-by: Sebastian Huber <***@embedded-brains.de>
---
newlib/README | 6 ++++++
newlib/configure | 24 ++++++++++++++++++++++--
newlib/configure.in | 13 +++++++++++++
newlib/libc/include/sys/config.h | 6 ++++++
newlib/newlib.hin | 5 +++++
5 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/newlib/README b/newlib/README
index ed8fb3540..78f4de846 100644
--- a/newlib/README
+++ b/newlib/README
@@ -297,6 +297,12 @@ One feature can be enabled by specifying `--enable-FEATURE=yes' or
is not referenced.
Disabled by default.

+`--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 b/newlib/configure
index de28c25b3..b2f0b3340 100755
--- a/newlib/configure
+++ b/newlib/configure
@@ -795,6 +795,7 @@ enable_newlib_iconv_external_ccs
enable_newlib_atexit_dynamic_alloc
enable_newlib_global_atexit
enable_newlib_reent_small
+enable_newlib_global_stdio_streams
enable_newlib_fvwrite_in_streamio
enable_newlib_fseek_optimization
enable_newlib_wide_orient
@@ -1467,6 +1468,7 @@ Optional Features:
--disable-newlib-atexit-dynamic-alloc disable dynamic allocation of atexit entries
--enable-newlib-global-atexit enable atexit data structure as global
--enable-newlib-reent-small enable small reentrant struct support
+ --enable-newlib-global-stdio-streams enable global stdio streams
--disable-newlib-fvwrite-in-streamio disable iov in streamio
--disable-newlib-fseek-optimization disable fseek optimization
--disable-newlib-wide-orient Turn off wide orientation in streamio
@@ -2387,6 +2389,17 @@ else
newlib_reent_small=
fi

+# Check whether --enable-newlib-global-stdio-streams was given.
+if test "${enable_newlib_global_stdio_streams+set}" = set; then :
+ enableval=$enable_newlib_global_stdio_streams; case "${enableval}" in
+ yes) newlib_global_stdio_streams=yes;;
+ no) newlib_global_stdio_streams=no ;;
+ *) as_fn_error $? "bad value ${enableval} for newlib-global-stdio-streams option" "$LINENO" 5 ;;
+ esac
+else
+ newlib_global_stdio_streams=
+fi
+
# Check whether --enable-newlib-fvwrite-in-streamio was given.
if test "${enable_newlib_fvwrite_in_streamio+set}" = set; then :
enableval=$enable_newlib_fvwrite_in_streamio; if test "${newlib_fvwrite_in_streamio+set}" != set; then
@@ -11794,7 +11807,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11797 "configure"
+#line 11810 "configure"
#include "confdefs.h"

#if HAVE_DLFCN_H
@@ -11900,7 +11913,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11903 "configure"
+#line 11916 "configure"
#include "confdefs.h"

#if HAVE_DLFCN_H
@@ -12347,6 +12360,13 @@ _ACEOF

fi

+if test "${newlib_global_stdio_streams}" = "yes"; then
+cat >>confdefs.h <<_ACEOF
+#define _WANT_REENT_GLOBAL_STDIO_STREAMS 1
+_ACEOF
+
+fi
+
if test "${newlib_mb}" = "yes"; then
cat >>confdefs.h <<_ACEOF
#define _MB_CAPABLE 1
diff --git a/newlib/configure.in b/newlib/configure.in
index 354c07ca5..5b86ee800 100644
--- a/newlib/configure.in
+++ b/newlib/configure.in
@@ -136,6 +136,15 @@ AC_ARG_ENABLE(newlib-reent-small,
no) newlib_reent_small=no ;;
*) AC_MSG_ERROR(bad value ${enableval} for newlib-reent-small option) ;;
esac], [newlib_reent_small=])dnl
+
+dnl Support --enable-newlib-global-stdio-streams
+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

dnl Support --disable-newlib-fvwrite-in-streamio
AC_ARG_ENABLE(newlib-fvwrite-in-streamio,
@@ -400,6 +409,10 @@ if test "${newlib_reent_small}" = "yes"; then
AC_DEFINE_UNQUOTED(_WANT_REENT_SMALL)
fi

+if test "${newlib_global_stdio_streams}" = "yes"; then
+AC_DEFINE_UNQUOTED(_WANT_REENT_GLOBAL_STDIO_STREAMS)
+fi
+
if test "${newlib_mb}" = "yes"; then
AC_DEFINE_UNQUOTED(_MB_CAPABLE)
AC_DEFINE_UNQUOTED(_MB_LEN_MAX,8)
diff --git a/newlib/libc/include/sys/config.h b/newlib/libc/include/sys/config.h
index ae8caff7b..e45aa5417 100644
--- a/newlib/libc/include/sys/config.h
+++ b/newlib/libc/include/sys/config.h
@@ -276,6 +276,12 @@
#endif
#endif

+#ifdef _WANT_REENT_GLOBAL_STDIO_STREAMS
+#ifndef _REENT_GLOBAL_STDIO_STREAMS
+#define _REENT_GLOBAL_STDIO_STREAMS
+#endif
+#endif
+
/* If _MB_EXTENDED_CHARSETS_ALL is set, we want all of the extended
charsets. The extended charsets add a few functions and a couple
of tables of a few K each. */
diff --git a/newlib/newlib.hin b/newlib/newlib.hin
index 397bc9b96..45c683187 100644
--- a/newlib/newlib.hin
+++ b/newlib/newlib.hin
@@ -79,6 +79,11 @@
/* Define if declare atexit data as global. */
#undef _REENT_GLOBAL_ATEXIT

+/* 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. */
+#undef _WANT_REENT_GLOBAL_STDIO_STREAMS
+
/* Define if small footprint nano-formatted-IO implementation used. */
#undef _NANO_FORMATTED_IO

[Sebastian Huber]

This simplifies further changes in this area.

Signed-off-by: Sebastian Huber <***@embedded-brains.de>
---
newlib/libc/stdio/findfp.c | 50 ++++++++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
index ecc65d6d3..b40aa9240 100644
--- a/newlib/libc/stdio/findfp.c
+++ b/newlib/libc/stdio/findfp.c
@@ -84,6 +84,36 @@ _DEFUN(std, (ptr, flags, file),
#endif
}

+static inline void
+stdin_init(FILE *ptr)
+{
+ std (ptr, __SRD, 0);
+}
+
+static inline void
+stdout_init(FILE *ptr)
+{
+ /* On platforms that have true file system I/O, we can verify
+ whether stdout is an interactive terminal or not, as part of
+ __smakebuf on first use of the stream. For all other platforms,
+ we will default to line buffered mode here. Technically, POSIX
+ requires both stdin and stdout to be line-buffered, but tradition
+ leaves stdin alone on systems without fcntl. */
+#ifdef HAVE_FCNTL
+ std (ptr, __SWR, 1);
+#else
+ std (ptr, __SWR | __SLBF, 1);
+#endif
+}
+
+static inline void
+stderr_init(FILE *ptr)
+{
+ /* POSIX requires stderr to be opened for reading and writing, even
+ when the underlying fd 2 is write-only. */
+ std (ptr, __SRW | __SNBF, 2);
+}
+
struct glue_with_file {
struct _glue glue;
FILE file;
@@ -235,23 +265,9 @@ _DEFUN(__sinit, (s),
s->_stderr = __sfp(s);
#endif

- std (s->_stdin, __SRD, 0);
-
- /* On platforms that have true file system I/O, we can verify
- whether stdout is an interactive terminal or not, as part of
- __smakebuf on first use of the stream. For all other platforms,
- we will default to line buffered mode here. Technically, POSIX
- requires both stdin and stdout to be line-buffered, but tradition
- leaves stdin alone on systems without fcntl. */
-#ifdef HAVE_FCNTL
- std (s->_stdout, __SWR, 1);
-#else
- std (s->_stdout, __SWR | __SLBF, 1);
-#endif
-
- /* POSIX requires stderr to be opened for reading and writing, even
- when the underlying fd 2 is write-only. */
- std (s->_stderr, __SRW | __SNBF, 2);
+ stdin_init (s->_stdin);
+ stdout_init (s->_stdout);
+ stderr_init (s->_stderr);

s->__sdidinit = 1;

[Sebastian Huber]

In Newlib, the stdio streams are defined to thread-specific pointers
_reent::_stdin, _reent::_stdout and _reent::_stderr. In case
_REENT_SMALL is not defined, then these pointers are initialized via
_REENT_INIT_PTR() or _REENT_INIT_PTR_ZEROED() to thread-specific FILE
objects provided via _reent::__sf[3]. There are two problems with this
(at least in case of RTEMS).

(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 (e.g. 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().

Introduce a new Newlib configuration option _REENT_GLOBAL_STDIO_STREAMS
to enable the use of global stdio FILE objects.

As a side-effect this reduces the size of struct _reent by more than
50%.

The _REENT_GLOBAL_STDIO_STREAMS should not be used without
_STDIO_CLOSE_PER_REENT_STD_STREAMS.

Signed-off-by: Sebastian Huber <***@embedded-brains.de>
---
newlib/libc/include/sys/reent.h | 21 +++++++++++++++------
newlib/libc/stdio/findfp.c | 24 ++++++++++++++++++++++++
2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index 8b67889ac..c045ca549 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -644,14 +644,23 @@ struct _reent
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
__FILE __sf[3]; /* first three file descriptors */
+# endif
};

+#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]
+#endif
+
#define _REENT_INIT(var) \
{ 0, \
- &(var).__sf[0], \
- &(var).__sf[1], \
- &(var).__sf[2], \
+ _REENT_STDIO_STREAM(&(var), 0), \
+ _REENT_STDIO_STREAM(&(var), 1), \
+ _REENT_STDIO_STREAM(&(var), 2), \
0, \
"", \
0, \
@@ -696,9 +705,9 @@ struct _reent
}

#define _REENT_INIT_PTR_ZEROED(var) \
- { (var)->_stdin = &(var)->__sf[0]; \
- (var)->_stdout = &(var)->__sf[1]; \
- (var)->_stderr = &(var)->__sf[2]; \
+ { (var)->_stdin = _REENT_STDIO_STREAM(var, 0); \
+ (var)->_stdout = _REENT_STDIO_STREAM(var, 1); \
+ (var)->_stderr = _REENT_STDIO_STREAM(var, 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; \
diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
index b40aa9240..737bde102 100644
--- a/newlib/libc/stdio/findfp.c
+++ b/newlib/libc/stdio/findfp.c
@@ -35,6 +35,10 @@ 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];
+#endif
+
#if (defined (__OPTIMIZE_SIZE__) || defined (PREFER_SIZE_OVER_SPEED))
_NOINLINE_STATIC _VOID
#else
@@ -218,6 +222,14 @@ _DEFUN(_cleanup_r, (ptr),
cleanup_func = _fclose_r;
#endif
#endif
+#ifdef _REENT_GLOBAL_STDIO_STREAMS
+ if (ptr->_stdin != &__sf[0])
+ (*cleanup_func) (ptr, ptr->_stdin);
+ if (ptr->_stdout != &__sf[1])
+ (*cleanup_func) (ptr, ptr->_stdout);
+ if (ptr->_stderr != &__sf[2])
+ (*cleanup_func) (ptr, ptr->_stderr);
+#endif
_CAST_VOID _fwalk_reent (ptr, cleanup_func);
}

@@ -250,8 +262,10 @@ _DEFUN(__sinit, (s),

s->__sglue._next = NULL;
#ifndef _REENT_SMALL
+# ifndef _REENT_GLOBAL_STDIO_STREAMS
s->__sglue._niobs = 3;
s->__sglue._iobs = &s->__sf[0];
+# endif
#else
s->__sglue._niobs = 0;
s->__sglue._iobs = NULL;
@@ -265,9 +279,19 @@ _DEFUN(__sinit, (s),
s->_stderr = __sfp(s);
#endif

+#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]);
+ }
+#else
stdin_init (s->_stdin);
stdout_init (s->_stdout);
stderr_init (s->_stderr);
+#endif

s->__sdidinit = 1;

[Corinna Vinschen]

Post by Sebastian Huber
In Newlib, the stdio streams are defined to thread-specific pointers
_reent::_stdin, _reent::_stdout and _reent::_stderr. In case
_REENT_SMALL is not defined, then these pointers are initialized via
_REENT_INIT_PTR() or _REENT_INIT_PTR_ZEROED() to thread-specific FILE
objects provided via _reent::__sf[3]. There are two problems with this
(at least in case of RTEMS).
(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 (e.g. 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().
Introduce a new Newlib configuration option _REENT_GLOBAL_STDIO_STREAMS
to enable the use of global stdio FILE objects.
As a side-effect this reduces the size of struct _reent by more than
50%.
The _REENT_GLOBAL_STDIO_STREAMS should not be used without
_STDIO_CLOSE_PER_REENT_STD_STREAMS.

Patch series looks good, builds and works on Cygwin, so I think this
is good to go. Please push.

And, JFYI, Cygwin will start to use it too after the next release :)


Thanks,
Corinna

[Sebastian Huber]

And, JFYI, Cygwin will start to use it too after the next release:)

There are some new problems with this change. We have no reference
counting in the FILE objects, so a freopen(..., stdin), closes the
global stdin FILE object (__sf[0]), etc. What works is a stdin =
fopen(). I guess this could break existing applications.
--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : [phone]
Fax : [phone]
E-Mail : ***@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

[Corinna Vinschen]

And, JFYI, Cygwin will start to use it too after the next release:)

There are some new problems with this change. We have no reference counting
in the FILE objects, so a freopen(..., stdin), closes the global stdin FILE
object (__sf[0]), etc. What works is a stdin = fopen(). I guess this could
break existing applications.

Erm... isn't that expected behaviour? stdin/stdout/stderr are global
objects, after all. They were never thread-local per POSIX.


Corinna

[Sebastian Huber]

Post by Corinna Vinschen

And, JFYI, Cygwin will start to use it too after the next release:)

There are some new problems with this change. We have no reference counting
in the FILE objects, so a freopen(..., stdin), closes the global stdin FILE
object (__sf[0]), etc. What works is a stdin = fopen(). I guess this could
break existing applications.

Erm... isn't that expected behaviour? stdin/stdout/stderr are global
objects, after all. They were never thread-local per POSIX.

The stdin/stdout/stderr pointers are still thread-local with this option. Only the FILE objects itself are now global. If you do a stdin = fopen() you get a completely thread-local stdin. I have absolutely no idea why this stuff is thread-local in Newlib by default.

[Corinna Vinschen]

Post by Sebastian Huber

Post by Corinna Vinschen

And, JFYI, Cygwin will start to use it too after the next release:)

There are some new problems with this change. We have no reference counting
in the FILE objects, so a freopen(..., stdin), closes the global stdin FILE
object (__sf[0]), etc. What works is a stdin = fopen(). I guess this could
break existing applications.

Erm... isn't that expected behaviour? stdin/stdout/stderr are global
objects, after all. They were never thread-local per POSIX.

The stdin/stdout/stderr pointers are still thread-local with this
option. Only the FILE objects itself are now global. If you do a stdin
= fopen() you get a completely thread-local stdin. I have absolutely
no idea why this stuff is thread-local in Newlib by default.

Me neither. This predates my involvement in newlib.

I guess it sounded like a good idea at the time, to allow multiple,
independent applets on bare metal systems.


Corinna

[Freddie Chopin]

----- Am 30. Jun 2017 um 10:13 schrieb Corinna Vinschen

Post by Corinna Vinschen

Post by Sebastian Huber

And, JFYI, Cygwin will start to use it too after the next release:)

There are some new problems with this change. We have no
reference counting
in the FILE objects, so a freopen(..., stdin), closes the global stdin FILE
object (__sf[0]), etc. What works is a stdin = fopen(). I guess this could
break existing applications.

Erm... isn't that expected behaviour? stdin/stdout/stderr are global
objects, after all. They were never thread-local per POSIX.

The stdin/stdout/stderr pointers are still thread-local with this
option. Only the FILE objects itself are now global. If you do a
stdin = fopen() you get a completely thread-local stdin. I have
absolutely no idea why this stuff is thread-local in Newlib by
default.

Would that be possible, to make stdin/stdout/stderr completely global
(remove them from _reent, use global objects) when
_REENT_GLOBAL_STDIO_STREAMS is enabled?

Regards,
FCh

[Sebastian Huber]

Post by Freddie Chopin

----- Am 30. Jun 2017 um 10:13 schrieb Corinna Vinschen

Post by Corinna Vinschen

Post by Sebastian Huber

Post by Corinna Vinschen
And, JFYI, Cygwin will start to use it too after the next
release:)

There are some new problems with this change. We have no
reference counting
in the FILE objects, so a freopen(..., stdin), closes the global stdin FILE
object (__sf[0]), etc. What works is a stdin = fopen(). I guess this could
break existing applications.

Erm... isn't that expected behaviour? stdin/stdout/stderr are global
objects, after all. They were never thread-local per POSIX.

The stdin/stdout/stderr pointers are still thread-local with this
option. Only the FILE objects itself are now global. If you do a
stdin = fopen() you get a completely thread-local stdin. I have
absolutely no idea why this stuff is thread-local in Newlib by
default.

Would that be possible, to make stdin/stdout/stderr completely global
(remove them from _reent, use global objects) when
_REENT_GLOBAL_STDIO_STREAMS is enabled?

The thread-local IO streams are a Newlib feature that is used by our
applications.
--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : [phone]
Fax : [phone]
E-Mail : ***@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

[Freddie Chopin]

Post by Sebastian Huber
The thread-local IO streams are a Newlib feature that is used by our
applications.

Don't get me wrong, but I would expect an option with "global" in the
name to make the streams really global, not "sort-of-global" (;
Otherwise it looks like a half-baked-hack to fix just some particular
problem instead of looking at the whole picture.

Regards,
FCh

[Sebastian Huber]

Post by Freddie Chopin

Post by Sebastian Huber
The thread-local IO streams are a Newlib feature that is used by our
applications.

Don't get me wrong, but I would expect an option with "global" in the
name to make the streams really global, not "sort-of-global" (;
Otherwise it looks like a half-baked-hack to fix just some particular
problem instead of looking at the whole picture.

There are two things involved here. The FILE objects and pointers to
FILE objects. The stdio, etc. are pointers. These pointers are
thread-local. The FILE objects are global and are used for the initial
values of these thread-local pointers. If you want to get rid of the
thread-local pointers, then you probably need a new configuration option.
--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : [phone]
Fax : [phone]
E-Mail : ***@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.