Re: [v4 1/4] ntdll: Proper handling of debug regs on x86-64.

[Sebastian Lackner]

On 12.02.2017 23:41, Andrew Wesie wrote:
> Based on existing code for i386. Code is shared when possible to avoid too
> much duplication, even though this leads to more ifdefs.
> Signed-off-by: Andrew Wesie
>
> dlls/ntdll/ntdll_misc.h    | 28 ++++++++++----------
>  dlls/ntdll/signal_x86_64.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
>  dlls/ntdll/thread.c        | 12 +++++++--
>  3 files changed, 85 insertions(+), 19 deletions(-)
>
> diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h
> index 5e4c39e..5c5cbd2 100644
> --- a/dlls/ntdll/ntdll_misc.h
> +++ b/dlls/ntdll/ntdll_misc.h
> @@ -215,25 +215,27 @@ struct debug_info
>  /* thread private data, stored in NtCurrentTeb()->SpareBytes1 */
>  struct ntdll_thread_data
>  {
> +#if defined(__i386__) || defined(__x86_64__)
> - DWORD_PTR          dr0;           /* 1bc/2e8 Debug registers */
> - DWORD_PTR          dr1;           /* 1c0/2f0 */
> - DWORD_PTR          dr2;           /* 1c4/2f8 */
> - DWORD_PTR          dr3;           /* 1c8/300 */
> - DWORD_PTR          dr6;           /* 1cc/308 */
> - DWORD_PTR          dr7;           /* 1d0/310 */
>
> +#endif
>  #ifdef __i386__
> - DWORD              dr0;           /* 1bc Debug registers */
> - DWORD              dr1;           /* 1c0 */
> - DWORD              dr2;           /* 1c4 */
> - DWORD              dr3;           /* 1c8 */
> - DWORD              dr6;           /* 1cc */
> - DWORD              dr7;           /* 1d0 */
> DWORD              fs;            /* 1d4 TEB selector */
> DWORD              gs;            /* 1d8 libc selector; update winebuild if you move this! */
> void              *vm86_ptr;      /* 1dc data for vm86 mode */
>
> #else
> - void              *exit_frame;    /*    /2e8 exit frame pointer */
> - void              *exit_frame;    /*    /318 exit frame pointer */
>
> #endif
> - struct debug_info *debug_info;    /* 1e0/2f0 info for debugstr functions */
> - int                request_fd;    /* 1e4/2f8 fd for sending server requests */
> - int                reply_fd;      /* 1e8/2fc fd for receiving server replies */
> - int                wait_fd[2];    /* 1ec/300 fd for sleeping server requests */
> - BOOL               wow64_redir;   /* 1f4/308 Wow64 filesystem redirection flag */
> - pthread_t          pthread_id;    /* 1f8/310 pthread thread id */
> - struct debug_info *debug_info;    /* 1e0/320 info for debugstr functions */
> - int                request_fd;    /* 1e4/328 fd for sending server requests */
> - int                reply_fd;      /* 1e8/32c fd for receiving server replies */
> - int                wait_fd[2];    /* 1ec/330 fd for sleeping server requests */
> - BOOL               wow64_redir;   /* 1f4/338 Wow64 filesystem redirection flag */
> - pthread_t          pthread_id;    /* 1f8/340 pthread thread id */
>
> #ifdef __i386__
>      WINE_VM86_TEB_INFO vm86;          /* 1fc vm86 private data */
>      void              *exit_frame;    /* 204 exit frame pointer */

Adding those fields should work, but it is a bit dangerous because we only have
limited space. I would suggest adding asserts to ensure we never make this struct
too big. Something like this should work:

C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + sizeof(struct ntdll_thread_data) <=
          FIELD_OFFSET(TEB, GdiTebBatch) + sizeof(((TEB *)0)->GdiTebBatch) );

Probably we should also use it for important i386 fields, to ensure they are not
moved.

