Re: [PATCH] perf jit: move test functionality in to a test

[Ian Rogers]

On Thu, Nov 28, 2019 at 5:19 AM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Wed, Nov 27, 2019 at 10:49:29AM -0800, Ian Rogers escreveu:
> > On Wed, Nov 27, 2019 at 8:06 AM Arnaldo Carvalho de Melo
> > <arnaldo.melo@xxxxxxxxx> wrote:
> > >
> > > Em Wed, Nov 27, 2019 at 12:23:28PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Tue, Nov 26, 2019 at 03:59:13PM -0800, Ian Rogers escreveu:
> > > > > Adds a test for minimal jit_write_elf functionality.
> > > >
> > > > Thanks, tested and applied.
> > >
> > > Had to apply this to have it built in systems where HAVE_JITDUMP isn't
> > > defined:
> >
> > Thanks for fixing this!
> > Ian
>
> This needs some more work, as it is failing in some of the cross build
> containers, for arches not supported by that genelf.h header, where it
> should just detect that this feature shouldn't be used, warn the user
> and build what is possible to build, e.g.:
>
>   CC       /tmp/build/perf/util/evswitch.o
> In file included from tests/genelf.c:15:
> tests/../util/genelf.h:42:2: error: #error "unsupported architecture"
>    42 | #error "unsupported architecture"
>       |  ^~~~~
> tests/../util/genelf.h:51:5: error: "GEN_ELF_CLASS" is not defined, evaluates to 0 [-Werror=undef]
>    51 | #if GEN_ELF_CLASS == ELFCLASS64
>       |     ^~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> mv: cannot stat '/tmp/build/perf/tests/.genelf.o.tmp': No such file or directory
> make[4]: *** [/git/linux/tools/build/Makefile.build:96: /tmp/build/perf/tests/genelf.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
>   CC       /tmp/build/perf/util/find_bit.o
>   CC       /tmp/build/perf/util/get_current_dir_name.o
>
>
> There is some wiring up to do here, related to:
> [acme@quaco perf]$ vim tools/perf/Makefile.config
> [acme@quaco perf]$ find . -type f| xargs grep PERF_HAVE_JITDUMP
> grep: ./et: No such file or directory
> grep: vi: No such file or directory
> ^Xgrep: ./perf.data: Permission denied
> ./tools/perf/arch/x86/Makefile:PERF_HAVE_JITDUMP := 1
> ./tools/perf/arch/sparc/Makefile:PERF_HAVE_JITDUMP := 1
> ./tools/perf/arch/arm/Makefile:PERF_HAVE_JITDUMP := 1
> ./tools/perf/arch/arm64/Makefile:PERF_HAVE_JITDUMP := 1
> ./tools/perf/arch/powerpc/Makefile:PERF_HAVE_JITDUMP := 1
> ./tools/perf/arch/s390/Makefile:PERF_HAVE_JITDUMP := 1
> ./tools/perf/Makefile.config:ifdef PERF_HAVE_JITDUMP
> ^C
> [acme@quaco perf]$
>
> But I'll defer this for later, not to hold what I have too much, after
> my next pull req I'll revisit this, if you haven't found a fix by then
> :-)
>
> The current patch is below, with my fixes, and looking at it again it
> seems its just to replace that HAVE_LIBELF_SUPPORT with HAVE_JITDUMP,
> will confirm that later, after sending the pull req to Ingo.
>
> - Arnaldo

Your fix sounds correct, I should work on getting a build test setup.
It sounds like you have it covered but if you need me to do anything
let me know.

Thanks,
Ian

