[NTOS:KD]

- Fix the condition check when setting twice (or more) the same breakpoint.
- Implement support for deferred breakpoints. For more information, see: http://www.osronline.com/article.cfm?article=541 (which also exposes an interesting problem about them).

svn path=/trunk/; revision=68841

reactos/ntoskrnl/include/internal/kd64.h

@@ -6,11 +6,6 @@
  * PROGRAMMERS:     Alex Ionescu (alex.ionescu@reactos.org)
  */
 
-//
-// Maximum supported number of breakpoints
-//
-#define KD_BREAKPOINT_MAX                               32
-
 //
 // Default size of the DbgPrint log buffer
 //
@@ -20,16 +15,30 @@
 #define KD_DEFAULT_LOG_BUFFER_SIZE                      0x1000
 #endif
 
+//
+// Maximum supported number of breakpoints
+//
+#define KD_BREAKPOINT_MAX   32
+
+//
+// Highest limit starting which we consider that breakpoint addresses
+// are either in system space, or in user space but inside shared DLLs.
+//
+// I'm wondering whether this can be computed using MmHighestUserAddress
+// or whether there is already some #define somewhere else...
+// See http://www.drdobbs.com/windows/faster-dll-load-load/184416918
+// and http://www.drdobbs.com/rebasing-win32-dlls/184416272
+// for a tentative explanation.
+//
+#define KD_HIGHEST_USER_BREAKPOINT_ADDRESS  (PVOID)0x60000000  // MmHighestUserAddress
+
 //
 // Breakpoint Status Flags
 //
-typedef enum _KDP_BREAKPOINT_FLAGS
+#define KD_BREAKPOINT_ACTIVE    0x01
-{
+#define KD_BREAKPOINT_PENDING   0x02
-    KdpBreakpointActive = 1,
+#define KD_BREAKPOINT_SUSPENDED 0x04
-    KdpBreakpointPending = 2,
+#define KD_BREAKPOINT_EXPIRED   0x08
-    KdpBreakpointSuspended = 4,
-    KdpBreakpointExpired = 8
-} KDP_BREAKPOINT_FLAGS;
 
 //
 // Structure for Breakpoints
@@ -37,7 +46,7 @@ typedef enum _KDP_BREAKPOINT_FLAGS
 typedef struct _BREAKPOINT_ENTRY
 {
     ULONG Flags;
-    PKPROCESS Process;
+    ULONG_PTR DirectoryTableBase;
     PVOID Address;
     KD_BREAKPOINT_TYPE Content;
 } BREAKPOINT_ENTRY, *PBREAKPOINT_ENTRY;
@@ -282,6 +291,12 @@ KdpAddBreakpoint(
     IN PVOID Address
 );
 
