[llvm-profdata] Fixed various issue with Sample Profile Reader

[huangjd]

Summary

Fixed various undefind behaviors with current Sample Profile Reader when reading unusual input. Furthermore, add the following rule on allowing multiple name table sections (current Reader has conflicted code handling such case):

When a new name table section is read (in the order sections are read), the names in the previous name table are cleared. Any subsequent sections referring to function names will index into the most recent read name table.

Also changed name table index to uint64_t to be consistent since there's a mix of using uint32_t and uint64_t.

[snehasish]

Can you split out the uint64_t type changes into a separate patch?

llvm/lib/ProfileData/SampleProfReader.cpp

1069

Just to confirm, the reason this is safe for existing profiles on disk is because the underlying number is uleb128 encoded?

[huangjd]

Separate patch into two

1. Use correct integral type when reading the profile

[huangjd]

2. D146182

[snehasish]

lgtm since I think this change is safe for existing profiles. @huangjd please confirm before submitting, thanks!

llvm/include/llvm/ProfileData/SampleProf.h

 // and SampleProfileExtBinaryBaseWriter.
 struct SecHdrTableEntry {
   SecType Type;
   uint64_t Flags;
   uint64_t Offset;
   uint64_t Size;
   // The index indicating the location of the current entry in
   // SectionHdrLayout table.
-  uint32_t LayoutIndex;
+  uint64_t LayoutIndex;
 };
 
 // Flags common for all sections are defined here. In SecHdrTableEntry::Flags,
 // common flags will be saved in the lower 32bits and section specific flags
 // will be saved in the higher 32 bits.
 enum class SecCommonFlags : uint32_t {
   SecFlagInValid = 0,
   SecFlagCompress = (1 << 0),

llvm/include/llvm/ProfileData/SampleProfReader.h

 //            multiplied by 1000000.
 //        MIN_COUNT (uint64_t)
 //            The minimum number of samples required to reach the target
 //            CUTOFF.
 //        NUM_COUNTS (uint64_t)
 //            Number of samples to get to the desrired percentile.
 //
 // NAME TABLE
-//    SIZE (uint32_t)
+//    SIZE (uint64_t)
 //        Number of entries in the name table.
 //    NAMES
 //        A NUL-separated list of SIZE strings.
 //
 // FUNCTION BODY (one for each uninlined function body present in the profile)
 //    HEAD_SAMPLES (uint64_t) [only for top-level functions]
 //        Total number of samples collected at the head (prologue) of the
 //        function.
 //        NOTE: This field should only be present for top-level functions
 //              (i.e., not inlined into any caller). Inlined function calls
 //              have no prologue, so they don't need this.
-//    NAME_IDX (uint32_t)
+//    NAME_IDX (uint64_t)
 //        Index into the name table indicating the function name.
 //    SAMPLES (uint64_t)
 //        Total number of samples collected in this function.
 //    NRECS (uint32_t)
 //        Total number of sampling records this function's profile.
 //    BODY RECORDS
 //        A list of NRECS entries. Each entry contains:
 //          OFFSET (uint32_t)
 //            Line offset from the start of the function.
 //          DISCRIMINATOR (uint32_t)
 //            Discriminator value (see description of discriminators
 //            in the text format documentation above).
 //          SAMPLES (uint64_t)
 //            Number of samples collected at this location.
 //          NUM_CALLS (uint32_t)
 //            Number of non-inlined function calls made at this location. In the
 //            case of direct calls, this number will always be 1. For indirect
 //            calls (virtual functions and function pointers) this will
 //            represent all the actual functions called at runtime.
 //          CALL_TARGETS
 //            A list of NUM_CALLS entries for each called function:
-//               NAME_IDX (uint32_t)
+//               NAME_IDX (uint64_t)
 //                  Index into the name table with the callee name.
 //               SAMPLES (uint64_t)
 //                  Number of samples collected at the call site.
 //    NUM_INLINED_FUNCTIONS (uint32_t)
 //      Number of callees inlined into this function.
 //    INLINED FUNCTION RECORDS
 //      A list of NUM_INLINED_FUNCTIONS entries describing each of the inlined
 //      callees.
protected:
   ///
   /// If an error occurs during decoding, a diagnostic message is emitted and
   /// EC is set.
   ///
   /// \returns the read value.
   ErrorOr<StringRef> readString();
 
   /// Read the string index and check whether it overflows the table.
-  template <typename T> inline ErrorOr<uint32_t> readStringIndex(T &Table);
+  template <typename T> inline ErrorOr<size_t> readStringIndex(T &Table);
 
   /// Return true if we've reached the end of file.
   bool at_eof() const { return Data >= End; }
 
   /// Read the next function profile instance.
   std::error_code readFuncProfile(const uint8_t *Start);
 
   /// Read the contents of the given profile instance.
std::error_code decompressSection(const uint8_t *SecStart,
                                     const uint64_t SecSize,
                                     const uint8_t *&DecompressBuf,
                                     uint64_t &DecompressBufSize);
 
   BumpPtrAllocator Allocator;
 
 protected:
   std::vector<SecHdrTableEntry> SecHdrTable;
-  std::error_code readSecHdrTableEntry(uint32_t Idx);
+  std::error_code readSecHdrTableEntry(uint64_t Idx);
   std::error_code readSecHdrTable();
 
   std::error_code readFuncMetadata(bool ProfileHasAttribute);
   std::error_code readFuncMetadata(bool ProfileHasAttribute,
                                    FunctionSamples *FProfile);
   std::error_code readFuncOffsetTable();
   std::error_code readFuncProfiles();
   std::error_code readMD5NameTable();

llvm/lib/ProfileData/SampleProfReader.cpp

ErrorOr<T> SampleProfileReaderBinary::readUnencodedNumber() {
   }
 
   using namespace support;
   T Val = endian::readNext<T, little, unaligned>(Data);
   return Val;
 }
 
 template <typename T>