#ifdef __i386__
C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct ntdll_thread_data, vm86) == FIELD_OFFSET(TEB, GdiTebBatch) );
C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct ntdll_thread_data, vm86) == 0x1fc );
C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct ntdll_thread_data, gs)   == 0x1d8 );
#endif
> diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c
> index f33fe4c..d2b040e 100644
> --- a/dlls/ntdll/signal_x86_64.c
> +++ b/dlls/ntdll/signal_x86_64.c
> @@ -1649,7 +1649,9 @@ static inline BOOL is_inside_signal_stack( void *ptr )
>   */
>  static void save_context( CONTEXT *context, const ucontext_t *sigcontext )
>  {
> - context->ContextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER | CONTEXT_SEGMENTS;
> - struct ntdll_thread_data * const regs = ntdll_get_thread_data();
> -
> - context->ContextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER | CONTEXT_SEGMENTS | CONTEXT_DEBUG_REGISTERS;
> context->Rax    = RAX_sig(sigcontext);
> context->Rcx    = RCX_sig(sigcontext);
> context->Rdx    = RDX_sig(sigcontext);
>
> @@ -1686,6 +1688,12 @@ static void save_context( CONTEXT *context, const ucontext_t *sigcontext )
>  #else
>      __asm__("movw %%ss,%0" : "=m" (context->SegSs));
>  #endif
> - context->Dr0          = regs->dr0;
> - context->Dr1          = regs->dr1;
> - context->Dr2          = regs->dr2;
> - context->Dr3          = regs->dr3;
> - context->Dr6          = regs->dr6;
> - context->Dr7          = regs->dr7;
> if (FPU_sig(sigcontext))
> {
>     context->ContextFlags |= CONTEXT_FLOATING_POINT;
>
> @@ -1702,6 +1710,14 @@ static void save_context( CONTEXT *context, const ucontext_t *sigcontext )
>   */
>  static void restore_context( const CONTEXT *context, ucontext_t *sigcontext )
>  {
> - struct ntdll_thread_data * const regs = ntdll_get_thread_data();
> -
> - regs->dr0 = context->Dr0;
> - regs->dr1 = context->Dr1;
> - regs->dr2 = context->Dr2;
> - regs->dr3 = context->Dr3;
> - regs->dr6 = context->Dr6;
> - regs->dr7 = context->Dr7;
> RAX_sig(sigcontext) = context->Rax;
> RCX_sig(sigcontext) = context->Rcx;
> RDX_sig(sigcontext) = context->Rdx;
>
> @@ -1861,6 +1877,16 @@ __ASM_GLOBAL_FUNC( set_full_cpu_context,
>  void set_cpu_context( const CONTEXT *context )
>  {
>      DWORD flags = context->ContextFlags & ~CONTEXT_AMD64;
> -
> - if (flags & CONTEXT_DEBUG_REGISTERS)
> - {
> -
>    ntdll_get_thread_data()->dr0 = context->Dr0;
> -
>    ntdll_get_thread_data()->dr1 = context->Dr1;
> -
>    ntdll_get_thread_data()->dr2 = context->Dr2;
> -
>    ntdll_get_thread_data()->dr3 = context->Dr3;
> -
>    ntdll_get_thread_data()->dr6 = context->Dr6;
> -
>    ntdll_get_thread_data()->dr7 = context->Dr7;
> - }
> if (flags & CONTEXT_FULL)
> {
>     if (!(flags & CONTEXT_CONTROL))
>
> @@ -2559,7 +2585,7 @@ static void raise_segv_exception( EXCEPTION_RECORD *rec, CONTEXT *context )
>          }
>          break;
>      }
> - status = raise_exception( rec, context, TRUE );
> - status = NtRaiseException( rec, context, TRUE );
> if (status) raise_status( status, rec );

NtRaiseException will always return a status != 0, so there
is no need for the check anymore.
> done:
>      set_cpu_context( context );
> @@ -2567,13 +2593,43 @@ done:
>
> /**********************************************************************
> -
> -
> raise_trap_exception
> - */
>
> +static void raise_trap_exception( EXCEPTION_RECORD *rec, CONTEXT *context )
> +{
> - NTSTATUS status;
> -
> - if (rec->ExceptionCode == EXCEPTION_SINGLE_STEP)
> - {
> -
>    /* when single stepping can't tell whether this is a hw bp or a
> -
>     * single step interrupt. try to avoid as much overhead as possible
> -
>     * and only do a server call if there is any hw bp enabled. */
> -
> -
>    if( !(context->EFlags & 0x100) || (ntdll_get_thread_data()->dr7 & 0xff) )
> -
>    {
> -
>        /* (possible) hardware breakpoint, fetch the debug registers */
> -
>        DWORD saved_flags = context->ContextFlags;
> -
>        context->ContextFlags = CONTEXT_DEBUG_REGISTERS;
> -
>        NtGetContextThread(GetCurrentThread(), context);
> -
>        context->ContextFlags |= saved_flags;  /* restore flags */
> -
>    }
> -
> -
>    context->EFlags &= ~0x100;  /* clear single-step flag */
> - }
> -
> - status = NtRaiseException( rec, context, TRUE );
> - raise_status( status, rec );
>
> +}
> -
> -
>
> +/**********************************************************************
> -
> raise_generic_exception
> -
> - Generic raise function for exceptions that don't need special treatment.
>
> */
>  static void raise_generic_exception( EXCEPTION_RECORD *rec, CONTEXT *context )
>  {
> - NTSTATUS status = raise_exception( rec, context, TRUE );
> - NTSTATUS status = NtRaiseException( rec, context, TRUE );
> if (status) raise_status( status, rec );
> set_cpu_context( context );

Similar to above, this can be simplified because status is always != 0.
> }
> @@ -2684,7 +2740,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext )
>   */
>  static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext )
>  {
> - EXCEPTION_RECORD *rec = setup_exception( sigcontext, raise_generic_exception );
> -
>
> EXCEPTION_RECORD *rec = setup_exception( sigcontext, raise_trap_exception );
>
> switch (siginfo->si_code)
> {
>
> diff --git a/dlls/ntdll/thread.c b/dlls/ntdll/thread.c
> index c9a5da6..934ada1 100644
> --- a/dlls/ntdll/thread.c
> +++ b/dlls/ntdll/thread.c
> @@ -744,10 +744,14 @@ NTSTATUS WINAPI NtSetContextThread( HANDLE handle, const CONTEXT *context )
>      DWORD dummy, i;
>      BOOL self;
>
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>      /* on i386 debug registers always require a server call */
>      self = (handle == GetCurrentThread());
> +#ifdef __i386__
>      if (self && (context->ContextFlags & (CONTEXT_DEBUG_REGISTERS & ~CONTEXT_i386)))
> +#elif defined(__x86_64__)
> - if (self && (context->ContextFlags & (CONTEXT_DEBUG_REGISTERS & ~CONTEXT_AMD64)))
>
> +#endif
>      {
>          self = (ntdll_get_thread_data()->dr0 == context->Dr0 &&
>                  ntdll_get_thread_data()->dr1 == context->Dr1 &&
> @@ -903,9 +907,13 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context )
>              copy_context( context, &ctx, ctx.ContextFlags & needed_flags );
>              context->ContextFlags |= ctx.ContextFlags & needed_flags;
>          }
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>          /* update the cached version of the debug registers */
> +#ifdef __i386__
>          if (context->ContextFlags & (CONTEXT_DEBUG_REGISTERS & ~CONTEXT_i386))
> +#elif defined(__x86_64__)
> -
>    if (context->ContextFlags & (CONTEXT_DEBUG_REGISTERS & ~CONTEXT_AMD64))
>
> +#endif
>          {
>              ntdll_get_thread_data()->dr0 = context->Dr0;
>              ntdll_get_thread_data()->dr1 = context->Dr1;
> -- 2.7.4

[v4 1/4] ntdll: Proper handling of debug regs on x86-64.

[Andrew Wesie]

On Mon, Feb 13, 2017 at 10:41 AM, Sebastian Lackner 
wrote:
> Adding those fields should work, but it is a bit dangerous because we only
> have
> limited space. I would suggest adding asserts to ensure we never make this
> struct
> too big. Something like this should work:
>
> C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + sizeof(struct
> ntdll_thread_data) <=
>           FIELD_OFFSET(TEB, GdiTebBatch) + sizeof(((TEB *)0)->GdiTebBatch)
> );

Is the following acceptable:

C_ASSERT( sizeof(struct ntdll_thread_data) <= FIELD_OFFSET(TEB, gdiRgn) -
FIELD_OFFSET(TEB, SpareBytes1) );

It should be equivalent, but it is shorter and has less parenthesis.
> Probably we should also use it for important i386 fields, to ensure they
> are not
> moved.
>
> #ifdef __i386__
> C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct
> ntdll_thread_data, vm86) == FIELD_OFFSET(TEB, GdiTebBatch) );
> C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct
> ntdll_thread_data, vm86) == 0x1fc );
> C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct
> ntdll_thread_data, gs)   == 0x1d8 );
> #endif

