threads / patch / 42273

patchWindows: only add a no-op pthread_sigmask() when needed

Subject: [PATCH] Windows: only add a no-op pthread_sigmask() when needed

## tl;dr

5 messages between May 10, 2016 and May 11, 2016. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Johannes Schindelin· May 10, 2016, 13:00 UTC · lore

In f924b52 (Windows: add pthread_sigmask() that does nothing, 2016-05-01), we introduced a no-op for Windows. However, this breaks building Git in Git for Windows' SDK because pthread_sigmask() is already a no-op there, #define'd in the pthread_signal.h header in /mingw64/x86_64-w64-mingw32/include/.

Let's guard the definition of pthread_sigmask() in #ifndef...#endif to make the code compile both with modern MinGW-w64 as well as with the previously common MinGW headers.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	This patch is obviously based on 'next' (because 'master' does not
	have the referenced commit yet).
Published-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v1
 compat/win32/pthread.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to compat/win32/pthread.h +2 −0
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index d336451..8df702c 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)
 	return TlsGetValue(key);
 }
 
+#ifndef pthread_sigmask
 static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
 {
 	return 0;
 }
+#endif
 
 #endif /* PTHREAD_H */
-- 
2.8.2.463.g99156ee
Junio C Hamano· May 10, 2016, 17:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 15 quoted lines
> In f924b52 (Windows: add pthread_sigmask() that does nothing,
> 2016-05-01), we introduced a no-op for Windows. However, this breaks
> building Git in Git for Windows' SDK because pthread_sigmask() is
> already a no-op there, #define'd in the pthread_signal.h header in
> /mingw64/x86_64-w64-mingw32/include/.
>
> Let's guard the definition of pthread_sigmask() in #ifndef...#endif to
> make the code compile both with modern MinGW-w64 as well as with the
> previously common MinGW headers.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	This patch is obviously based on 'next' (because 'master' does not
> 	have the referenced commit yet).

One thing that makes me wonder is what would happen when /mingw64/x86_64-w64-mingw32/include/pthread_signal.h changes its mind and uses "static inline" instead of "#define". How much control does Git-for-Windows folks have over that header file?

Also, could you explain "However" part a bit? Obviously in _some_ environment other than "Git for Windows' SDK", the previous patch helped you compile. And you need to #ifdef it out, because "Git for Windows' SDK" already has its own support. What is that other environment that lacks the support (hence we need our own "static inline") and is there a way to tell "Git for Windows' SDK" from that other environment?

What I am trying to get at is:
 (1) If the answer is "we have total control", then I am perfectly
     fine with using "#ifdef pthread_sigmask" here.
 (2) If not, i.e. "they can change the implementation to 'static
     inline' themselves", then I do not think it is prudent to use
     "#ifndef pthread_sigmask" as the conditional here--using a
     symbol that lets you check for that "other" environment and
     doing "#ifdef THAT_OTHER_ONE_THAT_LACKS_SIGMASK" would be
     safer.

Also is https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html relevant? Does /mingw64/x86_64-w64-mingw32/include/ implement "macro only without function"?

Show 20 quoted lines
> Published-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v1
>  compat/win32/pthread.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index d336451..8df702c 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)
>  	return TlsGetValue(key);
>  }
>  
> +#ifndef pthread_sigmask
>  static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
>  {
>  	return 0;
>  }
> +#endif
>  
>  #endif /* PTHREAD_H */
Johannes Schindelin· May 11, 2016, 15:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed

Hi Junio,
On Tue, 10 May 2016, Junio C Hamano wrote:
Show 22 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > In f924b52 (Windows: add pthread_sigmask() that does nothing,
> > 2016-05-01), we introduced a no-op for Windows. However, this breaks
> > building Git in Git for Windows' SDK because pthread_sigmask() is
> > already a no-op there, #define'd in the pthread_signal.h header in
> > /mingw64/x86_64-w64-mingw32/include/.
> >
> > Let's guard the definition of pthread_sigmask() in #ifndef...#endif to
> > make the code compile both with modern MinGW-w64 as well as with the
> > previously common MinGW headers.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	This patch is obviously based on 'next' (because 'master' does not
> > 	have the referenced commit yet).
> 
> One thing that makes me wonder is what would happen when
> /mingw64/x86_64-w64-mingw32/include/pthread_signal.h changes its
> mind and uses "static inline" instead of "#define".  How much
> control does Git-for-Windows folks have over that header file?

