<div dir="ltr">I missed this in the original review.<div><br></div><div>In theory, changes like this requires bumping up the raw and coverage format version. However since it is relatively new, I think it would be ok to not bumping up the version and treat this like a bug fix.</div><div><br></div><div>Also why does the expected result change in test/tools/llvm-cov/Inputs/instrprof-comdat.h need to be bundled with this patch?</div><div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 28, 2016 at 1:12 PM, Vedant Kumar via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: vedantk<br>
Date: Mon Mar 28 15:12:07 2016<br>
New Revision: 264641<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=264641&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=264641&view=rev</a><br>
Log:<br>
Reapply "[PGO] Fix name encoding for ObjC-like functions"<br>
<br>
Function names in ObjC can have spaces in them. This interacts poorly<br>
with name compression, which uses spaces to separate PGO names. Fix the<br>
issue by using a different separator and update a test.<br>
<br>
I chose "\01" as the separator because 1) it's non-printable, 2) we<br>
strip it from PGO names, and 3) it's the next natural choice once "\00"<br>
is discarded (that one's overloaded).<br>
<br>
This reverts the revert commit beaf3d18. What's changed?<br>
<br>
- I fixed up the covmap-V2 binary format tests using a linux VM.<br>
- I updated the expected counts in instrprof-comdat.h to account for<br>
the fact that there have been bugfixes to clang coverage.<br>
- I added an assert to make sure we don't get bitten by this again.<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D18516" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18516</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l<br>
llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l<br>
llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata<br>
llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h<br>
llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw<br>
llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test<br>
llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test<br>
llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test<br>
llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test<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-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)<br>
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Mon Mar 28 15:12:07 2016<br>
@@ -151,6 +151,9 @@ inline StringRef getInstrProfFileOverrid<br>
return "__llvm_profile_override_default_filename";<br>
}<br>
<br>
+/// Return the marker used to separate PGO names during serialization.<br>
+inline StringRef getInstrProfNameSeparator() { return "\01"; }<br>
+<br>
/// Return the modified name for function \c F suitable to be<br>
/// used the key for profile lookup.<br>
std::string getPGOFuncName(const Function &F,<br>
<br>
Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)<br>
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Mon Mar 28 15:12:07 2016<br>
@@ -158,9 +158,15 @@ void InstrProfSymtab::create(const Modul<br>
<br>
int collectPGOFuncNameStrings(const std::vector<std::string> &NameStrs,<br>
bool doCompression, std::string &Result) {<br>
+ assert(NameStrs.size() && "No name data to emit");<br>
+<br>
uint8_t Header[16], *P = Header;<br>
std::string UncompressedNameStrings =<br>
- join(NameStrs.begin(), NameStrs.end(), StringRef(" "));<br>
+ join(NameStrs.begin(), NameStrs.end(), getInstrProfNameSeparator());<br>
+<br>
+ assert(StringRef(UncompressedNameStrings)<br>
+ .count(getInstrProfNameSeparator()) == (NameStrs.size() - 1) &&<br>
+ "PGO name is invalid (contains separator token)");<br>
<br>
unsigned EncLen = encodeULEB128(UncompressedNameStrings.length(), P);<br>
P += EncLen;<br>
@@ -238,7 +244,7 @@ int readPGOFuncNameStrings(StringRef Nam<br>
}<br>
// Now parse the name strings.<br>
SmallVector<StringRef, 0> Names;<br>
- NameStrings.split(Names, ' ');<br>
+ NameStrings.split(Names, getInstrProfNameSeparator());<br>
for (StringRef &Name : Names)<br>
Symtab.addFuncName(Name);<br>
<br>
<br>
Modified: llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
Binary files - no diff available.<br>
<br>
Modified: llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
Binary files - no diff available.<br>
<br>
Modified: llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
Binary files - no diff available.<br>
<br>
Modified: llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h (original)<br>
+++ llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h Mon Mar 28 15:12:07 2016<br>
@@ -13,7 +13,12 @@ template <class T> T FOO<T>::DoIt(T ti)<br>
t += I; // HEADER: 20| [[@LINE]]| t += I;<br>
if (I > ti / 2) // HEADER: 20| [[@LINE]]| if (I > ti<br>
t -= 1; // HEADER: 8| [[@LINE]]| t -= 1;<br>
- } // HEADER: 10| [[@LINE]]| }<br>
- // HEADER: 1| [[@LINE]]|<br>
- return t; // HEADER: 1| [[@LINE]]| return t;<br>
+ } // HEADER: 20| [[@LINE]]| }<br>
+ // HEADER: 2| [[@LINE]]|<br>
+ return t; // HEADER: 2| [[@LINE]]| return t;<br>
}<br>
+<br>
+// To generate the binaries which correspond to this file, you must first<br>
+// compile a program with two calls to Foo<int>::DoIt(10) for each desired<br>
+// architecture. Collect a raw profile from any one of these binaries, index<br>
+// it, and check it in along with the executables.<br>
<br>
Modified: llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
Binary files llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw (original) and llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw Mon Mar 28 15:12:07 2016 differ<br>
<br>
Modified: llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test (original)<br>
+++ llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test Mon Mar 28 15:12:07 2016<br>
@@ -28,7 +28,7 @@ RUN: printf '\0\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\0\0\0\0\0\0\0\023' >> %t<br>
RUN: printf '\0\0\0\0\0\0\0\067' >> %t<br>
RUN: printf '\0\0\0\0\0\0\0\101' >> %t<br>
-RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t<br>
+RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t<br>
<br>
RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s<br>
<br>
<br>
Modified: llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test (original)<br>
+++ llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test Mon Mar 28 15:12:07 2016<br>
@@ -28,7 +28,7 @@ RUN: printf '\0\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\023\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\067\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\101\0\0\0\0\0\0\0' >> %t<br>
-RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t<br>
+RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t<br>
<br>
RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s<br>
<br>
<br>
Modified: llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test (original)<br>
+++ llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test Mon Mar 28 15:12:07 2016<br>
@@ -26,7 +26,7 @@ RUN: printf '\0\0\0\02\0\0\0\0' >> %t<br>
RUN: printf '\0\0\0\0\0\0\0\023' >> %t<br>
RUN: printf '\0\0\0\0\0\0\0\067' >> %t<br>
RUN: printf '\0\0\0\0\0\0\0\101' >> %t<br>
-RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t<br>
+RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t<br>
<br>
RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s<br>
<br>
<br>
Modified: llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test (original)<br>
+++ llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test Mon Mar 28 15:12:07 2016<br>
@@ -26,7 +26,7 @@ RUN: printf '\02\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\023\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\067\0\0\0\0\0\0\0' >> %t<br>
RUN: printf '\101\0\0\0\0\0\0\0' >> %t<br>
-RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t<br>
+RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t<br>
<br>
RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s<br>
<br>
<br>
Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=264641&r1=264640&r2=264641&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=264641&r1=264640&r2=264641&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)<br>
+++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Mon Mar 28 15:12:07 2016<br>
@@ -893,7 +893,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_p<br>
OS << "func_" << I;<br>
FuncNames1.push_back(OS.str());<br>
str.clear();<br>
- OS << "fooooooooooooooo_" << I;<br>
+ OS << "f oooooooooooooo_" << I;<br>
FuncNames1.push_back(OS.str());<br>
str.clear();<br>
OS << "BAR_" << I;<br>
@@ -931,7 +931,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_p<br>
StringRef R = Symtab.getFuncName(IndexedInstrProf::ComputeHash(FuncNames1[0]));<br>
ASSERT_EQ(StringRef("func_0"), R);<br>
R = Symtab.getFuncName(IndexedInstrProf::ComputeHash(FuncNames1[1]));<br>
- ASSERT_EQ(StringRef("fooooooooooooooo_0"), R);<br>
+ ASSERT_EQ(StringRef("f oooooooooooooo_0"), R);<br>
for (int I = 0; I < 3; I++) {<br>
std::string N[4];<br>
N[0] = FuncNames1[2 * I];<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">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><br></div>