Looks good to me. I'll add that to the patch.

-Andrew

[v4 1/4] ntdll: Proper handling of debug regs on x86-64.

[Sebastian Lackner]

On 13.02.2017 23:00, Andrew Wesie wrote:
> On Mon, Feb 13, 2017 at 10:41 AM, Sebastian Lackner 
> wrote:
> > Adding those fields should work, but it is a bit dangerous because we only
> > have
> > limited space. I would suggest adding asserts to ensure we never make this
> > struct
> > too big. Something like this should work:
> >
> > C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + sizeof(struct
> > ntdll_thread_data) <=
> >           FIELD_OFFSET(TEB, GdiTebBatch) + sizeof(((TEB *)0)->GdiTebBatch)
> > );
>
> Is the following acceptable:
>
> C_ASSERT( sizeof(struct ntdll_thread_data) <= FIELD_OFFSET(TEB, gdiRgn) -
> FIELD_OFFSET(TEB, SpareBytes1) );
>
> It should be equivalent, but it is shorter and has less parenthesis.

Yes, that should also be fine.
> > Probably we should also use it for important i386 fields, to ensure they
> > are not
> > moved.
> >
> > #ifdef __i386__
> > C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct
> > ntdll_thread_data, vm86) == FIELD_OFFSET(TEB, GdiTebBatch) );
> > C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct
> > ntdll_thread_data, vm86) == 0x1fc );
> > C_ASSERT( FIELD_OFFSET(TEB, SpareBytes1) + FIELD_OFFSET(struct
> > ntdll_thread_data, gs)   == 0x1d8 );
> > #endif
>
> Looks good to me. I'll add that to the patch.
>
> -Andrew