[llvm] r328132 - [InstrProf] Support for external functions in text format.

Reid Kleckner via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 21 14:55:45 PDT 2018


I went ahead and fixed this in r328157. In the future, avoid joining
/dev/null into arguments.


On Wed, Mar 21, 2018 at 2:34 PM via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Hi Mircea,
>
> Your change added a test which is failing because /dev/null does not exist
> on Windows. Can you fix the test?
>
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16048
>
> $ "not"
> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\bin\llvm-profdata.EXE"
> "merge"
> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\test\tools\llvm-profdata\Output\invalid-profdata.test.tmp.input"
> "-text" "-output=/dev/null"
> $
> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\bin\FileCheck.EXE"
> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\test\tools\llvm-profdata\invalid-profdata.test"
> "--check-prefix=BROKEN"
> # command stderr:
> C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\test\tools\llvm-profdata\invalid-profdata.test:24:9:
> error: expected string not found in input
>
> BROKEN: Malformed instrumentation profile data
>
>         ^
>
> <stdin>:1:1: note: scanning from here
>
> error: /dev/null: no such file or directory
>
> ^
>
> <stdin>:1:13: note: possible intended match here
>
> error: /dev/null: no such file or directory
>
>             ^
>
>
> error: command failed with exit status: 1
>
>
> Douglas Yung
>
> > -----Original Message-----
> > From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
> Behalf Of
> > Mircea Trofin via llvm-commits
> > Sent: Wednesday, March 21, 2018 12:06
> > To: llvm-commits at lists.llvm.org
> > Subject: [llvm] r328132 - [InstrProf] Support for external functions in
> text
> > format.
> >
> > Author: mtrofin
> > Date: Wed Mar 21 12:06:06 2018
> > New Revision: 328132
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=328132&view=rev
> > Log:
> > [InstrProf] Support for external functions in text format.
> >
> > Summary:
> > External functions appearing as indirect call targets could not be found
> in
> > the SymTab, and the value:counter record was represented, in the text
> format,
> > using an empty string for the name. This would then cause a silent
> parsing
> > error when reading.
> >
> > This CL:
> > - adds explicit support for such functions
> > - fixes the places where we would not propagate errors when reading
> > - addresses a performance issue due to eager resorting of the SymTab.
> >
> > Reviewers: xur, eraman, davidxl
> >
> > Reviewed By: davidxl
> >
> > Subscribers: llvm-commits
> >
> > Differential Revision: https://reviews.llvm.org/D44717
> >
> > Added:
> >     llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test
> > Modified:
> >     llvm/trunk/include/llvm/ProfileData/InstrProf.h
> >     llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
> >     llvm/trunk/lib/ProfileData/InstrProf.cpp
> >     llvm/trunk/lib/ProfileData/InstrProfReader.cpp
> >     llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
> >     llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> >
> > Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
> > URL: http://llvm.org/viewvc/llvm-
> >
> project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=328132&r1=328131&r
> > 2=328132&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
> > +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Mar 21 12:06:06
> > +++ 2018
> > @@ -425,9 +425,20 @@ private:
> >    // A map from function runtime address to function name MD5 hash.
> >    // This map is only populated and used by raw instr profile reader.
> >    AddrHashMap AddrToMD5Map;
> > +  bool Sorted = false;
> > +
> > +  static StringRef getExternalSymbol() {
> > +    return "** External Symbol **";
> > +  }
> > +
> > +  // If the symtab is created by a series of calls to \c addFuncName,
> > + \c  // finalizeSymtab needs to be called before looking up function
> names.
> > +  // This is required because the underlying map is a vector (for space
> > + // efficiency) which needs to be sorted.
> > +  inline void finalizeSymtab();
> >
> >  public:
> > -  InstrProfSymtab() = default;
> > +  InstrProfSymtab() = default;
> >
> >    /// Create InstrProfSymtab from an object file section which
> >    /// contains function PGO names. When section may contain raw @@
> -456,21
> > +467,17 @@ public:
> >    /// \p IterRange. This interface is used by IndexedProfReader.
> >    template <typename NameIterRange> Error create(const NameIterRange
> > &IterRange);
> >
> > -  // If the symtab is created by a series of calls to \c addFuncName, \c
> > -  // finalizeSymtab needs to be called before looking up function names.
> > -  // This is required because the underlying map is a vector (for space
> > -  // efficiency) which needs to be sorted.
> > -  inline void finalizeSymtab();
> > -
> >    /// 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);
> >      auto Ins = NameTab.insert(FuncName);
> > -    if (Ins.second)
> > +    if (Ins.second) {
> >        MD5NameMap.push_back(std::make_pair(
> >            IndexedInstrProf::ComputeHash(FuncName),
> Ins.first->getKey()));
> > +      Sorted = false;
> > +    }
> >      return Error::success();
> >    }
> >
> > @@ -491,6 +498,16 @@ public:
> >    /// If not found, return an empty string.
> >    inline StringRef getFuncName(uint64_t FuncMD5Hash);
> >
> > +  /// Just like getFuncName, except that it will return a non-empty
> > + StringRef  /// if the function is external to this symbol table. All
> > + such cases  /// will be represented using the same StringRef value.
> > +  inline StringRef getFuncNameOrExternalSymbol(uint64_t FuncMD5Hash);
> > +
> > +  /// True if Symbol is the value used to represent external symbols.
> > +  static bool isExternalSymbol(const StringRef &Symbol) {
> > +    return Symbol == InstrProfSymtab::getExternalSymbol();
> > +  }
> > +
> >    /// Return function from the name's md5 hash. Return nullptr if not
> found.
> >    inline Function *getFunction(uint64_t FuncMD5Hash);
> >
> > @@ -524,14 +541,25 @@ Error InstrProfSymtab::create(const Name  }
> >
> >  void InstrProfSymtab::finalizeSymtab() {
> > +  if (Sorted)
> > +    return;
> >    std::sort(MD5NameMap.begin(), MD5NameMap.end(), less_first());
> >    std::sort(MD5FuncMap.begin(), MD5FuncMap.end(), less_first());
> >    std::sort(AddrToMD5Map.begin(), AddrToMD5Map.end(), less_first());
> >    AddrToMD5Map.erase(std::unique(AddrToMD5Map.begin(),
> AddrToMD5Map.end()),
> >                       AddrToMD5Map.end());
> > +  Sorted = true;
> > +}
> > +
> > +StringRef InstrProfSymtab::getFuncNameOrExternalSymbol(uint64_t
> > +FuncMD5Hash) {
> > +  StringRef ret = getFuncName(FuncMD5Hash);
> > +  if (ret.empty())
> > +    return InstrProfSymtab::getExternalSymbol();
> > +  return ret;
> >  }
> >
> >  StringRef InstrProfSymtab::getFuncName(uint64_t FuncMD5Hash) {
> > +  finalizeSymtab();
> >    auto Result =
> >        std::lower_bound(MD5NameMap.begin(), MD5NameMap.end(),
> FuncMD5Hash,
> >                         [](const std::pair<uint64_t, std::string> &LHS,
> @@ -
> > 542,6 +570,7 @@ StringRef InstrProfSymtab::getFuncName(u  }
> >
> >  Function* InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) {
> > +  finalizeSymtab();
> >    auto Result =
> >        std::lower_bound(MD5FuncMap.begin(), MD5FuncMap.end(),
> FuncMD5Hash,
> >                         [](const std::pair<uint64_t, Function*> &LHS,
> >
> > Modified: llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
> > URL: http://llvm.org/viewvc/llvm-
> >
> project/llvm/trunk/include/llvm/ProfileData/InstrProfReader.h?rev=328132&r1=32
> > 8131&r2=328132&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/ProfileData/InstrProfReader.h (original)
> > +++ llvm/trunk/include/llvm/ProfileData/InstrProfReader.h Wed Mar 21
> > +++ 12:06:06 2018
> > @@ -101,7 +101,7 @@ protected:
> >      return make_error<InstrProfError>(Err);
> >    }
> >
> > -  Error error(Error E) { return
> error(InstrProfError::take(std::move(E))); }
> > +  Error error(Error &&E) { return
> > + error(InstrProfError::take(std::move(E))); }
> >
> >    /// Clear the current error and return a successful one.
> >    Error success() { return error(instrprof_error::success); }
> >
> > Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
> > URL: http://llvm.org/viewvc/llvm-
> >
> project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=328132&r1=328131&r2=32813
> > 2&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
> > +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Mar 21 12:06:06 2018
> > @@ -355,7 +355,7 @@ Error InstrProfSymtab::create(Module &M,
> >        }
> >      }
> >    }
> > -
> > +  Sorted = false;
> >    finalizeSymtab();
> >    return Error::success();
> >  }
> > @@ -461,7 +461,6 @@ Error readPGOFuncNameStrings(StringRef N
> >      while (P < EndP && *P == 0)
> >        P++;
> >    }
> > -  Symtab.finalizeSymtab();
> >    return Error::success();
> >  }
> >
> >
> > Modified: llvm/trunk/lib/ProfileData/InstrProfReader.cpp
> > URL: http://llvm.org/viewvc/llvm-
> >
> project/llvm/trunk/lib/ProfileData/InstrProfReader.cpp?rev=328132&r1=328131&r2
> > =328132&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/ProfileData/InstrProfReader.cpp (original)
> > +++ llvm/trunk/lib/ProfileData/InstrProfReader.cpp Wed Mar 21 12:06:06
> > +++ 2018
> > @@ -200,9 +200,13 @@ TextInstrProfReader::readValueProfileDat
> >          std::pair<StringRef, StringRef> VD = Line->rsplit(':');
> >          uint64_t TakenCount, Value;
> >          if (ValueKind == IPVK_IndirectCallTarget) {
> > -          if (Error E = Symtab->addFuncName(VD.first))
> > -            return E;
> > -          Value = IndexedInstrProf::ComputeHash(VD.first);
> > +          if (InstrProfSymtab::isExternalSymbol(VD.first)) {
> > +            Value = 0;
> > +          } else {
> > +            if (Error E = Symtab->addFuncName(VD.first))
> > +              return E;
> > +            Value = IndexedInstrProf::ComputeHash(VD.first);
> > +          }
> >          } else {
> >            READ_NUM(VD.first, Value);
> >          }
> > @@ -227,14 +231,13 @@ Error TextInstrProfReader::readNextRecor
> >      ++Line;
> >    // If we hit EOF while looking for a name, we're done.
> >    if (Line.is_at_end()) {
> > -    Symtab->finalizeSymtab();
> >      return error(instrprof_error::eof);
> >    }
> >
> >    // Read the function name.
> >    Record.Name = *Line++;
> >    if (Error E = Symtab->addFuncName(Record.Name))
> > -    return E;
> > +    return error(std::move(E));
> >
> >    // Read the function hash.
> >    if (Line.is_at_end())
> > @@ -265,11 +268,8 @@ Error TextInstrProfReader::readNextRecor
> >
> >    // Check if value profile data exists and read it if so.
> >    if (Error E = readValueProfileData(Record))
> > -    return E;
> > +    return error(std::move(E));
> >
> > -  // This is needed to avoid two pass parsing because llvm-profdata
> > -  // does dumping while reading.
> > -  Symtab->finalizeSymtab();
> >    return success();
> >  }
> >
> > @@ -331,7 +331,6 @@ Error RawInstrProfReader<IntPtrT>::creat
> >        continue;
> >      Symtab.mapAddress(FPtr, I->NameRef);
> >    }
> > -  Symtab.finalizeSymtab();
> >    return success();
> >  }
> >
> > @@ -449,23 +448,23 @@ Error RawInstrProfReader<IntPtrT>::readN
> >    if (atEnd())
> >      // At this point, ValueDataStart field points to the next header.
> >      if (Error E = readNextHeader(getNextHeaderPos()))
> > -      return E;
> > +      return error(std::move(E));
> >
> >    // Read name ad set it in Record.
> >    if (Error E = readName(Record))
> > -    return E;
> > +    return error(std::move(E));
> >
> >    // Read FuncHash and set it in Record.
> >    if (Error E = readFuncHash(Record))
> > -    return E;
> > +    return error(std::move(E));
> >
> >    // Read raw counts and set Record.
> >    if (Error E = readRawCounts(Record))
> > -    return E;
> > +    return error(std::move(E));
> >
> >    // Read value data and set Record.
> >    if (Error E = readValueProfilingData(Record))
> > -    return E;
> > +    return error(std::move(E));
> >
> >    // Iterate.
> >    advanceData();
> >
> > Modified: llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
> > URL: http://llvm.org/viewvc/llvm-
> >
> project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=328132&r1=328131&r2
> > =328132&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/ProfileData/InstrProfWriter.cpp (original)
> > +++ llvm/trunk/lib/ProfileData/InstrProfWriter.cpp Wed Mar 21 12:06:06
> > +++ 2018
> > @@ -361,7 +361,8 @@ void InstrProfWriter::writeRecordInText(
> >        std::unique_ptr<InstrProfValueData[]> VD =
> Func.getValueForSite(VK, S);
> >        for (uint32_t I = 0; I < ND; I++) {
> >          if (VK == IPVK_IndirectCallTarget)
> > -          OS << Symtab.getFuncName(VD[I].Value) << ":" << VD[I].Count <<
> > "\n";
> > +          OS << Symtab.getFuncNameOrExternalSymbol(VD[I].Value) << ":"
> > +             << VD[I].Count << "\n";
> >          else
> >            OS << VD[I].Value << ":" << VD[I].Count << "\n";
> >        }
> > @@ -379,7 +380,6 @@ Error InstrProfWriter::writeText(raw_fd_
> >      if (shouldEncodeData(I.getValue()))
> >        if (Error E = Symtab.addFuncName(I.getKey()))
> >          return E;
> > -  Symtab.finalizeSymtab();
> >
> >    for (const auto &I : FunctionData)
> >      if (shouldEncodeData(I.getValue()))
> >
> > Added: llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-
> > profdata/invalid-profdata.test?rev=328132&view=auto
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test (added)
> > +++ llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test Wed Mar 21
> > +++ 12:06:06 2018
> > @@ -0,0 +1,50 @@
> > +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
> > +RUN: echo "# Counter Values:"       >> %t.input
> > +RUN: echo "0"                       >> %t.input
> > +RUN: echo "12"                      >> %t.input
> > +RUN: echo "12"                      >> %t.input
> > +RUN: echo "# Num Value Kinds:"      >> %t.input
> > +RUN: echo "1"                       >> %t.input
> > +RUN: echo "# ValueKind = IPVK_IndirectCallTarget:"       >> %t.input
> > +RUN: echo "0"                                            >> %t.input
> > +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
> > +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
> > +RUN: echo "# Counter Values:"       >> %t.input
> > +RUN: echo "0"                       >> %t.input
> > +RUN: echo "12"                      >> %t.input
> > +RUN: echo "12"                      >> %t.input
> > +RUN: echo "# Num Value Kinds:"      >> %t.input
> > +RUN: echo "1"                       >> %t.input
> > +RUN: echo "# ValueKind = IPVK_IndirectCallTarget:"       >> %t.input
> > +RUN: echo "0"                                            >> %t.input
> > +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 "** External Symbol **:10"             >> %t.input
> > +
> > +# RUN: llvm-profdata merge %t.input -text -output=%t.out && cat %t.out
> > +| FileCheck %s
> > +
> > +CHECK: ** External Symbol **:10
> >
> > Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> > URL: http://llvm.org/viewvc/llvm-
> >
> project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=328132&r1=32813
> > 1&r2=328132&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
> > +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Mar 21
> > +++ 12:06:06 2018
> > @@ -769,7 +769,6 @@ TEST_P(MaybeSparseInstrProfTest, value_p
> >    Symtab.mapAddress(uint64_t(callee3), 0x3000ULL);
> >    Symtab.mapAddress(uint64_t(callee4), 0x4000ULL);
> >    // Missing mapping for callee5
> > -  Symtab.finalizeSymtab();
> >
> >    VPData->deserializeTo(Record, &Symtab.getAddrHashMap());
> >
> > @@ -858,8 +857,6 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
> >    EXPECT_THAT_ERROR(Symtab.addFuncName("blah_1"), Succeeded());
> >    EXPECT_THAT_ERROR(Symtab.addFuncName("blah_2"), Succeeded());
> >    EXPECT_THAT_ERROR(Symtab.addFuncName("blah_3"), Succeeded());
> > -  // Finalize it
> > -  Symtab.finalizeSymtab();
> >
> >    // Check again
> >    R = Symtab.getFuncName(IndexedInstrProf::ComputeHash("blah_1"));
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180321/3da2ba16/attachment-0001.html>


More information about the llvm-commits mailing list