[llvm-profdata] Emit warning when counter value is greater than 2^56. (#69513)

Fixes #65416
This commit is contained in:
Zequan Wu 2023-10-31 16:40:51 -04:00 committed by GitHub
parent 703de006d3
commit 89a2e70159
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 103 additions and 25 deletions

View File

@ -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

View File

@ -202,11 +202,13 @@ public:
/// 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 @@ private:
/// 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;

View File

@ -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.

View File

@ -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);
}
}

View File

@ -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

View File

@ -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 different -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(