[InstrProf] Encapsulates access to AddrToMD5Map.

[mtrofin]

Summary

This fixes a unittest failure introduced by D44717

D44717 introduced lazy sorting of the internal data structures of the
symbol table. The AddrToMD5Map getter was potentially exposing
inconsistent (unsorted) state. We could sort in the accessor, however,
a client may store the pointer and thus bypass the internal state
management of the symbol table. The alternative in this CL blocks
direct access to the state, thus ensuring consistent
externally-observable state.

[mtrofin]

Removed windows fix, since it was already addressed in r328157

[xur]

lgtm

include/llvm/ProfileData/InstrProf.h

public:
   }
 
   /// Map a function address to its name's MD5 hash. This interface
   /// is only used by the raw profiler reader.
   void mapAddress(uint64_t Addr, uint64_t MD5Val) {
     AddrToMD5Map.push_back(std::make_pair(Addr, MD5Val));
   }
 
-  AddrHashMap &getAddrHashMap() { return AddrToMD5Map; }
+  /// Return a function's hash, or 0, if the function isn't in this SymTab.
+  uint64_t getFunctionHashFromAddress(uint64_t Address);
 
   /// Return function's PGO name from the function name's symbol
   /// address in the object file. If an error occurs, return
   /// an empty string.
   StringRef getFuncName(uint64_t FuncNameAddress, size_t NameSize);
 
   /// Return function's PGO name from the name's md5 hash value.
   /// If not found, return an empty string.
InstrProfRecord &operator=(const InstrProfRecord &RHS) {
     }
     if (!ValueData)
       ValueData = llvm::make_unique<ValueProfData>(*RHS.ValueData);
     else
       *ValueData = *RHS.ValueData;
     return *this;
   }
 
-  using ValueMapType = std::vector<std::pair<uint64_t, uint64_t>>;
-
   /// Return the number of value profile kinds with non-zero number
   /// of profile sites.
   inline uint32_t getNumValueKinds() const;
   /// Return the number of instrumented sites for ValueKind.
   inline uint32_t getNumValueSites(uint32_t ValueKind) const;
 
   /// Return the total number of ValueData for ValueKind.
   inline uint32_t getNumValueData(uint32_t ValueKind) const;
inline uint64_t getValueForSite(InstrProfValueData Dest[], uint32_t ValueKind,
                                   uint32_t Site) const;
 
   /// Reserve space for NumValueSites sites.
   inline void reserveSites(uint32_t ValueKind, uint32_t NumValueSites);
 
   /// Add ValueData for ValueKind at value Site.
   void addValueData(uint32_t ValueKind, uint32_t Site,
                     InstrProfValueData *VData, uint32_t N,
-                    ValueMapType *ValueMap);
+                    InstrProfSymtab *SymTab);
 
   /// Merge the counts in \p Other into this one.
   /// Optionally scale merged counts by \p Weight.
   void merge(InstrProfRecord &Other, uint64_t Weight,
              function_ref<void(instrprof_error)> Warn);
 
   /// Scale up profile counts (including value profile data) by
   /// \p Weight.
case IPVK_MemOPSize:
       return ValueData->MemOPSizes;
     default:
       llvm_unreachable("Unknown value kind!");
     }
   }
 
   // Map indirect call target name hash to name string.
   uint64_t remapValue(uint64_t Value, uint32_t ValueKind,
-                      ValueMapType *HashKeys);
+                      InstrProfSymtab *SymTab);
 
   // Merge Value Profile data from Src record to this record for ValueKind.
   // Scale merged value counts by \p Weight.
   void mergeValueProfData(uint32_t ValkeKind, InstrProfRecord &Src,
                           uint64_t Weight,
                           function_ref<void(instrprof_error)> Warn);
 
   // Scale up value profile data count.

include/llvm/ProfileData/InstrProfData.inc

#ifdef __cplusplus
   /*!
    * \brief Return the number of value sites.
    */
   uint32_t getNumValueSites() const { return NumValueSites; }
   /*!
    * \brief Read data from this record and save it to Record.
    */
   void deserializeTo(InstrProfRecord &Record,
-                     InstrProfRecord::ValueMapType *VMap);
+                     InstrProfSymtab *SymTab);
   /*
    * In-place byte swap:
    * Do byte swap for this instance. \c Old is the original order before
    * the swap, and \c New is the New byte order.
    */
   void swapBytes(support::endianness Old, support::endianness New);
 #endif
 } ValueProfRecord;
