Hello Etaash; thanks for the patch! I have some comments inlined.
On 9/2/22 14:26, Etaash Mathamsetty wrote:
From: Etaash Mathamsetty etaash.mathamsetty@gmail.com
implement cmp instruction
fix minor styling
fix minor styling
Please fix the subject and patch description; we'd like all patches to be self-contained changes with coherent subjects.
dlls/ntoskrnl.exe/instr.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..cb3cce521c9 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -890,7 +890,29 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) } break; /* Unable to emulate it */ }
- case 0x83: /* cmp r/m imm8 */
This also needs to check the ModRM byte; this instruction multiplexes and CMP is only correct if the extension is 7.
- {
BYTE *data = INSTR_GetOperandAddr(context, instr + 1, prefixlen + 1, long_addr, rex, segprefix, &len);SIZE_T offset = data - user_shared_data;SIZE_T data_size = get_op_size(long_op, rex);if(offset <= KSHARED_USER_DATA_PAGE_SIZE - data_size){TRACE("USD offset %#x at %p\n", (unsigned int)offset, (void*)context->Rip);
These are copy-pasted from other lines in the same file, but with the style changed. Why?
/* Clear ZF and CF */context->EFlags &= ~(1UL << 6);context->EFlags &= ~(1UL);
I'd recommend adding symbolic #defines for these.
if(*(wine_user_shared_data + offset) == instr[2])
This is only comparing a single byte. The instruction should compare "data_size" bytes.
context->EFlags |= (1UL << 6); /* ZF */else if(*(wine_user_shared_data + offset) < instr[2])context->EFlags |= (1UL); /* CF */
This should also set OF, PF, AF, SF.
I'd recommend making this all into a helper, since it'll need to be used for other CMP or SUB instructions. (Note also that the ZF, SF, and PF only depend on the result and IIRC can be shared across all arithmetic instructions, so those may deserve a separate helper.)