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@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> --- >> 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@xxxxxxxxx >> >> 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@xxxxxxxxx> >> - ## 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 > >
