public inbox for newlib@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix stdio init handling
@ 2022-06-06 18:20 Hans-Peter Nilsson
  2022-06-06 20:16 ` Jeff Johnston
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Hans-Peter Nilsson @ 2022-06-06 18:20 UTC (permalink / raw)
  To: newlib

Ok to commit?

----- 8< -----
After commit e826fbb2ae88 "Fix stdio exit handling", for trivial
newlib using targets such as cris-elf and arm-eabihf, i.e.
(!_REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS && !cygwin) stdio
initialization no longer happens properly.  At that commit and
after, programs like the following are broken for such targets;
at the first opened file, the first FILE record seems
pre-allocated, unused and free, and is returned as the "FILE *"
at the fopen.  At the subsequent fgetc, it's *initialized* as
stdin and the fgetc returns EOF (without errno), yielding
"fgetc-EOF: Success" and the program aborted instead of the
expected "all-ok".

===============
/* There must exist a file "./fff" with the first byte an 'f'.  */
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
int main(void)
{
  int e = (errno = 0);
  FILE *f = fopen("fff", "r");
  int c = fgetc(f);

  if (f == NULL)
    {
      perror("fopen");
      abort();
    }

  if (c == EOF)
    {
      perror("fgetc-EOF");
      abort();
    }

  if (c != 'f')
    {
      perror("fgetc-not-f");
      abort();
    }

  if (fclose (f) != 0)
    {
      perror("fclose");
      abort();
    }

  puts("all-ok");
  return 0;
}
===============

But the problem started already with commit 26747c47bc0a "Add
stdio_exit_handler()" which made a half-way transition from
guarding initialization from _GLOBAL_REENT->__cleanup == NULL
and stdio initialization in __sinit, to __stdio_exit_handler ==
NULL and global_stdio_init.  The solution is to advance that
transition, making __sinit just a lock-accounting wrapper for
global_stdio_init.  Both "guards" are now initialized, allowing
either _GLOBAL_REENT->__cleanup or __stdio_exit_handler to be
checked and any other struct _reent should work too.

I'm also fixing what appears to be a deadlock problem:
consider what happens *before* the patch, with the "sfp
lock" for _REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS when
__sinit is called and global_stdio_init hasn't been called.

An alternative would be to revert 26747c47bc0a and depending
follow-up commits and move back to __sinit and
_GLOBAL_REENT->__cleanup.  (I don't really see *why* it's
important to avoid using _GLOBAL_REENT in exit() and the
conversation in the email archives doesn't tell me.)

Tested cris-elf, fixing the test-case as well as the gcc
testsuite regressions I noticed when attempting to update
newlib from 01c734b0d7c1 to e93bb6f9852a for my cris-elf
autotester.

Caveats and observations:

I'm not touching newlib/libc/machine/spu/stdio.c:__sinit,
which also wasn't updated for that patchset, and even before
that it also appears to not handle neither
_REENT_GLOBAL_STDIO_STREAMS nor _REENT_SMALL.  To wit, I
suspect it's broken since that time.

Perhaps the comment in reent.h should also be updated;
struct _reent doesn't contain all state (not
__stdio_exit_handler), at least since 26747c47bc0a but also
since the introduction of __atexit and __atexit0.

There's (still) redundancy between stdio_exit_handler and
cleanup_stdio.

Whomever calls __sinit or __sfp first, better do it with
_GLOBAL_REENT; there's confusion whenever the struct _reent
argument is something else.

There are cleanup opportunities wrt. the "sfp lock", to make
__sinit and __sfp use "names" at the same level, avoiding
confusion.

	* libc/stdio/findfp.c (__sfp_nolock): New function with the contents
	of __sfp.
	(__sfp): Keep just locking parts.
	(global_stdio_init): Add struct _reent parameter.  Include the
	remaining contents of __sinit but call __sfp_nolock instead of __sfp.
---
 newlib/libc/stdio/findfp.c | 67 ++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
index 6933ff1dbcef..ff112167a7ad 100644
--- a/newlib/libc/stdio/findfp.c
+++ b/newlib/libc/stdio/findfp.c
@@ -160,8 +160,11 @@ stdio_exit_handler (void)
   (void) _fwalk_sglue (_GLOBAL_REENT, CLEANUP_FILE, &__sglue);
 }
 
+static FILE *__sfp_nolock (struct _reent *);
+static void cleanup_stdio (struct _reent *);
+
 static void
-global_stdio_init (void)
+global_stdio_init (struct _reent *s)
 {
   if (__stdio_exit_handler == NULL) {
     __stdio_exit_handler = stdio_exit_handler;
@@ -171,22 +174,38 @@ global_stdio_init (void)
     stderr_init (&__sf[2]);
 #endif
   }
+
+  if (s->__cleanup == NULL) {
+    /* Make sure we clean up on exit.  */
+    s->__cleanup = cleanup_stdio;
+
+#ifdef _REENT_SMALL
+# ifndef _REENT_GLOBAL_STDIO_STREAMS
+    s->_stdin = __sfp_nolock(s);
+    s->_stdout = __sfp_nolock(s);
+    s->_stderr = __sfp_nolock(s);
+# endif /* _REENT_GLOBAL_STDIO_STREAMS */
+#endif
+
+#ifndef _REENT_GLOBAL_STDIO_STREAMS
+    stdin_init (s->_stdin);
+    stdout_init (s->_stdout);
+    stderr_init (s->_stderr);
+#endif /* _REENT_GLOBAL_STDIO_STREAMS */
+  }
 }
 
 /*
  * Find a free FILE for fopen et al.
  */
 
-FILE *
-__sfp (struct _reent *d)
+static FILE *
+__sfp_nolock (struct _reent *d)
 {
   FILE *fp;
   int n;
   struct _glue *g;
 
-  _newlib_sfp_lock_start ();
-  global_stdio_init ();
-
   for (g = &__sglue;; g = g->_next)
     {
       for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
@@ -196,7 +215,6 @@ __sfp (struct _reent *d)
 	  (g->_next = sfmoreglue (d, NDYNAMIC)) == NULL)
 	break;
     }
-  _newlib_sfp_lock_exit ();
   d->_errno = ENOMEM;
   return NULL;
 
@@ -207,7 +225,6 @@ found:
 #ifndef __SINGLE_THREAD__
   __lock_init_recursive (fp->_lock);
 #endif
-  _newlib_sfp_lock_end ();
 
   fp->_p = NULL;		/* no current pointer */
   fp->_w = 0;			/* nothing to read or write */
@@ -225,6 +242,19 @@ found:
   return fp;
 }
 
+FILE *
+__sfp (struct _reent *d)
+{
+  FILE *fp;
+
+  _newlib_sfp_lock_start ();
+  global_stdio_init (d);
+  fp = __sfp_nolock (d);
+  _newlib_sfp_lock_end ();
+
+  return fp;
+}
+
 /*
  * exit() calls _cleanup() through *__cleanup, set whenever we
  * open or buffer a file.  This chicanery is done so that programs
@@ -262,26 +292,7 @@ __sinit (struct _reent *s)
       __sfp_lock_release ();
       return;
     }
-
-  /* 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 */
-
+  global_stdio_init (s);
   __sfp_lock_release ();
 }
 
-- 
2.30.2


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

* Re: [PATCH] Fix stdio init handling
  2022-06-06 18:20 [PATCH] Fix stdio init handling Hans-Peter Nilsson
@ 2022-06-06 20:16 ` Jeff Johnston
  2022-06-07  6:21   ` Sebastian Huber
  2022-06-07  6:05 ` Torbjorn SVENSSON
  2022-06-07  6:18 ` Sebastian Huber
  2 siblings, 1 reply; 9+ messages in thread
From: Jeff Johnston @ 2022-06-06 20:16 UTC (permalink / raw)
  To: Hans-Peter Nilsson; +Cc: Newlib, Sebastian Huber

Thanks Han-Peter,

I'm inclined to allow Sebastian a chance to fix this in line with his
overall design.  Sebastian, do you have a fix for this?  If you are
absent, we can merge this patch and you can modify later.  BTW: Sebastian,
I had given you permission to commit your last set of patches if
you had accounted for Corinna's existing comments, but I haven't seen
anything.

-- Jeff J.


On Mon, Jun 6, 2022 at 2:20 PM Hans-Peter Nilsson <hp@axis.com> wrote:

> Ok to commit?
>
> ----- 8< -----
> After commit e826fbb2ae88 "Fix stdio exit handling", for trivial
> newlib using targets such as cris-elf and arm-eabihf, i.e.
> (!_REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS && !cygwin) stdio
> initialization no longer happens properly.  At that commit and
> after, programs like the following are broken for such targets;
> at the first opened file, the first FILE record seems
> pre-allocated, unused and free, and is returned as the "FILE *"
> at the fopen.  At the subsequent fgetc, it's *initialized* as
> stdin and the fgetc returns EOF (without errno), yielding
> "fgetc-EOF: Success" and the program aborted instead of the
> expected "all-ok".
>
> ===============
> /* There must exist a file "./fff" with the first byte an 'f'.  */
> #include <stdio.h>
> #include <errno.h>
> #include <stdlib.h>
> int main(void)
> {
>   int e = (errno = 0);
>   FILE *f = fopen("fff", "r");
>   int c = fgetc(f);
>
>   if (f == NULL)
>     {
>       perror("fopen");
>       abort();
>     }
>
>   if (c == EOF)
>     {
>       perror("fgetc-EOF");
>       abort();
>     }
>
>   if (c != 'f')
>     {
>       perror("fgetc-not-f");
>       abort();
>     }
>
>   if (fclose (f) != 0)
>     {
>       perror("fclose");
>       abort();
>     }
>
>   puts("all-ok");
>   return 0;
> }
> ===============
>
> But the problem started already with commit 26747c47bc0a "Add
> stdio_exit_handler()" which made a half-way transition from
> guarding initialization from _GLOBAL_REENT->__cleanup == NULL
> and stdio initialization in __sinit, to __stdio_exit_handler ==
> NULL and global_stdio_init.  The solution is to advance that
> transition, making __sinit just a lock-accounting wrapper for
> global_stdio_init.  Both "guards" are now initialized, allowing
> either _GLOBAL_REENT->__cleanup or __stdio_exit_handler to be
> checked and any other struct _reent should work too.
>
> I'm also fixing what appears to be a deadlock problem:
> consider what happens *before* the patch, with the "sfp
> lock" for _REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS when
> __sinit is called and global_stdio_init hasn't been called.
>
> An alternative would be to revert 26747c47bc0a and depending
> follow-up commits and move back to __sinit and
> _GLOBAL_REENT->__cleanup.  (I don't really see *why* it's
> important to avoid using _GLOBAL_REENT in exit() and the
> conversation in the email archives doesn't tell me.)
>
> Tested cris-elf, fixing the test-case as well as the gcc
> testsuite regressions I noticed when attempting to update
> newlib from 01c734b0d7c1 to e93bb6f9852a for my cris-elf
> autotester.
>
> Caveats and observations:
>
> I'm not touching newlib/libc/machine/spu/stdio.c:__sinit,
> which also wasn't updated for that patchset, and even before
> that it also appears to not handle neither
> _REENT_GLOBAL_STDIO_STREAMS nor _REENT_SMALL.  To wit, I
> suspect it's broken since that time.
>
> Perhaps the comment in reent.h should also be updated;
> struct _reent doesn't contain all state (not
> __stdio_exit_handler), at least since 26747c47bc0a but also
> since the introduction of __atexit and __atexit0.
>
> There's (still) redundancy between stdio_exit_handler and
> cleanup_stdio.
>
> Whomever calls __sinit or __sfp first, better do it with
> _GLOBAL_REENT; there's confusion whenever the struct _reent
> argument is something else.
>
> There are cleanup opportunities wrt. the "sfp lock", to make
> __sinit and __sfp use "names" at the same level, avoiding
> confusion.
>
>         * libc/stdio/findfp.c (__sfp_nolock): New function with the
> contents
>         of __sfp.
>         (__sfp): Keep just locking parts.
>         (global_stdio_init): Add struct _reent parameter.  Include the
>         remaining contents of __sinit but call __sfp_nolock instead of
> __sfp.
> ---
>  newlib/libc/stdio/findfp.c | 67 ++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
> index 6933ff1dbcef..ff112167a7ad 100644
> --- a/newlib/libc/stdio/findfp.c
> +++ b/newlib/libc/stdio/findfp.c
> @@ -160,8 +160,11 @@ stdio_exit_handler (void)
>    (void) _fwalk_sglue (_GLOBAL_REENT, CLEANUP_FILE, &__sglue);
>  }
>
> +static FILE *__sfp_nolock (struct _reent *);
> +static void cleanup_stdio (struct _reent *);
> +
>  static void
> -global_stdio_init (void)
> +global_stdio_init (struct _reent *s)
>  {
>    if (__stdio_exit_handler == NULL) {
>      __stdio_exit_handler = stdio_exit_handler;
> @@ -171,22 +174,38 @@ global_stdio_init (void)
>      stderr_init (&__sf[2]);
>  #endif
>    }
> +
> +  if (s->__cleanup == NULL) {
> +    /* Make sure we clean up on exit.  */
> +    s->__cleanup = cleanup_stdio;
> +
> +#ifdef _REENT_SMALL
> +# ifndef _REENT_GLOBAL_STDIO_STREAMS
> +    s->_stdin = __sfp_nolock(s);
> +    s->_stdout = __sfp_nolock(s);
> +    s->_stderr = __sfp_nolock(s);
> +# endif /* _REENT_GLOBAL_STDIO_STREAMS */
> +#endif
> +
> +#ifndef _REENT_GLOBAL_STDIO_STREAMS
> +    stdin_init (s->_stdin);
> +    stdout_init (s->_stdout);
> +    stderr_init (s->_stderr);
> +#endif /* _REENT_GLOBAL_STDIO_STREAMS */
> +  }
>  }
>
>  /*
>   * Find a free FILE for fopen et al.
>   */
>
> -FILE *
> -__sfp (struct _reent *d)
> +static FILE *
> +__sfp_nolock (struct _reent *d)
>  {
>    FILE *fp;
>    int n;
>    struct _glue *g;
>
> -  _newlib_sfp_lock_start ();
> -  global_stdio_init ();
> -
>    for (g = &__sglue;; g = g->_next)
>      {
>        for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
> @@ -196,7 +215,6 @@ __sfp (struct _reent *d)
>           (g->_next = sfmoreglue (d, NDYNAMIC)) == NULL)
>         break;
>      }
> -  _newlib_sfp_lock_exit ();
>    d->_errno = ENOMEM;
>    return NULL;
>
> @@ -207,7 +225,6 @@ found:
>  #ifndef __SINGLE_THREAD__
>    __lock_init_recursive (fp->_lock);
>  #endif
> -  _newlib_sfp_lock_end ();
>
>    fp->_p = NULL;               /* no current pointer */
>    fp->_w = 0;                  /* nothing to read or write */
> @@ -225,6 +242,19 @@ found:
>    return fp;
>  }
>
> +FILE *
> +__sfp (struct _reent *d)
> +{
> +  FILE *fp;
> +
> +  _newlib_sfp_lock_start ();
> +  global_stdio_init (d);
> +  fp = __sfp_nolock (d);
> +  _newlib_sfp_lock_end ();
> +
> +  return fp;
> +}
> +
>  /*
>   * exit() calls _cleanup() through *__cleanup, set whenever we
>   * open or buffer a file.  This chicanery is done so that programs
> @@ -262,26 +292,7 @@ __sinit (struct _reent *s)
>        __sfp_lock_release ();
>        return;
>      }
> -
> -  /* 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 */
> -
> +  global_stdio_init (s);
>    __sfp_lock_release ();
>  }
>
> --
> 2.30.2
>
>

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

* RE: [PATCH] Fix stdio init handling
  2022-06-06 18:20 [PATCH] Fix stdio init handling Hans-Peter Nilsson
  2022-06-06 20:16 ` Jeff Johnston
