[BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Randall S. Becker]

On Wed, 17 Feb 2021 21:48:36, Jeff Hostetler wrote:

>Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT
>shutting down the server to make unit tests more predictable on CI servers.
>(https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev)

I missed this at the time, but it appears that ipc-unix-socket.c forces a dependency on pthreads for Git under Unix-like platforms.
This is probably not a correct assumption (or likely intended), but causes git to no longer build on NonStop x86 and ia64 as of
2.32.0-rc0. I am not suggesting undoing this, but amending to make the change more sensitive to a lack of pthread support.
pthread_sigmask() showed up as an undefined external:

**** ERROR **** [1210]:
   libgit.a(ipc-unix-socket.o): In function `thread_block_sigpipe':
   ipc-unix-socket.o(.text+0xb87): unresolved reference to pthread_sigmask.

On NonStop, pthread_sigmask is defined in -lput or -lspt, which are not used in our build – and would cause a bunch of other issues
if referenced. The build does define NO_PTHREADS. 

A simple, but probably wrong fix to get the build to work is:

diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
index 38689b278d..07b2c407c1 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -535,7 +535,9 @@ static void thread_block_sigpipe(sigset_t *old_set)
        sigaddset(&new_set, SIGPIPE);

        sigemptyset(old_set);
+#ifndef NO_PTHREADS
        pthread_sigmask(SIG_BLOCK, &new_set, old_set);
+#endif
 }

But I suspect that this will not perform the desired action associated with blocking the signal, although since we are not in a
threading situation, that might be fine.

Thoughts?

Randall

-- Brief whoami:
NonStop developer since approximately 211288444200000000
UNIX developer since approximately 421664400
IBM developer since "I'm not talking about it"
-- In my real life, I talk too much.

Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Jeff King]

On Mon, May 17, 2021 at 01:46:46PM -0400, Randall S. Becker wrote:

> On Wed, 17 Feb 2021 21:48:36, Jeff Hostetler wrote:
> 
> >Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT
> >shutting down the server to make unit tests more predictable on CI servers.
> >(https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev)
> 
> I missed this at the time, but it appears that ipc-unix-socket.c
> forces a dependency on pthreads for Git under Unix-like platforms.
> This is probably not a correct assumption (or likely intended), but
> causes git to no longer build on NonStop x86 and ia64 as of
> 2.32.0-rc0. I am not suggesting undoing this, but amending to make the
> change more sensitive to a lack of pthread support.
> pthread_sigmask() showed up as an undefined external:

Hrm. Usually we do not assume that threads are available. For "async"
stuff via run-command, we allow it to be implemented via fork(), and
insist that the async process talk back to us only over a pipe
descriptor (so it works whether it's a thread or a separate process).
In cases where we use worker threads for performance (like index-pack or
pack-objects), we just run a single "thread" instead, waiting for it to
complete.

In the simple-ipc API, there's an explicit "async" interface. But it's
not clear to me how rich it expects the communication with the caller to
be (i.e., whether we could get away with the fork() trick here). Or if
it would be OK for the threading to remain an implementation detail,
with one "worker" upon whom we wait for completion.

> **** ERROR **** [1210]:
>    libgit.a(ipc-unix-socket.o): In function `thread_block_sigpipe':
>    ipc-unix-socket.o(.text+0xb87): unresolved reference to pthread_sigmask.
> 
> On NonStop, pthread_sigmask is defined in -lput or -lspt, which are not used in our build – and would cause a bunch of other issues
> if referenced. The build does define NO_PTHREADS.

So yeah, you hit that problem because you only have a
sort-of-pthreads-ish case. But it seems like a system which truly has no
pthread support at all and defines NO_PTHREADS to tell us so will have
much more of its compilation broken (because it's also missing obvious
bits like pthread_create()).

We already make simple-ipc compilation conditional on NO_UNIX_SOCKETS. I
think we could probably just do the same for NO_PTHREADS?

Something like:

diff --git a/Makefile b/Makefile
index 3a2d3c80a8..bd7fe0fc24 100644
--- a/Makefile
+++ b/Makefile
@@ -1687,14 +1687,20 @@ ifdef NO_UNIX_SOCKETS
 else
 	LIB_OBJS += unix-socket.o
 	LIB_OBJS += unix-stream-server.o