#if __cplusplus
   /*!
    * Return the total size of \c ValueProfileData.
    */
   uint32_t getSize() const { return TotalSize; }
   /*!
    * Read data from this data and save it to \c Record.
    */
   void deserializeTo(InstrProfRecord &Record,
-                     InstrProfRecord::ValueMapType *VMap);
+                     InstrProfSymtab *SymTab);
   void operator delete(void *ptr) { ::operator delete(ptr); }
 #endif
 } ValueProfData;
 
 /*
  * The closure is designed to abstact away two types of value profile data:
  * - InstrProfRecord which is the primary data structure used to
  *   represent profile data in host tools (reader, writer, and profile-use)

include/llvm/ProfileData/InstrProfReader.h

private:
   const char *NamesStart;
   uint64_t NamesSize;
   // After value profile is all read, this pointer points to
   // the header of next profile data (if exists)
   const uint8_t *ValueDataStart;
   uint32_t ValueKindLast;
   uint32_t CurValueDataSize;
 
-  InstrProfRecord::ValueMapType FunctionPtrToNameMap;
-
 public:
   RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)
       : DataBuffer(std::move(DataBuffer)) {}
   RawInstrProfReader(const RawInstrProfReader &) = delete;
   RawInstrProfReader &operator=(const RawInstrProfReader &) = delete;
 
   static bool hasFormat(const MemoryBuffer &DataBuffer);
   Error readHeader() override;

lib/ProfileData/InstrProf.cpp

if (InLTO) {
       }
     }
   }
   Sorted = false;
   finalizeSymtab();
   return Error::success();
 }
 
+uint64_t InstrProfSymtab::getFunctionHashFromAddress(uint64_t Address) {
+  finalizeSymtab();
+  auto Result =
+      std::lower_bound(AddrToMD5Map.begin(), AddrToMD5Map.end(), Address,
+                       [](const std::pair<uint64_t, uint64_t> &LHS,
+                          uint64_t RHS) { return LHS.first < RHS; });
+  // Raw function pointer collected by value profiler may be from
+  // external functions that are not instrumented. They won't have
+  // mapping data to be used by the deserializer. Force the value to
+  // be 0 in this case.
+  if (Result != AddrToMD5Map.end() && Result->first == Address)
+    return (uint64_t)Result->second;
+  return 0;
+}
+
 Error collectPGOFuncNameStrings(ArrayRef<std::string> NameStrs,
                                 bool doCompression, std::string &Result) {
   assert(!NameStrs.empty() && "No name data to emit");
 
   uint8_t Header[16], *P = Header;
   std::string UncompressedNameStrings =
       join(NameStrs.begin(), NameStrs.end(), getInstrProfNameSeparator());
 
if (Overflowed)
       Warn(instrprof_error::counter_overflow);
   }
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     scaleValueProfData(Kind, Weight, Warn);
 }
 
 // Map indirect call target name hash to name string.
 uint64_t InstrProfRecord::remapValue(uint64_t Value, uint32_t ValueKind,
-                                     ValueMapType *ValueMap) {
-  if (!ValueMap)
+                                     InstrProfSymtab *SymTab) {
+  if (!SymTab)
     return Value;
-  switch (ValueKind) {
-  case IPVK_IndirectCallTarget: {
-    auto Result =
-        std::lower_bound(ValueMap->begin(), ValueMap->end(), Value,
-                         [](const std::pair<uint64_t, uint64_t> &LHS,
-                            uint64_t RHS) { return LHS.first < RHS; });
-   // Raw function pointer collected by value profiler may be from 
-   // external functions that are not instrumented. They won't have
-   // mapping data to be used by the deserializer. Force the value to
-   // be 0 in this case.
-    if (Result != ValueMap->end() && Result->first == Value)
-      Value = (uint64_t)Result->second;
-    else
-      Value = 0;
-    break;
-  }
-  }
+
+  if (ValueKind == IPVK_IndirectCallTarget)
+    return SymTab->getFunctionHashFromAddress(Value);
+
   return Value;
 }
 
 void InstrProfRecord::addValueData(uint32_t ValueKind, uint32_t Site,
                                    InstrProfValueData *VData, uint32_t N,
-                                   ValueMapType *ValueMap) {
+                                   InstrProfSymtab *ValueMap) {
   for (uint32_t I = 0; I < N; I++) {
     VData[I].Value = remapValue(VData[I].Value, ValueKind, ValueMap);
   }
   std::vector<InstrProfValueSiteRecord> &ValueSites =
       getOrCreateValueSitesForKind(ValueKind);
   if (N == 0)
     ValueSites.emplace_back();
   else
ValueProfData::serializeFrom(const InstrProfRecord &Record) {
   InstrProfRecordClosure.Record = &Record;
 
   std::unique_ptr<ValueProfData> VPD(
       serializeValueProfDataFrom(&InstrProfRecordClosure, nullptr));
   return VPD;
 }
 
 void ValueProfRecord::deserializeTo(InstrProfRecord &Record,
-                                    InstrProfRecord::ValueMapType *VMap) {
+                                    InstrProfSymtab *SymTab) {
   Record.reserveSites(Kind, NumValueSites);
 
   InstrProfValueData *ValueData = getValueProfRecordValueData(this);
   for (uint64_t VSite = 0; VSite < NumValueSites; ++VSite) {
     uint8_t ValueDataCount = this->SiteCountArray[VSite];
-    Record.addValueData(Kind, VSite, ValueData, ValueDataCount, VMap);
+    Record.addValueData(Kind, VSite, ValueData, ValueDataCount, SymTab);
     ValueData += ValueDataCount;
   }
 }
 
 // For writing/serializing,  Old is the host endianness, and  New is
 // byte order intended on disk. For Reading/deserialization, Old
 // is the on-disk source endianness, and New is the host endianness.
 void ValueProfRecord::swapBytes(support::endianness Old,
void ValueProfRecord::swapBytes(support::endianness Old,
   }
   if (getHostEndianness() == Old) {
     sys::swapByteOrder<uint32_t>(NumValueSites);
     sys::swapByteOrder<uint32_t>(Kind);
   }
 }
 
 void ValueProfData::deserializeTo(InstrProfRecord &Record,
-                                  InstrProfRecord::ValueMapType *VMap) {
+                                  InstrProfSymtab *SymTab) {
   if (NumValueKinds == 0)
     return;
 
   ValueProfRecord *VR = getFirstValueProfRecord(this);
   for (uint32_t K = 0; K < NumValueKinds; K++) {
-    VR->deserializeTo(Record, VMap);
+    VR->deserializeTo(Record, SymTab);
     VR = getValueProfRecordNext(VR);
   }
 }
 
 template <class T>
 static T swapToHostOrder(const unsigned char *&D, support::endianness Orig) {
   using namespace support;
 

lib/ProfileData/InstrProfReader.cpp

Expected<std::unique_ptr<ValueProfData>> VDataPtrOrErr =
           getDataEndianness());
 
   if (Error E = VDataPtrOrErr.takeError())
     return E;
 
   // Note that besides deserialization, this also performs the conversion for
   // indirect call targets.  The function pointers from the raw profile are
   // remapped into function name hashes.
-  VDataPtrOrErr.get()->deserializeTo(Record, &Symtab->getAddrHashMap());
+  VDataPtrOrErr.get()->deserializeTo(Record, Symtab.get());
   CurValueDataSize = VDataPtrOrErr.get()->getSize();
   return success();
 }
 
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readNextRecord(NamedInstrProfRecord &Record) {
   if (atEnd())
     // At this point, ValueDataStart field points to the next header.

test/tools/llvm-profdata/invalid-profdata.test

 RUN: echo "# NumValueSites:"                             >> %t.input
 RUN: echo "2"                                            >> %t.input
 RUN: echo "2"                                            >> %t.input
 RUN: echo "f1:10"                                        >> %t.input
 RUN: echo "f2:0"                                         >> %t.input
 RUN: echo "1"                                            >> %t.input
 RUN: echo ":10"                                          >> %t.input
 
-RUN: not llvm-profdata merge %t.input -text -output=/dev/null 2>&1  | FileCheck %s --check-prefix=BROKEN
+RUN: not llvm-profdata merge %t.input -text -output=%t.out.broken 2>&1  | FileCheck %s --check-prefix=BROKEN
 BROKEN: Malformed instrumentation profile data
 
 RUN: echo ":ir"                     > %t.input
 RUN: echo "_ZN6Thread5StartEv"      >> %t.input
 RUN: echo "# Func Hash:"            >> %t.input
 RUN: echo "288793635542036872"      >> %t.input
 RUN: echo "# Num Counters:"         >> %t.input
 RUN: echo "3"                       >> %t.input

unittests/ProfileData/InstrProfTest.cpp

TEST_P(MaybeSparseInstrProfTest, value_prof_data_read_write_mapping) {
   NamedInstrProfRecord Record("caller", 0x1234, {1ULL << 31, 2});
   InstrProfSymtab Symtab;
   Symtab.mapAddress(uint64_t(callee1), 0x1000ULL);
   Symtab.mapAddress(uint64_t(callee2), 0x2000ULL);
   Symtab.mapAddress(uint64_t(callee3), 0x3000ULL);
   Symtab.mapAddress(uint64_t(callee4), 0x4000ULL);
   // Missing mapping for callee5
 
-  VPData->deserializeTo(Record, &Symtab.getAddrHashMap());
+  VPData->deserializeTo(Record, &Symtab);
 
   // Now read data from Record and sanity check the data
   ASSERT_EQ(5U, Record.getNumValueSites(IPVK_IndirectCallTarget));
   ASSERT_EQ(5U, Record.getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
 
   auto Cmp = [](const InstrProfValueData &VD1, const InstrProfValueData &VD2) {
     return VD1.Count > VD2.Count;
   };