[PATCH 0/3] perf tools: Fix test "perf probe of function from different CU"

[PATCH 0/3] perf tools: Fix test "perf probe of function from different CU"

[Chaitanya S Prakash]

From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>

Defconfig doesn't provide all the necessary configs required for the
test "perf probe of function from different CU" to run successfully on
all platforms. Therefore the required configs have been added to
config fragments to resolve this issue. On further investigation it was
seen that the Perf treated all files beginning with "/tmp/perf-" as a
map file despite them always ending in ".map", this caused the test to
fail when Perf was built with NO_DWARF=1. As the file was parsed as a
map file, the probe...--funcs command output garbage values instead of
listing the functions in the binary. After fixing the issue an
additional check to test the output of the probe...--funcs command has
been added.

This patch series has been tested on 6.8-rc3 mainline kernerl, both on
arm64 and x86 platforms.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: Georg Müller <georgmueller@gmx.net>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Liam Howlett <liam.howlett@oracle.com>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: linux-perf-users@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Chaitanya S Prakash (3):
  perf tools: Enable configs required for
    test_uprobe_from_different_cu.sh
  perf tools: Only treat files as map files when they have the extension
    .map
  perf test: Check output of the probe ... --funcs command

 tools/perf/builtin-script.c                    | 15 +--------------
 tools/perf/tests/config-fragments/config       |  3 +++
 .../shell/test_uprobe_from_different_cu.sh     |  2 +-
 tools/perf/util/string.c                       | 18 ++++++++++++++++++
 tools/perf/util/string2.h                      |  1 +
 tools/perf/util/symbol.c                       |  5 ++++-
 6 files changed, 28 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH 1/3] perf tools: Enable configs required for test_uprobe_from_different_cu.sh

[Chaitanya S Prakash]

From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>

Test "perf probe of function from different CU" fails due to certain
configs not being enabled. Building the kernel with
CONFIG_KPROBE_EVENTS=y and CONFIG_UPROBE_EVENTS=y fixes the issue. As
CONFIG_KPROBE_EVENTS is dependent on CONFIG_KPROBES, enable it as well.
Some platforms enable these configs as a part of their defconfig, so
this change is only required for the ones that don't do so.

Signed-off-by: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
---
 tools/perf/tests/config-fragments/config | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/tests/config-fragments/config b/tools/perf/tests/config-fragments/config
index c340b3195fca..4fca12851016 100644
--- a/tools/perf/tests/config-fragments/config
+++ b/tools/perf/tests/config-fragments/config
@@ -9,3 +9,6 @@ CONFIG_GENERIC_TRACER=y
 CONFIG_FTRACE=y
 CONFIG_FTRACE_SYSCALLS=y
 CONFIG_BRANCH_PROFILE_NONE=y
+CONFIG_KPROBES=y
+CONFIG_KPROBE_EVENTS=y
+CONFIG_UPROBE_EVENTS=y
-- 
2.34.1

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

[Chaitanya S Prakash]

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;
+		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);
 
 #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

[PATCH 3/3] perf test: Check output of the probe ... --funcs command

[Chaitanya S Prakash]

From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>

Test "perf probe of function from different CU" only checks if the perf
command has failed and doesn't test the --funcs output. In the issue
reported in the previous commit, the garbage output of the --funcs
command was being ignored by the test when it could have been caught.

An additional check to grep for "foo" has been added to test the --funcs
output.

Signed-off-by: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
---
 tools/perf/tests/shell/test_uprobe_from_different_cu.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/test_uprobe_from_different_cu.sh b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
index 319f36ebb9a4..82bc774a078a 100755
--- a/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
+++ b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
@@ -77,7 +77,7 @@ gcc -g -Og -flto -c ${temp_dir}/testfile-foo.c -o ${temp_dir}/testfile-foo.o
 gcc -g -Og -c ${temp_dir}/testfile-main.c -o ${temp_dir}/testfile-main.o
 gcc -g -Og -o ${temp_dir}/testfile ${temp_dir}/testfile-foo.o ${temp_dir}/testfile-main.o
 