+endif
+
+# All simple-ipc requires threads, and then individual
+# mechanisms have their own requirements.
+ifndef NO_PTHREADS
+	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
 	LIB_OBJS += compat/simple-ipc/ipc-shared.o
+ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
 endif
-
 ifdef USE_WIN32_IPC
-	LIB_OBJS += compat/simple-ipc/ipc-shared.o
 	LIB_OBJS += compat/simple-ipc/ipc-win32.o
 endif
+endif
 
 ifdef NO_ICONV
 	BASIC_CFLAGS += -DNO_ICONV
diff --git a/simple-ipc.h b/simple-ipc.h
index dc3606e30b..0f58be7945 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -4,11 +4,6 @@
 /*
  * See Documentation/technical/api-simple-ipc.txt
  */
-
-#if defined(GIT_WINDOWS_NATIVE) || !defined(NO_UNIX_SOCKETS)
-#define SUPPORTS_SIMPLE_IPC
-#endif
-
 #ifdef SUPPORTS_SIMPLE_IPC
 #include "pkt-line.h"
 

Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Jeff Hostetler]

On 5/18/21 4:23 AM, Jeff King wrote:
> On Mon, May 17, 2021 at 01:46:46PM -0400, Randall S. Becker wrote:
> 
>> On Wed, 17 Feb 2021 21:48:36, Jeff Hostetler wrote:
>>
>>> Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT
>>> shutting down the server to make unit tests more predictable on CI servers.
>>> (https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev)
>>
>> I missed this at the time, but it appears that ipc-unix-socket.c
>> forces a dependency on pthreads for Git under Unix-like platforms.
>> This is probably not a correct assumption (or likely intended), but
>> causes git to no longer build on NonStop x86 and ia64 as of
>> 2.32.0-rc0. I am not suggesting undoing this, but amending to make the
>> change more sensitive to a lack of pthread support.
>> pthread_sigmask() showed up as an undefined external:
> 
> Hrm. Usually we do not assume that threads are available. For "async"
> stuff via run-command, we allow it to be implemented via fork(), and
> insist that the async process talk back to us only over a pipe
> descriptor (so it works whether it's a thread or a separate process).
> In cases where we use worker threads for performance (like index-pack or
> pack-objects), we just run a single "thread" instead, waiting for it to
> complete.
> 
> In the simple-ipc API, there's an explicit "async" interface. But it's
> not clear to me how rich it expects the communication with the caller to
> be (i.e., whether we could get away with the fork() trick here). Or if
> it would be OK for the threading to remain an implementation detail,
> with one "worker" upon whom we wait for completion.
> 

TBH I forgot that we still support NO_PTHREAD systems.
I seem to remember that we got rid of some of the non-pthread
stub functions at one point, but I'm fuzzy on the details.

WRT to "simple ipc" (and future "builtin fsmonitor"), it's heavily
threaded.  There's no point in trying to fake it with forks.

The server side of simple ipc implements a thread pool.  And
the builtin fsmonitor will use a thread to monitor FS events
and that thread pool to respond to clients.  All driven from a
shared queue of events.

It would be a major overhaul to do all that without threads
-- and even that assumes that nonstop has a sufficient file
system notification mechanism.

So, yes, we should ifdef it out as Peff suggests.

Jeff



>> **** ERROR **** [1210]:
>>     libgit.a(ipc-unix-socket.o): In function `thread_block_sigpipe':
>>     ipc-unix-socket.o(.text+0xb87): unresolved reference to pthread_sigmask.
>>
>> On NonStop, pthread_sigmask is defined in -lput or -lspt, which are not used in our build – and would cause a bunch of other issues
>> if referenced. The build does define NO_PTHREADS.
> 
> So yeah, you hit that problem because you only have a
> sort-of-pthreads-ish case. But it seems like a system which truly has no
> pthread support at all and defines NO_PTHREADS to tell us so will have
> much more of its compilation broken (because it's also missing obvious
> bits like pthread_create()).
> 
> We already make simple-ipc compilation conditional on NO_UNIX_SOCKETS. I
> think we could probably just do the same for NO_PTHREADS?
> 
> Something like:
> 
> diff --git a/Makefile b/Makefile
> index 3a2d3c80a8..bd7fe0fc24 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1687,14 +1687,20 @@ ifdef NO_UNIX_SOCKETS
>   else
>   	LIB_OBJS += unix-socket.o
>   	LIB_OBJS += unix-stream-server.o
> +endif
> +
> +# All simple-ipc requires threads, and then individual
> +# mechanisms have their own requirements.
> +ifndef NO_PTHREADS
> +	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
>   	LIB_OBJS += compat/simple-ipc/ipc-shared.o
> +ifndef NO_UNIX_SOCKETS
>   	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
>   endif
> -
>   ifdef USE_WIN32_IPC
> -	LIB_OBJS += compat/simple-ipc/ipc-shared.o
>   	LIB_OBJS += compat/simple-ipc/ipc-win32.o
>   endif
> +endif
>   
>   ifdef NO_ICONV
>   	BASIC_CFLAGS += -DNO_ICONV
> diff --git a/simple-ipc.h b/simple-ipc.h
> index dc3606e30b..0f58be7945 100644
> --- a/simple-ipc.h
> +++ b/simple-ipc.h
> @@ -4,11 +4,6 @@
>   /*
>    * See Documentation/technical/api-simple-ipc.txt
>    */
> -
> -#if defined(GIT_WINDOWS_NATIVE) || !defined(NO_UNIX_SOCKETS)
> -#define SUPPORTS_SIMPLE_IPC
> -#endif
> -
>   #ifdef SUPPORTS_SIMPLE_IPC
>   #include "pkt-line.h"
>   
> 

Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Jeff King]

On Tue, May 18, 2021 at 07:21:33AM -0400, Jeff Hostetler wrote:

> > In the simple-ipc API, there's an explicit "async" interface. But it's
> > not clear to me how rich it expects the communication with the caller to
> > be (i.e., whether we could get away with the fork() trick here). Or if
> > it would be OK for the threading to remain an implementation detail,
> > with one "worker" upon whom we wait for completion.
> > 
> 
> TBH I forgot that we still support NO_PTHREAD systems.
> I seem to remember that we got rid of some of the non-pthread
> stub functions at one point, but I'm fuzzy on the details.

You're probably thinking of when we got rid of a bunch of #ifdef code
paths in index-pack, and replaced it with stubs that turn the pthread
calls into "do nothing" (so all the ugly stuff is in thread-utils.h
now). But we still very much support systems that don't handle pthreads
at all.

> WRT to "simple ipc" (and future "builtin fsmonitor"), it's heavily
> threaded.  There's no point in trying to fake it with forks.
> 
> The server side of simple ipc implements a thread pool.  And
> the builtin fsmonitor will use a thread to monitor FS events
> and that thread pool to respond to clients.  All driven from a
> shared queue of events.
> 
> It would be a major overhaul to do all that without threads
> -- and even that assumes that nonstop has a sufficient file
> system notification mechanism.

OK, that matches my guess from a brief look at the code. Thanks for
confirming.

> So, yes, we should ifdef it out as Peff suggests.

The patch I sent wasn't really tested beyond confirming that "make
NO_PTHREADS=1" finished compiling (and that test-tool simple-ipc
barfed appropriately at runtime).

Do you want to pick it up from there and produce a polished patch? I
think we should deal with this prior to the v2.32.0 release (and thanks
Randall for testing and finding it during the -rc0 period).

-Peff

RE: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Randall S. Becker]

On May 18, 2021 4:24 AM, Jeff King wrote:
>On Mon, May 17, 2021 at 01:46:46PM -0400, Randall S. Becker wrote:
>
>> On Wed, 17 Feb 2021 21:48:36, Jeff Hostetler wrote:
>>
>> >Here is V4 of my "Simple IPC" series. It addresses Gábor's comment
>> >WRT shutting down the server to make unit tests more predictable on CI servers.
>> >(https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev)
>>
>> I missed this at the time, but it appears that ipc-unix-socket.c
>> forces a dependency on pthreads for Git under Unix-like platforms.
>> This is probably not a correct assumption (or likely intended), but
>> causes git to no longer build on NonStop x86 and ia64 as of
>> 2.32.0-rc0. I am not suggesting undoing this, but amending to make the
>> change more sensitive to a lack of pthread support.
>> pthread_sigmask() showed up as an undefined external:
>
>Hrm. Usually we do not assume that threads are available. For "async"
>stuff via run-command, we allow it to be implemented via fork(), and insist that the async process talk back to us only over a pipe
>descriptor (so it works whether it's a thread or a separate process).
>In cases where we use worker threads for performance (like index-pack or pack-objects), we just run a single "thread" instead, waiting for
>it to complete.
>
>In the simple-ipc API, there's an explicit "async" interface. But it's not clear to me how rich it expects the communication with the caller to
>be (i.e., whether we could get away with the fork() trick here). Or if it would be OK for the threading to remain an implementation detail,
>with one "worker" upon whom we wait for completion.
>
>> **** ERROR **** [1210]:
>>    libgit.a(ipc-unix-socket.o): In function `thread_block_sigpipe':
>>    ipc-unix-socket.o(.text+0xb87): unresolved reference to pthread_sigmask.
>>
>> On NonStop, pthread_sigmask is defined in -lput or -lspt, which are
>> not used in our build – and would cause a bunch of other issues if referenced. The build does define NO_PTHREADS.
>
>So yeah, you hit that problem because you only have a sort-of-pthreads-ish case. But it seems like a system which truly has no pthread
>support at all and defines NO_PTHREADS to tell us so will have much more of its compilation broken (because it's also missing obvious bits
>like pthread_create()).
>
>We already make simple-ipc compilation conditional on NO_UNIX_SOCKETS. I think we could probably just do the same for
>NO_PTHREADS?
>
>Something like:
>
>diff --git a/Makefile b/Makefile
>index 3a2d3c80a8..bd7fe0fc24 100644
>--- a/Makefile
>+++ b/Makefile
>@@ -1687,14 +1687,20 @@ ifdef NO_UNIX_SOCKETS  else
> 	LIB_OBJS += unix-socket.o
> 	LIB_OBJS += unix-stream-server.o
>+endif
>+
>+# All simple-ipc requires threads, and then individual # mechanisms
>+have their own requirements.
>+ifndef NO_PTHREADS
>+	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
> 	LIB_OBJS += compat/simple-ipc/ipc-shared.o
>+ifndef NO_UNIX_SOCKETS
> 	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
> endif
>-
> ifdef USE_WIN32_IPC
>-	LIB_OBJS += compat/simple-ipc/ipc-shared.o
> 	LIB_OBJS += compat/simple-ipc/ipc-win32.o  endif
>+endif
>
> ifdef NO_ICONV
> 	BASIC_CFLAGS += -DNO_ICONV
>diff --git a/simple-ipc.h b/simple-ipc.h index dc3606e30b..0f58be7945 100644
>--- a/simple-ipc.h
>+++ b/simple-ipc.h
>@@ -4,11 +4,6 @@
> /*
>  * See Documentation/technical/api-simple-ipc.txt
>  */
>-
>-#if defined(GIT_WINDOWS_NATIVE) || !defined(NO_UNIX_SOCKETS) -#define SUPPORTS_SIMPLE_IPC -#endif
>-
> #ifdef SUPPORTS_SIMPLE_IPC
> #include "pkt-line.h"

I'm not sure this is going to work. The platform *does* support UNIX sockets (and not disabled) and pthreads, but we have disabled pthreads in our build. So in the above, ipc-unix-socket.o will be included at the ifndef NO_UNIX_SOCKETS. If NO_PTHREADS, not being pedantic, there should be no pthread references, regardless of other considerations. Although, at some point, I hope to resolve why pthreads (PUT) is having issues in git on the platform but not at this point.

Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Jeff Hostetler]

