win32: prefer beginthreadex over CreateThread #1440
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 2378629: |
AtariDreams
force-pushed
the
CreateThread
branch
from
2378629
to
8621afa
Compare
AtariDreams
changed the title
AtariDreams
force-pushed
the
CreateThread
branch
3 times, most recently
from
351c50c
to
f5de6bf
Compare
|
/submit |
|
Submitted as pull.1440.git.git.1674334159116.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Sixt wrote (reply to this): Am 21.01.23 um 21:49 schrieb Rose via GitGitGadget:
> From: Seija Kijin <doremylover123@gmail.com>
>
> Use pthread_exit instead of async_exit.
>
> This means we do not have
> to deal with Windows's implementation
> requiring an unsigned exit coded
> despite the POSIX exit code requiring
> a signed exit code.
>
> Use _beginthreadex instead of CreateThread
> since we use the Windows CRT.
>
> Finally, check for NULL handles, not "INVALID_HANDLE,"
> as _beginthreadex guarantees a valid handle in most cases
This explains *what* the patch does, but not *why*. You replace
CreateThread() in winansi.c, but what has this to do with async_exit and
why must it be changed?
Please take the time to explain this story more thoroughly.
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
> win32: fix thread usage for win32
>
> Use pthread_exit instead of async_exit.
>
> This means we do not have to deal with Windows's implementation
> requiring an unsigned exit coded despite the POSIX exit code requiring a
> signed exit code.
>
> Use _beginthreadex instead of CreateThread since we use the Windows CRT.
>
> Finally, check for NULL handles, not "INVALID_HANDLE," as _beginthreadex
> guarantees a valid handle in most cases
>
> Signed-off-by: Seija Kijin doremylover123@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1440%2FAtariDreams%2FCreateThread-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1440/AtariDreams/CreateThread-v1
> Pull-Request: https://github.com/git/git/pull/1440
>
> compat/mingw.c | 2 +-
> compat/winansi.c | 8 ++++----
> run-command.c | 33 ++++++++++++++-------------------
> 3 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index af397e68a1d..c41d821b382 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2295,7 +2295,7 @@ static int start_timer_thread(void)
> timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> if (timer_event) {
> timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
> - if (!timer_thread )
> + if (!timer_thread)
> return errno = ENOMEM,
> error("cannot start timer thread");
> } else
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 3abe8dd5a27..be65b27bd75 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -340,7 +340,7 @@ enum {
> TEXT = 0, ESCAPE = 033, BRACKET = '['
> };
>
> -static DWORD WINAPI console_thread(LPVOID unused)
> +static unsigned int WINAPI console_thread(LPVOID unused)
> {
> unsigned char buffer[BUFFER_SIZE];
> DWORD bytes;
> @@ -643,9 +643,9 @@ void winansi_init(void)
> die_lasterr("CreateFile for named pipe failed");
>
> /* start console spool thread on the pipe's read end */
> - hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);
> - if (hthread == INVALID_HANDLE_VALUE)
> - die_lasterr("CreateThread(console_thread) failed");
> + hthread = (HANDLE)_beginthreadex(NULL, 0, console_thread, NULL, 0, NULL);
> + if (!hthread)
> + die_lasterr("_beginthreadex(console_thread) failed");
>
> /* schedule cleanup routine */
> if (atexit(winansi_exit))
> diff --git a/run-command.c b/run-command.c
> index 50cc011654e..93fd0d22d4f 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1030,6 +1030,13 @@ static void *run_thread(void *data)
> return (void *)ret;
> }
>
> +int in_async(void)
> +{
> + if (!main_thread_set)
> + return 0; /* no asyncs started yet */
> + return !pthread_equal(main_thread, pthread_self());
> +}
> +
> static NORETURN void die_async(const char *err, va_list params)
> {
> report_fn die_message_fn = get_die_message_routine();
> @@ -1055,18 +1062,6 @@ static int async_die_is_recursing(void)
> return ret != NULL;
> }
>
> -int in_async(void)
> -{
> - if (!main_thread_set)
> - return 0; /* no asyncs started yet */
> - return !pthread_equal(main_thread, pthread_self());
> -}
> -
> -static void NORETURN async_exit(int code)
> -{
> - pthread_exit((void *)(intptr_t)code);
> -}
> -
> #else
>
> static struct {
> @@ -1112,18 +1107,18 @@ int in_async(void)
> return process_is_async;
> }
>
> -static void NORETURN async_exit(int code)
> -{
> - exit(code);
> -}
> -
> #endif
>
> void check_pipe(int err)
> {
> if (err == EPIPE) {
> - if (in_async())
> - async_exit(141);
> + if (in_async()) {
> +#ifdef NO_PTHREADS
> + exit(141);
> +#else
> + pthread_exit((void *)141);
> +#endif
> + }
>
> signal(SIGPIPE, SIG_DFL);
> raise(SIGPIPE);
>
> base-commit: 904d404274fef6695c78a6b055edd184b72e2f9b
|
|
User |
AtariDreams
force-pushed
the
CreateThread
branch
from
f5de6bf
to
4a2c3da
Compare
|
/submit |
|
Submitted as pull.1440.v2.git.git.1674491796648.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
AtariDreams
force-pushed
the
CreateThread
branch
from
4a2c3da
to
5582682
Compare
|
There are issues in commit 5582682: |
AtariDreams
force-pushed
the
CreateThread
branch
from
5582682
to
68baafb
Compare
|
/submit |
|
Submitted as pull.1440.v3.git.git.1674492373925.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
AtariDreams
force-pushed
the
CreateThread
branch
from
68baafb
to
2e2d5ce
Compare
|
/submit |
|
Submitted as pull.1440.v4.git.git.1674492499537.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff Hostetler wrote (reply to this): On 1/23/23 11:48 AM, Rose via GitGitGadget wrote:
> From: Seija Kijin <doremylover123@gmail.com>
> > Use _beginthreadex instead of CreateThread
> since we use the Windows CRT,
> as Microsoft recommends _beginthreadex
> over CreateThread for these situations.
> > Finally, check for NULL handles, not "INVALID_HANDLE,"
> as _beginthreadex guarantees a valid handle in most cases
> > Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
> win32: fix thread usage for win32
> > Use pthread_exit instead of async_exit.
> > This means we do not have to deal with Windows's implementation
> requiring an unsigned exit coded despite the POSIX exit code requiring a
> signed exit code.
> > Use _beginthreadex instead of CreateThread since we use the Windows CRT.
> > Finally, check for NULL handles, not "INVALID_HANDLE," as _beginthreadex
> guarantees a valid handle in most cases
> > Signed-off-by: Seija Kijin doremylover123@gmail.com
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1440%2FAtariDreams%2FCreateThread-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1440/AtariDreams/CreateThread-v4
> Pull-Request: https://github.com/git/git/pull/1440
> > Range-diff vs v3:
> > 1: 68baafba2bd ! 1: 2e2d5ce7745 win32: fix thread usage for win32
> @@ Commit message
> > Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> > - ## compat/mingw.c ##
> -@@ compat/mingw.c: static int start_timer_thread(void)
> - timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> - if (timer_event) {
> - timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
> -- if (!timer_thread )
> -+ if (!timer_thread)
> - return errno = ENOMEM,
> - error("cannot start timer thread");
> - } else
> -
> ## compat/winansi.c ##
> @@ compat/winansi.c: enum {
> TEXT = 0, ESCAPE = 033, BRACKET = '['
> > > compat/winansi.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> > diff --git a/compat/winansi.c b/compat/winansi.c
> index 3abe8dd5a27..be65b27bd75 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -340,7 +340,7 @@ enum {
> TEXT = 0, ESCAPE = 033, BRACKET = '['
> };
> > -static DWORD WINAPI console_thread(LPVOID unused)
> +static unsigned int WINAPI console_thread(LPVOID unused)
> {
> unsigned char buffer[BUFFER_SIZE];
> DWORD bytes;
> @@ -643,9 +643,9 @@ void winansi_init(void)
> die_lasterr("CreateFile for named pipe failed");
> > /* start console spool thread on the pipe's read end */
> - hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);
> - if (hthread == INVALID_HANDLE_VALUE)
> - die_lasterr("CreateThread(console_thread) failed");
> + hthread = (HANDLE)_beginthreadex(NULL, 0, console_thread, NULL, 0, NULL);
> + if (!hthread)
> + die_lasterr("_beginthreadex(console_thread) failed");
> > /* schedule cleanup routine */
> if (atexit(winansi_exit))
> > base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
This change may or may not be harmless, but it scares me
because it is possibly a very subtle change and is being
made for an unknown reason -- is there a problem being
fixed here? Or is this just churn for the sake of churn
to avoid an awkward cast of the return code?
What does _beginthreadex() specifically do that we need
it to do for us?
_beginthreadex() does some CRT init and then calls CreateThread(),
so what are we missing by calling CreateThread() directly?
The code in question is 11+ years old and it hasn't been a
problem (right?), so I have to wonder what value do we get
from this change.
The containing function here is setting up a special console
thread and named pipe to access the console, so I doubt that
any of the tests in the test suite actually would actually
exercise this change (since the tests aren't interactive).
The low-level Windows startup code is very tricky and sensitive
(and we need to test with both GCC's CRT and MSVC's CRT).
As I said earlier, the change may or may not be harmless, but
I question the need for it.
Jeff
|
|
User |
|
On the Git mailing list, Johannes Sixt wrote (reply to this): Am 23.01.23 um 18:43 schrieb Jeff Hostetler:
>
>
> On 1/23/23 11:48 AM, Rose via GitGitGadget wrote:
>> From: Seija Kijin <doremylover123@gmail.com>
>>
>> Use _beginthreadex instead of CreateThread
>> since we use the Windows CRT,
>> as Microsoft recommends _beginthreadex
>> over CreateThread for these situations.
>>
>> Finally, check for NULL handles, not "INVALID_HANDLE,"
>> as _beginthreadex guarantees a valid handle in most cases
>>
>> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
>> ---
>> win32: fix thread usage for win32
>> Use pthread_exit instead of async_exit.
>> This means we do not have to deal with Windows's implementation
>> requiring an unsigned exit coded despite the POSIX exit code
>> requiring a
>> signed exit code.
>> Use _beginthreadex instead of CreateThread since we use the
>> Windows CRT.
>> Finally, check for NULL handles, not "INVALID_HANDLE," as
>> _beginthreadex
>> guarantees a valid handle in most cases
>> Signed-off-by: Seija Kijin doremylover123@gmail.com
>>
>> Published-As:
>> https://github.com/gitgitgadget/git/releases/tag/pr-git-1440%2FAtariDreams%2FCreateThread-v4
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
>> pr-git-1440/AtariDreams/CreateThread-v4
>> Pull-Request: https://github.com/git/git/pull/1440
>>
>> Range-diff vs v3:
>>
>> 1: 68baafba2bd ! 1: 2e2d5ce7745 win32: fix thread usage for win32
>> @@ Commit message
>> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
>> - ## compat/mingw.c ##
>> -@@ compat/mingw.c: static int start_timer_thread(void)
>> - timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
>> - if (timer_event) {
>> - timer_thread = (HANDLE) _beginthreadex(NULL, 0,
>> ticktack, NULL, 0, NULL);
>> -- if (!timer_thread )
>> -+ if (!timer_thread)
>> - return errno = ENOMEM,
>> - error("cannot start timer thread");
>> - } else
>> -
>> ## compat/winansi.c ##
>> @@ compat/winansi.c: enum {
>> TEXT = 0, ESCAPE = 033, BRACKET = '['
>>
>>
>> compat/winansi.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/compat/winansi.c b/compat/winansi.c
>> index 3abe8dd5a27..be65b27bd75 100644
>> --- a/compat/winansi.c
>> +++ b/compat/winansi.c
>> @@ -340,7 +340,7 @@ enum {
>> TEXT = 0, ESCAPE = 033, BRACKET = '['
>> };
>> -static DWORD WINAPI console_thread(LPVOID unused)
>> +static unsigned int WINAPI console_thread(LPVOID unused)
>> {
>> unsigned char buffer[BUFFER_SIZE];
>> DWORD bytes;
>> @@ -643,9 +643,9 @@ void winansi_init(void)
>> die_lasterr("CreateFile for named pipe failed");
>> /* start console spool thread on the pipe's read end */
>> - hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);
>> - if (hthread == INVALID_HANDLE_VALUE)
>> - die_lasterr("CreateThread(console_thread) failed");
>> + hthread = (HANDLE)_beginthreadex(NULL, 0, console_thread, NULL,
>> 0, NULL);
>> + if (!hthread)
>> + die_lasterr("_beginthreadex(console_thread) failed");
>> /* schedule cleanup routine */
>> if (atexit(winansi_exit))
>>
>> base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
>
> This change may or may not be harmless, but it scares me
> because it is possibly a very subtle change and is being
> made for an unknown reason -- is there a problem being
> fixed here? Or is this just churn for the sake of churn
> to avoid an awkward cast of the return code?
>
> What does _beginthreadex() specifically do that we need
> it to do for us?
>
> _beginthreadex() does some CRT init and then calls CreateThread(),
> so what are we missing by calling CreateThread() directly?
I also question the value of this change. As long as the thread does not
call into any CRT functions, we do not need the services of
_beginthreadex(). AFAICS, it only uses WinAPI functions and some
uncritical C functions like memmove and memset. Am I missing something?
>
> The code in question is 11+ years old and it hasn't been a
> problem (right?), so I have to wonder what value do we get
> from this change.
>
> The containing function here is setting up a special console
> thread and named pipe to access the console, so I doubt that
> any of the tests in the test suite actually would actually
> exercise this change (since the tests aren't interactive).
>
> The low-level Windows startup code is very tricky and sensitive
> (and we need to test with both GCC's CRT and MSVC's CRT).
> As I said earlier, the change may or may not be harmless, but
> I question the need for it.
>
> Jeff
>
>
|
AtariDreams
force-pushed
the
CreateThread
branch
from
2e2d5ce
to
6ab79d9
Compare
|
/submit |
|
Submitted as pull.1440.v5.git.git.1675176442381.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Sixt wrote (reply to this): Am 31.01.23 um 15:47 schrieb Rose via GitGitGadget:
> Range-diff vs v4:
>
> 1: 2e2d5ce7745 = 1: 6ab79d9275d win32: fix thread usage for win32
If you do not agree with review comments, it would be more polite to
express your thinking than to just resend a patch unchanged and silently.
-- Hannes
|
AtariDreams
force-pushed
the
CreateThread
branch
from
6ab79d9
to
67cef4b
Compare
Use _beginthreadex instead of CreateThread since we use the Windows CRT, as Microsoft recommends _beginthreadex over CreateThread for these situations. Finally, check for NULL handles, not "INVALID_HANDLE," as _beginthreadex guarantees a valid handle in most cases Signed-off-by: Seija Kijin <doremylover123@gmail.com>
AtariDreams
force-pushed
the
CreateThread
branch
from
67cef4b
to
edee5e7
Compare
AtariDreams
changed the title
|
Submitted as pull.1440.v6.git.git.1676041473607.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
/submit |
|
Submitted as pull.1440.v7.git.git.1676041618809.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
The review comments on earlier rounds of this patch suggest that it is not needed. If you think that these reviews miss a point, please explain it. To express your view, you can use the issue tracker on Github if you do not like to write emails. If you have nothing to add, please stop submitting the same patch again and again. -- Hannes |
This had to be rebased because of conflicts. Nothing to do with my feelings towards anyone involved. I resent the patch just in case as a result. |
|
What about the review comments that suggest that he patch is not needed in the first place? Did you plan on addressing them at some stage? |
|
So no reply at all? |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use pthread_exit instead of async_exit.
This means we do not have
to deal with Windows's implementation
requiring an unsigned exit coded
despite the POSIX exit code requiring
a signed exit code.
Use _beginthreadex instead of CreateThread
since we use the Windows CRT.
Finally, check for NULL handles, not "INVALID_HANDLE," as _beginthreadex guarantees a valid handle in most cases
Signed-off-by: Seija Kijin doremylover123@gmail.com
cc: Johannes Sixt j6t@kdbg.org
cc: Jeff Hostetler git@jeffhostetler.com