How did our Fiber tests ever pass?

Best regards,
Alex Ionescu

On Thu, Oct 8, 2015 at 9:26 AM, <sginsberg@svn.reactos.org> wrote:
Author: sginsberg
Date: Thu Oct  8 16:26:29 2015
New Revision: 69466

URL: http://svn.reactos.org/svn/reactos?rev=69466&view=rev
Log:
[KERNEL32] Fix bug in CreateFiberEx that made it replace the CONTEXT_FULL bits rather than ORing in CONTEXT_FLOATING_POINT when caller wanted FPU state saved. SwitchToFiber checks for CONTEXT_FULL OR CONTEXT_FLOATING_POINT so the save/restore would fail. Moreover, fix BaseInitializeContext that was not checking CONTEXT_FLOATING_POINT correctly for some fibers and, as a result, not initializing FPU context correctly for callers of ConvertThreadToFiberEx. Finally, because trying to access address 0x6 is generally a bad idea, fix SwitchToFiber to use the correct shared user data offsets. Misc cleanup all around. Bonus: Sort TEB/PEB asm offsets and add GDI Batch Count offset, needed soon.

Modified:
    trunk/reactos/dll/win32/kernel32/client/fiber.c
    trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
    trunk/reactos/dll/win32/kernel32/client/utils.c
    trunk/reactos/include/asm/ks386.template.h
    trunk/reactos/include/ndk/i386/asm.h

Modified: trunk/reactos/dll/win32/kernel32/client/fiber.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/fiber.c?rev=69466&r1=69465&r2=69466&view=diff
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/fiber.c     [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/fiber.c     [iso-8859-1] Thu Oct  8 16:26:29 2015
@@ -27,10 +27,9 @@

 VOID
 WINAPI
-BaseRundownFls(IN PVOID FlsData)
+BaseRundownFls(_In_ PVOID FlsData)
 {
     /* No FLS support yet */
-
 }

 /* PUBLIC FUNCTIONS ***********************************************************/
@@ -55,16 +54,18 @@
         return FALSE;
     }

-    /* this thread won't run a fiber anymore */
+    /* This thread won't run a fiber anymore */
     Teb->HasFiberData = FALSE;
     FiberData = Teb->NtTib.FiberData;
     Teb->NtTib.FiberData = NULL;

     /* Free the fiber */
     ASSERT(FiberData != NULL);
-    RtlFreeHeap(GetProcessHeap(), 0, FiberData);
-
-    /* success */
+    RtlFreeHeap(GetProcessHeap(),
+                0,
+                FiberData);
+
+    /* Success */
     return TRUE;
 }

@@ -73,15 +74,15 @@
  */
 LPVOID
 WINAPI
