[llvm] 89a2e70 - [llvm-profdata] Emit warning when counter value is greater than 2^56. (#69513)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 31 13:40:55 PDT 2023
Author: Zequan Wu
Date: 2023-10-31T16:40:51-04:00
New Revision: 89a2e701596835ba2714e190990da09d4e976a9a
URL: https://github.com/llvm/llvm-project/commit/89a2e701596835ba2714e190990da09d4e976a9a
DIFF: https://github.com/llvm/llvm-project/commit/89a2e701596835ba2714e190990da09d4e976a9a.diff
LOG: [llvm-profdata] Emit warning when counter value is greater than 2^56. (#69513)
Fixes #65416
Added:
Modified:
llvm/include/llvm/ProfileData/InstrProf.h
llvm/include/llvm/ProfileData/InstrProfReader.h
llvm/lib/ProfileData/InstrProf.cpp
llvm/lib/ProfileData/InstrProfReader.cpp
llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
llvm/tools/llvm-profdata/llvm-profdata.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index e16785efc3a3652..84eb6d3d68ad158 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -348,7 +348,8 @@ enum class instrprof_error {
uncompress_failed,
empty_raw_profile,
zlib_unavailable,
- raw_profile_version_mismatch
+ raw_profile_version_mismatch,
+ counter_value_too_large,
};
/// An ordered list of functions identified by their NameRef found in
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index cf6429a324d36b3..1ebbb5c63ed681d 100644
--- a/llvm/include/llvm/ProfileData/InstrProfReader.h
+++ b/llvm/include/llvm/ProfileData/InstrProfReader.h
@@ -202,11 +202,13 @@ class InstrProfReader {
/// instrprof file.
static Expected<std::unique_ptr<InstrProfReader>>
create(const Twine &Path, vfs::FileSystem &FS,
- const InstrProfCorrelator *Correlator = nullptr);
+ const InstrProfCorrelator *Correlator = nullptr,
+ std::function<void(Error)> Warn = nullptr);
static Expected<std::unique_ptr<InstrProfReader>>
create(std::unique_ptr<MemoryBuffer> Buffer,
- const InstrProfCorrelator *Correlator = nullptr);
+ const InstrProfCorrelator *Correlator = nullptr,
+ std::function<void(Error)> Warn = nullptr);
/// \param Weight for raw profiles use this as the temporal profile trace
/// weight
@@ -344,12 +346,19 @@ class RawInstrProfReader : public InstrProfReader {
/// Start address of binary id length and data pairs.
const uint8_t *BinaryIdsStart;
+ std::function<void(Error)> Warn;
+
+ /// Maxium counter value 2^56.
+ static const uint64_t MaxCounterValue = (1ULL << 56);
+
public:
RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer,
- const InstrProfCorrelator *Correlator)
+ const InstrProfCorrelator *Correlator,
+ std::function<void(Error)> Warn)
: DataBuffer(std::move(DataBuffer)),
Correlator(dyn_cast_or_null<const InstrProfCorrelatorImpl<IntPtrT>>(
- Correlator)) {}
+ Correlator)),
+ Warn(Warn) {}
RawInstrProfReader(const RawInstrProfReader &) = delete;
RawInstrProfReader &operator=(const RawInstrProfReader &) = delete;
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 583415ff451a087..47bd1bd93490db1 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -161,6 +161,9 @@ static std::string getInstrProfErrString(instrprof_error Err,
case instrprof_error::raw_profile_version_mismatch:
OS << "raw profile version mismatch";
break;
+ case instrprof_error::counter_value_too_large:
+ OS << "excessively large counter value suggests corrupted profile data";
+ 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 31d3ff2bcbf6ca7..fd02f1f0c123598 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -167,17 +167,20 @@ static Error printBinaryIdsInternal(raw_ostream &OS,
Expected<std::unique_ptr<InstrProfReader>>
InstrProfReader::create(const Twine &Path, vfs::FileSystem &FS,
- const InstrProfCorrelator *Correlator) {
+ const InstrProfCorrelator *Correlator,
+ std::function<void(Error)> Warn) {
// Set up the buffer to read.
auto BufferOrError = setupMemoryBuffer(Path, FS);
if (Error E = BufferOrError.takeError())
return std::move(E);
- return InstrProfReader::create(std::move(BufferOrError.get()), Correlator);
+ return InstrProfReader::create(std::move(BufferOrError.get()), Correlator,
+ Warn);
}
Expected<std::unique_ptr<InstrProfReader>>
InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
- const InstrProfCorrelator *Correlator) {
+ const InstrProfCorrelator *Correlator,
+ std::function<void(Error)> Warn) {
if (Buffer->getBufferSize() == 0)
return make_error<InstrProfError>(instrprof_error::empty_raw_profile);
@@ -186,9 +189,9 @@ InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
if (IndexedInstrProfReader::hasFormat(*Buffer))
Result.reset(new IndexedInstrProfReader(std::move(Buffer)));
else if (RawInstrProfReader64::hasFormat(*Buffer))
- Result.reset(new RawInstrProfReader64(std::move(Buffer), Correlator));
+ Result.reset(new RawInstrProfReader64(std::move(Buffer), Correlator, Warn));
else if (RawInstrProfReader32::hasFormat(*Buffer))
- Result.reset(new RawInstrProfReader32(std::move(Buffer), Correlator));
+ Result.reset(new RawInstrProfReader32(std::move(Buffer), Correlator, Warn));
else if (TextInstrProfReader::hasFormat(*Buffer))
Result.reset(new TextInstrProfReader(std::move(Buffer)));
else
@@ -706,8 +709,12 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(
// A value of zero signifies the block is covered.
Record.Counts.push_back(*Ptr == 0 ? 1 : 0);
} else {
- const auto *CounterValue = reinterpret_cast<const uint64_t *>(Ptr);
- Record.Counts.push_back(swap(*CounterValue));
+ uint64_t CounterValue = swap(*reinterpret_cast<const uint64_t *>(Ptr));
+ if (CounterValue > MaxCounterValue && Warn)
+ Warn(make_error<InstrProfError>(
+ instrprof_error::counter_value_too_large, Twine(CounterValue)));
+
+ Record.Counts.push_back(CounterValue);
}
}
diff --git a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
index e1e33824bf2f887..2e747f81a6bfae7 100644
--- a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
+++ b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
@@ -21,7 +21,6 @@ RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
-RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\10\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\4\0\1\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
@@ -42,6 +41,11 @@ RUN: printf '\0\0\4\0\1\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+
+// Make two copies for another test.
+RUN: cp %t.profraw %t-bad.profraw
+RUN: cp %t.profraw %t-good.profraw
+
// Make NumCounters = 0 so that we get "number of counters is zero" error message
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
@@ -49,5 +53,33 @@ RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\023\0\0\0\0\0\0\0' >> %t.profraw
RUN: printf '\3\0foo\0\0\0' >> %t.profraw
-RUN: not llvm-profdata show %t.profraw 2>&1 | FileCheck %s
-CHECK: malformed instrumentation profile data: number of counters is zero
+RUN: not llvm-profdata show %t.profraw 2>&1 | FileCheck %s --check-prefix=ZERO
+ZERO: malformed instrumentation profile data: number of counters is zero
+
+// Test a counter value greater than 2^56.
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t-bad.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t-bad.profraw
+// Counter value is 72057594037927937
+RUN: printf '\1\0\0\0\0\0\0\1' >> %t-bad.profraw
+RUN: printf '\3\0foo\0\0\0' >> %t-bad.profraw
+
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t-good.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t-good.profraw
+// Counter value is 72057594037927937
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t-good.profraw
+RUN: printf '\3\0foo\0\0\0' >> %t-good.profraw
+
+// llvm-profdata fails if there is a warning for any input file under default failure mode (any).
+RUN: not llvm-profdata merge %t-bad.profraw %t-good.profraw -o %t.profdata 2>&1 | FileCheck %s --check-prefix=ANY
+ANY: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
+
+// -failure-mode=all only fails if there is a warning for every input file.
+RUN: not llvm-profdata merge %t-bad.profraw -failure-mode=all -o %t.profdata 2>&1 | FileCheck %s --check-prefix=ALL-ERR
+ALL-ERR: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
+
+RUN: llvm-profdata merge %t-bad.profraw %t-good.profraw -failure-mode=all -o %t.profdata 2>&1 | FileCheck %s --check-prefix=ALL-WARN
+ALL-WARN: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
+
+// -failure-mode=warn does not fail at all. It only prints warnings.
+RUN: llvm-profdata merge %t-bad.profraw -failure-mode=warn -o %t.profdata 2>&1 | FileCheck %s --check-prefix=WARN
+WARN: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 7d665a8005b0d62..84290ac609a6acd 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -114,8 +114,8 @@ static void exitWithErrorCode(std::error_code EC, StringRef Whence = "") {
namespace {
enum ProfileKinds { instr, sample, memory };
-enum FailureMode { failIfAnyAreInvalid, failIfAllAreInvalid };
-}
+enum FailureMode { warnOnly, failIfAnyAreInvalid, failIfAllAreInvalid };
+} // namespace
static void warnOrExitGivenError(FailureMode FailMode, std::error_code EC,
StringRef Whence = "") {
@@ -314,7 +314,22 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
}
auto FS = vfs::getRealFileSystem();
- auto ReaderOrErr = InstrProfReader::create(Input.Filename, *FS, Correlator);
+ // TODO: This only saves the first non-fatal error from InstrProfReader, and
+ // then added to WriterContext::Errors. However, this is not extensible, if
+ // we have more non-fatal errors from InstrProfReader in the future. How
+ // should this interact with
diff erent -failure-mode?
+ std::optional<std::pair<Error, std::string>> ReaderWarning;
+ auto Warn = [&](Error E) {
+ if (ReaderWarning) {
+ consumeError(std::move(E));
+ return;
+ }
+ // Only show the first time an error occurs in this file.
+ auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
+ ReaderWarning = {make_error<InstrProfError>(ErrCode, Msg), Filename};
+ };
+ auto ReaderOrErr =
+ InstrProfReader::create(Input.Filename, *FS, Correlator, Warn);
if (Error E = ReaderOrErr.takeError()) {
// Skip the empty profiles by returning silently.
auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
@@ -362,14 +377,23 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
Traces, Reader->getTemporalProfTraceStreamSize());
}
if (Reader->hasError()) {
- if (Error E = Reader->getError())
+ if (Error E = Reader->getError()) {
WC->Errors.emplace_back(std::move(E), Filename);
+ return;
+ }
}
std::vector<llvm::object::BuildID> BinaryIds;
- if (Error E = Reader->readBinaryIds(BinaryIds))
+ if (Error E = Reader->readBinaryIds(BinaryIds)) {
WC->Errors.emplace_back(std::move(E), Filename);
+ return;
+ }
WC->Writer.addBinaryIds(BinaryIds);
+
+ if (ReaderWarning) {
+ WC->Errors.emplace_back(std::move(ReaderWarning->first),
+ ReaderWarning->second);
+ }
}
/// Merge the \p Src writer context into \p Dst.
@@ -492,7 +516,7 @@ mergeInstrProfile(const WeightedFileVector &Inputs, StringRef DebugInfoFilename,
warn(toString(std::move(ErrorPair.first)), ErrorPair.second);
}
}
- if (NumErrors == Inputs.size() ||
+ if ((NumErrors == Inputs.size() && FailMode == failIfAllAreInvalid) ||
(NumErrors > 0 && FailMode == failIfAnyAreInvalid))
exitWithError("no profile can be merged");
@@ -1202,10 +1226,12 @@ static int merge_main(int argc, const char *argv[]) {
"GCC encoding (only meaningful for -sample)")));
cl::opt<FailureMode> FailureMode(
"failure-mode", cl::init(failIfAnyAreInvalid), cl::desc("Failure mode:"),
- cl::values(clEnumValN(failIfAnyAreInvalid, "any",
- "Fail if any profile is invalid."),
- clEnumValN(failIfAllAreInvalid, "all",
- "Fail only if all profiles are invalid.")));
+ cl::values(
+ clEnumValN(warnOnly, "warn", "Do not fail and just print warnings."),
+ clEnumValN(failIfAnyAreInvalid, "any",
+ "Fail if any profile is invalid."),
+ clEnumValN(failIfAllAreInvalid, "all",
+ "Fail only if all profiles are invalid.")));
cl::opt<bool> OutputSparse("sparse", cl::init(false),
cl::desc("Generate a sparse profile (only meaningful for -instr)"));
cl::opt<unsigned> NumThreads(
More information about the llvm-commits
mailing list