Possiblly bug of cygwin1.dll

Possiblly bug of cygwin1.dll

[Takashi Yano]

Hi,

I might find the bug of cygwin1.dll (including 3.4.x, 3.5.0 (TEST)).
The following test case (c++ code) causes handle leak.

This issue is reproducible with both g++ and clang++.
However, it does not happen in Linux environment.
So I guess this is the cygwin1.dlll bug.

I looked into this problem a bit, and found number of event handle
increases every loop.

I doubt pthread_mutex_xxx functions.

#include <future>
int func() { return 0; }
int main()
{
  for (;;) {
    std::future<int> f = std::async(std::launch::async, func);
    f.get();
  }
  return 0;
}

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 19 22:44, Takashi Yano via Cygwin wrote:
> Hi,
> 
> I might find the bug of cygwin1.dll (including 3.4.x, 3.5.0 (TEST)).
> The following test case (c++ code) causes handle leak.
> 
> This issue is reproducible with both g++ and clang++.
> However, it does not happen in Linux environment.
> So I guess this is the cygwin1.dlll bug.
> 
> I looked into this problem a bit, and found number of event handle
> increases every loop.
> 
> I doubt pthread_mutex_xxx functions.
> 
> #include <future>
> int func() { return 0; }
> int main()
> {
>   for (;;) {
>     std::future<int> f = std::async(std::launch::async, func);
>     f.get();
>   }
>   return 0;
> }

Can you create a plain C testcase from there?  It's much easier to
debug.


Thanks,
Corinna

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

Hi Corinna,

On Fri, 19 Jan 2024 15:28:40 +0100
Corinna Vinschen wrote:
> On Jan 19 22:44, Takashi Yano via Cygwin wrote:
> > Hi,
> > 
> > I might find the bug of cygwin1.dll (including 3.4.x, 3.5.0 (TEST)).
> > The following test case (c++ code) causes handle leak.
> > 
> > This issue is reproducible with both g++ and clang++.
> > However, it does not happen in Linux environment.
> > So I guess this is the cygwin1.dlll bug.
> > 
> > I looked into this problem a bit, and found number of event handle
> > increases every loop.
> > 
> > I doubt pthread_mutex_xxx functions.
> > 
> > #include <future>
> > int func() { return 0; }
> > int main()
> > {
> >   for (;;) {
> >     std::future<int> f = std::async(std::launch::async, func);
> >     f.get();
> >   }
> >   return 0;
> > }
> 
> Can you create a plain C testcase from there?  It's much easier to
> debug.

I could symplify the test case:
#include <mutex>
int main()
{
  for (;;) {
    std::mutex *m = new std::mutex;
    m->lock();
    m->unlock();
    delete m;
  }
  return 0;
}

And I tried to observe the pthread_mutex_xxx() call. Then found the
test case does like:

#include <pthread.h>
int main()
{
  for (;;) {
    pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
    pthread_mutex_lock(&m);
    pthread_mutex_unlock(&m);
  }
  return 0;
}

POSIX states pthread_mutex_t can be initialized with
PTREAD_MUTEX_INITIALZER when it is STATICALLY allocated.

In this case, m is not static. So it seems that this is
a bug of libstdc++. However, the plain c code above works
in Linux without problems even with non-static mutex m.

I guess it is very difficult to make the plain c code above
work in cygwin, because cygwin can not know when cygwin can
discard the mutex resources...

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

Hi Corinna and Achim,

On Sat, 20 Jan 2024 13:18:25 +0900
Takashi Yano wrote:
> Hi Corinna,
> 
> On Fri, 19 Jan 2024 15:28:40 +0100
> Corinna Vinschen wrote:
> > On Jan 19 22:44, Takashi Yano via Cygwin wrote:
> > > Hi,
> > > 
> > > I might find the bug of cygwin1.dll (including 3.4.x, 3.5.0 (TEST)).
> > > The following test case (c++ code) causes handle leak.
> > > 
> > > This issue is reproducible with both g++ and clang++.
> > > However, it does not happen in Linux environment.
> > > So I guess this is the cygwin1.dlll bug.
> > > 
> > > I looked into this problem a bit, and found number of event handle
> > > increases every loop.
> > > 
> > > I doubt pthread_mutex_xxx functions.
> > > 
> > > #include <future>
> > > int func() { return 0; }
> > > int main()
> > > {
> > >   for (;;) {
> > >     std::future<int> f = std::async(std::launch::async, func);
> > >     f.get();
> > >   }
> > >   return 0;
> > > }
> > 
> > Can you create a plain C testcase from there?  It's much easier to
> > debug.
> 
> I could symplify the test case:
> #include <mutex>
> int main()
> {
>   for (;;) {
>     std::mutex *m = new std::mutex;
>     m->lock();
>     m->unlock();
>     delete m;
>   }
>   return 0;
> }
> 
> And I tried to observe the pthread_mutex_xxx() call. Then found the
> test case does like:
> 
> #include <pthread.h>
> int main()
> {
>   for (;;) {
>     pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
>     pthread_mutex_lock(&m);
>     pthread_mutex_unlock(&m);
>   }
>   return 0;
> }
> 
> POSIX states pthread_mutex_t can be initialized with
> PTREAD_MUTEX_INITIALZER when it is STATICALLY allocated.
> 
> In this case, m is not static. So it seems that this is
> a bug of libstdc++. However, the plain c code above works
> in Linux without problems even with non-static mutex m.
> 
> I guess it is very difficult to make the plain c code above
> work in cygwin, because cygwin can not know when cygwin can
> discard the mutex resources...