On 5/18/21 8:11 AM, Jeff King wrote:
> On Tue, May 18, 2021 at 07:21:33AM -0400, Jeff Hostetler wrote:
> 
>>> In the simple-ipc API, there's an explicit "async" interface. But it's
>>> not clear to me how rich it expects the communication with the caller to
>>> be (i.e., whether we could get away with the fork() trick here). Or if
>>> it would be OK for the threading to remain an implementation detail,
>>> with one "worker" upon whom we wait for completion.
>>>
>>
>> TBH I forgot that we still support NO_PTHREAD systems.
>> I seem to remember that we got rid of some of the non-pthread
>> stub functions at one point, but I'm fuzzy on the details.
> 
> You're probably thinking of when we got rid of a bunch of #ifdef code
> paths in index-pack, and replaced it with stubs that turn the pthread
> calls into "do nothing" (so all the ugly stuff is in thread-utils.h
> now). But we still very much support systems that don't handle pthreads
> at all.
> 
>> WRT to "simple ipc" (and future "builtin fsmonitor"), it's heavily
>> threaded.  There's no point in trying to fake it with forks.
>>
>> The server side of simple ipc implements a thread pool.  And
>> the builtin fsmonitor will use a thread to monitor FS events
>> and that thread pool to respond to clients.  All driven from a
>> shared queue of events.
>>
>> It would be a major overhaul to do all that without threads
>> -- and even that assumes that nonstop has a sufficient file
>> system notification mechanism.
> 
> OK, that matches my guess from a brief look at the code. Thanks for
> confirming.
> 
>> So, yes, we should ifdef it out as Peff suggests.
> 
> The patch I sent wasn't really tested beyond confirming that "make
> NO_PTHREADS=1" finished compiling (and that test-tool simple-ipc
> barfed appropriately at runtime).
> 
> Do you want to pick it up from there and produce a polished patch? I
> think we should deal with this prior to the v2.32.0 release (and thanks
> Randall for testing and finding it during the -rc0 period).
> 
> -Peff
> 

