Re: [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map

[Ian Rogers <irogers@google.com>]

On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash
<ChaitanyaS.Prakash@arm.com> wrote:
>
> From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
>
> Perf currently treats any file beginning with "/tmp/perf-" as a map
> file, even when map files always have the extension ".map". This causes
> the test "perf probe of function from different CU" to fail because it
> generates a binary with a filename in that format. When the test passes
> it as an argument to be opened as an elf file, it's parsed as a map file
> and results in garbage symbol names. Fix this by only considering files
> ending in ".map" as map files.
>
> The above failure is masked if Perf is built with dwarf support. But
> with NO_DWARF=1, try_to_find_probe_trace_events() is skipped over in
> convert_to_probe_trace_events() falling into
> find_probe_trace_events_from_map() which results in the bad map file
> parse data being consumed and the inability to create the probe event.
>
> Signed-off-by: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
> ---
>  tools/perf/builtin-script.c | 15 +--------------
>  tools/perf/util/string.c    | 18 ++++++++++++++++++
>  tools/perf/util/string2.h   |  1 +
>  tools/perf/util/symbol.c    |  5 ++++-
>  4 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index b1f57401ff23..c1c38a3df37a 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -48,6 +48,7 @@
>  #include <errno.h>
>  #include <inttypes.h>
>  #include <signal.h>
> +#include <string2.h>
>  #include <sys/param.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> @@ -3240,20 +3241,6 @@ static struct script_desc *script_desc__findnew(const char *name)
>         return s;
>  }
>
> -static const char *ends_with(const char *str, const char *suffix)
> -{
> -       size_t suffix_len = strlen(suffix);
> -       const char *p = str;
> -
> -       if (strlen(str) > suffix_len) {
> -               p = str + strlen(str) - suffix_len;
> -               if (!strncmp(p, suffix, suffix_len))
> -                       return p;
> -       }
> -
> -       return NULL;
> -}
> -
>  static int read_script_info(struct script_desc *desc, const char *filename)
>  {
>         char line[BUFSIZ], *p;
> diff --git a/tools/perf/util/string.c b/tools/perf/util/string.c
> index 116a642ad99d..94c7af70e636 100644
> --- a/tools/perf/util/string.c
> +++ b/tools/perf/util/string.c
> @@ -349,3 +349,21 @@ char *strreplace_chars(char needle, const char *haystack, const char *replace)
>
>         return new_s;
>  }
> +
> +/*
> + * Returns a pointer to the first character of the suffix in str if the string
> + * ends with suffix, otherwise returns NULL.
> + */
> +const char *ends_with(const char *str, const char *suffix)
> +{
> +       size_t suffix_len = strlen(suffix);
> +       const char *p = str;
> +
> +       if (strlen(str) > suffix_len) {
> +               p = str + strlen(str) - suffix_len;

nit: I know this is just moving code but it seems unnecessary here to
call "strlen(str)" twice rather than just hold the result in a local
variable. A reasonable compiler should optimize this away, but it'd be
better to have the local imo.

> +               if (!strncmp(p, suffix, suffix_len))
> +                       return p;
> +       }
> +
> +       return NULL;
> +}
> diff --git a/tools/perf/util/string2.h b/tools/perf/util/string2.h
> index 52cb8ba057c7..0b9d1579a701 100644
> --- a/tools/perf/util/string2.h
> +++ b/tools/perf/util/string2.h
> @@ -40,5 +40,6 @@ char *strdup_esc(const char *str);
>
>  unsigned int hex(char c);
>  char *strreplace_chars(char needle, const char *haystack, const char *replace);
> +const char *ends_with(const char *str, const char *suffix);

nit: string2.h is an extension of linux's string.h. The tools copy of
that is missing functions in the kernel version:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/linux/string.h?h=perf-tools-next
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/string.h?h=perf-tools-next#n398
specifically str_has_prefix.

The naming ends_with makes sense but there is also strstarts and
str_has_prefix, perhaps str_has_suffix would be the most consistent
and intention revealing name?

Also, we have strtailcmp which behaves like a reverse strcmp that
doesn't compare the lengths of the strings. It seems all uses of
strtailcmp are just for a "str_has_suffix". It would make sense to me
to remove that function and switch to a common str_has_suffix function
which I think is a more intention revealing way of naming what the
code is doing.

Thanks,
Ian

>
>  #endif /* PERF_STRING_H */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index be212ba157dc..b87be3f1175a 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -4,6 +4,7 @@
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <string.h>
> +#include <string2.h>
>  #include <linux/capability.h>
>  #include <linux/kernel.h>
>  #include <linux/mman.h>
> @@ -1760,7 +1761,9 @@ int dso__load(struct dso *dso, struct map *map)
>         const char *map_path = dso->long_name;
>
>         mutex_lock(&dso->lock);
> -       perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0;
> +       perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0 &&
> +                 ends_with(dso->name, ".map") != NULL;
> +
>         if (perfmap) {
>                 if (dso->nsinfo && (dso__find_perf_map(newmapname,
>                     sizeof(newmapname), &dso->nsinfo) == 0)) {
> --
> 2.34.1
>
>