From 8d2758fb04ad4c17c196fe167967adce73b496f5 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Mon, 30 Jan 2017 14:47:29 -0800 Subject: [PATCH] Fix Thread constructor with stack size argument (dotnet/coreclr#9186) Fix Thread constructor with stack size argument Functional fix for dotnet/coreclr#9158 and dotnet/coreclr#9167 Commit migrated from https://github.com/dotnet/coreclr/commit/b390ea94d52a9089e24973a12a6413bc8eb9bc3d --- .../src/mscorlib/src/System/Threading/Thread.cs | 3 +- src/coreclr/src/pal/src/file/file.cpp | 6 +- src/coreclr/src/pal/src/include/pal/thread.hpp | 18 ---- src/coreclr/src/pal/src/include/pal/utils.h | 56 +++++++++++ src/coreclr/src/pal/src/init/pal.cpp | 17 +--- src/coreclr/src/pal/src/loader/module.cpp | 6 +- src/coreclr/src/pal/src/misc/utils.cpp | 6 +- src/coreclr/src/pal/src/thread/process.cpp | 6 +- src/coreclr/src/pal/src/thread/thread.cpp | 108 ++++++++------------- src/coreclr/src/vm/threads.cpp | 35 +++++-- 10 files changed, 141 insertions(+), 120 deletions(-) diff --git a/src/coreclr/src/mscorlib/src/System/Threading/Thread.cs b/src/coreclr/src/mscorlib/src/System/Threading/Thread.cs index b23e8a9..ead7a5e 100644 --- a/src/coreclr/src/mscorlib/src/System/Threading/Thread.cs +++ b/src/coreclr/src/mscorlib/src/System/Threading/Thread.cs @@ -427,8 +427,7 @@ namespace System.Threading { private void SetStartHelper(Delegate start, int maxStackSize) { - // We only support default stacks in CoreCLR - Debug.Assert(maxStackSize == 0); + Debug.Assert(maxStackSize >= 0); ThreadHelper threadStartCallBack = new ThreadHelper(start); if(start is ThreadStart) diff --git a/src/coreclr/src/pal/src/file/file.cpp b/src/coreclr/src/pal/src/file/file.cpp index 5d8d241..d70e62b 100644 --- a/src/coreclr/src/pal/src/file/file.cpp +++ b/src/coreclr/src/pal/src/file/file.cpp @@ -18,6 +18,9 @@ Abstract: --*/ +#include "pal/dbgmsg.h" +SET_DEFAULT_DEBUG_CHANNEL(FILE); // some headers have code with asserts, so do this first + #include "pal/thread.hpp" #include "pal/file.hpp" #include "shmfilelockmgr.hpp" @@ -25,7 +28,6 @@ Abstract: #include "pal/stackstring.hpp" #include "pal/palinternal.h" -#include "pal/dbgmsg.h" #include "pal/file.h" #include "pal/filetime.h" #include "pal/utils.h" @@ -42,8 +44,6 @@ Abstract: using namespace CorUnix; -SET_DEFAULT_DEBUG_CHANNEL(FILE); - int MaxWCharToAcpLengthFactor = 3; PAL_ERROR diff --git a/src/coreclr/src/pal/src/include/pal/thread.hpp b/src/coreclr/src/pal/src/include/pal/thread.hpp index e6dacd2..ddacfb9 100644 --- a/src/coreclr/src/pal/src/include/pal/thread.hpp +++ b/src/coreclr/src/pal/src/include/pal/thread.hpp @@ -94,11 +94,6 @@ namespace CorUnix ); PAL_ERROR - InitializeGlobalThreadData( - void - ); - - PAL_ERROR CreateThreadData( CPalThread **ppThread ); @@ -243,12 +238,6 @@ namespace CorUnix friend PAL_ERROR - InitializeGlobalThreadData( - void - ); - - friend - PAL_ERROR CreateThreadData( CPalThread **ppThread ); @@ -338,13 +327,6 @@ namespace CorUnix // Limit address of the stack of this thread void* m_stackLimit; - // The default stack size of a newly created thread (currently 256KB) - // when the dwStackSize paramter of PAL_CreateThread() - // is zero. This value can be set by setting the - // environment variable PAL_THREAD_DEFAULT_STACK_SIZE - // (the value should be in bytes and in hex). - static DWORD s_dwDefaultThreadStackSize; - // // The thread entry routine (called from InternalCreateThread) // diff --git a/src/coreclr/src/pal/src/include/pal/utils.h b/src/coreclr/src/pal/src/include/pal/utils.h index 3ddad4a..f381d95 100644 --- a/src/coreclr/src/pal/src/include/pal/utils.h +++ b/src/coreclr/src/pal/src/include/pal/utils.h @@ -20,10 +20,66 @@ Abstract: #ifndef _PAL_UTILS_H_ #define _PAL_UTILS_H_ +#include + #include #include #include +//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// Alignment helpers (copied for PAL use from stdmacros.h) + +inline size_t ALIGN_UP(size_t val, size_t alignment) +{ + // alignment must be a power of 2 for this implementation to work (need modulo otherwise) + _ASSERTE(0 == (alignment & (alignment - 1))); + size_t result = (val + (alignment - 1)) & ~(alignment - 1); + _ASSERTE(result >= val); // check for overflow + return result; +} + +inline void* ALIGN_UP(void* val, size_t alignment) +{ + return (void*)ALIGN_UP((size_t)val, alignment); +} + +inline uint8_t* ALIGN_UP(uint8_t* val, size_t alignment) +{ + return (uint8_t*)ALIGN_UP((size_t)val, alignment); +} + +inline size_t ALIGN_DOWN(size_t val, size_t alignment) +{ + // alignment must be a power of 2 for this implementation to work (need modulo otherwise) + _ASSERTE(0 == (alignment & (alignment - 1))); + size_t result = val & ~(alignment - 1); + return result; +} + +inline void* ALIGN_DOWN(void* val, size_t alignment) +{ + return (void*)ALIGN_DOWN((size_t)val, alignment); +} + +inline uint8_t* ALIGN_DOWN(uint8_t* val, size_t alignment) +{ + return (uint8_t*)ALIGN_DOWN((size_t)val, alignment); +} + +inline BOOL IS_ALIGNED(size_t val, size_t alignment) +{ + // alignment must be a power of 2 for this implementation to work (need modulo otherwise) + _ASSERTE(0 == (alignment & (alignment - 1))); + return 0 == (val & (alignment - 1)); +} + +inline BOOL IS_ALIGNED(const void* val, size_t alignment) +{ + return IS_ALIGNED((size_t)val, alignment); +} + +//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + #ifdef __cplusplus extern "C" { diff --git a/src/coreclr/src/pal/src/init/pal.cpp b/src/coreclr/src/pal/src/init/pal.cpp index 0bda276..e6db7dc 100644 --- a/src/coreclr/src/pal/src/init/pal.cpp +++ b/src/coreclr/src/pal/src/init/pal.cpp @@ -18,6 +18,9 @@ Abstract: --*/ +#include "pal/dbgmsg.h" +SET_DEFAULT_DEBUG_CHANNEL(PAL); // some headers have code with asserts, so do this first + #include "pal/thread.hpp" #include "pal/synchobjects.hpp" #include "pal/procobj.hpp" @@ -27,7 +30,6 @@ Abstract: #include "../objmgr/shmobjectmanager.hpp" #include "pal/seh.hpp" #include "pal/palinternal.h" -#include "pal/dbgmsg.h" #include "pal/sharedmemory.h" #include "pal/shmemory.h" #include "pal/process.h" @@ -90,8 +92,6 @@ using namespace CorUnix; extern "C" BOOL CRTInitStdStreams( void ); -SET_DEFAULT_DEBUG_CHANNEL(PAL); - Volatile init_count = 0; Volatile shutdown_intent = 0; Volatile g_coreclrInitialized = 0; @@ -314,17 +314,6 @@ Initialize( #endif // HAVE_MACH_EXCEPTIONS // - // Initialize global thread data - // - - palError = InitializeGlobalThreadData(); - if (NO_ERROR != palError) - { - ERROR("Unable to initialize thread data\n"); - goto CLEANUP1; - } - - // // Allocate the initial thread data // diff --git a/src/coreclr/src/pal/src/loader/module.cpp b/src/coreclr/src/pal/src/loader/module.cpp index a4fc494..63a65ff 100644 --- a/src/coreclr/src/pal/src/loader/module.cpp +++ b/src/coreclr/src/pal/src/loader/module.cpp @@ -18,11 +18,13 @@ Abstract: --*/ +#include "pal/dbgmsg.h" +SET_DEFAULT_DEBUG_CHANNEL(LOADER); // some headers have code with asserts, so do this first + #include "pal/thread.hpp" #include "pal/malloc.hpp" #include "pal/file.hpp" #include "pal/palinternal.h" -#include "pal/dbgmsg.h" #include "pal/module.h" #include "pal/cs.hpp" #include "pal/process.h" @@ -60,8 +62,6 @@ Abstract: using namespace CorUnix; -SET_DEFAULT_DEBUG_CHANNEL(LOADER); - // In safemath.h, Template SafeInt uses macro _ASSERTE, which need to use variable // defdbgchan defined by SET_DEFAULT_DEBUG_CHANNEL. Therefore, the include statement // should be placed after the SET_DEFAULT_DEBUG_CHANNEL(LOADER) diff --git a/src/coreclr/src/pal/src/misc/utils.cpp b/src/coreclr/src/pal/src/misc/utils.cpp index 1e333d1..f0ff634 100644 --- a/src/coreclr/src/pal/src/misc/utils.cpp +++ b/src/coreclr/src/pal/src/misc/utils.cpp @@ -18,21 +18,21 @@ Abstract: --*/ +#include "pal/dbgmsg.h" +SET_DEFAULT_DEBUG_CHANNEL(MISC); // some headers have code with asserts, so do this first + #include "pal/palinternal.h" #if HAVE_VM_ALLOCATE #include #endif //HAVE_VM_ALLOCATE #include "pal/utils.h" -#include "pal/dbgmsg.h" #include "pal/file.h" #include #include -SET_DEFAULT_DEBUG_CHANNEL(MISC); - // In safemath.h, Template SafeInt uses macro _ASSERTE, which need to use variable // defdbgchan defined by SET_DEFAULT_DEBUG_CHANNEL. Therefore, the include statement // should be placed after the SET_DEFAULT_DEBUG_CHANNEL(MISC) diff --git a/src/coreclr/src/pal/src/thread/process.cpp b/src/coreclr/src/pal/src/thread/process.cpp index cdd0723..ae069ae 100644 --- a/src/coreclr/src/pal/src/thread/process.cpp +++ b/src/coreclr/src/pal/src/thread/process.cpp @@ -18,6 +18,9 @@ Abstract: --*/ +#include "pal/dbgmsg.h" +SET_DEFAULT_DEBUG_CHANNEL(PROCESS); // some headers have code with asserts, so do this first + #include "pal/procobj.hpp" #include "pal/thread.hpp" #include "pal/file.hpp" @@ -29,7 +32,6 @@ Abstract: #include "pal/init.h" #include "pal/critsect.h" #include "pal/debug.h" -#include "pal/dbgmsg.h" #include "pal/utils.h" #include "pal/environ.h" #include "pal/virtual.h" @@ -67,8 +69,6 @@ Abstract: using namespace CorUnix; -SET_DEFAULT_DEBUG_CHANNEL(PROCESS); - CObjectType CorUnix::otProcess( otiProcess, NULL, diff --git a/src/coreclr/src/pal/src/thread/thread.cpp b/src/coreclr/src/pal/src/thread/thread.cpp index 566ef85..5328332 100644 --- a/src/coreclr/src/pal/src/thread/thread.cpp +++ b/src/coreclr/src/pal/src/thread/thread.cpp @@ -34,6 +34,8 @@ SET_DEFAULT_DEBUG_CHANNEL(THREAD); // some headers have code with asserts, so do #include "pal/module.h" #include "pal/environ.h" #include "pal/init.h" +#include "pal/utils.h" +#include "pal/virtual.h" #if defined(__NetBSD__) && !HAVE_PTHREAD_GETCPUCLOCKID #include @@ -77,13 +79,6 @@ using namespace CorUnix; /* ------------------- Definitions ------------------------------*/ -// The default stack size of a newly created thread (currently 256KB) -// when the dwStackSize parameter of PAL_CreateThread() -// is zero. This value can be set by setting the -// environment variable PAL_THREAD_DEFAULT_STACK_SIZE -// (the value should be in bytes and in hex). -DWORD CPalThread::s_dwDefaultThreadStackSize = 256*1024; - /* list of free CPalThread objects */ static Volatile free_threads_list = NULL; @@ -528,6 +523,7 @@ CorUnix::InternalCreateThread( #endif // PTHREAD_CREATE_MODIFIES_ERRNO BOOL fHoldingProcessLock = FALSE; int iError = 0; + size_t alignedStackSize; if (0 != terminator) { @@ -573,7 +569,24 @@ CorUnix::InternalCreateThread( palError = ERROR_INVALID_PARAMETER; goto EXIT; } - + + alignedStackSize = dwStackSize; + if (alignedStackSize != 0) + { + // Some systems require the stack size to be aligned to the page size + if (sizeof(alignedStackSize) <= sizeof(dwStackSize) && alignedStackSize + (VIRTUAL_PAGE_SIZE - 1) < alignedStackSize) + { + // When coming here from the public API surface, the incoming value is originally a nonnegative signed int32, so + // this shouldn't happen + ASSERT( + "Couldn't align the requested stack size (%Iu) to the page size because the stack size was too large\n", + alignedStackSize); + palError = ERROR_INVALID_PARAMETER; + goto EXIT; + } + alignedStackSize = ALIGN_UP(alignedStackSize, VIRTUAL_PAGE_SIZE); + } + // Ignore the STACK_SIZE_PARAM_IS_A_RESERVATION flag dwCreationFlags &= ~STACK_SIZE_PARAM_IS_A_RESERVATION; @@ -616,42 +629,34 @@ CorUnix::InternalCreateThread( fAttributesInitialized = TRUE; /* adjust the stack size if necessary */ - if (0 != pthread_attr_getstacksize(&pthreadAttr, &pthreadStackSize)) + if (alignedStackSize != 0) { - ERROR("couldn't set thread stack size\n"); - palError = ERROR_INTERNAL_ERROR; - goto EXIT; - } - - TRACE("default pthread stack size is %d, caller requested %d (default is %d)\n", - pthreadStackSize, dwStackSize, CPalThread::s_dwDefaultThreadStackSize); - - if (0 == dwStackSize) - { - dwStackSize = CPalThread::s_dwDefaultThreadStackSize; - } - #ifdef PTHREAD_STACK_MIN - if (PTHREAD_STACK_MIN > pthreadStackSize) - { - WARN("default stack size is reported as %d, but PTHREAD_STACK_MIN is " - "%d\n", pthreadStackSize, PTHREAD_STACK_MIN); - } -#endif - - if (pthreadStackSize < dwStackSize) - { - TRACE("setting thread stack size to %d\n", dwStackSize); - if (0 != pthread_attr_setstacksize(&pthreadAttr, dwStackSize)) + const size_t MinStackSize = PTHREAD_STACK_MIN; +#else // !PTHREAD_STACK_MIN + const size_t MinStackSize = 64 * 1024; // this value is typically accepted by pthread_attr_setstacksize() +#endif // PTHREAD_STACK_MIN + _ASSERTE(IS_ALIGNED(MinStackSize, VIRTUAL_PAGE_SIZE)); + if (alignedStackSize < MinStackSize) { - ERROR("couldn't set pthread stack size to %d\n", dwStackSize); + // Adjust the stack size to a minimum value that is likely to be accepted by pthread_attr_setstacksize(). If this + // function fails, typically the caller will end up throwing OutOfMemoryException under the assumption that the + // requested stack size is too large or the system does not have sufficient memory to create a thread. Try to + // prevent failing just just because the stack size value is too low. + alignedStackSize = MinStackSize; + } + + TRACE("setting thread stack size to %Iu\n", alignedStackSize); + if (0 != pthread_attr_setstacksize(&pthreadAttr, alignedStackSize)) + { + ERROR("couldn't set pthread stack size to %Iu\n", alignedStackSize); palError = ERROR_INTERNAL_ERROR; goto EXIT; } } else { - TRACE("using the system default thread stack size of %d\n", pthreadStackSize); + TRACE("using the system default thread stack size\n"); } #if HAVE_THREAD_SELF || HAVE__LWP_SELF @@ -1755,39 +1760,6 @@ fail: return NULL; } - -#define PAL_THREAD_DEFAULT_STACK_SIZE "PAL_THREAD_DEFAULT_STACK_SIZE" - -PAL_ERROR -CorUnix::InitializeGlobalThreadData( - void - ) -{ - PAL_ERROR palError = NO_ERROR; - char *pszStackSize = NULL; - - // - // Read in the environment to see whether we need to change the default - // thread stack size. - // - pszStackSize = EnvironGetenv(PAL_THREAD_DEFAULT_STACK_SIZE); - if (NULL != pszStackSize) - { - // Environment variable exists - char *pszEnd; - DWORD dw = PAL_strtoul(pszStackSize, &pszEnd, 16); // treat it as hex - if ( (pszStackSize != pszEnd) && (0 != dw) ) - { - CPalThread::s_dwDefaultThreadStackSize = dw; - } - - free(pszStackSize); - } - - return palError; -} - - /*++ Function: CreateThreadData diff --git a/src/coreclr/src/vm/threads.cpp b/src/coreclr/src/vm/threads.cpp index 392b262..5bb9bf3 100644 --- a/src/coreclr/src/vm/threads.cpp +++ b/src/coreclr/src/vm/threads.cpp @@ -3060,7 +3060,16 @@ BOOL Thread::CreateNewOSThread(SIZE_T sizeToCommitOrReserve, LPTHREAD_START_ROUT DWORD dwCreationFlags = CREATE_SUSPENDED; #ifdef FEATURE_CORECLR - dwCreationFlags |= STACK_SIZE_PARAM_IS_A_RESERVATION; + dwCreationFlags |= STACK_SIZE_PARAM_IS_A_RESERVATION; + +#ifndef FEATURE_PAL // the PAL does its own adjustments as necessary + if (sizeToCommitOrReserve != 0 && sizeToCommitOrReserve <= OS_PAGE_SIZE) + { + // On Windows, passing a value that is <= one page size bizarrely causes the OS to use the default stack size instead of + // a minimum, which is undesirable. This adjustment fixes that issue to use a minimum stack size (typically 64 KB). + sizeToCommitOrReserve = OS_PAGE_SIZE + 1; + } +#endif // !FEATURE_PAL #else if(sizeToCommitOrReserve != 0) { @@ -7316,11 +7325,25 @@ BOOL Thread::SetStackLimits(SetStackLimitScope scope) return FALSE; } - // Compute the limit used by EnsureSufficientExecutionStack and cache it on the thread. The limit - // is currently set at 50% of the stack, which should be sufficient to allow the average Framework - // function to run, and to allow us to throw and dispatch an exception up a reasonable call chain. - m_CacheStackSufficientExecutionLimit = reinterpret_cast(m_CacheStackBase) - - (reinterpret_cast(m_CacheStackBase) - reinterpret_cast(m_CacheStackLimit)) / 2; + // Compute the limit used by EnsureSufficientExecutionStack and cache it on the thread. This minimum stack size should + // be sufficient to allow a typical non-recursive call chain to execute, including potential exception handling and + // garbage collection. Used for probing for available stack space through RuntimeImports.EnsureSufficientExecutionStack, + // among other things. +#ifdef BIT64 + const UINT_PTR MinExecutionStackSize = 128 * 1024; +#else // !BIT64 + const UINT_PTR MinExecutionStackSize = 64 * 1024; +#endif // BIT64 + _ASSERTE(m_CacheStackBase >= m_CacheStackLimit); + if ((reinterpret_cast(m_CacheStackBase) - reinterpret_cast(m_CacheStackLimit)) > + MinExecutionStackSize) + { + m_CacheStackSufficientExecutionLimit = reinterpret_cast(m_CacheStackLimit) + MinExecutionStackSize; + } + else + { + m_CacheStackSufficientExecutionLimit = reinterpret_cast(m_CacheStackBase); + } } // Ensure that we've setup the stack guarantee properly before we cache the stack limits -- 2.7.4