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.
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.
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.
Bruno
--
Problem reports: https://cygwin.com/problems.html
FAQ: https://cygwin.com/faq/
Documentation: https://cygwin.com/docs.html
Unsubscribe info: https://cygwin.com/ml/#unsubscribe-simple
