commit f1f12824347f9714238f973e009945e1b0628b42
parent e378f44562245e675730580d935069112efe6864
Author: Greg Stoll <gstoll@mozilla.com>
Date: Thu, 20 Nov 2025 16:30:29 +0000
Bug 1997530 - skip test that requires detouring a B instruction on Windows AArch64 r=yjuglaret,win-reviewers
TrackPopupMenu() on ARM uses a B instruction, which is PC-relative. We
don't actually need to detour this, so I think it's OK to just skip this
test on ARM64.
I originally went down the path of making us support B instructions, since
they don't set a register as a side effect. However, this is harder than
I thought as explained in the PatcherDetour.h code - since we use a two-pass
system, on the first pass we assume the remote offset is 0, and that may mean
the offset is out of the 26-bit range in the B instruction.
Since I had already done the work of splitting up the BL and B instructions
in gPCRelTests I went ahead and kept those so it will be (slightly) easier
to implement this in the future if we need to.
While doing this work, I noticed that the already-existing BUncondImmDecode
was incorrect; per the ARM64 documentation the offset to jump to is the
26 bits in the instruction multipled by 4. ( https://developer.arm.com/documentation/ddi0602/2025-03/Base-Instructions/B--Branch- )
We were only using this in Clear4BytePatch ( https://searchfox.org/firefox-main/rev/1a8c62b86277005f907151bc5389cf5c5091e76f/toolkit/xre/dllservices/mozglue/interceptor/PatcherDetour.h#373-374 ), so I'm guessing this didn't come up much.
Differential Revision: https://phabricator.services.mozilla.com/D271077
Diffstat:
4 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/toolkit/xre/dllservices/mozglue/interceptor/Arm64.cpp b/toolkit/xre/dllservices/mozglue/interceptor/Arm64.cpp
@@ -43,7 +43,7 @@ static LoadOrBranch ADRPDecode(const uintptr_t aPC, const uint32_t aInst) {
MFBT_API LoadOrBranch BUncondImmDecode(const uintptr_t aPC,
const uint32_t aInst) {
- int32_t offset = SignExtend<int32_t>(aInst & 0x03FFFFFFU, 26);
+ intptr_t offset = SignExtend<intptr_t>((aInst & 0x03FFFFFFU) << 2, 28);
return LoadOrBranch(aPC + offset);
}
@@ -55,10 +55,11 @@ static const PCRelativeLoadTest gPCRelTests[] = {
{0x9F000000, 0x90000000, &ADRPDecode}, // ADRP
{0xFF000000, 0x58000000, nullptr}, // LDR (literal) 64-bit GPR
{0x3B000000, 0x18000000, nullptr}, // LDR (literal) (remaining forms)
- {0x7C000000, 0x14000000, nullptr}, // B (unconditional immediate)
- {0xFE000000, 0x54000000, nullptr}, // B.Cond
- {0x7E000000, 0x34000000, nullptr}, // Compare and branch (imm)
- {0x7E000000, 0x36000000, nullptr}, // Test and branch (imm)
+ {0xFC000000, 0x94000000, nullptr}, // BL (unconditional immediate)
+ {0xFC000000, 0x14000000, &BUncondImmDecode}, // B (unconditional immediate)
+ {0xFE000000, 0x54000000, nullptr}, // B.Cond
+ {0x7E000000, 0x34000000, nullptr}, // Compare and branch (imm)
+ {0x7E000000, 0x36000000, nullptr}, // Test and branch (imm)
};
/**
diff --git a/toolkit/xre/dllservices/mozglue/interceptor/PatcherDetour.h b/toolkit/xre/dllservices/mozglue/interceptor/PatcherDetour.h
@@ -937,6 +937,8 @@ class WindowsDllDetourPatcher final
}
#endif // defined(_M_X64)
+ // Note that *aOutTramp is not set until CreateTrampoline has completed
+ // successfully, so callers can use that to check for success.
void CreateTrampoline(ReadOnlyTargetFunction<MMPolicyT>& origBytes,
TrampPoolT* aTrampPool, Trampoline<MMPolicyT>& aTramp,
intptr_t aDest, void** aOutTramp) {
@@ -964,8 +966,7 @@ class WindowsDllDetourPatcher final
auto clearInstanceOnFailure = MakeScopeExit([this, aOutTramp, &tramp,
&origBytes]() -> void {
- // *aOutTramp is not set until CreateTrampoline has completed
- // successfully, so we can use that to check for success.
+ // If *aOutTramp was set this method ran successfully.
if (*aOutTramp) {
return;
}
@@ -1699,6 +1700,23 @@ class WindowsDllDetourPatcher final
break;
}
+ if (pcRelInfo.inspect().mType == arm64::LoadOrBranch::Type::Branch) {
+ // We need to branch to the original target of this instruction.
+ // This is a problem because on the first pass
+ // GetCurrentRemoteAddress() is 0, so the offset may be too large for
+ // BuildUnconditionalBranchImm. We don't need this for now on ARM64.
+ // so just behave like we don't have a decoder.
+
+ // origBytes is now pointing one instruction past the one that we
+ // need the trampoline to jump back to.
+ if (!origBytes.BackUpOneInstruction()) {
+ return;
+ }
+
+ break;
+ }
+
+ MOZ_ASSERT(pcRelInfo.inspect().mType == arm64::LoadOrBranch::Type::Load);
// We need to load an absolute address into a particular register
tramp.WriteLoadLiteral(pcRelInfo.inspect().mAbsAddress,
pcRelInfo.inspect().mDestReg);
diff --git a/toolkit/xre/dllservices/tests/TestArm64Disassembler.cpp b/toolkit/xre/dllservices/tests/TestArm64Disassembler.cpp
@@ -139,6 +139,33 @@ bool TestCheckForPCRelBr() {
return true;
}
+bool TestCheckForAbsB() {
+ // b 0x400
+ Result<LoadOrBranch, PCRelCheckError> result =
+ CheckForPCRel(kExamplePCValue, 0x14000100);
+ if (result.isErr()) {
+ auto error = result.unwrapErr();
+ TEST_FAILED("Failed to decode b, got PCRelCheckError %d.\n", error);
+ }
+
+ auto loadOrBranch = result.unwrap();
+ if (loadOrBranch.mType != LoadOrBranch::Type::Branch) {
+ TEST_FAILED("Computed an incorrect LoadOrBranch::Type for b, got %d.\n",
+ loadOrBranch.mType);
+ }
+ if (loadOrBranch.mAbsAddress != (kExamplePCValue + 0x400)) {
+ TEST_FAILED("Computed a wrong absolute address for b, got address %p.\n",
+ loadOrBranch.mAbsAddress);
+ }
+ if (loadOrBranch.mCond != IntegerConditionCode::AL) {
+ TEST_FAILED("Computed a wrong condition code for b, got %d.\n",
+ static_cast<int32_t>(loadOrBranch.mCond));
+ }
+
+ TEST_PASS("Properly decoded b.\n");
+ return true;
+}
+
int wmain(int argc, wchar_t* argv[]) {
if (!TestCheckForPCRelAdrp() || !TestCheckForPCRelAdr() ||
!TestCheckForPCRelBlr() || !TestCheckForPCRelBr()) {
diff --git a/toolkit/xre/dllservices/tests/TestDllInterceptor.cpp b/toolkit/xre/dllservices/tests/TestDllInterceptor.cpp
@@ -1547,7 +1547,10 @@ extern "C" int wmain(int argc, wchar_t* argv[]) {
TEST_HOOK("user32.dll", GetKeyState, Ignore, 0) && // see Bug 1316415
#endif
TEST_HOOK("user32.dll", GetWindowInfo, Equals, FALSE) &&
+#if defined(_M_IX86) || defined(_M_X64)
+ // Bug 1997530: TrackPopupMenu uses a PC-relative branch on ARM64.
TEST_HOOK("user32.dll", TrackPopupMenu, Equals, FALSE) &&
+#endif
TEST_DETOUR("user32.dll", CreateWindowExW, Equals, nullptr) &&
TEST_HOOK("user32.dll", InSendMessageEx, Equals, ISMEX_NOSEND) &&
TEST_HOOK("user32.dll", SendMessageTimeoutW, Equals, 0) &&