+VOID
+NTAPI
+KdSetOwedBreakpoints(
+    VOID
+);
+
 BOOLEAN
 NTAPI
 KdpDeleteBreakpoint(

reactos/ntoskrnl/kd64/kdbreak.c

@@ -23,18 +23,18 @@ KdpAddBreakpoint(IN PVOID Address)
     ULONG i;
     NTSTATUS Status;
 
-    /* Loop current breakpoints */
+    /* Check whether we are not setting a breakpoint twice */
     for (i = 0; i < KD_BREAKPOINT_MAX; i++)
     {
         /* Check if the breakpoint is valid */
-        if ((KdpBreakpointTable[i].Flags & KdpBreakpointActive) &&
+        if ((KdpBreakpointTable[i].Flags & KD_BREAKPOINT_ACTIVE) &&
             (KdpBreakpointTable[i].Address == Address))
         {
-            /* Check if it's pending */
+            /* Were we not able to remove it earlier? */
-            if ((KdpBreakpointTable[i].Flags & KdpBreakpointPending))
+            if (KdpBreakpointTable[i].Flags & KD_BREAKPOINT_EXPIRED)
             {
-                /* It's not pending anymore now */
+                /* Just re-use it! */
-                KdpBreakpointTable[i].Flags &= ~KdpBreakpointPending;
+                KdpBreakpointTable[i].Flags &= ~KD_BREAKPOINT_EXPIRED;
                 return i + 1;
             }
             else
@@ -46,30 +46,34 @@ KdpAddBreakpoint(IN PVOID Address)
     }
 
     /* Find a free entry */
-    for (i = 0; i < KD_BREAKPOINT_MAX; i++) if (!(KdpBreakpointTable[i].Flags)) break;
+    for (i = 0; i < KD_BREAKPOINT_MAX; i++)
+    {
+        if (KdpBreakpointTable[i].Flags == 0)
+            break;
+    }
 
     /* Fail if no free entry was found */
     if (i == KD_BREAKPOINT_MAX) return 0;
 
-    /* Save the old instruction */
+    /* Save the breakpoint */
+    KdpBreakpointTable[i].Address = Address;
+
+    /* If we are setting the breakpoint in user space, save the active process context */
+    if (Address < KD_HIGHEST_USER_BREAKPOINT_ADDRESS)
+        KdpBreakpointTable[i].DirectoryTableBase = KeGetCurrentThread()->ApcState.Process->DirectoryTableBase[0];
+
+    /* Try to save the old instruction */
     Status = KdpCopyMemoryChunks((ULONG_PTR)Address,
                                  &Content,
                                  KD_BREAKPOINT_SIZE,
                                  0,
                                  MMDBG_COPY_UNSAFE,
                                  NULL);
-
+    if (NT_SUCCESS(Status))
-    if (!NT_SUCCESS(Status))
     {
-        /* TODO: Set it as a owed breakpoint */
+        /* Memory accessible, set the breakpoint */
-        KdpDprintf("Failed to set breakpoint at address 0x%p\n", Address);
-        return 0;
-    }
-
-    /* Write the entry */
-    KdpBreakpointTable[i].Address = Address;
         KdpBreakpointTable[i].Content = Content;
-    KdpBreakpointTable[i].Flags = KdpBreakpointActive;
+        KdpBreakpointTable[i].Flags   = KD_BREAKPOINT_ACTIVE;
 
         /* Write the breakpoint */
         Status = KdpCopyMemoryChunks((ULONG_PTR)Address,
@@ -83,11 +87,140 @@ KdpAddBreakpoint(IN PVOID Address)
             /* This should never happen */
             KdpDprintf("Unable to write breakpoint to address 0x%p\n", Address);
         }
+    }
+    else
+    {
+        /* Memory is inaccessible now, setting breakpoint is deferred */
+        KdpDprintf("Failed to set breakpoint at address 0x%p, adding deferred breakpoint.\n", Address);
+        KdpBreakpointTable[i].Flags = KD_BREAKPOINT_ACTIVE | KD_BREAKPOINT_PENDING;
+        KdpOweBreakpoint = TRUE;
+    }
 
     /* Return the breakpoint handle */
     return i + 1;
 }
 
+VOID
+NTAPI
+KdSetOwedBreakpoints(VOID)
+{
+    BOOLEAN Enable;
+    KD_BREAKPOINT_TYPE Content;
+    ULONG i;
+    NTSTATUS Status;
+
+    /* If we don't owe any breakpoints, just return */
+    if (!KdpOweBreakpoint) return;
+
+    /* Enter the debugger */
+    Enable = KdEnterDebugger(NULL, NULL);
+
+    /*
+     * Suppose we succeed in setting all the breakpoints.
+     * If we fail to do so, the flag will be set again.
+     */
+    KdpOweBreakpoint = FALSE;
+
+    /* Loop through current breakpoints and try to set or delete the pending ones */
+    for (i = 0; i < KD_BREAKPOINT_MAX; i++)
+    {
+        if (KdpBreakpointTable[i].Flags & (KD_BREAKPOINT_PENDING | KD_BREAKPOINT_EXPIRED))
+        {
+            /*
+             * Set the breakpoint only if it is in kernel space, or if it is
+             * in user space and the active process context matches.
+             */
+            if (KdpBreakpointTable[i].Address < KD_HIGHEST_USER_BREAKPOINT_ADDRESS &&
+                KdpBreakpointTable[i].DirectoryTableBase != KeGetCurrentThread()->ApcState.Process->DirectoryTableBase[0])
+            {
+                KdpOweBreakpoint = TRUE;
+                continue;
+            }
+
+            /* Try to save the old instruction */
+            Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[i].Address,
+                                         &Content,
+                                         KD_BREAKPOINT_SIZE,
+                                         0,
+                                         MMDBG_COPY_UNSAFE,
+                                         NULL);
+            if (!NT_SUCCESS(Status))
+            {
+                /* Memory is still inaccessible, breakpoint setting will be deferred again */
+                // KdpDprintf("Failed to set deferred breakpoint at address 0x%p\n",
+                //            KdpBreakpointTable[i].Address);
+                KdpOweBreakpoint = TRUE;
+                continue;
+            }
+
+            /* Check if we need to write the breakpoint */
+            if (KdpBreakpointTable[i].Flags & KD_BREAKPOINT_PENDING)
+            {
+                /* Memory accessible, set the breakpoint */
+                KdpBreakpointTable[i].Content = Content;
+
+                /* Write the breakpoint */
+                Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[i].Address,
+                                             &KdpBreakpointInstruction,
+                                             KD_BREAKPOINT_SIZE,
+                                             0,
+                                             MMDBG_COPY_UNSAFE | MMDBG_COPY_WRITE,
+                                             NULL);
+                if (!NT_SUCCESS(Status))
+                {
+                    /* This should never happen */
+                    KdpDprintf("Unable to write deferred breakpoint to address 0x%p\n",
+                               KdpBreakpointTable[i].Address);
+                    KdpOweBreakpoint = TRUE;
+                }
+                else
+                {
+                    KdpBreakpointTable[i].Flags = KD_BREAKPOINT_ACTIVE;
+                }
+
+                continue;
+            }
+
+            /* Check if we need to restore the original instruction */
+            if (KdpBreakpointTable[i].Flags & KD_BREAKPOINT_EXPIRED)
+            {
+                /* Write it back */
+                Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[i].Address,
+                                             &KdpBreakpointTable[i].Content,
+                                             KD_BREAKPOINT_SIZE,
+                                             0,
+                                             MMDBG_COPY_UNSAFE | MMDBG_COPY_WRITE,
+                                             NULL);
+                if (!NT_SUCCESS(Status))
+                {
+                    /* This should never happen */
+                    KdpDprintf("Failed to delete deferred breakpoint at address 0x%p\n",
+                               KdpBreakpointTable[i].Address);
+                    KdpOweBreakpoint = TRUE;
+                }
+                else
+                {
+                    /* Check if the breakpoint is suspended */
+                    if (KdpBreakpointTable[i].Flags & KD_BREAKPOINT_SUSPENDED)
+                    {
+                        KdpBreakpointTable[i].Flags = KD_BREAKPOINT_SUSPENDED | KD_BREAKPOINT_ACTIVE;
+                    }
+                    else
+                    {
+                        /* Invalidate it */
+                        KdpBreakpointTable[i].Flags = 0;
+                    }
+                }
+
+                continue;
+            }
+        }
+    }
+
+    /* Exit the debugger */
+    KdExitDebugger(Enable);
+}
+
 BOOLEAN
 NTAPI
 KdpLowWriteContent(IN ULONG BpIndex)
