<div dir="ltr"><div>Both comments have been addressed.<br><br></div>Sorry again for missing them earlier.<br><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 17, 2015 at 4:07 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Nathan Slingerland <<a href="mailto:slingn@gmail.com">slingn@gmail.com</a>> writes:<br>
> Hi Justin,<br>
><br>
> Yes - sorry about that. I realized that after I committed.<br>
> It's really a shame that comments from email aren't aggregated in<br>
> Phabricator.<br>
<br>
</span>Cool, no problem.<br>
<span class=""><br>
> I have addressed the issue with hash_mismatch / value_site_mismatch also<br>
> showing the same error in a follow-on commit.<br>
><br>
> I will make the change to SmallSet. We have one hint now - but the<br>
> intention is that they be added as needed / use cases come up.<br>
<br>
</span>Sounds good. Thanks!<br>
<div class="HOEnZb"><div class="h5"><br>
> -Nathan<br>
><br>
><br>
> On Tue, Nov 17, 2015 at 3:13 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>><br>
> wrote:<br>
><br>
>> Nathan Slingerland via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
>> > Author: slingn<br>
>> > Date: Tue Nov 17 16:08:53 2015<br>
>> > New Revision: 253384<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=253384&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=253384&view=rev</a><br>
>> > Log:<br>
>> > [llvm-profdata] Improve error messaging when merging mismatched profile<br>
>> data<br>
>><br>
>> You committed this without addressing all of the comments in the review<br>
>> thread. Please don't do that in the future. I've repeated my comments<br>
>> below.<br>
>><br>
>> > Summary:<br>
>> > This change tries to make the root cause of instrumented profile data<br>
>> > merge failures clearer.<br>
>> ><br>
>> > Previous:<br>
>> ><br>
>> > $ llvm-profdata merge test_0.profraw test_1.profraw -o<br>
>> test_merged.profdata<br>
>> > test_1.profraw: foo: Function count mismatch<br>
>> > test_1.profraw: bar: Function count mismatch<br>
>> > test_1.profraw: baz: Function count mismatch<br>
>> > ...<br>
>> ><br>
>> > Changed:<br>
>> ><br>
>> > $ llvm-profdata merge test_0.profraw test_1.profraw -o<br>
>> test_merged.profdata<br>
>> > test_1.profraw: foo: Function basic block count change detected<br>
>> > (counter mismatch)<br>
>> > Make sure that all profile data to be merged is generated from the same<br>
>> binary.<br>
>> > test_1.profraw: bar: Function basic block count change detected<br>
>> > (counter mismatch)<br>
>> > test_1.profraw: baz: Function basic block count change detected<br>
>> > (counter mismatch)<br>
>> > ...<br>
>> ><br>
>> > Reviewers: dnovillo, davidxl, bogner<br>
>> ><br>
>> > Subscribers: llvm-commits<br>
>> ><br>
>> > Differential Revision: <a href="http://reviews.llvm.org/D14739" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14739</a><br>
>> ><br>
>> > Modified:<br>
>> > llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
>> > llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext<br>
>> > llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp<br>
>> ><br>
>> > Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
>> > URL:<br>
>> ><br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=253384&r1=253383&r2=253384&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=253384&r1=253383&r2=253384&view=diff</a><br>
>> ><br>
>> ==============================================================================<br>
>> > --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)<br>
>> > +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Tue Nov 17 16:08:53 2015<br>
>> > @@ -51,13 +51,13 @@ class InstrProfErrorCategoryType : publi<br>
>> > case instrprof_error::unknown_function:<br>
>> > return "No profile data available for function";<br>
>> > case instrprof_error::hash_mismatch:<br>
>> > - return "Function hash mismatch";<br>
>> > + return "Function control flow change detected (hash mismatch)";<br>
>> > case instrprof_error::count_mismatch:<br>
>> > - return "Function count mismatch";<br>
>> > + return "Function basic block count change detected (counter<br>
>> mismatch)";<br>
>> > case instrprof_error::counter_overflow:<br>
>> > return "Counter overflow";<br>
>> > case instrprof_error::value_site_count_mismatch:<br>
>> > - return "Function's value site counts mismatch";<br>
>> > + return "Function value site count change detected (counter<br>
>> mismatch)";<br>
>> > }<br>
>> > llvm_unreachable("A value of instrprof_error has no message.");<br>
>> > }<br>
>> ><br>
>> > Modified: llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext<br>
>> > URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext?rev=253384&r1=253383&r2=253384&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext?rev=253384&r1=253383&r2=253384&view=diff</a><br>
>> ><br>
>> ==============================================================================<br>
>> > --- llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext<br>
>> (original)<br>
>> > +++ llvm/trunk/test/tools/llvm-profdata/count-mismatch.proftext Tue Nov<br>
>> 17 16:08:53 2015<br>
>> > @@ -14,7 +14,8 @@ foo<br>
>> ><br>
>> > # The hash matches, but we can't combine these because the number of<br>
>> > # counters differs.<br>
>> > -# MERGE_ERRS: count-mismatch.proftext: foo: Function count mismatch<br>
>> > +# MERGE_ERRS: count-mismatch.proftext: foo: Function basic block count<br>
>> change detected (counter mismatch)<br>
>> > +# MERGE_ERRS: Make sure that all profile data to be merged is generated<br>
>> from the same binary.<br>
>> > foo<br>
>> > 1024<br>
>> > 3<br>
>> ><br>
>> > Modified: llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp<br>
>> > URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp?rev=253384&r1=253383&r2=253384&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp?rev=253384&r1=253383&r2=253384&view=diff</a><br>
>> ><br>
>> ==============================================================================<br>
>> > --- llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp (original)<br>
>> > +++ llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp Tue Nov 17 16:08:53<br>
>> 2015<br>
>> > @@ -27,6 +27,8 @@<br>
>> > #include "llvm/Support/Signals.h"<br>
>> > #include "llvm/Support/raw_ostream.h"<br>
>> ><br>
>> > +#include <set><br>
>> > +<br>
>> > using namespace llvm;<br>
>> ><br>
>> > static void exitWithError(const Twine &Message,<br>
>> > @@ -41,7 +43,8 @@ static void exitWithError(const Twine &M<br>
>> > ::exit(1);<br>
>> > }<br>
>> ><br>
>> > -static void exitWithErrorCode(const std::error_code &Error, StringRef<br>
>> Whence = "") {<br>
>> > +static void exitWithErrorCode(const std::error_code &Error,<br>
>> > + StringRef Whence = "") {<br>
>> > if (Error.category() == instrprof_category()) {<br>
>> > instrprof_error instrError =<br>
>> static_cast<instrprof_error>(Error.value());<br>
>> > if (instrError == instrprof_error::unrecognized_format) {<br>
>> > @@ -57,6 +60,32 @@ namespace {<br>
>> > enum ProfileKinds { instr, sample };<br>
>> > }<br>
>> ><br>
>> > +static void handleMergeWriterError(std::error_code &Error,<br>
>> > + StringRef WhenceFile = "",<br>
>> > + StringRef WhenceFunction = "",<br>
>> > + bool ShowHint = true)<br>
>> > +{<br>
>> > + if (!WhenceFile.empty())<br>
>> > + errs() << WhenceFile << ": ";<br>
>> > + if (!WhenceFunction.empty())<br>
>> > + errs() << WhenceFunction << ": ";<br>
>> > + errs() << Error.message() << "\n";<br>
>> > +<br>
>> > + if (ShowHint) {<br>
>> > + StringRef Hint = "";<br>
>> > + if (Error.category() == instrprof_category()) {<br>
>> > + instrprof_error instrError =<br>
>> static_cast<instrprof_error>(Error.value());<br>
>> > + if (instrError == instrprof_error::count_mismatch) {<br>
>> > + Hint = "Make sure that all profile data to be merged is<br>
>> generated " \<br>
>> > + "from the same binary.";<br>
>><br>
>> hash_mismatch and value_site_mismatch could probably use the same hint, no?<br>
>><br>
>> > + }<br>
>> > + }<br>
>> > +<br>
>> > + if (!Hint.empty())<br>
>> > + errs() << Hint << "\n";<br>
>> > + }<br>
>> > +}<br>
>> > +<br>
>> > static void mergeInstrProfile(const cl::list<std::string> &Inputs,<br>
>> > StringRef OutputFilename) {<br>
>> > if (OutputFilename.compare("-") == 0)<br>
>> > @@ -68,15 +97,20 @@<br>
>> > exitWithErrorCode(EC, OutputFilename);<br>
>> ><br>
>> > InstrProfWriter Writer;<br>
>> > + std::set<std::error_code> WriterErrorCodes;<br>
>><br>
>> llvm/ADT/SmallSet.h is probably a better fit here, assuming more<br>
>> distinct hints are added. If we only have the one hint we can probably<br>
>> just use a bool.<br>
>><br>
>> > for (const auto &Filename : Inputs) {<br>
>> > auto ReaderOrErr = InstrProfReader::create(Filename);<br>
>> > if (std::error_code ec = ReaderOrErr.getError())<br>
>> > exitWithErrorCode(ec, Filename);<br>
>> ><br>
>> > auto Reader = std::move(ReaderOrErr.get());<br>
>> > - for (auto &I : *Reader)<br>
>> > - if (std::error_code EC = Writer.addRecord(std::move(I)))<br>
>> > - errs() << Filename << ": " << I.Name << ": " << EC.message() <<<br>
>> "\n";<br>
>> > + for (auto &I : *Reader) {<br>
>> > + if (std::error_code EC = Writer.addRecord(std::move(I))) {<br>
>> > + // Only show hint the first time an error occurs.<br>
>> > + bool firstTime = WriterErrorCodes.insert(EC).second;<br>
>> > + handleMergeWriterError(EC, Filename, I.Name, firstTime);<br>
>> > + }<br>
>> > + }<br>
>> > if (Reader->hasError())<br>
>> > exitWithErrorCode(Reader->getError(), Filename);<br>
>> > }<br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>><br>
</div></div></blockquote></div><br></div></div></div></div>