We have no control over this, it is defined by one of the MSYS2 packages. (I think those headers come directly from the MinGW-w64 GCC project.)

I can think of two different ways to resolve this, would you please indicate your preference?

1. fix the non-POSIX-ness:
	#ifdef pthread_sigmask
	#undef pthread_sigmask
	#endif
   or even shorter:
	#undef pthread_sigmask
2. just go with whatever MSYS2 provides:
	#ifndef __MINGW64_VERSION_MAJOR
	[... define the no-op ...]
	#endif
> Also, could you explain "However" part a bit?  Obviously in _some_
> environment other than "Git for Windows' SDK", the previous patch
> helped you compile.

Yes, Hannes uses his own MSys environment. (Which is different from everything *I* have, I think, it's not even msysGit.)

Show 11 quoted lines
> What I am trying to get at is:
> 
>  (1) If the answer is "we have total control", then I am perfectly
>      fine with using "#ifdef pthread_sigmask" here.
> 
>  (2) If not, i.e. "they can change the implementation to 'static
>      inline' themselves", then I do not think it is prudent to use
>      "#ifndef pthread_sigmask" as the conditional here--using a
>      symbol that lets you check for that "other" environment and
>      doing "#ifdef THAT_OTHER_ONE_THAT_LACKS_SIGMASK" would be
>      safer.

So I guess that you're preferring my 2. above. Going on that assumption, I will send out another iteration.

> Also is
> https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html
> relevant?  Does /mingw64/x86_64-w64-mingw32/include/ implement "macro
> only without function"?
Yes, it has that problem. Do we care, really?

Ciao, Dscho

Junio C Hamano· May 11, 2016, 21:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> So I guess that you're preferring my 2. above. Going on that assumption, I
> will send out another iteration.
OK.
Show 6 quoted lines
>> Also is
>> https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html
>> relevant?  Does /mingw64/x86_64-w64-mingw32/include/ implement "macro
>> only without function"?
>
> Yes, it has that problem. Do we care, really?

Not really. I was curious how actively this area is evolving; knowing that would help me judge pros-and-cons between your 1 & 2 above.

Johannes Schindelin· May 11, 2016, 15:10 UTC · re: Johannes Schindelin · lore

[PATCH v2] Windows: only add a no-op pthread_sigmask() when needed

In f924b52 (Windows: add pthread_sigmask() that does nothing, 2016-05-01), we introduced a no-op for Windows. However, this breaks building Git in Git for Windows' SDK because pthread_sigmask() is already a no-op there, #define'd in the pthread_signal.h header in /mingw64/x86_64-w64-mingw32/include/.

Let's wrap the definition of pthread_sigmask() in a guard that skips it when compiling with MinGW-w64' headers.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v2
 compat/win32/pthread.h | 2 ++
 1 file changed, 2 insertions(+)
Interdiff vs v1:
 diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
 index 8df702c..1c16408 100644
 --- a/compat/win32/pthread.h
 +++ b/compat/win32/pthread.h
 @@ -104,7 +104,7 @@ static inline void *pthread_getspecific(pthread_key_t key)
  	return TlsGetValue(key);
  }
  
 -#ifndef pthread_sigmask
 +#ifndef __MINGW64_VERSION_MAJOR
  static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
  {
  	return 0;
Show changes to compat/win32/pthread.h +2 −0
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index d336451..1c16408 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)
 	return TlsGetValue(key);
 }
 
+#ifndef __MINGW64_VERSION_MAJOR
 static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
 {
 	return 0;
 }
+#endif
 
 #endif /* PTHREAD_H */
-- 
2.8.2.465.gb077790

← back to recent threads