From 823c93ce47184493f68b10498148d949ab7ea885 Mon Sep 17 00:00:00 2001 From: janwas Date: Sun, 11 Jun 2006 17:16:24 +0000 Subject: [PATCH] # bugfixes (string, pthread, screenshot writing) - CStr : early out if empty, don't deref iterator (crashes in VC8) - init VFS In archive_builder test - wpthread: avoid race condition when creating threads. cleanup; replace debug_warn with error codes - Util: file_buf_free wasn't specifying FB_FROM_HEAP -> http://www.wildfiregames.com/forum/index.php?showtopic=10085 This was SVN commit r4006. --- .../lib/res/file/tests/test_archive_builder.h | 1 + source/lib/sysdep/win/win_internal.h | 1 - source/lib/sysdep/win/wpthread.cpp | 279 ++++++++++-------- source/ps/CStr.cpp | 3 + source/ps/Util.cpp | 4 +- 5 files changed, 163 insertions(+), 125 deletions(-) diff --git a/source/lib/res/file/tests/test_archive_builder.h b/source/lib/res/file/tests/test_archive_builder.h index 69cc8db482..9d88e7a0a5 100644 --- a/source/lib/res/file/tests/test_archive_builder.h +++ b/source/lib/res/file/tests/test_archive_builder.h @@ -46,6 +46,7 @@ class TestArchiveBuilder : public CxxTest::TestSuite void generate_random_files() { path_init(); // required for file_make_unique_fn_copy + vfs_init(); for(size_t i = 0; i < NUM_FILES; i++) { diff --git a/source/lib/sysdep/win/win_internal.h b/source/lib/sysdep/win/win_internal.h index 8b7f1e2491..d92fb57a3f 100644 --- a/source/lib/sysdep/win/win_internal.h +++ b/source/lib/sysdep/win/win_internal.h @@ -396,7 +396,6 @@ enum WAIO_CS, WIN_CS, WDBG_CS, - WPTHREAD_CS, NUM_CS }; diff --git a/source/lib/sysdep/win/wpthread.cpp b/source/lib/sysdep/win/wpthread.cpp index 5d9c7f89a3..a9ea25e363 100644 --- a/source/lib/sysdep/win/wpthread.cpp +++ b/source/lib/sysdep/win/wpthread.cpp @@ -31,6 +31,12 @@ #include "win_internal.h" #include "../cpu.h" // CAS +#pragma data_seg(WIN_CALLBACK_PRE_LIBC(b)) +WIN_REGISTER_FUNC(wpthread_init); +#pragma data_seg(WIN_CALLBACK_POST_ATEXIT(y)) +WIN_REGISTER_FUNC(wpthread_shutdown); +#pragma data_seg() + static HANDLE HANDLE_from_pthread(pthread_t p) { @@ -43,11 +49,9 @@ static pthread_t pthread_from_HANDLE(HANDLE h) } -////////////////////////////////////////////////////////////////////////////// -// +//----------------------------------------------------------------------------- // misc -// -////////////////////////////////////////////////////////////////////////////// +//----------------------------------------------------------------------------- pthread_t pthread_self(void) { @@ -100,11 +104,9 @@ int pthread_setschedparam(pthread_t thread, int policy, const struct sched_param } -////////////////////////////////////////////////////////////////////////////// -// +//----------------------------------------------------------------------------- // thread-local storage -// -////////////////////////////////////////////////////////////////////////////// +//----------------------------------------------------------------------------- // minimum amount of TLS slots every Windows version provides; // used to validate indices. @@ -141,7 +143,7 @@ int pthread_key_create(pthread_key_t* key, void (*dtor)(void*)) } // not enough slots; we have a valid key, but its dtor won't be called. - debug_warn("increase pthread MAX_DTORS"); + WARN_ERR(ERR_LIMIT); return -1; have_slot: @@ -181,7 +183,7 @@ void* pthread_getspecific(pthread_key_t key) SetLastError(last_err); } else - debug_warn("pthread_getspecific: TlsGetValue failed"); + WARN_ERR(ERR_FAIL); return data; } @@ -230,112 +232,9 @@ again: } -////////////////////////////////////////////////////////////////////////////// -// -// threads -// -////////////////////////////////////////////////////////////////////////////// - -// _beginthreadex cannot call the user's thread function directly due to -// differences in calling convention; we need to pass its address and -// the user-specified data pointer to our trampoline. -// a) a local variable in pthread_create isn't safe because the -// new thread might not start before pthread_create returns. -// b) allocating on the heap would work but we're trying to keep that -// to a minimum. -// c) we therefore use static data protected by a critical section. -static struct FuncAndArg -{ - void* (*func)(void*); - void* arg; -} -func_and_arg; - - -// bridge calling conventions required by _beginthreadex and POSIX. -static unsigned __stdcall thread_start(void* UNUSED(param)) -{ - void* (*func)(void*) = func_and_arg.func; - void* arg = func_and_arg.arg; - win_unlock(WPTHREAD_CS); - - void* ret = (void*)-1; - __try - { - ret = func(arg); - } - __except(wdbg_exception_filter(GetExceptionInformation())) - { - } - - call_tls_dtors(); - - return (unsigned)(uintptr_t)ret; -} - - -int pthread_create(pthread_t* thread_id, const void* UNUSED(attr), void* (*func)(void*), void* arg) -{ - win_lock(WPTHREAD_CS); - func_and_arg.func = func; - func_and_arg.arg = arg; - - // _beginthreadex has more overhead and no value added vs. - // CreateThread, but it avoids small memory leaks in - // ExitThread when using the statically-linked CRT (-> MSDN). - const uintptr_t id = _beginthreadex(0, 0, thread_start, 0, 0, 0); - if(!id) - { - win_unlock(WPTHREAD_CS); - debug_warn("_beginthreadex failed"); - return -1; - } - - // SUSv3 doesn't specify whether this is optional - go the safe route. - if(thread_id) - *thread_id = (pthread_t)id; - - return 0; -} - - -int pthread_cancel(pthread_t thread) -{ - HANDLE hThread = HANDLE_from_pthread(thread); - TerminateThread(hThread, 0); - debug_printf("WARNING: pthread_cancel is unsafe\n"); - return 0; -} - - -int pthread_join(pthread_t thread, void** value_ptr) -{ - HANDLE hThread = HANDLE_from_pthread(thread); - - // note: pthread_join doesn't call for a timeout. if this wait - // locks up the process, at least it'll be easy to see why. - DWORD ret = WaitForSingleObject(hThread, INFINITE); - if(ret != WAIT_OBJECT_0) - { - debug_warn("pthread_join: WaitForSingleObject failed"); - return -1; - } - - // pass back the code that was passed to pthread_exit. - // SUS says <*value_ptr> need only be set on success! - if(value_ptr) - GetExitCodeThread(hThread, (LPDWORD)value_ptr); - - CloseHandle(hThread); - return 0; -} - - -////////////////////////////////////////////////////////////////////////////// -// -// locks -// -////////////////////////////////////////////////////////////////////////////// +//----------------------------------------------------------------------------- +// mutex +//----------------------------------------------------------------------------- // rationale: CRITICAL_SECTIONS have less overhead than Win32 Mutex. // disadvantage is that pthread_mutex_timedlock isn't supported, but @@ -403,7 +302,9 @@ int pthread_mutex_timedlock(pthread_mutex_t* UNUSED(m), const struct timespec* U } -////////////////////////////////////////////////////////////////////////////// +//----------------------------------------------------------------------------- +// semaphore +//----------------------------------------------------------------------------- static HANDLE HANDLE_from_sem_t(sem_t* sem) @@ -432,8 +333,7 @@ int sem_wait(sem_t* sem) { HANDLE h = HANDLE_from_sem_t(sem); DWORD ret = WaitForSingleObject(h, INFINITE); - if(ret != WAIT_OBJECT_0) - debug_warn("unexpected WaitForSingleObject return value"); + debug_assert(ret == WAIT_OBJECT_0); return 0; } @@ -482,7 +382,7 @@ static DWORD calc_timeout_length_ms(const struct timespec* abs_timeout, // that's the Win32 INFINITE value. if(length_ms >= 0xffffffff) { - debug_warn("calc_timeout_length_ms: 32-bit overflow"); + WARN_ERR(ERR_LIMIT); length_ms = 0xfffffffe; } return (DWORD)(length_ms & 0xffffffff); @@ -541,8 +441,143 @@ int sem_msgwait_np(sem_t* sem) else { errno = EINVAL; - debug_warn("unexpected MsgWaitForMultipleObjects return value"); + WARN_ERR(ERR_FAIL); } return -1; +} -} \ No newline at end of file + +//----------------------------------------------------------------------------- +// threads +//----------------------------------------------------------------------------- + +// _beginthreadex cannot call the user's thread function directly due to +// differences in calling convention; we need to pass its address and +// the user-specified data pointer to our trampoline. +// +// rationale: +// - a local variable in pthread_create isn't safe because the +// new thread might not start before pthread_create returns. +// - using one static FuncAndArg protected by critical section doesn't +// work. win_lock allows recursive locking, so if creating 2 threads, +// the parent thread may create both without being stopped and thus +// stomp on the first thread's func_and_arg. +// - stashing func and arg in TLS would work, but it is a +// very limited resource. +// - heap allocations are the obvious safe solution, but we're trying to +// minimize those here. +// - blocking pthread_create until the trampoline has latched func_and_arg +// works. this seems a bit easier to understand than nonrecursive CS. + +struct FuncAndArg +{ + void* (*func)(void*); + void* arg; + + FuncAndArg(void* (*func_)(void*), void* arg_) + : func(func_), arg(arg_) {} +}; + + +static sem_t sem_thread_create; + +// bridge calling conventions required by _beginthreadex and POSIX. +static unsigned __stdcall thread_start(void* param) +{ + const FuncAndArg* func_and_arg = (const FuncAndArg*)param; + void* (*func)(void*) = func_and_arg->func; + void* arg = func_and_arg->arg; + // allow creator to run again. + // potentially pulls rug out from under . + int err = sem_post(&sem_thread_create); + debug_assert(err == 0); + + void* ret = (void*)-1; + __try + { + ret = func(arg); + } + __except(wdbg_exception_filter(GetExceptionInformation())) + { + } + + call_tls_dtors(); + + return (unsigned)(uintptr_t)ret; +} + + +int pthread_create(pthread_t* thread_id, const void* UNUSED(attr), void* (*func)(void*), void* arg) +{ + const FuncAndArg func_and_arg(func, arg); + + // _beginthreadex has more overhead and no value added vs. + // CreateThread, but it avoids small memory leaks in + // ExitThread when using the statically-linked CRT (-> MSDN). + const uintptr_t id = _beginthreadex(0, 0, thread_start, (void*)&func_and_arg, 0, 0); + if(!id) + { + WARN_ERR(ERR_FAIL); + return -1; + } + + // block until thread_start has latched func_and_arg. + // (forces thread-switch) + int err = sem_wait(&sem_thread_create); + debug_assert(err == 0); + + // SUSv3 doesn't specify whether this is optional - go the safe route. + if(thread_id) + *thread_id = (pthread_t)id; + + return 0; +} + + +int pthread_cancel(pthread_t thread) +{ + HANDLE hThread = HANDLE_from_pthread(thread); + TerminateThread(hThread, 0); + debug_printf("WARNING: pthread_cancel is unsafe\n"); + return 0; +} + + +int pthread_join(pthread_t thread, void** value_ptr) +{ + HANDLE hThread = HANDLE_from_pthread(thread); + + // note: pthread_join doesn't call for a timeout. if this wait + // locks up the process, at least it'll be easy to see why. + DWORD ret = WaitForSingleObject(hThread, INFINITE); + if(ret != WAIT_OBJECT_0) + { + WARN_ERR(ERR_FAIL); + return -1; + } + + // pass back the code that was passed to pthread_exit. + // SUS says <*value_ptr> need only be set on success! + if(value_ptr) + GetExitCodeThread(hThread, (LPDWORD)value_ptr); + + CloseHandle(hThread); + return 0; +} + + + + +static LibError wpthread_init() +{ + int err = sem_init(&sem_thread_create, 0, 0); + debug_assert(err == 0); + return INFO_OK; +} + +static LibError wpthread_shutdown() +{ + int err = sem_destroy(&sem_thread_create); + debug_assert(err == 0); + return INFO_OK; +} diff --git a/source/ps/CStr.cpp b/source/ps/CStr.cpp index dc4865f9a1..b7708bc25a 100644 --- a/source/ps/CStr.cpp +++ b/source/ps/CStr.cpp @@ -92,6 +92,9 @@ CStrW CStr8::FromUTF8() const { CStrW result; + if(empty()) + return result; + const unsigned char* source = (const unsigned char*)&*begin(); const unsigned char* sourceEnd = source + length(); while (source < sourceEnd) diff --git a/source/ps/Util.cpp b/source/ps/Util.cpp index 25a64d924d..15ab13670c 100644 --- a/source/ps/Util.cpp +++ b/source/ps/Util.cpp @@ -190,7 +190,7 @@ void WriteScreenshot(const char* extension) glReadPixels(0, 0, w, h, fmt, GL_UNSIGNED_BYTE, img); (void)tex_write(&t, fn); (void)tex_free(&t); - (void)file_buf_free(buf); + (void)file_buf_free(buf, FB_FROM_HEAP); } @@ -295,5 +295,5 @@ void WriteBigScreenshot(const char* extension, int tiles) (void)tex_write(&t, fn); (void)tex_free(&t); free(tile_data); - (void)file_buf_free(img_buf); + (void)file_buf_free(img_buf, FB_FROM_HEAP); }