2024-04-08 06:23:41

by Chaitanya S Prakash

[permalink] [raw]
Subject: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"

From: Chaitanya S Prakash <[email protected]>

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.

Additionally, various functions within the codebase have been refactored
and restructured. The definition of str_has_suffix() has been adopted
from tools/bpf/bpftool/gen.c and added to tools/lib/string.c in an
attempt to make the function more generic. The implementation has been
retained but the return values have been modified to resemble that of
str_has_prefix(), i.e., return strlen(suffix) on success and 0 on
failure. In light of the new addition, "ends_with()", a locally defined
function used for checking if a string had a given suffix has been
deleted and str_has_suffix() has replaced its usage. A call to
strtailcmp() has also been replaced as str_has_suffix() seemed more
suited for that particular use case.

Finally str_has_prefix() is adopted from the kernel and is added to
tools/lib/string.c, following which strstarts() is deleted and its use
has been replaced with str_has_prefix().

This patch series has been tested on 6.9-rc2 mainline kernel, both on
arm64 and x86 platforms.

Changes in V2:
- Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
- Delete ends_with() and replace its usage with str_has_suffix()
- Replace an instance of strtailcmp() with str_has_suffix()
- Delete strstarts() from tools/include/linux/string.h and replace its
usage with str_has_prefix()

Cc: Josh Poimboeuf <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: James Clark <[email protected]>
Cc: John Garry <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Ian Rogers <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: Chenyuan Mi <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Ravi Bangoria <[email protected]>
Cc: Ahelenia Ziemiańska <[email protected]>
Cc: Colin Ian King <[email protected]>
Cc: Changbin Du <[email protected]>
Cc: Kan Liang <[email protected]>
Cc: Athira Rajeev <[email protected]>
Cc: Tiezhu Yang <[email protected]>
Cc: Alexey Dobriyan <[email protected]>
Cc: Georg Müller <[email protected]>
Cc: Liam Howlett <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

Chaitanya S Prakash (8):
tools lib: adopt str_has_suffix() from bpftool/gen.c
perf util: Delete ends_with() and replace its use with
str_has_suffix()
perf util: Replace an instance of strtailcmp() by str_has_suffix()
tools lib: Adopt str_has_prefix() from kernel
tools: Delete strstarts() and replace its usage with str_has_prefix()
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/include/linux/string.h | 12 ++----
tools/lib/string.c | 42 +++++++++++++++++++
tools/lib/subcmd/help.c | 2 +-
tools/lib/subcmd/parse-options.c | 18 ++++----
tools/objtool/check.c | 2 +-
tools/perf/arch/arm/util/pmu.c | 4 +-
tools/perf/arch/x86/annotate/instructions.c | 14 +++----
tools/perf/arch/x86/util/env.c | 2 +-
tools/perf/builtin-c2c.c | 4 +-
tools/perf/builtin-config.c | 2 +-
tools/perf/builtin-daemon.c | 2 +-
tools/perf/builtin-ftrace.c | 2 +-
tools/perf/builtin-help.c | 6 +--
tools/perf/builtin-kmem.c | 2 +-
tools/perf/builtin-kvm.c | 14 +++----
tools/perf/builtin-kwork.c | 10 ++---
tools/perf/builtin-lock.c | 6 +--
tools/perf/builtin-mem.c | 4 +-
tools/perf/builtin-sched.c | 6 +--
tools/perf/builtin-script.c | 30 ++++---------
tools/perf/builtin-stat.c | 4 +-
tools/perf/builtin-timechart.c | 2 +-
tools/perf/builtin-trace.c | 6 +--
tools/perf/perf.c | 12 +++---
tools/perf/tests/config-fragments/config | 3 ++
.../shell/test_uprobe_from_different_cu.sh | 2 +-
tools/perf/tests/symbols.c | 2 +-
tools/perf/ui/browser.c | 2 +-
tools/perf/ui/browsers/scripts.c | 2 +-
tools/perf/ui/stdio/hist.c | 2 +-
tools/perf/util/amd-sample-raw.c | 4 +-
tools/perf/util/annotate.c | 2 +-
tools/perf/util/callchain.c | 2 +-
tools/perf/util/config.c | 12 +++---
tools/perf/util/map.c | 8 ++--
tools/perf/util/pmus.c | 2 +-
tools/perf/util/probe-event.c | 2 +-
tools/perf/util/sample-raw.c | 2 +-
tools/perf/util/symbol-elf.c | 4 +-
tools/perf/util/symbol.c | 4 +-
40 files changed, 146 insertions(+), 117 deletions(-)

