[profdata] Consume reader error if returned early

[ellishg]

#69513 added logic to emit an error when something goes wrong in readRawCounts() by passing a Warn() lambda to capture errors. If we capture an error in this way and we also early return due to some other error (say from readBinaryIds()) then we will hit an assert due to an unconsumed error. This is because we don't consume the error at the end of this function.

llvm-project/llvm/tools/llvm-profdata/llvm-profdata.cpp

Lines 864 to 867 in 1c7ae89

	if (ReaderWarning) {
	WC->Errors.emplace_back(std::move(ReaderWarning->first),
	ReaderWarning->second);
	}

Fix this assert by calling consumeError() via llvm::make_scope_exit() when the function returns for any reason.

We might be able to simplify this by directly adding the error to WC->Errors in Warn(), but that might change the order of the errors reported and I am less confident in that change.

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Ellis Hoag (ellishg)

Changes

#69513 added logic to emit an error when something goes wrong in readRawCounts() by passing a Warn() lambda to capture errors. If we capture an error in this way and we also early return due to some other error (say from readBinaryIds()) then we will hit an assert due to an unconsumed error. This is because we don't consume the error at the end of this function.

llvm-project/llvm/tools/llvm-profdata/llvm-profdata.cpp

Lines 864 to 867 in 1c7ae89

	if (ReaderWarning) {
	WC->Errors.emplace_back(std::move(ReaderWarning->first),
	ReaderWarning->second);
	}

Fix this assert by calling consumeError() via llvm::make_scope_exit() when the function returns for any reason.

We might be able to simplify this by directly adding the error to WC->Errors in Warn(), but that might change the order of the errors reported and I am less confident in that change.

---

Full diff: https://github.com/llvm/llvm-project/pull/163671.diff

1 Files Affected:

• (modified) llvm/tools/llvm-profdata/llvm-profdata.cpp (+7)

diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 15ddb05f953ee..585a39de9985f 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -778,6 +779,12 @@ 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 ReaderWarningScope = llvm::make_scope_exit([&] {
+    // If we hit a different error we may still have an error in ReaderWarning.
+    // Consume it now to avoid an assert
+    if (ReaderWarning)
+      consumeError(std::move(ReaderWarning->first));
+  });
   auto Warn = [&](Error E) {
     if (ReaderWarning) {
       consumeError(std::move(E));

[david-xl]

is it possible to add a small test?

[ellishg]

is it possible to add a small test?

@david-xl we only hit an assert if we early return in one of these places

llvm-project/llvm/tools/llvm-profdata/llvm-profdata.cpp

Lines 857 to 868 in 68fa9c4

	if (Reader->hasError()) {
	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)) {
	WC->Errors.emplace_back(std::move(E), Filename);
	return;
	}

The problem is it is difficult to reproduce these errors in .profraw files. The tests malformed-num-counters-zero.test and malformed-ptr-to-counter-array.test have to manually create .profraw files and corrupt them somehow, which involve parsing the byte representation of these files. Ideally we would have some textual representation of .profraw files, similar to .proftext files. For now, it is pretty difficult to write a test for this.