Skip to content

Commit 00856fc

Browse files
harsh-amdharsh-amd
authored
AMDGPU: reduce hotswap helper duplication (#3138)
This addresses the implementation-redundancy feedback from #3008 in a separate refactor PR. It is based directly on amd-staging and touches only hotswap implementation helper code, so it can be reviewed in parallel with #3098.
1 parent 08af1c4 commit 00856fc

4 files changed

Lines changed: 75 additions & 82 deletions

File tree

‎amd/comgr/src/comgr-hotswap-b0a0.cpp‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,22 @@ static void runScratchVerification(WritableMemoryBuffer &OutBuf,
605605
<< "scratch conflicts\n";
606606
}
607607

608+
static std::unique_ptr<WritableMemoryBuffer>
609+
copyOutputBuffer(const void *Data, size_t Size, StringRef CopyKind) {
610+
std::unique_ptr<WritableMemoryBuffer> Result =
611+
WritableMemoryBuffer::getNewUninitMemBuffer(Size);
612+
if (!Result) {
613+
log() << "hotswap: error: retargetCodeObject: "
614+
<< "getNewUninitMemBuffer(" << Size
615+
<< ") failed (out of memory) for the " << CopyKind
616+
<< " output copy.\n";
617+
return nullptr;
618+
}
619+
620+
std::memcpy(Result->getBufferStart(), Data, Size);
621+
return Result;
622+
}
623+
608624
// -- retargetCodeObject -------------------------------------------------------
609625

610626
amd_comgr_status_t retargetCodeObject(const void *ElfData, size_t ElfSize,
@@ -618,14 +634,9 @@ amd_comgr_status_t retargetCodeObject(const void *ElfData, size_t ElfSize,
618634

619635
if (!Options.RunB0A0Patches && !Options.RunEntryTrampolines) {
620636
std::unique_ptr<WritableMemoryBuffer> Result =
621-
WritableMemoryBuffer::getNewUninitMemBuffer(ElfSize);
622-
if (!Result) {
623-
log() << "hotswap: error: retargetCodeObject: "
624-
<< "getNewUninitMemBuffer(" << ElfSize
625-
<< ") failed (out of memory) for the no-op output copy.\n";
637+
copyOutputBuffer(ElfData, ElfSize, "no-op");
638+
if (!Result)
626639
return AMD_COMGR_STATUS_ERROR_OUT_OF_RESOURCES;
627-
}
628-
std::memcpy(Result->getBufferStart(), ElfData, ElfSize);
629640
Out = std::move(Result);
630641
return AMD_COMGR_STATUS_SUCCESS;
631642
}
@@ -739,14 +750,9 @@ amd_comgr_status_t retargetCodeObject(const void *ElfData, size_t ElfSize,
739750
EntryFixups))
740751
return AMD_COMGR_STATUS_ERROR;
741752
} else {
742-
Result = WritableMemoryBuffer::getNewUninitMemBuffer(ElfSize);
743-
if (!Result) {
744-
log() << "hotswap: error: retargetCodeObject: "
745-
<< "getNewUninitMemBuffer(" << ElfSize
746-
<< ") failed (out of memory) for the patched output copy.\n";
753+
Result = copyOutputBuffer(Buf.data(), ElfSize, "patched");
754+
if (!Result)
747755
return AMD_COMGR_STATUS_ERROR_OUT_OF_RESOURCES;
748-
}
749-
std::memcpy(Result->getBufferStart(), Buf.data(), ElfSize);
750756
}
751757

752758
if (!ScratchPatches.empty())

‎amd/comgr/src/comgr-hotswap-elf.cpp‎

Lines changed: 20 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
#include "llvm/ADT/StringExtras.h"
1919
#include "llvm/ADT/Twine.h"
2020
#include "llvm/BinaryFormat/MsgPackDocument.h"
21-
#include "llvm/Support/CheckedArithmetic.h"
2221

2322
#include <algorithm>
2423
#include <limits>
@@ -42,16 +41,6 @@ enum class MetadataSgprUpdateStatus {
4241
Error,
4342
};
4443

45-
static std::optional<uint64_t> checkedAdd(uint64_t LHS, uint64_t RHS,
46-
StringRef Context) {
47-
std::optional<uint64_t> Result = checkedAddUnsigned(LHS, RHS);
48-
if (Result)
49-
return Result;
50-
51-
log() << "hotswap: error: " << Context << " overflows uint64_t.\n";
52-
return std::nullopt;
53-
}
54-
5544
static std::optional<size_t>
5645
checkedAlignToSize(size_t Value, uint64_t Alignment, StringRef Context) {
5746
if (Alignment <= 1)
@@ -61,7 +50,7 @@ checkedAlignToSize(size_t Value, uint64_t Alignment, StringRef Context) {
6150
if (Remainder == 0)
6251
return Value;
6352
std::optional<uint64_t> Aligned =
64-
checkedAdd(Value64, Alignment - Remainder, Context);
53+
checkedAddUint64(Value64, Alignment - Remainder, Context);
6554
if (!Aligned)
6655
return std::nullopt;
6756
if (*Aligned > static_cast<uint64_t>(std::numeric_limits<size_t>::max())) {
@@ -84,8 +73,8 @@ static std::optional<uint64_t> checkedSectionFileOffset(const ELFT::Shdr &Sec,
8473
}
8574

8675
uint64_t Delta = VAddr - Sec.sh_addr;
87-
std::optional<uint64_t> FileOffset =
88-
checkedAdd(Sec.sh_offset, Delta, (Twine(Context) + " file offset").str());
76+
std::optional<uint64_t> FileOffset = checkedAddUint64(
77+
Sec.sh_offset, Delta, (Twine(Context) + " file offset").str());
8978
if (!FileOffset)
9079
return std::nullopt;
9180

@@ -868,7 +857,7 @@ static bool adjustSectionHeaders(uint8_t *Elf, size_t ElfSize,
868857
return true;
869858

870859
std::optional<uint64_t> TextEnd =
871-
checkedAdd(TextOffset, TextSize, "section header .text end");
860+
checkedAddUint64(TextOffset, TextSize, "section header .text end");
872861
if (!TextEnd)
873862
return false;
874863
uint64_t Shoff;
@@ -882,7 +871,7 @@ static bool adjustSectionHeaders(uint8_t *Elf, size_t ElfSize,
882871

883872
if (Shoff >= *TextEnd) {
884873
std::optional<uint64_t> NewShoff =
885-
checkedAdd(Shoff, TrampTotal, "section header table offset");
874+
checkedAddUint64(Shoff, TrampTotal, "section header table offset");
886875
if (!NewShoff)
887876
return false;
888877
uint64_t NewShoffValue = *NewShoff;
@@ -894,7 +883,7 @@ static bool adjustSectionHeaders(uint8_t *Elf, size_t ElfSize,
894883
for (uint16_t I = 0; I < Shnum; ++I) {
895884
uint64_t ShTableDelta = static_cast<uint64_t>(I) * Shentsize;
896885
std::optional<uint64_t> ShPos =
897-
checkedAdd(Shoff, ShTableDelta, "section header entry offset");
886+
checkedAddUint64(Shoff, ShTableDelta, "section header entry offset");
898887
if (!ShPos)
899888
return false;
900889
if (*ShPos > ElfSize || sizeof(Shdr) > ElfSize - *ShPos)
@@ -905,15 +894,15 @@ static bool adjustSectionHeaders(uint8_t *Elf, size_t ElfSize,
905894

906895
if (ShOffset == TextOffset) {
907896
std::optional<uint64_t> NewTextSize =
908-
checkedAdd(TextSize, TrampTotal, ".text section size");
897+
checkedAddUint64(TextSize, TrampTotal, ".text section size");
909898
if (!NewTextSize)
910899
return false;
911900
uint64_t NewTextSizeValue = *NewTextSize;
912901
std::memcpy(Sh + offsetof(Shdr, sh_size), &NewTextSizeValue,
913902
sizeof(NewTextSizeValue));
914903
} else if (ShOffset > TextOffset) {
915904
std::optional<uint64_t> NewOffset =
916-
checkedAdd(ShOffset, TrampTotal, "post-.text section offset");
905+
checkedAddUint64(ShOffset, TrampTotal, "post-.text section offset");
917906
if (!NewOffset)
918907
return false;
919908
uint64_t NewOffsetValue = *NewOffset;
@@ -925,7 +914,7 @@ static bool adjustSectionHeaders(uint8_t *Elf, size_t ElfSize,
925914
uint64_t ShAddr;
926915
std::memcpy(&ShAddr, Sh + offsetof(Shdr, sh_addr), sizeof(ShAddr));
927916
std::optional<uint64_t> NewAddr =
928-
checkedAdd(ShAddr, TrampTotal, "post-.text section address");
917+
checkedAddUint64(ShAddr, TrampTotal, "post-.text section address");
929918
if (!NewAddr)
930919
return false;
931920
ShAddr = *NewAddr;
@@ -943,7 +932,7 @@ static bool adjustProgramHeaders(uint8_t *Elf, size_t ElfSize,
943932
return true;
944933

945934
std::optional<uint64_t> TextEnd =
946-
checkedAdd(TextOffset, TextSize, "program header .text end");
935+
checkedAddUint64(TextOffset, TextSize, "program header .text end");
947936
if (!TextEnd)
948937
return false;
949938
uint64_t Phoff;
@@ -958,7 +947,7 @@ static bool adjustProgramHeaders(uint8_t *Elf, size_t ElfSize,
958947
for (uint16_t I = 0; I < Phnum; ++I) {
959948
uint64_t PhTableDelta = static_cast<uint64_t>(I) * Phentsize;
960949
std::optional<uint64_t> PhPos =
961-
checkedAdd(Phoff, PhTableDelta, "program header entry offset");
950+
checkedAddUint64(Phoff, PhTableDelta, "program header entry offset");
962951
if (!PhPos)
963952
return false;
964953
if (*PhPos > ElfSize || sizeof(Phdr) > ElfSize - *PhPos)
@@ -972,14 +961,14 @@ static bool adjustProgramHeaders(uint8_t *Elf, size_t ElfSize,
972961
std::memcpy(&PMemsz, Ph + offsetof(Phdr, p_memsz), sizeof(PMemsz));
973962

974963
std::optional<uint64_t> PEnd =
975-
checkedAdd(POffset, PFilesz, "program header file end");
964+
checkedAddUint64(POffset, PFilesz, "program header file end");
976965
if (!PEnd)
977966
return false;
978967
if (POffset <= TextOffset && *PEnd >= *TextEnd) {
979968
std::optional<uint64_t> NewPFilesz =
980-
checkedAdd(PFilesz, TrampTotal, "program header file size");
969+
checkedAddUint64(PFilesz, TrampTotal, "program header file size");
981970
std::optional<uint64_t> NewPMemsz =
982-
checkedAdd(PMemsz, TrampTotal, "program header memory size");
971+
checkedAddUint64(PMemsz, TrampTotal, "program header memory size");
983972
if (!NewPFilesz || !NewPMemsz)
984973
return false;
985974
PFilesz = *NewPFilesz;
@@ -988,23 +977,23 @@ static bool adjustProgramHeaders(uint8_t *Elf, size_t ElfSize,
988977
std::memcpy(Ph + offsetof(Phdr, p_memsz), &PMemsz, sizeof(PMemsz));
989978
} else if (POffset > TextOffset) {
990979
std::optional<uint64_t> NewPOffset =
991-
checkedAdd(POffset, TrampTotal, "post-.text program offset");
980+
checkedAddUint64(POffset, TrampTotal, "post-.text program offset");
992981
if (!NewPOffset)
993982
return false;
994983
POffset = *NewPOffset;
995984
std::memcpy(Ph + offsetof(Phdr, p_offset), &POffset, sizeof(POffset));
996985
uint64_t PVaddr;
997986
std::memcpy(&PVaddr, Ph + offsetof(Phdr, p_vaddr), sizeof(PVaddr));
998987
std::optional<uint64_t> NewPVaddr =
999-
checkedAdd(PVaddr, TrampTotal, "post-.text program vaddr");
988+
checkedAddUint64(PVaddr, TrampTotal, "post-.text program vaddr");
1000989
if (!NewPVaddr)
1001990
return false;
1002991
PVaddr = *NewPVaddr;
1003992
std::memcpy(Ph + offsetof(Phdr, p_vaddr), &PVaddr, sizeof(PVaddr));
1004993
uint64_t PPaddr;
1005994
std::memcpy(&PPaddr, Ph + offsetof(Phdr, p_paddr), sizeof(PPaddr));
1006995
std::optional<uint64_t> NewPPaddr =
1007-
checkedAdd(PPaddr, TrampTotal, "post-.text program paddr");
996+
checkedAddUint64(PPaddr, TrampTotal, "post-.text program paddr");
1008997
if (!NewPPaddr)
1009998
return false;
1010999
PPaddr = *NewPPaddr;
@@ -1081,7 +1070,7 @@ static bool adjustSymbolValues(uint8_t *Elf, size_t ElfSize,
10811070

10821071
uint64_t SymOffset = SymBytes - File.base();
10831072
std::optional<uint64_t> Value =
1084-
checkedAdd(Sym.st_value, TrampTotal, "post-.text symbol value");
1073+
checkedAddUint64(Sym.st_value, TrampTotal, "post-.text symbol value");
10851074
if (!Value)
10861075
return false;
10871076
uint64_t Value64 = *Value;
@@ -1122,8 +1111,8 @@ ElfView::growWithTrampolines(ArrayRef<Trampoline> Trampolines,
11221111
return nullptr;
11231112
}
11241113

1125-
std::optional<uint64_t> TextEnd =
1126-
checkedAdd(textOffset(), textSize(), "growWithTrampolines .text end");
1114+
std::optional<uint64_t> TextEnd = checkedAddUint64(
1115+
textOffset(), textSize(), "growWithTrampolines .text end");
11271116
if (!TextEnd || *TextEnd > InputSize) {
11281117
log() << "hotswap: error: growWithTrampolines: .text range exceeds input "
11291118
<< "ELF size.\n";

0 commit comments

Comments
 (0)