-perf probe -x ${temp_dir}/testfile --funcs foo
+perf probe -x ${temp_dir}/testfile --funcs foo | grep "foo"
 perf probe -x ${temp_dir}/testfile foo
 
 cleanup
-- 
2.34.1

Re: [PATCH 0/3] perf tools: Fix test "perf probe of function from different CU"

[James Clark]

On 20/02/2024 04:29, Chaitanya S Prakash wrote:
> From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
> 
> Defconfig doesn't provide all the necessary configs required for the
> test "perf probe of function from different CU" to run successfully on
> all platforms. Therefore the required configs have been added to
> config fragments to resolve this issue. On further investigation it was
> seen that the Perf treated all files beginning with "/tmp/perf-" as a
> map file despite them always ending in ".map", this caused the test to
> fail when Perf was built with NO_DWARF=1. As the file was parsed as a
> map file, the probe...--funcs command output garbage values instead of
> listing the functions in the binary. After fixing the issue an
> additional check to test the output of the probe...--funcs command has
> been added.
> 
> This patch series has been tested on 6.8-rc3 mainline kernerl, both on
> arm64 and x86 platforms.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: James Clark <james.clark@arm.com>
> Cc: Georg Müller <georgmueller@gmx.net>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Liam Howlett <liam.howlett@oracle.com>
> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Cc: linux-perf-users@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Chaitanya S Prakash (3):
>   perf tools: Enable configs required for
>     test_uprobe_from_different_cu.sh
>   perf tools: Only treat files as map files when they have the extension
>     .map
>   perf test: Check output of the probe ... --funcs command
> 
>  tools/perf/builtin-script.c                    | 15 +--------------
>  tools/perf/tests/config-fragments/config       |  3 +++
>  .../shell/test_uprobe_from_different_cu.sh     |  2 +-
>  tools/perf/util/string.c                       | 18 ++++++++++++++++++
>  tools/perf/util/string2.h                      |  1 +
>  tools/perf/util/symbol.c                       |  5 ++++-
>  6 files changed, 28 insertions(+), 16 deletions(-)
> 

Reviewed-by: James Clark <james.clark@arm.com>

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

[Ian Rogers]

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

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

[Arnaldo Carvalho de Melo]

On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
> On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
> > +++ 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.

So far in perf we try to just reuse whatever function the kernel has for
the purpose at hand, right now the kernel has:

/**
 * strstarts - does @str start with @prefix?
 * @str: string to examine
 * @prefix: prefix to look for.
 */
static inline bool strstarts(const char *str, const char *prefix)
{
        return strncmp(str, prefix, strlen(prefix)) == 0;
}

And:

/**
 * str_has_prefix - Test if a string has a given prefix
 * @str: The string to test
 * @prefix: The string to see if @str starts with
 *
 * A common way to test a prefix of a string is to do:
 *  strncmp(str, prefix, sizeof(prefix) - 1)
 *
 * But this can lead to bugs due to typos, or if prefix is a pointer
 * and not a constant. Instead use str_has_prefix().
 *
 * Returns:
 * * strlen(@prefix) if @str starts with @prefix
 * * 0 if @str does not start with @prefix
 */
static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
{
        size_t len = strlen(prefix);
        return strncmp(str, prefix, len) == 0 ? len : 0;
}

The later seems to give more bang for the buck, so to say, returning the
prefix lenght.

It is a new addition:

72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)

While:

66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?

⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
94
⬢[acme@toolbox linux]$ git grep strstarts| wc -l
177
⬢[acme@toolbox linux]$ 

Some places use it:

kernel/printk/printk.c: len = str_has_prefix(str, "on");
kernel/printk/printk.c: len = str_has_prefix(str, "off");
kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");