@ 2022-06-07  6:05 ` Torbjorn SVENSSON
  2022-06-07 14:40   ` Hans-Peter Nilsson
  2022-06-07  6:18 ` Sebastian Huber
  2 siblings, 1 reply; 9+ messages in thread
From: Torbjorn SVENSSON @ 2022-06-07  6:05 UTC (permalink / raw)
  To: Hans-Peter Nilsson, newlib



> -----Original Message-----
> From: Newlib <newlib-
> bounces+torbjorn.svensson=st.com@sourceware.org> On Behalf Of Hans-
> Peter Nilsson
> Sent: den 6 juni 2022 20:20
> To: newlib@sourceware.org
> Subject: [PATCH] Fix stdio init handling
> 
> Ok to commit?
> 
> ----- 8< -----
> After commit e826fbb2ae88 "Fix stdio exit handling", for trivial
> newlib using targets such as cris-elf and arm-eabihf, i.e.
> (!_REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS && !cygwin) stdio
> initialization no longer happens properly.  At that commit and
> after, programs like the following are broken for such targets;
> at the first opened file, the first FILE record seems
> pre-allocated, unused and free, and is returned as the "FILE *"
> at the fopen.  At the subsequent fgetc, it's *initialized* as
> stdin and the fgetc returns EOF (without errno), yielding
> "fgetc-EOF: Success" and the program aborted instead of the
> expected "all-ok".
> 
> ===============
> /* There must exist a file "./fff" with the first byte an 'f'.  */
> #include <stdio.h>
> #include <errno.h>
> #include <stdlib.h>
> int main(void)
> {
>   int e = (errno = 0);
>   FILE *f = fopen("fff", "r");
>   int c = fgetc(f);
> 
>   if (f == NULL)
>     {
>       perror("fopen");
>       abort();
>     }
> 

Unrelated to the patch proposed, but I think you should avoid calling fgetc(f) prior to checking if f is NULL or a valid pointer.

Kind regards,
Torbjörn

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

* Re: [PATCH] Fix stdio init handling
  2022-06-06 18:20 [PATCH] Fix stdio init handling Hans-Peter Nilsson
  2022-06-06 20:16 ` Jeff Johnston
  2022-06-07  6:05 ` Torbjorn SVENSSON
@ 2022-06-07  6:18 ` Sebastian Huber
  2022-06-07 19:31   ` Sebastian Huber
  2022-06-07 23:30   ` Hans-Peter Nilsson
  2 siblings, 2 replies; 9+ messages in thread
