On Thu, 30 May 2024 12:14:10 +0200
Bruno Haible wrote:
> Takashi Yano wrote in cygwin-patches:
> > int
> > pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> > {
> > - // already done ?
> > - if (once_control->state)
> > + /* Sign bit of once_control->state is used as done flag */
> > + if (once_control->state & INT_MIN)
> > return 0;
> >
> > + /* The type of &once_control->state is int *, which is compatible with
> > + LONG * (the type of the first argument of InterlockedIncrement()). */
> > + InterlockedIncrement (&once_control->state);
> > pthread_mutex_lock (&once_control->mutex);
> > - /* Here we must set a cancellation handler to unlock the mutex if needed
> > */
> > - /* but a cancellation handler is not the right thing. We need this in
> > the thread
> > - *cleanup routine. Assumption: a thread can only be in one pthread_once
> > routine
> > - *at a time. Stote a mutex_t *in the pthread_structure. if that's non
> > null unlock
> > - *on pthread_exit ();
> > - */
>
> Sorry, in a unified diff form this is unreadable. One needs to look at the
> entire function. A context diff would have been better. So:
>
> int
> pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> {
> /* Sign bit of once_control->state is used as done flag */
> if (once_control->state & INT_MIN)
> return 0;
>
> /* The type of &once_control->state is int *, which is compatible with
> LONG * (the type of the first argument of InterlockedIncrement()). */
> InterlockedIncrement (&once_control->state);
> pthread_mutex_lock (&once_control->mutex);
> if (!(once_control->state & INT_MIN))
> {
> init_routine ();
> once_control->state |= INT_MIN;
> }
> pthread_mutex_unlock (&once_control->mutex);
> if (InterlockedDecrement (&once_control->state) == INT_MIN)
> pthread_mutex_destroy (&once_control->mutex);
> return 0;
> }
>
> 1) The overall logic seems right (using bit 31 of the state word as a
> 'done' bit), and the last thread that used the mutex destroys it.
>
> 2) However, the 'state' field is not volatile, and therefore the memory
> model does not guarantee that an assignment
>
> once_control->state |= INT_MIN;
>
> done in one thread has an effect on the values seen by other threads
> that do
>
> if (once_control->state & INT_MIN)
>
> As long as there is no synchronization between the two CPUs that execute
> the two threads, one CPU may indefinitely see the old value of
> once_control->state, while the other CPU's new value is not being
> "broadcasted" to all CPUs.
OK. I'll add volatile attribute to state in thread.h.
> 3) Also, as noted by Noel Grandin, there is a race: If one thread does
>
> once_control->state |= INT_MIN;
>
> while another thread does
>
> InterlockedIncrement (&once_control->state);
> or
> InterlockedDecrement (&once_control->state)
>
> the result can be that the increment or decrement is nullified. If it's
> an increment that gets nullified, the pthread_mutex_destroy occurs too
> early, with fatal consequences. If it's a decrement that get nullified,
> the pthread_mutex_destroy never happens, and there is therefore a resource
> leak.
That's right. I'll use InterlockedOr(&onece_control->state, INT_MIN)
here instead.
> 4) Does the test program that I posted in
> <https://cygwin.com/pipermail/cygwin/2024-May/255987.html> now pass?
> Notes:
> - You should set
> #define ENABLE_DEBUGGING 0
> because with the debugging output, it nearly always succeeds.
> - You should run it 10 times in a row, not just once. It may well
> succeed 9 out of 10 times and fail 1 out of 10 times.
I start to run a few hours ago:
while true; do ./a.exe; done
and it does not detect any errors so far.
--
Takashi Yano <takashi.y...@nifty.ne.jp>