static int __control_devkmsg(char *str)
{
	size_t len;

	if (!str)
		return -EINVAL;

	len = str_has_prefix(str, "on");
	if (len) {
		devkmsg_log = DEVKMSG_LOG_MASK_ON;
		return len;
	}

	len = str_has_prefix(str, "off");
	if (len) {
		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
		return len;
	}

	len = str_has_prefix(str, "ratelimit");
	if (len) {
		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
		return len;
	}

	return -EINVAL;
}


                err = __control_devkmsg(devkmsg_log_str);
 
                /*
                 * Do not accept an unknown string OR a known string with
                 * trailing crap...
                 */
                if (err < 0 || (err + 1 != *lenp)) {

                        /* ... and restore old setting. */
                        devkmsg_log = old;
                        strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);

                        return -EINVAL;
                }


So yeah, I agree with Ian that it is more intention revealing, has this
bonus of returning the strlen for the above use cases, is in the kernel
sources, so I'm in favour of grabbing a copy of it and replacing the
strstarts() usage with it, drop strstarts(), then also introduce
str_has_suffix(), the kernel will get it when it needs, possibly from
tools/lib/ :-)

- Arnaldo

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

[Chaitanya S Prakash]

I'll make the changes, thanks for the review.

On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
> On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
>> On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
>>> +++ 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.
> So far in perf we try to just reuse whatever function the kernel has for
> the purpose at hand, right now the kernel has:
>
> /**
>   * strstarts - does @str start with @prefix?
>   * @str: string to examine
>   * @prefix: prefix to look for.
>   */
> static inline bool strstarts(const char *str, const char *prefix)
> {
>          return strncmp(str, prefix, strlen(prefix)) == 0;
> }
>
> And:
>
> /**
>   * str_has_prefix - Test if a string has a given prefix
>   * @str: The string to test
>   * @prefix: The string to see if @str starts with
>   *
>   * A common way to test a prefix of a string is to do:
>   *  strncmp(str, prefix, sizeof(prefix) - 1)
>   *
>   * But this can lead to bugs due to typos, or if prefix is a pointer
>   * and not a constant. Instead use str_has_prefix().
>   *
>   * Returns:
>   * * strlen(@prefix) if @str starts with @prefix
>   * * 0 if @str does not start with @prefix
>   */
> static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> {
>          size_t len = strlen(prefix);
>          return strncmp(str, prefix, len) == 0 ? len : 0;
> }
>
> The later seems to give more bang for the buck, so to say, returning the
> prefix lenght.
>
> It is a new addition:
>
> 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>
> While:
>
> 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
>
> ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
> 94
> ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
> 177
> ⬢[acme@toolbox linux]$
>
> Some places use it:
>
> kernel/printk/printk.c: len = str_has_prefix(str, "on");
> kernel/printk/printk.c: len = str_has_prefix(str, "off");
> kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
>
>
> static int __control_devkmsg(char *str)
> {
> 	size_t len;
>
> 	if (!str)
> 		return -EINVAL;
>
> 	len = str_has_prefix(str, "on");
> 	if (len) {
> 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
> 		return len;
> 	}
>
> 	len = str_has_prefix(str, "off");
> 	if (len) {
> 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
> 		return len;
> 	}
>
> 	len = str_has_prefix(str, "ratelimit");
> 	if (len) {
> 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> 		return len;
> 	}
>
> 	return -EINVAL;
> }
>
>
>                  err = __control_devkmsg(devkmsg_log_str);
>   
>                  /*
>                   * Do not accept an unknown string OR a known string with
>                   * trailing crap...
>                   */
>                  if (err < 0 || (err + 1 != *lenp)) {
>
>                          /* ... and restore old setting. */
>                          devkmsg_log = old;
>                          strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
>
>                          return -EINVAL;
>                  }
>
>
> So yeah, I agree with Ian that it is more intention revealing, has this
> bonus of returning the strlen for the above use cases, is in the kernel
> sources, so I'm in favour of grabbing a copy of it and replacing the
> strstarts() usage with it, drop strstarts(), then also introduce
> str_has_suffix(), the kernel will get it when it needs, possibly from
> tools/lib/ :-)
>
> - Arnaldo

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