@@ -95,10 +228,10 @@ KdpLowWriteContent(IN ULONG BpIndex)
     NTSTATUS Status;
 
     /* Make sure that the breakpoint is actually active */
-    if (KdpBreakpointTable[BpIndex].Flags & KdpBreakpointPending)
+    if (KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_PENDING)
     {
         /* So we have a valid breakpoint, but it hasn't been used yet... */
-        KdpBreakpointTable[BpIndex].Flags &= ~KdpBreakpointPending;
+        KdpBreakpointTable[BpIndex].Flags &= ~KD_BREAKPOINT_PENDING;
         return TRUE;
     }
 
@@ -110,8 +243,7 @@ KdpLowWriteContent(IN ULONG BpIndex)
     }
 
     /* We have an active breakpoint with an instruction to bring back. Do it. */
-    Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[BpIndex].
+    Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[BpIndex].Address,
-                                 Address,
                                  &KdpBreakpointTable[BpIndex].Content,
                                  KD_BREAKPOINT_SIZE,
                                  0,
@@ -119,9 +251,11 @@ KdpLowWriteContent(IN ULONG BpIndex)
                                  NULL);
     if (!NT_SUCCESS(Status))
     {
-        /* TODO: Set it as a owed breakpoint */
+        /* Memory is inaccessible now, restoring original instruction is deferred */
         KdpDprintf("Failed to delete breakpoint at address 0x%p\n",
                    KdpBreakpointTable[BpIndex].Address);
+        KdpBreakpointTable[BpIndex].Flags |= KD_BREAKPOINT_EXPIRED;
+        KdpOweBreakpoint = TRUE;
         return FALSE;
     }
 
@@ -136,23 +270,22 @@ KdpLowRestoreBreakpoint(IN ULONG BpIndex)
     NTSTATUS Status;
 
     /* Were we not able to remove it earlier? */