I might find the culprit in gcc's libstdc++ code such as:
libstdc++-v3/include/ext/concurrentce.h:
  class __mutex
  {
  private:
#if __GTHREADS && defined __GTHREAD_MUTEX_INIT
    __gthread_mutex_t _M_mutex = __GTHREAD_MUTEX_INIT;
#else
    __gthread_mutex_t _M_mutex;
#endif

__GTHREAD_MUTEX_INIT here is PTHREAD_MUTEX_INITIALIZER and
__gthread_mutex_t is pthread_mutex_t.

I think this code vaiolates the POSIX statement.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[ASSI]

Takashi Yano via Cygwin writes:
> I might find the culprit in gcc's libstdc++ code such as:
> libstdc++-v3/include/ext/concurrentce.h:
>   class __mutex
>   {
>   private:
> #if __GTHREADS && defined __GTHREAD_MUTEX_INIT
>     __gthread_mutex_t _M_mutex = __GTHREAD_MUTEX_INIT;
> #else
>     __gthread_mutex_t _M_mutex;
> #endif
>
> __GTHREAD_MUTEX_INIT here is PTHREAD_MUTEX_INITIALIZER and
> __gthread_mutex_t is pthread_mutex_t.
>
> I think this code vaiolates the POSIX statement.

So what happens if you undefine __GTHREAD_MUTEX_INIT?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for KORG EX-800 and Poly-800MkII V0.9:
http://Synth.Stromeko.net/Downloads.html#KorgSDada

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

On Sat, 20 Jan 2024 10:13:22 +0100
ASSI wrote:
> Takashi Yano via Cygwin writes:
> > I might find the culprit in gcc's libstdc++ code such as:
> > libstdc++-v3/include/ext/concurrentce.h:
> >   class __mutex
> >   {
> >   private:
> > #if __GTHREADS && defined __GTHREAD_MUTEX_INIT
> >     __gthread_mutex_t _M_mutex = __GTHREAD_MUTEX_INIT;
> > #else
> >     __gthread_mutex_t _M_mutex;
> > #endif
> >
> > __GTHREAD_MUTEX_INIT here is PTHREAD_MUTEX_INITIALIZER and
> > __gthread_mutex_t is pthread_mutex_t.
> >
> > I think this code vaiolates the POSIX statement.
> 
> So what happens if you undefine __GTHREAD_MUTEX_INIT?

I have tried. The test case:
#include <mutex>
int main()
{
  for (;;) {
    std::mutex *m = new std::mutex;
    m->lock();
    m->unlock();
    delete m;
  }
  return 0;
}
gets working fine. However, this test case:
#include <future>
int func() { return 0; }
int main()
{
  for (;;) {
    std::future<int> f = std::async(std::launch::async, func);
    f.get();
  }
  return 0;
}
still has the problem.

pthread_mutex_t might be initialized also at another place...


-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[ASSI]

Takashi Yano via Cygwin writes:
>> So what happens if you undefine __GTHREAD_MUTEX_INIT?
>
> I have tried. The test case:

…and then rebuild libstdc++ of course.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf Blofeld V1.15B11:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

Hi Corinna,

On Sat, 20 Jan 2024 21:24:27 +0900
Takashi Yano wrote:
> On Sat, 20 Jan 2024 10:13:22 +0100
> ASSI wrote:
> > Takashi Yano via Cygwin writes:
> > > I might find the culprit in gcc's libstdc++ code such as:
> > > libstdc++-v3/include/ext/concurrentce.h:
> > >   class __mutex
> > >   {
> > >   private:
> > > #if __GTHREADS && defined __GTHREAD_MUTEX_INIT
> > >     __gthread_mutex_t _M_mutex = __GTHREAD_MUTEX_INIT;
> > > #else
> > >     __gthread_mutex_t _M_mutex;
> > > #endif
> > >
> > > __GTHREAD_MUTEX_INIT here is PTHREAD_MUTEX_INITIALIZER and
> > > __gthread_mutex_t is pthread_mutex_t.
> > >
> > > I think this code vaiolates the POSIX statement.
> > 
> > So what happens if you undefine __GTHREAD_MUTEX_INIT?
> 
> I have tried. The test case:
> #include <mutex>
> int main()
> {
>   for (;;) {
>     std::mutex *m = new std::mutex;
>     m->lock();
>     m->unlock();
>     delete m;
>   }
>   return 0;
> }
> gets working fine. However, this test case:
> #include <future>
> int func() { return 0; }
> int main()
> {
>   for (;;) {
>     std::future<int> f = std::async(std::launch::async, func);
>     f.get();
>   }
>   return 0;
> }
> still has the problem.
> 
> pthread_mutex_t might be initialized also at another place...

I found the cause. In pthread.h of cygwin, PTHREAD_ONCE_INIT is defined as:
#define PTHREAD_ONCE_INIT { PTHREAD_MUTEX_INITIALIZER, 0 }
however, libstdc++ initializes non-static pthread_once_t using this macro.

I cannot find the POSIX statement that only static pthread_once_t can be
initialized using PTHREAD_ONCE_INIT. If I do not overlook something,
this is the problem of cygwin side, isn't it?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[ASSI]

Takashi Yano via Cygwin writes:
> I found the cause. In pthread.h of cygwin, PTHREAD_ONCE_INIT is defined as:
> #define PTHREAD_ONCE_INIT { PTHREAD_MUTEX_INITIALIZER, 0 }
> however, libstdc++ initializes non-static pthread_once_t using this macro.

https://www.ibm.com/docs/en/aix/7.3?topic=p-pthread-once-init-macro
https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_once.html

"The behavior of pthread_once() is undefined if once_control has
automatic storage duration or is not initialized by PTHREAD_ONCE_INIT."

> I cannot find the POSIX statement that only static pthread_once_t can be
> initialized using PTHREAD_ONCE_INIT. If I do not overlook something,
> this is the problem of cygwin side, isn't it?

You can initialize just about anything with PTHREAD_ONCE_INIT, but you
cannot expect the resulting structure to work as intended if there is
more than instance per library / program, so the libstdc++ object should
be a singleton, not automatic.

Still looks like ATWIL to me…


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

On Sun, 21 Jan 2024 14:30:00 +0100
ASSI wrote:
> Takashi Yano via Cygwin writes:
> > I found the cause. In pthread.h of cygwin, PTHREAD_ONCE_INIT is defined as:
> > #define PTHREAD_ONCE_INIT { PTHREAD_MUTEX_INITIALIZER, 0 }
> > however, libstdc++ initializes non-static pthread_once_t using this macro.
> 
> https://www.ibm.com/docs/en/aix/7.3?topic=p-pthread-once-init-macro
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_once.html
> 
> "The behavior of pthread_once() is undefined if once_control has
> automatic storage duration or is not initialized by PTHREAD_ONCE_INIT."
> 
> > I cannot find the POSIX statement that only static pthread_once_t can be
> > initialized using PTHREAD_ONCE_INIT. If I do not overlook something,
> > this is the problem of cygwin side, isn't it?
> 
> You can initialize just about anything with PTHREAD_ONCE_INIT, but you
> cannot expect the resulting structure to work as intended if there is
> more than instance per library / program, so the libstdc++ object should
> be a singleton, not automatic.
> 
> Still looks like ATWIL to me…

Thanks for the information.

Anyway, I confirmed the two patches (one is for gcc, the other is for cygwin)
resolve the issue.

PATCH1: (for gcc)
Do not use PTHREAD_MUTEX_INITIALIZER etc. for non-static pthread_mutex_t.
diff -ur origsrc/gcc-13-20230902/libgcc/gthr-posix.h src/gcc-13-20230902/libgcc/gthr-posix.h
--- origsrc/gcc-13-20230902/libgcc/gthr-posix.h	2023-09-03 07:32:49.000000000 +0900
+++ src/gcc-13-20230902/libgcc/gthr-posix.h	2024-01-22 09:04:54.342189600 +0900
@@ -34,6 +34,12 @@
 
 #include <pthread.h>
 
+#ifdef __CYGWIN__
+#define _GTHREAD_USE_MUTEX_INIT_FUNC 1
+#define _GTHREAD_USE_RECURSIVE_MUTEX_INIT_FUNC 1
+#define _GTHREAD_USE_COND_INIT_FUNC 1
+#endif
+
 #if ((defined(_LIBOBJC) || defined(_LIBOBJC_WEAK)) \
      || !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK))
 # include <unistd.h>


PATCH2: (for cygwin)
Avoid handle leak caused when non-static pthread_once_t is initialized
with PTHREAD_ONCE_INIT
diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
index 7bb4f9fc8..127569160 100644
--- a/winsup/cygwin/thread.cc
+++ b/winsup/cygwin/thread.cc
@@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
     {
       init_routine ();
       once_control->state = 1;
+      pthread_mutex_unlock (&once_control->mutex);
+      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
+      return 0;
     }
   /* Here we must remove our cancellation handler */
   pthread_mutex_unlock (&once_control->mutex);

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 22 12:30, Takashi Yano via Cygwin wrote:
> PATCH2: (for cygwin)
> Avoid handle leak caused when non-static pthread_once_t is initialized
> with PTHREAD_ONCE_INIT
> diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> index 7bb4f9fc8..127569160 100644
> --- a/winsup/cygwin/thread.cc
> +++ b/winsup/cygwin/thread.cc
> @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
>      {
>        init_routine ();
>        once_control->state = 1;
> +      pthread_mutex_unlock (&once_control->mutex);
> +      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> +      return 0;
>      }
>    /* Here we must remove our cancellation handler */
>    pthread_mutex_unlock (&once_control->mutex);

I see what you're doing here.  Wouldn't it be simpler, though, to do this?

diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
index 7bb4f9fc8341..7ec3aace395d 100644
--- a/winsup/cygwin/thread.cc
+++ b/winsup/cygwin/thread.cc
@@ -2063,6 +2063,7 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
     }
   /* Here we must remove our cancellation handler */
   pthread_mutex_unlock (&once_control->mutex);
+  while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
   return 0;
 }


