[compiler-rt] Add more diagnostic to InstrProfError

[gulfem]

Summary

If profile data is malformed for any kind of reason, we generate
an error that only reports "malformed instrumentation profile data"
without any further information. This patch extends InstrProfError
class to receive an optional error message argument, so that we can
do better error reporting.

[phosek]

I think this is a great change but the messages could be improved. Saying that value is corrupted in every case doesn't really say much plus it's not clear if the value has really been corrupted, there are other ways in which the data can become invalid (like using incompatible runtime version or compiler error). Rather I'd try to explain what's the issue in concise terms. I left a few suggestions, hopefully you'll get the point.

llvm/include/llvm/ProfileData/InstrProf.h

325

I'd call this just getMessage which is what StringError does as well.

342

I'd call this just Msg which is what StringError does as well.

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

572

I'd omit the CovMapFuncRecordReader which may not be useful for most users.

llvm/lib/ProfileData/InstrProf.cpp

79

I'd use raw_string_ostream to assemble the message. This would like like this:

std::string Msg;
raw_string_ostream OS(Msg);
switch (Err) {
case instrprof_error::success:
  OS << "success";
  break;
...
}
...
return OS.str();

152

Some of the error messages above already include details in parenthesis and having multiple parenthesis might look awkward so I'd use : as a separator instead.

914

These are not full sentences so they shouldn't be capitalized, the same for all the other messages below.. I'd also consider moving the Integrity check failure at the end in parenthesis as an additional qualifier.

Regarding this particular error message, I think you can make it even more specific (corrupted is too generic) by saying something like "number of value profile kinds is invalid".

919

Rather than corrupted, I'd say something like "total size is not congruent modulo quardword size".

925

I'd use "value kind is invalid" not corrupted.

929

Maybe something like "address of the next value profile record is greater than the total size".

[davidxl]

Can some test be added?

[gulfem]

1. Addressed Petr's comments
2. Add two malformed profile tests
3. Change error messages in existing malformed profile tests

[gulfem]

@davidxl and @phosek did you have a chance to look at the latest revision?

[davidxl]

I find it cleaner just to expand known malformed errors into its own error category. WIth that the change can also be minimized.

[phosek]

llvm/lib/ProfileData/InstrProf.cpp

153

I'd omit the space here.

llvm/lib/ProfileData/InstrProfReader.cpp

273

We could provide some additional details.

280

292

342

There might still be some padding, just not the right amount.

450

462

553

562

[phosek]

llvm/lib/ProfileData/InstrProfReader.cpp

462

Actually here it could be either the counter offset that's out of bounds, or the number of counters that's out of bounds, so perhaps we should split this into two separate checks.

[gulfem]

Addressed reviwer comments

[gulfem]

In D108942#3087968, @davidxl wrote:

I find it cleaner just to expand known malformed errors into its own error category. WIth that the change can also be minimized.

@davidxl, adding an optional diagnostic error message to InstrProfError gives us the flexibility to do that not only for malformed profiles, but for other kind of profile errors.
I only added for malformed profiles because it was our current use case. But, we can later extend it for other kind of profile issues.
This is why I would prefer not to have another error category for malformed profiles.
Please let me know what you think about that.

[davidxl]

lgtm

[phosek]

I missed the latest patchset. I have a few more comments, feel free to address those in follow up changes.

llvm/lib/ProfileData/InstrProf.cpp

153

I'd omit the extra quotes, that's more consistent with the way Clang and other tools report errors

llvm/lib/ProfileData/InstrProfReader.cpp

465

In error messages, we usually prefer spelling things out fully, so this should be counter offset X is negative, I'd omit the parenthesis.

469–471

Here I'd say counter offset X is greater that the maximum number of counters Y.

476–480

Similarly here I'd say just number of counters X is greater than the maximum number of counters Y where X would be a sum of CounterOffset and NumCounters, no need to report those separately since you already check the offset above.

[RKSimon]

@gulfem This is failing on some buildbots: https://lab.llvm.org/buildbot/#/builders/109/builds/25948 please can you take a look?

[dyung]

In D108942#3119594, @RKSimon wrote:

@gulfem This is failing on some buildbots: https://lab.llvm.org/buildbot/#/builders/109/builds/25948 please can you take a look?