-ConvertThreadToFiberEx(LPVOID lpParameter,
-                       DWORD dwFlags)
+ConvertThreadToFiberEx(_In_opt_ LPVOID lpParameter,
+                       _In_ DWORD dwFlags)
 {
     PTEB Teb;
     PFIBER Fiber;
     DPRINT1("Converting Thread to Fiber\n");

     /* Check for invalid flags */
-    if (dwFlags &~ FIBER_FLAG_FLOAT_SWITCH)
+    if (dwFlags & ~FIBER_FLAG_FLOAT_SWITCH)
     {
         /* Fail */
         SetLastError(ERROR_INVALID_PARAMETER);
@@ -98,9 +99,12 @@
     }

     /* Allocate the fiber */
-    Fiber = RtlAllocateHeap(RtlGetProcessHeap(), 0, sizeof(FIBER));
+    Fiber = RtlAllocateHeap(RtlGetProcessHeap(),
+                            0,
+                            sizeof(FIBER));
     if (!Fiber)
     {
+        /* Fail */
         SetLastError(ERROR_NOT_ENOUGH_MEMORY);
         return NULL;
     }
@@ -114,13 +118,11 @@
     Fiber->FlsData = Teb->FlsData;
     Fiber->GuaranteedStackBytes = Teb->GuaranteedStackBytes;
     Fiber->ActivationContextStackPointer = Teb->ActivationContextStackPointer;
-    Fiber->FiberContext.ContextFlags = CONTEXT_FULL;
-
-    /* Save FPU State if requested */
-    if (dwFlags & FIBER_FLAG_FLOAT_SWITCH)
-    {
-        Fiber->FiberContext.ContextFlags |= CONTEXT_FLOATING_POINT;
-    }
+
+    /* Save FPU State if requested, otherwise just the basic registers */
+    Fiber->FiberContext.ContextFlags = (dwFlags & FIBER_FLAG_FLOAT_SWITCH) ?
+                                       (CONTEXT_FULL | CONTEXT_FLOATING_POINT) :
+                                       CONTEXT_FULL;

     /* Associate the fiber to the current thread */
     Teb->NtTib.FiberData = Fiber;
@@ -135,10 +137,11 @@
  */
 LPVOID
 WINAPI
-ConvertThreadToFiber(LPVOID lpParameter)
+ConvertThreadToFiber(_In_opt_ LPVOID lpParameter)
 {
     /* Call the newer function */
-    return ConvertThreadToFiberEx(lpParameter, 0);
+    return ConvertThreadToFiberEx(lpParameter,
+                                  0);
 }

 /*
@@ -146,12 +149,16 @@
  */
 LPVOID
 WINAPI
-CreateFiber(SIZE_T dwStackSize,
-            LPFIBER_START_ROUTINE lpStartAddress,
-            LPVOID lpParameter)
+CreateFiber(_In_ SIZE_T dwStackSize,
+            _In_ LPFIBER_START_ROUTINE lpStartAddress,
+            _In_opt_ LPVOID lpParameter)
 {
     /* Call the Newer Function */
-    return CreateFiberEx(dwStackSize, 0, 0, lpStartAddress, lpParameter);
+    return CreateFiberEx(dwStackSize,
+                         0,
+                         0,
+                         lpStartAddress,
+                         lpParameter);
 }

 /*
@@ -159,20 +166,20 @@
  */
 LPVOID
 WINAPI
-CreateFiberEx(SIZE_T dwStackCommitSize,
-              SIZE_T dwStackReserveSize,
-              DWORD dwFlags,
-              LPFIBER_START_ROUTINE lpStartAddress,
-              LPVOID lpParameter)
+CreateFiberEx(_In_ SIZE_T dwStackCommitSize,
+              _In_ SIZE_T dwStackReserveSize,
+              _In_ DWORD dwFlags,
+              _In_ LPFIBER_START_ROUTINE lpStartAddress,
+              _In_opt_ LPVOID lpParameter)
 {
     PFIBER Fiber;
     NTSTATUS Status;
     INITIAL_TEB InitialTeb;
-    PACTIVATION_CONTEXT_STACK ActivationContextStackPointer = NULL;
+    PACTIVATION_CONTEXT_STACK ActivationContextStackPointer;
     DPRINT("Creating Fiber\n");

     /* Check for invalid flags */
-    if (dwFlags &~ FIBER_FLAG_FLOAT_SWITCH)
+    if (dwFlags & ~FIBER_FLAG_FLOAT_SWITCH)
     {
         /* Fail */
         SetLastError(ERROR_INVALID_PARAMETER);
@@ -180,6 +187,7 @@
     }

     /* Allocate the Activation Context Stack */
+    ActivationContextStackPointer = NULL;
     Status = RtlAllocateActivationContextStack(&ActivationContextStackPointer);
     if (!NT_SUCCESS(Status))
     {
@@ -189,7 +197,9 @@
     }

     /* Allocate the fiber */
-    Fiber = RtlAllocateHeap(RtlGetProcessHeap(), 0, sizeof(FIBER));
+    Fiber = RtlAllocateHeap(RtlGetProcessHeap(),
+                            0,
+                            sizeof(FIBER));
     if (!Fiber)
     {
         /* Free the activation context stack */
@@ -208,7 +218,9 @@
     if (!NT_SUCCESS(Status))
     {
         /* Free the fiber */
-        RtlFreeHeap(GetProcessHeap(), 0, Fiber);
+        RtlFreeHeap(GetProcessHeap(),
+                    0,
+                    Fiber);

         /* Free the activation context stack */
         RtlFreeActivationContextStack(ActivationContextStackPointer);
@@ -219,7 +231,8 @@
     }

     /* Clear the context */
-    RtlZeroMemory(&Fiber->FiberContext, sizeof(CONTEXT));
+    RtlZeroMemory(&Fiber->FiberContext,
+                  sizeof(CONTEXT));

     /* Copy the data into the fiber */
     Fiber->StackBase = InitialTeb.StackBase;
@@ -230,12 +243,13 @@
     Fiber->GuaranteedStackBytes = 0;
     Fiber->FlsData = NULL;
     Fiber->ActivationContextStackPointer = ActivationContextStackPointer;
-    Fiber->FiberContext.ContextFlags = CONTEXT_FULL;
-
-    /* Save FPU State if requested */
-    Fiber->FiberContext.ContextFlags = (dwFlags & FIBER_FLAG_FLOAT_SWITCH) ? CONTEXT_FLOATING_POINT : 0;
-
-    /* initialize the context for the fiber */
+
+    /* Save FPU State if requested, otherwise just the basic registers */
+    Fiber->FiberContext.ContextFlags = (dwFlags & FIBER_FLAG_FLOAT_SWITCH) ?
+                                       (CONTEXT_FULL | CONTEXT_FLOATING_POINT) :
+                                       CONTEXT_FULL;
+
+    /* Initialize the context for the fiber */
     BaseInitializeContext(&Fiber->FiberContext,
                           lpParameter,
                           lpStartAddress,
@@ -251,17 +265,24 @@
  */
 VOID
 WINAPI
-DeleteFiber(LPVOID lpFiber)
-{
-    SIZE_T Size = 0;
-    PFIBER Fiber = (PFIBER)lpFiber;
+DeleteFiber(_In_ LPVOID lpFiber)
+{
+    SIZE_T Size;
+    PFIBER Fiber;
     PTEB Teb;

-    /* First, exit the thread */
+    /* Are we deleting ourselves? */
     Teb = NtCurrentTeb();
-    if ((Teb->HasFiberData) && (Teb->NtTib.FiberData == Fiber)) ExitThread(1);
-
-    /* Now de-allocate the stack */
+    Fiber = (PFIBER)lpFiber;
+    if ((Teb->HasFiberData) &&
+        (Teb->NtTib.FiberData == Fiber))
+    {
+        /* Just exit */
+        ExitThread(1);
+    }
+
+    /* Not ourselves, de-allocate the stack */
+    Size = 0 ;
     NtFreeVirtualMemory(NtCurrentProcess(),
                         &Fiber->DeallocationStack,
                         &Size,
@@ -274,7 +295,9 @@
     RtlFreeActivationContextStack(Fiber->ActivationContextStackPointer);

     /* Free the fiber data */
-    RtlFreeHeap(GetProcessHeap(), 0, lpFiber);
+    RtlFreeHeap(GetProcessHeap(),
+                0,
+                lpFiber);
 }

 /*
@@ -284,6 +307,7 @@
 WINAPI
 IsThreadAFiber(VOID)
 {
+    /* Return flag in the TEB */
     return NtCurrentTeb()->HasFiberData;
 }

@@ -349,7 +373,8 @@
  */
 BOOL
 WINAPI
-FlsSetValue(DWORD dwFlsIndex, PVOID lpFlsData)
+FlsSetValue(DWORD dwFlsIndex,
+            PVOID lpFlsData)
 {
     PVOID *ppFlsSlots;
     TEB *pTeb = NtCurrentTeb();

Modified: trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/i386/fiber.S?rev=69466&r1=69465&r2=69466&view=diff
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/i386/fiber.S        [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/i386/fiber.S        [iso-8859-1] Thu Oct  8 16:26:29 2015
@@ -15,21 +15,26 @@
 .code

 PUBLIC _BaseFiberStartup@0
-.PROC _BaseFiberStartup@0
-    /* Frame pointer is zeroed */
+FUNC _BaseFiberStartup@0
     FPO 0, 0, 0, 0, 0, FRAME_FPO
+
+    /* Note that EBP is already zeroed for us during fiber creation */

     /* Get the fiber data */
     mov eax, fs:[TEB_FIBER_DATA]

-    push dword ptr [eax+FIBER_CONTEXT_EBX] /* Parameter */
-    push dword ptr [eax+FIBER_CONTEXT_EAX] /* Start Address */
-    call _BaseThreadStartup@8
-.ENDP
+    /* Start the thread with our parameters */
+    push dword ptr [eax+FIBER_CONTEXT_EBX]
+    push dword ptr [eax+FIBER_CONTEXT_EAX]
+    jmp _BaseThreadStartup@8
+
+ENDFUNC


 PUBLIC _SwitchToFiber@4
-_SwitchToFiber@4:
+FUNC _SwitchToFiber@4
+    FPO 0, 0, 0, 0, 0, FRAME_FPO
+
     /* Get the TEB */
     mov edx, fs:[TEB_SELF]

@@ -51,7 +56,7 @@
     fnstcw [eax+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]

     /* Check if the CPU supports SIMD MXCSR State Save */
-    cmp byte ptr ds:[PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
+    cmp byte ptr ds:[USER_SHARED_DATA + USER_SHARED_DATA_PROCESSOR_FEATURES + PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
     jnz NoFpuStateSave
     stmxcsr [eax+FIBER_CONTEXT_DR6]

@@ -109,13 +114,13 @@
 StatusWordChanged:

     /* Load the new one */
-    mov word ptr [ecx+FIBER_CONTEXT_FLOAT_SAVE_TAG_WORD], HEX(0FFFF)
+    mov word ptr [ecx+FIBER_CONTEXT_FLOAT_SAVE_TAG_WORD], HEX(FFFF)
     fldenv [ecx+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]

 ControlWordEqual:

     /* Load the new one */
-    cmp byte ptr ds:[PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
+    cmp byte ptr ds:[USER_SHARED_DATA + USER_SHARED_DATA_PROCESSOR_FEATURES + PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
     jnz NoFpuStateRestore
     ldmxcsr [ecx+FIBER_CONTEXT_DR6]

@@ -136,6 +141,7 @@
     mov esp, [ecx+FIBER_CONTEXT_ESP]
     ret 4

+ENDFUNC

 END
 /* EOF */

Modified: trunk/reactos/dll/win32/kernel32/client/utils.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/utils.c?rev=69466&r1=69465&r2=69466&view=diff
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/utils.c     [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/utils.c     [iso-8859-1] Thu Oct  8 16:26:29 2015
@@ -548,7 +548,7 @@

         /* Is FPU state required? */
         Context->ContextFlags |= ContextFlags;
-        if (ContextFlags == CONTEXT_FLOATING_POINT)
+        if ((ContextFlags & CONTEXT_FLOATING_POINT) == CONTEXT_FLOATING_POINT)
         {
             /* Set an initial state */
             Context->FloatSave.ControlWord = 0x27F;

Modified: trunk/reactos/include/asm/ks386.template.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/include/asm/ks386.template.h?rev=69466&r1=69465&r2=69466&view=diff
==============================================================================
--- trunk/reactos/include/asm/ks386.template.h  [iso-8859-1] (original)
+++ trunk/reactos/include/asm/ks386.template.h  [iso-8859-1] Thu Oct  8 16:26:29 2015
@@ -724,17 +724,20 @@

 HEADER("TEB"),
 OFFSET(TEB_EXCEPTION_LIST, TEB, NtTib.ExceptionList),
+OFFSET(TEB_STACK_BASE, TEB, NtTib.StackBase),
 OFFSET(TEB_STACK_LIMIT, TEB, NtTib.StackLimit),
-OFFSET(TEB_STACK_BASE, TEB, NtTib.StackBase),
+OFFSET(TEB_FIBER_DATA, TEB, NtTib.FiberData),
 OFFSET(TEB_SELF, TEB, NtTib.Self),
-OFFSET(TEB_FIBER_DATA, TEB, NtTib.FiberData),
 OFFSET(TEB_PEB, TEB, ProcessEnvironmentBlock),
 OFFSET(TEB_EXCEPTION_CODE, TEB, ExceptionCode),
+OFFSET(TEB_ACTIVATION_CONTEXT_STACK_POINTER, TEB, ActivationContextStackPointer),
+OFFSET(TEB_DEALLOCATION_STACK, TEB, DeallocationStack),
+OFFSET(TEB_GDI_BATCH_COUNT, TEB, GdiBatchCount),
+OFFSET(TEB_GUARANTEED_STACK_BYTES, TEB, GuaranteedStackBytes),
+OFFSET(TEB_FLS_DATA, TEB, FlsData),
+
+HEADER("PEB"),
 OFFSET(PEB_KERNEL_CALLBACK_TABLE, PEB, KernelCallbackTable),
-OFFSET(TEB_FLS_DATA, TEB, FlsData),
-OFFSET(TEB_ACTIVATION_CONTEXT_STACK_POINTER, TEB, ActivationContextStackPointer),
-OFFSET(TEB_GUARANTEED_STACK_BYTES, TEB, GuaranteedStackBytes),
-OFFSET(TEB_DEALLOCATION_STACK, TEB, DeallocationStack),

 HEADER("Misc"),
 CONSTANT(NPX_FRAME_LENGTH),
@@ -754,4 +757,6 @@
 CONSTANT(CONTEXT_ALIGNED_SIZE),
 CONSTANT(PROCESSOR_FEATURE_FXSR),
 CONSTANT(KUSER_SHARED_SYSCALL_RET),
-
+CONSTANT(USER_SHARED_DATA),
+CONSTANT(USER_SHARED_DATA_PROCESSOR_FEATURES),
+

Modified: trunk/reactos/include/ndk/i386/asm.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/include/ndk/i386/asm.h?rev=69466&r1=69465&r2=69466&view=diff
==============================================================================
--- trunk/reactos/include/ndk/i386/asm.h        [iso-8859-1] (original)
+++ trunk/reactos/include/ndk/i386/asm.h        [iso-8859-1] Thu Oct  8 16:26:29 2015
@@ -315,13 +315,14 @@
 #define FRAME_EDITED                            0xFFF8

 //
-// KUSER_SHARED_DATA Offsets
+// USER_SHARED_DATA Offsets
 //
 #ifdef __ASM__
 #define USER_SHARED_DATA                        0xFFDF0000
 #endif
 #define USER_SHARED_DATA_INTERRUPT_TIME         0x8
 #define USER_SHARED_DATA_SYSTEM_TIME            0x14
+#define USER_SHARED_DATA_PROCESSOR_FEATURES     0x274
 #define USER_SHARED_DATA_TICK_COUNT             0x320

 //