[Tarantool-patches] [PATCH] Add a check whether glibc is used

[Tarantool-patches] [PATCH] Add a check whether glibc is used

[Leonid Vasiliev]

The cbus hang test uses glibc pthread mutex implementation details.
Therefore, it should not compile in case of using another library.
---
Now the compilation for alpine 3.5 is broken.

https://github.com/tarantool/tarantool/tree/lvasiliev/gh-noticket-glibc-check

 test/unit/CMakeLists.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 24586c2..699cd8c 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -104,7 +104,9 @@ target_link_libraries(cbus_stress.test core stat)
 add_executable(cbus.test cbus.c)
 target_link_libraries(cbus.test core unit stat)
 
-if (${CMAKE_HOST_SYSTEM_NAME} MATCHES "Linux")
+include(CheckSymbolExists)
+check_symbol_exists(__GLIBC__ stdio.h GLIBC_USED)
+if (GLIBC_USED)
     add_executable(cbus_hang.test cbus_hang.c)
     target_link_libraries(cbus_hang.test core unit stat)
 endif ()
-- 
2.7.4

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Cyrill Gorcunov]

On Tue, Apr 21, 2020 at 08:44:57PM +0300, Leonid Vasiliev wrote:
> The cbus hang test uses glibc pthread mutex implementation details.
> Therefore, it should not compile in case of using another library.
> ---
> Now the compilation for alpine 3.5 is broken.

Wait, I don't understand. We'are using tt_pthread in core code
as well. How tarantool compiles on it then?

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[lvasiliev]

Hi!

On 22.04.2020 17:20, Cyrill Gorcunov wrote:
> On Tue, Apr 21, 2020 at 08:44:57PM +0300, Leonid Vasiliev wrote:
>> The cbus hang test uses glibc pthread mutex implementation details.
>> Therefore, it should not compile in case of using another library.
>> ---
>> Now the compilation for alpine 3.5 is broken.
> 
> Wait, I don't understand. We'are using tt_pthread in core code
> as well. How tarantool compiles on it then?
The cbus hang test uses glibc pthread mutex implementation details
(internal fields of mutex).
Comment from code of cbus_hang test:
" We want to cancel canceled thread in the moment of cpipe_flush_cb
  will be processing.
  A Linux specific dirty hack will be used for reproduce the bug.
  We need to synchronize the main thread and the canceled worker thread.
  So, do it using the endpoint's mutex internal field(__data.__lock)."
"A Linux specific" - a glibc specific.

> 

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Cyrill Gorcunov]

On Wed, Apr 22, 2020 at 05:52:18PM +0300, lvasiliev wrote:
> Comment from code of cbus_hang test:
> " We want to cancel canceled thread in the moment of cpipe_flush_cb
>  will be processing.
>  A Linux specific dirty hack will be used for reproduce the bug.
>  We need to synchronize the main thread and the canceled worker thread.
>  So, do it using the endpoint's mutex internal field(__data.__lock)."
> "A Linux specific" - a glibc specific.

Aha! So it is test's specific. Pleas add this into changelog itself.
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[lvasiliev]

Hi! Thanks for the feedback.

On 22.04.2020 18:36, Cyrill Gorcunov wrote:
> On Wed, Apr 22, 2020 at 05:52:18PM +0300, lvasiliev wrote:
>> Comment from code of cbus_hang test:
>> " We want to cancel canceled thread in the moment of cpipe_flush_cb
>>   will be processing.
>>   A Linux specific dirty hack will be used for reproduce the bug.
>>   We need to synchronize the main thread and the canceled worker thread.
>>   So, do it using the endpoint's mutex internal field(__data.__lock)."
>> "A Linux specific" - a glibc specific.
> 
> Aha! So it is test's specific. Pleas add this into changelog itself.
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
Updated log message:

commit 3e21641638075fe0b8ad909a6a3ce0b064695906
Author: Leonid Vasiliev <lvasiliev@tarantool.org>
Date:   Tue Apr 21 19:16:17 2020 +0300

     Add a check whether glibc is used

     The cbus hang test uses glibc pthread mutex implementation details.
     The reason why mutex implementation details is used:
     "For the bug reproducing the canceled thread must be canceled
     during processing cpipe_flush_cb. We need to synchronize
     the main thread and the canceled worker thread for that.
     So, thread synchronization has been realized by means of
     endpoint's mutex internal field(__data.__lock)."
     Therefore, it should not compile in case of using another library.

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Cyrill Gorcunov]

On Thu, Apr 23, 2020 at 12:10:25PM +0300, lvasiliev wrote:
> 
> commit 3e21641638075fe0b8ad909a6a3ce0b064695906
> Author: Leonid Vasiliev <lvasiliev@tarantool.org>
> Date:   Tue Apr 21 19:16:17 2020 +0300
> 
>     Add a check whether glibc is used
> 
>     The cbus hang test uses glibc pthread mutex implementation details.
>     The reason why mutex implementation details is used:
>     "For the bug reproducing the canceled thread must be canceled
>     during processing cpipe_flush_cb. We need to synchronize
>     the main thread and the canceled worker thread for that.
>     So, thread synchronization has been realized by means of
>     endpoint's mutex internal field(__data.__lock)."
>     Therefore, it should not compile in case of using another library.

Thank you!

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Alexander Turenko]

LGTM.

Several minor comments are below.

> Add a check whether glibc is used

Please, add 'build' prefix.

On Tue, Apr 21, 2020 at 08:44:57PM +0300, Leonid Vasiliev wrote:
> The cbus hang test uses glibc pthread mutex implementation details.
> Therefore, it should not compile in case of using another library.

Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>

> ---
> Now the compilation for alpine 3.5 is broken.

