Re: [PULL 3/5] tcg: add perfmap and jitdump

[Peter Maydell]

On Mon, 16 Jan 2023 at 22:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Ilya Leoshkevich <iii@linux.ibm.com>
>
> Add ability to dump /tmp/perf-<pid>.map and jit-<pid>.dump.
> The first one allows the perf tool to map samples to each individual
> translation block. The second one adds the ability to resolve symbol
> names, line numbers and inspect JITed code.
>
> Example of use:
>
>     perf record qemu-x86_64 -perfmap ./a.out
>     perf report
>
> or
>
>     perf record -k 1 qemu-x86_64 -jitdump ./a.out
>     DEBUGINFOD_URLS= perf inject -j -i perf.data -o perf.data.jitted
>     perf report -i perf.data.jitted
>
> Co-developed-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Co-developed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Message-Id: <20230112152013.125680-4-iii@linux.ibm.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Hi; Coverity thinks (CID 1507521) that there's a memory leak
in this code:

> +void perf_enable_jitdump(void)
> +{
> +    struct jitheader header;
> +    char jitdump_file[32];
> +    void *perf_marker;
> +
> +    if (!use_rt_clock) {
> +        warn_report("CLOCK_MONOTONIC is not available, proceeding without 
> jitdump");
> +        return;
> +    }
> +
> +    snprintf(jitdump_file, sizeof(jitdump_file), "jit-%d.dump", getpid());
> +    jitdump = safe_fopen_w(jitdump_file);
> +    if (jitdump == NULL) {
> +        warn_report("Could not open %s: %s, proceeding without jitdump",
> +                    jitdump_file, strerror(errno));
> +        return;
> +    }
> +
> +    /*
> +     * `perf inject` will see that the mapped file name in the corresponding
> +     * PERF_RECORD_MMAP or PERF_RECORD_MMAP2 event is of the form jit-%d.dump
> +     * and will process it as a jitdump file.
> +     */
> +    perf_marker = mmap(NULL, qemu_real_host_page_size(), PROT_READ | 
> PROT_EXEC,
> +                       MAP_PRIVATE, fileno(jitdump), 0);

Here we mmap() something...

> +    if (perf_marker == MAP_FAILED) {
> +        warn_report("Could not map %s: %s, proceeding without jitdump",
> +                    jitdump_file, strerror(errno));
> +        fclose(jitdump);
> +        jitdump = NULL;
> +        return;
> +    }
> +
> +    header.magic = JITHEADER_MAGIC;
> +    header.version = JITHEADER_VERSION;
> +    header.total_size = sizeof(header);
> +    header.elf_mach = get_e_machine();
> +    header.pad1 = 0;
> +    header.pid = getpid();
> +    header.timestamp = get_clock();
> +    header.flags = 0;
> +    fwrite(&header, sizeof(header), 1, jitdump);

...but we never do anything with that pointer, so the memory
we just mmap()ed is never going to be freed.

Is this doing something particularly magical, or should that
pointer be kept track of somewhere ?

thanks
-- PMM