Skip to content

Commit 89a2e70

Browse files
ZequanWuZequanWu
authored
[llvm-profdata] Emit warning when counter value is greater than 2^56. (#69513)
Fixes #65416
1 parent 703de00 commit 89a2e70

File tree

6 files changed

+103
-25
lines changed

6 files changed

+103
-25
lines changed

‎llvm/include/llvm/ProfileData/InstrProf.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,8 @@ enum class instrprof_error {
348348
uncompress_failed,
349349
empty_raw_profile,
350350
zlib_unavailable,
351-
raw_profile_version_mismatch
351+
raw_profile_version_mismatch,
352+
counter_value_too_large,
352353
};
353354

354355
/// An ordered list of functions identified by their NameRef found in

‎llvm/include/llvm/ProfileData/InstrProfReader.h

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,11 +202,13 @@ class InstrProfReader {
202202
/// instrprof file.
203203
static Expected<std::unique_ptr<InstrProfReader>>
204204
create(const Twine &Path, vfs::FileSystem &FS,
205-
const InstrProfCorrelator *Correlator = nullptr);
205+
const InstrProfCorrelator *Correlator = nullptr,
206+
std::function<void(Error)> Warn = nullptr);
206207

207208
static Expected<std::unique_ptr<InstrProfReader>>
208209
create(std::unique_ptr<MemoryBuffer> Buffer,
209-
const InstrProfCorrelator *Correlator = nullptr);
210+
const InstrProfCorrelator *Correlator = nullptr,
211+
std::function<void(Error)> Warn = nullptr);
210212

211213
/// \param Weight for raw profiles use this as the temporal profile trace
212214
/// weight
@@ -344,12 +346,19 @@ class RawInstrProfReader : public InstrProfReader {
344346
/// Start address of binary id length and data pairs.
345347
const uint8_t *BinaryIdsStart;
346348

349+
std::function<void(Error)> Warn;
350+
351+
/// Maxium counter value 2^56.
352+
static const uint64_t MaxCounterValue = (1ULL << 56);
353+
347354
public:
348355
RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer,
349-
const InstrProfCorrelator *Correlator)
356+
const InstrProfCorrelator *Correlator,
357+
std::function<void(Error)> Warn)
350358
: DataBuffer(std::move(DataBuffer)),
351359
Correlator(dyn_cast_or_null<const InstrProfCorrelatorImpl<IntPtrT>>(
352-
Correlator)) {}
360+
Correlator)),
361+
Warn(Warn) {}
353362
RawInstrProfReader(const RawInstrProfReader &) = delete;
354363
RawInstrProfReader &operator=(const RawInstrProfReader &) = delete;
355364

‎llvm/lib/ProfileData/InstrProf.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,9 @@ static std::string getInstrProfErrString(instrprof_error Err,
161161
case instrprof_error::raw_profile_version_mismatch:
162162
OS << "raw profile version mismatch";
163163
break;
164+
case instrprof_error::counter_value_too_large:
165+
OS << "excessively large counter value suggests corrupted profile data";
166+
break;
164167
}
165168

166169
// If optional error message is not empty, append it to the message.

‎llvm/lib/ProfileData/InstrProfReader.cpp

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -167,17 +167,20 @@ static Error printBinaryIdsInternal(raw_ostream &OS,
167167

168168
Expected<std::unique_ptr<InstrProfReader>>
169169
InstrProfReader::create(const Twine &Path, vfs::FileSystem &FS,
170-
const InstrProfCorrelator *Correlator) {
170+
const InstrProfCorrelator *Correlator,
171+
std::function<void(Error)> Warn) {
171172
// Set up the buffer to read.
172173
auto BufferOrError = setupMemoryBuffer(Path, FS);
173174
if (Error E = BufferOrError.takeError())
174175
return std::move(E);
175-
return InstrProfReader::create(std::move(BufferOrError.get()), Correlator);
176+
return InstrProfReader::create(std::move(BufferOrError.get()), Correlator,
177+
Warn);
176178
}
177179

178180
Expected<std::unique_ptr<InstrProfReader>>
179181
InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
180-
const InstrProfCorrelator *Correlator) {
182+
const InstrProfCorrelator *Correlator,
183+
std::function<void(Error)> Warn) {
181184
if (Buffer->getBufferSize() == 0)
182185
return make_error<InstrProfError>(instrprof_error::empty_raw_profile);
183186

@@ -186,9 +189,9 @@ InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
186189
if (IndexedInstrProfReader::hasFormat(*Buffer))
187190
Result.reset(new IndexedInstrProfReader(std::move(Buffer)));
188191
else if (RawInstrProfReader64::hasFormat(*Buffer))
189-
Result.reset(new RawInstrProfReader64(std::move(Buffer), Correlator));
192+
Result.reset(new RawInstrProfReader64(std::move(Buffer), Correlator, Warn));
190193
else if (RawInstrProfReader32::hasFormat(*Buffer))
191-
Result.reset(new RawInstrProfReader32(std::move(Buffer), Correlator));
194+
Result.reset(new RawInstrProfReader32(std::move(Buffer), Correlator, Warn));
192195
else if (TextInstrProfReader::hasFormat(*Buffer))
193196
Result.reset(new TextInstrProfReader(std::move(Buffer)));
194197
else
@@ -706,8 +709,12 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(
706709
// A value of zero signifies the block is covered.
707710
Record.Counts.push_back(*Ptr == 0 ? 1 : 0);
708711
} else {
709-
const auto *CounterValue = reinterpret_cast<const uint64_t *>(Ptr);
710-
Record.Counts.push_back(swap(*CounterValue));
712+
uint64_t CounterValue = swap(*reinterpret_cast<const uint64_t *>(Ptr));
713+
if (CounterValue > MaxCounterValue && Warn)
714+
Warn(make_error<InstrProfError>(
715+
instrprof_error::counter_value_too_large, Twine(CounterValue)));
716+
717+
Record.Counts.push_back(CounterValue);
711718
}
712719
}
713720