Corinna

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 22 10:25, Corinna Vinschen via Cygwin wrote:
> On Jan 22 12:30, Takashi Yano via Cygwin wrote:
> > PATCH2: (for cygwin)
> > Avoid handle leak caused when non-static pthread_once_t is initialized
> > with PTHREAD_ONCE_INIT
> > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > index 7bb4f9fc8..127569160 100644
> > --- a/winsup/cygwin/thread.cc
> > +++ b/winsup/cygwin/thread.cc
> > @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> >      {
> >        init_routine ();
> >        once_control->state = 1;
> > +      pthread_mutex_unlock (&once_control->mutex);
> > +      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> > +      return 0;
> >      }
> >    /* Here we must remove our cancellation handler */
> >    pthread_mutex_unlock (&once_control->mutex);
> 
> I see what you're doing here.  Wouldn't it be simpler, though, to do this?
> 
> diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> index 7bb4f9fc8341..7ec3aace395d 100644
> --- a/winsup/cygwin/thread.cc
> +++ b/winsup/cygwin/thread.cc
> @@ -2063,6 +2063,7 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
>      }
>    /* Here we must remove our cancellation handler */

Strange enough, this comment accompanies the code since its inception
in 2001.  It says explicitly "remove" the cancellation handler.
That sounds like the idea was right, just the programmer forgot about
it afterwards...