--
2.30.2



2024-04-09 05:32:34

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"

On Mon, Apr 08, 2024 at 11:52:22AM +0530, Chaitanya S Prakash wrote:
> - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
> - Delete ends_with() and replace its usage with str_has_suffix()

> - Delete strstarts() from tools/include/linux/string.h and replace its
> usage with str_has_prefix()

It should be the other way: starts_with is normal in userspace.
C++, Python, Java, C# all have it. JavaScript too!

2024-04-10 03:35:25

by Chaitanya S Prakash

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"

Link to V1:
https://lore.kernel.org/all/[email protected]/

On 4/8/24 11:52, Chaitanya S Prakash wrote:
> From: Chaitanya S Prakash <[email protected]>
>
> 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.
>
> Additionally, various functions within the codebase have been refactored
> and restructured. The definition of str_has_suffix() has been adopted
> from tools/bpf/bpftool/gen.c and added to tools/lib/string.c in an
> attempt to make the function more generic. The implementation has been
> retained but the return values have been modified to resemble that of
> str_has_prefix(), i.e., return strlen(suffix) on success and 0 on
> failure. In light of the new addition, "ends_with()", a locally defined
> function used for checking if a string had a given suffix has been
> deleted and str_has_suffix() has replaced its usage. A call to
> strtailcmp() has also been replaced as str_has_suffix() seemed more
> suited for that particular use case.
>
> Finally str_has_prefix() is adopted from the kernel and is added to
> tools/lib/string.c, following which strstarts() is deleted and its use
> has been replaced with str_has_prefix().
>
> This patch series has been tested on 6.9-rc2 mainline kernel, both on
> arm64 and x86 platforms.
>
> Changes in V2:
> - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
> - Delete ends_with() and replace its usage with str_has_suffix()
> - Replace an instance of strtailcmp() with str_has_suffix()
> - Delete strstarts() from tools/include/linux/string.h and replace its
> usage with str_has_prefix()
>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: James Clark <[email protected]>
> Cc: John Garry <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Ian Rogers <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Chenyuan Mi <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Ravi Bangoria <[email protected]>
> Cc: Ahelenia Ziemiańska <[email protected]>
> Cc: Colin Ian King <[email protected]>
> Cc: Changbin Du <[email protected]>
> Cc: Kan Liang <[email protected]>
> Cc: Athira Rajeev <[email protected]>
> Cc: Tiezhu Yang <[email protected]>
> Cc: Alexey Dobriyan <[email protected]>
> Cc: Georg Müller <[email protected]>
> Cc: Liam Howlett <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Chaitanya S Prakash (8):
> tools lib: adopt str_has_suffix() from bpftool/gen.c
> perf util: Delete ends_with() and replace its use with
> str_has_suffix()
> perf util: Replace an instance of strtailcmp() by str_has_suffix()
> tools lib: Adopt str_has_prefix() from kernel
> tools: Delete strstarts() and replace its usage with str_has_prefix()
> 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/include/linux/string.h | 12 ++----
> tools/lib/string.c | 42 +++++++++++++++++++
> tools/lib/subcmd/help.c | 2 +-
> tools/lib/subcmd/parse-options.c | 18 ++++----
> tools/objtool/check.c | 2 +-
> tools/perf/arch/arm/util/pmu.c | 4 +-
> tools/perf/arch/x86/annotate/instructions.c | 14 +++----
> tools/perf/arch/x86/util/env.c | 2 +-
> tools/perf/builtin-c2c.c | 4 +-
> tools/perf/builtin-config.c | 2 +-
> tools/perf/builtin-daemon.c | 2 +-
> tools/perf/builtin-ftrace.c | 2 +-
> tools/perf/builtin-help.c | 6 +--
> tools/perf/builtin-kmem.c | 2 +-
> tools/perf/builtin-kvm.c | 14 +++----
> tools/perf/builtin-kwork.c | 10 ++---
> tools/perf/builtin-lock.c | 6 +--
> tools/perf/builtin-mem.c | 4 +-
> tools/perf/builtin-sched.c | 6 +--
> tools/perf/builtin-script.c | 30 ++++---------
> tools/perf/builtin-stat.c | 4 +-
> tools/perf/builtin-timechart.c | 2 +-
> tools/perf/builtin-trace.c | 6 +--
> tools/perf/perf.c | 12 +++---
> tools/perf/tests/config-fragments/config | 3 ++
> .../shell/test_uprobe_from_different_cu.sh | 2 +-
> tools/perf/tests/symbols.c | 2 +-
> tools/perf/ui/browser.c | 2 +-
> tools/perf/ui/browsers/scripts.c | 2 +-
> tools/perf/ui/stdio/hist.c | 2 +-
> tools/perf/util/amd-sample-raw.c | 4 +-
> tools/perf/util/annotate.c | 2 +-
> tools/perf/util/callchain.c | 2 +-
> tools/perf/util/config.c | 12 +++---
> tools/perf/util/map.c | 8 ++--
> tools/perf/util/pmus.c | 2 +-
> tools/perf/util/probe-event.c | 2 +-
> tools/perf/util/sample-raw.c | 2 +-
> tools/perf/util/symbol-elf.c | 4 +-
> tools/perf/util/symbol.c | 4 +-
> 40 files changed, 146 insertions(+), 117 deletions(-)
>