-inline ErrorOr<uint32_t> SampleProfileReaderBinary::readStringIndex(T &Table) {
+inline ErrorOr<size_t> SampleProfileReaderBinary::readStringIndex(T &Table) {
   std::error_code EC;
-  auto Idx = readNumber<uint32_t>();
+  auto Idx = readNumber<size_t>();
   if (std::error_code EC = Idx.getError())
     return EC;
   if (*Idx >= Table.size())
     return sampleprof_error::truncated_name_table;
   return *Idx;
 }
 
 ErrorOr<StringRef> SampleProfileReaderBinary::readStringFromTable() {
if (std::error_code EC = readFuncProfile(Data))
       return EC;
   }
 
   return sampleprof_error::success;
 }
 
 ErrorOr<SampleContextFrames>
 SampleProfileReaderExtBinaryBase::readContextFromTable() {
-  auto ContextIdx = readNumber<uint32_t>();
+  auto ContextIdx = readNumber<size_t>();
   if (std::error_code EC = ContextIdx.getError())
     return EC;
   if (*ContextIdx >= CSNameTable->size())
     return sampleprof_error::truncated_name_table;
   return (*CSNameTable)[*ContextIdx];
 }
 
 ErrorOr<SampleContext>

 std::error_code
 SampleProfileReaderCompactBinary::verifySPMagic(uint64_t Magic) {
   if (Magic == SPMagic(SPF_Compact_Binary))
     return sampleprof_error::success;
   return sampleprof_error::bad_magic;
 }
 
 std::error_code SampleProfileReaderBinary::readNameTable() {
-  auto Size = readNumber<uint32_t>();
+  auto Size = readNumber<size_t>();
   if (std::error_code EC = Size.getError())
     return EC;
   NameTable.reserve(*Size + NameTable.size());
-  for (uint32_t I = 0; I < *Size; ++I) {
+  for (size_t I = 0; I < *Size; ++I) {
     auto Name(readString());
     if (std::error_code EC = Name.getError())
       return EC;
     NameTable.push_back(*Name);
   }
 
   return sampleprof_error::success;
 }
std::error_code SampleProfileReaderExtBinaryBase::readNameTableSec(bool IsMD5) {
   return SampleProfileReaderBinary::readNameTable();
 }
 
 // Read in the CS name table section, which basically contains a list of context
 // vectors. Each element of a context vector, aka a frame, refers to the
 // underlying raw function names that are stored in the name table, as well as
 // a callsite identifier that only makes sense for non-leaf frames.
 std::error_code SampleProfileReaderExtBinaryBase::readCSNameTableSec() {
-  auto Size = readNumber<uint32_t>();
+  auto Size = readNumber<size_t>();
   if (std::error_code EC = Size.getError())
     return EC;
 
   std::vector<SampleContextFrameVector> *PNameVec =
       new std::vector<SampleContextFrameVector>();
   PNameVec->reserve(*Size);
-  for (uint32_t I = 0; I < *Size; ++I) {
+  for (size_t I = 0; I < *Size; ++I) {
     PNameVec->emplace_back(SampleContextFrameVector());
     auto ContextSize = readNumber<uint32_t>();
     if (std::error_code EC = ContextSize.getError())
       return EC;
     for (uint32_t J = 0; J < *ContextSize; ++J) {
       auto FName(readStringFromTable());
       if (std::error_code EC = FName.getError())
         return EC;
for (uint64_t I = 0; I < *Size; ++I) {
     if (std::error_code EC = FID.getError())
       return EC;
     NameTable.push_back(std::to_string(*FID));
   }
   return sampleprof_error::success;
 }
 
 std::error_code
-SampleProfileReaderExtBinaryBase::readSecHdrTableEntry(uint32_t Idx) {
+SampleProfileReaderExtBinaryBase::readSecHdrTableEntry(uint64_t Idx) {
   SecHdrTableEntry Entry;
   auto Type = readUnencodedNumber<uint64_t>();
   if (std::error_code EC = Type.getError())
     return EC;
   Entry.Type = static_cast<SecType>(*Type);
 
   auto Flags = readUnencodedNumber<uint64_t>();
   if (std::error_code EC = Flags.getError())