>    pthread_mutex_unlock (&once_control->mutex);
> +  while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
>    return 0;
>  }


Corinna

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

On Mon, 22 Jan 2024 10:25:28 +0100
Corinna Vinschen wrote:
> On Jan 22 12:30, Takashi Yano via Cygwin wrote:
> > PATCH2: (for cygwin)
> > Avoid handle leak caused when non-static pthread_once_t is initialized
> > with PTHREAD_ONCE_INIT
> > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > index 7bb4f9fc8..127569160 100644
> > --- a/winsup/cygwin/thread.cc
> > +++ b/winsup/cygwin/thread.cc
> > @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> >      {
> >        init_routine ();
> >        once_control->state = 1;
> > +      pthread_mutex_unlock (&once_control->mutex);
> > +      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> > +      return 0;
> >      }
> >    /* Here we must remove our cancellation handler */
> >    pthread_mutex_unlock (&once_control->mutex);
> 
> I see what you're doing here.  Wouldn't it be simpler, though, to do this?
> 
> diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> index 7bb4f9fc8341..7ec3aace395d 100644
> --- a/winsup/cygwin/thread.cc
> +++ b/winsup/cygwin/thread.cc
> @@ -2063,6 +2063,7 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
>      }
>    /* Here we must remove our cancellation handler */
>    pthread_mutex_unlock (&once_control->mutex);
> +  while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
>    return 0;
>  }

