Re: [PATCH v2 1/1] gdb, breakpoint: add breakpoint location debugging logs

[Simon Marchi <simark@simark.ca>]

On 3/13/23 14:34, Christina Schimpe via Gdb-patches wrote:
> From: Mihails Strasuns <mihails.strasuns@intel.com>
> 
> Add new commands:
> 
>   set debug breakpoints on|off
>   show debug breakpoints
> 
> This patch introduces new debugging information that prints
> breakpoint location insertion and removal flow.
> 
> The debug output looks like:
> ~~~
> (gdb) set debug breakpoints on
> (gdb) disassemble main
> Dump of assembler code for function main:
>    0x0000555555555129 <+0>:	endbr64
>    0x000055555555512d <+4>:	push   %rbp
>    0x000055555555512e <+5>:	mov    %rsp,%rbp
> => 0x0000555555555131 <+8>:	mov    $0x0,%eax
>    0x0000555555555136 <+13>:	pop    %rbp
>    0x0000555555555137 <+14>:	ret
> End of assembler dump.
> (gdb) break *0x0000555555555137
> Breakpoint 2 at 0x555555555137: file main.c, line 4.
> [breakpoints] update_global_location_list: UGLL_MAY_INSERT
> (gdb) c
> Continuing.
> [breakpoints] update_global_location_list: UGLL_INSERT
> [breakpoints] insert_bp_location: bp_location (0x562881637fb0) at address 0x555555555137 in main at main.c:4
> [breakpoints] insert_bp_location: bp_location (0x56288179a4f0) at address 0x7ffff7fd37b5 <dl_main+8661>
> [breakpoints] insert_bp_location: bp_location (0x56288179ea60) at address 0x7ffff7fe509e <dl_open_worker+1150>
> [breakpoints] insert_bp_location: bp_location (0x5628817184d0) at address 0x7ffff7fe63f4 <_dl_close_worker+2356>
> [breakpoints] remove_breakpoint_1: bp_location (0x562881637fb0) due to regular remove at address 0x555555555137 in main at main.c:4
> [breakpoints] remove_breakpoint_1: bp_location (0x56288179a4f0) due to regular remove at address 0x7ffff7fd37b5 <dl_main+8661>
> [breakpoints] remove_breakpoint_1: bp_location (0x56288179ea60) due to regular remove at address 0x7ffff7fe509e <dl_open_worker+1150>
> [breakpoints] remove_breakpoint_1: bp_location (0x5628817184d0) due to regular remove at address 0x7ffff7fe63f4 <_dl_close_worker+2356>

Hi,

Thanks for doing this.  Debug output is always appreciated.

I just have some minor comments below, with those fixed the patch LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index abee22cd162..e3666c34bee 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -163,6 +163,8 @@ static bool bl_address_is_meaningful (bp_location *loc);
>  
>  static int find_loc_num_by_location (const bp_location *loc);
>  
> +static std::string breakpoint_location_to_buffer (bp_location *bl);

Instead of adding a forward declaration, you can just put the new
function wherever needed so it's seen by all its users.

> @@ -198,6 +200,22 @@ enum ugll_insert_mode
>    UGLL_INSERT
>  };
>  
> +static const char *
> +ugll_insert_mode_text (ugll_insert_mode insert_mode)

For completeness, please add a comment for this function.  It can be:

/* Return a textual version of INSERT_MODE.  */

> @@ -508,6 +526,19 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
>  	      value);
>  }
>  
> +static bool debug_breakpoints = false;

Add comment:

/* True if breakpoints debug output is enabled.  */

> +
> +#define breakpoint_debug_printf(fmt, ...) \
> +  debug_prefixed_printf_cond (debug_breakpoints, "breakpoints",fmt,\

Add spaces after the last two commas.

> +			      ##__VA_ARGS__)

Add comment:

/* Print a "breakpoints" debug statement.  */

> +
> +static void
> +show_debug_breakpoints (struct ui_file *file, int from_tty,
> +			struct cmd_list_element *c, const char *value)
> +{
> +  gdb_printf (file, _("Breakpoint location debugging is %s.\n"), value);
> +}

Add comment:

/* "show debug breakpoints" implementation.  */

> @@ -3264,6 +3300,8 @@ remove_breakpoints_inf (inferior *inf)
>  {
>    int val;
>  
> +  breakpoint_debug_printf ("remove_breakpoints_inf (%d)", inf->num);

No need to say remove_breakpoints_inf in the message, it's the name of
the function, so it will automatically be added.  I would suggest the
following format:

  "inf->num = %d", inf->num

> @@ -6210,6 +6255,16 @@ print_breakpoint_location (const breakpoint *b,
>      }
>  }
>  
> +static std::string
> +breakpoint_location_to_buffer (bp_location *bl)

Add comment:

/* Return the output of print_breakpoint_location for BL as a string.  */

> +{
> +  string_file stb;
> +  current_uiout->redirect (&stb);
> +  print_breakpoint_location (bl->owner, bl);
> +  current_uiout->redirect (nullptr);
> +  return stb.string ();
> +}
> +
>  static const char *
>  bptype_string (enum bptype type)
>  {
> @@ -11140,6 +11195,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>    /* Last breakpoint location program space that was marked for update.  */
>    int last_pspace_num = -1;
>  
> +  breakpoint_debug_printf ("%s", ugll_insert_mode_text (insert_mode));

I would suggest the following format:

  "insert_mode = %s", ugll_insert_mode_text (insert_mode)

Simon