From b066079628f2faaf6ebccef1797bd47ae13d3458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Franch?= Date: Sat, 3 Oct 2026 08:20:31 +0200 Subject: [PATCH] fix(relinker): zero dst[127:64] in EXTRQ and INSERTQ lowerings The APM leaves bits 127:64 of the destination undefined after EXTRQ and INSERTQ, but hardware zeroes them, and the register-form INSERTQ stub already does the same (#185). The other paths kept the old high qword, so a following register-form INSERTQ that reads its length and index from that qword computed the wrong low qword too. Every stub path that left the high qword now ends with movq dst, dst. The in-place PSRLQ, PMOVZX and PBLENDW forms leave it non-zero and have no room for the movq, so those sites now take a stub. The two in-place forms that emitted nothing now emit movq dst, dst. Co-Authored-By: Claude --- .../codegen/src/x86/Sse4aLowering.cpp | 17 +++---- .../codegen/tests/Amd64OnlyConverterTests.cpp | 45 +++++++++++++------ 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/core/relinker/codegen/src/x86/Sse4aLowering.cpp b/core/relinker/codegen/src/x86/Sse4aLowering.cpp index 1f2a7da24..347231750 100644 --- a/core/relinker/codegen/src/x86/Sse4aLowering.cpp +++ b/core/relinker/codegen/src/x86/Sse4aLowering.cpp @@ -29,6 +29,10 @@ void _nopFill(std::vector& out, std::size_t count) { } } +void _zeroUpper(StubBodyBuilder& body, const std::uint8_t dst) { + body.Sse(kPrefixScalar, {0x0F, 0x7E}, dst, dst); +} + std::uint64_t _fieldMask(const std::uint8_t length) { return length >= kFieldBits ? ~std::uint64_t{0} : ((std::uint64_t{1} << length) - 1); } @@ -101,6 +105,7 @@ void _emitOutOfLine(StubBodyBuilder& body, const Sse4aOperands& operands) { body.Sse(kPrefixPacked, {0x0F, 0xD3}, dst, scratch[0]); body.Sse(kPrefixPacked, {0x0F, 0xF3}, dst, scratch[1]); body.Sse(kPrefixPacked, {0x0F, 0xD3}, dst, scratch[1]); + _zeroUpper(body, dst); body.Restore(scratch[1]); body.Restore(scratch[0]); return; @@ -124,6 +129,7 @@ void _emitOutOfLine(StubBodyBuilder& body, const Sse4aOperands& operands) { body.ShiftImm(kShiftLeft, dst, static_cast(kFieldBits - length)); body.ShiftImm(kShiftRight, dst, static_cast(kFieldBits - length)); } + _zeroUpper(body, dst); } } else if (byteAligned) { if (src != dst) @@ -150,6 +156,7 @@ void _emitOutOfLine(StubBodyBuilder& body, const Sse4aOperands& operands) { hole[byte] = static_cast(holeMask >> (byte * 8)); body.RipOperand({0x0F, 0xDB}, scratch, hole); body.Sse(kPrefixPacked, {0x0F, 0xEF}, dst, scratch); + _zeroUpper(body, dst); body.Restore(scratch); } } @@ -166,21 +173,15 @@ std::optional> Sse4aLowering::LowerInPlace(const Sse4a std::vector sequence; if (operands.Insertq) { if (dst == src && index == 0) { + EmitSse(sequence, kPrefixScalar, {0x0F, 0x7E}, dst, dst); } else if (length == kFieldBits && index == 0) { EmitSse(sequence, kPrefixScalar, {0x0F, 0x7E}, dst, src); - } else if (index == 0 && length % 16 == 0) { - EmitSse(sequence, kPrefixPacked, {0x0F, 0x3A, 0x0E}, dst, src); - sequence.push_back(static_cast((1u << (length / 16)) - 1)); } else { return std::nullopt; } } else { if (index == 0 && length == kFieldBits) { - } else if (index + length == kFieldBits) { - EmitShiftImm(sequence, kShiftRight, dst, index); - } else if (index == 0 && (length == 8 || length == 16 || length == 32)) { - const std::uint8_t opcode = length == 8 ? 0x32 : (length == 16 ? 0x34 : 0x35); - EmitSse(sequence, kPrefixPacked, {0x0F, 0x38, opcode}, dst, dst); + EmitSse(sequence, kPrefixScalar, {0x0F, 0x7E}, dst, dst); } else { return std::nullopt; } diff --git a/core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp b/core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp index 82bad675a..65ae3f586 100644 --- a/core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp +++ b/core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp @@ -240,8 +240,8 @@ void matcherSubstitutions() { require(!match({0x0F, 0x38, 0xC9, 0xCA}), "SHA-1 instruction was matched"); const auto stub = match(kInsertqHighSite); require(stub && stub->Lowering == Codegen::Amd64OnlyLowering::Trampoline && stub->StubBody == kInsertqHighBody && stub->ReturnBranchOffset == 15 && stub->InstructionName == "INSERTQ", "INSERTQ was not lowered through a stub"); - const auto shiftInPlace = match({0x66, 0x0F, 0x78, 0xC3, 0x18, 0x28}); - require(shiftInPlace && shiftInPlace->Lowering == Codegen::Amd64OnlyLowering::InPlace && shiftInPlace->ReplacementBytes == Bytes{0x66, 0x0F, 0x73, 0xD3, 0x28, 0x90}, "Top-aligned EXTRQ was not lowered in place"); + const auto topAligned = match({0x66, 0x0F, 0x78, 0xC3, 0x18, 0x28}); + require(topAligned && topAligned->Lowering == Codegen::Amd64OnlyLowering::Trampoline && topAligned->InstructionName == "EXTRQ", "Top-aligned EXTRQ was not lowered through a stub"); } void goldenBodies() { @@ -264,11 +264,12 @@ void goldenBodies() { require(sequence.has_value() && *sequence == expected, "In-place lowering differs from the golden encoding"); }; inPlace({0xF2, 0x0F, 0x78, 0xC8, 0x00, 0x00}, {0xF3, 0x0F, 0x7E, 0xC8, 0x66, 0x90}); - inPlace({0x66, 0x0F, 0x78, 0xC3, 0x18, 0x28}, {0x66, 0x0F, 0x73, 0xD3, 0x28, 0x90}); - inPlace({0x66, 0x0F, 0x78, 0xC3, 0x08, 0x00}, {0x66, 0x0F, 0x38, 0x32, 0xDB, 0x90}); - inPlace({0xF2, 0x0F, 0x78, 0xDB, 0x08, 0x00}, {0x66, 0x0F, 0x1F, 0x44, 0x00, 0x00}); - inPlace({0xF2, 0x0F, 0x78, 0xC8, 0x20, 0x00}, {0x66, 0x0F, 0x3A, 0x0E, 0xC8, 0x03}); - inPlace({0xF2, 0x45, 0x0F, 0x78, 0xC8, 0x10, 0x00}, {0x66, 0x45, 0x0F, 0x3A, 0x0E, 0xC8, 0x01}); + inPlace({0xF2, 0x0F, 0x78, 0xDB, 0x08, 0x00}, {0xF3, 0x0F, 0x7E, 0xDB, 0x66, 0x90}); + inPlace({0x66, 0x0F, 0x78, 0xC3, 0x00, 0x00}, {0xF3, 0x0F, 0x7E, 0xDB, 0x66, 0x90}); + for (const auto& site : {Bytes{0x66, 0x0F, 0x78, 0xC3, 0x18, 0x28}, Bytes{0x66, 0x0F, 0x78, 0xC3, 0x08, 0x00}, Bytes{0xF2, 0x0F, 0x78, 0xC8, 0x20, 0x00}, Bytes{0xF2, 0x45, 0x0F, 0x78, 0xC8, 0x10, 0x00}}) { + const auto operands = Codegen::DecodeSse4a(site.data(), site.size()); + require(!lowering.LowerInPlace(operands, site.size()).has_value(), "SSE4a site was lowered in place without zeroing dst[127:64]"); + } const Bytes clzeroSite = {0x0F, 0x01, 0xFC}; const auto clzero = Codegen::ClzeroLowering{}.LowerOutOfLine(Codegen::DecodeClzero(clzeroSite.data(), clzeroSite.size())); require(clzero.Bytes == kClzeroBody && clzero.ReturnBranchOffset == 69, "CLZERO stub differs from the golden encoding"); @@ -592,12 +593,14 @@ std::array sha256Reference(const std::uint8_t opcode, const st std::array runRegisterFormStub(const Bytes& site, const std::uint64_t (&destination)[2], const std::uint64_t (&source)[2]) { const auto matcher = Codegen::MakeAmd64OnlyInstructionMatcher(); const auto match = matcher->Match(site.data(), site.size()); - require(match && match->Lowering == Codegen::Amd64OnlyLowering::Trampoline, "Register form stub was not produced"); - auto body = match->StubBody; + require(match && match->Lowering != Codegen::Amd64OnlyLowering::Unsupported, "Register form stub was not produced"); + auto body = match->Lowering == Codegen::Amd64OnlyLowering::InPlace ? match->ReplacementBytes : match->StubBody; const auto ret = body.size(); body.push_back(0xC3); - const auto displacement = static_cast(ret - (match->ReturnBranchOffset + 5)); - std::memcpy(body.data() + match->ReturnBranchOffset + 1, &displacement, sizeof(displacement)); + if (match->Lowering == Codegen::Amd64OnlyLowering::Trampoline) { + const auto displacement = static_cast(ret - (match->ReturnBranchOffset + 5)); + std::memcpy(body.data() + match->ReturnBranchOffset + 1, &displacement, sizeof(displacement)); + } void* code = mmap(nullptr, 4096, PROT_READ | PROT_WRITE | PROT_EXEC, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); require(code != MAP_FAILED, "cannot map executable memory for the stub"); std::memcpy(code, body.data(), body.size()); @@ -632,14 +635,28 @@ void registerFormExecution() { const std::uint64_t destination = 0x0f1e2d3c4b5a6978ull; for (const auto [length, index] : {std::pair{8u, 4u}, {0u, 0u}, {40u, 20u}, {63u, 1u}, {1u, 63u}, {16u, 48u}, {1u, 0u}, {32u, 32u}}) { const auto control = static_cast(length) | (static_cast(index) << 8) | 0xffffc000ull; - require(runRegisterFormStub(extrqDistinct, {value, 0x1122334455667788ull}, {control, 0})[0] == extrqReference(value, control), "EXTRQ register form stub computed the wrong field"); - require(runRegisterFormStub(extrqSame, {control, 0}, {control, 0})[0] == extrqReference(control, control), "EXTRQ register form stub with equal operands computed the wrong field"); + require(runRegisterFormStub(extrqDistinct, {value, 0x1122334455667788ull}, {control, 0}) == std::array{extrqReference(value, control), 0}, "EXTRQ register form stub computed the wrong field"); + require(runRegisterFormStub(extrqSame, {control, 0x1122334455667788ull}, {control, 0x1122334455667788ull}) == std::array{extrqReference(control, control), 0}, "EXTRQ register form stub with equal operands computed the wrong field"); const auto insertqControl = control | 0xC0ull; require(runRegisterFormStub(insertqDistinct, {destination, 0x1122334455667788ull}, {value, insertqControl})[0] == insertqReference(destination, value, insertqControl), "INSERTQ register form stub computed the wrong field"); require(runRegisterFormStub(insertqSame, {value, insertqControl}, {value, insertqControl})[0] == insertqReference(value, value, insertqControl), "INSERTQ register form stub with equal operands computed the wrong field"); } } +void immediateFormExecution() { + const std::uint64_t value = 0x9e3779b97f4a7c15ull; + const std::uint64_t destination = 0x0f1e2d3c4b5a6978ull; + const std::uint64_t high = 0x1122334455667788ull; + for (const auto [length, index] : {std::pair{0u, 0u}, {24u, 40u}, {8u, 0u}, {16u, 0u}, {32u, 0u}, {8u, 40u}, {5u, 3u}, {63u, 1u}, {1u, 63u}}) { + const auto control = static_cast(length) | (static_cast(index) << 8); + const auto length8 = static_cast(length); + const auto index8 = static_cast(index); + require(runRegisterFormStub({0x66, 0x0F, 0x78, 0xC2, length8, index8}, {value, high}, {0, 0}) == std::array{extrqReference(value, control), 0}, "EXTRQ immediate form computed the wrong result"); + require(runRegisterFormStub({0xF2, 0x0F, 0x78, 0xD5, length8, index8}, {destination, high}, {value, high}) == std::array{insertqReference(destination, value, control), 0}, "INSERTQ immediate form computed the wrong result"); + require(runRegisterFormStub({0xF2, 0x0F, 0x78, 0xD2, length8, index8}, {value, high}, {value, high}) == std::array{insertqReference(value, value, control), 0}, "INSERTQ immediate form with equal operands computed the wrong result"); + } +} + void sha256Execution() { const std::uint64_t state[2] = {0x6a09e667bb67ae85ull, 0x3c6ef372a54ff53aull}; const std::uint64_t words[2] = {0x510e527f9b05688cull, 0x1f83d9ab5be0cd19ull}; @@ -767,6 +784,7 @@ void clzeroExecution() { void registerFormExecution() {} void sha256Execution() {} void clzeroExecution() {} +void immediateFormExecution() {} #endif void scannerZeroTail() { @@ -786,6 +804,7 @@ int main() { matcherSubstitutions(); goldenBodies(); registerFormExecution(); + immediateFormExecution(); sha256Execution(); clzeroExecution(); converterSegment();