Re: [PATCH v2 1/1] Make __sdidinit unused

Hello Corinna,

On 18/02/2022 13:41, Corinna Vinschen wrote:
> Hi Matt,
> 
> On Feb 18 10:45, Matthew Joyce wrote:
>> Remove dependency on __sdidinit member of struct _reent to check
>> object initialization. Like __sdidinit, the __cleanup member of
>> struct _reent is initialized in the __sinit() function. Checking
>> initialization against __cleanup serves the same purpose and will
>> reduce overhead in the __sfp() function in a follow up patch.
> 
> The patch looks right now.  But.
> 
> What exactly are you going to do in __sfp?  The reason I'm asking is
> that it's absolutely not clear yet which purpose this change serves.
> Looking at this patch, I only see that, rather than using an available
> flag, a pointer is now set to an invalid value -1, which looks more
> dangerous than what we did before.
> 
> Care to explain or, even better, send the patch you have in mind?

this is related to our attempt to use individual thread-local storage 
objects instead of the monolithic struct _reent:

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

It turned out that this is not possible while _GLOBAL_REENT exists. 
Getting rid of struct _reent members with similar functionality helps to 
refactor the code.

Instead of using -1 as a magic value a safer option would be to assign a 
dummy function which does nothing:

diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc
index 6ce1202ef..c8352adf9 100644
--- a/winsup/cygwin/cygtls.cc
+++ b/winsup/cygwin/cygtls.cc
@@ -60,9 +60,8 @@ _cygtls::init_thread (void *x, DWORD (*func) (void *, 
void *))
           local_clib._stdin = _GLOBAL_REENT->_stdin;
           local_clib._stdout = _GLOBAL_REENT->_stdout;
           local_clib._stderr = _GLOBAL_REENT->_stderr;
-         local_clib.__cleanup = (void (*) (struct _reent *))
-                                (_GLOBAL_REENT->__cleanup
-                                 ? (void *) -1 : NULL);
+         if (_GLOBAL_REENT->__cleanup)
+           local_clib.__cleanup = _cygtls::cleanup_early;
           local_clib.__sglue._niobs = 3;
           local_clib.__sglue._iobs = &_GLOBAL_REENT->__sf[0];
         }
@@ -150,6 +149,12 @@ _cygtls::remove (DWORD wait)
      }
  }

+void
+_cygtls::cleanup_early (struct _reent *)
+{
+  /* Do nothing */
+}
+
  #ifdef __x86_64__
  void san::leave ()
  {
diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h
index a2e3676fc..c2c4141bf 100644
--- a/winsup/cygwin/cygtls.h
+++ b/winsup/cygwin/cygtls.h
@@ -272,6 +272,7 @@ public:
      will_wait_for_signal = false;
    }
    void handle_SIGCONT ();
+  static void cleanup_early(struct _reent *);
  private:
    void __reg3 call2 (DWORD (*) (void *, void *), void *, void *);
    void remove_pending_sigs ();
diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index 7f26c6304..e757c47ec 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -829,8 +829,8 @@ main_thread_sinit ()
       wrong spot.  The input or output buffer will be NULLed and nothing is
       read or written in the first stdio function call in the main thread.

-     To fix this issue we set __cleanup to -1 here. */
-  _REENT->__cleanup = (void (*) (struct _reent *)) -1;
+     To fix this issue we set __cleanup to _cygtls::cleanup_early here. */
+  _REENT->__cleanup = _cygtls::cleanup_early;
  }

  /* Take over from libc's crt0.o and start the application. Note the
diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
index 21b2dbe46..b7da4d0c7 100644
--- a/winsup/cygwin/thread.cc
+++ b/winsup/cygwin/thread.cc
@@ -564,7 +564,7 @@ pthread::exit (void *value_ptr)
        mutex.unlock ();
      }

-  if (_my_tls.local_clib.__cleanup == (void (*) (struct _reent *)) -1)
+  if (_my_tls.local_clib.__cleanup == _cygtls::cleanup_early)
      _my_tls.local_clib.__cleanup = NULL;
    _reclaim_reent (_REENT);


-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: [email protected]
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/