It worth to include certain motivation right into the commit message:
fix compilation on Alpine 3.5 is this case.

> 
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-noticket-glibc-check

I would check it on '-full-ci' branch just in case.

> 
>  test/unit/CMakeLists.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> index 24586c2..699cd8c 100644
> --- a/test/unit/CMakeLists.txt
> +++ b/test/unit/CMakeLists.txt
> @@ -104,7 +104,9 @@ target_link_libraries(cbus_stress.test core stat)
>  add_executable(cbus.test cbus.c)
>  target_link_libraries(cbus.test core unit stat)
>  
> -if (${CMAKE_HOST_SYSTEM_NAME} MATCHES "Linux")
> +include(CheckSymbolExists)
> +check_symbol_exists(__GLIBC__ stdio.h GLIBC_USED)

On my system the macro is defined in features.h.

> +if (GLIBC_USED)
>      add_executable(cbus_hang.test cbus_hang.c)
>      target_link_libraries(cbus_hang.test core unit stat)
>  endif ()

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[lvasiliev]

On 23.04.2020 13:52, Alexander Turenko wrote:
> LGTM.
> 
> Several minor comments are below.
> 
>> Add a check whether glibc is used
> 
> Please, add 'build' prefix.
> 
> On Tue, Apr 21, 2020 at 08:44:57PM +0300, Leonid Vasiliev wrote:
>> The cbus hang test uses glibc pthread mutex implementation details.
>> Therefore, it should not compile in case of using another library.
> 
> Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> 
>> ---
>> Now the compilation for alpine 3.5 is broken.
> 
> It worth to include certain motivation right into the commit message:
> fix compilation on Alpine 3.5 is this case.
> 
>>
>> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-noticket-glibc-check
> 
> I would check it on '-full-ci' branch just in case.
> 
>>
>>   test/unit/CMakeLists.txt | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
>> index 24586c2..699cd8c 100644
>> --- a/test/unit/CMakeLists.txt
>> +++ b/test/unit/CMakeLists.txt
>> @@ -104,7 +104,9 @@ target_link_libraries(cbus_stress.test core stat)
>>   add_executable(cbus.test cbus.c)
>>   target_link_libraries(cbus.test core unit stat)
>>   
>> -if (${CMAKE_HOST_SYSTEM_NAME} MATCHES "Linux")
>> +include(CheckSymbolExists)
>> +check_symbol_exists(__GLIBC__ stdio.h GLIBC_USED)
> 
> On my system the macro is defined in features.h.
> 
>> +if (GLIBC_USED)
>>       add_executable(cbus_hang.test cbus_hang.c)
>>       target_link_libraries(cbus_hang.test core unit stat)
>>   endif ()
Updated.

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 699cd8c..84eb066 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -105,7 +105,7 @@ add_executable(cbus.test cbus.c)
  target_link_libraries(cbus.test core unit stat)

  include(CheckSymbolExists)
-check_symbol_exists(__GLIBC__ stdio.h GLIBC_USED)
+check_symbol_exists(__GLIBC__ features.h GLIBC_USED)
  if (GLIBC_USED)
      add_executable(cbus_hang.test cbus_hang.c)
      target_link_libraries(cbus_hang.test core unit stat)

Message:
commit 7afdd51bf559a27565302067a078996139fec4b8
Author: Leonid Vasiliev <lvasiliev@tarantool.org>
Date:   Tue Apr 21 19:16:17 2020 +0300

     build: fix compilation on Alpine 3.5

     The cbus hang test uses glibc pthread mutex implementation details.
     The reason why mutex implementation details is used:
     "For the bug reproducing the canceled thread must be canceled
     during processing cpipe_flush_cb. We need to synchronize
     the main thread and the canceled worker thread for that.
     So, thread synchronization has been realized by means of
     endpoint's mutex internal field(__data.__lock)."
     Therefore, it should not compile in case of using another library.

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Kirill Yukhin]

Hello,

On 21 апр 20:44, Leonid Vasiliev wrote:
> The cbus hang test uses glibc pthread mutex implementation details.
> Therefore, it should not compile in case of using another library.
> ---
> Now the compilation for alpine 3.5 is broken.
> 
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-noticket-glibc-check
> 
>  test/unit/CMakeLists.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I've checked your patch into master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Kirill Yukhin]

Hello,

On 24 апр 10:24, Kirill Yukhin wrote:
> Hello,
> 
> On 21 апр 20:44, Leonid Vasiliev wrote:
> > The cbus hang test uses glibc pthread mutex implementation details.
> > Therefore, it should not compile in case of using another library.
> > ---
> > Now the compilation for alpine 3.5 is broken.
> > 
> > https://github.com/tarantool/tarantool/tree/lvasiliev/gh-noticket-glibc-check
> > 
> >  test/unit/CMakeLists.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I've checked your patch into master.

Cherry-picked to 1.10, 2.2 and 2.3 as well

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] Add a check whether glibc is used

[Alexander Turenko]

On Fri, Apr 24, 2020 at 10:24:32AM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 21 апр 20:44, Leonid Vasiliev wrote:
> > The cbus hang test uses glibc pthread mutex implementation details.
> > Therefore, it should not compile in case of using another library.
> > ---
> > Now the compilation for alpine 3.5 is broken.
> > 
> > https://github.com/tarantool/tarantool/tree/lvasiliev/gh-noticket-glibc-check
> > 
> >  test/unit/CMakeLists.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I've checked your patch into master.

I had urge to push it downward to 1.10, but found that you already did
this (except EOL 2.1 and 2.2).

So now the build should pass on Alpine on all alive tarantool versions.
This is nice.

WBR, Alexander Turenko.