Stefan Hajnoczi <stefa...@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.
>
>> +
>> + struct jr_code_load *load_event = g_new0(struct jr_code_load, 1);
>
> No need to allocate load_event on the heap.
>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 9621e934c0..1c26eeeb9c 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -4147,6 +4147,18 @@ STEXI
>> Enable FIPS 140-2 compliance mode.
>> ETEXI
>>
>> +#ifdef __linux__
>> +DEF("perf", 0, QEMU_OPTION_perf,
>> + "-perf dump jitdump files to help linux perf JIT code
>> visualization\n",
>> + QEMU_ARCH_ALL)
>> +#endif
>> +STEXI
>> +@item -perf
>> +@findex -perf
>> +Dumps jitdump files to help linux perf JIT code visualization
>
> Suggestions on expanding the documentation:
>
> Where are the jitdump files dumped? The current working directory?
>
> Anything to say about the naming scheme for these files?
>
> Can you include an example of how to load them into perf(1)?
--
Alex Bennée