-    if (KdpBreakpointTable[BpIndex].Flags & KdpBreakpointExpired)
+    if (KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_EXPIRED)
     {
-        /* Well then, we'll just re-use it and return success! */
+        /* Just re-use it! */
-        KdpBreakpointTable[BpIndex].Flags &= ~KdpBreakpointExpired;
+        KdpBreakpointTable[BpIndex].Flags &= ~KD_BREAKPOINT_EXPIRED;
         return TRUE;
     }
 
     /* Are we merely writing a breakpoint on top of another breakpoint? */
     if (KdpBreakpointTable[BpIndex].Content == KdpBreakpointInstruction)
     {
-        /* Nothing to do then... */
+        /* Nothing to do */
         return TRUE;
     }
 
     /* Ok, we actually have to overwrite the instruction now */
-    Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[BpIndex].
+    Status = KdpCopyMemoryChunks((ULONG_PTR)KdpBreakpointTable[BpIndex].Address,
-                                 Address,
                                  &KdpBreakpointInstruction,
                                  KD_BREAKPOINT_SIZE,
                                  0,
@@ -160,14 +293,15 @@ KdpLowRestoreBreakpoint(IN ULONG BpIndex)
                                  NULL);
     if (!NT_SUCCESS(Status))
     {
-        /* FIXME: Set it as a owed breakpoint */
         KdpDprintf("Failed to restore breakpoint at address 0x%p\n",
                    KdpBreakpointTable[BpIndex].Address);
+        KdpBreakpointTable[BpIndex].Flags |= KD_BREAKPOINT_PENDING;
+        KdpOweBreakpoint = TRUE;
         return FALSE;
     }
 
     /* Clear any possible previous pending flag and return success */
-    KdpBreakpointTable[BpIndex].Flags &= ~KdpBreakpointPending;
+    KdpBreakpointTable[BpIndex].Flags &= ~KD_BREAKPOINT_PENDING;
     return TRUE;
 }
 
@@ -178,16 +312,16 @@ KdpDeleteBreakpoint(IN ULONG BpEntry)
     ULONG BpIndex = BpEntry - 1;
 
     /* Check for invalid breakpoint entry */
-    if (!(BpEntry) || (BpEntry > KD_BREAKPOINT_MAX)) return FALSE;
+    if (!BpEntry || (BpEntry > KD_BREAKPOINT_MAX)) return FALSE;
 
     /* If the specified breakpoint table entry is not valid, then return FALSE. */
     if (!KdpBreakpointTable[BpIndex].Flags) return FALSE;
 
     /* Check if the breakpoint is suspended */
