[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();
    }

  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