‎llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
2121
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
2222
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
2323
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
24-
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
2524
RUN: printf '\10\0\0\0\0\0\0\0' >> %t.profraw
2625
RUN: printf '\0\0\4\0\1\0\0\0' >> %t.profraw
2726
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
@@ -42,12 +41,45 @@ RUN: printf '\0\0\4\0\1\0\0\0' >> %t.profraw
4241
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
4342
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
4443
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
44+
45+
// Make two copies for another test.
46+
RUN: cp %t.profraw %t-bad.profraw
47+
RUN: cp %t.profraw %t-good.profraw
48+
4549
// Make NumCounters = 0 so that we get "number of counters is zero" error message
4650
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
4751
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
4852

4953
RUN: printf '\023\0\0\0\0\0\0\0' >> %t.profraw
5054
RUN: printf '\3\0foo\0\0\0' >> %t.profraw
5155

52-
RUN: not llvm-profdata show %t.profraw 2>&1 | FileCheck %s
53-
CHECK: malformed instrumentation profile data: number of counters is zero
56+
RUN: not llvm-profdata show %t.profraw 2>&1 | FileCheck %s --check-prefix=ZERO
57+
ZERO: malformed instrumentation profile data: number of counters is zero
58+
59+
// Test a counter value greater than 2^56.
60+
RUN: printf '\1\0\0\0\0\0\0\0' >> %t-bad.profraw
61+
RUN: printf '\0\0\0\0\0\0\0\0' >> %t-bad.profraw
62+
// Counter value is 72057594037927937
63+
RUN: printf '\1\0\0\0\0\0\0\1' >> %t-bad.profraw
64+
RUN: printf '\3\0foo\0\0\0' >> %t-bad.profraw
65+
66+
RUN: printf '\1\0\0\0\0\0\0\0' >> %t-good.profraw
67+
RUN: printf '\0\0\0\0\0\0\0\0' >> %t-good.profraw
68+
// Counter value is 72057594037927937
69+
RUN: printf '\1\0\0\0\0\0\0\0' >> %t-good.profraw
70+
RUN: printf '\3\0foo\0\0\0' >> %t-good.profraw
71+
72+
// llvm-profdata fails if there is a warning for any input file under default failure mode (any).
73+
RUN: not llvm-profdata merge %t-bad.profraw %t-good.profraw -o %t.profdata 2>&1 | FileCheck %s --check-prefix=ANY
74+
ANY: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
75+
76+
// -failure-mode=all only fails if there is a warning for every input file.
77+
RUN: not llvm-profdata merge %t-bad.profraw -failure-mode=all -o %t.profdata 2>&1 | FileCheck %s --check-prefix=ALL-ERR
78+
ALL-ERR: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
79+
80+
RUN: llvm-profdata merge %t-bad.profraw %t-good.profraw -failure-mode=all -o %t.profdata 2>&1 | FileCheck %s --check-prefix=ALL-WARN
81+
ALL-WARN: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937
82+
83+
// -failure-mode=warn does not fail at all. It only prints warnings.
84+
RUN: llvm-profdata merge %t-bad.profraw -failure-mode=warn -o %t.profdata 2>&1 | FileCheck %s --check-prefix=WARN
85+
WARN: {{.*}} excessively large counter value suggests corrupted profile data: 72057594037927937

