Fix Thread constructor with stack size argument (#9186)

Fix Thread constructor with stack size argument

Functional fix for #9158 and #9167

10 files changed, 141 insertions, 120 deletions

diff --git a/src/mscorlib/src/System/Threading/Thread.cs b/src/mscorlib/src/System/Threading/Thread.cs
index b23e8a98b2..ead7a5e4c9 100644
--- a/src/mscorlib/src/System/Threading/Thread.cs
+++ b/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/pal/src/file/file.cpp b/src/pal/src/file/file.cpp
index 5d8d24128c..d70e62bd52 100644
--- a/src/pal/src/file/file.cpp
+++ b/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/pal/src/include/pal/thread.hpp b/src/pal/src/include/pal/thread.hpp
index e6dacd2136..ddacfb9039 100644
--- a/src/pal/src/include/pal/thread.hpp
+++ b/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/pal/src/include/pal/utils.h b/src/pal/src/include/pal/utils.h
index 3ddad4ae2f..f381d957ab 100644
--- a/src/pal/src/include/pal/utils.h
+++ b/src/pal/src/include/pal/utils.h
@@ -20,10 +20,66 @@ Abstract:
 #ifndef _PAL_UTILS_H_
 #define _PAL_UTILS_H_
 
+#include <stdint.h>
+
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+// 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/pal/src/init/pal.cpp b/src/pal/src/init/pal.cpp
index 0bda27644e..e6db7dca2e 100644
--- a/src/pal/src/init/pal.cpp
+++ b/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<INT> init_count = 0;
 Volatile<BOOL> shutdown_intent = 0;
 Volatile<LONG> 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/pal/src/loader/module.cpp b/src/pal/src/loader/module.cpp
index a4fc4949e4..63a65ffb61 100644
--- a/src/pal/src/loader/module.cpp
+++ b/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/pal/src/misc/utils.cpp b/src/pal/src/misc/utils.cpp
index 1e333d19ac..f0ff63439f 100644
--- a/src/pal/src/misc/utils.cpp
+++ b/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 <mach/message.h>
 #endif //HAVE_VM_ALLOCATE
 
 #include "pal/utils.h"
-#include "pal/dbgmsg.h"
 #include "pal/file.h"
 
 #include <errno.h>
 #include <string.h>
 
 
-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/pal/src/thread/process.cpp b/src/pal/src/thread/process.cpp
index cdd0723660..ae069aec86 100644
--- a/src/pal/src/thread/process.cpp
+++ b/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/pal/src/thread/thread.cpp b/src/pal/src/thread/thread.cpp
index 566ef855b4..53283320c5 100644
--- a/src/pal/src/thread/thread.cpp
+++ b/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 <sys/cdefs.h>
@@ -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<CPalThread*> 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/vm/threads.cpp b/src/vm/threads.cpp
index 392b262673..5bb9bf35d7 100644
--- a/src/vm/threads.cpp
+++ b/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<UINT_PTR>(m_CacheStackBase) - 
-            (reinterpret_cast<UINT_PTR>(m_CacheStackBase) - reinterpret_cast<UINT_PTR>(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<UINT_PTR>(m_CacheStackBase) - reinterpret_cast<UINT_PTR>(m_CacheStackLimit)) >
+            MinExecutionStackSize)
+        {
+            m_CacheStackSufficientExecutionLimit = reinterpret_cast<UINT_PTR>(m_CacheStackLimit) + MinExecutionStackSize;
+        }
+        else
+        {
+            m_CacheStackSufficientExecutionLimit = reinterpret_cast<UINT_PTR>(m_CacheStackBase);
+        }
     }
 
     // Ensure that we've setup the stack guarantee properly before we cache the stack limits