In this code, if several threads call pthread_once() at the same time,
only one thread will succeed pthread_mutex_destroy() and the others
will fail with EINVAL. But it does not matter. The code will be
simpler.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

On Mon, 22 Jan 2024 10:57:48 +0100
Corinna Vinschen wrote:
> On Jan 22 10:25, Corinna Vinschen via Cygwin wrote:
> > On Jan 22 12:30, Takashi Yano via Cygwin wrote:
> > > PATCH2: (for cygwin)
> > > Avoid handle leak caused when non-static pthread_once_t is initialized
> > > with PTHREAD_ONCE_INIT
> > > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > > index 7bb4f9fc8..127569160 100644
> > > --- a/winsup/cygwin/thread.cc
> > > +++ b/winsup/cygwin/thread.cc
> > > @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> > >      {
> > >        init_routine ();
> > >        once_control->state = 1;
> > > +      pthread_mutex_unlock (&once_control->mutex);
> > > +      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> > > +      return 0;
> > >      }
> > >    /* Here we must remove our cancellation handler */
> > >    pthread_mutex_unlock (&once_control->mutex);
> > 
> > I see what you're doing here.  Wouldn't it be simpler, though, to do this?
> > 
> > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > index 7bb4f9fc8341..7ec3aace395d 100644
> > --- a/winsup/cygwin/thread.cc
> > +++ b/winsup/cygwin/thread.cc
> > @@ -2063,6 +2063,7 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> >      }
> >    /* Here we must remove our cancellation handler */
> 
> Strange enough, this comment accompanies the code since its inception
> in 2001.  It says explicitly "remove" the cancellation handler.
> That sounds like the idea was right, just the programmer forgot about
> it afterwards...

I am not sure what 'cancellation handler' means here. Is it the
event handler in pthread_mutex_t?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 22 20:06, Takashi Yano via Cygwin wrote:
> On Mon, 22 Jan 2024 10:25:28 +0100
> Corinna Vinschen wrote:
> > On Jan 22 12:30, Takashi Yano via Cygwin wrote:
> > > PATCH2: (for cygwin)
> > > Avoid handle leak caused when non-static pthread_once_t is initialized
> > > with PTHREAD_ONCE_INIT
> > > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > > index 7bb4f9fc8..127569160 100644
> > > --- a/winsup/cygwin/thread.cc
> > > +++ b/winsup/cygwin/thread.cc
> > > @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> > >      {
> > >        init_routine ();
> > >        once_control->state = 1;
> > > +      pthread_mutex_unlock (&once_control->mutex);
> > > +      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> > > +      return 0;
> > >      }
> > >    /* Here we must remove our cancellation handler */
> > >    pthread_mutex_unlock (&once_control->mutex);
> > 
> > I see what you're doing here.  Wouldn't it be simpler, though, to do this?
> > 
> > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > index 7bb4f9fc8341..7ec3aace395d 100644
> > --- a/winsup/cygwin/thread.cc
> > +++ b/winsup/cygwin/thread.cc
> > @@ -2063,6 +2063,7 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> >      }
> >    /* Here we must remove our cancellation handler */
> >    pthread_mutex_unlock (&once_control->mutex);
> > +  while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> >    return 0;
> >  }
> 
> In this code, if several threads call pthread_once() at the same time,
> only one thread will succeed pthread_mutex_destroy() and the others
> will fail with EINVAL. But it does not matter. The code will be
> simpler.

Yeah, but you're right.  It's cleaner to do this only in the thread
actually performing the init action so your original patch makes
more sense.


