[PATCH 1/2] ntdll/tests: Check that creating huge thread stacks should work.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---

I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
was for, but it seems to be causing spurious failures. This fix is
trying to keep the current behavior, but it may be better to completely
revert the commit as, although less likely, TEB allocation could also
fail much more often.

dlls/ntdll/tests/virtual.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
index 8f5b0092bea..686b4076801 100644
--- a/dlls/ntdll/tests/virtual.c
+++ b/dlls/ntdll/tests/virtual.c
@@ -488,6 +488,11 @@ static DWORD WINAPI test_stack_size_thread(void *ptr)
     return 0;
 }

+static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
+{
+    return 0;
+}
+
 static void test_RtlCreateUserStack(void)
 {
     IMAGE_NT_HEADERS *nt = RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress );
@@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
     thread = CreateThread(NULL, 0x3ff000, test_stack_size_thread, &args, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
     WaitForSingleObject(thread, INFINITE);
     CloseHandle(thread);
+
+    thread = CreateThread(NULL, 0x80000000, test_stack_size_dummy_thread, NULL, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
+    todo_wine ok(thread != NULL, "CreateThread with huge stack failed\n");
+    WaitForSingleObject(thread, INFINITE);
+    CloseHandle(thread);
 }

static void test_NtMapViewOfSection(void)

[PATCH 2/2] ntdll: Retry creating thread stack in higher address space.

[Rémi Bernon]

When creating it in the lower 2G fails.

This fixes a regression from 789c1db18a4e192425da3771cac4726cda77130b,
which causes several apps to fail when the lower 2G are getting crowded,
or when they have unusually large default thread stack sizes.

Signed-off-by: Rémi Bernon 
---
 dlls/ntdll/tests/virtual.c | 2 +-
 dlls/ntdll/unix/virtual.c  | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
index 686b4076801..853ff9c27a7 100644
--- a/dlls/ntdll/tests/virtual.c
+++ b/dlls/ntdll/tests/virtual.c
@@ -570,7 +570,7 @@ static void test_RtlCreateUserStack(void)
     CloseHandle(thread);

thread = CreateThread(NULL, 0x80000000, test_stack_size_dummy_thread, NULL, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
-    todo_wine ok(thread != NULL, "CreateThread with huge stack failed\n");
+    ok(thread != NULL, "CreateThread with huge stack failed\n");
     WaitForSingleObject(thread, INFINITE);
     CloseHandle(thread);
 }
diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c
index c3baa5f09c7..e036ebdec86 100644
--- a/dlls/ntdll/unix/virtual.c
+++ b/dlls/ntdll/unix/virtual.c
@@ -3051,6 +3051,7 @@ NTSTATUS virtual_alloc_thread_stack( INITIAL_TEB *stack, SIZE_T reserve_size, SI
     NTSTATUS status;
     sigset_t sigset;
     SIZE_T size, extra_size = 0;
+    unsigned int vprot = VPROT_READ | VPROT_WRITE | VPROT_COMMITTED;

if (!reserve_size) reserve_size = main_image_info.MaximumStackSize;
     if (!commit_size) commit_size = main_image_info.CommittedStackSize;
@@ -3062,9 +3063,9 @@ NTSTATUS virtual_alloc_thread_stack( INITIAL_TEB *stack, SIZE_T reserve_size, SI

server_enter_uninterrupted_section( &virtual_mutex, &sigset );

-    if ((status = map_view( &view, NULL, size + extra_size, FALSE,
-                            VPROT_READ | VPROT_WRITE | VPROT_COMMITTED, 33 )) != STATUS_SUCCESS)
-        goto done;
+    status = map_view( &view, NULL, size + extra_size, FALSE, vprot, 33 );
+    if (status == STATUS_NO_MEMORY) status = map_view( &view, NULL, size + extra_size, FALSE, vprot, 0 );
+    if (status != STATUS_SUCCESS) goto done;

#ifdef VALGRIND_STACK_REGISTER
     VALGRIND_STACK_REGISTER( view->base, (char *)view->base + view->size );

[PATCH 2/2] ntdll: Retry creating thread stack in higher address space.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=90057

Your paranoid android.

=== w2008s64 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w7u_2qxl (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w7u_adm (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w7u_el (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w8 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w8adm (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w864 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064v1507 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064v1809 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064_tsign (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w10pro64 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== debiant2 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== debiant2 (32 bit French report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== debiant2 (32 bit Japanese:Japan report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== debiant2 (32 bit Chinese:China report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== debiant2 (32 bit WoW report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

[Rémi Bernon]

On 5/7/21 12:08 PM, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon
>
> I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> was for, but it seems to be causing spurious failures. This fix is
> trying to keep the current behavior, but it may be better to completely
> revert the commit as, although less likely, TEB allocation could also
> fail much more often.
>
> dlls/ntdll/tests/virtual.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> index 8f5b0092bea..686b4076801 100644
> --- a/dlls/ntdll/tests/virtual.c
> +++ b/dlls/ntdll/tests/virtual.c
> @@ -488,6 +488,11 @@ static DWORD WINAPI test_stack_size_thread(void *ptr)
>       return 0;
>   }
>
> +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> +{
> - return 0;
>
> +}
> - static void test_RtlCreateUserStack(void)
> {
>     IMAGE_NT_HEADERS *nt = RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress );
>
> @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
>       thread = CreateThread(NULL, 0x3ff000, test_stack_size_thread, &args, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
>       WaitForSingleObject(thread, INFINITE);
>       CloseHandle(thread);
> -
> -
>
> thread = CreateThread(NULL, 0x80000000, test_stack_size_dummy_thread, NULL, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> -
>
> todo_wine ok(thread != NULL, "CreateThread with huge stack failed\n");
> -
>
> WaitForSingleObject(thread, INFINITE);
> -
>
> CloseHandle(thread);
> }
>
> static void test_NtMapViewOfSection(void)

Eh obviously that fails on 32bit. Anyway, I'm not sure at all about the 
fix, it's maybe something related to allocations that should be 
bottom-up by default?

[Gabriel Ivăncescu]

On 07/05/2021 13:20, Rémi Bernon wrote:
> On 5/7/21 12:08 PM, Rémi Bernon wrote:
> > Signed-off-by: Rémi Bernon
> >
> > I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> > was for, but it seems to be causing spurious failures. This fix is
> > trying to keep the current behavior, but it may be better to completely
> > revert the commit as, although less likely, TEB allocation could also
> > fail much more often.
> >
> > dlls/ntdll/tests/virtual.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> > index 8f5b0092bea..686b4076801 100644
> > --- a/dlls/ntdll/tests/virtual.c
> > +++ b/dlls/ntdll/tests/virtual.c
> > @@ -488,6 +488,11 @@ static DWORD WINAPI test_stack_size_thread(void 
> > *ptr)
> >       return 0;
> >   }
> > +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> > +{
> > +    return 0;
> > +}
> > -
> >
> > static void test_RtlCreateUserStack(void)
> >   {
> >       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
> > NtCurrentTeb()->Peb->ImageBaseAddress );
> > @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
> >       thread = CreateThread(NULL, 0x3ff000, test_stack_size_thread, 
> > &args, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> >       WaitForSingleObject(thread, INFINITE);
> >       CloseHandle(thread);
> > -
> >
> > +    thread = CreateThread(NULL, 0x80000000, 
> > test_stack_size_dummy_thread, NULL, STACK_SIZE_PARAM_IS_A_RESERVATION, 
> > NULL);
> > +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
> > failed\n");
> > +    WaitForSingleObject(thread, INFINITE);
> > +    CloseHandle(thread);
> >   }
> >   static void test_NtMapViewOfSection(void)
>
> Eh obviously that fails on 32bit. Anyway, I'm not sure at all about the 
> fix, it's maybe something related to allocations that should be 
> bottom-up by default?

There's a wine-staging patch that does this and fixes some bugs:

ntdll-ForceBottomUpAlloc

Does it help? Also what happens on Windows if you force top-down allocation?

[Rémi Bernon]

On 5/7/21 1:38 PM, Gabriel Ivăncescu wrote:
> On 07/05/2021 13:20, Rémi Bernon wrote:
> > On 5/7/21 12:08 PM, Rémi Bernon wrote:
> > > Signed-off-by: Rémi Bernon
> > >
> > > I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> > > was for, but it seems to be causing spurious failures. This fix is
> > > trying to keep the current behavior, but it may be better to completely
> > > revert the commit as, although less likely, TEB allocation could also
> > > fail much more often.
> > >
> > > dlls/ntdll/tests/virtual.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > >
> > > diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> > > index 8f5b0092bea..686b4076801 100644
> > > --- a/dlls/ntdll/tests/virtual.c
> > > +++ b/dlls/ntdll/tests/virtual.c
> > > @@ -488,6 +488,11 @@ static DWORD WINAPI test_stack_size_thread(void 
> > > *ptr)
> > >       return 0;
> > >   }
> > > +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> > > +{
> > > +    return 0;
> > > +}
> > > -
> > >
> > > static void test_RtlCreateUserStack(void)
> > >   {
> > >       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
> > > NtCurrentTeb()->Peb->ImageBaseAddress );
> > > @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
> > >       thread = CreateThread(NULL, 0x3ff000, test_stack_size_thread, 
> > > &args, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > >       WaitForSingleObject(thread, INFINITE);
> > >       CloseHandle(thread);
> > > -
> > >
> > > +    thread = CreateThread(NULL, 0x80000000, 
> > > test_stack_size_dummy_thread, NULL, 
> > > STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
> > > failed\n");
> > > +    WaitForSingleObject(thread, INFINITE);
> > > +    CloseHandle(thread);
> > >   }
> > >   static void test_NtMapViewOfSection(void)
> >
> > Eh obviously that fails on 32bit. Anyway, I'm not sure at all about 
> > the fix, it's maybe something related to allocations that should be 
> > bottom-up by default?
>
> There's a wine-staging patch that does this and fixes some bugs:
>
> ntdll-ForceBottomUpAlloc
>
> Does it help? Also what happens on Windows if you force top-down 
> allocation?

It doesn't help with the regression, because we now forcefully limit the 
address space where the thread stack / teb can be allocated.

What I meant was that although I don't know why it's been added, and if 
the reason was to make sure early threads are allocated on low 
addresses, maybe making default allocation strategy bottom-up is the 
correct fix (instead of trying to forcefully put them in the low 2G).

I think that patch set has been sent already before but it was 
considered a bit over complicated.

[Gabriel Ivăncescu]

On 07/05/2021 14:44, Rémi Bernon wrote:
> On 5/7/21 1:38 PM, Gabriel Ivăncescu wrote:
> > On 07/05/2021 13:20, Rémi Bernon wrote:
> > > On 5/7/21 12:08 PM, Rémi Bernon wrote:
> > > > Signed-off-by: Rémi Bernon
> > > >
> > > > I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> > > > was for, but it seems to be causing spurious failures. This fix is
> > > > trying to keep the current behavior, but it may be better to completely
> > > > revert the commit as, although less likely, TEB allocation could also
> > > > fail much more often.
> > > >
> > > > dlls/ntdll/tests/virtual.c | 10 ++++++++++
> > > >   1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> > > > index 8f5b0092bea..686b4076801 100644
> > > > --- a/dlls/ntdll/tests/virtual.c
> > > > +++ b/dlls/ntdll/tests/virtual.c
> > > > @@ -488,6 +488,11 @@ static DWORD WINAPI test_stack_size_thread(void 
> > > > *ptr)
> > > >       return 0;
> > > >   }
> > > > +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> > > > +{
> > > > +    return 0;
> > > > +}
> > > > -
> > > >
> > > > static void test_RtlCreateUserStack(void)
> > > >   {
> > > >       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
> > > > NtCurrentTeb()->Peb->ImageBaseAddress );
> > > > @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
> > > >       thread = CreateThread(NULL, 0x3ff000, test_stack_size_thread, 
> > > > &args, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > >       WaitForSingleObject(thread, INFINITE);
> > > >       CloseHandle(thread);
> > > > -
> > > >
> > > > +    thread = CreateThread(NULL, 0x80000000, 
> > > > test_stack_size_dummy_thread, NULL, 
> > > > STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > > +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
> > > > failed\n");
> > > > +    WaitForSingleObject(thread, INFINITE);
> > > > +    CloseHandle(thread);
> > > >   }
> > > >   static void test_NtMapViewOfSection(void)
> > >
> > > Eh obviously that fails on 32bit. Anyway, I'm not sure at all about 
> > > the fix, it's maybe something related to allocations that should be 
> > > bottom-up by default?
> >
> > There's a wine-staging patch that does this and fixes some bugs:
> >
> > ntdll-ForceBottomUpAlloc
> >
> > Does it help? Also what happens on Windows if you force top-down 
> > allocation?
>
> It doesn't help with the regression, because we now forcefully limit the 
> address space where the thread stack / teb can be allocated.
>
> What I meant was that although I don't know why it's been added, and if 
> the reason was to make sure early threads are allocated on low 
> addresses, maybe making default allocation strategy bottom-up is the 
> correct fix (instead of trying to forcefully put them in the low 2G).
>
> I think that patch set has been sent already before but it was 
> considered a bit over complicated.

Yeah, but if all allocations are forced bottom-up, not just the thread 
stack/TEB, then the staging patch should already fix this, implicitly.

An interesting thing is if you forced top-down allocation on Windows, do 
they get allocated in low 2G? Info here:

https://tedwvc.wordpress.com/2013/07/16/porting-win32-code-to-64-bit-watch-o...

I think that would explain Windows behavior and whether it special cases 
them or not.

[Rémi Bernon]

On 5/7/21 1:56 PM, Gabriel Ivăncescu wrote:
> On 07/05/2021 14:44, Rémi Bernon wrote:
> > On 5/7/21 1:38 PM, Gabriel Ivăncescu wrote:
> > > On 07/05/2021 13:20, Rémi Bernon wrote:
> > > > On 5/7/21 12:08 PM, Rémi Bernon wrote:
> > > > > Signed-off-by: Rémi Bernon
> > > > >
> > > > > I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> > > > > was for, but it seems to be causing spurious failures. This fix is
> > > > > trying to keep the current behavior, but it may be better to 
> > > > > completely
> > > > > revert the commit as, although less likely, TEB allocation could also
> > > > > fail much more often.
> > > > >
> > > > > dlls/ntdll/tests/virtual.c | 10 ++++++++++
> > > > >   1 file changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> > > > > index 8f5b0092bea..686b4076801 100644
> > > > > --- a/dlls/ntdll/tests/virtual.c
> > > > > +++ b/dlls/ntdll/tests/virtual.c
> > > > > @@ -488,6 +488,11 @@ static DWORD WINAPI 
> > > > > test_stack_size_thread(void *ptr)
> > > > >       return 0;
> > > > >   }
> > > > > +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> > > > > +{
> > > > > +    return 0;
> > > > > +}
> > > > > -
> > > > >
> > > > > static void test_RtlCreateUserStack(void)
> > > > >   {
> > > > >       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
> > > > > NtCurrentTeb()->Peb->ImageBaseAddress );
> > > > > @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
> > > > >       thread = CreateThread(NULL, 0x3ff000, test_stack_size_thread, 
> > > > > &args, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > > >       WaitForSingleObject(thread, INFINITE);
> > > > >       CloseHandle(thread);
> > > > > -
> > > > >
> > > > > +    thread = CreateThread(NULL, 0x80000000, 
> > > > > test_stack_size_dummy_thread, NULL, 
> > > > > STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > > > +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
> > > > > failed\n");
> > > > > +    WaitForSingleObject(thread, INFINITE);
> > > > > +    CloseHandle(thread);
> > > > >   }
> > > > >   static void test_NtMapViewOfSection(void)
> > > >
> > > > Eh obviously that fails on 32bit. Anyway, I'm not sure at all about 
> > > > the fix, it's maybe something related to allocations that should be 
> > > > bottom-up by default?
> > >
> > > There's a wine-staging patch that does this and fixes some bugs:
> > >
> > > ntdll-ForceBottomUpAlloc
> > >
> > > Does it help? Also what happens on Windows if you force top-down 
> > > allocation?
> >
> > It doesn't help with the regression, because we now forcefully limit 
> > the address space where the thread stack / teb can be allocated.
> >
> > What I meant was that although I don't know why it's been added, and 
> > if the reason was to make sure early threads are allocated on low 
> > addresses, maybe making default allocation strategy bottom-up is the 
> > correct fix (instead of trying to forcefully put them in the low 2G).
> >
> > I think that patch set has been sent already before but it was 
> > considered a bit over complicated.
>
> Yeah, but if all allocations are forced bottom-up, not just the thread 
> stack/TEB, then the staging patch should already fix this, implicitly.

I think it makes things ever worse as more things will be allocated in 
the low address space?
> An interesting thing is if you forced top-down allocation on Windows, do 
> they get allocated in low 2G? Info here:
>
> https://tedwvc.wordpress.com/2013/07/16/porting-win32-code-to-64-bit-watch-o...
>
> I think that would explain Windows behavior and whether it special cases 
> them or not.

I don't understand, you can't control thread stack allocation strategy. 
At least not using CreateThread.

[Rémi Bernon]

On 5/7/21 1:58 PM, Rémi Bernon wrote:
> On 5/7/21 1:56 PM, Gabriel Ivăncescu wrote:
> > On 07/05/2021 14:44, Rémi Bernon wrote:
> > > On 5/7/21 1:38 PM, Gabriel Ivăncescu wrote:
> > > > On 07/05/2021 13:20, Rémi Bernon wrote:
> > > > > On 5/7/21 12:08 PM, Rémi Bernon wrote:
> > > > > > Signed-off-by: Rémi Bernon
> > > > > >
> > > > > > I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> > > > > > was for, but it seems to be causing spurious failures. This fix is
> > > > > > trying to keep the current behavior, but it may be better to 
> > > > > > completely
> > > > > > revert the commit as, although less likely, TEB allocation could also
> > > > > > fail much more often.
> > > > > >
> > > > > > dlls/ntdll/tests/virtual.c | 10 ++++++++++
> > > > > >   1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> > > > > > index 8f5b0092bea..686b4076801 100644
> > > > > > --- a/dlls/ntdll/tests/virtual.c
> > > > > > +++ b/dlls/ntdll/tests/virtual.c
> > > > > > @@ -488,6 +488,11 @@ static DWORD WINAPI 
> > > > > > test_stack_size_thread(void *ptr)
> > > > > >       return 0;
> > > > > >   }
> > > > > > +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> > > > > > +{
> > > > > > +    return 0;
> > > > > > +}
> > > > > > -
> > > > > >
> > > > > > static void test_RtlCreateUserStack(void)
> > > > > >   {
> > > > > >       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
> > > > > > NtCurrentTeb()->Peb->ImageBaseAddress );
> > > > > > @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
> > > > > >       thread = CreateThread(NULL, 0x3ff000, 
> > > > > > test_stack_size_thread, &args, STACK_SIZE_PARAM_IS_A_RESERVATION, 
> > > > > > NULL);
> > > > > >       WaitForSingleObject(thread, INFINITE);
> > > > > >       CloseHandle(thread);
> > > > > > -
> > > > > >
> > > > > > +    thread = CreateThread(NULL, 0x80000000, 
> > > > > > test_stack_size_dummy_thread, NULL, 
> > > > > > STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > > > > +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
> > > > > > failed\n");
> > > > > > +    WaitForSingleObject(thread, INFINITE);
> > > > > > +    CloseHandle(thread);
> > > > > >   }
> > > > > >   static void test_NtMapViewOfSection(void)
> > > > >
> > > > > Eh obviously that fails on 32bit. Anyway, I'm not sure at all about 
> > > > > the fix, it's maybe something related to allocations that should be 
> > > > > bottom-up by default?
> > > >
> > > > There's a wine-staging patch that does this and fixes some bugs:
> > > >
> > > > ntdll-ForceBottomUpAlloc
> > > >
> > > > Does it help? Also what happens on Windows if you force top-down 
> > > > allocation?
> > >
> > > It doesn't help with the regression, because we now forcefully limit 
> > > the address space where the thread stack / teb can be allocated.
> > >
> > > What I meant was that although I don't know why it's been added, and 
> > > if the reason was to make sure early threads are allocated on low 
> > > addresses, maybe making default allocation strategy bottom-up is the 
> > > correct fix (instead of trying to forcefully put them in the low 2G).
> > >
> > > I think that patch set has been sent already before but it was 
> > > considered a bit over complicated.
> >
> > Yeah, but if all allocations are forced bottom-up, not just the thread 
> > stack/TEB, then the staging patch should already fix this, implicitly.
>
> I think it makes things ever worse as more things will be allocated in 
> the low address space?
> > An interesting thing is if you forced top-down allocation on Windows, 
> > do they get allocated in low 2G? Info here:
> >
> > https://tedwvc.wordpress.com/2013/07/16/porting-win32-code-to-64-bit-watch-o...
> >
> > I think that would explain Windows behavior and whether it special 
> > cases them or not.
>
> I don't understand, you can't control thread stack allocation strategy. 
> At least not using CreateThread.

Oh sorry, just reading that link now. I guess you can.

[Gabriel Ivăncescu]

On 07/05/2021 14:58, Rémi Bernon wrote:
> On 5/7/21 1:56 PM, Gabriel Ivăncescu wrote:
> > On 07/05/2021 14:44, Rémi Bernon wrote:
> > > On 5/7/21 1:38 PM, Gabriel Ivăncescu wrote:
> > > > On 07/05/2021 13:20, Rémi Bernon wrote:
> > > > > On 5/7/21 12:08 PM, Rémi Bernon wrote:
> > > > > > Signed-off-by: Rémi Bernon
> > > > > >
> > > > > > I'm not completely sure what 789c1db18a4e192425da3771cac4726cda77130b
> > > > > > was for, but it seems to be causing spurious failures. This fix is
> > > > > > trying to keep the current behavior, but it may be better to 
> > > > > > completely
> > > > > > revert the commit as, although less likely, TEB allocation could also
> > > > > > fail much more often.
> > > > > >
> > > > > > dlls/ntdll/tests/virtual.c | 10 ++++++++++
> > > > > >   1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> > > > > > index 8f5b0092bea..686b4076801 100644
> > > > > > --- a/dlls/ntdll/tests/virtual.c
> > > > > > +++ b/dlls/ntdll/tests/virtual.c
> > > > > > @@ -488,6 +488,11 @@ static DWORD WINAPI 
> > > > > > test_stack_size_thread(void *ptr)
> > > > > >       return 0;
> > > > > >   }
> > > > > > +static DWORD WINAPI test_stack_size_dummy_thread(void *ptr)
> > > > > > +{
> > > > > > +    return 0;
> > > > > > +}
> > > > > > -
> > > > > >
> > > > > > static void test_RtlCreateUserStack(void)
> > > > > >   {
> > > > > >       IMAGE_NT_HEADERS *nt = RtlImageNtHeader( 
> > > > > > NtCurrentTeb()->Peb->ImageBaseAddress );
> > > > > > @@ -563,6 +568,11 @@ static void test_RtlCreateUserStack(void)
> > > > > >       thread = CreateThread(NULL, 0x3ff000, 
> > > > > > test_stack_size_thread, &args, STACK_SIZE_PARAM_IS_A_RESERVATION, 
> > > > > > NULL);
> > > > > >       WaitForSingleObject(thread, INFINITE);
> > > > > >       CloseHandle(thread);
> > > > > > -
> > > > > >
> > > > > > +    thread = CreateThread(NULL, 0x80000000, 
> > > > > > test_stack_size_dummy_thread, NULL, 
> > > > > > STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
> > > > > > +    todo_wine ok(thread != NULL, "CreateThread with huge stack 
> > > > > > failed\n");
> > > > > > +    WaitForSingleObject(thread, INFINITE);
> > > > > > +    CloseHandle(thread);
> > > > > >   }
> > > > > >   static void test_NtMapViewOfSection(void)
> > > > >
> > > > > Eh obviously that fails on 32bit. Anyway, I'm not sure at all about 
> > > > > the fix, it's maybe something related to allocations that should be 
> > > > > bottom-up by default?
> > > >
> > > > There's a wine-staging patch that does this and fixes some bugs:
> > > >
> > > > ntdll-ForceBottomUpAlloc
> > > >
> > > > Does it help? Also what happens on Windows if you force top-down 
> > > > allocation?
> > >
> > > It doesn't help with the regression, because we now forcefully limit 
> > > the address space where the thread stack / teb can be allocated.
> > >
> > > What I meant was that although I don't know why it's been added, and 
> > > if the reason was to make sure early threads are allocated on low 
> > > addresses, maybe making default allocation strategy bottom-up is the 
> > > correct fix (instead of trying to forcefully put them in the low 2G).
> > >
> > > I think that patch set has been sent already before but it was 
> > > considered a bit over complicated.
> >
> > Yeah, but if all allocations are forced bottom-up, not just the thread 
> > stack/TEB, then the staging patch should already fix this, implicitly.
>
> I think it makes things ever worse as more things will be allocated in 
> the low address space?

AFAIK that's already the case on Windows (bottom-up default allocation 
behavior), and some things are known to break if top-down is forced in 
the registry. Even Microsoft's own xaudio breaks. Not related to stack 
or teb.

I don't know the purpose of the current code, I'm just assuming it might 
be a semi-hack (probably dealing with apps that expect stack in lower 
2G) and the "correct" fix would be to force bottom up allocation, as 
that also fixes other apps and matches Windows default behavior. Then we 
won't need this. And of course, I doubt Windows restricts the stack to 
low 2G if it doesn't have any more room there. Anyway this will need 
investigation.

[Gabriel Ivăncescu]

Hi Rémi,

Sorry, it seems my last email didn't get sent for some reason.

Anyway, I wanted to say I did a quick test on Windows 10. Setting that 
registry value will make all stacks be allocated from top-down, even the 
main thread's stack (i.e. far above 4 GB).

So it seems that Windows doesn't really treat the stack special here. 
The fact it's allocated below 2G for most apps is due to the side-effect 
of the bottom-up allocation behavior on Windows by default.

The current Wine code probably does that since some apps depend on it, 
but IMO correct fix would be to have all allocations bottom-up like the 
staging patchset, since other apps depend on that.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=90056

Your paranoid android.

=== w2008s64 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w7u_2qxl (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w7u_adm (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w7u_el (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w8 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w8adm (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w864 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064v1507 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064v1809 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w1064_tsign (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed

=== w10pro64 (32 bit report) ===

ntdll:
virtual.c:573: Test failed: CreateThread with huge stack failed