[PGO]: Do not use invalid Char in instrumentation variable names

[davidxl]

Summary

: and < are not valid characters for assemblers. See MCAsmInfo::isValidUnquotedName in MCAsmInfo.cpp and method llvm::printLLVMNameWithoutPrefix in AsmWriter.cpp.

Because this, when the such symbols names emitted in .s file will be quoted which will also upset the assembler.

To reproduce the problem, simply build the following source with option -fprofile-instr-generate -c -no-integrated-as:

static int cmp(int a, int b)
{

return a > b;

}

extern int foo(int (*)(int, int));

int bar()
{

return foo(cmp);

}

However there is a downside of this fix -- this is not a backward compatible fix -- once the fix is in, the old version of the indexed profile can not be guaranteed to be usable again (only partially usable) -- the static function's profile data will be dropped (can not be found anymore).

Need more opinion on the impact of partially breaking the old version of indexed format.

[silvas]

I would prefer not to silently break compatibility. These things tend to come back and bite. In particular, static functions are quite common, and we are unlikely to "do the right thing" with just this patch (actually, I know that this will affect the intrinsics in our built-in headers, so most code is likely to be affected). Do we have a versioning mechanism available so that we can produce a proper diagnostic and reject the file?

Personally, I am not strongly opposed to changing the format. We are still rolling out PGO to our customers and haven't produced explicit documentation on this aspect, but my intention is that we will document to our users that we do not guarantee any compatibility of the profraw or profdata files across releases. So in principle I have no concerns with breaking old profraw or profdata files as long as we cleanly reject them (besides a slight apprehension since I often am comparing compilers across releases and it is convenient to not have to have a separate llvm-profdata for each release).

So I will defer to Justin about whether we should break compatibility over this aspect and if so, how we should handle the transition.

[davidxl]

Updated patch.

With this change, the format change will be backward compatible.

Clang test case changes will be in another patch -- mostly mechanical. Existing test cases also include one binary profile test that covers the old naming scheme.

[davidxl]

The correct update.

[vsk]

This lgtm.

[silvas]

Tests are needed.

[vsk]

Oof, this hit me a second after I pressed enter. It looks like we don't have coverage for this at the moment.

[davidxl]

I will post test changes in a different patch -- existing tests already cover the case.

[silvas]

In D15243#307935, @davidxl wrote:

I will post test changes in a different patch -- existing tests already cover the case.

No. This patch changes functionality and needs a test.

[davidxl]

Very well. I had just added a test case to cover version 3. Will add
another one in this patch to cover the new naming scheme.

thanks,

David

[davidxl]

Update the patch with a test case.

include/llvm/ProfileData/InstrProf.h

 #include <system_error>
 #include <vector>
 
-#define INSTR_PROF_INDEX_VERSION 3
+#define INSTR_PROF_INDEX_VERSION 4
 namespace llvm {
 
 class Function;

lib/ProfileData/InstrProf.cpp

 
 std::string getPGOFuncName(StringRef RawFuncName,
                            GlobalValue::LinkageTypes Linkage,
-                           StringRef FileName,
-                           uint64_t Version LLVM_ATTRIBUTE_UNUSED) {
+                           StringRef FileName, uint64_t Version) {
 
   // Function names may be prefixed with a binary '1' to indicate
   // that the backend should not modify the symbols due to any platform
   // naming convention. Do not include that '1' in the PGO profile name.
   if (RawFuncName[0] == '\1')
     RawFuncName = RawFuncName.substr(1);
+  const char *Unknown = (Version <= 3 ? "<unknown>:" : " __unknown__");
+  const char *Sep = (Version <= 3 ? ":" : "__");
 
   std::string FuncName = RawFuncName;
   if (llvm::GlobalValue::isLocalLinkage(Linkage)) {

     // that it will stay the same, e.g., if the files are checked out from
     // version control in different locations.
     if (FileName.empty())
-      FuncName = FuncName.insert(0, "<unknown>:");
+      FuncName = FuncName.insert(0, Unknown);
     else
-      FuncName = FuncName.insert(0, FileName.str() + ":");
+      FuncName = FuncName.insert(0, FileName.str() + Sep);
   }
   return FuncName;
 }