[clang] [llvm] [clang-tools-extra] [llvm-profdata] Emit warning when counter value is greater than 2^56. (PR #69513)
Zequan Wu via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 31 13:40:32 PDT 2023
https://github.com/ZequanWu updated https://github.com/llvm/llvm-project/pull/69513
>From 9a1af6e1d47ab622979796f2319edec8a9c77928 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Wed, 18 Oct 2023 16:28:30 -0400
Subject: [PATCH 1/7] [llvm-profdata] Emit error when counter value is greater
than 2^56.
---
llvm/lib/ProfileData/InstrProfReader.cpp | 13 +++++++++++--
.../malformed-num-counters-zero.test | 17 +++++++++++++++--
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index a920a31d0a4b229..d62a816cdeed4e6 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -38,6 +38,9 @@
using namespace llvm;
+// Maxium counter value 2^56.
+static uint64_t MaxCounterValue = 0xffffffffffffff;
+
// Extracts the variant information from the top 32 bits in the version and
// returns an enum specifying the variants present.
static InstrProfKind getProfileKindFromVersion(uint64_t Version) {
@@ -676,8 +679,14 @@ 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)
+ return error(instrprof_error::malformed,
+ ("counter value " + Twine(CounterValue) +
+ " is greater than " + Twine(MaxCounterValue))
+ .str());
+
+ 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 b718cf0fd8e9723..011c1cbf73af3bb 100644
--- a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
+++ b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
@@ -35,11 +35,24 @@ RUN: printf '\1\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
RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+
+// Make a copy for another test.
+RUN: cp %t.profraw %t1.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 '\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' >> %t1.profraw
+
+RUN: printf '\0\0\0\0\0\0\0\1' >> %t1.profraw
+RUN: printf '\3\0foo\0\0\0' >> %t1.profraw
+
+RUN: not llvm-profdata show %t1.profraw 2>&1 | FileCheck %s --check-prefix=MAX
+MAX: malformed instrumentation profile data: counter value 72057594037927936 is greater than 72057594037927935
>From 5d4c2ce9b8f5c49041bcbdf12f7890f75e4754c7 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Thu, 19 Oct 2023 15:41:38 -0400
Subject: [PATCH 2/7] address comments
---
llvm/include/llvm/ProfileData/InstrProfReader.h | 3 +++
llvm/lib/ProfileData/InstrProfReader.cpp | 10 +++-------
.../llvm-profdata/malformed-num-counters-zero.test | 6 +++---
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index 5f54cbeb1b01eda..e62ee42d09f4145 100644
--- a/llvm/include/llvm/ProfileData/InstrProfReader.h
+++ b/llvm/include/llvm/ProfileData/InstrProfReader.h
@@ -341,6 +341,9 @@ class RawInstrProfReader : public InstrProfReader {
/// Start address of binary id length and data pairs.
const uint8_t *BinaryIdsStart;
+ // Maxium counter value 2^56.
+ static const uint64_t MaxCounterValue = (1ULL << 56);
+
public:
RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer,
const InstrProfCorrelator *Correlator)
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index d62a816cdeed4e6..3a1d5ef3a9d827e 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -27,6 +27,7 @@
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/SwapByteOrder.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/WithColor.h"
#include <algorithm>
#include <cstddef>
#include <cstdint>
@@ -38,9 +39,6 @@
using namespace llvm;
-// Maxium counter value 2^56.
-static uint64_t MaxCounterValue = 0xffffffffffffff;
-
// Extracts the variant information from the top 32 bits in the version and
// returns an enum specifying the variants present.
static InstrProfKind getProfileKindFromVersion(uint64_t Version) {
@@ -681,10 +679,8 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(
} else {
uint64_t CounterValue = swap(*reinterpret_cast<const uint64_t *>(Ptr));
if (CounterValue > MaxCounterValue)
- return error(instrprof_error::malformed,
- ("counter value " + Twine(CounterValue) +
- " is greater than " + Twine(MaxCounterValue))
- .str());
+ WithColor::warning() << "counter value " + Twine(CounterValue) +
+ " suggests corrupted profile data";
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 011c1cbf73af3bb..a5f05095d6d52de 100644
--- a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
+++ b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
@@ -51,8 +51,8 @@ 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' >> %t1.profraw
-RUN: printf '\0\0\0\0\0\0\0\1' >> %t1.profraw
+RUN: printf '\1\0\0\0\0\0\0\1' >> %t1.profraw
RUN: printf '\3\0foo\0\0\0' >> %t1.profraw
-RUN: not llvm-profdata show %t1.profraw 2>&1 | FileCheck %s --check-prefix=MAX
-MAX: malformed instrumentation profile data: counter value 72057594037927936 is greater than 72057594037927935
+RUN: llvm-profdata show %t1.profraw 2>&1 | FileCheck %s --check-prefix=MAX
+MAX: warning: counter value 72057594037927937 suggests corrupted profile data
>From fb2339c8764bcf515fbd749d50b4992381a65016 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Fri, 20 Oct 2023 14:33:03 -0400
Subject: [PATCH 3/7] Use callback function to print warning for the first
time.
---
.../llvm/ProfileData/InstrProfReader.h | 16 +++++++----
llvm/lib/ProfileData/InstrProfReader.cpp | 28 +++++++++++--------
.../malformed-num-counters-zero.test | 4 +--
llvm/tools/llvm-profdata/llvm-profdata.cpp | 14 +++++++++-
4 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index e62ee42d09f4145..fbda29d5217d744 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::optional<std::function<void(Error)>> MaybeWarnFn = {});
static Expected<std::unique_ptr<InstrProfReader>>
create(std::unique_ptr<MemoryBuffer> Buffer,
- const InstrProfCorrelator *Correlator = nullptr);
+ const InstrProfCorrelator *Correlator = nullptr,
+ std::optional<std::function<void(Error)>> MaybeWarnFn = {});
/// \param Weight for raw profiles use this as the temporal profile trace
/// weight
@@ -341,15 +343,19 @@ class RawInstrProfReader : public InstrProfReader {
/// Start address of binary id length and data pairs.
const uint8_t *BinaryIdsStart;
- // Maxium counter value 2^56.
+ std::optional<std::function<void(Error)>> MaybeWarnFn;
+
+ /// 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::optional<std::function<void(Error)>> MaybeWarnFn)
: DataBuffer(std::move(DataBuffer)),
Correlator(dyn_cast_or_null<const InstrProfCorrelatorImpl<IntPtrT>>(
- Correlator)) {}
+ Correlator)),
+ MaybeWarnFn(MaybeWarnFn) {}
RawInstrProfReader(const RawInstrProfReader &) = delete;
RawInstrProfReader &operator=(const RawInstrProfReader &) = delete;
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index 3a1d5ef3a9d827e..286d7a877f673cb 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -27,7 +27,6 @@
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/SwapByteOrder.h"
#include "llvm/Support/VirtualFileSystem.h"
-#include "llvm/Support/WithColor.h"
#include <algorithm>
#include <cstddef>
#include <cstdint>
@@ -166,19 +165,22 @@ static Error printBinaryIdsInternal(raw_ostream &OS,
return Error::success();
}
-Expected<std::unique_ptr<InstrProfReader>>
-InstrProfReader::create(const Twine &Path, vfs::FileSystem &FS,
- const InstrProfCorrelator *Correlator) {
+Expected<std::unique_ptr<InstrProfReader>> InstrProfReader::create(
+ const Twine &Path, vfs::FileSystem &FS,
+ const InstrProfCorrelator *Correlator,
+ std::optional<std::function<void(Error)>> MaybeWarnFn) {
// 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,
+ MaybeWarnFn);
}
Expected<std::unique_ptr<InstrProfReader>>
InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
- const InstrProfCorrelator *Correlator) {
+ const InstrProfCorrelator *Correlator,
+ std::optional<std::function<void(Error)>> MaybeWarnFn) {
if (Buffer->getBufferSize() == 0)
return make_error<InstrProfError>(instrprof_error::empty_raw_profile);
@@ -187,9 +189,11 @@ 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, MaybeWarnFn));
else if (RawInstrProfReader32::hasFormat(*Buffer))
- Result.reset(new RawInstrProfReader32(std::move(Buffer), Correlator));
+ Result.reset(
+ new RawInstrProfReader32(std::move(Buffer), Correlator, MaybeWarnFn));
else if (TextInstrProfReader::hasFormat(*Buffer))
Result.reset(new TextInstrProfReader(std::move(Buffer)));
else
@@ -678,9 +682,11 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(
Record.Counts.push_back(*Ptr == 0 ? 1 : 0);
} else {
uint64_t CounterValue = swap(*reinterpret_cast<const uint64_t *>(Ptr));
- if (CounterValue > MaxCounterValue)
- WithColor::warning() << "counter value " + Twine(CounterValue) +
- " suggests corrupted profile data";
+ if (CounterValue > MaxCounterValue && MaybeWarnFn)
+ (*MaybeWarnFn)(make_error<InstrProfError>(
+ instrprof_error::malformed,
+ "excessively large counter value " + Twine(CounterValue) +
+ " suggests corrupted profile data"));
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 a5f05095d6d52de..293e7c862b516c4 100644
--- a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
+++ b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
@@ -54,5 +54,5 @@ RUN: printf '\1\0\0\0\0\0\0\0' >> %t1.profraw
RUN: printf '\1\0\0\0\0\0\0\1' >> %t1.profraw
RUN: printf '\3\0foo\0\0\0' >> %t1.profraw
-RUN: llvm-profdata show %t1.profraw 2>&1 | FileCheck %s --check-prefix=MAX
-MAX: warning: counter value 72057594037927937 suggests corrupted profile data
+RUN: llvm-profdata merge %t1.profraw -o %t.profdata 2>&1 | FileCheck %s --check-prefix=MAX
+MAX: warning: excessively large counter value 72057594037927937 suggests corrupted profile data
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 7d665a8005b0d62..9a658cdf8356ad8 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -314,7 +314,19 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
}
auto FS = vfs::getRealFileSystem();
- auto ReaderOrErr = InstrProfReader::create(Input.Filename, *FS, Correlator);
+ bool Reported = false;
+ auto WarnFn = [&](Error E) {
+ if (Reported) {
+ consumeError(std::move(E));
+ return;
+ }
+ Reported = true;
+ // Only show hint the first time an error occurs.
+ auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
+ warn(Msg);
+ };
+ auto ReaderOrErr =
+ InstrProfReader::create(Input.Filename, *FS, Correlator, WarnFn);
if (Error E = ReaderOrErr.takeError()) {
// Skip the empty profiles by returning silently.
auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
>From 172fdb9a3a65236bae4fd442b34a092968c36e87 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Fri, 20 Oct 2023 14:46:58 -0400
Subject: [PATCH 4/7] format
---
llvm/lib/ProfileData/InstrProfReader.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index 286d7a877f673cb..943cc4ccd53ca55 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -165,10 +165,10 @@ static Error printBinaryIdsInternal(raw_ostream &OS,
return Error::success();
}
-Expected<std::unique_ptr<InstrProfReader>> InstrProfReader::create(
- const Twine &Path, vfs::FileSystem &FS,
- const InstrProfCorrelator *Correlator,
- std::optional<std::function<void(Error)>> MaybeWarnFn) {
+Expected<std::unique_ptr<InstrProfReader>>
+InstrProfReader::create(const Twine &Path, vfs::FileSystem &FS,
+ const InstrProfCorrelator *Correlator,
+ std::optional<std::function<void(Error)>> MaybeWarnFn) {
// Set up the buffer to read.
auto BufferOrError = setupMemoryBuffer(Path, FS);
if (Error E = BufferOrError.takeError())
>From 1d89e5f48e915f2b750f00487e03c09ee77b55bb Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Mon, 23 Oct 2023 15:12:42 -0400
Subject: [PATCH 5/7] Make large counter value warning a hard error by default.
Use -failure-mode=warn to treat it as a warning.
---
llvm/include/llvm/ProfileData/InstrProf.h | 3 +-
llvm/lib/ProfileData/InstrProf.cpp | 3 ++
llvm/lib/ProfileData/InstrProfReader.cpp | 6 ++--
.../malformed-num-counters-zero.test | 32 +++++++++++++----
llvm/tools/llvm-profdata/llvm-profdata.cpp | 36 +++++++++++++------
5 files changed, 57 insertions(+), 23 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 9239c1a691eca19..55d8fe2666d66b3 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -341,7 +341,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/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index ddc11304742df76..49c9b8c4191e299 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -158,6 +158,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 943cc4ccd53ca55..7d09caa18fbb053 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -683,10 +683,8 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(
} else {
uint64_t CounterValue = swap(*reinterpret_cast<const uint64_t *>(Ptr));
if (CounterValue > MaxCounterValue && MaybeWarnFn)
- (*MaybeWarnFn)(make_error<InstrProfError>(
- instrprof_error::malformed,
- "excessively large counter value " + Twine(CounterValue) +
- " suggests corrupted profile data"));
+ (*MaybeWarnFn)(make_error<InstrProfError>(instrprof_error::malformed,
+ 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 293e7c862b516c4..26c033f2475f51e 100644
--- a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
+++ b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
@@ -36,8 +36,9 @@ 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
-// Make a copy for another test.
-RUN: cp %t.profraw %t1.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
@@ -49,10 +50,27 @@ 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' >> %t1.profraw
+RUN: printf '\1\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\1' >> %t1.profraw
-RUN: printf '\3\0foo\0\0\0' >> %t1.profraw
+RUN: printf '\1\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
-RUN: llvm-profdata merge %t1.profraw -o %t.profdata 2>&1 | FileCheck %s --check-prefix=MAX
-MAX: warning: excessively large counter value 72057594037927937 suggests corrupted profile data
+// 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: {{.*}}malformed instrumentation 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: {{.*}}malformed instrumentation 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: {{.*}}malformed instrumentation 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: {{.*}}malformed instrumentation profile data: 72057594037927937
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 9a658cdf8356ad8..b437eee5ee71e82 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,16 +314,19 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
}
auto FS = vfs::getRealFileSystem();
- bool Reported = false;
+ // TODO: This only saves the first non-fatal error from InstrProfReader, and
+ // then added to WriterContext::Errors. However, this is not extensiable, 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 WarnFn = [&](Error E) {
- if (Reported) {
+ if (ReaderWarning) {
consumeError(std::move(E));
return;
}
- Reported = true;
- // Only show hint the first time an error occurs.
+ // Only show the first time an error occurs in this file.
auto [ErrCode, Msg] = InstrProfError::take(std::move(E));
- warn(Msg);
+ ReaderWarning = {make_error<InstrProfError>(ErrCode, Msg), Filename};
};
auto ReaderOrErr =
InstrProfReader::create(Input.Filename, *FS, Correlator, WarnFn);
@@ -374,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.
@@ -504,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");
@@ -1214,7 +1226,9 @@ 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",
+ 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.")));
>From 9433fb902421c34d3adbc91d81df906dd4ed5904 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Tue, 24 Oct 2023 12:27:26 -0400
Subject: [PATCH 6/7] Remove std::optional.
---
.../include/llvm/ProfileData/InstrProfReader.h | 10 +++++-----
llvm/lib/ProfileData/InstrProfReader.cpp | 18 ++++++++----------
.../malformed-num-counters-zero.test | 8 ++++----
llvm/tools/llvm-profdata/llvm-profdata.cpp | 16 ++++++++--------
4 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index fbda29d5217d744..32127b0bd6d7b70 100644
--- a/llvm/include/llvm/ProfileData/InstrProfReader.h
+++ b/llvm/include/llvm/ProfileData/InstrProfReader.h
@@ -203,12 +203,12 @@ class InstrProfReader {
static Expected<std::unique_ptr<InstrProfReader>>
create(const Twine &Path, vfs::FileSystem &FS,
const InstrProfCorrelator *Correlator = nullptr,
- std::optional<std::function<void(Error)>> MaybeWarnFn = {});
+ std::function<void(Error)> Warn = nullptr);
static Expected<std::unique_ptr<InstrProfReader>>
create(std::unique_ptr<MemoryBuffer> Buffer,
const InstrProfCorrelator *Correlator = nullptr,
- std::optional<std::function<void(Error)>> MaybeWarnFn = {});
+ std::function<void(Error)> Warn = nullptr);
/// \param Weight for raw profiles use this as the temporal profile trace
/// weight
@@ -343,7 +343,7 @@ class RawInstrProfReader : public InstrProfReader {
/// Start address of binary id length and data pairs.
const uint8_t *BinaryIdsStart;
- std::optional<std::function<void(Error)>> MaybeWarnFn;
+ std::function<void(Error)> Warn;
/// Maxium counter value 2^56.
static const uint64_t MaxCounterValue = (1ULL << 56);
@@ -351,11 +351,11 @@ class RawInstrProfReader : public InstrProfReader {
public:
RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer,
const InstrProfCorrelator *Correlator,
- std::optional<std::function<void(Error)>> MaybeWarnFn)
+ std::function<void(Error)> Warn)
: DataBuffer(std::move(DataBuffer)),
Correlator(dyn_cast_or_null<const InstrProfCorrelatorImpl<IntPtrT>>(
Correlator)),
- MaybeWarnFn(MaybeWarnFn) {}
+ Warn(Warn) {}
RawInstrProfReader(const RawInstrProfReader &) = delete;
RawInstrProfReader &operator=(const RawInstrProfReader &) = delete;
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index 7d09caa18fbb053..c171fc86086ddfe 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -168,19 +168,19 @@ static Error printBinaryIdsInternal(raw_ostream &OS,
Expected<std::unique_ptr<InstrProfReader>>
InstrProfReader::create(const Twine &Path, vfs::FileSystem &FS,
const InstrProfCorrelator *Correlator,
- std::optional<std::function<void(Error)>> MaybeWarnFn) {
+ 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,
- MaybeWarnFn);
+ Warn);
}
Expected<std::unique_ptr<InstrProfReader>>
InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
const InstrProfCorrelator *Correlator,
- std::optional<std::function<void(Error)>> MaybeWarnFn) {
+ std::function<void(Error)> Warn) {
if (Buffer->getBufferSize() == 0)
return make_error<InstrProfError>(instrprof_error::empty_raw_profile);
@@ -189,11 +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, MaybeWarnFn));
+ Result.reset(new RawInstrProfReader64(std::move(Buffer), Correlator, Warn));
else if (RawInstrProfReader32::hasFormat(*Buffer))
- Result.reset(
- new RawInstrProfReader32(std::move(Buffer), Correlator, MaybeWarnFn));
+ Result.reset(new RawInstrProfReader32(std::move(Buffer), Correlator, Warn));
else if (TextInstrProfReader::hasFormat(*Buffer))
Result.reset(new TextInstrProfReader(std::move(Buffer)));
else
@@ -682,9 +680,9 @@ Error RawInstrProfReader<IntPtrT>::readRawCounts(
Record.Counts.push_back(*Ptr == 0 ? 1 : 0);
} else {
uint64_t CounterValue = swap(*reinterpret_cast<const uint64_t *>(Ptr));
- if (CounterValue > MaxCounterValue && MaybeWarnFn)
- (*MaybeWarnFn)(make_error<InstrProfError>(instrprof_error::malformed,
- Twine(CounterValue)));
+ 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 26c033f2475f51e..29df5fdea513656 100644
--- a/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
+++ b/llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test
@@ -62,15 +62,15 @@ 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: {{.*}}malformed instrumentation profile data: 72057594037927937
+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: {{.*}}malformed instrumentation profile data: 72057594037927937
+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: {{.*}}malformed instrumentation profile data: 72057594037927937
+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: {{.*}}malformed instrumentation profile data: 72057594037927937
+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 b437eee5ee71e82..de7381bdc7c593e 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -319,7 +319,7 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
// 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 WarnFn = [&](Error E) {
+ auto Warn = [&](Error E) {
if (ReaderWarning) {
consumeError(std::move(E));
return;
@@ -329,7 +329,7 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
ReaderWarning = {make_error<InstrProfError>(ErrCode, Msg), Filename};
};
auto ReaderOrErr =
- InstrProfReader::create(Input.Filename, *FS, Correlator, WarnFn);
+ 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));
@@ -1226,12 +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(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::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(
>From ebe3c38f7420f5638c1462c1a3f3529369218656 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Tue, 31 Oct 2023 16:40:19 -0400
Subject: [PATCH 7/7] Fix typo
---
llvm/tools/llvm-profdata/llvm-profdata.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index de7381bdc7c593e..84290ac609a6acd 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -315,7 +315,7 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
auto FS = vfs::getRealFileSystem();
// TODO: This only saves the first non-fatal error from InstrProfReader, and
- // then added to WriterContext::Errors. However, this is not extensiable, if
+ // 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;
More information about the cfe-commits
mailing list