[llvm] [SHT_LLVM_BB_ADDR_MAP] Updates ELFYaml and adds tests for PGOAnalysisMap. (PR #77366)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 8 14:09:04 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-objectyaml
Author: Micah Weston (red1bluelost)
<details>
<summary>Changes</summary>
New test as mentioned here https://github.com/llvm/llvm-project/pull/77139#discussion_r1443586319
---
Full diff: https://github.com/llvm/llvm-project/pull/77366.diff
4 Files Affected:
- (modified) llvm/include/llvm/ObjectYAML/ELFYAML.h (+2-2)
- (modified) llvm/lib/ObjectYAML/ELFEmitter.cpp (+80-74)
- (modified) llvm/lib/ObjectYAML/ELFYAML.cpp (+2-2)
- (modified) llvm/unittests/Object/ELFObjectFileTest.cpp (+98-3)
``````````diff
diff --git a/llvm/include/llvm/ObjectYAML/ELFYAML.h b/llvm/include/llvm/ObjectYAML/ELFYAML.h
index 12b47c271da2cd..fc51237e395172 100644
--- a/llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -173,8 +173,8 @@ struct BBAddrMapEntry {
struct PGOAnalysisMapEntry {
struct PGOBBEntry {
struct SuccessorEntry {
- uint32_t ID;
- llvm::yaml::Hex32 BrProb;
+ std::optional<uint32_t> ID;
+ std::optional<llvm::yaml::Hex32> BrProb;
};
std::optional<uint64_t> BBFreq;
std::optional<std::vector<SuccessorEntry>> Successors;
diff --git a/llvm/lib/ObjectYAML/ELFEmitter.cpp b/llvm/lib/ObjectYAML/ELFEmitter.cpp
index 94b0529f761052..1078d8c2de1a09 100644
--- a/llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -1391,86 +1391,92 @@ template <class ELFT>
void ELFState<ELFT>::writeSectionContent(
Elf_Shdr &SHeader, const ELFYAML::BBAddrMapSection &Section,
ContiguousBlobAccumulator &CBA) {
- if (!Section.Entries) {
- if (Section.PGOAnalyses)
- WithColor::warning()
- << "PGOAnalyses should not exist in SHT_LLVM_BB_ADDR_MAP when "
- "Entries does not exist";
- return;
- }
-
- const std::vector<ELFYAML::PGOAnalysisMapEntry> *PGOAnalyses = nullptr;
- if (Section.PGOAnalyses) {
- if (Section.Entries->size() != Section.PGOAnalyses->size())
- WithColor::warning() << "PGOAnalyses must be the same length as Entries "
- "in SHT_LLVM_BB_ADDR_MAP";
- else
- PGOAnalyses = &Section.PGOAnalyses.value();
- }
-
- for (const auto &[Idx, E] : llvm::enumerate(*Section.Entries)) {
- // Write version and feature values.
- if (Section.Type == llvm::ELF::SHT_LLVM_BB_ADDR_MAP) {
- if (E.Version > 2)
- WithColor::warning() << "unsupported SHT_LLVM_BB_ADDR_MAP version: "
- << static_cast<int>(E.Version)
- << "; encoding using the most recent version";
- CBA.write(E.Version);
- CBA.write(E.Feature);
- SHeader.sh_size += 2;
- }
+ if (!Section.Entries && Section.PGOAnalyses)
+ WithColor::warning()
+ << "PGOAnalyses should not exist in SHT_LLVM_BB_ADDR_MAP when Entries "
+ "does not exist\n";
+ if (Section.Entries && Section.PGOAnalyses &&
+ Section.Entries->size() != Section.PGOAnalyses->size())
+ WithColor::warning() << "PGOAnalyses should be the same length as Entries "
+ "in SHT_LLVM_BB_ADDR_MAP\n";
+
+ std::vector<ELFYAML::BBAddrMapEntry> EmptyBB;
+ std::vector<ELFYAML::PGOAnalysisMapEntry> EmptyPGO;
+ for (const auto &[Entry, PGOEntry] :
+ zip_longest(Section.Entries ? *Section.Entries : EmptyBB,
+ Section.PGOAnalyses ? *Section.PGOAnalyses : EmptyPGO)) {
+ if (Entry) {
+ const auto &E = *Entry;
+ // Write version and feature values.
+ if (Section.Type == llvm::ELF::SHT_LLVM_BB_ADDR_MAP) {
+ if (E.Version > 2)
+ WithColor::warning() << "unsupported SHT_LLVM_BB_ADDR_MAP version: "
+ << static_cast<int>(E.Version)
+ << "; encoding using the most recent version\n";
+ CBA.write(E.Version);
+ CBA.write(E.Feature);
+ SHeader.sh_size += 2;
+ }
- if (Section.PGOAnalyses) {
- if (E.Version < 2)
- WithColor::warning()
- << "unsupported SHT_LLVM_BB_ADDR_MAP version when using PGO: "
- << static_cast<int>(E.Version) << "; must use version >= 2";
- }
+ if (Section.PGOAnalyses) {
+ if (E.Version < 2)
+ WithColor::warning()
+ << "unsupported SHT_LLVM_BB_ADDR_MAP version when using PGO: "
+ << static_cast<int>(E.Version) << "; must use version >= 2\n";
+ }
- // Write the address of the function.
- CBA.write<uintX_t>(E.Address, ELFT::TargetEndianness);
- // Write number of BBEntries (number of basic blocks in the function). This
- // is overridden by the 'NumBlocks' YAML field when specified.
- uint64_t NumBlocks =
- E.NumBlocks.value_or(E.BBEntries ? E.BBEntries->size() : 0);
- SHeader.sh_size += sizeof(uintX_t) + CBA.writeULEB128(NumBlocks);
- // Write all BBEntries.
- if (E.BBEntries) {
- for (const ELFYAML::BBAddrMapEntry::BBEntry &BBE : *E.BBEntries) {
- if (Section.Type == llvm::ELF::SHT_LLVM_BB_ADDR_MAP && E.Version > 1)
- SHeader.sh_size += CBA.writeULEB128(BBE.ID);
- SHeader.sh_size += CBA.writeULEB128(BBE.AddressOffset) +
- CBA.writeULEB128(BBE.Size) +
- CBA.writeULEB128(BBE.Metadata);
+ // Write the address of the function.
+ CBA.write<uintX_t>(E.Address, ELFT::TargetEndianness);
+ // Write number of BBEntries (number of basic blocks in the function).
+ // This is overridden by the 'NumBlocks' YAML field when specified.
+ uint64_t NumBlocks =
+ E.NumBlocks.value_or(E.BBEntries ? E.BBEntries->size() : 0);
+ SHeader.sh_size += sizeof(uintX_t) + CBA.writeULEB128(NumBlocks);
+ // Write all BBEntries.
+ if (E.BBEntries) {
+ for (const ELFYAML::BBAddrMapEntry::BBEntry &BBE : *E.BBEntries) {
+ if (Section.Type == llvm::ELF::SHT_LLVM_BB_ADDR_MAP && E.Version > 1)
+ SHeader.sh_size += CBA.writeULEB128(BBE.ID);
+ SHeader.sh_size += CBA.writeULEB128(BBE.AddressOffset) +
+ CBA.writeULEB128(BBE.Size) +
+ CBA.writeULEB128(BBE.Metadata);
+ }
}
}
- if (!PGOAnalyses)
- continue;
- const ELFYAML::PGOAnalysisMapEntry &PGOEntry = PGOAnalyses->at(Idx);
-
- if (PGOEntry.FuncEntryCount)
- SHeader.sh_size += CBA.writeULEB128(*PGOEntry.FuncEntryCount);
-
- if (!PGOEntry.PGOBBEntries)
- continue;
-
- const auto &PGOBBEntries = PGOEntry.PGOBBEntries.value();
- if (!E.BBEntries || E.BBEntries->size() != PGOBBEntries.size()) {
- WithColor::warning() << "PBOBBEntries must be the same length as "
- "BBEntries in SHT_LLVM_BB_ADDR_MAP.\n"
- << "Mismatch on function with address: "
- << E.Address;
- continue;
- }
+ if (PGOEntry) {
+ const auto &PE = *PGOEntry;
+ if (PE.FuncEntryCount)
+ SHeader.sh_size += CBA.writeULEB128(*PE.FuncEntryCount);
+
+ if (PE.PGOBBEntries) {
+ const auto &PGOBBEntries = *PE.PGOBBEntries;
+ if (!Entry || !Entry->BBEntries ||
+ Entry->BBEntries->size() != PGOBBEntries.size()) {
+ auto &Errs = WithColor::warning()
+ << "PBOBBEntries should be the same length as BBEntries "
+ "in SHT_LLVM_BB_ADDR_MAP.\n";
+ if (Entry) {
+ Errs << "Mismatch on function with address: 0x";
+ Errs.write_hex(Entry->Address) << "\n";
+ } else {
+ Errs << "No functions exist to match\n";
+ }
+ }
- for (const auto &PGOBBE : PGOBBEntries) {
- if (PGOBBE.BBFreq)
- SHeader.sh_size += CBA.writeULEB128(*PGOBBE.BBFreq);
- if (PGOBBE.Successors) {
- SHeader.sh_size += CBA.writeULEB128(PGOBBE.Successors->size());
- for (const auto &[ID, BrProb] : *PGOBBE.Successors)
- SHeader.sh_size += CBA.writeULEB128(ID) + CBA.writeULEB128(BrProb);
+ for (const auto &PGOBBE : PGOBBEntries) {
+ if (PGOBBE.BBFreq)
+ SHeader.sh_size += CBA.writeULEB128(*PGOBBE.BBFreq);
+ if (PGOBBE.Successors) {
+ SHeader.sh_size += CBA.writeULEB128(PGOBBE.Successors->size());
+ for (const auto &[ID, BrProb] : *PGOBBE.Successors) {
+ if (ID)
+ SHeader.sh_size += CBA.writeULEB128(*ID);
+ if (BrProb)
+ SHeader.sh_size += CBA.writeULEB128(*BrProb);
+ }
+ }
+ }
}
}
}
diff --git a/llvm/lib/ObjectYAML/ELFYAML.cpp b/llvm/lib/ObjectYAML/ELFYAML.cpp
index 6ad4a067415ac8..40b0566b5f1021 100644
--- a/llvm/lib/ObjectYAML/ELFYAML.cpp
+++ b/llvm/lib/ObjectYAML/ELFYAML.cpp
@@ -1844,8 +1844,8 @@ void MappingTraits<ELFYAML::PGOAnalysisMapEntry::PGOBBEntry::SuccessorEntry>::
mapping(IO &IO,
ELFYAML::PGOAnalysisMapEntry::PGOBBEntry::SuccessorEntry &E) {
assert(IO.getContext() && "The IO context is not initialized");
- IO.mapRequired("ID", E.ID);
- IO.mapRequired("BrProb", E.BrProb);
+ IO.mapOptional("ID", E.ID);
+ IO.mapOptional("BrProb", E.BrProb);
}
void MappingTraits<ELFYAML::GnuHashHeader>::mapping(IO &IO,
diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp
index 3a2a8e3d3264db..0adb5d92608bc3 100644
--- a/llvm/unittests/Object/ELFObjectFileTest.cpp
+++ b/llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -865,6 +865,72 @@ TEST(ELFObjectFileTest, InvalidDecodePGOAnalysisMap) {
DoCheck(MissingBBFreq, "unable to decode LEB128 at offset 0x0000000f: "
"malformed uleb128, extends past end");
+ // Check that we fail when sucessors are not provided.
+ SmallString<128> MissingSuccessorEntry(CommonYamlString);
+ MissingSuccessorEntry += R"(
+ Version: 2
+ Feature: 0x04
+ BBEntries:
+ - ID: 1
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x6
+ - ID: 2
+ AddressOffset: 0x1
+ Size: 0x1
+ Metadata: 0x2
+ - ID: 3
+ AddressOffset: 0x2
+ Size: 0x1
+ Metadata: 0x2
+ PGOAnalyses:
+ - PGOBBEntries:
+ - Successors:
+ - ID: 2
+ BrProb: 0x80000000
+ - ID: 3
+ BrProb: 0x80000000
+ - Successors: []
+)";
+
+ DoCheck(MissingSuccessorEntry,
+ "unable to decode LEB128 at offset 0x00000025: malformed uleb128, "
+ "extends past end");
+
+ // Check that we fail when branch probability is enabled but not provided.
+ SmallString<128> MissingSuccessorEntryId(CommonYamlString);
+ MissingSuccessorEntryId += R"(
+ Version: 2
+ Feature: 0x04
+ BBEntries:
+ - ID: 1
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x6
+ - ID: 2
+ AddressOffset: 0x1
+ Size: 0x1
+ Metadata: 0x2
+ - ID: 3
+ AddressOffset: 0x2
+ Size: 0x1
+ Metadata: 0x2
+ PGOAnalyses:
+ - PGOBBEntries:
+ - Successors:
+ - ID: 2
+ BrProb: 0x80000000
+ - ID: 3
+ BrProb: 0x80000000
+ - Successors:
+ - BrProb: 0xF0000000
+ - Successors: []
+)";
+
+ DoCheck(MissingSuccessorEntryId,
+ "unable to decode LEB128 at offset 0x0000002b: malformed uleb128, "
+ "extends past end");
+
// Check that we fail when branch probability is enabled but not provided.
SmallString<128> MissingBrProb(CommonYamlString);
MissingBrProb += R"(
@@ -889,14 +955,43 @@ TEST(ELFObjectFileTest, InvalidDecodePGOAnalysisMap) {
- ID: 2
BrProb: 0x80000000
- ID: 3
- BrProb: 0x80000000
- Successors:
- ID: 3
BrProb: 0xF0000000
+ - Successors: []
)";
- DoCheck(MissingBrProb, "unable to decode LEB128 at offset 0x00000017: "
- "malformed uleb128, extends past end");
+ DoCheck(MissingBrProb,
+ "unable to decode LEB128 at offset 0x00000027: malformed uleb128, "
+ "extends past end");
+
+ // Check that we fail when pgo data exists but the feature bits are disabled.
+ SmallString<128> ZeroFeatureButWithAnalyses(CommonYamlString);
+ ZeroFeatureButWithAnalyses += R"(
+ Version: 2
+ Feature: 0x00
+ BBEntries:
+ - ID: 1
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x6
+ - ID: 2
+ AddressOffset: 0x1
+ Size: 0x1
+ Metadata: 0x2
+ - Address: 0x11111
+ Version: 2
+ Feature: 0x00
+ BBEntries: []
+ PGOAnalyses:
+ - PGOBBEntries:
+ - BBFreq: 1000
+ - BBFreq: 1000
+ - PGOBBEntries: []
+)";
+
+ DoCheck(ZeroFeatureButWithAnalyses,
+ "unsupported SHT_LLVM_BB_ADDR_MAP version: 232");
}
// Test for the ELFObjectFile::readBBAddrMap API with PGOAnalysisMap.
``````````
</details>
https://github.com/llvm/llvm-project/pull/77366
More information about the llvm-commits
mailing list