Re: [PATCH] stdlib: Fix data race in __run_exit_handlers

[Vitaly Buka via Libc-alpha <libc-alpha@sourceware.org>]

Oh, this atomics are not what I thought. So the test reproduced bugs in the
test.
I tried different approaches and I failed to reproduce the data race.

On Sun, 20 Sep 2020 at 14:26, Vitaly Buka <vitalybuka@google.com> wrote:

>
>
> On Sun, 20 Sep 2020 at 13:42, Paul Pluzhnikov <ppluzhnikov@google.com>
> wrote:
>
>> On Sun, Sep 20, 2020 at 5:10 AM Vitaly Buka via Libc-alpha
>> <libc-alpha@sourceware.org> wrote:
>>
>> > +static void *
>> > +threadfunc (void *unused)
>> > +{
>> > +  for (; done < 1e6;)
>> > +    {
>> > +      if (added < done + 100)
>> > +        {
>> > +          __cxa_atexit (&atexitcb, (void *)(++added), __dso_handle);
>>
>> Isn't there a data race on "added" here (in addition to a data race on
>> "done")?
>> What prevents two threads from observing "added == 100" at the same
>> time and adding two calls with value of 101, which would later trigger
>> abort() in exitcb()?
>>
>
> They are atomic. Isn't (++added) guarantee to return different values in
> all threads?
>
>
>>
>> > +  /* With default 8MiB Linux stack size, creating 1024 threads can
>> cause
>> > +     VM exhausiton on 32-bit machines.  Reduce stack size of each
>> thread to
>> > +     128KiB for a maximum required VM size of 128MiB.  */
>>
>> This comment is far removed from the computation of kStacksize (and
>> the name violates the naming conventions used here).
>>
>> I suggest:
>>
>>   size_t stack_size = 128 << 10; /* 128KiB  */
>>   if (stack_size < PTHREAD_STACK_MIN) stack_size = PTHREAD_STACK_MIN;
>>
>> Also, I suspect that 32KiB would be more than enough for stack size here.
>>
>> > +  for (i = 0; i < kNumThreads; ++i)
>>
>> Since kNumThreads isn't used anywhere else, I suggest making it a local:
>>
>>   const int num_threads = 50;
>>
>> --
>> Paul Pluzhnikov
>>
>