threads / patch / 41580

patchMark win32's pthread_exit() as NORETURN

Subject: [PATCH] Mark win32's pthread_exit() as NORETURN

## tl;dr

12 messages between Mar 1, 2016 and Mar 2, 2016. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Johannes Schindelin· Mar 1, 2016, 13:53 UTC · lore
The pthread_exit() function is not expected to return. Ever.
Pointed out by Jeff King.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to compat/win32/pthread.h +1 −1
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..148db60 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
 #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
 extern pthread_t pthread_self(void);
 
-static inline int pthread_exit(void *ret)
+static inline int NORETURN pthread_exit(void *ret)
 {
 	ExitThread((DWORD)(intptr_t)ret);
 }
-- 
2.7.2.windows.1.5.g64acc33
Jeff King· Mar 1, 2016, 13:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Mark win32's pthread_exit() as NORETURN

On Tue, Mar 01, 2016 at 02:53:04PM +0100, Johannes Schindelin wrote:
Show 22 quoted lines
> The pthread_exit() function is not expected to return. Ever.
> 
> Pointed out by Jeff King.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/win32/pthread.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 20b35a2..148db60 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
>  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>  extern pthread_t pthread_self(void);
>  
> -static inline int pthread_exit(void *ret)
> +static inline int NORETURN pthread_exit(void *ret)
>  {
>  	ExitThread((DWORD)(intptr_t)ret);
>  }

Looks obviously correct to me (I'll assume Windows isn't so crazy as to let ExitThread ever return :) ).

-Peff
> -- 
> 2.7.2.windows.1.5.g64acc33
Johannes Schindelin· Mar 1, 2016, 14:11 UTC · re: Jeff King · lore

Re: [PATCH] Mark win32's pthread_exit() as NORETURN

Hi Peff,
On Tue, 1 Mar 2016, Jeff King wrote:
Show 27 quoted lines
> On Tue, Mar 01, 2016 at 02:53:04PM +0100, Johannes Schindelin wrote:
> 
> > The pthread_exit() function is not expected to return. Ever.
> > 
> > Pointed out by Jeff King.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  compat/win32/pthread.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> > index 20b35a2..148db60 100644
> > --- a/compat/win32/pthread.h
> > +++ b/compat/win32/pthread.h
> > @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
> >  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
> >  extern pthread_t pthread_self(void);
> >  
> > -static inline int pthread_exit(void *ret)
> > +static inline int NORETURN pthread_exit(void *ret)
> >  {
> >  	ExitThread((DWORD)(intptr_t)ret);
> >  }
> 
> Looks obviously correct to me (I'll assume Windows isn't so crazy as to
> let ExitThread ever return :) ).

Indeed, you are correct in your implicit assumption that I should have clarified that in my commit message. I will amend the commit message.

Ciao, Dscho

Johannes Schindelin· Mar 1, 2016, 14:13 UTC · re: Johannes Schindelin · lore

[PATCH v2] Mark win32's pthread_exit() as NORETURN

The pthread_exit() function is not expected to return. Ever. On Windows, we call ExitThread() whose documentation claims: "This function does not return a value.":

	https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
Pointed out by Jeff King.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Relative to v1, only the commit message changed (to clarify that
	ExitThread() indeed never returns).
 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to compat/win32/pthread.h +1 −1
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..148db60 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
 #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
 extern pthread_t pthread_self(void);
 
-static inline int pthread_exit(void *ret)
+static inline int NORETURN pthread_exit(void *ret)
 {
 	ExitThread((DWORD)(intptr_t)ret);
 }
-- 
2.7.2.windows.1.5.g64acc33
stefan.naewe@atlas-elektronik.com· Mar 1, 2016, 14:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
> The pthread_exit() function is not expected to return. Ever. On Windows,
> we call ExitThread() whose documentation claims: "This function does not
> return a value.":
Does this really mean that ExitThread() does not return ?
Just wondering...
Show 27 quoted lines
>         https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
> 
> Pointed out by Jeff King.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
>         Relative to v1, only the commit message changed (to clarify that
>         ExitThread() indeed never returns).
> 
>  compat/win32/pthread.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 20b35a2..148db60 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
>  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>  extern pthread_t pthread_self(void);
> 
> -static inline int pthread_exit(void *ret)
> +static inline int NORETURN pthread_exit(void *ret)
>  {
>         ExitThread((DWORD)(intptr_t)ret);
>  }
> --
Stefan
-- 
----------------------------------------------------------------
/dev/random says: We're lost, but we're making good time.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')" 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF
Junio C Hamano· Mar 1, 2016, 17:57 UTC · re: stefan.naewe@atlas-elektronik.com · lore

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

