[llvm] [SHT_LLVM_BB_ADDR_MAP] Adds test for when PGO exists but feature bit is off. (PR #77366)
Micah Weston via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 8 14:08:33 PST 2024
https://github.com/red1bluelost updated https://github.com/llvm/llvm-project/pull/77366
>From 0353f3719169be101cca57ebda0cbffe1a171724 Mon Sep 17 00:00:00 2001
From: Micah Weston <micahsweston at gmail.com>
Date: Mon, 8 Jan 2024 14:58:48 -0500
Subject: [PATCH 1/2] [SHT_LLVM_BB_ADDR_MAP] Adds test for when PGO exists but
feature bit is off.
---
llvm/unittests/Object/ELFObjectFileTest.cpp | 28 +++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp
index 3a2a8e3d3264db..005f1448982a98 100644
--- a/llvm/unittests/Object/ELFObjectFileTest.cpp
+++ b/llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -897,6 +897,34 @@ TEST(ELFObjectFileTest, InvalidDecodePGOAnalysisMap) {
DoCheck(MissingBrProb, "unable to decode LEB128 at offset 0x00000017: "
"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.
>From f6374bd9b45c62cfcd6b13e4d2369b423128a524 Mon Sep 17 00:00:00 2001
From: Micah Weston <micahsweston at gmail.com>
Date: Mon, 8 Jan 2024 17:08:17 -0500
Subject: [PATCH 2/2] fixup! [SHT_LLVM_BB_ADDR_MAP] Adds test for when PGO
exists but feature bit is off.
Updates ELFYaml to be geared for testing and adds tests.
---
llvm/include/llvm/ObjectYAML/ELFYAML.h | 4 +-
llvm/lib/ObjectYAML/ELFEmitter.cpp | 154 ++++++++++----------
llvm/lib/ObjectYAML/ELFYAML.cpp | 4 +-
llvm/unittests/Object/ELFObjectFileTest.cpp | 73 +++++++++-
4 files changed, 154 insertions(+), 81 deletions(-)
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 005f1448982a98..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,15 @@ 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);
More information about the llvm-commits
mailing list