On Mon, Nov 27, 2017 at 5:00 AM, Markus Armbruster <arm...@redhat.com> wrote: > Drive-by question... > > Doug Gale <doug...@gmail.com> writes: > >> Signed-off-by: Doug Gale <doug...@gmail.com> >> --- >> gdbstub.c | 100 >> ++++++++++++++++++++++++++++++++++++++--------------------- >> trace-events | 21 +++++++++++++ >> 2 files changed, 86 insertions(+), 35 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 2a94030d3b..a75f319bd0 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -21,6 +21,7 @@ >> #include "qemu/error-report.h" >> #include "qemu/cutils.h" >> #include "cpu.h" >> +#include "trace-root.h" >> #ifdef CONFIG_USER_ONLY >> #include "qemu.h" >> #else >> @@ -287,21 +288,6 @@ static int gdb_signal_to_target (int sig) >> return -1; >> } >> >> -/* #define DEBUG_GDB */ >> - >> -#ifdef DEBUG_GDB >> -# define DEBUG_GDB_GATE 1 >> -#else >> -# define DEBUG_GDB_GATE 0 >> -#endif >> - >> -#define gdb_debug(fmt, ...) do { \ >> - if (DEBUG_GDB_GATE) { \ >> - fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \ >> - } \ >> -} while (0) >> - >> - >> typedef struct GDBRegisterState { >> int base_reg; >> int num_regs; >> @@ -538,12 +524,47 @@ static void hextomem(uint8_t *mem, const char *buf, >> int len) >> } >> } >> >> +static void hexdump(const char *buf, int len, >> + void (*trace_fn)(size_t ofs, char const *text)) >> +{ >> + char line_buffer[3 * 16 + 4 + 16 + 1]; >> + >> + for (size_t i = 0; i < len || (i & 0xF); ++i) { >> + size_t byte_ofs = i & 15; >> + >> + if (byte_ofs == 0) { >> + memset(line_buffer, ' ', 3 * 16 + 4 + 16); >> + line_buffer[3 * 16 + 4 + 16] = 0; >> + } >> + >> + size_t col_group = (i >> 2) & 3; >> + size_t hex_col = byte_ofs * 3 + col_group; >> + size_t txt_col = 3 * 16 + 4 + byte_ofs; >> + >> + if (i < len) { >> + char value = buf[i]; >> + >> + line_buffer[hex_col + 0] = tohex((value >> 4) & 0xF); >> + line_buffer[hex_col + 1] = tohex((value >> 0) & 0xF); >> + line_buffer[txt_col + 0] = (value >= ' ' && value < 127) >> + ? value >> + : '.'; >> + } >> + >> + if (byte_ofs == 0xF) >> + trace_fn(i & -16, line_buffer); >> + } >> +} > > Could existing qemu_hexdump() serve? > >> + >> /* return -1 if error, 0 if OK */ >> -static int put_packet_binary(GDBState *s, const char *buf, int len) >> +static int put_packet_binary(GDBState *s, const char *buf, int len, bool >> dump) >> { >> int csum, i; >> uint8_t *p; >> >> + if (TRACE_GDBSTUB_IO_BINARYREPLY_ENABLED && dump) >> + hexdump(buf, len, trace_gdbstub_io_binaryreply); >> + >> for(;;) { >> p = s->last_packet; >> *(p++) = '$'; > [...]
It is incorrect to use qemu_hexdump, tracing isn't printf. The whole point of this patch is to _not_ dump traces to printf. I have already submitted tracing that used printf, and it was rejected with a request to use trace_ infrastructure. It might be a better idea to refactor qemu_hexdump to use this algorithm and use a trace_fn callback which dumps lines to printf for qemu_hexdump case. qemu_hexdump hammers printf with a couple of characters at a time of output and uses several loops, this algorithm generates whole lines and more efficiently does one I/O per line. This algorithm also breaks the hex into groups of four bytes to make it easy to visually find a given byte. I opted not to do that refactor of qemu_hexdump to reduce the scope of my patch. Should I do that and resubmit?