2024-04-11 12:10:35

by Chaitanya S Prakash

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"


On 4/9/24 11:02, Alexey Dobriyan wrote:
> On Mon, Apr 08, 2024 at 11:52:22AM +0530, Chaitanya S Prakash wrote:
>> - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
>> - Delete ends_with() and replace its usage with str_has_suffix()
>> - Delete strstarts() from tools/include/linux/string.h and replace its
>> usage with str_has_prefix()
> It should be the other way: starts_with is normal in userspace.
> C++, Python, Java, C# all have it. JavaScript too!

This is done in accordance with Ian's comments on V1 of this patch
series. Please find the link to the same below.

https://lore.kernel.org/all/CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com/

>


2024-04-11 18:49:25

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"

On Mon, Apr 08, 2024 at 11:52:22AM +0530, Chaitanya S Prakash wrote:
> From: Chaitanya S Prakash <[email protected]>
>
> 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.
>
> Additionally, various functions within the codebase have been refactored
> and restructured. The definition of str_has_suffix() has been adopted
> from tools/bpf/bpftool/gen.c and added to tools/lib/string.c in an
> attempt to make the function more generic. The implementation has been
> retained but the return values have been modified to resemble that of
> str_has_prefix(), i.e., return strlen(suffix) on success and 0 on
> failure. In light of the new addition, "ends_with()", a locally defined
> function used for checking if a string had a given suffix has been
> deleted and str_has_suffix() has replaced its usage. A call to
> strtailcmp() has also been replaced as str_has_suffix() seemed more
> suited for that particular use case.
>
> Finally str_has_prefix() is adopted from the kernel and is added to
> tools/lib/string.c, following which strstarts() is deleted and its use
> has been replaced with str_has_prefix().
>
> This patch series has been tested on 6.9-rc2 mainline kernel, both on
> arm64 and x86 platforms.
>
> Changes in V2:
> - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
> - Delete ends_with() and replace its usage with str_has_suffix()
> - Replace an instance of strtailcmp() with str_has_suffix()
> - Delete strstarts() from tools/include/linux/string.h and replace its
> usage with str_has_prefix()
>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: James Clark <[email protected]>
> Cc: John Garry <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Ian Rogers <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: Chenyuan Mi <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Ravi Bangoria <[email protected]>
> Cc: Ahelenia Ziemiańska <[email protected]>
> Cc: Colin Ian King <[email protected]>
> Cc: Changbin Du <[email protected]>
> Cc: Kan Liang <[email protected]>
> Cc: Athira Rajeev <[email protected]>
> Cc: Tiezhu Yang <[email protected]>
> Cc: Alexey Dobriyan <[email protected]>
> Cc: Georg Müller <[email protected]>
> Cc: Liam Howlett <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Chaitanya S Prakash (8):
> tools lib: adopt str_has_suffix() from bpftool/gen.c
> perf util: Delete ends_with() and replace its use with
> str_has_suffix()
> perf util: Replace an instance of strtailcmp() by str_has_suffix()
> tools lib: Adopt str_has_prefix() from kernel
> tools: Delete strstarts() and replace its usage with str_has_prefix()
> 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/include/linux/string.h | 12 ++----
> tools/lib/string.c | 42 +++++++++++++++++++
> tools/lib/subcmd/help.c | 2 +-
> tools/lib/subcmd/parse-options.c | 18 ++++----
> tools/objtool/check.c | 2 +-

