2023-07-27 18:17:13

by Ian Rogers

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] perf probe: add test for regression introduced by switch to die_get_decl_file

On Wed, Jun 28, 2023 at 1:25 AM Georg Müller <[email protected]> wrote:
>
> This patch adds a test to validate that perf probe works for binaries
> where DWARF info is split into multiple CUs
>
> Signed-off-by: Georg Müller <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]/
> ---
> .../shell/test_uprobe_from_different_cu.sh | 77 +++++++++++++++++++
> 1 file changed, 77 insertions(+)
> create mode 100755 tools/perf/tests/shell/test_uprobe_from_different_cu.sh
>
> diff --git a/tools/perf/tests/shell/test_uprobe_from_different_cu.sh b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> new file mode 100755
> index 000000000000..00d2e0e2e0c2
> --- /dev/null
> +++ b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> @@ -0,0 +1,77 @@
> +#!/bin/bash
> +# test perf probe of function from different CU
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +temp_dir=$(mktemp -d /tmp/perf-uprobe-different-cu-sh.XXXXXXXXXX)
> +
> +cleanup()
> +{
> + trap - EXIT TERM INT
> + if [[ "${temp_dir}" =~ ^/tmp/perf-uprobe-different-cu-sh.*$ ]]; then
> + echo "--- Cleaning up ---"
> + perf probe -x ${temp_dir}/testfile -d foo
> + rm -f "${temp_dir}/"*
> + rmdir "${temp_dir}"
> + fi
> +}
> +
> +trap_cleanup()
> +{
> + cleanup
> + exit 1
> +}
> +
> +trap trap_cleanup EXIT TERM INT
> +
> +cat > ${temp_dir}/testfile-foo.h << EOF
> +struct t
> +{
> + int *p;
> + int c;
> +};
> +
> +extern int foo (int i, struct t *t);
> +EOF
> +
> +cat > ${temp_dir}/testfile-foo.c << EOF
> +#include "testfile-foo.h"
> +
> +int
> +foo (int i, struct t *t)
> +{
> + int j, res = 0;
> + for (j = 0; j < i && j < t->c; j++)
> + res += t->p[j];
> +
> + return res;
> +}
> +EOF
> +
> +cat > ${temp_dir}/testfile-main.c << EOF
> +#include "testfile-foo.h"
> +
> +static struct t g;
> +
> +int
> +main (int argc, char **argv)
> +{
> + int i;
> + int j[argc];
> + g.c = argc;
> + g.p = j;
> + for (i = 0; i < argc; i++)
> + j[i] = (int) argv[i][0];
> + return foo (3, &g);
> +}
> +EOF
> +
> +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

Thanks for the test Georg! By directly relying on gcc this test fails
for me in some constrained environments, like containers. I think
there should be a skip if gcc isn't present. A different option is to
just build the test code into the perf binary itself as a test
workload:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/workloads?h=perf-tools-next

Wdyt? Thanks,
Ian

> +
> +perf probe -x ${temp_dir}/testfile --funcs foo
> +perf probe -x ${temp_dir}/testfile foo
> +
> +cleanup
> --
> 2.41.0
>


2023-07-28 15:15:25

by Georg Müller

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] perf probe: add test for regression introduced by switch to die_get_decl_file


Am 27.07.23 um 19:45 schrieb Ian Rogers:
> On Wed, Jun 28, 2023 at 1:25 AM Georg Müller <[email protected]> wrote:
>
> Thanks for the test Georg! By directly relying on gcc this test fails
> for me in some constrained environments, like containers. I think
> there should be a skip if gcc isn't present. A different option is to
> just build the test code into the perf binary itself as a test
> workload:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/workloads?h=perf-tools-next
>
> Wdyt? Thanks,
> Ian
>

I prepare a commit which checks for gcc and skips the test in this case.

I think building thi test code into the perf binary itself is not an option
here, since the test relies on a special setup of using -flto for one of the
compilation units.

There is also a cleanup issue if anything fails. This will be included in
the patch as well.

Best regards,
Georg