[llvm] r253384 - [llvm-profdata] Improve error messaging when merging mismatched profile data
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 17 15:13:34 PST 2015
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
More information about the llvm-commits
mailing list