-    if (KdpBreakpointTable[BpIndex].Flags & KdpBreakpointSuspended)
+    if (KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_SUSPENDED)
     {
-        /* Check if breakpoint is not ...? */
+        /* Check if breakpoint is not being deleted */
-        if (!(KdpBreakpointTable[BpIndex].Flags & KdpBreakpointExpired))
+        if (!(KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_EXPIRED))
         {
             /* Invalidate it and return success */
             KdpBreakpointTable[BpIndex].Flags = 0;
@@ -215,7 +349,7 @@ KdpDeleteBreakpointRange(IN PVOID Base,
     for (BpIndex = 0; BpIndex < KD_BREAKPOINT_MAX; BpIndex++)
     {
         /* Make sure that the breakpoint is active and matches the range. */
-        if ((KdpBreakpointTable[BpIndex].Flags & KdpBreakpointActive) &&
+        if ((KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_ACTIVE) &&
             ((KdpBreakpointTable[BpIndex].Address >= Base) &&
              (KdpBreakpointTable[BpIndex].Address <= Limit)))
         {
@@ -241,11 +375,11 @@ KdpRestoreAllBreakpoints(VOID)
     for (BpIndex = 0; BpIndex < KD_BREAKPOINT_MAX; BpIndex++)
     {
         /* Check if they are valid, suspended breakpoints */
-        if ((KdpBreakpointTable[BpIndex].Flags & KdpBreakpointActive) &&
+        if ((KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_ACTIVE) &&
-            (KdpBreakpointTable[BpIndex].Flags & KdpBreakpointSuspended))
+            (KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_SUSPENDED))
         {
             /* Unsuspend them */
-            KdpBreakpointTable[BpIndex].Flags &= ~KdpBreakpointSuspended;
+            KdpBreakpointTable[BpIndex].Flags &= ~KD_BREAKPOINT_SUSPENDED;
             KdpLowRestoreBreakpoint(BpIndex);
         }
     }
@@ -258,11 +392,11 @@ KdpSuspendBreakPoint(IN ULONG BpEntry)
     ULONG BpIndex = BpEntry - 1;
 
     /* Check if this is a valid, unsuspended breakpoint */
-    if ((KdpBreakpointTable[BpIndex].Flags & KdpBreakpointActive) &&
+    if ((KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_ACTIVE) &&
-        !(KdpBreakpointTable[BpIndex].Flags & KdpBreakpointSuspended))
+        !(KdpBreakpointTable[BpIndex].Flags & KD_BREAKPOINT_SUSPENDED))
     {
         /* Suspend it */
-        KdpBreakpointTable[BpIndex].Flags |= KdpBreakpointSuspended;
+        KdpBreakpointTable[BpIndex].Flags |= KD_BREAKPOINT_SUSPENDED;
         KdpLowWriteContent(BpIndex);
     }
 }

reactos/ntoskrnl/kd64/kdinit.c

@@ -281,7 +281,7 @@ KdInitSystem(IN ULONG BootPhase,
     KdDebuggerDataBlock.KernBase = (ULONG_PTR)KdVersionBlock.KernBase;
 
     /* Initialize the debugger if requested */
-    if ((EnableKd) && (NT_SUCCESS(KdDebuggerInitialize0(LoaderBlock))))
+    if (EnableKd && (NT_SUCCESS(KdDebuggerInitialize0(LoaderBlock))))
     {
         /* Now set our real KD routine */
         KiDebugRoutine = KdpTrap;
@@ -293,6 +293,15 @@ KdInitSystem(IN ULONG BootPhase,
             KdpContext.KdpDefaultRetries = 20;
             KiDebugSwitchRoutine = KdpSwitchProcessor;
 
+            /* Initialize breakpoints owed flag and table */
+            KdpOweBreakpoint = FALSE;
+            for (i = 0; i < KD_BREAKPOINT_MAX; i++)
+            {
+                KdpBreakpointTable[i].Flags   = 0;
+                KdpBreakpointTable[i].DirectoryTableBase = 0;
+                KdpBreakpointTable[i].Address = NULL;
+            }
+
             /* Initialize the Time Slip DPC */
             KeInitializeDpc(&KdpTimeSlipDpc, KdpTimeSlipDpcRoutine, NULL);
             KeInitializeTimer(&KdpTimeSlipTimer);

reactos/ntoskrnl/ke/amd64/cpu.c

@@ -136,7 +136,7 @@ KiGetFeatureBits(VOID)
 {
     PKPRCB Prcb = KeGetCurrentPrcb();
     ULONG Vendor;
-    ULONG FeatureBits = KF_WORKING_PTE;;
+    ULONG FeatureBits = KF_WORKING_PTE;
     CPU_INFO CpuInfo;
 
     /* Get the Vendor ID */

reactos/ntoskrnl/ke/amd64/trap.S

@@ -15,6 +15,7 @@ EXTERN KiDispatchException:PROC
 EXTERN FrLdrDbgPrint:DWORD
 EXTERN KeBugCheckWithTf:PROC
 EXTERN MmAccessFault:PROC
+EXTERN KdSetOwedBreakpoints:PROC
 EXTERN KiSystemFatalException:PROC
 EXTERN KiNpxNotAvailableFaultHandler:PROC
 EXTERN KiGeneralProtectionFaultHandler:PROC
@@ -508,8 +509,16 @@ FUNC KiPageFault
 
     /* Check for success */
     test eax, eax
-    jge PageFaultReturn
+    jl PageFaultError
 
+    /*
+     * We succeeded. Check whether the kernel debugger has
+     * owed breakpoints to be inserted, then return.
+     */
+    call KdSetOwedBreakpoints
+    jmp PageFaultReturn
+
+PageFaultError:
     /* Disable interrupts again for the debugger */
     cli
 
@@ -527,7 +536,6 @@ FUNC KiPageFault
     je SpecialCode
 
 InPageException:
-
     /* Dispatch in-page exception */
     mov r11d, eax // Param3 = Status
     mov eax, STATUS_IN_PAGE_ERROR // ExceptionCode

reactos/ntoskrnl/ke/i386/traphdlr.c

@@ -1275,7 +1275,15 @@ KiTrap0EHandler(IN PKTRAP_FRAME TrapFrame)
                            (PVOID)Cr2,
                            KiUserTrap(TrapFrame),
                            TrapFrame);
-    if (NT_SUCCESS(Status)) KiEoiHelper(TrapFrame);
+    if (NT_SUCCESS(Status))
+    {
+        /*
+         * We succeeded. Check whether the kernel debugger has
+         * owed breakpoints to be inserted, then return.
+         */
+        KdSetOwedBreakpoints();
+        KiEoiHelper(TrapFrame);
+    }
 
     /* Check for syscall fault */
 #if 0