[Arnaldo Carvalho de Melo]

On Fri, Feb 23, 2024 at 05:40:02PM +0530, Chaitanya S Prakash wrote:
> I'll make the changes, thanks for the review.

Have you submitted a new series?

Thanks,

- Arnaldo
 
> On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
> > On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
> > > On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
> > > > +++ 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.
> > So far in perf we try to just reuse whatever function the kernel has for
> > the purpose at hand, right now the kernel has:
> > 
> > /**
> >   * strstarts - does @str start with @prefix?
> >   * @str: string to examine
> >   * @prefix: prefix to look for.
> >   */
> > static inline bool strstarts(const char *str, const char *prefix)
> > {
> >          return strncmp(str, prefix, strlen(prefix)) == 0;
> > }
> > 
> > And:
> > 
> > /**
> >   * str_has_prefix - Test if a string has a given prefix
> >   * @str: The string to test
> >   * @prefix: The string to see if @str starts with
> >   *
> >   * A common way to test a prefix of a string is to do:
> >   *  strncmp(str, prefix, sizeof(prefix) - 1)
> >   *
> >   * But this can lead to bugs due to typos, or if prefix is a pointer
> >   * and not a constant. Instead use str_has_prefix().
> >   *
> >   * Returns:
> >   * * strlen(@prefix) if @str starts with @prefix
> >   * * 0 if @str does not start with @prefix
> >   */
> > static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> > {
> >          size_t len = strlen(prefix);
> >          return strncmp(str, prefix, len) == 0 ? len : 0;
> > }
> > 
> > The later seems to give more bang for the buck, so to say, returning the
> > prefix lenght.
> > 
> > It is a new addition:
> > 
> > 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> > 
> > While:
> > 
> > 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
> > 
> > ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
> > 94
> > ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
> > 177
> > ⬢[acme@toolbox linux]$
> > 
> > Some places use it:
> > 
> > kernel/printk/printk.c: len = str_has_prefix(str, "on");
> > kernel/printk/printk.c: len = str_has_prefix(str, "off");
> > kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
> > 
> > 
> > static int __control_devkmsg(char *str)
> > {
> > 	size_t len;
> > 
> > 	if (!str)
> > 		return -EINVAL;
> > 
> > 	len = str_has_prefix(str, "on");
> > 	if (len) {
> > 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
> > 		return len;
> > 	}
> > 
> > 	len = str_has_prefix(str, "off");
> > 	if (len) {
> > 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
> > 		return len;
> > 	}
> > 
> > 	len = str_has_prefix(str, "ratelimit");
> > 	if (len) {
> > 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> > 		return len;
> > 	}
> > 
> > 	return -EINVAL;
> > }
> > 
> > 
> >                  err = __control_devkmsg(devkmsg_log_str);
> >                  /*
> >                   * Do not accept an unknown string OR a known string with
> >                   * trailing crap...
> >                   */
> >                  if (err < 0 || (err + 1 != *lenp)) {
> > 
> >                          /* ... and restore old setting. */
> >                          devkmsg_log = old;
> >                          strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
> > 
> >                          return -EINVAL;
> >                  }
> > 
> > 
> > So yeah, I agree with Ian that it is more intention revealing, has this
> > bonus of returning the strlen for the above use cases, is in the kernel
> > sources, so I'm in favour of grabbing a copy of it and replacing the
> > strstarts() usage with it, drop strstarts(), then also introduce
> > str_has_suffix(), the kernel will get it when it needs, possibly from
> > tools/lib/ :-)
> > 
> > - Arnaldo

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

[Chaitanya S Prakash]