<stefan.naewe@atlas-elektronik.com> writes:
Show 8 quoted lines
> Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
>> The pthread_exit() function is not expected to return. Ever. On Windows,
>> we call ExitThread() whose documentation claims: "This function does not
>> return a value.":
>
> Does this really mean that ExitThread() does not return ?
>
> Just wondering...
;-).

That made me re-read the patch and notice another thing... Dscho, I think you would need s/int/void/, too, no?

Show 31 quoted lines
>
>>         https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
>> 
>> Pointed out by Jeff King.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> 
>>         Relative to v1, only the commit message changed (to clarify that
>>         ExitThread() indeed never returns).
>> 
>>  compat/win32/pthread.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
>> index 20b35a2..148db60 100644
>> --- a/compat/win32/pthread.h
>> +++ b/compat/win32/pthread.h
>> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
>>  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>>  extern pthread_t pthread_self(void);
>> 
>> -static inline int pthread_exit(void *ret)
>> +static inline int NORETURN pthread_exit(void *ret)
>>  {
>>         ExitThread((DWORD)(intptr_t)ret);
>>  }
>> --
>
>
> Stefan
Johannes Schindelin· Mar 2, 2016, 19:11 UTC · re: stefan.naewe@atlas-elektronik.com · lore

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

Hi,
On Tue, 1 Mar 2016, stefan.naewe@atlas-elektronik.com wrote:
Show 6 quoted lines
> Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
> > The pthread_exit() function is not expected to return. Ever. On Windows,
> > we call ExitThread() whose documentation claims: "This function does not
> > return a value.":
> 
> Does this really mean that ExitThread() does not return ?
I fixed the commit message in v3.

Thanks, Dscho

Johannes Sixt· Mar 1, 2016, 19:34 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
Show 5 quoted lines
> The pthread_exit() function is not expected to return. Ever. On Windows,
> we call ExitThread() whose documentation claims: "This function does not
> return a value.":
>
> 	https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659

This is misleading: MSDN marks all functions declared void as "does not return a value," for example, look at EnterCriticalSection:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682608

For this reason, I actually prefer your version 1 patch without the explanation.

Show 22 quoted lines
>
> Pointed out by Jeff King.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	Relative to v1, only the commit message changed (to clarify that
> 	ExitThread() indeed never returns).
>
>   compat/win32/pthread.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 20b35a2..148db60 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
>   #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>   extern pthread_t pthread_self(void);
>
> -static inline int pthread_exit(void *ret)
> +static inline int NORETURN pthread_exit(void *ret)
I would have written it as

#ifdef __GNUC__ __attribute__((__noreturn__)) #endif static inline int pthread_exit(void *ret) ...

but I can live with your version as long as it compiles.

Your solution is pragmatic: NORETURN is defined in git-compat-util.h, and by using it here, we depend on that pthread.h is included sufficiently late that the macro is available at this point. The instance in compat/nedmalloc/malloc.c.h is bracketed with #ifndef WIN32 so that it is not compiled on Windows, all other instances are after git-compat-util.h or cache.h or in headers that are to be included only after git-compat-util.h or cache.h per convention. Looks like we are safe.

>   {
>   	ExitThread((DWORD)(intptr_t)ret);
>   }
>
Junio C Hamano· Mar 1, 2016, 19:44 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

Johannes Sixt <j6t@kdbg.org> writes:
Show 14 quoted lines
> Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
>> The pthread_exit() function is not expected to return. Ever. On Windows,
>> we call ExitThread() whose documentation claims: "This function does not
>> return a value.":
>>
>> 	https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
>
> This is misleading: MSDN marks all functions declared void as "does
> not return a value," for example, look at EnterCriticalSection:
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682608
>
> For this reason, I actually prefer your version 1 patch without the
> explanation.
;-)
Show 11 quoted lines
>> -static inline int pthread_exit(void *ret)
>> +static inline int NORETURN pthread_exit(void *ret)
>
> I would have written it as
>
> #ifdef __GNUC__
> __attribute__((__noreturn__))
> #endif
> static inline int pthread_exit(void *ret) ...
>
> but I can live with your version as long as it compiles.

