[PATCH] D14739: [llvm-profdata] Improve error messaging when merging mismatched profile data

[Justin Bogner via llvm-commits]

Nathan Slingerland <slingn at gmail.com> writes:
> slingn created this revision.
> slingn added reviewers: dnovillo, davidxl, bogner.
> slingn added a subscriber: llvm-commits.
>
> This change tries to make the root cause of instrumented profile data
> merge failures clearer.
>
> Previous:
>
> $ llvm-profdata merge test_0.profraw test_1.profraw -o test_merged.profdata
> test_1.profraw: foo: Function count mismatch
> test_1.profraw: bar: Function count mismatch
> test_1.profraw: baz: Function count mismatch
> ...
>
> Changed:
>
> $ llvm-profdata merge test_0.profraw test_1.profraw -o test_merged.profdata
> test_1.profraw: foo: Function basic block count change detected
> (counter mismatch)
> Make sure that all profile data to be merged is generated from the same binary.
> test_1.profraw: bar: Function basic block count change detected
> (counter mismatch)
> test_1.profraw: baz: Function basic block count change detected
> (counter mismatch)
> ...
>
> http://reviews.llvm.org/D14739
>
> Files:
>   lib/ProfileData/InstrProf.cpp
>   test/tools/llvm-profdata/count-mismatch.proftext
>   tools/llvm-profdata/llvm-profdata.cpp
>
> Index: tools/llvm-profdata/llvm-profdata.cpp
> ===================================================================
> --- tools/llvm-profdata/llvm-profdata.cpp
> +++ tools/llvm-profdata/llvm-profdata.cpp
> @@ -27,6 +27,8 @@
>  #include "llvm/Support/Signals.h"
>  #include "llvm/Support/raw_ostream.h"
>  
> +#include <set>
> +
>  using namespace llvm;
>  
>  static void exitWithError(const Twine &Message,
> @@ -41,7 +43,8 @@
>    ::exit(1);
>  }
>  
> -static void exitWithErrorCode(const std::error_code &Error, StringRef Whence = "") {
> +static void exitWithErrorCode(const std::error_code &Error,
> +                              StringRef Whence = "") {
>    if (Error.category() == instrprof_category()) {
>      instrprof_error instrError = static_cast<instrprof_error>(Error.value());
>      if (instrError == instrprof_error::unrecognized_format) {
> @@ -57,6 +60,32 @@
>      enum ProfileKinds { instr, sample };
>  }
>  
> +static void handleMergeWriterError(std::error_code &Error,
> +                                   StringRef WhenceFile = "",
> +                                   StringRef WhenceFunction = "",
> +                                   bool ShowHint = true)
> +{
> +  if (!WhenceFile.empty())
> +    errs() << WhenceFile << ": ";
> +  if (!WhenceFunction.empty())
> +    errs() << WhenceFunction << ": ";
> +  errs() << Error.message() << "\n";
> +
> +  if (ShowHint) {
> +    StringRef Hint = "";
> +    if (Error.category() == instrprof_category()) {
> +      instrprof_error instrError = static_cast<instrprof_error>(Error.value());
> +      if (instrError == instrprof_error::count_mismatch) {
> +        Hint = "Make sure that all profile data to be merged is generated " \
> +               "from the same binary.";

hash_mismatch and value_site_mismatch could probably use the same hint, no?

> +      }
> +    }
> +
> +    if (!Hint.empty())
> +      errs() << Hint << "\n";
> +  }
> +}
> +
>  static void mergeInstrProfile(const cl::list<std::string> &Inputs,
>                                StringRef OutputFilename) {
>    if (OutputFilename.compare("-") == 0)
> @@ -68,15 +97,20 @@
>      exitWithErrorCode(EC, OutputFilename);
>  
>    InstrProfWriter Writer;
> +  std::set<std::error_code> WriterErrorCodes;

llvm/ADT/SmallSet.h is probably a better fit here, assuming more
distinct hints are added. If we only have the one hint we can probably
just use a bool.

>    for (const auto &Filename : Inputs) {
>      auto ReaderOrErr = InstrProfReader::create(Filename);
>      if (std::error_code ec = ReaderOrErr.getError())
>        exitWithErrorCode(ec, Filename);
>  
>      auto Reader = std::move(ReaderOrErr.get());
> -    for (auto &I : *Reader)
> -      if (std::error_code EC = Writer.addRecord(std::move(I)))
> -        errs() << Filename << ": " << I.Name << ": " << EC.message() << "\n";
> +    for (auto &I : *Reader) {
> +      if (std::error_code EC = Writer.addRecord(std::move(I))) {
> +        // Only show hint the first time an error occurs.
> +        bool firstTime = WriterErrorCodes.insert(EC).second;
> +        handleMergeWriterError(EC, Filename, I.Name, firstTime);
> +      }
> +    }
>      if (Reader->hasError())
>        exitWithErrorCode(Reader->getError(), Filename);
>    }
> Index: test/tools/llvm-profdata/count-mismatch.proftext
> ===================================================================
> --- test/tools/llvm-profdata/count-mismatch.proftext
> +++ test/tools/llvm-profdata/count-mismatch.proftext
> @@ -14,7 +14,8 @@
>  
>  # The hash matches, but we can't combine these because the number of
>  # counters differs.
> -# MERGE_ERRS: count-mismatch.proftext: foo: Function count mismatch
> +# MERGE_ERRS: count-mismatch.proftext: foo: Function basic block count change detected (counter mismatch)
> +# MERGE_ERRS: Make sure that all profile data to be merged is generated from the same binary.
>  foo
>  1024
>  3
> Index: lib/ProfileData/InstrProf.cpp
> ===================================================================
> --- lib/ProfileData/InstrProf.cpp
> +++ lib/ProfileData/InstrProf.cpp
> @@ -51,13 +51,13 @@
>      case instrprof_error::unknown_function:
>        return "No profile data available for function";
>      case instrprof_error::hash_mismatch:
> -      return "Function hash mismatch";
> +      return "Function control flow change detected (hash mismatch)";
>      case instrprof_error::count_mismatch:
> -      return "Function count mismatch";
> +      return "Function basic block count change detected (counter mismatch)";
>      case instrprof_error::counter_overflow:
>        return "Counter overflow";
>      case instrprof_error::value_site_count_mismatch:
> -      return "Function's value site counts mismatch";
> +      return "Function value site count change detected (counter mismatch)";
>      }
>      llvm_unreachable("A value of instrprof_error has no message.");
>    }