Try not mixing things that are maintained by different people into the
same patch kit, for instance, in my case its failing with:

Cover: ./v2_20240408_chaitanyas_prakash_perf_tools_fix_test_perf_probe_of_function_from_different_cu.cover
Link: https://lore.kernel.org/r/[email protected]
Base: not specified
git am ./v2_20240408_chaitanyas_prakash_perf_tools_fix_test_perf_probe_of_function_from_different_cu.mbx
⬢[acme@toolbox perf-tools-next]$ git am ./v2_20240408_chaitanyas_prakash_perf_tools_fix_test_perf_probe_of_function_from_different_cu.mbx
Applying: tools lib: adopt str_has_suffix() from bpftool/gen.c
Applying: perf util: Delete ends_with() and replace its use with str_has_suffix()
Applying: perf util: Replace an instance of strtailcmp() by str_has_suffix()
Applying: tools lib: Adopt str_has_prefix() from kernel
Applying: tools: Delete strstarts() and replace its usage with str_has_prefix()
error: patch failed: tools/objtool/check.c:2535
error: tools/objtool/check.c: patch does not apply
Patch failed at 0005 tools: Delete strstarts() and replace its usage with str_has_prefix()
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
⬢[acme@toolbox perf-tools-next]$

So I'm checking if removing the objtool part makes this work.

- Arnaldo

> tools/perf/arch/arm/util/pmu.c | 4 +-
> tools/perf/arch/x86/annotate/instructions.c | 14 +++----
> tools/perf/arch/x86/util/env.c | 2 +-
> tools/perf/builtin-c2c.c | 4 +-
> tools/perf/builtin-config.c | 2 +-
> tools/perf/builtin-daemon.c | 2 +-
> tools/perf/builtin-ftrace.c | 2 +-
> tools/perf/builtin-help.c | 6 +--
> tools/perf/builtin-kmem.c | 2 +-
> tools/perf/builtin-kvm.c | 14 +++----
> tools/perf/builtin-kwork.c | 10 ++---
> tools/perf/builtin-lock.c | 6 +--
> tools/perf/builtin-mem.c | 4 +-
> tools/perf/builtin-sched.c | 6 +--
> tools/perf/builtin-script.c | 30 ++++---------
> tools/perf/builtin-stat.c | 4 +-
> tools/perf/builtin-timechart.c | 2 +-
> tools/perf/builtin-trace.c | 6 +--
> tools/perf/perf.c | 12 +++---
> tools/perf/tests/config-fragments/config | 3 ++
> .../shell/test_uprobe_from_different_cu.sh | 2 +-
> tools/perf/tests/symbols.c | 2 +-
> tools/perf/ui/browser.c | 2 +-
> tools/perf/ui/browsers/scripts.c | 2 +-
> tools/perf/ui/stdio/hist.c | 2 +-
> tools/perf/util/amd-sample-raw.c | 4 +-
> tools/perf/util/annotate.c | 2 +-
> tools/perf/util/callchain.c | 2 +-
> tools/perf/util/config.c | 12 +++---
> tools/perf/util/map.c | 8 ++--
> tools/perf/util/pmus.c | 2 +-
> tools/perf/util/probe-event.c | 2 +-
> tools/perf/util/sample-raw.c | 2 +-
> tools/perf/util/symbol-elf.c | 4 +-
> tools/perf/util/symbol.c | 4 +-
> 40 files changed, 146 insertions(+), 117 deletions(-)
>
> --
> 2.30.2
>
>

2024-04-14 11:41:51

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"

