[PATCH] ntoskrnl.exe: Make USD pointers volatile to silent a warning.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..05976c38b27 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -497,8 +497,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #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; +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 ) { @@ -843,7 +843,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; @@ -869,19 +869,19 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) switch (*instr) { case 0x8a: - store_reg_byte( context, instr[1], wine_user_shared_data + offset, + store_reg_byte( context, instr[1], (BYTE *)wine_user_shared_data + offset, rex, INSTR_OP_MOV ); break; case 0x8b: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_MOV ); break; case 0x0b: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_OR ); break; case 0x33: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_XOR ); break; } @@ -902,7 +902,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; } -- 2.35.1
On 3/29/22 12:39, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..05976c38b27 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -497,8 +497,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #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; +static const volatile BYTE *const volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const volatile BYTE *const volatile user_shared_data = (BYTE *)0xfffff78000000000;
I might be misunderstanding something, but I don't think "static const volatile" makes sense? I.e. the second "volatile" probably shouldn't be there.
static inline DWORD64 *get_int_reg( CONTEXT *context, int index ) { @@ -843,7 +843,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; @@ -869,19 +869,19 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) switch (*instr) { case 0x8a: - store_reg_byte( context, instr[1], wine_user_shared_data + offset, + store_reg_byte( context, instr[1], (BYTE *)wine_user_shared_data + offset, rex, INSTR_OP_MOV ); break; case 0x8b: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_MOV ); break; case 0x0b: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_OR ); break; case 0x33: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_XOR ); break; } @@ -902,7 +902,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; }
On 3/29/22 20:21, Zebediah Figura wrote:
On 3/29/22 12:39, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..05976c38b27 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -497,8 +497,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #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; +static const volatile BYTE *const volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const volatile BYTE *const volatile user_shared_data = (BYTE *)0xfffff78000000000;
I might be misunderstanding something, but I don't think "static const volatile" makes sense? I.e. the second "volatile" probably shouldn't be there.
I think we had the discussion already last time I sent the patch, and this is the version we ended up upon. The warning fix is actually about making the pointers themselves volatile, GCC 11 doesn't like us accessing a pointer to a fixed address, which it considers as an empty array. The other volatile is then here for correctness only, and to make the second one less confusing maybe, because it's how it should be (and it actually then requires the casts in the memcpy calls). Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On 3/29/22 13:24, Rémi Bernon wrote:
On 3/29/22 20:21, Zebediah Figura wrote:
On 3/29/22 12:39, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/instr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..05976c38b27 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -497,8 +497,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int); #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; +static const volatile BYTE *const volatile wine_user_shared_data = (BYTE *)0x7ffe0000; +static const volatile BYTE *const volatile user_shared_data = (BYTE *)0xfffff78000000000;
I might be misunderstanding something, but I don't think "static const volatile" makes sense? I.e. the second "volatile" probably shouldn't be there.
I think we had the discussion already last time I sent the patch, and this is the version we ended up upon.
The warning fix is actually about making the pointers themselves volatile, GCC 11 doesn't like us accessing a pointer to a fixed address, which it considers as an empty array.
Right, I forgot about that... Could we maybe add a comment in the code to that effect as well?
On 3/29/22 12:39, Rémi Bernon wrote:
@@ -869,19 +869,19 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) switch (*instr) { case 0x8a: - store_reg_byte( context, instr[1], wine_user_shared_data + offset, + store_reg_byte( context, instr[1], (BYTE *)wine_user_shared_data + offset, rex, INSTR_OP_MOV ); break; case 0x8b: - store_reg_word( context, instr[1], wine_user_shared_data + offset, + store_reg_word( context, instr[1], (BYTE *)wine_user_shared_data + offset, long_op, rex, INSTR_OP_MOV ); break;
Couldn't we just modify those functions to take a "const volatile BYTE *" pointer instead?
participants (2)
-
Rémi Bernon -
Zebediah Figura