Corinna

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 22 20:16, Takashi Yano via Cygwin wrote:
> On Mon, 22 Jan 2024 10:57:48 +0100
> Corinna Vinschen wrote:
> > On Jan 22 10:25, Corinna Vinschen via Cygwin wrote:
> > > On Jan 22 12:30, Takashi Yano via Cygwin wrote:
> > > > PATCH2: (for cygwin)
> > > > Avoid handle leak caused when non-static pthread_once_t is initialized
> > > > with PTHREAD_ONCE_INIT
> > > > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > > > index 7bb4f9fc8..127569160 100644
> > > > --- a/winsup/cygwin/thread.cc
> > > > +++ b/winsup/cygwin/thread.cc
> > > > @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> > > >      {
> > > >        init_routine ();
> > > >        once_control->state = 1;
> > > > +      pthread_mutex_unlock (&once_control->mutex);
> > > > +      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
> > > > +      return 0;
> > > >      }
> > > >    /* Here we must remove our cancellation handler */
> > > >    pthread_mutex_unlock (&once_control->mutex);
> > > 
> > > I see what you're doing here.  Wouldn't it be simpler, though, to do this?
> > > 
> > > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > > index 7bb4f9fc8341..7ec3aace395d 100644
> > > --- a/winsup/cygwin/thread.cc
> > > +++ b/winsup/cygwin/thread.cc
> > > @@ -2063,6 +2063,7 @@ pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> > >      }
> > >    /* Here we must remove our cancellation handler */
> > 
> > Strange enough, this comment accompanies the code since its inception
> > in 2001.  It says explicitly "remove" the cancellation handler.
> > That sounds like the idea was right, just the programmer forgot about
> > it afterwards...
> 
> I am not sure what 'cancellation handler' means here. Is it the
> event handler in pthread_mutex_t?

Aaah, no.  I just read and re-read the stuff and it occured to me that
this is based on the preceeding, longer comment in pthread::once.
Theoretically, the comments say, we need to set up a cancellation handler
so pthread_once becomes cancellable.

However, I don't find this in the standards.  pthread_once is neither
one of the required cancellation points, nor one of the optional
cancellation points.

So now I wonder if we shouldn't just get rid of the cokmments talking
about the cancellation in pthread::once entirely.


Corinna

Re: Possiblly bug of cygwin1.dll

[ASSI]

Corinna Vinschen via Cygwin writes:
> However, I don't find this in the standards.  pthread_once is neither
> one of the required cancellation points, nor one of the optional
> cancellation points.

The initializer can be cancellable per POSIX, though:

"The pthread_once() function is not a cancellation point. However, if
init_routine is a cancellation point and is canceled, the effect on
once_control shall be as if pthread_once() was never called."


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 22 13:41, ASSI via Cygwin wrote:
> Corinna Vinschen via Cygwin writes:
> > However, I don't find this in the standards.  pthread_once is neither
> > one of the required cancellation points, nor one of the optional
> > cancellation points.
> 
> The initializer can be cancellable per POSIX, though:
> 
> "The pthread_once() function is not a cancellation point. However, if
> init_routine is a cancellation point and is canceled, the effect on
> once_control shall be as if pthread_once() was never called."

I didn't test that, but that should be automatically correct, the
way cancellation works in Cygwin.


Corinna

Re: Possiblly bug of cygwin1.dll

[Kaz Kylheku]

On 2024-01-19 20:18, Takashi Yano via Cygwin wrote:
> And I tried to observe the pthread_mutex_xxx() call. Then found the
> test case does like:
> 
> #include <pthread.h>
> int main()
> {
>   for (;;) {
>     pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
>     pthread_mutex_lock(&m);
>     pthread_mutex_unlock(&m);
>   }
>   return 0;
> }

Note POSIX:

In cases where default mutex attributes are appropriate,
the macro PTHREAD_MUTEX_INITIALIZER can be used to initialize
mutexes. The effect shall be equivalent to dynamic initialization
by a call to pthread_mutex_init() with parameter attr specified as NULL,
except that no error checks are performed.

Thus, the following is correct:

   for (;;) {
     pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
     pthread_mutex_lock(&m);
     pthread_mutex_unlock(&m);
     pthread_mutex_destroy(&m); // <--- added
   }

Does your above code leak if you add the destroy call?

If so, pthread_mutex_destroy needs to be fixed.

Either way, libstdc++ should be calling pthread_mutex_destroy
in the destructor, in spite of initializing the object with
a simple initializer.

That libstdc++ library could be fixed in the same way;
the mutex object's destructor should call pthread_mutex_destroy,
even though the constructor didn't call pthread_mutex_init.

This is a "moral equivalent":

  class buf {
    unsigned char *ptr;
  public:
    buf() : ptr(NULL) { }
    ~buf() { delete [] ptr; }
    // ...
  };

Just because you have a constructor that trivially initializes
some resource with a constant expression doesn't mean that the
destructor has nothing to free. In between there the object
is mutated so that it holds resources.


> POSIX states pthread_mutex_t can be initialized with
> PTREAD_MUTEX_INITIALZER when it is STATICALLY allocated.

I'm looking at this and don't see such a constraint:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_destroy.html

The word "static" only occurs in the Rationale section.

Use of the initializer is not restricted to static objects
by any normative wording.

In real systems, the static distinction has no meaning.

This code can be inside a shared library:

   static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER;

this library could be loaded by dlopen and unloaded with dlclose.
Thus static becomes dynamic!

