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

[Xinliang David Li via llvm-commits]

It is a little ugly, but I can teach the FE to check the version of
profile data and synthesize profile function names accordingly before
profile lookup.

This is not urgent, so I will get back to it some time later.

On Fri, Dec 4, 2015 at 4:09 PM, Justin Bogner <mail at justinbogner.com> wrote:
> David Li <davidxl at google.com> writes:
>> davidxl created this revision.
>> davidxl added reviewers: bogner, vsk.
>> davidxl added a subscriber: llvm-commits.
>>
>> : 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.
>
> Breaking the old version of the indexed format could be problematic - I
> think we'd need to do something to support reading either the pre- or
> post- fix versions of the format here, which might get ugly. Looping in
> Bob for his thoughts.
>
>>
>> http://reviews.llvm.org/D15243
>>
>> Files:
>>   lib/ProfileData/InstrProf.cpp
>>
>> Index: lib/ProfileData/InstrProf.cpp
>> ===================================================================
>> --- lib/ProfileData/InstrProf.cpp
>> +++ lib/ProfileData/InstrProf.cpp
>> @@ -89,9 +89,9 @@
>>      // 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() + "_");
>
> We might want something more distinctive than "_", to minimize the risk
> of collisions between functions-with-file-prefix and unprefixed. I guess
> we could use "__" since that could only possibly collide with reserved
> names, which limits the space significantly.
>
>>    }
>>    return FuncName;
>>  }
>>