This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Tom,

Thanks for your review.

On Wed, May 8, 2013 at 4:49 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "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.

Add:
/* Convert arguments in ARGV to the string in "format",argv,argv...
   and return it.  */

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

Obstack is better than what I did in before.   Thanks for that.
Updated patch for it.

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

I changed this part to:
sprintf (tmp, "\\%o", (unsigned char)argv[format_num][i]);

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

Fixed.

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

/* Insert breakpoint.
   If dprintf is true, it will insert dprintf.
   If not, it will insert other type breakpoint.  */

>
> Hui> +      extra_string = mi_argv_to_format (oind + 1, argv, argc);
> Hui> +      make_cleanup (xfree, extra_string);
>
> This makes a dangling cleanup.

Fixed.

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

Fixed.

I post  the new patches that updated according to your comments.

Thanks,
Hui

2013-05-10  Hui Zhu  <hui@codesourcery.com>

	* breakpoint.c (dprintf_breakpoint_ops): Remove its static.
	* breakpoint.h (dprintf_breakpoint_ops): Add extern.
	* mi/mi-cmd-break.c (ctype.h): New include.
	(gdb_obstack.h): New include.
	(mi_argv_to_format, mi_cmd_break_insert_1): New.
	(mi_cmd_break_insert): Call mi_cmd_break_insert_1.
	(mi_cmd_dprintf_insert): New.
	* mi/mi-cmds.c (mi_cmds): Add "dprintf-insert".
	* mi/mi-cmds.h (mi_cmd_dprintf_insert): New extern.

2013-05-10  Hui Zhu  <hui@codesourcery.com>

	* gdb.texinfo (GDB/MI Breakpoint Commands): Describe the
	"-dprintf-insert" command.

2013-05-10  Hui Zhu  <hui@codesourcery.com>

	* gdb.mi/Makefile.in (PROGS): Add "mi-dprintf".
	* gdb.mi/mi-dprintf.exp, gdb.mi/mi-dprintf.c: New.

Attachment: mi-dprintf.txt
Description: Text document

Attachment: mi-dprintf-doc.txt
Description: Text document

Attachment: mi-dprintf-test.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]