| Message ID | 1454597781-18115-7-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 02/05/2016 01:56 AM, Alex Bennée wrote: > + gchar *range_op = g_strstr_len(r, -1, "-"); This is strchr. > + range_op = g_strstr_len(r, -1, "."); Or at least if you're going to make use of strstr, search for "..". > + g_strdelimit(range_val, ".", ' '); Cause this is just weird. It accepts "1+.2" just the same as "1..2". > + err = qemu_strtoul(r, NULL, 0, &range.begin); This should be strtoull. You probably broke 32-bit compilation here. > + case '+': > + { > + unsigned long len; > + err |= qemu_strtoul(range_val, NULL, 0, &len); > + range.end = range.begin + len; > + break; > + } > + case '-': > + { > + unsigned long len; > + err |= qemu_strtoul(range_val, NULL, 0, &len); > + range.end = range.begin; > + range.begin = range.end - len; > + break; > + } Both of these have off-by-one bugs, since end is inclusive. > + case '.': > + err |= qemu_strtoul(range_val, NULL, 0, &range.end); > + break; I'd think multiple dot detection belongs here, and you need not smash them to ' ' but merely notice that there are two of them and then strtoull range_val+1. r~
Richard Henderson <rth@twiddle.net> writes: > On 02/05/2016 01:56 AM, Alex Bennée wrote: >> + gchar *range_op = g_strstr_len(r, -1, "-"); > > This is strchr. > >> + range_op = g_strstr_len(r, -1, "."); > > Or at least if you're going to make use of strstr, search for "..". > >> + g_strdelimit(range_val, ".", ' '); > > Cause this is just weird. It accepts "1+.2" just the same as "1..2". > >> + err = qemu_strtoul(r, NULL, 0, &range.begin); > > This should be strtoull. You probably broke 32-bit compilation here. OK I think this version is a lot cleaner: void qemu_set_dfilter_ranges(const char *filter_spec) { gchar **ranges = g_strsplit(filter_spec, ",", 0); if (ranges) { gchar **next = ranges; gchar *r = *next++; debug_regions = g_array_sized_new(FALSE, FALSE, sizeof(Range), g_strv_length(ranges)); while (r) { gchar *range_op = g_strstr_len(r, -1, "-"); gchar *r2 = range_op ? range_op + 1 : NULL; if (!range_op) { range_op = g_strstr_len(r, -1, "+"); r2 = range_op ? range_op + 1 : NULL; } if (!range_op) { range_op = g_strstr_len(r, -1, ".."); r2 = range_op ? range_op + 2 : NULL; } if (range_op) { struct Range range; int err; const char *e = NULL; err = qemu_strtoull(r, &e, 0, &range.begin); g_assert(e == range_op); switch (*range_op) { case '+': { unsigned long len; err |= qemu_strtoull(r2, NULL, 0, &len); range.end = range.begin + len; break; } case '-': { unsigned long len; err |= qemu_strtoull(r2, NULL, 0, &len); range.end = range.begin; range.begin = range.end - len; break; } case '.': err |= qemu_strtoull(r2, NULL, 0, &range.end); break; default: g_assert_not_reached(); } if (err) { g_error("Failed to parse range in: %s, %d", r, err); } else { g_array_append_val(debug_regions, range); } } else { g_error("Bad range specifier in: %s", r); } r = *next++; } g_strfreev(ranges); } } > >> + case '+': >> + { >> + unsigned long len; >> + err |= qemu_strtoul(range_val, NULL, 0, &len); >> + range.end = range.begin + len; >> + break; >> + } >> + case '-': >> + { >> + unsigned long len; >> + err |= qemu_strtoul(range_val, NULL, 0, &len); >> + range.end = range.begin; >> + range.begin = range.end - len; >> + break; >> + } > > Both of these have off-by-one bugs, since end is inclusive. Sorry I don't quite follow, do you mean the position of range_val (now r2) or the final value of range.end? > >> + case '.': >> + err |= qemu_strtoul(range_val, NULL, 0, &range.end); >> + break; > > I'd think multiple dot detection belongs here, and you need not smash them to ' > ' but merely notice that there are two of them and then strtoull > range_val+1. I think this is covered with the new code now. > > > r~ -- Alex Bennée
On 02/11/2016 04:40 AM, Alex Bennée wrote: > OK I think this version is a lot cleaner: > > void qemu_set_dfilter_ranges(const char *filter_spec) > { > gchar **ranges = g_strsplit(filter_spec, ",", 0); > if (ranges) { > gchar **next = ranges; > gchar *r = *next++; > debug_regions = g_array_sized_new(FALSE, FALSE, > sizeof(Range), g_strv_length(ranges)); > while (r) { > gchar *range_op = g_strstr_len(r, -1, "-"); > gchar *r2 = range_op ? range_op + 1 : NULL; > if (!range_op) { > range_op = g_strstr_len(r, -1, "+"); > r2 = range_op ? range_op + 1 : NULL; > } > if (!range_op) { > range_op = g_strstr_len(r, -1, ".."); > r2 = range_op ? range_op + 2 : NULL; > } I guess I'll quit quibbling about silly glib functions. But really, with the -1 argument, you gain nothing except obfuscation over using the standard C library. > if (range_op) { > struct Range range; > int err; > const char *e = NULL; > > err = qemu_strtoull(r, &e, 0, &range.begin); > > g_assert(e == range_op); > > switch (*range_op) { > case '+': > { > unsigned long len; > err |= qemu_strtoull(r2, NULL, 0, &len); You can't or errno's together and then... > g_error("Failed to parse range in: %s, %d", r, err); ... expect to get anything meaningful out of them. >>> + case '+': >>> + { >>> + unsigned long len; >>> + err |= qemu_strtoul(range_val, NULL, 0, &len); >>> + range.end = range.begin + len; >>> + break; >>> + } >>> + case '-': >>> + { >>> + unsigned long len; >>> + err |= qemu_strtoul(range_val, NULL, 0, &len); >>> + range.end = range.begin; >>> + range.begin = range.end - len; >>> + break; >>> + } >> >> Both of these have off-by-one bugs, since end is inclusive. > > Sorry I don't quite follow, do you mean the position of range_val (now > r2) or the final value of range.end? Final value of range.end. In that 0x1000..0x1000 and 0x1000+1 should both produce a range that covers a single byte at 0x1000. r~
Richard Henderson <rth@twiddle.net> writes: > On 02/11/2016 04:40 AM, Alex Bennée wrote: >> OK I think this version is a lot cleaner: >> >> void qemu_set_dfilter_ranges(const char *filter_spec) >> { >> gchar **ranges = g_strsplit(filter_spec, ",", 0); >> if (ranges) { >> gchar **next = ranges; >> gchar *r = *next++; >> debug_regions = g_array_sized_new(FALSE, FALSE, >> sizeof(Range), g_strv_length(ranges)); >> while (r) { >> gchar *range_op = g_strstr_len(r, -1, "-"); >> gchar *r2 = range_op ? range_op + 1 : NULL; >> if (!range_op) { >> range_op = g_strstr_len(r, -1, "+"); >> r2 = range_op ? range_op + 1 : NULL; >> } >> if (!range_op) { >> range_op = g_strstr_len(r, -1, ".."); >> r2 = range_op ? range_op + 2 : NULL; >> } > > I guess I'll quit quibbling about silly glib functions. But really, with the > -1 argument, you gain nothing except obfuscation over using the > standard C library. No you are quite right to quibble. It's a hard habit to break because I've gotten used to glib's arguably more predictable behaviour when string munging. > >> if (range_op) { >> struct Range range; >> int err; >> const char *e = NULL; >> >> err = qemu_strtoull(r, &e, 0, &range.begin); >> >> g_assert(e == range_op); >> >> switch (*range_op) { >> case '+': >> { >> unsigned long len; >> err |= qemu_strtoull(r2, NULL, 0, &len); > > You can't or errno's together and then... > >> g_error("Failed to parse range in: %s, %d", r, err); > > ... expect to get anything meaningful out of them. True, I'll drop the %d, I was just trying to avoid having multiple error handling legs. > >>>> + case '+': >>>> + { >>>> + unsigned long len; >>>> + err |= qemu_strtoul(range_val, NULL, 0, &len); >>>> + range.end = range.begin + len; >>>> + break; >>>> + } >>>> + case '-': >>>> + { >>>> + unsigned long len; >>>> + err |= qemu_strtoul(range_val, NULL, 0, &len); >>>> + range.end = range.begin; >>>> + range.begin = range.end - len; >>>> + break; >>>> + } >>> >>> Both of these have off-by-one bugs, since end is inclusive. >> >> Sorry I don't quite follow, do you mean the position of range_val (now >> r2) or the final value of range.end? > > Final value of range.end. In that > > 0x1000..0x1000 > and > 0x1000+1 > > should both produce a range that covers a single byte at 0x1000. Ahh OK. I suppose if I'm being good about this I should add some tests to defend the ranges. I wonder how easy command line parsing unit tests are in qtest? > > > r~ -- Alex Bennée
diff --git a/include/qemu/log.h b/include/qemu/log.h index 776ceaf..34a8fe6 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -190,6 +190,8 @@ static inline void qemu_set_log(int log_flags) } void qemu_set_log_filename(const char *filename); +void qemu_set_dfilter_ranges(const char *ranges); +bool qemu_log_in_addr_range(uint64_t addr); int qemu_str_to_log_mask(const char *str); /* Print a usage message listing all the valid logging categories diff --git a/qemu-log.c b/qemu-log.c index 2eebac0..5433a78 100644 --- a/qemu-log.c +++ b/qemu-log.c @@ -19,11 +19,13 @@ #include "qemu-common.h" #include "qemu/log.h" +#include "qemu/range.h" static char *logfilename; FILE *qemu_logfile; int qemu_loglevel; static int log_append = 0; +static GArray *debug_regions; void qemu_log(const char *fmt, ...) { @@ -92,6 +94,89 @@ void qemu_set_log_filename(const char *filename) qemu_set_log(qemu_loglevel); } +/* Returns true if addr is in our debug filter or no filter defined + */ +bool qemu_log_in_addr_range(uint64_t addr) +{ + if (debug_regions) { + int i = 0; + for (i = 0; i < debug_regions->len; i++) { + struct Range *range = &g_array_index(debug_regions, Range, i); + if (addr >= range->begin && addr <= range->end) { + return true; + } + } + return false; + } else { + return true; + } +} + + +void qemu_set_dfilter_ranges(const char *filter_spec) +{ + gchar **ranges = g_strsplit(filter_spec, ",", 0); + if (ranges) { + gchar **next = ranges; + gchar *r = *next++; + debug_regions = g_array_sized_new(FALSE, FALSE, + sizeof(Range), g_strv_length(ranges)); + while (r) { + gchar *range_op = g_strstr_len(r, -1, "-"); + if (!range_op) { + range_op = g_strstr_len(r, -1, "+"); + } + if (!range_op) { + range_op = g_strstr_len(r, -1, "."); + } + if (range_op) { + struct Range range; + int err; + gchar d = *range_op; + gchar *range_val = range_op + 1; + /* ensure first value terminated and stray extra + * range_ops are swallowed to keep qemu_stroul happy + */ + *range_op = 0; + g_strdelimit(range_val, ".", ' '); + + err = qemu_strtoul(r, NULL, 0, &range.begin); + switch (d) { + case '+': + { + unsigned long len; + err |= qemu_strtoul(range_val, NULL, 0, &len); + range.end = range.begin + len; + break; + } + case '-': + { + unsigned long len; + err |= qemu_strtoul(range_val, NULL, 0, &len); + range.end = range.begin; + range.begin = range.end - len; + break; + } + case '.': + err |= qemu_strtoul(range_val, NULL, 0, &range.end); + break; + default: + g_assert_not_reached(); + } + if (err) { + g_error("Failed to parse range in: %s, %d", r, err); + } else { + g_array_append_val(debug_regions, range); + } + } else { + g_error("Bad range specifier in: %s", r); + } + r = *next++; + } + g_strfreev(ranges); + } +} + const QEMULogItem qemu_log_items[] = { { CPU_LOG_TB_OUT_ASM, "out_asm", "show generated host assembly code for each compiled TB" }, diff --git a/qemu-options.hx b/qemu-options.hx index f31a240..ca0f4c9 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3090,6 +3090,24 @@ STEXI Output log in @var{logfile} instead of to stderr ETEXI +DEF("dfilter", HAS_ARG, QEMU_OPTION_DFILTER, \ + "-dfilter range,.. filter debug output to range of addresses (useful for -d cpu,exec,etc..)\n", + QEMU_ARCH_ALL) +STEXI +@item -dfilter @var{range1}[,...] +@findex -dfilter +Filter debug output to that relevant to a range of target addresses. The filter +spec can be either @var{start}+@var{size}, @var{start}-@var{size} or +@var{start}..@var{end} where @var{start} @var{end} and @var{size} are the +addresses and sizes required. For example: +@example + -dfilter 0x8000..0x9000,0xffffffc000080000+0x200,0xffffffc000060000-0x1000 +@end example +Will dump output for any code in the 0x1000 sized block starting at 0x8000 and +the 0x200 sized block starting at 0xffffffc000080000 and another 0x1000 sized +block starting at 0xffffffc00005f000. +ETEXI + DEF("L", HAS_ARG, QEMU_OPTION_L, \ "-L path set the directory for the BIOS, VGA BIOS and keymaps\n", QEMU_ARCH_ALL) diff --git a/vl.c b/vl.c index f043009..a0a546f 100644 --- a/vl.c +++ b/vl.c @@ -3361,6 +3361,9 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_D: log_file = optarg; break; + case QEMU_OPTION_DFILTER: + qemu_set_dfilter_ranges(optarg); + break; case QEMU_OPTION_s: add_device_config(DEV_GDB, "tcp::" DEFAULT_GDBSTUB_PORT); break;
When debugging big programs or system emulation sometimes you want both the verbosity of cpu,exec et all but don't want to generate lots of logs for unneeded stuff. This patch adds a new option -dfilter which allows you to specify interesting address ranges in the form: -dfilter 0x8000..0x9000,0xffffffc000080000+0x200,... Then logging code can use the new qemu_log_in_addr_range() function to decide if it will output logging information for the given range. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - More clean-ups to the documentation v3 - re-base - use GArray instead of GList to avoid cache bouncing - checkpatch fixes v5 - minor re-base conflicts - *did not* convert to -d dfilter=x fmt as easier to document - strtoul -> qemu_strtoul - fix a few checkpatch spacing warnings - made range operator .., - now intuitively subtracts from start - no added r-b tag as changes a bit --- include/qemu/log.h | 2 ++ qemu-log.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu-options.hx | 18 ++++++++++++ vl.c | 3 ++ 4 files changed, 108 insertions(+)