[llvm-profdata] Add mode to recover from profile read failures

[vsk]

Summary

Add a mode in which profile read errors are not immediately treated as
fatal. In this mode, merging makes forward progress and reports failure
only if no inputs can be read.

[vsk]

• Fix a comment.

[vsk]

• Don't record a read failure when a soft error is encountered within mergeRecordsFromWriter.

[Dor1s]

That's a great change, Vedant!! Thank you! //cc @sajjadm @liaoyuke FYI

Might make sense to clarify in the description that this is for merge only, if I understand correctly :)

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

279 ↗

(On Diff #218007)

How bad will it be if we decide to always print the warning here, not just for the first time?

[davidxl]

This is a behavior change and I think it should be guarded with an option. The default of the option should be set to 'FailOnAnyWhenMerge' instead of 'FailOnAllWhenMerge'.

[vsk]

Need to add an option to preserve the old error reporting behavior. I'll get back to this on Tuesday.

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

279 ↗

(On Diff #218007)

I agree that reporting of soft errors during merging is not as helpful as it should be. We should come up with a better solution. Istm that warning on all soft errors here could be very annoying (e.g. if there are lots of counter overflows).

[vsk]

@davidxl Are you recommending that the old behavior (fail the merge if any profiles are malformed) should remain the default? I think that's reasonable (it raises the alarm about toolchain bugs), just wanted to double-check.

[davidxl]

yes, the old behavior should be the default. It raises the awareness of the problem (and merge errors are usually rare in reality).

[vsk]

In D66985#1655761, @davidxl wrote:

yes, the old behavior should be the default. It raises the awareness of the problem (and merge errors are usually rare in reality).

Right, that's a good point. I wonder if it's worth introducing a separate mode, then, if it's not the recommended way to run the tool in production? Perhaps this should be set aside.

[davidxl]

A separate mode is fine. Buffering the errors and reporting them later is also useful (allowing more context info to be emitted).

[vsk]

• Allow users to pick the failure mode for profile merging.

[davidxl]

llvm/docs/CommandGuide/llvm-profdata.rst

127 ↗

(On Diff #218512)

The enums can be named with descriptive strings, but can the command line be simplified? --failure-mode=any|all

[vsk]

• Shorten failure mode names.

[davidxl]

lgtm

llvm/trunk/docs/CommandGuide/llvm-profdata.rst

  conjunction with -instr. Defaults to false, since it can inhibit compiler
  optimization during PGO.
 
 .. option:: -num-threads=N, -j=N
 
  Use N threads to perform profile merging. When N=0, llvm-profdata auto-detects
  an appropriate number of threads to use. This is the default.
 
+.. option:: -failure-mode=[any|all]
+
+ Set the failure mode. There are two options: 'any' causes the merge command to
+ fail if any profiles are invalid, and 'all' causes the merge command to fail
+ only if all profiles are invalid. If 'all' is set, information from any
+ invalid profiles is excluded from the final merged product. The default
+ failure mode is 'any'.
+
 EXAMPLES
 ^^^^^^^^
 Basic Usage
 +++++++++++
 Merge three profiles:
 
 ::
 

llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test

 RUN: echo "2"                                            >> %t.input
 RUN: echo "2"                                            >> %t.input
 RUN: echo "f1:10"                                        >> %t.input
 RUN: echo "f2:0"                                         >> %t.input
 RUN: echo "1"                                            >> %t.input
 RUN: echo ":10"                                          >> %t.input
 
 RUN: not llvm-profdata merge %t.input -text -output=/dev/null 2>&1  | FileCheck %s --check-prefix=BROKEN
-BROKEN: Malformed instrumentation profile data
+BROKEN: warning: {{.*}}invalid-profdata.test.tmp.input: Malformed instrumentation profile data
+BROKEN-NEXT: error: No profiles could be merged.
 
 RUN: echo ":ir"                     > %t.input
 RUN: echo "_ZN6Thread5StartEv"      >> %t.input
 RUN: echo "# Func Hash:"            >> %t.input
 RUN: echo "288793635542036872"      >> %t.input
 RUN: echo "# Num Counters:"         >> %t.input
 RUN: echo "3"                       >> %t.input
 RUN: echo "# Counter Values:"       >> %t.input

llvm/trunk/test/tools/llvm-profdata/text-format-errors.test

 Tests for instrumentation profile bad encoding.
 
 1- Detect invalid count
-RUN: not llvm-profdata show %p/Inputs/invalid-count-later.proftext 2>&1 | FileCheck %s --check-prefix=INVALID-COUNT-LATER
-RUN: not llvm-profdata merge %p/Inputs/invalid-count-later.proftext %p/Inputs/invalid-count-later.proftext -o %t.out 2>&1 | FileCheck %s --check-prefix=INVALID-COUNT-LATER
-INVALID-COUNT-LATER: error: {{.*}}invalid-count-later.proftext: Malformed instrumentation profile data
+RUN: not llvm-profdata show %p/Inputs/invalid-count-later.proftext 2>&1 | FileCheck %s --check-prefix=INVALID-COUNT-LATER-SHOW
+INVALID-COUNT-LATER-SHOW: error: {{.*}}invalid-count-later.proftext: Malformed instrumentation profile data
+
+RUN: not llvm-profdata merge %p/Inputs/invalid-count-later.proftext %p/Inputs/invalid-count-later.proftext -o %t.out 2>&1 | FileCheck %s --check-prefix=INVALID-COUNT-LATER-MERGE
+RUN: not llvm-profdata merge -failure-mode=all %p/Inputs/invalid-count-later.proftext %p/Inputs/invalid-count-later.proftext -o %t.out 2>&1 | FileCheck %s --check-prefix=INVALID-COUNT-LATER-MERGE
+INVALID-COUNT-LATER-MERGE: warning: {{.*}}invalid-count-later.proftext: Malformed instrumentation profile data
+INVALID-COUNT-LATER-MERGE-NEXT: warning: {{.*}}invalid-count-later.proftext: Malformed instrumentation profile data
+INVALID-COUNT-LATER-MERGE-NEXT: error: No profiles could be merged.
 
 2- Detect bad hash
-RUN: not llvm-profdata show %p/Inputs/bad-hash.proftext 2>&1 | FileCheck %s --check-prefix=BAD-HASH
-RUN: not llvm-profdata merge %p/Inputs/bad-hash.proftext %p/Inputs/bad-hash.proftext -o %t.out 2>&1 | FileCheck %s --check-prefix=BAD-HASH
-BAD-HASH: error: {{.*}}bad-hash.proftext: Malformed instrumentation profile data
+RUN: not llvm-profdata show %p/Inputs/bad-hash.proftext 2>&1 | FileCheck %s --check-prefix=BAD-HASH-SHOW
+BAD-HASH-SHOW: error: {{.*}}bad-hash.proftext: Malformed instrumentation profile data
+
+RUN: not llvm-profdata merge %p/Inputs/bad-hash.proftext %p/Inputs/bad-hash.proftext -o %t.out 2>&1 | FileCheck %s --check-prefix=BAD-HASH-MERGE
+BAD-HASH-MERGE: warning: {{.*}}bad-hash.proftext: Malformed instrumentation profile data
+BAD-HASH-NEXT: error: No profiles could be merged.
 
 3- Detect no counts
 RUN: not llvm-profdata show %p/Inputs/no-counts.proftext 2>&1 | FileCheck %s --check-prefix=NO-COUNTS
 NO-COUNTS: error: {{.*}}no-counts.proftext: Malformed instrumentation profile data
 
 4- Detect binary input
 RUN: not llvm-profdata show %p/Inputs/text-format-errors.text.bin 2>&1 | FileCheck %s --check-prefix=BINARY
 BINARY: error: {{.+}}: Unrecognized instrumentation profile encoding format

llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp

 }
 
 static void exitWithErrorCode(std::error_code EC, StringRef Whence = "") {
   exitWithError(EC.message(), Whence);
 }
 
 namespace {
 enum ProfileKinds { instr, sample };
+enum FailureMode { failIfAnyAreInvalid, failIfAllAreInvalid };
+}
+
+static void warnOrExitGivenError(FailureMode FailMode, std::error_code EC,
+                                 StringRef Whence = "") {
+  if (FailMode == failIfAnyAreInvalid)
+    exitWithErrorCode(EC, Whence);
+  else
+    warn(EC.message(), Whence);
 }
 
 static void handleMergeWriterError(Error E, StringRef WhenceFile = "",
                                    StringRef WhenceFunction = "",
                                    bool ShowHint = true) {
   if (!WhenceFile.empty())
     errs() << WhenceFile << ": ";
   if (!WhenceFunction.empty())
struct WeightedFile {
   uint64_t Weight;
 };
 typedef SmallVector<WeightedFile, 5> WeightedFileVector;
 
 /// Keep track of merged data and reported errors.
 struct WriterContext {
   std::mutex Lock;
   InstrProfWriter Writer;
-  Error Err;
-  std::string ErrWhence;
+  std::vector<std::pair<Error, std::string>> Errors;
   std::mutex &ErrLock;
   SmallSet<instrprof_error, 4> &WriterErrorCodes;
 
   WriterContext(bool IsSparse, std::mutex &ErrLock,
                 SmallSet<instrprof_error, 4> &WriterErrorCodes)
-      : Lock(), Writer(IsSparse), Err(Error::success()), ErrWhence(""),
-        ErrLock(ErrLock), WriterErrorCodes(WriterErrorCodes) {}
+      : Lock(), Writer(IsSparse), Errors(), ErrLock(ErrLock),
+        WriterErrorCodes(WriterErrorCodes) {}
 };
 
-/// Determine whether an error is fatal for profile merging.
-static bool isFatalError(instrprof_error IPE) {
-  switch (IPE) {
-  default:
-    return true;
-  case instrprof_error::success:
-  case instrprof_error::eof:
-  case instrprof_error::unknown_function:
-  case instrprof_error::hash_mismatch:
-  case instrprof_error::count_mismatch:
-  case instrprof_error::counter_overflow:
-  case instrprof_error::value_site_count_mismatch:
-    return false;
-  }
-}
-
 /// Computer the overlap b/w profile BaseFilename and TestFileName,
 /// and store the program level result to Overlap.
 static void overlapInput(const std::string &BaseFilename,
                          const std::string &TestFilename, WriterContext *WC,
                          OverlapStats &Overlap,
                          const OverlapFuncFilters &FuncFilter,
                          raw_fd_ostream &OS, bool IsCS) {
   auto ReaderOrErr = InstrProfReader::create(TestFilename);
   if (Error E = ReaderOrErr.takeError()) {
     // Skip the empty profiles by returning sliently.
     instrprof_error IPE = InstrProfError::take(std::move(E));
     if (IPE != instrprof_error::empty_raw_profile)
-      WC->Err = make_error<InstrProfError>(IPE);
+      WC->Errors.emplace_back(make_error<InstrProfError>(IPE), TestFilename);
     return;
   }
 
   auto Reader = std::move(ReaderOrErr.get());
   for (auto &I : *Reader) {
     OverlapStats FuncOverlap(OverlapStats::FunctionLevel);
     FuncOverlap.setFuncInfo(I.Name, I.Hash);
 
     WC->Writer.overlapRecord(std::move(I), Overlap, FuncOverlap, FuncFilter);
     FuncOverlap.dump(OS);
   }
 }
 
 /// Load an input into a writer context.
 static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
                       WriterContext *WC) {
   std::unique_lock<std::mutex> CtxGuard{WC->Lock};
 
-  // If there's a pending hard error, don't do more work.
-  if (WC->Err)
-    return;
-
   // Copy the filename, because llvm::ThreadPool copied the input "const
   // WeightedFile &" by value, making a reference to the filename within it
   // invalid outside of this packaged task.
-  WC->ErrWhence = Input.Filename;
+  std::string Filename = Input.Filename;
 
   auto ReaderOrErr = InstrProfReader::create(Input.Filename);
   if (Error E = ReaderOrErr.takeError()) {
     // Skip the empty profiles by returning sliently.
     instrprof_error IPE = InstrProfError::take(std::move(E));
     if (IPE != instrprof_error::empty_raw_profile)
-      WC->Err = make_error<InstrProfError>(IPE);
+      WC->Errors.emplace_back(make_error<InstrProfError>(IPE), Filename);
     return;
   }
 
   auto Reader = std::move(ReaderOrErr.get());
   bool IsIRProfile = Reader->isIRLevelProfile();
   bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
   if (WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
-    WC->Err = make_error<StringError>(
+    WC->Errors.emplace_back(
+        make_error<StringError>(
             "Merge IR generated profile with Clang generated profile.",
-        std::error_code());
+            std::error_code()),
+        Filename);
     return;
   }
 
   for (auto &I : *Reader) {
     if (Remapper)
       I.Name = (*Remapper)(I.Name);
     const StringRef FuncName = I.Name;
     bool Reported = false;
     WC->Writer.addRecord(std::move(I), Input.Weight, [&](Error E) {
       if (Reported) {
         consumeError(std::move(E));
         return;
       }
       Reported = true;
       // Only show hint the first time an error occurs.
       instrprof_error IPE = InstrProfError::take(std::move(E));
       std::unique_lock<std::mutex> ErrGuard{WC->ErrLock};
       bool firstTime = WC->WriterErrorCodes.insert(IPE).second;
       handleMergeWriterError(make_error<InstrProfError>(IPE), Input.Filename,
                              FuncName, firstTime);
     });
   }
-  if (Reader->hasError()) {
-    if (Error E = Reader->getError()) {
-      instrprof_error IPE = InstrProfError::take(std::move(E));
-      if (isFatalError(IPE))
-        WC->Err = make_error<InstrProfError>(IPE);
-    }
-  }
+  if (Reader->hasError())
+    if (Error E = Reader->getError())
+      WC->Errors.emplace_back(std::move(E), Filename);
 }
 
 /// Merge the \p Src writer context into \p Dst.
 static void mergeWriterContexts(WriterContext *Dst, WriterContext *Src) {
-  // If we've already seen a hard error, continuing with the merge would
-  // clobber it.
-  if (Dst->Err || Src->Err)
-    return;
+  for (auto &ErrorPair : Src->Errors)
+    Dst->Errors.push_back(std::move(ErrorPair));
+  Src->Errors.clear();
 
-  bool Reported = false;
   Dst->Writer.mergeRecordsFromWriter(std::move(Src->Writer), [&](Error E) {
-    if (Reported) {
-      consumeError(std::move(E));
-      return;
-    }
-    Reported = true;
-    Dst->Err = std::move(E);
+    instrprof_error IPE = InstrProfError::take(std::move(E));
+    std::unique_lock<std::mutex> ErrGuard{Dst->ErrLock};
+    bool firstTime = Dst->WriterErrorCodes.insert(IPE).second;
+    if (firstTime)
+      warn(toString(make_error<InstrProfError>(IPE)));
   });
 }
 
 static void mergeInstrProfile(const WeightedFileVector &Inputs,
                               SymbolRemapper *Remapper,
                               StringRef OutputFilename,
                               ProfileFormat OutputFormat, bool OutputSparse,
-                              unsigned NumThreads) {
+                              unsigned NumThreads, FailureMode FailMode) {
   if (OutputFilename.compare("-") == 0)
     exitWithError("Cannot write indexed profdata format to stdout.");
 
   if (OutputFormat != PF_Binary && OutputFormat != PF_Compact_Binary &&
       OutputFormat != PF_Ext_Binary && OutputFormat != PF_Text)
     exitWithError("Unknown format is specified.");
 
   std::mutex ErrorLock;
do {
                    Contexts[End - 1].get());
         Pool.wait();
       }
       End = Mid;
       Mid /= 2;
     } while (Mid > 0);
   }
 
