From 556f8f6adc39e306749ace5582b2842981c0cd7f Mon Sep 17 00:00:00 2001 From: Mircea Trofin Date: Wed, 21 Mar 2018 22:27:31 +0000 Subject: [PATCH] [InstrProf] Encapsulates access to AddrToMD5Map. 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. Reviewers: davidxl, xur, eraman Reviewed By: xur Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D44757 llvm-svn: 328163 --- llvm/include/llvm/ProfileData/InstrProf.h | 9 ++--- llvm/include/llvm/ProfileData/InstrProfData.inc | 4 +- llvm/include/llvm/ProfileData/InstrProfReader.h | 2 - llvm/lib/ProfileData/InstrProf.cpp | 50 +++++++++++++------------ llvm/lib/ProfileData/InstrProfReader.cpp | 2 +- llvm/unittests/ProfileData/InstrProfTest.cpp | 2 +- 6 files changed, 34 insertions(+), 35 deletions(-) diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h index f15cf9a..88ae0f0 100644 --- a/llvm/include/llvm/ProfileData/InstrProf.h +++ b/llvm/include/llvm/ProfileData/InstrProf.h @@ -487,7 +487,8 @@ public: 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 @@ -643,8 +644,6 @@ struct InstrProfRecord { return *this; } - using ValueMapType = std::vector>; - /// Return the number of value profile kinds with non-zero number /// of profile sites. inline uint32_t getNumValueKinds() const; @@ -678,7 +677,7 @@ struct InstrProfRecord { /// 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. @@ -752,7 +751,7 @@ private: // 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. diff --git a/llvm/include/llvm/ProfileData/InstrProfData.inc b/llvm/include/llvm/ProfileData/InstrProfData.inc index eb4a792c..bac8ccec 100644 --- a/llvm/include/llvm/ProfileData/InstrProfData.inc +++ b/llvm/include/llvm/ProfileData/InstrProfData.inc @@ -315,7 +315,7 @@ typedef struct ValueProfRecord { * \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 @@ -393,7 +393,7 @@ typedef struct ValueProfData { * 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; diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h index ea0aad0..efc22dc 100644 --- a/llvm/include/llvm/ProfileData/InstrProfReader.h +++ b/llvm/include/llvm/ProfileData/InstrProfReader.h @@ -199,8 +199,6 @@ private: uint32_t ValueKindLast; uint32_t CurValueDataSize; - InstrProfRecord::ValueMapType FunctionPtrToNameMap; - public: RawInstrProfReader(std::unique_ptr DataBuffer) : DataBuffer(std::move(DataBuffer)) {} diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp index 7681d5c..fd25728 100644 --- a/llvm/lib/ProfileData/InstrProf.cpp +++ b/llvm/lib/ProfileData/InstrProf.cpp @@ -360,6 +360,21 @@ Error InstrProfSymtab::create(Module &M, bool InLTO) { return Error::success(); } +uint64_t InstrProfSymtab::getFunctionHashFromAddress(uint64_t Address) { + finalizeSymtab(); + auto Result = + std::lower_bound(AddrToMD5Map.begin(), AddrToMD5Map.end(), Address, + [](const std::pair &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 NameStrs, bool doCompression, std::string &Result) { assert(!NameStrs.empty() && "No name data to emit"); @@ -560,32 +575,19 @@ void InstrProfRecord::scale(uint64_t Weight, // 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 &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); } @@ -665,13 +667,13 @@ ValueProfData::serializeFrom(const InstrProfRecord &Record) { } 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; } } @@ -705,13 +707,13 @@ void ValueProfRecord::swapBytes(support::endianness Old, } 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); } } diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp index 69fc2bd..64f98ad 100644 --- a/llvm/lib/ProfileData/InstrProfReader.cpp +++ b/llvm/lib/ProfileData/InstrProfReader.cpp @@ -438,7 +438,7 @@ Error RawInstrProfReader::readValueProfilingData( // 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(); } diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp index 309241d..09be2e3 100644 --- a/llvm/unittests/ProfileData/InstrProfTest.cpp +++ b/llvm/unittests/ProfileData/InstrProfTest.cpp @@ -770,7 +770,7 @@ TEST_P(MaybeSparseInstrProfTest, value_prof_data_read_write_mapping) { 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)); -- 2.7.4