Re: [PATCH] add -s option to make -break-insert support dprintf

Tom Tromey
2013-05-07T20:50:04Z

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> According to your comments. I did some update with these patches to
Hui> added special command -dprintf-insert to insert dprintf. Its format
Hui> is close to simple dprintf command:
Hui> -dprintf-insert LOCATION FORMAT ARG ARG ...

I like this approach much more.
Thanks for doing it.

Hui> +static char *
Hui> +mi_argv_to_format (int format_num, char **argv, int argc)

This needs an introductory comment explaining the arguments and result.

Hui> + /* If all the string need convert to \ddd mode, so * 2.
Hui> + + 2 for two ".
Hui> + + 1 for \0. */
Hui> + format_size = strlen (argv[format_num]) * 4 + 3;

The comment says "* 2" but the code says "* 4".
Personally I'd just use an obstack rather than mess around with explicit
reallocs and the like.

Hui> + sprintf (format + format_current_size, "\\%o",
Hui> + argv[format_num][i]);

It seems that this could do the wrong thing for a char that
sign-extends.

Hui> + /* Apply other argv to FORMAT. */
Hui> + for (i = format_num + 1; i < argc; i++)

It seems to me that it would be better to just pass in argv+argc to
mi_argv_to_format, and omit the format_num argument entirely.

Hui> +static void
Hui> +mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)

Intro comment.

Hui> + extra_string = mi_argv_to_format (oind + 1, argv, argc);
Hui> + make_cleanup (xfree, extra_string);

This makes a dangling cleanup.

Hui> + if (tracepoint)
Hui> + {
Hui> + /* Note that to request a fast tracepoint, the client uses the
Hui> + "hardware" flag, although there's nothing of hardware related to
Hui> + fast tracepoints -- one can implement slow tracepoints with
Hui> + hardware breakpoints, but fast tracepoints are always software.
Hui> + "fast" is a misnomer, actually, "jump" would be more appropriate.
Hui> + A simulator or an emulator could conceivably implement fast
Hui> + regular non-jump based tracepoints. */
Hui> + type_wanted = hardware ? bp_fast_tracepoint : bp_tracepoint;
Hui> + ops = &tracepoint_breakpoint_ops;
Hui> + }
Hui> + else if (dprintf)
Hui> + {

It seems that 'tracepoint' and 'dprintf' are exclusive, so this should
be checked above where "hardware" is checked.

Tom