And, by the way, this is a problem: if we have a library
which does the above, and we repeatedly load it and unload
it while using the mutex in between, it will leak.

I think you don't want to do this kind of initialization in
reloadable plugins, unless you put in some destructor hooks,
or wrap it with C++ objects with destructors.

Re: Possiblly bug of cygwin1.dll

[Takashi Yano]

On Mon, 22 Jan 2024 19:24:52 -0800
Kaz Kylheku wrote:
> On 2024-01-19 20:18, Takashi Yano via Cygwin wrote:
> > And I tried to observe the pthread_mutex_xxx() call. Then found the
> > test case does like:
> > 
> > #include <pthread.h>
> > int main()
> > {
> >   for (;;) {
> >     pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
> >     pthread_mutex_lock(&m);
> >     pthread_mutex_unlock(&m);
> >   }
> >   return 0;
> > }
> 
> Note POSIX:
> 
> In cases where default mutex attributes are appropriate,
> the macro PTHREAD_MUTEX_INITIALIZER can be used to initialize
> mutexes. The effect shall be equivalent to dynamic initialization
> by a call to pthread_mutex_init() with parameter attr specified as NULL,
> except that no error checks are performed.
> 
> Thus, the following is correct:
> 
>    for (;;) {
>      pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
>      pthread_mutex_lock(&m);
>      pthread_mutex_unlock(&m);
>      pthread_mutex_destroy(&m); // <--- added
>    }
> 
> Does your above code leak if you add the destroy call?

No.

> If so, pthread_mutex_destroy needs to be fixed.
> 
> Either way, libstdc++ should be calling pthread_mutex_destroy
> in the destructor, in spite of initializing the object with
> a simple initializer.

Are there any code examples that use PTHREAD_MUTEX_INITIALIZER
with pthread_mutex_destroy()?

> That libstdc++ library could be fixed in the same way;
> the mutex object's destructor should call pthread_mutex_destroy,
> even though the constructor didn't call pthread_mutex_init.
> 
> This is a "moral equivalent":
> 
>   class buf {
>     unsigned char *ptr;
>   public:
>     buf() : ptr(NULL) { }
>     ~buf() { delete [] ptr; }
>     // ...
>   };
> 
> Just because you have a constructor that trivially initializes
> some resource with a constant expression doesn't mean that the
> destructor has nothing to free. In between there the object
> is mutated so that it holds resources.
> 
> 
> > POSIX states pthread_mutex_t can be initialized with
> > PTREAD_MUTEX_INITIALZER when it is STATICALLY allocated.
> 
> I'm looking at this and don't see such a constraint:
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_destroy.html
> 
> The word "static" only occurs in the Rationale section.
> 
> Use of the initializer is not restricted to static objects
> by any normative wording.

It seems that I had read the older POSIX document.

https://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mutex_destroy.html

> In real systems, the static distinction has no meaning.
> 
> This code can be inside a shared library:
> 
>    static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER;
> 
> this library could be loaded by dlopen and unloaded with dlclose.
> Thus static becomes dynamic!
> 
> And, by the way, this is a problem: if we have a library
> which does the above, and we repeatedly load it and unload
> it while using the mutex in between, it will leak.

As you pointed out, if dlopen()/dlclose() are called repeatedly,
handle leak might occur even if pthread_mutex_t is statically
allocated.

> I think you don't want to do this kind of initialization in
> reloadable plugins, unless you put in some destructor hooks,
> or wrap it with C++ objects with destructors.


-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 24 20:55, Takashi Yano via Cygwin wrote:
> On Mon, 22 Jan 2024 19:24:52 -0800
> Kaz Kylheku wrote:
> > In real systems, the static distinction has no meaning.
> > 
> > This code can be inside a shared library:
> > 
> >    static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER;
> > 
> > this library could be loaded by dlopen and unloaded with dlclose.
> > Thus static becomes dynamic!
> > 
> > And, by the way, this is a problem: if we have a library
> > which does the above, and we repeatedly load it and unload
> > it while using the mutex in between, it will leak.
> 
> As you pointed out, if dlopen()/dlclose() are called repeatedly,
> handle leak might occur even if pthread_mutex_t is statically
> allocated.

Cygwin 3.5 is due really soon now, so we can't change anything here,
except fixing the pthread_once problem (@takashi, didn't you want to
apply your patch?)

As for the next major release, do we have a chance to revamp
pthread_mutex_t so that it does NOT dynamically create an OS synch
object?  Is there a way we can change the really much too complex
pthreads code to simplify things and use, say, SRWLOCKs, or any other
synch mechanism which is faster and less intrusive?