‎llvm/tools/llvm-profdata/llvm-profdata.cpp

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ static void exitWithErrorCode(std::error_code EC, StringRef Whence = "") {
114114

115115
namespace {
116116
enum ProfileKinds { instr, sample, memory };
117-
enum FailureMode { failIfAnyAreInvalid, failIfAllAreInvalid };
118-
}
117+
enum FailureMode { warnOnly, failIfAnyAreInvalid, failIfAllAreInvalid };
118+
} // namespace
119119

120120
static void warnOrExitGivenError(FailureMode FailMode, std::error_code EC,
121121
StringRef Whence = "") {
@@ -314,7 +314,22 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
314314
}
315315

316316
auto FS = vfs::getRealFileSystem();
317-
auto ReaderOrErr = InstrProfReader::create(Input.Filename, *FS, Correlator);
317+
// TODO: This only saves the first non-fatal error from InstrProfReader, and
318+
// then added to WriterContext::Errors. However, this is not extensible, if
319+
// we have more non-fatal errors from InstrProfReader in the future. How
320+
// should this interact with different -failure-mode?
321+
std::optional<std::pair<Error, std::string>> ReaderWarning;
322+
auto Warn = [&](Error E) {
323+
if (ReaderWarning) {
324+
consumeError(std::move(E));
325+
return;
326+
}
327+
// Only show the first time an error occurs in this file.
328+
auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
329+
ReaderWarning = {make_error<InstrProfError>(ErrCode, Msg), Filename};
330+
};
331+
auto ReaderOrErr =
332+
InstrProfReader::create(Input.Filename, *FS, Correlator, Warn);
318333
if (Error E = ReaderOrErr.takeError()) {
319334
// Skip the empty profiles by returning silently.
320335
auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
@@ -362,14 +377,23 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
362377
Traces, Reader->getTemporalProfTraceStreamSize());
363378
}
364379
if (Reader->hasError()) {
365-
if (Error E = Reader->getError())
380+
if (Error E = Reader->getError()) {
366381
WC->Errors.emplace_back(std::move(E), Filename);
382+
return;
383+
}
367384
}
368385

369386
std::vector<llvm::object::BuildID> BinaryIds;
370-
if (Error E = Reader->readBinaryIds(BinaryIds))
387+
if (Error E = Reader->readBinaryIds(BinaryIds)) {
371388
WC->Errors.emplace_back(std::move(E), Filename);
389+
return;
390+
}
372391
WC->Writer.addBinaryIds(BinaryIds);
392+
393+
if (ReaderWarning) {
394+
WC->Errors.emplace_back(std::move(ReaderWarning->first),
395+
ReaderWarning->second);
396+
}
373397
}
374398

375399
/// Merge the \p Src writer context into \p Dst.
@@ -492,7 +516,7 @@ mergeInstrProfile(const WeightedFileVector &Inputs, StringRef DebugInfoFilename,
492516
warn(toString(std::move(ErrorPair.first)), ErrorPair.second);
493517
}
494518
}
495-
if (NumErrors == Inputs.size() ||
519+
if ((NumErrors == Inputs.size() && FailMode == failIfAllAreInvalid) ||
496520
(NumErrors > 0 && FailMode == failIfAnyAreInvalid))
497521
exitWithError("no profile can be merged");
498522

@@ -1202,10 +1226,12 @@ static int merge_main(int argc, const char *argv[]) {
12021226
"GCC encoding (only meaningful for -sample)")));
12031227
cl::opt<FailureMode> FailureMode(
12041228
"failure-mode", cl::init(failIfAnyAreInvalid), cl::desc("Failure mode:"),
1205-
cl::values(clEnumValN(failIfAnyAreInvalid, "any",
1206-
"Fail if any profile is invalid."),
1207-
clEnumValN(failIfAllAreInvalid, "all",
1208-
"Fail only if all profiles are invalid.")));
1229+
cl::values(
1230+
clEnumValN(warnOnly, "warn", "Do not fail and just print warnings."),
1231+
clEnumValN(failIfAnyAreInvalid, "any",
1232+
"Fail if any profile is invalid."),
1233+
clEnumValN(failIfAllAreInvalid, "all",
1234+
"Fail only if all profiles are invalid.")));
12091235
cl::opt<bool> OutputSparse("sparse", cl::init(false),
12101236
cl::desc("Generate a sparse profile (only meaningful for -instr)"));
12111237
cl::opt<unsigned> NumThreads(

0 commit comments

Comments
 (0)