yeah, i'll take it from here and get a patch out today.

Thanks!
Jeff

Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)

[Jeff King]

On Tue, May 18, 2021 at 09:37:38AM -0400, Randall S. Becker wrote:

> >+# All simple-ipc requires threads, and then individual # mechanisms
> >+have their own requirements.
> >+ifndef NO_PTHREADS
> >+	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
> > 	LIB_OBJS += compat/simple-ipc/ipc-shared.o
> >+ifndef NO_UNIX_SOCKETS
> > 	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
> > endif
> >-
> > ifdef USE_WIN32_IPC
> >-	LIB_OBJS += compat/simple-ipc/ipc-shared.o
> > 	LIB_OBJS += compat/simple-ipc/ipc-win32.o  endif
> >+endif
> >
> > ifdef NO_ICONV
> > 	BASIC_CFLAGS += -DNO_ICONV
> >diff --git a/simple-ipc.h b/simple-ipc.h index dc3606e30b..0f58be7945 100644
> >--- a/simple-ipc.h
> >+++ b/simple-ipc.h
> >@@ -4,11 +4,6 @@
> > /*
> >  * See Documentation/technical/api-simple-ipc.txt
> >  */
> >-
> >-#if defined(GIT_WINDOWS_NATIVE) || !defined(NO_UNIX_SOCKETS) -#define SUPPORTS_SIMPLE_IPC -#endif
> >-
> > #ifdef SUPPORTS_SIMPLE_IPC
> > #include "pkt-line.h"
> 
> I'm not sure this is going to work. The platform *does* support UNIX
> sockets (and not disabled) and pthreads, but we have disabled pthreads
> in our build. So in the above, ipc-unix-socket.o will be included at
> the ifndef NO_UNIX_SOCKETS. If NO_PTHREADS, not being pedantic, there
> should be no pthread references, regardless of other considerations.
> Although, at some point, I hope to resolve why pthreads (PUT) is
> having issues in git on the platform but not at this point.

Unless I screwed something up, it shouldn't be. There's an outer ifndef
for NO_PTHREADS. Double negation aside, that means that we don't even
hit the ifndef NO_UNIX_SOCKETS inside it unless we now pthreads are
supported. And so we do not include ipc-unix-socket.o.

For the SUPPORTS_SIMPLE_IPC flag, I just moved the logic into the
Makefile. It could continue to live in simple-ipc.h, too (but then that
is basically a repetition of the Makefile logic; it would have to learn
the same "only if pthreads are available" conditional, too).

-Peff