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