[SHT_LLVM_BB_ADDR_MAP] Updates ELFYaml and adds tests for PGOAnalysisMap. #77366
Conversation
| )"; | ||
|
|
||
| DoCheck(ZeroFeatureButWithAnalyses, | ||
| "unsupported SHT_LLVM_BB_ADDR_MAP version: 232"); |
There was a problem hiding this comment.
My guess (based on code in llvm/lib/ObjectYAML/ELFEmitter.cpp) is that PGOAnalyses data won't be emitted in the section. We should be able to encode and emit the data (regardless of the feature value), but decoding must fail because of the inconsistent feature value. @jh7370 initially asked me to test a similar scenario for the NumBlocks and Version fields.
There was a problem hiding this comment.
That's only the case if len(PGOAnalyses) != len(Entries) which isn't the case here.
I could remove those returns in ELFEmitter so that it just emits Entries and PGOAnalyses interspersed until the shorter one runs out then just emit the longer one until the end. Do you think this is preferable for testing?
There was a problem hiding this comment.
I noticed that this is the case on the previous test MissingBrProb. I'm going to remove the early returns so that ELFEmitter is more geared for testing.
There was a problem hiding this comment.
I updated ELFYaml emitting so that we can emit invalid forms for testing. The emitter will now just emit Entries and PGOAnalyses interspersed until one runs out in which is just emits the other one until it ends.
Kept in the warnings in ELFYaml. I also added a two more tests.
There was a problem hiding this comment.
My bad. I misinterpreted the if conditions in ELFEmitter.cpp (I thought they were checking the feature value).
On a different note, I think we should make these lit tests and not unit tests. As we add more unit tests in a single file, maintenance/reviews get harder. WDYT?
There was a problem hiding this comment.
Sounds good, I'll close this and make it a part of a separate PR.
There was a problem hiding this comment.
FWIW, I would lean towards unit tests if the setup is straightforward. Apart from anything else, lit tests on Windows have a non-trivial performance cost (much worse than extending an existing unit test), due to the process launching overhead.
Regardless of the approach taken, it's important to test all failure cases in the ObjectYAML code, plus cases like "this thing is optional - test that it can be omitted or specified" and "this thing is required - test that an error is emitted if missing".
There was a problem hiding this comment.
FWIW, I would lean towards unit tests if the setup is straightforward. Apart from anything else, lit tests on Windows have a non-trivial performance cost (much worse than extending an existing unit test), due to the process launching overhead.
Thanks for chiming in @jh7370 . One problem I have with our unit tests is that if we test multiple cases in a single test function, it's very hard to associate a failure to the test case. Although, while writing this I realized we can extend our test lambda (DoCheckFails and DoCheckSucceeds) with a string to represent the test case.
There was a problem hiding this comment.
On the note of tracing back errors to test cases, some of the clang-format unit tests have a pattern where they take a source location as arguments, then provide a macro to automatically put in __FILE__ and __LINE__ macros to make it easier to trace back errors. I'll try to implement that in a separate PR and then possibly reopen this if we find that approach good.
There was a problem hiding this comment.
That seems a reasonable way to do it. gtest has a couple of options for printing additional context, but the simplest is to just do << Message after the EXPECT_* or ASSERT_* macro (with the Message being whatever extra context you want printed).
…re bit is off. Updates ELFYaml to be geared for testing and adds tests.
|
@llvm/pr-subscribers-objectyaml Author: Micah Weston (red1bluelost) ChangesNew test as mentioned here #77139 (comment) Full diff: https://github.com/llvm/llvm-project/pull/77366.diff 4 Files Affected:
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.
|
red1bluelost
changed the title
New test as mentioned here #77139 (comment)