> commit d006842faa9a26feb51b818e02c681f064b83b0a
> Author: Ian Rogers <irogers@xxxxxxxxxx>
> Date:   Tue Nov 26 15:59:13 2019 -0800
>
>     perf jit: Move test functionality in to a test
>
>     Adds a test for minimal jit_write_elf functionality.
>
>     Committer testing:
>
>       # perf test jit
>       61: Test jit_write_elf                                    : Ok
>       #
>
>       # perf test -v jit
>       61: Test jit_write_elf                                    :
>       --- start ---
>       test child forked, pid 10460
>       Writing jit code to: /tmp/perf-test-KqxURR
>       test child finished with 0
>       ---- end ----
>       Test jit_write_elf: Ok
>       #
>
>     Committer notes:
>
>     Fix up the case where HAVE_JITDUMP is no defined.
>
>     Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
>     Tested-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>     Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>     Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>     Cc: Alexios Zavras <alexios.zavras@xxxxxxxxx>
>     Cc: Allison Randal <allison@xxxxxxxxxxx>
>     Cc: Florian Fainelli <f.fainelli@xxxxxxxxx>
>     Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>     Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>     Cc: Kate Stewart <kstewart@xxxxxxxxxxxxxxxxxxx>
>     Cc: Leo Yan <leo.yan@xxxxxxxxxx>
>     Cc: Mark Rutland <mark.rutland@xxxxxxx>
>     Cc: Michael Petlan <mpetlan@xxxxxxxxxx>
>     Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>     Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>     Cc: Song Liu <songliubraving@xxxxxx>
>     Cc: Stephane Eranian <eranian@xxxxxxxxxx>
>     Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>     Link: http://lore.kernel.org/lkml/20191126235913.41855-1-irogers@xxxxxxxxxx
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index a3c595fba943..1692529639b0 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -54,6 +54,7 @@ perf-y += unit_number__scnprintf.o
>  perf-y += mem2node.o
>  perf-y += maps.o
>  perf-y += time-utils-test.o
> +perf-y += genelf.o
>
>  $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
>         $(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 7115aa32a51e..970e2ecfb39f 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -296,6 +296,10 @@ static struct test generic_tests[] = {
>                 .desc = "time utils",
>                 .func = test__time_utils,
>         },
> +       {
> +               .desc = "Test jit_write_elf",
> +               .func = test__jit_write_elf,
> +       },
>         {
>                 .desc = "maps__merge_in",
>                 .func = test__maps__merge_in,
> diff --git a/tools/perf/tests/genelf.c b/tools/perf/tests/genelf.c
> new file mode 100644
> index 000000000000..28dfd17a6b9f
> --- /dev/null
> +++ b/tools/perf/tests/genelf.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <limits.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/compiler.h>
> +
> +#include "debug.h"
> +#include "tests.h"
> +
> +
> +#ifdef HAVE_LIBELF_SUPPORT
> +#include <libelf.h>
> +#include "../util/genelf.h"
> +#endif
> +
> +#define TEMPL "/tmp/perf-test-XXXXXX"
> +
> +int test__jit_write_elf(struct test *test __maybe_unused,
> +                       int subtest __maybe_unused)
> +{
> +#ifdef HAVE_JITDUMP
> +       static unsigned char x86_code[] = {
> +               0xBB, 0x2A, 0x00, 0x00, 0x00, /* movl $42, %ebx */
> +               0xB8, 0x01, 0x00, 0x00, 0x00, /* movl $1, %eax */
> +               0xCD, 0x80            /* int $0x80 */
> +       };
> +       char path[PATH_MAX];
> +       int fd, ret;
> +
> +       strcpy(path, TEMPL);
> +
> +       fd = mkstemp(path);
> +       if (fd < 0) {
> +               perror("mkstemp failed");
> +               return TEST_FAIL;
> +       }
> +
> +       pr_info("Writing jit code to: %s\n", path);
> +
> +       ret = jit_write_elf(fd, 0, "main", x86_code, sizeof(x86_code),
> +                       NULL, 0, NULL, 0, 0);
> +       close(fd);
> +
> +       unlink(path);
> +
> +       return ret ? TEST_FAIL : 0;
> +#else
> +       return TEST_SKIP;
> +#endif
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index 25aea387e2bf..0e6d67910b0a 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -109,6 +109,7 @@ int test__unit_number__scnprint(struct test *test, int subtest);
>  int test__mem2node(struct test *t, int subtest);
>  int test__maps__merge_in(struct test *t, int subtest);
>  int test__time_utils(struct test *t, int subtest);
> +int test__jit_write_elf(struct test *test, int subtest);
>
>  bool test__bp_signal_is_supported(void);
>  bool test__bp_account_is_supported(void);
> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> index f9f18b8b1df9..aed49806a09b 100644
> --- a/tools/perf/util/genelf.c
> +++ b/tools/perf/util/genelf.c
> @@ -8,15 +8,12 @@
>   */
>
>  #include <sys/types.h>
> -#include <stdio.h>
> -#include <getopt.h>
>  #include <stddef.h>
>  #include <libelf.h>
>  #include <string.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <inttypes.h>
> -#include <limits.h>
>  #include <fcntl.h>
>  #include <err.h>
>  #ifdef HAVE_DWARF_SUPPORT
> @@ -31,8 +28,6 @@
>  #define NT_GNU_BUILD_ID 3
>  #endif
>
> -#define JVMTI
> -
>  #define BUILD_ID_URANDOM /* different uuid for each run */
>
>  #ifdef HAVE_LIBCRYPTO
> @@ -511,44 +506,3 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>
>         return retval;
>  }
> -
> -#ifndef JVMTI
> -
> -static unsigned char x86_code[] = {
> -    0xBB, 0x2A, 0x00, 0x00, 0x00, /* movl $42, %ebx */
> -    0xB8, 0x01, 0x00, 0x00, 0x00, /* movl $1, %eax */
> -    0xCD, 0x80            /* int $0x80 */
> -};
> -
> -static struct options options;
> -
> -int main(int argc, char **argv)
> -{
> -       int c, fd, ret;
> -
> -       while ((c = getopt(argc, argv, "o:h")) != -1) {
> -               switch (c) {
> -               case 'o':
> -                       options.output = optarg;
> -                       break;
> -               case 'h':
> -                       printf("Usage: genelf -o output_file [-h]\n");
> -                       return 0;
> -               default:
> -                       errx(1, "unknown option");
> -               }
> -       }
> -
> -       fd = open(options.output, O_CREAT|O_TRUNC|O_RDWR, 0666);
> -       if (fd == -1)
> -               err(1, "cannot create file %s", options.output);
> -
> -       ret = jit_write_elf(fd, "main", x86_code, sizeof(x86_code));
> -       close(fd);
> -
> -       if (ret != 0)
> -               unlink(options.output);
> -
> -       return ret;
> -}
> -#endif