On Thu, Aug 15, 2019 at 05:17:49PM +0100, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Wed, Aug 14, 2019 at 11:37:24PM -0300, vandersonmr wrote:
> >> This commit adds support to Linux Perf in order
> >> to be able to analyze qemu jitted code and
> >> also to able to see the TBs PC in it.
> >
> > Is there any reference to the file format?  Please include it in a code
> > comment, if such a thing exists.
> >
> >> diff --git a/accel/tcg/perf/jitdump.c b/accel/tcg/perf/jitdump.c
> >> new file mode 100644
> >> index 0000000000..6f4c0911c2
> >> --- /dev/null
> >> +++ b/accel/tcg/perf/jitdump.c
> >> @@ -0,0 +1,180 @@
> >
> > License header?
> >
> >> +#ifdef __linux__
> >
> > If the entire source file is #ifdef __linux__ then Makefile.objs should
> > probably contain obj-$(CONFIG_LINUX) += jitdump.o instead.  Letting the
> > build system decide what to compile is cleaner than ifdeffing large
> > amounts of code.
> >
> >> +
> >> +#include <stdint.h>
> >> +
> >> +#include <stdio.h>
> >> +#include <unistd.h>
> >> +#include <time.h>
> >> +#include <sys/syscall.h>
> >> +#include <elf.h>
> >> +
> >> +#include "jitdump.h"
> >> +#include "qemu-common.h"
> >
> > Please follow QEMU's header ordering conventions.  See ./HACKING "1.2.
> > Include directives".
> >
> >> +void start_jitdump_file(void)
> >> +{
> >> +    GString *dumpfile_name = g_string_new(NULL);;
> >> +    g_string_printf(dumpfile_name, "./jit-%d.dump", getpid());
> >
> > Simpler:
> >
> >   gchar *dumpfile_name = g_strdup_printf("./jit-%d.dump", getpid());
> >   ...
> >   g_free(dumpfile_name);
> >
> >> +    dumpfile = fopen(dumpfile_name->str, "w+");
> >
> > getpid() and the global dumpfile variable make me wonder what happens
> > with multi-threaded TCG?
> >
> >> +
> >> +    perf_marker = mmap(NULL, sysconf(_SC_PAGESIZE),
> >
> > Please mention the point of this mmap in a comment.  My best guess is
> > that perf stores the /proc/$PID/maps and this is how it finds the
> > jitdump file?
> >
> >> +                          PROT_READ | PROT_EXEC,
> >> +                          MAP_PRIVATE,
> >> +                          fileno(dumpfile), 0);
> >> +
> >> +    if (perf_marker == MAP_FAILED) {
> >> +        printf("Failed to create mmap marker file for perf %d\n", fileno(dumpfile));
> >> +        fclose(dumpfile);
> >> +        return;
> >> +    }
> >> +
> >> +    g_string_free(dumpfile_name, TRUE);
> >> +
> >> +    struct jitheader *header = g_new0(struct jitheader, 1);
> >
> > Why g_new this struct?  It's small and can be declared on the stack.
> >
> > Please use g_free() with g_malloc/new/etc().  It's not safe to mismatch
> > glib and libc memory allocation functions.
> >
> >> +    header->magic = 0x4A695444;
> >> +    header->version = 1;
> >> +    header->elf_mach = get_e_machine();
> >> +    header->total_size = sizeof(struct jitheader);
> >> +    header->pid = getpid();
> >> +    header->timestamp = get_timestamp();
> >> +
> >> +    fwrite(header, header->total_size, 1, dumpfile);
> >> +
> >> +    free(header);
> >> +    fflush(dumpfile);
> >> +}
> >> +
> >> +void append_load_in_jitdump_file(TranslationBlock *tb)
> >> +{
> >> +    GString *func_name = g_string_new(NULL);
> >> +    g_string_printf(func_name, "TB virt:0x"TARGET_FMT_lx"%c", tb->pc, '\0');
> >
> > The explicit NUL character looks strange to me.  I think the idea is to
> > avoid func_name->len + 1?  Adding NUL characters to C strings can be a
> > source of bugs, I would stick to convention and do len + 1 instead of
> > putting NUL characters into the GString.  This is a question of style
> > though.
> 
> The glib functions always add null characters so you shouldn't need to
> manually.

Yep, just remember to do func_name->len + 1 since glib doesn't count the
NUL character that gets added automatically.