[clang] 17cfd2e - [profiling] Improve error message for raw profile header mismatches
Jessica Paquette via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 27 14:52:12 PDT 2023
Author: Jessica Paquette
Date: 2023-04-27T14:51:38-07:00
New Revision: 17cfd2e025cb3aa929ad219c6ed0974d6198bf5b
URL: https://github.com/llvm/llvm-project/commit/17cfd2e025cb3aa929ad219c6ed0974d6198bf5b
DIFF: https://github.com/llvm/llvm-project/commit/17cfd2e025cb3aa929ad219c6ed0974d6198bf5b.diff
LOG: [profiling] Improve error message for raw profile header mismatches
When a user uses a mismatched clang + llvm-profdata, they didn't get a very
informative error message. It would just say "unsupported version".
As a result, users are often confused as to what they are supposed to do and
tend to assume that it's a bug in the profiling runtime.
This patch improves the error message by:
- Adding a new class of error (`raw_profile_version_mismatch`) to make it clear
that, specifically, the *raw profile* version is unsupported because of a
tool mismatch.
- Adding an error message that tells the user which raw profile version was
encountered, which version was expected, and instructs them to align their
tool versions.
To support this, this patch also updates `InstrProfError::take` to also
propagate the optional error message.
Differential Revision: https://reviews.llvm.org/D149361
Added:
llvm/test/tools/llvm-profdata/mismatched-raw-profile-header.test
Modified:
clang/lib/CodeGen/CodeGenPGO.cpp
llvm/include/llvm/ProfileData/InstrProf.h
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
llvm/lib/ProfileData/InstrProf.cpp
llvm/lib/ProfileData/InstrProfReader.cpp
llvm/tools/llvm-profdata/llvm-profdata.cpp
llvm/unittests/ProfileData/InstrProfTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 15a3d74666ca2..b80317529b72b 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1036,7 +1036,7 @@ void CodeGenPGO::loadRegionCounts(llvm::IndexedInstrProfReader *PGOReader,
llvm::Expected<llvm::InstrProfRecord> RecordExpected =
PGOReader->getInstrProfRecord(FuncName, FunctionHash);
if (auto E = RecordExpected.takeError()) {
- auto IPE = llvm::InstrProfError::take(std::move(E));
+ auto IPE = std::get<0>(llvm::InstrProfError::take(std::move(E)));
if (IPE == llvm::instrprof_error::unknown_function)
CGM.getPGOStats().addMissing(IsInMainFile);
else if (IPE == llvm::instrprof_error::hash_mismatch)
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 2a9da23f4f02f..ef01070832dee 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -330,7 +330,8 @@ enum class instrprof_error {
compress_failed,
uncompress_failed,
empty_raw_profile,
- zlib_unavailable
+ zlib_unavailable,
+ raw_profile_version_mismatch
};
/// An ordered list of functions identified by their NameRef found in
@@ -362,15 +363,18 @@ class InstrProfError : public ErrorInfo<InstrProfError> {
instrprof_error get() const { return Err; }
const std::string &getMessage() const { return Msg; }
- /// Consume an Error and return the raw enum value contained within it. The
- /// Error must either be a success value, or contain a single InstrProfError.
- static instrprof_error take(Error E) {
+ /// Consume an Error and return the raw enum value contained within it, and
+ /// the optional error message. The Error must either be a success value, or
+ /// contain a single InstrProfError.
+ static std::pair<instrprof_error, std::string> take(Error E) {
auto Err = instrprof_error::success;
- handleAllErrors(std::move(E), [&Err](const InstrProfError &IPE) {
+ std::string Msg = "";
+ handleAllErrors(std::move(E), [&Err, &Msg](const InstrProfError &IPE) {
assert(Err == instrprof_error::success && "Multiple errors encountered");
Err = IPE.get();
+ Msg = IPE.getMessage();
});
- return Err;
+ return {Err, Msg};
}
static char ID;
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index d6aec276838e6..849ee80bfaa33 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -249,7 +249,7 @@ Error CoverageMapping::loadFunctionRecord(
std::vector<uint64_t> Counts;
if (Error E = ProfileReader.getFunctionCounts(Record.FunctionName,
Record.FunctionHash, Counts)) {
- instrprof_error IPE = InstrProfError::take(std::move(E));
+ instrprof_error IPE = std::get<0>(InstrProfError::take(std::move(E)));
if (IPE == instrprof_error::hash_mismatch) {
FuncHashMismatches.emplace_back(std::string(Record.FunctionName),
Record.FunctionHash);
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index d8dbcf1ebbe9a..1dd1ce4b3e50a 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -153,6 +153,9 @@ static std::string getInstrProfErrString(instrprof_error Err,
OS << "profile uses zlib compression but the profile reader was built "
"without zlib support";
break;
+ case instrprof_error::raw_profile_version_mismatch:
+ OS << "raw profile version mismatch";
+ break;
}
// If optional error message is not empty, append it to the message.
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index 47c0a57a3da92..4160f7e6dfd55 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -532,7 +532,13 @@ Error RawInstrProfReader<IntPtrT>::readHeader(
const RawInstrProf::Header &Header) {
Version = swap(Header.Version);
if (GET_VERSION(Version) != RawInstrProf::Version)
- return error(instrprof_error::unsupported_version);
+ return error(instrprof_error::raw_profile_version_mismatch,
+ ("Profile uses raw profile format version = " +
+ Twine(GET_VERSION(Version)) +
+ "; expected version = " + Twine(RawInstrProf::Version) +
+ "\nPLEASE update this tool to version in the raw profile, or "
+ "regenerate raw profile with expected version.")
+ .str());
if (useDebugInfoCorrelate() && !Correlator)
return error(instrprof_error::missing_debug_info_for_correlation);
if (!useDebugInfoCorrelate() && Correlator)
@@ -1199,7 +1205,8 @@ InstrProfSymtab &IndexedInstrProfReader::getSymtab() {
std::unique_ptr<InstrProfSymtab> NewSymtab = std::make_unique<InstrProfSymtab>();
if (Error E = Index->populateSymtab(*NewSymtab)) {
- consumeError(error(InstrProfError::take(std::move(E))));
+ auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
+ consumeError(error(ErrCode, Msg));
}
Symtab = std::move(NewSymtab);
diff --git a/llvm/test/tools/llvm-profdata/mismatched-raw-profile-header.test b/llvm/test/tools/llvm-profdata/mismatched-raw-profile-header.test
new file mode 100644
index 0000000000000..24f3f563e9689
--- /dev/null
+++ b/llvm/test/tools/llvm-profdata/mismatched-raw-profile-header.test
@@ -0,0 +1,19 @@
+// Magic
+RUN: printf '\377lprofr\201' > %t
+// Version
+RUN: printf '\0\01\0\0\0\0\0\10' >> %t
+// The rest of the header needs to be there to prevent a broken header error.
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t
+RUN: printf '\0\0\0\0\0\0\0\2' >> %t
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t
+RUN: printf '\0\0\0\0\0\0\0\3' >> %t
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t
+RUN: printf '\0\0\0\0\0\0\0\20' >> %t
+RUN: printf '\0\0\0\1\0\4\0\0' >> %t
+RUN: printf '\0\0\0\2\0\4\0\0' >> %t
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t
+
+RUN: not llvm-profdata show %t -o /dev/null 2>&1 | FileCheck %s
+
+CHECK: raw profile version mismatch: Profile uses raw profile format version = 281474976710664; expected version = {{[0-9]+}}
+CHECK-NEXT: PLEASE update this tool to version in the raw profile, or regenerate raw profile with expected version.
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 3db5479b8e592..e14bfd826cfd7 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -233,9 +233,10 @@ static void overlapInput(const std::string &BaseFilename,
auto ReaderOrErr = InstrProfReader::create(TestFilename, *FS);
if (Error E = ReaderOrErr.takeError()) {
// Skip the empty profiles by returning sliently.
- instrprof_error IPE = InstrProfError::take(std::move(E));
- if (IPE != instrprof_error::empty_raw_profile)
- WC->Errors.emplace_back(make_error<InstrProfError>(IPE), TestFilename);
+ auto [ErrorCode, Msg] = InstrProfError::take(std::move(E));
+ if (ErrorCode != instrprof_error::empty_raw_profile)
+ WC->Errors.emplace_back(make_error<InstrProfError>(ErrorCode, Msg),
+ TestFilename);
return;
}
@@ -280,8 +281,9 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
}
auto MemProfError = [&](Error E) {
- instrprof_error IPE = InstrProfError::take(std::move(E));
- WC->Errors.emplace_back(make_error<InstrProfError>(IPE), Filename);
+ auto [ErrorCode, Msg] = InstrProfError::take(std::move(E));
+ WC->Errors.emplace_back(make_error<InstrProfError>(ErrorCode, Msg),
+ Filename);
};
// Add the frame mappings into the writer context.
@@ -306,9 +308,10 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
auto ReaderOrErr = InstrProfReader::create(Input.Filename, *FS, Correlator);
if (Error E = ReaderOrErr.takeError()) {
// Skip the empty profiles by returning silently.
- instrprof_error IPE = InstrProfError::take(std::move(E));
- if (IPE != instrprof_error::empty_raw_profile)
- WC->Errors.emplace_back(make_error<InstrProfError>(IPE), Filename);
+ auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
+ if (ErrCode != instrprof_error::empty_raw_profile)
+ WC->Errors.emplace_back(make_error<InstrProfError>(ErrCode, Msg),
+ Filename);
return;
}
@@ -335,11 +338,11 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
}
Reported = true;
// Only show hint the first time an error occurs.
- instrprof_error IPE = InstrProfError::take(std::move(E));
+ auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
std::unique_lock<std::mutex> ErrGuard{WC->ErrLock};
- bool firstTime = WC->WriterErrorCodes.insert(IPE).second;
- handleMergeWriterError(make_error<InstrProfError>(IPE), Input.Filename,
- FuncName, firstTime);
+ bool firstTime = WC->WriterErrorCodes.insert(ErrCode).second;
+ handleMergeWriterError(make_error<InstrProfError>(ErrCode, Msg),
+ Input.Filename, FuncName, firstTime);
});
}
@@ -370,11 +373,11 @@ static void mergeWriterContexts(WriterContext *Dst, WriterContext *Src) {
exitWithError(std::move(E));
Dst->Writer.mergeRecordsFromWriter(std::move(Src->Writer), [&](Error E) {
- instrprof_error IPE = InstrProfError::take(std::move(E));
+ auto [ErrorCode, Msg] = InstrProfError::take(std::move(E));
std::unique_lock<std::mutex> ErrGuard{Dst->ErrLock};
- bool firstTime = Dst->WriterErrorCodes.insert(IPE).second;
+ bool firstTime = Dst->WriterErrorCodes.insert(ErrorCode).second;
if (firstTime)
- warn(toString(make_error<InstrProfError>(IPE)));
+ warn(toString(make_error<InstrProfError>(ErrorCode, Msg)));
});
}
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index 3ed21767035d2..d7b9d73c17b1b 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -869,7 +869,9 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1_saturation) {
const uint64_t MaxEdgeCount = getInstrMaxCountValue();
instrprof_error Result;
- auto Err = [&](Error E) { Result = InstrProfError::take(std::move(E)); };
+ auto Err = [&](Error E) {
+ Result = std::get<0>(InstrProfError::take(std::move(E)));
+ };
Result = instrprof_error::success;
Writer.addRecord({"foo", 0x1234, {1}}, Err);
ASSERT_EQ(Result, instrprof_error::success);
More information about the cfe-commits
mailing list