eal/windows: fix pthreads macros return values

  The macro definitions of the following pthread functions
return incorrect values from the inner function return code.

while pthread_barrier_init, pthread_barrier_destroy and
pthread_cancel return 0 in a case of success and non zero (errno) value
otherwise the shimming functions InitializeSynchronizationBarrier,
DeleteSynchronizationBarrier and TerminateThread return FALSE (0)
in a case of failure and TRUE(1) in a case of success.

This issue was undetected as none of the functions return codes was
checked until such check was added in commit 34cc55cce6b1 ("eal: fix
race in control thread creation") exposing the issue by failing
pthread_barrier_init and rte_eal_init on Windows as a result.

The fix aligned the return value of the 3 function with the expected
pthread API return values.

Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
Fixes: 34cc55cce6b1 ("eal: fix race in control thread creation")
Cc: stable@dpdk.org

Signed-off-by: Tal Shnaiderman <talshn@nvidia.com>
---
 lib/librte_eal/windows/include/pthread.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Hi Tal,

Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:

../../../lib/librte_eal/common/eal_common_thread.c: In function ‘ctrl_params_free’:
../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value computed is not used [-Wunused-value]
   42 |  !DeleteSynchronizationBarrier(barrier)

Probably applies to other functions and may fire in combination with future
backported patches. Hopefully since 21.05 there will be new threading API.
  

> Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Tal,
> 
> Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:
> 
> ../../../lib/librte_eal/common/eal_common_thread.c: In function
> ‘ctrl_params_free’:
> ../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value
> computed is not used [-Wunused-value]
>    42 |  !DeleteSynchronizationBarrier(barrier)
> 
> Probably applies to other functions and may fire in combination with future
> backported patches. Hopefully since 21.05 there will be new threading API.

Thanks Dmitry, it's odd that the compiler complains only now, I'd expect to see this warning even before the change.

Do we know if the new threading API will be in 21.05? API changes close in 3 days and I didn't see it get reviewed/acked.

I can change only pthread_barrier_init for now, since currently without this change Windows runtime is broken, what do you think?
  

12/04/2021 09:59, Tal Shnaiderman:
> > Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Tal,
> > 
> > Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:
> > 
> > ../../../lib/librte_eal/common/eal_common_thread.c: In function
> > ‘ctrl_params_free’:
> > ../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value
> > computed is not used [-Wunused-value]
> >    42 |  !DeleteSynchronizationBarrier(barrier)
> > 
> > Probably applies to other functions and may fire in combination with future
> > backported patches. Hopefully since 21.05 there will be new threading API.
> 
> Thanks Dmitry, it's odd that the compiler complains only now, I'd expect to see this warning even before the change.
> 
> Do we know if the new threading API will be in 21.05? API changes close in 3 days and I didn't see it get reviewed/acked.

Yes it seems too short for 21.05.

> I can change only pthread_barrier_init for now, since currently without this change Windows runtime is broken, what do you think?
  

2021-04-12 07:59 (UTC+0000), Tal Shnaiderman:
> > Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Tal,
> > 
> > Getting warnings from x86_64-w64-mingw32-gcc (GCC) 9.3.0:
> > 
> > ../../../lib/librte_eal/common/eal_common_thread.c: In function
> > ‘ctrl_params_free’:
> > ../../../lib/librte_eal/windows/include/pthread.h:42:2: warning: value
> > computed is not used [-Wunused-value]
> >    42 |  !DeleteSynchronizationBarrier(barrier)
> > 
> > Probably applies to other functions and may fire in combination with future
> > backported patches. Hopefully since 21.05 there will be new threading API.  
> 
> Thanks Dmitry, it's odd that the compiler complains only now, I'd expect to see this warning even before the change.

These functions don't have "nodiscard"-like attributes,
so a call without using result was OK, now it's an expression.

> Do we know if the new threading API will be in 21.05? API changes close in 3 days and I didn't see it get reviewed/acked.
> I can change only pthread_barrier_init for now, since currently without this change Windows runtime is broken, what do you think? 

(You probably mean pthread_barrier_destroy(), from which the warning comes.)
Yes, this is worth merging as soon as warnings are fixed.
Not sure new threading API will make it into 21.05.
  

On Sat, Apr 10, 2021 at 9:55 PM Tal Shnaiderman <talshn@nvidia.com> wrote:
>
> The macro definitions of the following pthread functions
> return incorrect values from the inner function return code.
>
> while pthread_barrier_init, pthread_barrier_destroy and
> pthread_cancel return 0 in a case of success and non zero (errno) value
> otherwise the shimming functions InitializeSynchronizationBarrier,
> DeleteSynchronizationBarrier and TerminateThread return FALSE (0)
> in a case of failure and TRUE(1) in a case of success.
>
> This issue was undetected as none of the functions return codes was
> checked until such check was added in commit 34cc55cce6b1 ("eal: fix
> race in control thread creation") exposing the issue by failing
> pthread_barrier_init and rte_eal_init on Windows as a result.
>
> The fix aligned the return value of the 3 function with the expected
> pthread API return values.
>
> Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and macros")
> Fixes: 34cc55cce6b1 ("eal: fix race in control thread creation")

Only the first Fixes: makes sense.
The second commit you refer to relies on a working pthread implementation.
  

> Subject: Re: [PATCH] eal/windows: fix pthreads macros return values
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sat, Apr 10, 2021 at 9:55 PM Tal Shnaiderman <talshn@nvidia.com>
> wrote:
> >
> > The macro definitions of the following pthread functions return
> > incorrect values from the inner function return code.
> >
> > while pthread_barrier_init, pthread_barrier_destroy and pthread_cancel
> > return 0 in a case of success and non zero (errno) value otherwise the
> > shimming functions InitializeSynchronizationBarrier,
> > DeleteSynchronizationBarrier and TerminateThread return FALSE (0) in a
> > case of failure and TRUE(1) in a case of success.
> >
> > This issue was undetected as none of the functions return codes was
> > checked until such check was added in commit 34cc55cce6b1 ("eal: fix
> > race in control thread creation") exposing the issue by failing
> > pthread_barrier_init and rte_eal_init on Windows as a result.
> >
> > The fix aligned the return value of the 3 function with the expected
> > pthread API return values.
> >
> > Fixes: e8428a9d89f1 ("eal/windows: add some basic functions and
> > macros")
> > Fixes: 34cc55cce6b1 ("eal: fix race in control thread creation")
> 
> Only the first Fixes: makes sense.
> The second commit you refer to relies on a working pthread implementation.
> 

Thanks, will remove in v2.

> 
> --
> David Marchand