On 3/16/24 02:04, Arnaldo Carvalho de Melo wrote:
> On Fri, Feb 23, 2024 at 05:40:02PM +0530, Chaitanya S Prakash wrote:
>> I'll make the changes, thanks for the review.
> Have you submitted a new series?
>
> Thanks,
>
> - Arnaldo
I will be posting it this week, extremely sorry for the delay!
>   
>> On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
>>> On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
>>>> On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
>>>>> +++ 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.
>>> So far in perf we try to just reuse whatever function the kernel has for
>>> the purpose at hand, right now the kernel has:
>>>
>>> /**
>>>    * strstarts - does @str start with @prefix?
>>>    * @str: string to examine
>>>    * @prefix: prefix to look for.
>>>    */
>>> static inline bool strstarts(const char *str, const char *prefix)
>>> {
>>>           return strncmp(str, prefix, strlen(prefix)) == 0;
>>> }
>>>
>>> And:
>>>
>>> /**
>>>    * str_has_prefix - Test if a string has a given prefix
>>>    * @str: The string to test
>>>    * @prefix: The string to see if @str starts with
>>>    *
>>>    * A common way to test a prefix of a string is to do:
>>>    *  strncmp(str, prefix, sizeof(prefix) - 1)
>>>    *
>>>    * But this can lead to bugs due to typos, or if prefix is a pointer
>>>    * and not a constant. Instead use str_has_prefix().
>>>    *
>>>    * Returns:
>>>    * * strlen(@prefix) if @str starts with @prefix
>>>    * * 0 if @str does not start with @prefix
>>>    */
>>> static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>>> {
>>>           size_t len = strlen(prefix);
>>>           return strncmp(str, prefix, len) == 0 ? len : 0;
>>> }
>>>
>>> The later seems to give more bang for the buck, so to say, returning the
>>> prefix lenght.
>>>
>>> It is a new addition:
>>>
>>> 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>>>
>>> While:
>>>
>>> 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
>>>
>>> ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
>>> 94
>>> ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
>>> 177
>>> ⬢[acme@toolbox linux]$
>>>
>>> Some places use it:
>>>
>>> kernel/printk/printk.c: len = str_has_prefix(str, "on");
>>> kernel/printk/printk.c: len = str_has_prefix(str, "off");
>>> kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
>>>
>>>
>>> static int __control_devkmsg(char *str)
>>> {
>>> 	size_t len;
>>>
>>> 	if (!str)
>>> 		return -EINVAL;
>>>
>>> 	len = str_has_prefix(str, "on");
>>> 	if (len) {
>>> 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
>>> 		return len;
>>> 	}
>>>
>>> 	len = str_has_prefix(str, "off");
>>> 	if (len) {
>>> 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
>>> 		return len;
>>> 	}
>>>
>>> 	len = str_has_prefix(str, "ratelimit");
>>> 	if (len) {
>>> 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
>>> 		return len;
>>> 	}
>>>
>>> 	return -EINVAL;
>>> }
>>>
>>>
>>>                   err = __control_devkmsg(devkmsg_log_str);
>>>                   /*
>>>                    * Do not accept an unknown string OR a known string with
>>>                    * trailing crap...
>>>                    */
>>>                   if (err < 0 || (err + 1 != *lenp)) {
>>>
>>>                           /* ... and restore old setting. */
>>>                           devkmsg_log = old;
>>>                           strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
>>>
>>>                           return -EINVAL;
>>>                   }
>>>
>>>
>>> So yeah, I agree with Ian that it is more intention revealing, has this
>>> bonus of returning the strlen for the above use cases, is in the kernel
>>> sources, so I'm in favour of grabbing a copy of it and replacing the
>>> strstarts() usage with it, drop strstarts(), then also introduce
>>> str_has_suffix(), the kernel will get it when it needs, possibly from
>>> tools/lib/ :-)
>>>
>>> - Arnaldo

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

[James Clark]

On 20/02/2024 14:40, Ian Rogers wrote:
> 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.
> 

I glossed over this before, but looking in more detail I couldn't see
that strtailcmp() was definitely being used as str_has_suffix(). Was it
done this way because either the created probe or the dwarf entry could
be using the tail as the filename, but it's not always known which is which?

I couldn't see that function being hit in any of the tests either, so it
seems quite risky to change it and it doesn't cost anything to leave it
as it is seeing as it is a function that does something different to
str_has_suffix().