[llvm-profdata] Add check for text profile formats and improve error reporting

[slingn]

Summary

This change addresses two possible instances of user error / confusion when
merging sampled profile data.

Previously any input that didn't match the raw or processed instrumented format
would automatically be interpreted as instrumented profile text format data.
No error would be reported during the merge.

Example:
If foo-sampled.profdata and bar-sampled.profdata are binary sampled profiles:

Old behavior:
$ llvm-profdata merge foo-sampled.profdata bar-sampled.profdata -output foobar-sampled.profdata
$ llvm-profdata show -sample foobar-sampled.profdata
error: foobar-sampled.profdata:1: Expected 'mangled_name:NUM:NUM', found lprofi

This change adds basic checks for valid input data when assuming text input.
It also makes error messages related to file format validity more specific about
the assumbed profile data type.

New behavior:
$ llvm-profdata merge foo-sampled.profdata bar-sampled.profdata -o foobar-sampled.profdata
error: foo.profdata: Unrecognized instrumentation profile encoding format
Perhaps you forgot to use the -sample option?

[davidxl]

Looks good, but if the message is augmented to be something like the
following:

error: foo.profdata: Malformed instrumented profile data. Perhaps you
forgot to use -sample option?

would make it even more friendlier.

thanks,

David

[dnovillo]

Could you add a test for the new behaviour that you described in the patch?

Thanks.

lib/ProfileData/SampleProfReader.cpp

257 ↗