From: Sebastian Huber @ 2022-06-07  6:18 UTC (permalink / raw)
  To: Hans-Peter Nilsson, newlib

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

Hello Hans-Peter,

sorry for breaking your Newlib configuration. I was able to reproduce 
the issue in RTEMS with a corresponding configuration. The problem 
didn't show up in our tests since in RTEMS __getreent() returns a 
thread-specific reentrancy structure. With

struct _reent *
__getreent (void)
{
   return _impure_ptr;
}

your test case reproduced the issue. Could you please try the attached 
patch?

We would like to introduce a configuration option for Newlib to use 
thread-local storage for the members of struct _reent:

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

With this option, struct _reent is not defined, so using members of 
_GLOBAL_REENT to maintain global Newlib state would not work. This is 
why we tried to use dedicated global objects for the FILE object list 
and the exit handlers.

We also would like to make the _REENT_GLOBAL_STDIO_STREAMS option the 
default Newlib behaviour:

https://sourceware.org/pipermail/newlib/2022/019735.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/

[-- Attachment #2: 0001-Fix-__sglue-inititialization.patch --]
[-- Type: text/x-patch, Size: 991 bytes --]

From 9629ec0b7a2e657d693960269506fcc6c715c6ff Mon Sep 17 00:00:00 2001
From: Sebastian Huber <sebastian.huber@embedded-brains.de>
Date: Tue, 7 Jun 2022 07:55:02 +0200
Subject: [PATCH] Fix __sglue inititialization

Do not initialize __sglue with the FILE objects of _GLOBAL_REENT to avoid a
double use in the !_REENT_SMALL and !_REENT_GLOBAL_STDIO_STREAMS configurations
which didn't use a thread-specific reentrancy structure.
---
 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 6933ff1db..ff6804d19 100644
--- a/newlib/libc/stdio/findfp.c
+++ b/newlib/libc/stdio/findfp.c
@@ -41,11 +41,7 @@ const struct __sFILE_fake __sf_fake_stderr =
 __FILE __sf[3];
 struct _glue __sglue = {NULL, 3, &__sf[0]};
 #else
-#ifdef _REENT_SMALL
 struct _glue __sglue = {NULL, 0, NULL};
-#else
-struct _glue __sglue = {NULL, 3, &_GLOBAL_REENT->__sf[0]};
-#endif
 #endif
 
 #ifdef _STDIO_BSD_SEMANTICS
-- 
2.35.3


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

* Re: [PATCH] Fix stdio init handling
  2022-06-06 20:16 ` Jeff Johnston
@ 2022-06-07  6:21   ` Sebastian Huber
  2022-06-07 14:47     ` Jeff Johnston
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Huber @ 2022-06-07  6:21 UTC (permalink / raw)
  To: Jeff Johnston, Hans-Peter Nilsson; +Cc: Newlib

Hello Jeff,

On 06/06/2022 22:16, Jeff Johnston wrote:
> I'm inclined to allow Sebastian a chance to fix this in line with his 
> overall design.  Sebastian, do you have a fix for this?  

thanks for waiting with the commit.  I tried to fix it with an 
alternative patch.

> If you are
> absent, we can merge this patch and you can modify later.  BTW: 
> Sebastian, I had given you permission to commit your last set of patches if
> you had accounted for Corinna's existing comments, but I haven't seen 
> anything.

I think all our patches are integrated, except

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

which needs a closer look.

-- 
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] 9+ messages in thread

* Re: [PATCH] Fix stdio init handling
  2022-06-07  6:05 ` Torbjorn SVENSSON
@ 2022-06-07 14:40   ` Hans-Peter Nilsson
  0 siblings, 0 replies; 9+ messages in thread
From: Hans-Peter Nilsson @ 2022-06-07 14:40 UTC (permalink / raw)
  To: Torbjorn SVENSSON; +Cc: newlib

> From: Torbjorn SVENSSON <torbjorn.svensson@st.com>
> Date: Tue, 7 Jun 2022 08:05:01 +0200


> >   FILE *f = fopen("fff", "r");
> >   int c = fgetc(f);
> > 
> >   if (f == NULL)
> >     {
> >       perror("fopen");
> >       abort();
> >     }
> > 
> 
> Unrelated to the patch proposed, but I think you should
> avoid calling fgetc(f) prior to checking if f is NULL or a
> valid pointer.
> 
> Kind regards,
> Torbjörn

You're absolutely right, and I am fully aware.  I'll
clarify: this was only intended was a quick reproducer of
the problem, not a model program.

brgds, H-P

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

* Re: [PATCH] Fix stdio init handling
  2022-06-07  6:21   ` Sebastian Huber
@ 2022-06-07 14:47     ` Jeff Johnston
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff Johnston @ 2022-06-07 14:47 UTC (permalink / raw)
  To: Sebastian Huber; +Cc: Hans-Peter Nilsson, Newlib

On Tue, Jun 7, 2022 at 2:21 AM Sebastian Huber <
sebastian.huber@embedded-brains.de> wrote:

> Hello Jeff,
>
> On 06/06/2022 22:16, Jeff Johnston wrote:
> > I'm inclined to allow Sebastian a chance to fix this in line with his
> > overall design.  Sebastian, do you have a fix for this?
>
> thanks for waiting with the commit.  I tried to fix it with an
> alternative patch.
>
> > If you are
> > absent, we can merge this patch and you can modify later.  BTW:
> > Sebastian, I had given you permission to commit your last set of patches
> if
> > you had accounted for Corinna's existing comments, but I haven't seen
> > anything.
>
> I think all our patches are integrated, except
>
> https://sourceware.org/pipermail/newlib/2022/019735.html
>
> which needs a closer look.
>
>
LGTM.

-- Jeff J.

-- 
> 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] 9+ messages in thread

* Re: [PATCH] Fix stdio init handling
  2022-06-07  6:18 ` Sebastian Huber
@ 2022-06-07 19:31   ` Sebastian Huber
  2022-06-07 23:30   ` Hans-Peter Nilsson
  1 sibling, 0 replies; 9+ messages in thread
From: Sebastian Huber @ 2022-06-07 19:31 UTC (permalink / raw)
  To: Hans-Peter Nilsson, newlib

On 07/06/2022 08:18, Sebastian Huber wrote:
> Hello Hans-Peter,
> 
> sorry for breaking your Newlib configuration. I was able to reproduce 
> the issue in RTEMS with a corresponding configuration. The problem 
> didn't show up in our tests since in RTEMS __getreent() returns a 
> thread-specific reentrancy structure. With
> 
> struct _reent *
> __getreent (void)
> {
>    return _impure_ptr;
> }
> 
> your test case reproduced the issue. Could you please try the attached 
> patch?

I checked in my attempt to fix this issue:

https://sourceware.org/git/?p=newlib-cygwin.git;a=commit;h=14fc9be2349259ed4754457775ab4a14069395c5

Please let me know if you still have problems.

-- 
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] 9+ messages in thread

