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

[Tom Tromey <tromey@redhat.com>]

>>>>> "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