Re: [PATCH v2] ntoskrnl.exe: Make USD pointers volatile to silent a warning.

[Jinoh Kang]

On 3/30/22 04:08, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon
>
> dlls/ntoskrnl.exe/instr.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c
> index 8f1aa4d45a3..0aba2413ab1 100644
> --- a/dlls/ntoskrnl.exe/instr.c
> +++ b/dlls/ntoskrnl.exe/instr.c
> @@ -496,9 +496,14 @@ WINE_DEFAULT_DEBUG_CHANNEL(int);
>  #define SIB_INDEX( sib, rex )   (((sib) >> 3) & 7) | (((rex) & REX_X) ? 8 : 0)
>  #define SIB_BASE( sib, rex )    (((sib) & 7) | (((rex) & REX_B) ? 8 : 0))
>
> -/* keep in sync with dlls/ntdll/thread.c:thread_init */
> -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000;
> -static const BYTE *user_shared_data      = (BYTE *)0xfffff78000000000;
> +/* keep in sync with dlls/ntdll/thread.c:thread_init.
> -
> -
> -
> - Both the data and the pointers need to be volatile, as GCC 11
> -
> - considers that a fixed value pointer is an empty array, and will
> -
> - emit warnings when dereferencing it otherwise.
> - */
>
> +static const volatile BYTE *const volatile wine_user_shared_data = (BYTE *)0x7ffe0000;
> +static const volatile BYTE *const volatile user_shared_data      = (BYTE *)0xfffff78000000000;
>
> static inline DWORD64 *get_int_reg( CONTEXT *context, int index )
>  {
> @@ -516,7 +521,7 @@ static inline int get_op_size( int long_op, int rex )
>  }
>
> /* store an operand into a register */
> -static void store_reg_word( CONTEXT *context, BYTE regmodrm, const BYTE *addr, int long_op, int rex,
> +static void store_reg_word( CONTEXT *context, BYTE regmodrm, const volatile BYTE *addr, int long_op, int rex,
>          enum instr_op op )
>  {
>      int index = REGMODRM_REG( regmodrm, rex );
> @@ -527,7 +532,7 @@ static void store_reg_word( CONTEXT *context, BYTE regmodrm, const BYTE *addr, i
>      switch (op)
>      {
>          case INSTR_OP_MOV:
> -
>        memcpy( reg, addr, op_size );
> -
>        memcpy( reg, (BYTE *)addr, op_size );

I think we should not use memcpy() here in the first place.

1. There is no guarantee that memcpy() is atomic. The load might race with USD update from wineserver, causing inconsistent tick counter read.
2. Not using memcpy() avoids the GCC 11 -Warray-bounds warning in the first place.
3. Unaligned pointers wouldn't confuse the optimizer as long as the access is volatile.

I think what we need here is an helper that accurately emulates the memory load.  Something along the lines of:

static void load_volatile( void *dest, const volatile void *src, size_t size )
{
    switch (size)
    {
    case sizeof(BYTE): *(BYTE *)dest = *(volatile BYTE *)src;
    case sizeof(WORD): *(WORD *)dest = *(volatile WORD *)src;
    case sizeof(DWORD): *(DWORD *)dest = *(volatile DWORD *)src;
    case sizeof(QWORD): *(QWORD *)dest = *(volatile QWORD *)src;
    default: assert(0);
    }
}
>          break;
>      case INSTR_OP_OR:
>          for (i = 0; i < op_size; ++i)
>
> @@ -541,7 +546,7 @@ static void store_reg_word( CONTEXT *context, BYTE regmodrm, const BYTE *addr, i
>  }
>
> /* store an operand into a byte register */
> -static void store_reg_byte( CONTEXT *context, BYTE regmodrm, const BYTE *addr, int rex, enum instr_op op )
> +static void store_reg_byte( CONTEXT *context, BYTE regmodrm, const volatile BYTE *addr, int rex, enum instr_op op )
>  {
>      int index = REGMODRM_REG( regmodrm, rex );
>      BYTE *reg = (BYTE *)get_int_reg( context, index );
> @@ -843,7 +848,7 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context )
>                  ULONGLONG temp = 0;
>              TRACE("USD offset %#x at %p.\n", (unsigned int)offset, (void *)context->Rip);
> -
>            memcpy( &temp, wine_user_shared_data + offset, data_size );
> -
>            memcpy( &temp, (BYTE *)wine_user_shared_data + offset, data_size );
>            store_reg_word( context, instr[2], (BYTE *)&temp, long_op, rex, INSTR_OP_MOV );
>            context->Rip += prefixlen + len + 2;
>            return ExceptionContinueExecution;
>
> @@ -902,7 +907,7 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context )
>          if (offset <= KSHARED_USER_DATA_PAGE_SIZE - data_size)
>          {
>              TRACE("USD offset %#x at %p.\n", (unsigned int)offset, (void *)context->Rip);
> -
>        memcpy( &context->Rax, wine_user_shared_data + offset, data_size );
> -
>        memcpy( &context->Rax, (BYTE *)wine_user_shared_data + offset, data_size );
>        context->Rip += prefixlen + len + 1;
>        return ExceptionContinueExecution;
>    }