* Re: [PATCH] Fix stdio init handling
  2022-06-07  6:18 ` Sebastian Huber
  2022-06-07 19:31   ` Sebastian Huber
@ 2022-06-07 23:30   ` Hans-Peter Nilsson
  1 sibling, 0 replies; 9+ messages in thread
From: Hans-Peter Nilsson @ 2022-06-07 23:30 UTC (permalink / raw)
  To: Sebastian Huber; +Cc: newlib

> From: Sebastian Huber <sebastian.huber@embedded-brains.de>
> Date: Tue, 7 Jun 2022 08:18:49 +0200

> sorry for breaking your Newlib configuration.

(A point I tried to make was that it wasn't just "mine", but
most "*-elf" ones.  A "real" CI bot for a stable *-elf
simulator target with newlib, gcc, binutils, updating only
each at a time would be nice, and JFTR, no I don't consider
mine "real".)

> Could you please try the attached 
> patch?

Aha; yes it works; I regtested gcc for cris-elf as with my
patch.  Thanks for attending to this.  I hope the other
points I raised in my submission will eventually be handled.

> We would like to introduce a configuration option for Newlib to use 
> thread-local storage for the members of struct _reent:
> 
> https://sourceware.org/pipermail/newlib/2022/018855.html

Ok, I see, thanks for clarifying.

> We also would like to make the _REENT_GLOBAL_STDIO_STREAMS option the 
> default Newlib behaviour:
> 
> https://sourceware.org/pipermail/newlib/2022/019735.html

Thanks; FWIW, I don't have a strong opinion about that.  As
long as it still works, I'm happy.  (Not actually using
newlib for production here, AFAIK.)

brgds, H-P

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

end of thread, other threads:[~2022-06-07 23:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-06 18:20 [PATCH] Fix stdio init handling Hans-Peter Nilsson
2022-06-06 20:16 ` Jeff Johnston
2022-06-07  6:21   ` Sebastian Huber
2022-06-07 14:47     ` Jeff Johnston
2022-06-07  6:05 ` Torbjorn SVENSSON
2022-06-07 14:40   ` Hans-Peter Nilsson
2022-06-07  6:18 ` Sebastian Huber
2022-06-07 19:31   ` Sebastian Huber
2022-06-07 23:30   ` Hans-Peter Nilsson

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