The test seems to require zlib which means either that dependency needs to be removed, or a "REQUIRES: zlib" needs to be added.

[gulfem]

In D108942#3119615, @dyung wrote:

In D108942#3119594, @RKSimon wrote:

@gulfem This is failing on some buildbots: https://lab.llvm.org/buildbot/#/builders/109/builds/25948 please can you take a look?

The test seems to require zlib which means either that dependency needs to be removed, or a "REQUIRES: zlib" needs to be added.

@RKSimon and @dyung,
Thanks for reporting, and I have already pushed a change that removed the dependency to zlib on those tests:
https://github.com/llvm/llvm-project/commit/126e7611c70ca41782aa851c2bec132607eb8127
After the fix, bot turned green.
https://lab.llvm.org/buildbot/#/builders/109/builds/25973

[gulfem]

In D108942#3119117, @phosek wrote:

I missed the latest patchset. I have a few more comments, feel free to address those in follow up changes.

@phosek, I also addressed your comments in:
https://github.com/llvm/llvm-project/commit/126e7611c70ca41782aa851c2bec132607eb8127

llvm/include/llvm/ProfileData/InstrProf.h

 };
 
 inline std::error_code make_error_code(instrprof_error E) {
   return std::error_code(static_cast<int>(E), instrprof_category());
 }
 
 class InstrProfError : public ErrorInfo<InstrProfError> {
 public:
-  InstrProfError(instrprof_error Err) : Err(Err) {
+  InstrProfError(instrprof_error Err, const Twine &ErrStr = Twine())
+      : Err(Err), Msg(ErrStr.str()) {
     assert(Err != instrprof_error::success && "Not an error");
   }
 
   std::string message() const override;
 
   void log(raw_ostream &OS) const override { OS << message(); }
 
   std::error_code convertToErrorCode() const override {
     return make_error_code(Err);
   }
 
   instrprof_error get() const { return Err; }
+  const std::string &getMessage() const { return Msg; }
 
   /// Consume an Error and return the raw enum value contained within it. The
   /// Error must either be a success value, or contain a single InstrProfError.
   static instrprof_error take(Error E) {
     auto Err = instrprof_error::success;
     handleAllErrors(std::move(E), [&Err](const InstrProfError &IPE) {
       assert(Err == instrprof_error::success && "Multiple errors encountered");
       Err = IPE.get();
     });
     return Err;
   }
 
   static char ID;
 
 private:
   instrprof_error Err;
+  std::string Msg;
 };
 
 class SoftInstrProfErrors {
   /// Count the number of soft instrprof_errors encountered and keep track of
   /// the first such error for reporting purposes.
 
   /// The first soft error encountered.
   instrprof_error FirstError = instrprof_error::success;
public:
   /// Create InstrProfSymtab from a set of names iteratable from
   /// \p IterRange. This interface is used by IndexedProfReader.
   template <typename NameIterRange> Error create(const NameIterRange &IterRange);
 
   /// Update the symtab by adding \p FuncName to the table. This interface
   /// is used by the raw and text profile readers.
   Error addFuncName(StringRef FuncName) {
     if (FuncName.empty())
-      return make_error<InstrProfError>(instrprof_error::malformed);
+      return make_error<InstrProfError>(instrprof_error::malformed,
+                                        "function name is empty");
     auto Ins = NameTab.insert(FuncName);
     if (Ins.second) {
       MD5NameMap.push_back(std::make_pair(
           IndexedInstrProf::ComputeHash(FuncName), Ins.first->getKey()));
       Sorted = false;
     }
     return Error::success();
   }

llvm/include/llvm/ProfileData/InstrProfReader.h

public:
   value_type &operator*() { return Record; }
   value_type *operator->() { return &Record; }
 };
 
 /// Base class and interface for reading profiling data of any known instrprof
 /// format. Provides an iterator over NamedInstrProfRecords.
 class InstrProfReader {
   instrprof_error LastError = instrprof_error::success;
+  std::string LastErrorMsg;
 
 public:
   InstrProfReader() = default;
   virtual ~InstrProfReader() = default;
 
   /// Read the header.  Required before reading first record.
   virtual Error readHeader() = 0;
 
public:
 
   /// Compute the sum of counts and return in Sum.
   void accumulateCounts(CountSumOrPercent &Sum, bool IsCS);
 
 protected:
   std::unique_ptr<InstrProfSymtab> Symtab;
 
   /// Set the current error and return same.
-  Error error(instrprof_error Err) {
+  Error error(instrprof_error Err, const std::string &ErrMsg = "") {
     LastError = Err;
+    LastErrorMsg = ErrMsg;
     if (Err == instrprof_error::success)
       return Error::success();
-    return make_error<InstrProfError>(Err);
+    return make_error<InstrProfError>(Err, ErrMsg);
   }
 
-  Error error(Error &&E) { return error(InstrProfError::take(std::move(E))); }
+  Error error(Error &&E) {
+    handleAllErrors(std::move(E), [&](const InstrProfError &IPE) {
+      LastError = IPE.get();
+      LastErrorMsg = IPE.getMessage();
+    });
+    return make_error<InstrProfError>(LastError, LastErrorMsg);
+  }
 
   /// Clear the current error and return a successful one.
   Error success() { return error(instrprof_error::success); }
 
 public:
   /// Return true if the reader has finished reading the profile data.
   bool isEOF() { return LastError == instrprof_error::eof; }
 
   /// Return true if the reader encountered an error reading profiling data.
   bool hasError() { return LastError != instrprof_error::success && !isEOF(); }
 
   /// Get the current error.
   Error getError() {
     if (hasError())
-      return make_error<InstrProfError>(LastError);
+      return make_error<InstrProfError>(LastError, LastErrorMsg);
     return Error::success();
   }
 
   /// Factory method to create an appropriately typed reader for the given
   /// instrprof file.
   static Expected<std::unique_ptr<InstrProfReader>> create(const Twine &Path);
 
   static Expected<std::unique_ptr<InstrProfReader>>

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

Error insertFunctionRecordIfNeeded(const FuncRecordType *CFR,
     NameRefType NameRef = CFR->template getFuncNameRef<Endian>();
     auto InsertResult =
         FunctionRecords.insert(std::make_pair(NameRef, Records.size()));
     if (InsertResult.second) {
       StringRef FuncName;
       if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
         return Err;
       if (FuncName.empty())
-        return make_error<InstrProfError>(instrprof_error::malformed);
+        return make_error<InstrProfError>(instrprof_error::malformed,
+                                          "function name is empty");
       ++CovMapNumUsedRecords;
       Records.emplace_back(Version, FuncName, FuncHash, Mapping,
                            FileRange.StartingIndex, FileRange.Length);
       return Error::success();
     }
     // Update the existing record if it's a dummy and the new record is real.
     size_t OldRecordIndex = InsertResult.first->second;
     BinaryCoverageReader::ProfileMappingRecord &OldRecord =
         Records[OldRecordIndex];

llvm/lib/ProfileData/InstrProf.cpp

 // Note current ThinLTO module importing for the indirect-calls assumes
 // the source directory name not being stripped. A non-zero option value here
 // can potentially prevent some inter-module indirect-call-promotions.
 static cl::opt<unsigned> StaticFuncStripDirNamePrefix(
     "static-func-strip-dirname-prefix", cl::init(0), cl::Hidden,
     cl::desc("Strip specified level of directory name from source path in "
              "the profile counter name for static functions."));
 
-static std::string getInstrProfErrString(instrprof_error Err) {
+static std::string getInstrProfErrString(instrprof_error Err,
+                                         const std::string &ErrMsg = "") {
+  std::string Msg;
+  raw_string_ostream OS(Msg);
+
   switch (Err) {
   case instrprof_error::success:
-    return "success";
+    OS << "success";
+    break;
   case instrprof_error::eof:
-    return "end of File";
+    OS << "end of File";
+    break;
   case instrprof_error::unrecognized_format:
-    return "unrecognized instrumentation profile encoding format";
+    OS << "unrecognized instrumentation profile encoding format";
+    break;
   case instrprof_error::bad_magic:
-    return "invalid instrumentation profile data (bad magic)";
+    OS << "invalid instrumentation profile data (bad magic)";
+    break;
   case instrprof_error::bad_header:
-    return "invalid instrumentation profile data (file header is corrupt)";
+    OS << "invalid instrumentation profile data (file header is corrupt)";
+    break;
   case instrprof_error::unsupported_version:
-    return "unsupported instrumentation profile format version";
+    OS << "unsupported instrumentation profile format version";
+    break;
   case instrprof_error::unsupported_hash_type:
-    return "unsupported instrumentation profile hash type";
+    OS << "unsupported instrumentation profile hash type";
+    break;
   case instrprof_error::too_large:
-    return "too much profile data";
+    OS << "too much profile data";
+    break;
   case instrprof_error::truncated:
-    return "truncated profile data";
+    OS << "truncated profile data";
+    break;
   case instrprof_error::malformed:
-    return "malformed instrumentation profile data";
+    OS << "malformed instrumentation profile data";
+    break;
   case instrprof_error::invalid_prof:
-    return "invalid profile created. Please file a bug "
-           "at: " BUG_REPORT_URL
-           " and include the profraw files that caused this error.";
+    OS << "invalid profile created. Please file a bug "
+    break;
   case instrprof_error::unknown_function:
-    return "no profile data available for function";
+    OS << "no profile data available for function";
+    break;
   case instrprof_error::hash_mismatch:
-    return "function control flow change detected (hash mismatch)";
+    OS << "function control flow change detected (hash mismatch)";
+    break;
   case instrprof_error::count_mismatch:
-    return "function basic block count change detected (counter mismatch)";
+    OS << "function basic block count change detected (counter mismatch)";
+    break;
   case instrprof_error::counter_overflow:
-    return "counter overflow";
+    OS << "counter overflow";
+    break;
   case instrprof_error::value_site_count_mismatch:
-    return "function value site count change detected (counter mismatch)";
+    OS << "function value site count change detected (counter mismatch)";
+    break;
   case instrprof_error::compress_failed:
-    return "failed to compress data (zlib)";
+    OS << "failed to compress data (zlib)";
+    break;
   case instrprof_error::uncompress_failed:
-    return "failed to uncompress data (zlib)";
+    OS << "failed to uncompress data (zlib)";
+    break;
   case instrprof_error::empty_raw_profile:
-    return "empty raw profile file";
+    OS << "empty raw profile file";
+    break;
   case instrprof_error::zlib_unavailable:
-    return "profile uses zlib compression but the profile reader was built "
-           "without zlib support";
-  }
+    OS << "profile uses zlib compression but the profile reader was built "
+    break;
+  default:
     llvm_unreachable("A value of instrprof_error has no message.");
+    break;
+  }
+
+  // If optional error message is not empty, append it to the message.
+  if (!ErrMsg.empty())
+    OS << " : " << ErrMsg;
+
+  return OS.str();
 }
 
 namespace {
 
 // FIXME: This class is only here to support the transition to llvm::Error. It
 // will be removed once this transition is complete. Clients should prefer to
 // deal with the Error value directly, rather than converting to error_code.
 class InstrProfErrorCategoryType : public std::error_category {
case instrprof_error::value_site_count_mismatch:
     ++NumValueSiteCountMismatches;
     break;
   default:
     llvm_unreachable("Not a soft error");
   }
 }
 
 std::string InstrProfError::message() const {
-  return getInstrProfErrString(Err);
+  return getInstrProfErrString(Err, Msg);
 }
 
 char InstrProfError::ID = 0;
 
 std::string getPGOFuncName(StringRef RawFuncName,
                            GlobalValue::LinkageTypes Linkage,
                            StringRef FileName,
                            uint64_t Version LLVM_ATTRIBUTE_UNUSED) {

 
 static std::unique_ptr<ValueProfData> allocValueProfData(uint32_t TotalSize) {
   return std::unique_ptr<ValueProfData>(new (::operator new(TotalSize))
                                             ValueProfData());
 }
 
 Error ValueProfData::checkIntegrity() {
   if (NumValueKinds > IPVK_Last + 1)
-    return make_error<InstrProfError>(instrprof_error::malformed);
-  // Total size needs to be mulltiple of quadword size.
+    return make_error<InstrProfError>(
+        instrprof_error::malformed, "number of value profile kinds is invalid");
+  // Total size needs to be multiple of quadword size.
   if (TotalSize % sizeof(uint64_t))
-    return make_error<InstrProfError>(instrprof_error::malformed);
+    return make_error<InstrProfError>(
+        instrprof_error::malformed, "total size is not multiples of quardword");
 
   ValueProfRecord *VR = getFirstValueProfRecord(this);
   for (uint32_t K = 0; K < this->NumValueKinds; K++) {
     if (VR->Kind > IPVK_Last)
-      return make_error<InstrProfError>(instrprof_error::malformed);
+      return make_error<InstrProfError>(instrprof_error::malformed,
+                                        "value kind is invalid");
     VR = getValueProfRecordNext(VR);
     if ((char *)VR - (char *)this > (ptrdiff_t)TotalSize)
-      return make_error<InstrProfError>(instrprof_error::malformed);
+      return make_error<InstrProfError>(
+          instrprof_error::malformed,
+          "value profile address is greater than total size");
   }
   return Error::success();
 }
 
 Expected<std::unique_ptr<ValueProfData>>
 ValueProfData::getValueProfData(const unsigned char *D,
                                 const unsigned char *const BufferEnd,
                                 support::endianness Endianness) {

llvm/lib/ProfileData/InstrProfReader.cpp

if (Line.is_at_end())
     return success();
 
   uint32_t NumValueKinds;
   if (Line->getAsInteger(10, NumValueKinds)) {
     // No value profile data
     return success();
   }
   if (NumValueKinds == 0 || NumValueKinds > IPVK_Last + 1)
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed,
+                 "number of value kinds is invalid");
   Line++;
 
   for (uint32_t VK = 0; VK < NumValueKinds; VK++) {
     VP_READ_ADVANCE(ValueKind);
     if (ValueKind > IPVK_Last)
-      return error(instrprof_error::malformed);
+      return error(instrprof_error::malformed, "value kind is invalid");
+    ;
     VP_READ_ADVANCE(NumValueSites);
     if (!NumValueSites)
       continue;
 
     Record.reserveSites(VK, NumValueSites);
     for (uint32_t S = 0; S < NumValueSites; S++) {
       VP_READ_ADVANCE(NumValueData);
 
Error TextInstrProfReader::readNextRecord(NamedInstrProfRecord &Record) {
   Record.Name = *Line++;
   if (Error E = Symtab->addFuncName(Record.Name))
     return error(std::move(E));
 
   // Read the function hash.
   if (Line.is_at_end())
     return error(instrprof_error::truncated);
   if ((Line++)->getAsInteger(0, Record.Hash))
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed, "function hash is invalid");
 
   // Read the number of counters.
   uint64_t NumCounters;
   if (Line.is_at_end())
     return error(instrprof_error::truncated);
   if ((Line++)->getAsInteger(10, NumCounters))
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed, "number of counters is invalid");
   if (NumCounters == 0)
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed, "number of counters is zero");
 
   // Read each counter and fill our internal storage with the values.
   Record.Clear();
   Record.Counts.reserve(NumCounters);
   for (uint64_t I = 0; I < NumCounters; ++I) {
     if (Line.is_at_end())
       return error(instrprof_error::truncated);
     uint64_t Count;
     if ((Line++)->getAsInteger(10, Count))
-      return error(instrprof_error::malformed);
+      return error(instrprof_error::malformed, "count is invalid");
     Record.Counts.push_back(Count);
   }
 
   // Check if value profile data exists and read it if so.
   if (Error E = readValueProfileData(Record))
     return error(std::move(E));
 
   return success();
Error RawInstrProfReader<IntPtrT>::readNextHeader(const char *CurrentPos) {
   while (CurrentPos != End && *CurrentPos == 0)
     ++CurrentPos;
   // If there's nothing left, we're done.
   if (CurrentPos == End)
     return make_error<InstrProfError>(instrprof_error::eof);
   // If there isn't enough space for another header, this is probably just
   // garbage at the end of the file.
   if (CurrentPos + sizeof(RawInstrProf::Header) > End)
-    return make_error<InstrProfError>(instrprof_error::malformed);
+    return make_error<InstrProfError>(instrprof_error::malformed,
+                                      "not enough space for another header");
   // The writer ensures each profile is padded to start at an aligned address.
   if (reinterpret_cast<size_t>(CurrentPos) % alignof(uint64_t))
-    return make_error<InstrProfError>(instrprof_error::malformed);
+    return make_error<InstrProfError>(instrprof_error::malformed,
+                                      "padding is missing");
   // The magic should have the same byte order as in the previous header.
   uint64_t Magic = *reinterpret_cast<const uint64_t *>(CurrentPos);
   if (Magic != swap(RawInstrProf::getMagic<IntPtrT>()))
     return make_error<InstrProfError>(instrprof_error::bad_magic);
 
   // There's another profile to read, so we need to process the header.
   auto *Header = reinterpret_cast<const RawInstrProf::Header *>(CurrentPos);
   return readHeader(*Header);

 }
 
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readRawCounts(
     InstrProfRecord &Record) {
   uint32_t NumCounters = swap(Data->NumCounters);
   IntPtrT CounterPtr = Data->CounterPtr;
   if (NumCounters == 0)
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed, "number of counters is zero");
 
   auto *NamesStartAsCounter = reinterpret_cast<const uint64_t *>(NamesStart);
   ptrdiff_t MaxNumCounters = NamesStartAsCounter - CountersStart;
 
   // Check bounds. Note that the counter pointer embedded in the data record
   // may itself be corrupt.
   if (MaxNumCounters < 0 || NumCounters > (uint32_t)MaxNumCounters)
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed, "counter pointer is invalid");
 
   // We need to compute the in-buffer counter offset from the in-memory address
   // distance. The initial CountersDelta is the in-memory address difference
   // start(__llvm_prf_cnts)-start(__llvm_prf_data), so SrcData->CounterPtr -
   // CountersDelta computes the offset into the in-buffer counter section.
   //
   // CountersDelta decreases as we advance to the next data record.
   ptrdiff_t CounterOffset = getCounterOffset(CounterPtr);
   CountersDelta -= sizeof(*Data);
   if (CounterOffset < 0 || CounterOffset > MaxNumCounters ||
       ((uint32_t)CounterOffset + NumCounters) > (uint32_t)MaxNumCounters)
-    return error(instrprof_error::malformed);
+    return error(instrprof_error::malformed, "counter offset is invalid");
 
   auto RawCounts = makeArrayRef(getCounter(CounterOffset), NumCounters);
 
   if (ShouldSwapBytes) {
     Record.Counts.clear();
     Record.Counts.reserve(RawCounts.size());
     for (uint64_t Count : RawCounts)
       Record.Counts.push_back(swap(Count));
   } else
     Record.Counts = RawCounts;
 
   return success();
 }
 
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readValueProfilingData(
     InstrProfRecord &Record) {
   Record.clearValueData();
   CurValueDataSize = 0;
   // Need to match the logic in value profile dumper code in compiler-rt:
   uint32_t NumValueKinds = 0;
   for (uint32_t I = 0; I < IPVK_Last + 1; I++)
     NumValueKinds += (Data->NumValueSites[I] != 0);
 
   if (!NumValueKinds)
     return success();
Error RawInstrProfReader<IntPtrT>::printBinaryIds(raw_ostream &OS) {
   OS << "Binary IDs: \n";
   const uint8_t *BI = BinaryIdsStart;
   const uint8_t *BIEnd = BinaryIdsStart + BinaryIdsSize;
   while (BI < BIEnd) {
     size_t Remaining = BIEnd - BI;
 
     // There should be enough left to read the binary ID size field.
     if (Remaining < sizeof(uint64_t))
-      return make_error<InstrProfError>(instrprof_error::malformed);
+      return make_error<InstrProfError>(
+          instrprof_error::malformed,
+          "not enough space to read binary id length");
 
     uint64_t BinaryIdLen = swap(*reinterpret_cast<const uint64_t *>(BI));
 
     // There should be enough left to read the binary ID size field, and the
     // binary ID.
     if (Remaining < sizeof(BinaryIdLen) + BinaryIdLen)
-      return make_error<InstrProfError>(instrprof_error::malformed);
+      return make_error<InstrProfError>(
+          instrprof_error::malformed,
+          "not enough space to read binary id data");
 
     // Increment by binary id length data type size.
     BI += sizeof(BinaryIdLen);
     if (BI > (const uint8_t *)DataBuffer->getBufferEnd())
-      return make_error<InstrProfError>(instrprof_error::malformed);
+      return make_error<InstrProfError>(
+          instrprof_error::malformed,
+          "binary id that is read is bigger than buffer size");
 
     for (uint64_t I = 0; I < BinaryIdLen; I++)
       OS << format("%02x", BI[I]);
     OS << "\n";
 
     // Increment by binary id data length, rounded to the next 8 bytes. This
     // accounts for the zero-padding after each build ID.
     BI += RoundUp(BinaryIdLen, sizeof(uint64_t));

 Error InstrProfReaderIndex<HashTableImpl>::getRecords(
     StringRef FuncName, ArrayRef<NamedInstrProfRecord> &Data) {
   auto Iter = HashTable->find(FuncName);
   if (Iter == HashTable->end())
     return make_error<InstrProfError>(instrprof_error::unknown_function);
 
   Data = (*Iter);
   if (Data.empty())
-    return make_error<InstrProfError>(instrprof_error::malformed);
+    return make_error<InstrProfError>(instrprof_error::malformed,
+                                      "profile data is empty");
 
   return Error::success();
 }
 
 template <typename HashTableImpl>
 Error InstrProfReaderIndex<HashTableImpl>::getRecords(
     ArrayRef<NamedInstrProfRecord> &Data) {
   if (atEnd())
     return make_error<InstrProfError>(instrprof_error::eof);
 
   Data = *RecordIterator;
 
   if (Data.empty())
-    return make_error<InstrProfError>(instrprof_error::malformed);
+    return make_error<InstrProfError>(instrprof_error::malformed,
+                                      "profile data is empty");
 
   return Error::success();
 }
 
 template <typename HashTableImpl>
 InstrProfReaderIndex<HashTableImpl>::InstrProfReaderIndex(
     const unsigned char *Buckets, const unsigned char *const Payload,
     const unsigned char *const Base, IndexedInstrProf::HashT HashType,
public:
   InstrProfReaderNullRemapper(InstrProfReaderIndexBase &Underlying)
       : Underlying(Underlying) {}
 
   Error getRecords(StringRef FuncName,
                    ArrayRef<NamedInstrProfRecord> &Data) override {
     return Underlying.getRecords(FuncName, Data);
   }
 };
-}
+} // namespace
 
 /// A remapper that applies remappings based on a symbol remapping file.
 template <typename HashTableImpl>
 class llvm::InstrProfReaderItaniumRemapper
     : public InstrProfReaderRemapper {
 public:
   InstrProfReaderItaniumRemapper(
       std::unique_ptr<MemoryBuffer> RemapBuffer,

llvm/test/tools/llvm-profdata/large-binary-id-size.test

 
 // Check for a corrupted size being too large past the end of the file.
 RUN: printf '\7\7\7\7\7\7\7\7' >> %t.profraw
 RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
 RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
 RUN: printf '\0\1\2\3\0\0\0\0' >> %t.profraw
 
 // RUN: not llvm-profdata show --binary-ids  %t.profraw 2>&1 | FileCheck %s
-// CHECK: malformed instrumentation profile data
+// CHECK: malformed instrumentation profile data : not enough space to read binary id data

llvm/test/tools/llvm-profdata/malformed-not-space-for-another-header.test

+// Header
+//
+// INSTR_PROF_RAW_HEADER(uint64_t, Magic, __llvm_profile_get_magic())
+// INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
+// INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
+// INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, NamesSize,  NamesSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, CountersDelta, (uintptr_t)CountersBegin)
+// INSTR_PROF_RAW_HEADER(uint64_t, NamesDelta, (uintptr_t)NamesBegin)
+// INSTR_PROF_RAW_HEADER(uint64_t, ValueKindLast, IPVK_Last)
+
+RUN: printf '\201rforpl\377' > %t.profraw
+RUN: printf '\x8\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\xe\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\xf8\xff\xff\xff\xff\xff\xff\xff' >> %t.profraw
+RUN: printf '\x89\x7a\x40\x00\x00\x00\x00\x00' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+
+// Data Section
+//
+// struct ProfData {
+// #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) \
+//    Type Name;
+// #include "llvm/ProfileData/InstrProfData.inc"
+// };
+
+RUN: printf '\xfa\xd5\x8d\xe7\x36\x64\x95\xdb' >> %t.profraw // NameRef
+RUN: printf '\x18\0\0\0\0\0\0\0' >> %t.profraw               // FuncHash
+RUN: printf '\xf8\xff\xff\xff\xff\xff\xff\xff' >> %t.profraw // RelativeCounterPtr
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw                 // FunctionPointer
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw                 // Values
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw                 // NumCounters and NumValueSites
+
+// Counter section
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+
+// Name section (Name section is 14 bytes and 2 bytes padding is added)
+RUN: printf '\x04\x0c\x78\xda\xcb\x4d\xcc\xcc' >> %t.profraw
+RUN: printf '\x03\x00\x04\x1b\x01\xa6\x00\x00' >> %t.profraw
+
+// Write some garbage data at the end so we get "not enough space for another header" message
+RUN: printf '\x03\x00\' >> %t.profraw
+
+RUN: not llvm-profdata show %t.profraw 2>&1 | FileCheck %s
+CHECK: malformed instrumentation profile data : not enough space for another header

llvm/test/tools/llvm-profdata/malformed-num-counters-zero.test

+// Header
+//
+// INSTR_PROF_RAW_HEADER(uint64_t, Magic, __llvm_profile_get_magic())
+// INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
+// INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
+// INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, NamesSize,  NamesSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, CountersDelta, (uintptr_t)CountersBegin)
+// INSTR_PROF_RAW_HEADER(uint64_t, NamesDelta, (uintptr_t)NamesBegin)
+// INSTR_PROF_RAW_HEADER(uint64_t, ValueKindLast, IPVK_Last)
+
+RUN: printf '\201rforpl\377' > %t.profraw
+RUN: printf '\x8\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\xe\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\xf8\xff\xff\xff\xff\xff\xff\xff' >> %t.profraw
+RUN: printf '\x89\x7a\x40\x00\x00\x00\x00\x00' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+
+// Data Section
+//
+// struct ProfData {
+// #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) \
+//    Type Name;
+// #include "llvm/ProfileData/InstrProfData.inc"
+// };
+
+RUN: printf '\xfa\xd5\x8d\xe7\x36\x64\x95\xdb' >> %t.profraw // NameRef
+RUN: printf '\x18\0\0\0\0\0\0\0' >> %t.profraw               // FuncHash
+RUN: printf '\xf8\xff\xff\xff\xff\xff\xff\xff' >> %t.profraw // RelativeCounterPtr
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw                 // FunctionPointer
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw                 // Values
+// Make NumCounters = 0 so that we get "number of counters is zero" error message
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw                 // NumCounters and NumValueSites
+
+// Counter section
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+
+// Name section (Name section is 14 bytes and 2 bytes padding is added)
+RUN: printf '\x04\x0c\x78\xda\xcb\x4d\xcc\xcc' >> %t.profraw
+RUN: printf '\x03\x00\x04\x1b\x01\xa6\x00\x00' >> %t.profraw
+
+RUN: not llvm-profdata show %t.profraw 2>&1 | FileCheck %s
+CHECK: malformed instrumentation profile data : number of counters is zero

llvm/test/tools/llvm-profdata/malformed-ptr-to-counter-array.test

 RUN: printf '\067\0\0\0\0\0\0\0' >> %t.profraw
 RUN: printf '\101\0\0\0\0\0\0\0' >> %t.profraw
 
 // Name Section
 
 RUN: printf '\3\0bar\0\0\0' >> %t.profraw
 
 RUN: not llvm-profdata merge -o /dev/null %t.profraw 2>&1 | FileCheck %s
-CHECK: warning: {{.+}}: malformed instrumentation profile data
+CHECK: warning: {{.+}}: malformed instrumentation profile data : counter offset is invalid
 CHECK: error: no profile can be merged