Either way, let's make sure that the final version returns "void", cf.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_exit.html
Thanks.
Johannes Schindelin· Mar 2, 2016, 19:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

Hi,
On Tue, 1 Mar 2016, Junio C Hamano wrote:
Show 18 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
> >> The pthread_exit() function is not expected to return. Ever. On Windows,
> >> we call ExitThread() whose documentation claims: "This function does not
> >> return a value.":
> >>
> >> 	https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
> >
> > This is misleading: MSDN marks all functions declared void as "does
> > not return a value," for example, look at EnterCriticalSection:
> >
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms682608
> >
> > For this reason, I actually prefer your version 1 patch without the
> > explanation.
> 
> ;-)

Well, I really like to have that link so I can find it very easily myself by simply inspecting the Git log. And the explanation itself was easily fixed.

Show 17 quoted lines
> 
> >> -static inline int pthread_exit(void *ret)
> >> +static inline int NORETURN pthread_exit(void *ret)
> >
> > I would have written it as
> >
> > #ifdef __GNUC__
> > __attribute__((__noreturn__))
> > #endif
> > static inline int pthread_exit(void *ret) ...
> >
> > but I can live with your version as long as it compiles.
> 
> Either way, let's make sure that the final version returns "void",
> cf.
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_exit.html
Fixed, too, in the upcoming v3.

Ciao, Dscho

Johannes Schindelin· Mar 2, 2016, 19:11 UTC · re: Johannes Schindelin · lore

[PATCH v3] Mark win32's pthread_exit() as NORETURN

The pthread_exit() function is not expected to return. Ever. On Windows, we call ExitThread() whose documentation claims: "Ends the calling thread", i.e. there is no condition in which this function simply returns: https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659

While at it, fix the return type to be void, as per http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_exit.html

Pointed out by Jeff King, helped by Stefan Naewe, Junio Hamano & Johannes Sixt.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Interdiff vs v2:
 diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
 index 148db60..b6ed9e7 100644
 --- a/compat/win32/pthread.h
 +++ b/compat/win32/pthread.h
 @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
  extern pthread_t pthread_self(void);
  
 -static inline int NORETURN pthread_exit(void *ret)
 +static inline void NORETURN pthread_exit(void *ret)
  {
  	ExitThread((DWORD)(intptr_t)ret);
  }
Show changes to compat/win32/pthread.h +1 −1
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..b6ed9e7 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
 #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
 extern pthread_t pthread_self(void);
 
-static inline int pthread_exit(void *ret)
+static inline void NORETURN pthread_exit(void *ret)
 {
 	ExitThread((DWORD)(intptr_t)ret);
 }
-- 
2.7.2.windows.1.5.g64acc33
Junio C Hamano· Mar 2, 2016, 20:33 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3] Mark win32's pthread_exit() as NORETURN

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 13 quoted lines
> The pthread_exit() function is not expected to return. Ever. On Windows,
> we call ExitThread() whose documentation claims: "Ends the calling
> thread", i.e. there is no condition in which this function simply
> returns: https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
>
> While at it, fix the return type to be void, as per
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_exit.html
>
> Pointed out by Jeff King, helped by Stefan Naewe, Junio Hamano &
> Johannes Sixt.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Thanks, will queue.
Show 32 quoted lines
>  compat/win32/pthread.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> Interdiff vs v2:
>
>  diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
>  index 148db60..b6ed9e7 100644
>  --- a/compat/win32/pthread.h
>  +++ b/compat/win32/pthread.h
>  @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
>   #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>   extern pthread_t pthread_self(void);
>   
>  -static inline int NORETURN pthread_exit(void *ret)
>  +static inline void NORETURN pthread_exit(void *ret)
>   {
>   	ExitThread((DWORD)(intptr_t)ret);
>   }
>
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 20b35a2..b6ed9e7 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void **value_ptr);
>  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>  extern pthread_t pthread_self(void);
>  
> -static inline int pthread_exit(void *ret)
> +static inline void NORETURN pthread_exit(void *ret)
>  {
>  	ExitThread((DWORD)(intptr_t)ret);
>  }

← back to recent threads