diff --git a/bolt/include/bolt/Profile/DataAggregator.h b/bolt/include/bolt/Profile/DataAggregator.h --- a/bolt/include/bolt/Profile/DataAggregator.h +++ b/bolt/include/bolt/Profile/DataAggregator.h @@ -278,7 +278,7 @@ /// Parser helpers /// Return false if we exhausted our parser buffer and finished parsing /// everything - bool hasData(); + bool hasData() const { return !ParsingBuf.empty(); } /// Print heat map based on LBR samples. std::error_code printLBRHeatMap(); @@ -359,10 +359,6 @@ /// Parse a single pair of binary full path and associated build-id Optional> parseNameBuildIDPair(); - /// Parse the output generated by "perf buildid-list" to extract build-ids - /// and return a file name matching a given \p FileBuildID. - Optional getFileNameForBuildID(StringRef FileBuildID); - /// Coordinate reading and parsing of pre-aggregated file /// /// The regular perf2bolt aggregation job is to read perf output directly. @@ -470,6 +466,17 @@ void dump(const LBREntry &LBR) const; void dump(const PerfBranchSample &Sample) const; void dump(const PerfMemSample &Sample) const; + +public: + /// If perf.data was collected without build ids, the buildid-list may contain + /// incomplete entries. Return true if the buffer containing + /// "perf buildid-list" output has only valid entries and is non- empty. + /// Return false otherwise. + bool hasAllBuildIDs(); + + /// Parse the output generated by "perf buildid-list" to extract build-ids + /// and return a file name matching a given \p FileBuildID. + Optional getFileNameForBuildID(StringRef FileBuildID); }; } // namespace bolt } // namespace llvm diff --git a/bolt/include/bolt/Profile/DataReader.h b/bolt/include/bolt/Profile/DataReader.h --- a/bolt/include/bolt/Profile/DataReader.h +++ b/bolt/include/bolt/Profile/DataReader.h @@ -502,6 +502,9 @@ /// Maps of common LTO names to possible matching profiles. StringMap> LTOCommonNameMap; StringMap> LTOCommonNameMemMap; + +public: + void setParsingBuffer(StringRef Buffer) { ParsingBuf = Buffer; } }; } // namespace bolt diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp --- a/bolt/lib/Profile/DataAggregator.cpp +++ b/bolt/lib/Profile/DataAggregator.cpp @@ -77,7 +77,7 @@ cl::Hidden, cl::cat(AggregatorCategory)); -static cl::opt ReadPreAggregated( +cl::opt ReadPreAggregated( "pa", cl::desc("skip perf and read data from a pre-aggregated file format"), cl::cat(AggregatorCategory)); @@ -294,23 +294,22 @@ FileBuf = std::move(*MB); ParsingBuf = FileBuf->getBuffer(); - if (ParsingBuf.empty()) { - errs() << "PERF2BOLT-WARNING: build-id will not be checked because perf " - "data was recorded without it\n"; - return; - } - Col = 0; - Line = 1; Optional FileName = getFileNameForBuildID(FileBuildID); if (!FileName) { - errs() << "PERF2BOLT-ERROR: failed to match build-id from perf output. " - "This indicates the input binary supplied for data aggregation " - "is not the same recorded by perf when collecting profiling " - "data, or there were no samples recorded for the binary. " - "Use -ignore-build-id option to override.\n"; - if (!opts::IgnoreBuildID) - abort(); + if (hasAllBuildIDs()) { + errs() << "PERF2BOLT-ERROR: failed to match build-id from perf output. " + "This indicates the input binary supplied for data aggregation " + "is not the same recorded by perf when collecting profiling " + "data, or there were no samples recorded for the binary. " + "Use -ignore-build-id option to override.\n"; + if (!opts::IgnoreBuildID) + abort(); + } else { + errs() << "PERF2BOLT-WARNING: build-id will not be checked because perf " + "data was recorded without it\n"; + return; + } } else if (*FileName != llvm::sys::path::filename(BC->getFilename())) { errs() << "PERF2BOLT-WARNING: build-id matched a different file name\n"; BuildIDBinaryName = std::string(*FileName); @@ -1274,13 +1273,6 @@ Type}; } -bool DataAggregator::hasData() { - if (ParsingBuf.size() == 0) - return false; - - return true; -} - bool DataAggregator::ignoreKernelInterrupt(LBREntry &LBR) const { return opts::IgnoreInterruptLBR && (LBR.From >= KernelBaseAddr || LBR.To >= KernelBaseAddr); @@ -2152,6 +2144,11 @@ if (std::error_code EC = BuildIDStr.getError()) return NoneType(); + // If one of the strings is missing, don't issue a parsing error, but still + // do not return a value. + if (ParsingBuf[0] == '\n') + return NoneType(); + ErrorOr NameStr = parseString(FieldSeparator, true); if (std::error_code EC = NameStr.getError()) return NoneType(); @@ -2160,16 +2157,48 @@ return std::make_pair(NameStr.get(), BuildIDStr.get()); } +bool DataAggregator::hasAllBuildIDs() { + const StringRef SavedParsingBuf = ParsingBuf; + + if (!hasData()) + return false; + + bool HasInvalidEntries = false; + while (hasData()) { + if (!parseNameBuildIDPair()) { + HasInvalidEntries = true; + break; + } + } + + ParsingBuf = SavedParsingBuf; + + return !HasInvalidEntries; +} + Optional DataAggregator::getFileNameForBuildID(StringRef FileBuildID) { + const StringRef SavedParsingBuf = ParsingBuf; + + StringRef FileName; while (hasData()) { Optional> IDPair = parseNameBuildIDPair(); - if (!IDPair) - return NoneType(); + if (!IDPair) { + consumeRestOfLine(); + continue; + } - if (IDPair->second.startswith(FileBuildID)) - return sys::path::filename(IDPair->first); + if (IDPair->second.startswith(FileBuildID)) { + FileName = sys::path::filename(IDPair->first); + break; + } } + + ParsingBuf = SavedParsingBuf; + + if (!FileName.empty()) + return FileName; + return NoneType(); } diff --git a/bolt/unittests/CMakeLists.txt b/bolt/unittests/CMakeLists.txt --- a/bolt/unittests/CMakeLists.txt +++ b/bolt/unittests/CMakeLists.txt @@ -6,3 +6,4 @@ endfunction() add_subdirectory(Core) +add_subdirectory(Profile) diff --git a/bolt/unittests/Profile/CMakeLists.txt b/bolt/unittests/Profile/CMakeLists.txt new file mode 100644 --- /dev/null +++ b/bolt/unittests/Profile/CMakeLists.txt @@ -0,0 +1,9 @@ +add_bolt_unittest(ProfileTests + DataAggregator.cpp + ) + +target_link_libraries(ProfileTests + PRIVATE + LLVMBOLTProfile + ) + diff --git a/bolt/unittests/Profile/DataAggregator.cpp b/bolt/unittests/Profile/DataAggregator.cpp new file mode 100644 --- /dev/null +++ b/bolt/unittests/Profile/DataAggregator.cpp @@ -0,0 +1,51 @@ +//===- bolt/unittests/Profile/DataAggregator.cpp --------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "bolt/Profile/DataAggregator.h" +#include "llvm/Support/CommandLine.h" +#include "gtest/gtest.h" + +using namespace llvm; +using namespace llvm::bolt; + +namespace opts { +extern cl::opt ReadPreAggregated; +} + +TEST(DataAggregatorTest, buildID) { + // Avoid looking for perf tool. + opts::ReadPreAggregated = true; + + DataAggregator DA(""); + Optional FileName; + + DA.setParsingBuffer(""); + ASSERT_FALSE(DA.hasAllBuildIDs()); + FileName = DA.getFileNameForBuildID("1234"); + ASSERT_FALSE(FileName); + + StringRef PartialValidBuildIDs = " File0\n" + "1111 File1\n" + " File2\n"; + DA.setParsingBuffer(PartialValidBuildIDs); + ASSERT_FALSE(DA.hasAllBuildIDs()); + FileName = DA.getFileNameForBuildID("0000"); + ASSERT_FALSE(FileName); + FileName = DA.getFileNameForBuildID("1111"); + ASSERT_EQ(*FileName, "File1"); + + StringRef AllValidBuildIDs = "0000 File0\n" + "1111 File1\n" + "2222 File2\n"; + DA.setParsingBuffer(AllValidBuildIDs); + ASSERT_TRUE(DA.hasAllBuildIDs()); + FileName = DA.getFileNameForBuildID("1234"); + ASSERT_FALSE(FileName); + FileName = DA.getFileNameForBuildID("2222"); + ASSERT_EQ(*FileName, "File2"); +}