From 2536bfe42c6973cdfb2529be8bdbc5dc02496fdb Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Wed, 15 Jul 2020 00:10:20 -0700 Subject: [PATCH] Correctly fixes prefix ordering in the frontend This was sort of partially worked around. The issue comes down to REX.W and Operand size prefix ordering on an instruction. When an instruction has both prefixes, then the one that has occured later will overwrite the previous one. Additionally in the case of 66h being reused as an escape prefix, we needed to ignore it in this case. The previous workaround was that if an instruction happened to have both widening and narrowing, then widening would win(for MODRM args), but then later on in the instruction decoding the literal constant decoding would properly check which order the flags came in. This resulted in literal constants working, but not modrm bytes Instead replace the "last" flag handling with a two deep stack that we can push and pop the flags correctly on to it, this ensures that the flags stay around in the 66h ignoring mode (so widening doesn't get obliterated). This effects every instruction that supports REX.W and 66h Operand size prefix, so it could be fixing some unknown bugs elsewhere. Very specifically this one was found in OpenSSL, because it looks like GCC is padding a mov instruction with 66h. Instruction Examples: Only difference is the prefix order being swapped 0x00000000 4 66488b1f mov rbx, qword [rdi] 0x00000004 4 48668b1f mov bx, word [rdi] --- .../Source/Interface/Core/Frontend.cpp | 29 ++++--------- .../Interface/Core/OpcodeDispatcher.cpp | 6 +-- .../FEXCore/include/FEXCore/Debug/X86Tables.h | 42 +++++++++++++++++-- 3 files changed, 50 insertions(+), 27 deletions(-) diff --git a/External/FEXCore/Source/Interface/Core/Frontend.cpp b/External/FEXCore/Source/Interface/Core/Frontend.cpp index aa9e95132..8266030bf 100644 --- a/External/FEXCore/Source/Interface/Core/Frontend.cpp +++ b/External/FEXCore/Source/Interface/Core/Frontend.cpp @@ -223,6 +223,8 @@ bool Decoder::NormalOp(FEXCore::X86Tables::X86InstInfo const *Info, uint16_t Op) "Group Ops should have been decoded before this!"); uint8_t DestSize{}; + bool HasWideningDisplacement = FEXCore::X86Tables::DecodeFlags::GetOpAddr(DecodeInst->Flags, 0) & FEXCore::X86Tables::DecodeFlags::FLAG_WIDENING_SIZE_LAST; + bool HasNarrowingDisplacement = FEXCore::X86Tables::DecodeFlags::GetOpAddr(DecodeInst->Flags, 0) & FEXCore::X86Tables::DecodeFlags::FLAG_OPERAND_SIZE_LAST; // New instruction size decoding { @@ -242,7 +244,7 @@ bool Decoder::NormalOp(FEXCore::X86Tables::X86InstInfo const *Info, uint16_t Op) DecodeInst->Flags |= DecodeFlags::GenSizeDstSize(DecodeFlags::SIZE_128BIT); DestSize = 16; } - else if (DecodeInst->Flags & DecodeFlags::FLAG_OPERAND_SIZE && + else if (HasNarrowingDisplacement && (DstSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_DEF || DstSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_64BITDEF)) { // See table 1-2. Operand-Size Overrides for this decoding @@ -251,7 +253,7 @@ bool Decoder::NormalOp(FEXCore::X86Tables::X86InstInfo const *Info, uint16_t Op) DestSize = 2; } else if (CTX->Config.Is64BitMode && - (DecodeInst->Flags & DecodeFlags::FLAG_REX_WIDENING || + (HasWideningDisplacement || DstSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_64BIT || DstSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_64BITDEF)) { DecodeInst->Flags |= DecodeFlags::GenSizeDstSize(DecodeFlags::SIZE_64BIT); @@ -272,7 +274,7 @@ bool Decoder::NormalOp(FEXCore::X86Tables::X86InstInfo const *Info, uint16_t Op) else if (SrcSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_128BIT) { DecodeInst->Flags |= DecodeFlags::GenSizeSrcSize(DecodeFlags::SIZE_128BIT); } - else if (DecodeInst->Flags & DecodeFlags::FLAG_OPERAND_SIZE && + else if (HasNarrowingDisplacement && (SrcSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_DEF || SrcSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_64BITDEF)) { // See table 1-2. Operand-Size Overrides for this decoding @@ -280,7 +282,7 @@ bool Decoder::NormalOp(FEXCore::X86Tables::X86InstInfo const *Info, uint16_t Op) DecodeInst->Flags |= DecodeFlags::GenSizeSrcSize(DecodeFlags::SIZE_16BIT); } else if (CTX->Config.Is64BitMode && - (DecodeInst->Flags & DecodeFlags::FLAG_REX_WIDENING || + (HasWideningDisplacement || SrcSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_64BIT || SrcSizeFlag == FEXCore::X86Tables::InstFlags::SIZE_64BITDEF)) { DecodeInst->Flags |= DecodeFlags::GenSizeSrcSize(DecodeFlags::SIZE_64BIT); @@ -295,21 +297,7 @@ bool Decoder::NormalOp(FEXCore::X86Tables::X86InstInfo const *Info, uint16_t Op) HasMODRM |= !!(Info->Flags & FEXCore::X86Tables::InstFlags::FLAGS_MODRM); bool HasSIB = false; - bool HasWideningDisplacement = DecodeInst->Flags & DecodeFlags::FLAG_REX_WIDENING; - bool HasNarrowingDisplacement = DecodeInst->Flags & DecodeFlags::FLAG_OPERAND_SIZE; - if (HasWideningDisplacement && HasNarrowingDisplacement) { - // This is a fun edge case where widening displacement has a precedence over narrowing displacement - // If you have both then widening (REX.W) takes precedence over narrowing (66h) - // Found with a fun CALL instruction in libLLVM for `call __tls_get_addr` - // Instruction was: 66 66 48 e8 a7 ff ec ff - // 66: Operand size override - // 66: Operand Size override - // 48: REX.W - // E8: call - // a7 ff ec ff: Immediate offset - HasNarrowingDisplacement = false; - } // This is used for ModRM register modification // For both modrm.reg and modrm.rm(when mod == 0b11) when value is >= 0b100 // then it changes from expected registers to the high 8bits of the lower registers @@ -650,7 +638,7 @@ bool Decoder::NormalOpHeader(FEXCore::X86Tables::X86InstInfo const *Info, uint16 // Widening displacement if (Op & 0b1000) { DecodeInst->Flags |= DecodeFlags::FLAG_REX_WIDENING; - DecodeInst->Flags = (DecodeInst->Flags & ~DecodeFlags::FLAG_OPADDR_MASK) | DecodeFlags::FLAG_WIDENING_SIZE_LAST; + DecodeFlags::PushOpAddr(&DecodeInst->Flags, DecodeFlags::FLAG_WIDENING_SIZE_LAST); } // XGPR_B bit set @@ -782,6 +770,7 @@ bool Decoder::DecodeInstruction(uint64_t PC) { // Remove prefix so it doesn't effect calculations. // This is only an escape prefix rather tan modifier now DecodeInst->Flags &= ~DecodeFlags::FLAG_OPERAND_SIZE; + DecodeFlags::PopOpAddrIf(&DecodeInst->Flags, DecodeFlags::FLAG_OPERAND_SIZE_LAST); if (NormalOpHeader(&FEXCore::X86Tables::OpSizeModOps[EscapeOp], EscapeOp)) { InstructionDecoded = true; } @@ -796,7 +785,7 @@ bool Decoder::DecodeInstruction(uint64_t PC) { case 0x66: // Operand Size prefix DecodeInst->Flags |= DecodeFlags::FLAG_OPERAND_SIZE; DecodeInst->LastEscapePrefix = Op; - DecodeInst->Flags = (DecodeInst->Flags & ~DecodeFlags::FLAG_OPADDR_MASK) | DecodeFlags::FLAG_OPERAND_SIZE_LAST; + DecodeFlags::PushOpAddr(&DecodeInst->Flags, DecodeFlags::FLAG_OPERAND_SIZE_LAST); break; case 0x67: // Address Size override prefix DecodeInst->Flags |= DecodeFlags::FLAG_ADDRESS_SIZE; diff --git a/External/FEXCore/Source/Interface/Core/OpcodeDispatcher.cpp b/External/FEXCore/Source/Interface/Core/OpcodeDispatcher.cpp index 1f148e9b0..7504785da 100644 --- a/External/FEXCore/Source/Interface/Core/OpcodeDispatcher.cpp +++ b/External/FEXCore/Source/Interface/Core/OpcodeDispatcher.cpp @@ -87,15 +87,15 @@ void OpDispatchBuilder::SyscallOp(OpcodeArgs) { void OpDispatchBuilder::LEAOp(OpcodeArgs) { if (CTX->Config.Is64BitMode) { - uint32_t DstSize = X86Tables::DecodeFlags::GetOpAddr(Op->Flags) == X86Tables::DecodeFlags::FLAG_OPERAND_SIZE_LAST ? 2 : - X86Tables::DecodeFlags::GetOpAddr(Op->Flags) == X86Tables::DecodeFlags::FLAG_WIDENING_SIZE_LAST ? 8 : 4; + uint32_t DstSize = X86Tables::DecodeFlags::GetOpAddr(Op->Flags, 0) == X86Tables::DecodeFlags::FLAG_OPERAND_SIZE_LAST ? 2 : + X86Tables::DecodeFlags::GetOpAddr(Op->Flags, 0) == X86Tables::DecodeFlags::FLAG_WIDENING_SIZE_LAST ? 8 : 4; uint32_t SrcSize = (Op->Flags & X86Tables::DecodeFlags::FLAG_ADDRESS_SIZE) ? 4 : 8; OrderedNode *Src = LoadSource_WithOpSize(GPRClass, Op, Op->Src[0], SrcSize, Op->Flags, -1, false); StoreResult_WithOpSize(GPRClass, Op, Op->Dest, Src, DstSize, -1); } else { - uint32_t DstSize = X86Tables::DecodeFlags::GetOpAddr(Op->Flags) == X86Tables::DecodeFlags::FLAG_OPERAND_SIZE_LAST ? 2 : 4; + uint32_t DstSize = X86Tables::DecodeFlags::GetOpAddr(Op->Flags, 0) == X86Tables::DecodeFlags::FLAG_OPERAND_SIZE_LAST ? 2 : 4; uint32_t SrcSize = (Op->Flags & X86Tables::DecodeFlags::FLAG_ADDRESS_SIZE) ? 2 : 4; OrderedNode *Src = LoadSource_WithOpSize(GPRClass, Op, Op->Src[0], SrcSize, Op->Flags, -1, false); diff --git a/External/FEXCore/include/FEXCore/Debug/X86Tables.h b/External/FEXCore/include/FEXCore/Debug/X86Tables.h index 05c12d748..82f5c74fd 100644 --- a/External/FEXCore/include/FEXCore/Debug/X86Tables.h +++ b/External/FEXCore/include/FEXCore/Debug/X86Tables.h @@ -49,10 +49,13 @@ constexpr uint32_t SIZE_128BIT = 0b101; constexpr uint32_t SIZE_256BIT = 0b110; constexpr uint32_t FLAG_OPADDR_OFF = (FLAG_SIZE_SRC_OFF + 3); -constexpr uint32_t FLAG_OPADDR_MASK = (0b11 << FLAG_OPADDR_OFF); +constexpr uint32_t FLAG_OPADDR_STACKSIZE = 4; // Two level deep stack +constexpr uint32_t FLAG_OPADDR_FLAG_SIZE = 2; +constexpr uint32_t FLAG_OPADDR_MASK = (((1 << FLAG_OPADDR_STACKSIZE) - 1) << FLAG_OPADDR_OFF); -constexpr uint32_t FLAG_OPERAND_SIZE_LAST = (0b01 << FLAG_OPADDR_OFF); -constexpr uint32_t FLAG_WIDENING_SIZE_LAST = (0b10 << FLAG_OPADDR_OFF); +// 00 = NONE +constexpr uint32_t FLAG_OPERAND_SIZE_LAST = 0b01; +constexpr uint32_t FLAG_WIDENING_SIZE_LAST = 0b10; inline uint32_t GetSizeDstFlags(uint32_t Flags) { return (Flags >> FLAG_SIZE_DST_OFF) & SIZE_MASK; } inline uint32_t GetSizeSrcFlags(uint32_t Flags) { return (Flags >> FLAG_SIZE_SRC_OFF) & SIZE_MASK; } @@ -60,7 +63,38 @@ inline uint32_t GetSizeSrcFlags(uint32_t Flags) { return (Flags >> FLAG_SIZE_SRC inline uint32_t GenSizeDstSize(uint32_t Size) { return Size << FLAG_SIZE_DST_OFF; } inline uint32_t GenSizeSrcSize(uint32_t Size) { return Size << FLAG_SIZE_SRC_OFF; } -inline uint32_t GetOpAddr(uint32_t Flags) { return Flags & FLAG_OPADDR_MASK; } +inline uint32_t GetOpAddr(uint32_t Flags, int Index) { return (((Flags & FLAG_OPADDR_MASK) >> FLAG_OPADDR_OFF) >> (Index * 2)) & ((1 << FLAG_OPADDR_FLAG_SIZE) - 1); } + +inline void PushOpAddr(uint32_t *Flags, uint32_t Flag) { + uint32_t TmpFlags = *Flags; + uint32_t BottomOfStack = ((TmpFlags & FLAG_OPADDR_MASK) >> FLAG_OPADDR_OFF) & ((1 << FLAG_OPADDR_FLAG_SIZE) - 1); + + TmpFlags &= ~(FLAG_OPADDR_MASK); + TmpFlags |= + (BottomOfStack << (FLAG_OPADDR_OFF + FLAG_OPADDR_FLAG_SIZE)) | + (Flag << FLAG_OPADDR_OFF); + + *Flags = TmpFlags; +} + +inline void PopOpAddrIf(uint32_t *Flags, uint32_t Flag) { + uint32_t TmpFlags = *Flags; + uint32_t BottomOfStack = ((TmpFlags & FLAG_OPADDR_MASK) >> FLAG_OPADDR_OFF) & ((1 << FLAG_OPADDR_FLAG_SIZE) - 1); + + // Only pop the stack if the bottom flag is the one we care about + // Necessary for escape prefixes that overlap regular prefixes + if (BottomOfStack != Flag) { + return; + } + + uint32_t TopOfStack = ((TmpFlags & FLAG_OPADDR_MASK) >> (FLAG_OPADDR_OFF + FLAG_OPADDR_FLAG_SIZE)) & ((1 << FLAG_OPADDR_FLAG_SIZE) - 1); + + TmpFlags &= ~(FLAG_OPADDR_MASK); + TmpFlags |= (TopOfStack << FLAG_OPADDR_OFF); + + *Flags = TmpFlags; +} + } union DecodedOperand {