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

[Justin Bogner via llvm-commits]

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;
>  }
>