-  // Handle deferred hard errors encountered during merging.
+  // Handle deferred errors encountered during merging. If the number of errors
+  // is equal to the number of inputs the merge failed.
+  unsigned NumErrors = 0;
   for (std::unique_ptr<WriterContext> &WC : Contexts) {
-    if (!WC->Err)
-      continue;
-    if (!WC->Err.isA<InstrProfError>())
-      exitWithError(std::move(WC->Err), WC->ErrWhence);
-
-    instrprof_error IPE = InstrProfError::take(std::move(WC->Err));
-    if (isFatalError(IPE))
-      exitWithError(make_error<InstrProfError>(IPE), WC->ErrWhence);
-    else
-      warn(toString(make_error<InstrProfError>(IPE)),
-           WC->ErrWhence);
+    for (auto &ErrorPair : WC->Errors) {
+      ++NumErrors;
+      warn(toString(std::move(ErrorPair.first)), ErrorPair.second);
     }
+  }
+  if (NumErrors == Inputs.size() ||
+      (NumErrors > 0 && FailMode == failIfAnyAreInvalid))
+    exitWithError("No profiles could be merged.");
 
   std::error_code EC;
   raw_fd_ostream Output(OutputFilename.data(), EC, sys::fs::OF_None);
   if (EC)
     exitWithErrorCode(EC, OutputFilename);
 
   InstrProfWriter &Writer = Contexts[0]->Writer;
   if (OutputFormat == PF_Text) {
static void populateProfileSymbolList(MemoryBuffer *Buffer,
   SmallVector<StringRef, 32> SymbolVec;
   StringRef Data = Buffer->getBuffer();
   Data.split(SymbolVec, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
 
   for (StringRef symbol : SymbolVec)
     PSL.add(symbol);
 }
 
-static void
-mergeSampleProfile(const WeightedFileVector &Inputs, SymbolRemapper *Remapper,
-                   StringRef OutputFilename, ProfileFormat OutputFormat,
-                   StringRef ProfileSymbolListFile, bool CompressProfSymList) {
+static void mergeSampleProfile(const WeightedFileVector &Inputs,
+                               SymbolRemapper *Remapper,
+                               StringRef OutputFilename,
+                               ProfileFormat OutputFormat,
+                               StringRef ProfileSymbolListFile,
+                               bool CompressProfSymList, FailureMode FailMode) {
   using namespace sampleprof;
   StringMap<FunctionSamples> ProfileMap;
   SmallVector<std::unique_ptr<sampleprof::SampleProfileReader>, 5> Readers;
   LLVMContext Context;
   sampleprof::ProfileSymbolList WriterList;
   for (const auto &Input : Inputs) {
     auto ReaderOrErr = SampleProfileReader::create(Input.Filename, Context);
-    if (std::error_code EC = ReaderOrErr.getError())
-      exitWithErrorCode(EC, Input.Filename);
+    if (std::error_code EC = ReaderOrErr.getError()) {
+      warnOrExitGivenError(FailMode, EC, Input.Filename);
+      continue;
+    }
 
     // We need to keep the readers around until after all the files are
     // read so that we do not lose the function names stored in each
     // reader's memory. The function names are needed to write out the
     // merged profile map.
     Readers.push_back(std::move(ReaderOrErr.get()));
     const auto Reader = Readers.back().get();
-    if (std::error_code EC = Reader->read())
-      exitWithErrorCode(EC, Input.Filename);
+    if (std::error_code EC = Reader->read()) {
+      warnOrExitGivenError(FailMode, EC, Input.Filename);
+      Readers.pop_back();
+      continue;
+    }
 
     StringMap<FunctionSamples> &Profiles = Reader->getProfiles();
     for (StringMap<FunctionSamples>::iterator I = Profiles.begin(),
                                               E = Profiles.end();
          I != E; ++I) {
       sampleprof_error Result = sampleprof_error::success;
       FunctionSamples Remapped =
           Remapper ? remapSamples(I->second, *Remapper, Result)
cl::opt<ProfileFormat> OutputFormat(
       cl::values(
           clEnumValN(PF_Binary, "binary", "Binary encoding (default)"),
           clEnumValN(PF_Compact_Binary, "compbinary",
                      "Compact binary encoding"),
           clEnumValN(PF_Ext_Binary, "extbinary", "Extensible binary encoding"),
           clEnumValN(PF_Text, "text", "Text encoding"),
           clEnumValN(PF_GCC, "gcc",
                      "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::opt<bool> OutputSparse("sparse", cl::init(false),
       cl::desc("Generate a sparse profile (only meaningful for -instr)"));
   cl::opt<unsigned> NumThreads(
       "num-threads", cl::init(0),
       cl::desc("Number of merge threads to use (default: autodetect)"));
   cl::alias NumThreadsA("j", cl::desc("Alias for --num-threads"),
                         cl::aliasopt(NumThreads));
   cl::opt<std::string> ProfileSymbolListFile(
static int merge_main(int argc, const char *argv[]) {
   }
 
   std::unique_ptr<SymbolRemapper> Remapper;
   if (!RemappingFile.empty())
     Remapper = SymbolRemapper::create(RemappingFile);
 
   if (ProfileKind == instr)
     mergeInstrProfile(WeightedInputs, Remapper.get(), OutputFilename,
-                      OutputFormat, OutputSparse, NumThreads);
+                      OutputFormat, OutputSparse, NumThreads, FailureMode);
   else
     mergeSampleProfile(WeightedInputs, Remapper.get(), OutputFilename,
                        OutputFormat, ProfileSymbolListFile,
-                       CompressProfSymList);
+                       CompressProfSymList, FailureMode);
 
   return 0;
 }
 
 /// Computer the overlap b/w profile BaseFilename and profile TestFilename.
 static void overlapInstrProfile(const std::string &BaseFilename,
                                 const std::string &TestFilename,
                                 const OverlapFuncFilters &FuncFilter,