(On Diff #39869)

This generic check won't let us distinguish text formats for sample vs instrumented profiles. I'd suggest actually trying to parse a single line here (factor it out of the text parser) rather than checking for ASCII characters.

[slingn]

Updated for davidxl and dvoillo feedback.

[slingn]

lib/ProfileData/SampleProfReader.cpp

257 ↗

(On Diff #39869)

Good point. The sample profile text format is structured enough to enable that. Instrumented profile text format not so much.

[davidxl]

The only limitation here is that when a sample text file is passed to llvm-profdata tool without -sample option, the tool can still not give very useful information -- supporting that requires non trivial refactoring so that 'hasFormat' interfaces are moved to a common place. Probably not worth the effort.

Looks good to me. Watch out for more comments post-commit.

[dnovillo]

LGTM. Thanks for fixing this.

[slingn]

Fix indentation.

llvm/trunk/include/llvm/ProfileData/InstrProf.h

GlobalVariable *createPGOFuncNameVar(Module &M,
                                      GlobalValue::LinkageTypes Linkage,
                                      StringRef FuncName);
 
 const std::error_category &instrprof_category();
 
 enum class instrprof_error {
   success = 0,
   eof,
+  unrecognized_format,
   bad_magic,
   bad_header,
   unsupported_version,
   unsupported_hash_type,
   too_large,
   truncated,
   malformed,
   unknown_function,

llvm/trunk/include/llvm/ProfileData/InstrProfReader.h

private:
   line_iterator Line;
 
   TextInstrProfReader(const TextInstrProfReader &) = delete;
   TextInstrProfReader &operator=(const TextInstrProfReader &) = delete;
 public:
   TextInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer_)
       : DataBuffer(std::move(DataBuffer_)), Line(*DataBuffer, true, '#') {}
 
+  /// Return true if the given buffer is in text instrprof format.
+  static bool hasFormat(const MemoryBuffer &Buffer);
+
   /// Read the header.
   std::error_code readHeader() override { return success(); }
   /// Read a single record.
   std::error_code readNextRecord(InstrProfRecord &Record) override;
 };
 
 /// Reader for the raw instrprof binary format from runtime.
 ///

llvm/trunk/include/llvm/ProfileData/SampleProfReader.h

public:
   SampleProfileReaderText(std::unique_ptr<MemoryBuffer> B, LLVMContext &C)
       : SampleProfileReader(std::move(B), C) {}
 
   /// \brief Read and validate the file header.
   std::error_code readHeader() override { return sampleprof_error::success; }
 
   /// \brief Read sample profiles from the associated file.
   std::error_code read() override;
+
+  /// \brief Return true if \p Buffer is in the format supported by this class.
+  static bool hasFormat(const MemoryBuffer &Buffer);
 };
 
 class SampleProfileReaderBinary : public SampleProfileReader {
 public:
   SampleProfileReaderBinary(std::unique_ptr<MemoryBuffer> B, LLVMContext &C)
       : SampleProfileReader(std::move(B), C), Data(nullptr), End(nullptr) {}
 
   /// \brief Read and validate the file header.

llvm/trunk/lib/ProfileData/InstrProf.cpp

class InstrProfErrorCategoryType : public std::error_category {
   const char *name() const LLVM_NOEXCEPT override { return "llvm.instrprof"; }
   std::string message(int IE) const override {
     instrprof_error E = static_cast<instrprof_error>(IE);
     switch (E) {
     case instrprof_error::success:
       return "Success";
     case instrprof_error::eof:
       return "End of File";
+    case instrprof_error::unrecognized_format:
+      return "Unrecognized instrumentation profile encoding format";
     case instrprof_error::bad_magic:
-      return "Invalid profile data (bad magic)";
+      return "Invalid instrumentation profile data (bad magic)";
     case instrprof_error::bad_header:
-      return "Invalid profile data (file header is corrupt)";
+      return "Invalid instrumentation profile data (file header is corrupt)";
     case instrprof_error::unsupported_version:
-      return "Unsupported profiling format version";
+      return "Unsupported instrumentation profile format version";
     case instrprof_error::unsupported_hash_type:
-      return "Unsupported profiling hash";
+      return "Unsupported instrumentation profile hash type";
     case instrprof_error::too_large:
       return "Too much profile data";
     case instrprof_error::truncated:
       return "Truncated profile data";
     case instrprof_error::malformed:
-      return "Malformed profile data";
+      return "Malformed instrumentation profile data";
     case instrprof_error::unknown_function:
       return "No profile data available for function";
     case instrprof_error::hash_mismatch:
       return "Function hash mismatch";
     case instrprof_error::count_mismatch:
       return "Function count mismatch";
     case instrprof_error::counter_overflow:
       return "Counter overflow";

llvm/trunk/lib/ProfileData/InstrProfReader.cpp

InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer) {
   std::unique_ptr<InstrProfReader> Result;
   // Create the reader.
   if (IndexedInstrProfReader::hasFormat(*Buffer))
     Result.reset(new IndexedInstrProfReader(std::move(Buffer)));
   else if (RawInstrProfReader64::hasFormat(*Buffer))
     Result.reset(new RawInstrProfReader64(std::move(Buffer)));
   else if (RawInstrProfReader32::hasFormat(*Buffer))
     Result.reset(new RawInstrProfReader32(std::move(Buffer)));
-  else
+  else if (TextInstrProfReader::hasFormat(*Buffer))
     Result.reset(new TextInstrProfReader(std::move(Buffer)));
+  else
+    return instrprof_error::unrecognized_format;
 
   // Initialize the reader and return the result.
   if (std::error_code EC = initializeReader(*Result))
     return EC;
 
   return std::move(Result);
 }
 
IndexedInstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer) {
   return std::move(Result);
 }
 
 void InstrProfIterator::Increment() {
   if (Reader->readNextRecord(Record))
     *this = InstrProfIterator();
 }
 
+bool TextInstrProfReader::hasFormat(const MemoryBuffer &Buffer) {
+  // Verify that this really looks like plain ASCII text by checking a
+  // 'reasonable' number of characters (up to profile magic size).
+  size_t count = std::min(Buffer.getBufferSize(), sizeof(uint64_t));
+  StringRef buffer = Buffer.getBufferStart();
+  return count == 0 || std::all_of(buffer.begin(), buffer.begin() + count,
+    [](char c) { return ::isprint(c) || ::isspace(c); });
+}
+
 std::error_code TextInstrProfReader::readNextRecord(InstrProfRecord &Record) {
   // Skip empty lines and comments.
   while (!Line.is_at_end() && (Line->empty() || Line->startswith("#")))
     ++Line;
   // If we hit EOF while looking for a name, we're done.
   if (Line.is_at_end())
     return error(instrprof_error::eof);
 

llvm/trunk/lib/ProfileData/SampleProf.cpp

 class SampleProfErrorCategoryType : public std::error_category {
   const char *name() const LLVM_NOEXCEPT override { return "llvm.sampleprof"; }
   std::string message(int IE) const override {
     sampleprof_error E = static_cast<sampleprof_error>(IE);
     switch (E) {
     case sampleprof_error::success:
       return "Success";
     case sampleprof_error::bad_magic:
-      return "Invalid file format (bad magic)";
+      return "Invalid sample profile data (bad magic)";
     case sampleprof_error::unsupported_version:
-      return "Unsupported format version";
+      return "Unsupported sample profile format version";
     case sampleprof_error::too_large:
       return "Too much profile data";
     case sampleprof_error::truncated:
       return "Truncated profile data";
     case sampleprof_error::malformed:
-      return "Malformed profile data";
+      return "Malformed sample profile data";
     case sampleprof_error::unrecognized_format:
-      return "Unrecognized profile encoding format";
+      return "Unrecognized sample profile encoding format";
     case sampleprof_error::unsupported_writing_format:
       return "Profile encoding format unsupported for writing operations";
     case sampleprof_error::truncated_name_table:
       return "Truncated function name table";
     case sampleprof_error::not_implemented:
       return "Unimplemented feature";
     }
     llvm_unreachable("A value of sampleprof_error has no message.");

llvm/trunk/lib/ProfileData/SampleProfReader.cpp

if ((*LineIt)[0] != ' ') {
         FProfile.addBodySamples(LineOffset, Discriminator, NumSamples);
       }
     }
   }
 
   return sampleprof_error::success;
 }
 
+bool SampleProfileReaderText::hasFormat(const MemoryBuffer &Buffer) {
+  bool result = false;
+
+  // Check that the first non-comment line is a valid function header.
+  line_iterator LineIt(Buffer, /*SkipBlanks=*/true, '#');
+  if (!LineIt.is_at_eof()) {
+    if ((*LineIt)[0] != ' ') {
+      uint64_t NumSamples, NumHeadSamples;
+      StringRef FName;
+      result = ParseHead(*LineIt, FName, NumSamples, NumHeadSamples);
+    }
+  }
+
+  return result;
+}
+
 template <typename T> ErrorOr<T> SampleProfileReaderBinary::readNumber() {
   unsigned NumBytesRead = 0;
   std::error_code EC;
   uint64_t Val = decodeULEB128(Data, &NumBytesRead);
 
   if (Val > std::numeric_limits<T>::max())
     EC = sampleprof_error::malformed;
   else if (Data + NumBytesRead > End)
if (std::error_code EC = BufferOrError.getError())
     return EC;
 
   auto Buffer = std::move(BufferOrError.get());
   std::unique_ptr<SampleProfileReader> Reader;
   if (SampleProfileReaderBinary::hasFormat(*Buffer))
     Reader.reset(new SampleProfileReaderBinary(std::move(Buffer), C));
   else if (SampleProfileReaderGCC::hasFormat(*Buffer))
     Reader.reset(new SampleProfileReaderGCC(std::move(Buffer), C));
-  else
+  else if (SampleProfileReaderText::hasFormat(*Buffer))
     Reader.reset(new SampleProfileReaderText(std::move(Buffer), C));
+  else
+    return sampleprof_error::unrecognized_format;
 
   if (std::error_code EC = Reader->readHeader())
     return EC;
 
   return std::move(Reader);
 }

llvm/trunk/test/tools/llvm-profdata/raw-magic-but-no-header.test

 RUN: printf '\201rforpl\377' > %t
 RUN: not llvm-profdata show %t 2>&1 | FileCheck %s
 RUN: printf '\377lprofr\201' > %t
 RUN: not llvm-profdata show %t 2>&1 | FileCheck %s
 
-CHECK: error: {{.+}}: Invalid profile data (file header is corrupt)
+CHECK: error: {{.+}}: Invalid instrumentation profile data (file header is corrupt)

llvm/trunk/test/tools/llvm-profdata/sample-profile-basic.test

 
 4- Merge the binary and text encodings of the profile and check that the
    counters have doubled.
 RUN: llvm-profdata merge --sample %p/Inputs/sample-profile.proftext -o %t-binprof
 RUN: llvm-profdata merge --sample --text %p/Inputs/sample-profile.proftext %t-binprof -o - | FileCheck %s --check-prefix=MERGE1
 MERGE1: main:368038:0
 MERGE1: 9: 4128 _Z3fooi:1262 _Z3bari:2942
 MERGE1: _Z3fooi:15422:1220
+
+5- Detect invalid text encoding (e.g. instrumentation profile text format).
+RUN: not llvm-profdata show --sample %p/Inputs/foo3bar3-1.proftext 2>&1 | FileCheck %s --check-prefix=BADTEXT
+BADTEXT: error: {{.+}}: Unrecognized sample profile encoding format

llvm/trunk/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.profdata -o %t.out 2>&1 | FileCheck %s --check-prefix=INVALID-COUNT-LATER
-INVALID-COUNT-LATER: error: {{.*}}invalid-count-later.proftext: Malformed profile data
+INVALID-COUNT-LATER: error: {{.*}}invalid-count-later.proftext: Malformed instrumentation profile data
 
+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 profile data
+BAD-HASH: error: {{.*}}bad-hash.proftext: Malformed instrumentation profile data
 
+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 profile data
+NO-COUNTS: error: {{.*}}no-counts.proftext: Malformed instrumentation profile data
+
+4- Detect binary input
+RUN: echo -n $'\xff\xe5\xd0\xb1\xf4\c9\x94\xa8' > %t.bin
+RUN: not llvm-profdata show %t.bin 2>&1 | FileCheck %s --check-prefix=BINARY
+BINARY: error: {{.+}}: Unrecognized instrumentation profile encoding format
+BINARY: Perhaps you forgot to use the -sample option?

llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp

 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
-static void exitWithError(const Twine &Message, StringRef Whence = "") {
+static void exitWithError(const Twine &Message,
+                          StringRef Whence = "",
+                          StringRef Hint = "") {
   errs() << "error: ";
   if (!Whence.empty())
     errs() << Whence << ": ";
   errs() << Message << "\n";
+  if (!Hint.empty())
+    errs() << Hint << "\n";
   ::exit(1);
 }
 
+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) {
+      // Hint for common error of forgetting -sample for sample profiles.
+      exitWithError(Error.message(), Whence,
+                    "Perhaps you forgot to use the -sample option?");
+    }
+  }
+  exitWithError(Error.message(), Whence);
+}
+
 namespace {
     enum ProfileKinds { instr, sample };
 }
 
 static void mergeInstrProfile(const cl::list<std::string> &Inputs,
                               StringRef OutputFilename) {
   if (OutputFilename.compare("-") == 0)
     exitWithError("Cannot write indexed profdata format to stdout.");
 
   std::error_code EC;
   raw_fd_ostream Output(OutputFilename.data(), EC, sys::fs::F_None);
   if (EC)
-    exitWithError(EC.message(), OutputFilename);
+    exitWithErrorCode(EC, OutputFilename);
 
   InstrProfWriter Writer;
   for (const auto &Filename : Inputs) {
     auto ReaderOrErr = InstrProfReader::create(Filename);
     if (std::error_code ec = ReaderOrErr.getError())
-      exitWithError(ec.message(), Filename);
+      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";
     if (Reader->hasError())
-      exitWithError(Reader->getError().message(), Filename);
+      exitWithErrorCode(Reader->getError(), Filename);
   }
   Writer.write(Output);
 }
 
 static void mergeSampleProfile(const cl::list<std::string> &Inputs,
                                StringRef OutputFilename,
                                sampleprof::SampleProfileFormat OutputFormat) {
   using namespace sampleprof;
   auto WriterOrErr = SampleProfileWriter::create(OutputFilename, OutputFormat);
   if (std::error_code EC = WriterOrErr.getError())
-    exitWithError(EC.message(), OutputFilename);
+    exitWithErrorCode(EC, OutputFilename);
 
   auto Writer = std::move(WriterOrErr.get());
   StringMap<FunctionSamples> ProfileMap;
   SmallVector<std::unique_ptr<sampleprof::SampleProfileReader>, 5> Readers;
   for (const auto &Filename : Inputs) {
     auto ReaderOrErr =
         SampleProfileReader::create(Filename, getGlobalContext());
     if (std::error_code EC = ReaderOrErr.getError())
-      exitWithError(EC.message(), Filename);
+      exitWithErrorCode(EC, Filename);
 
     // We need to keep the readers around until after all the files are
     // read so that we do not lose the function names stored in each
     // reader's memory. The function names are needed to write out the
     // merged profile map.
     Readers.push_back(std::move(ReaderOrErr.get()));
     const auto Reader = Readers.back().get();
     if (std::error_code EC = Reader->read())
-      exitWithError(EC.message(), Filename);
+      exitWithErrorCode(EC, Filename);
 
     StringMap<FunctionSamples> &Profiles = Reader->getProfiles();
     for (StringMap<FunctionSamples>::iterator I = Profiles.begin(),
                                               E = Profiles.end();
          I != E; ++I) {
       StringRef FName = I->first();
       FunctionSamples &Samples = I->second;
       ProfileMap[FName].merge(Samples);
static int merge_main(int argc, const char *argv[]) {
   return 0;
 }
 
 static int showInstrProfile(std::string Filename, bool ShowCounts,
                             bool ShowIndirectCallTargets, bool ShowAllFunctions,
                             std::string ShowFunction, raw_fd_ostream &OS) {
   auto ReaderOrErr = InstrProfReader::create(Filename);
   if (std::error_code EC = ReaderOrErr.getError())
-    exitWithError(EC.message(), Filename);
+    exitWithErrorCode(EC, Filename);
 
   auto Reader = std::move(ReaderOrErr.get());
   uint64_t MaxFunctionCount = 0, MaxBlockCount = 0;
   size_t ShownFunctions = 0, TotalFunctions = 0;
   for (const auto &Func : *Reader) {
     bool Show =
         ShowAllFunctions || (!ShowFunction.empty() &&
                              Func.Name.find(ShowFunction) != Func.Name.npos);
if (Show && ShowIndirectCallTargets) {
         for (uint32_t V = 0; V < NV; V++) {
           OS << "\t[ " << I << ", ";
           OS << (const char *)VD[V].Value << ", " << VD[V].Count << " ]\n";
         }
       }
     }
   }
   if (Reader->hasError())
-    exitWithError(Reader->getError().message(), Filename);
+    exitWithErrorCode(Reader->getError(), Filename);
 
   if (ShowAllFunctions || !ShowFunction.empty())
     OS << "Functions shown: " << ShownFunctions << "\n";
   OS << "Total functions: " << TotalFunctions << "\n";
   OS << "Maximum function count: " << MaxFunctionCount << "\n";
   OS << "Maximum internal block count: " << MaxBlockCount << "\n";
   return 0;
 }
 
 static int showSampleProfile(std::string Filename, bool ShowCounts,
                              bool ShowAllFunctions, std::string ShowFunction,
                              raw_fd_ostream &OS) {
   using namespace sampleprof;
   auto ReaderOrErr = SampleProfileReader::create(Filename, getGlobalContext());
   if (std::error_code EC = ReaderOrErr.getError())
-    exitWithError(EC.message(), Filename);
+    exitWithErrorCode(EC, Filename);
 
   auto Reader = std::move(ReaderOrErr.get());
   if (std::error_code EC = Reader->read())
-    exitWithError(EC.message(), Filename);
+    exitWithErrorCode(EC, Filename);
 
   if (ShowAllFunctions || ShowFunction.empty())
     Reader->dump(OS);
   else
     Reader->dumpFunctionProfile(ShowFunction, OS);
 
   return 0;
 }
static int show_main(int argc, const char *argv[]) {
   cl::ParseCommandLineOptions(argc, argv, "LLVM profile data summary\n");
 
   if (OutputFilename.empty())
     OutputFilename = "-";
 
   std::error_code EC;
   raw_fd_ostream OS(OutputFilename.data(), EC, sys::fs::F_Text);
   if (EC)
-    exitWithError(EC.message(), OutputFilename);
+      exitWithErrorCode(EC, OutputFilename);
 
   if (ShowAllFunctions && !ShowFunction.empty())
     errs() << "warning: -function argument ignored: showing all functions\n";
 
   if (ProfileKind == instr)
     return showInstrProfile(Filename, ShowCounts, ShowIndirectCallTargets,
                             ShowAllFunctions, ShowFunction, OS);
   else