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

[Ilya Leoshkevich]

On Fri, 2023-06-02 at 18:21 +0100, Peter Maydell wrote:
> 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

It's magic that points perf to the location of the jitdump file,
but it won't hurt munmap()ping it in perf_exit(). I'll send a patch.