The biggest problem, IMHO, is the DREADED fact that the original author
of the pthreads code defined the exposed pthread types as, for instance,

  typedef struct __pthread_mutex_t {char __dummy;} *pthread_mutex_t;

So they only take 1 byte in user space and there's no chance to fit
an SRWLOCK or, FWIW, a LONG value in there to be used with Interlocked
functions.  That's really a problem we're kind of stuck with, I fear.


Corinna

Re: Possiblly bug of cygwin1.dll

[Corinna Vinschen]

On Jan 24 14:05, Corinna Vinschen via Cygwin wrote:
> On Jan 24 20:55, Takashi Yano via Cygwin wrote:
> > On Mon, 22 Jan 2024 19:24:52 -0800
> > Kaz Kylheku wrote:
> > > In real systems, the static distinction has no meaning.
> > > 
> > > This code can be inside a shared library:
> > > 
> > >    static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER;
> > > 
> > > this library could be loaded by dlopen and unloaded with dlclose.
> > > Thus static becomes dynamic!
> > > 
> > > And, by the way, this is a problem: if we have a library
> > > which does the above, and we repeatedly load it and unload
> > > it while using the mutex in between, it will leak.
> > 
> > As you pointed out, if dlopen()/dlclose() are called repeatedly,
> > handle leak might occur even if pthread_mutex_t is statically
> > allocated.
> 
> Cygwin 3.5 is due really soon now, so we can't change anything here,
> except fixing the pthread_once problem (@takashi, didn't you want to
> apply your patch?)
> 
> As for the next major release, do we have a chance to revamp
> pthread_mutex_t so that it does NOT dynamically create an OS synch
> object?  Is there a way we can change the really much too complex
> pthreads code to simplify things and use, say, SRWLOCKs, or any other
> synch mechanism which is faster and less intrusive?
> 
> The biggest problem, IMHO, is the DREADED fact that the original author
> of the pthreads code defined the exposed pthread types as, for instance,
> 
>   typedef struct __pthread_mutex_t {char __dummy;} *pthread_mutex_t;
> 
> So they only take 1 byte in user space and there's no chance to fit
> an SRWLOCK or, FWIW, a LONG value in there to be used with Interlocked
> functions.  That's really a problem we're kind of stuck with, I fear.

No, wait, I'm an idiot.  The types are defined as *pointers*, so
they have a size of 8 bytes in user code.  That means we should
be able to implement this differently, less dynamic, and still
be able to do it backward compatible.

We *really should try that and simplifying things at the same time.

Is anybody willing to give this a whirl?  We have a good year until
the next major release...


Corinna

Re: Possiblly bug of cygwin1.dll

[Kaz Kylheku]

On 2024-01-24 03:55, Takashi Yano via Cygwin wrote:
> Are there any code examples that use PTHREAD_MUTEX_INITIALIZER
> with pthread_mutex_destroy()?

I don't think I've seen one.

I think they are rare in the field, precisely because
PTHREAD_MUTEX_INITIALIZER is mainly used in C code to
"initialize and forget" global mutexes, so they are ready
before the first thread is every created (and before main
is even called).

Nobody cares about destruction of global mutexes in an executable
(not shared library).

>> Use of the initializer is not restricted to static objects
>> by any normative wording.
> 
> It seems that I had read the older POSIX document.
> 
> https://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mutex_destroy.html

Aha! OK!

Because I had a memory of there had been something about a static restriction.

I took it for granted you were right about that, and was looking at that for other reasons.

So, good, that is gone. It never made sense anyway.

Pairing the initializer with the destructor call can be done in all situations now.

Re: Possiblly bug of cygwin1.dll

[Kaz Kylheku]

On 2024-01-24 05:11, Corinna Vinschen via Cygwin wrote:
> Is anybody willing to give this a whirl?  We have a good year until
> the next major release...

As far as the problem of not allocating per-mutex kernel objects,
this can be done by implementing futex.

Linux has futexes, mainly for solving certain problems having to do
with doing synchronization efficiently in user space, while requiring
the kernel to actually make threads wait.

But the technique has an attractive aspect in that programs do not
have to allocate and free futexes. Any memory location is a futex.

(A vaguely similar idea was implemented in early Unix: the "wait
channel" (wchan). Any memory location in the kernel could be waited
on and signaled, without allocating or freeing any sync object.
That's where the wchan field comes from in ps; showing the address
of what the process is waiting in. Because the wait channel had
no state, any address could be used. Addresses of functions were
used, because those could be resolved back to meaningful names via
the symbol table. Futexes have state, though.)