Skip to content

[comgr][hotswap] Bound finite external PC call targets #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
harsh-amd wants to merge 1 commit into
base: users/harsh/hotswap-split-ref-index-reconciled
Choose a base branch
from
Draft

[comgr][hotswap] Bound finite external PC call targets #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 80 additions & 22 deletions amd/comgr/src/comgr-hotswap-b0a0.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2005,6 +2005,11 @@ static bool compareKnownCallEntries(const KnownCallEntry &LHS,
std::tie(RHS.Entry, RHS.CallIndex);
}

struct ExternalCallContinuation {
size_t InstIndex = 0;
uint64_t Continuation = 0;
};

struct FallthroughEntryInfo {
bool Proven = false;
uint64_t ChainBegin = 0;
Expand All @@ -2017,6 +2022,7 @@ struct ControlFlowScanIndex {
SmallVector<KnownCallEntry, 8> CallsByTarget;
SmallVector<KnownCallEntry, 16> CallEntries;
DenseMap<size_t, MCRegister> CallReturnRegistersBySource;
SmallVector<ExternalCallContinuation, 4> ExternalCallContinuations;
SmallVector<size_t, 16> SetPcIndices;
SmallVector<size_t, 16> BranchOrCallIndices;
SmallVector<DirectTargetSource, 16> DirectTargetsByTarget;
Expand Down Expand Up @@ -2125,19 +2131,44 @@ buildControlFlowScanIndex(ArrayRef<InternalDecodedInst> Decoded,
std::optional<MCRegister> ReturnRegister = getCallReturnRegister(DI, LS);
if (ReturnRegister) {
std::optional<uint64_t> Target;
bool HasFiniteExternalTarget = false;
if (Materialized) {
uint64_t AbsoluteTarget = Materialized->Target;
if (AbsoluteTarget >= TextAddr && AbsoluteTarget < TextEnd)
Target = AbsoluteTarget - TextAddr;
else
HasFiniteExternalTarget = true;
} else if (DI.Inst.getOpcode() == LS.SSwapPcI64Opcode &&
DI.Inst.getNumOperands() != 0 &&
DI.Inst.getOperand(DI.Inst.getNumOperands() - 1).isImm()) {
uint64_t AbsoluteTarget = static_cast<uint64_t>(
DI.Inst.getOperand(DI.Inst.getNumOperands() - 1).getImm());
if (AbsoluteTarget >= TextAddr && AbsoluteTarget < TextEnd)
Target = AbsoluteTarget - TextAddr;
else
HasFiniteExternalTarget = true;
} else if (hasPcRelativeOperand(DI, LS)) {
std::optional<uint64_t> RelativeTarget =
evaluateDirectControlFlowTarget(DI, LS);
if (RelativeTarget) {
uint64_t TextSize = TextEnd - TextAddr;
if (*RelativeTarget < TextSize)
Target = *RelativeTarget;
else
HasFiniteExternalTarget = true;
}
} else {
Target = getDirectTextTarget(DI, LS, TextAddr, TextEnd);
}
if (Target) {
if (Target || HasFiniteExternalTarget) {
std::optional<uint64_t> Continuation = checkedAddUint64(
DI.Offset, DI.Size, "known call continuation address");
if (!Continuation)
return std::nullopt;
Index.Calls.push_back({I, *Target, *Continuation, *ReturnRegister});
if (Target)
Index.Calls.push_back({I, *Target, *Continuation, *ReturnRegister});
if (HasFiniteExternalTarget)
Index.ExternalCallContinuations.push_back({I, *Continuation});
}
}

Expand Down Expand Up @@ -2231,6 +2262,19 @@ static bool hasUnprovenFallthroughEntry(ArrayRef<InternalDecodedInst> Decoded,
return true;
}

for (const ExternalCallContinuation &Call :
Index.ExternalCallContinuations) {
uint64_t Source = Decoded[Call.InstIndex].Offset;
if (Call.Continuation >= ChainBegin &&
Call.Continuation < FunctionBegin) {
log() << "hotswap: s_set_pc_i64 at 0x" << utohexstr(ReturnOffset)
<< " is not a bounded return: external call at 0x"
<< utohexstr(Source) << " returns into the fallthrough chain at 0x"
<< utohexstr(Call.Continuation) << "\n";
return true;
}
}

SmallVector<DirectTargetSource, 16>::const_iterator FirstTarget =
llvm::lower_bound(Index.DirectTargetsByTarget, ChainBegin,
[](const DirectTargetSource &Source, uint64_t Target) {
Expand Down Expand Up @@ -2571,19 +2615,23 @@ std::optional<DirectControlFlowInfo> collectDirectBranchTargets(
continue;
std::optional<MCRegister> ReturnRegister =
getCallReturnRegister(Decoded[I], LS);
if (!ReturnRegister ||
llvm::any_of(ReusableCalls[I], [TextAddr, TextEnd](uint64_t Target) {
return Target < TextAddr || Target >= *TextEnd;
}))
if (!ReturnRegister)
continue;
std::optional<uint64_t> Continuation = checkedAddUint64(
Decoded[I].Offset, Decoded[I].Size,
"known reusable call continuation address");
if (!Continuation)
return std::nullopt;
bool HasExternalTarget = false;
for (uint64_t Target : ReusableCalls[I])
Index->Calls.push_back(
{I, Target - TextAddr, *Continuation, *ReturnRegister});
if (Target >= TextAddr && Target < *TextEnd) {
Index->Calls.push_back(
{I, Target - TextAddr, *Continuation, *ReturnRegister});
} else {
HasExternalTarget = true;
}
if (HasExternalTarget)
Index->ExternalCallContinuations.push_back({I, *Continuation});
}
indexKnownCalls(*Index);

Expand Down Expand Up @@ -2617,6 +2665,12 @@ std::optional<DirectControlFlowInfo> collectDirectBranchTargets(
}

DirectControlFlowInfo Info;
for (const BoundedSetPcReturn &Return : *BoundedReturns)
for (uint64_t Target : Return.Targets)
Info.Targets.insert(Target);
for (const ExternalCallContinuation &Call :
Index->ExternalCallContinuations)
Info.Targets.insert(Call.Continuation);
for (size_t InstIndex : Index->BranchOrCallIndices) {
const InternalDecodedInst &DI = Decoded[InstIndex];
// Existing indirect branches are handled by
Expand Down Expand Up @@ -2652,18 +2706,9 @@ std::optional<DirectControlFlowInfo> collectDirectBranchTargets(
Target = Materialized->second.Target;
}
if (!ReusableCalls[InstIndex].empty()) {
if (llvm::any_of(ReusableCalls[InstIndex],
[TextAddr, TextEnd](uint64_t ReusableTarget) {
return ReusableTarget < TextAddr ||
ReusableTarget >= *TextEnd;
})) {
log() << "hotswap: unresolved call target at 0x"
<< utohexstr(DI.Offset) << " (reusable target outside .text)\n";
Info.HasUnresolvedTargets = true;
continue;
}
for (uint64_t ReusableTarget : ReusableCalls[InstIndex])
Info.Targets.insert(ReusableTarget - TextAddr);
if (ReusableTarget >= TextAddr && ReusableTarget < *TextEnd)
Info.Targets.insert(ReusableTarget - TextAddr);
Info.BoundedIndirectTransfers.insert(DI.Offset);
if (LocallyProvenMaterializedCalls.test(InstIndex)) {
log() << "hotswap: resolved PC-materialized call at 0x"
Expand Down Expand Up @@ -2691,8 +2736,13 @@ std::optional<DirectControlFlowInfo> collectDirectBranchTargets(
log() << "hotswap: resolved PC-materialized call at 0x"
<< utohexstr(DI.Offset) << " to .text+0x"
<< utohexstr(RelativeTarget) << "\n";
if (DI.Inst.getOperand(DI.Inst.getNumOperands() - 1).isReg())
Info.BoundedIndirectTransfers.insert(DI.Offset);
}
// A proven finite register target outside this object's .text cannot
// enter a local instruction or synthetic source range. Keep that
// control-flow proof separate from whether the target contributes a
// local offset to the mutation-protection set.
if (DI.Inst.getOperand(DI.Inst.getNumOperands() - 1).isReg()) {
Info.BoundedIndirectTransfers.insert(DI.Offset);
}
continue;
}
Expand All @@ -2705,7 +2755,15 @@ std::optional<DirectControlFlowInfo> collectDirectBranchTargets(
<< "; adjacent far trampolines will not be coalesced\n";
return std::nullopt;
}
Info.Targets.insert(*Target);
if (*Target < TextSize) {
Info.Targets.insert(*Target);
} else if (LS.MIA->isCall(DI.Inst)) {
std::optional<uint64_t> Continuation = checkedAddUint64(
DI.Offset, DI.Size, "finite external direct call continuation");
if (!Continuation)
return std::nullopt;
Info.Targets.insert(*Continuation);
}
}
for (const BoundedSetPcReturn &Return : *BoundedReturns)
Info.BoundedIndirectTransfers.insert(Decoded[Return.InstIndex].Offset);
Expand Down
18 changes: 11 additions & 7 deletions amd/comgr/test-lit/hotswap-reusable-pc-call-targets.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,18 @@
// RUN: %t.outside.s -o %t.outside.elf
// RUN: env AMD_COMGR_EMIT_VERBOSE_LOGS=1 hotswap-rewrite %t.outside.elf \
// RUN: amdgcn-amd-amdhsa--gfx1250 amdgcn-amd-amdhsa--gfx1250 \
// RUN: --expect-status ERROR 2>&1 \
// RUN: --output %t.outside.out.elf 2>&1 \
// RUN: | %FileCheck --check-prefix=OUTSIDE %s
// OUTSIDE: hotswap: unresolved call target
// OUTSIDE-SAME: (reusable target outside .text)
// OUTSIDE: hotswap: unresolved control-flow target disables NOP-sled emission,
// OUTSIDE-SAME: trampoline coalescing, source relocation, and .text gateways
// OUTSIDE: hotswap: error: no safe short-branch gateway for far site
// OUTSIDE: RESULT: ERROR
// OUTSIDE: hotswap: resolved reusable PC-materialized call
// OUTSIDE-SAME: to 3 target(s)
// OUTSIDE-NOT: hotswap: unresolved call target
// OUTSIDE: hotswap: planned 1 shared far-dispatch gateway group(s) for 8 source site(s)
// OUTSIDE: RESULT: SUCCESS
// RUN: %llvm-objdump -d %t.outside.out.elf \
// RUN: | %FileCheck --check-prefix=OUTSIDE-DISASM %s
// OUTSIDE-DISASM-LABEL: <reusable_pc_targets>:
// OUTSIDE-DISASM: s_swap_pc_i64
// OUTSIDE-DISASM-NEXT: s_branch

// RUN: %llvm-objdump -d %t.out.elf | %FileCheck --check-prefix=DISASM \
// RUN: --implicit-check-not=s_add_pc_i64 %s
Expand Down
110 changes: 108 additions & 2 deletions amd/comgr/test-unit/HotswapMCTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,82 @@ TEST(CollectDirectBranchTargets, ResolvesProductionPcMaterializedCall) {
EXPECT_FALSE(Info->HasUnresolvedTargets);
}

TEST(CollectDirectBranchTargets, BoundsFiniteExternalPcMaterializedCall) {
LLVMState S = initLLVM(makeGfx1250Ident());
ASSERT_TRUE(S.Valid);
llvm::SmallVector<uint8_t> Bytes =
assembleInstructions("s_get_pc_i64 s[0:1]\n"
"s_add_nc_u64 s[0:1], s[0:1], 0x100\n"
"s_swap_pc_i64 s[30:31], s[0:1]",
S);
ASSERT_FALSE(Bytes.empty());

std::vector<InternalDecodedInst> Decoded;
ASSERT_TRUE(decodeTextSection(Bytes.data(), Bytes.size(), S, Decoded));
ASSERT_EQ(Decoded.size(), 3u);

// The exact target is outside this deliberately short .text range. The
// external callee may return through the link pair, so the local
// continuation remains a protected entry even though the external target
// contributes no local offset.
std::optional<DirectControlFlowInfo> Info =
collectDirectBranchTargets(Decoded, S, /*TextAddr=*/0,
/*TextSize=*/0x20,
/*DeclaredEntries=*/{});
ASSERT_TRUE(Info);
ASSERT_EQ(Info->Targets.size(), 1u);
EXPECT_TRUE(Info->Targets.contains(Decoded.back().Offset +
Decoded.back().Size));
EXPECT_TRUE(
Info->BoundedIndirectTransfers.contains(Decoded.back().Offset));
EXPECT_FALSE(Info->HasUnresolvedTargets);
}

TEST(CollectDirectBranchTargets,
RejectsExternalCallContinuationIntoReturnFunction) {
LLVMState S = initLLVM(makeGfx1250Ident());
ASSERT_TRUE(S.Valid);
llvm::SmallVector<uint8_t> Bytes =
assembleInstructions("s_endpgm\n"
"s_get_pc_i64 s[2:3]\n"
"s_add_co_i32 s4, 0x1000, 4\n"
"s_add_co_u32 s2, s2, s4\n"
"s_add_co_ci_u32 s3, s3, 0\n"
"s_swap_pc_i64 s[30:31], s[2:3]\n"
"s_nop 0\n"
"s_nop 0\n"
"s_set_pc_i64 s[30:31]\n"
"s_endpgm\n"
"s_get_pc_i64 s[0:1]\n"
"s_add_nc_u64 s[0:1], s[0:1], -16\n"
"s_swap_pc_i64 s[30:31], s[0:1]",
S);
ASSERT_FALSE(Bytes.empty());

std::vector<InternalDecodedInst> Decoded;
ASSERT_TRUE(decodeTextSection(Bytes.data(), Bytes.size(), S, Decoded));
ASSERT_EQ(Decoded.size(), 13u);
llvm::SmallVector<uint64_t, 2> DeclaredEntries{
Decoded[7].Offset, Decoded[10].Offset};
llvm::SmallVector<ElfView::FunctionTextRange, 3> FunctionRanges{
{Decoded[1].Offset, Decoded[7].Offset},
{Decoded[7].Offset, Decoded[9].Offset},
{Decoded[10].Offset, Decoded.back().Offset + Decoded.back().Size}};

// The first call has one finite target outside this .text, and returns to
// the padding that falls through into the local helper. The later local
// call alone would appear to justify the helper's s_set_pc_i64, but the
// external continuation is a second link-register provenance and must keep
// that return unbounded.
std::optional<DirectControlFlowInfo> Info = collectDirectBranchTargets(
Decoded, S, /*TextAddr=*/0, /*TextSize=*/Bytes.size(), DeclaredEntries,
FunctionRanges, /*ExternalEntries=*/{}, Bytes);
ASSERT_TRUE(Info);
EXPECT_TRUE(
Info->Targets.contains(Decoded[5].Offset + Decoded[5].Size));
EXPECT_TRUE(Info->HasUnresolvedTargets);
}

TEST(CollectDirectBranchTargets, RejectsClobberedPcMaterializedCall) {
LLVMState S = initLLVM(makeGfx1250Ident());
ASSERT_TRUE(S.Valid);
Expand Down Expand Up @@ -1370,9 +1446,11 @@ TEST(CollectDirectBranchTargets, BoundsCanonicalSetPcReturn) {
Decoded, S, /*TextAddr=*/0, /*TextSize=*/0x1000, DeclaredEntries,
FunctionRanges);
ASSERT_TRUE(Info);
ASSERT_EQ(Info->Targets.size(), 2u);
ASSERT_EQ(Info->Targets.size(), 3u);
EXPECT_TRUE(Info->Targets.contains(0));
EXPECT_TRUE(Info->Targets.contains(Decoded[1].Offset));
EXPECT_TRUE(Info->Targets.contains(Decoded.back().Offset +
Decoded.back().Size));
EXPECT_FALSE(Info->HasUnresolvedTargets);
}

Expand Down Expand Up @@ -1656,7 +1734,9 @@ TEST(CollectDirectBranchTargets, HandlesImmediateAbsoluteTargetCall) {
collectDirectBranchTargets(Decoded, S, /*TextAddr=*/0x220,
/*TextSize=*/0x40, /*DeclaredEntries=*/{});
ASSERT_TRUE(OutsideInfo);
EXPECT_TRUE(OutsideInfo->Targets.empty());
ASSERT_EQ(OutsideInfo->Targets.size(), 1u);
EXPECT_TRUE(OutsideInfo->Targets.contains(Decoded[0].Offset +
Decoded[0].Size));
EXPECT_FALSE(OutsideInfo->HasUnresolvedTargets);

std::optional<DirectControlFlowInfo> OverflowInfo =
Expand Down Expand Up @@ -1689,6 +1769,32 @@ TEST(CollectDirectBranchTargets, CollectsPcRelativeCall) {
EXPECT_FALSE(Info->HasUnresolvedTargets);
}

TEST(CollectDirectBranchTargets, ProtectsExternalPcRelativeCallContinuation) {
LLVMState S = initLLVM(makeGfx1250Ident());
ASSERT_TRUE(S.Valid);
llvm::SmallVector<uint8_t> Bytes =
assembleSingleInst("s_call_i64 s[30:31], 2", S);
ASSERT_FALSE(Bytes.empty());

std::vector<InternalDecodedInst> Decoded;
ASSERT_TRUE(decodeTextSection(Bytes.data(), Bytes.size(), S, Decoded));
ASSERT_EQ(Decoded.size(), 1u);

// The encoded target is beyond this deliberately short .text, while the
// instruction after the call remains inside .text. The target must not
// become a local protection offset, while the return continuation remains
// protected.
std::optional<DirectControlFlowInfo> Info =
collectDirectBranchTargets(Decoded, S, /*TextAddr=*/0,
/*TextSize=*/2 * Decoded[0].Size,
/*DeclaredEntries=*/{});
ASSERT_TRUE(Info);
ASSERT_EQ(Info->Targets.size(), 1u);
EXPECT_TRUE(
Info->Targets.contains(Decoded[0].Offset + Decoded[0].Size));
EXPECT_FALSE(Info->HasUnresolvedTargets);
}

TEST(SafeSgprScratchBlock, RejectsRegisterBeyondAddressableLimit) {
LLVMState S = initLLVM(makeGfx1250Ident());
ASSERT_TRUE(S.Valid);
Expand Down
Loading