[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

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

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/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/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
+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/tools/llvm-profdata/llvm-profdata.cpp

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) {
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())
+    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) {