[llvm] r253384 - [llvm-profdata] Improve error messaging when merging mismatched profile data

Nathan Slingerland via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 18 09:34:45 PST 2015 

Both comments have been addressed.

Sorry again for missing them earlier.

On Tue, Nov 17, 2015 at 4:07 PM, Justin Bogner <mail at justinbogner.com>
wrote:

> Nathan Slingerland <slingn at gmail.com> writes:
> > Hi Justin,
> >
> > Yes - sorry about that. I realized that after I committed.
> > It's really a shame that comments from email aren't aggregated in
> > Phabricator.
>
> Cool, no problem.
>
> > I have addressed the issue with hash_mismatch / value_site_mismatch also
> > showing the same error in a follow-on commit.
> >
> > I will make the change to SmallSet. We have one hint now - but the
> > intention is that they be added as needed / use cases come up.
>
> Sounds good. Thanks!
>
> > -Nathan
> >
> >
> > On Tue, Nov 17, 2015 at 3:13 PM, Justin Bogner <mail at justinbogner.com>
> > wrote:
> >
> >> Nathan Slingerland via llvm-commits <llvm-commits at lists.llvm.org>
> writes:
> >> > Author: slingn
> >> > Date: Tue Nov 17 16:08:53 2015
> >> > New Revision: 253384
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=253384&view=rev
> >> > Log:
> >> > [llvm-profdata] Improve error messaging when merging mismatched
> profile
> >> data
> >>
> >> You committed this without addressing all of the comments in the review
> >> thread. Please don't do that in the future. I've repeated my comments
> >> below.
> >>
> >> > Summary:
> >> > 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)
> >> > ...
> >> >
> >> > Reviewers: dnovillo, davidxl, bogner
> >> >
> >> > Subscribers: llvm-commits
> >> >
> >> > Differential Revision: http://reviews.llvm.org/D14739
> >> >
> >> > Modified:
> >> >     llvm/trunk/lib/ProfileData/InstrProf.cpp
> >> >     llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext
> >> >     llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp
> >> >
> >> > Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
> >> > URL:
> >> >
> >>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=253384&r1=253383&r2=253384&view=diff
> >> >
> >>
> ==============================================================================
> >> > --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
> >> > +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Tue Nov 17 16:08:53 2015
> >> > @@ -51,13 +51,13 @@ class InstrProfErrorCategoryType : publi
> >> >      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.");
> >> >    }
> >> >
> >> > Modified: llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext
> >> > URL:
> >>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext?rev=253384&r1=253383&r2=253384&view=diff
> >> >
> >>
> ==============================================================================
> >> > --- llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext
> >> (original)
> >> > +++ llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext Tue
> Nov
> >> 17 16:08:53 2015
> >> > @@ -14,7 +14,8 @@ foo
> >> >
> >> >  # 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
> >> >
> >> > Modified: llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp
> >> > URL:
> >>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp?rev=253384&r1=253383&r2=253384&view=diff
> >> >
> >>
> ==============================================================================
> >> > --- llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp (original)
> >> > +++ llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp Tue Nov 17
> 16:08:53
> >> 2015
> >> > @@ -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 @@ static void exitWithError(const Twine &M
> >> >    ::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 @@ namespace {
> >> >      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);
> >> >    }
> >> >
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at lists.llvm.org
> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151118/ff490c64/attachment.html>

More information about the llvm-commits mailing list