[PATCH] Tolerate unmangled names in sample profiles.

[Diego Novillo]

On Mon, Mar 17, 2014 at 1:50 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
>
>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:497-509
> @@ +496,15 @@
> +    //
> +    // FIXME: this does not detect all cases. It only works with unmangled
> +    // names that contain "invalid" characters (spaces, ':', '<', etc).
> +    if (FName.substr(0, 2) != "_Z") {
> +      // Only check names that do not start with _Z. Anything starting
> +      // with _Z should be a properly mangled name.
> +      Regex InvalidChars("[:,<>-]");
> +      if (InvalidChars.match(FName))
> +        reportParseWarning(LineIt.line_number(),
> +                           "Function '" + FName +
> +                               "' is not properly mangled. No "
> +                               "samples will be collected "
> +                               "for it");
> +    }
> +
> ----------------
> Especially based on the FIXME I wonder whether its even worth warning about this.
>
> Essentially, I'd like to either warn about every function mentioned in the profile and not consumed here (including ones containing invalid characters for function names), or none of them. Would that policy make sense? Is there a particular reason to emit warnings?

Yeah, now that I've used this a few times, I don't like it. It's not
really possible to warn one way or the other because we are always
reading a global profile while compiling a single TU. There will
always be function we read in from the profile that we do not see in
this TU.

In essence, we are left with just accepting any kind of valid function
names (mangled and unmangled). We'll silently accept them. Diagnosing
which functions did not get profiled can be done via other means,
actually.

> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:499-501
> @@ +498,5 @@
> +    // names that contain "invalid" characters (spaces, ':', '<', etc).
> +    if (FName.substr(0, 2) != "_Z") {
> +      // Only check names that do not start with _Z. Anything starting
> +      // with _Z should be a properly mangled name.
> +      Regex InvalidChars("[:,<>-]");
> ----------------
> Not in a freestanding implementation? Not on Windows?
>
> Unsure why this is a valid assumption. See my comment below, but I wonder if it make sense to just always skip function names with characters we can't use in the primary parse...

Ah, my mistake. Naive assumption. I removed the whole check.

>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:502
> @@ +501,3 @@
> +      // with _Z should be a properly mangled name.
> +      Regex InvalidChars("[:,<>-]");
> +      if (InvalidChars.match(FName))
> ----------------
> Please factor regex parsing out of the loop.

Done.


Diego.