On Thu, Apr 11, 2024 at 05:40:04PM +0530, Chaitanya S Prakash wrote:
>
> On 4/9/24 11:02, Alexey Dobriyan wrote:
> > On Mon, Apr 08, 2024 at 11:52:22AM +0530, Chaitanya S Prakash wrote:
> > > - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
> > > - Delete ends_with() and replace its usage with str_has_suffix()
> > > - Delete strstarts() from tools/include/linux/string.h and replace its
> > > usage with str_has_prefix()
> > It should be the other way: starts_with is normal in userspace.
> > C++, Python, Java, C# all have it. JavaScript too!
>
> This is done in accordance with Ian's comments on V1 of this patch
> series. Please find the link to the same below.

Yes, but str_has_suffix() doesn't make sense in the wider context.

> https://lore.kernel.org/all/CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com/

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?

2024-04-17 13:25:47

by James Clark

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"



On 14/04/2024 12:41, Alexey Dobriyan wrote:
> On Thu, Apr 11, 2024 at 05:40:04PM +0530, Chaitanya S Prakash wrote:
>>
>> On 4/9/24 11:02, Alexey Dobriyan wrote:
>>> On Mon, Apr 08, 2024 at 11:52:22AM +0530, Chaitanya S Prakash wrote:
>>>> - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
>>>> - Delete ends_with() and replace its usage with str_has_suffix()
>>>> - Delete strstarts() from tools/include/linux/string.h and replace its
>>>> usage with str_has_prefix()
>>> It should be the other way: starts_with is normal in userspace.
>>> C++, Python, Java, C# all have it. JavaScript too!
>>
>> This is done in accordance with Ian's comments on V1 of this patch
>> series. Please find the link to the same below.
>
> Yes, but str_has_suffix() doesn't make sense in the wider context.
>
>> https://lore.kernel.org/all/CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com/
>
> 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?
>

Hi Alexey,

From a brief check it looks like str_has_prefix() is already quite
common with 94 uses. So the path of least resistance is to make
everything self consistent and add str_has_suffix().

I agree it's a bit of a mouthful and not so common in other languages.
Once this more complicated set gets through we could always do a simple
search and replace change it to anything we like. But it would touch
_lots_ of different drivers and trees, so it would be hard to justify.

Thanks
James

2024-04-17 14:06:59

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH V2 0/8] perf tools: Fix test "perf probe of function from different CU"

On Wed, Apr 17, 2024 at 02:24:33PM +0100, James Clark wrote:
> On 14/04/2024 12:41, Alexey Dobriyan wrote:
> > On Thu, Apr 11, 2024 at 05:40:04PM +0530, Chaitanya S Prakash wrote:
> >> On 4/9/24 11:02, Alexey Dobriyan wrote:
> >>> On Mon, Apr 08, 2024 at 11:52:22AM +0530, Chaitanya S Prakash wrote:
> >>>> - Add str_has_suffix() and str_has_prefix() to tools/lib/string.c
> >>>> - Delete ends_with() and replace its usage with str_has_suffix()
> >>>> - Delete strstarts() from tools/include/linux/string.h and replace its
> >>>> usage with str_has_prefix()
> >>> It should be the other way: starts_with is normal in userspace.
> >>> C++, Python, Java, C# all have it. JavaScript too!
> >>
> >> This is done in accordance with Ian's comments on V1 of this patch
> >> series. Please find the link to the same below.
> >
> > Yes, but str_has_suffix() doesn't make sense in the wider context.
> >
> >> https://lore.kernel.org/all/CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com/
> >
> > 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?

> From a brief check it looks like str_has_prefix() is already quite
> common with 94 uses. So the path of least resistance is to make
> everything self consistent and add str_has_suffix().

> I agree it's a bit of a mouthful and not so common in other languages.
> Once this more complicated set gets through we could always do a simple
> search and replace change it to anything we like. But it would touch
> _lots_ of different drivers and trees, so it would be hard to justify.

Right, we try to follow the kernel APIs to make tools/perf more familiar
to kernel developers, this return strlen() thing on str_has_prefix()
looked too clever for me at first, but since we need to do it anyway,
and return !0 to indicate it has the prefix and there are usecase...

The bpftool people agreed as well, so in tools/ it seems we're in
general agreement about using str_has_{prefix,suffix}().

- Arnaldo