[PATCH v5 0/1] Add breakpoint location debugging logs

[PATCH v5 0/1] Add breakpoint location debugging logs

[Christina Schimpe]

Hi all,

this is my v5 for the patch "gdb, breakpoint: add breakpoint location debugging
logs", which addresses the feedback of Andrew.
The documentation part has been approved by Eli.

V4 of this patch can be found here: 
https://sourceware.org/pipermail/gdb-patches/2023-June/200012.html

Changes since V4:
* Remove 's' from "set debug breakpointS on"
* Remove 'location' from logging output
* Move NEWS entry to the correct location
* Add missing function comment

Regards,

Christina
Mihails Strasuns (1):
  gdb, breakpoint: add breakpoint location debugging logs

 gdb/NEWS            |   4 ++
 gdb/breakpoint.c    | 109 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/breakpoint.h    |   4 ++
 gdb/doc/gdb.texinfo |   8 ++++
 4 files changed, 125 insertions(+)

-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Christina Schimpe]

From: Mihails Strasuns <mihails.strasuns@intel.com>

Add new commands:

  set debug breakpoint on|off
  show debug breakpoint

This patch introduces new debugging information that prints
breakpoint location insertion and removal flow.

The debug output looks like:
~~~
(gdb) set debug breakpoint 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.
[breakpoint] update_global_location_list: insert_mode = UGLL_MAY_INSERT
(gdb) c
Continuing.
[breakpoint] update_global_location_list: insert_mode = UGLL_INSERT
[breakpoint] insert_bp_location: Breakpoint 2 (0x5565daddb1e0) at address 0x555555555137 in main at main.c:4
[breakpoint] insert_bp_location: Breakpoint -2 (0x5565dab51c10) at address 0x7ffff7fd37b5
[breakpoint] insert_bp_location: Breakpoint -5 (0x5565dab68f30) at address 0x7ffff7fe509e
[breakpoint] insert_bp_location: Breakpoint -7 (0x5565dab694f0) at address 0x7ffff7fe63f4
[breakpoint] remove_breakpoint_1: Breakpoint 2 (0x5565daddb1e0) at address 0x555555555137 in main at main.c:4 due to regular remove
[breakpoint] remove_breakpoint_1: Breakpoint -2 (0x5565dab51c10) at address 0x7ffff7fd37b5 due to regular remove
[breakpoint] remove_breakpoint_1: Breakpoint -5 (0x5565dab68f30) at address 0x7ffff7fe509e due to regular remove
[breakpoint] remove_breakpoint_1: Breakpoint -7 (0x5565dab694f0) at address 0x7ffff7fe63f4 due to regular remove

Breakpoint 2, 0x0000555555555137 in main () at main.c:4
4	}
~~~

Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
---
 gdb/NEWS            |   4 ++
 gdb/breakpoint.c    | 109 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/breakpoint.h    |   4 ++
 gdb/doc/gdb.texinfo |   8 ++++
 4 files changed, 125 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 649a3a9824a..2aafe103dab 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -80,6 +80,10 @@
 
 * New commands
 
+set debug breakpoint on|off
+  show debug breakpoint
+  Print additional debug messages about breakpoint insertion and removal.
+
 maintenance print record-instruction [ N ]
   Print the recorded information for a given instruction.  If N is not given
   prints how GDB would undo the last instruction executed.  If N is negative,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index da6c8de9d14..dc502df9d79 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -83,6 +83,7 @@
 #include "progspace-and-thread.h"
 #include "gdbsupport/array-view.h"
 #include "gdbsupport/gdb_optional.h"
+#include "gdbsupport/common-utils.h"
 
 /* Prototypes for local functions.  */
 
@@ -200,6 +201,68 @@ enum ugll_insert_mode
   UGLL_INSERT
 };
 
+/* Return a textual version of INSERT_MODE.  */
+
+static const char *
+ugll_insert_mode_text (ugll_insert_mode insert_mode)
+{
+/* Make sure the compiler warns if a new ugll_insert_mode enumerator is added
+   but not handled here.  */
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_ERROR_SWITCH
+  switch (insert_mode)
+    {
+    case UGLL_DONT_INSERT:
+      return "UGLL_DONT_INSERT";
+    case UGLL_MAY_INSERT:
+      return "UGLL_MAY_INSERT";
+    case UGLL_INSERT:
+      return "UGLL_INSERT";
+    }
+DIAGNOSTIC_POP
+
+  gdb_assert_not_reached ("must handle all enum values");
+}
+
+/* Return a textual version of REASON.  */
+
+static const char *
+remove_bp_reason_str (remove_bp_reason reason)
+{
+/* Make sure the compiler warns if a new remove_bp_reason enumerator is added
+   but not handled here.  */
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_ERROR_SWITCH
+  switch (reason)
+    {
+    case REMOVE_BREAKPOINT:
+      return "regular remove";
+    case DETACH_BREAKPOINT:
+      return "detach";
+    }
+DIAGNOSTIC_POP
+
+  gdb_assert_not_reached ("must handle all enum values");
+}
+
+/* Return a textual version of breakpoint BL describing number, location and
+   address.  */
+
+static std::string
+breakpoint_location_address_str (const bp_location* bl)
+{
+  std::string str = string_printf ("Breakpoint %d (%s) at address %s",
+				   bl->owner->number,
+				   host_address_to_string (bl),
+				   paddress (bl->gdbarch, bl->address));
+
+  std::string loc_string = bl->to_string ();
+  if (!loc_string.empty ())
+    str += string_printf (" %s", loc_string.c_str ());
+
+  return str;
+}
+
 static void update_global_location_list (enum ugll_insert_mode);
 
 static void update_global_location_list_nothrow (enum ugll_insert_mode);
@@ -510,6 +573,22 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
 	      value);
 }
 
+/* True if breakpoint debug output is enabled.  */
+static bool debug_breakpoint = false;
+
+/* Print a "breakpoint" debug statement.  */
+#define breakpoint_debug_printf(fmt, ...) \
+  debug_prefixed_printf_cond (debug_breakpoint, "breakpoint", fmt, \
+			      ##__VA_ARGS__)
+
+/* "show debug breakpoint" implementation.  */
+static void
+show_debug_breakpoint (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);
+}
+
 /* See breakpoint.h.  */
 
 int
@@ -2728,6 +2807,8 @@ insert_bp_location (struct bp_location *bl,
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
 
+  breakpoint_debug_printf ("%s", breakpoint_location_address_str (bl).c_str ());
+
   /* Note we don't initialize bl->target_info, as that wipes out
      the breakpoint location's shadow_contents if the breakpoint
      is still inserted at that location.  This in turn breaks
@@ -3270,6 +3351,8 @@ remove_breakpoints_inf (inferior *inf)
 {
   int val;
 
+  breakpoint_debug_printf ("inf->num = %d", inf->num);
+
   for (bp_location *bl : all_bp_locations ())
     {
       if (bl->pspace != inf->pspace)
@@ -3914,6 +3997,10 @@ detach_breakpoints (ptid_t ptid)
 static int
 remove_breakpoint_1 (struct bp_location *bl, enum remove_bp_reason reason)
 {
+  breakpoint_debug_printf ("%s due to %s",
+			   breakpoint_location_address_str (bl).c_str (),
+			   remove_bp_reason_str (reason));
+
   int val;
 
   /* BL is never in moribund_locations by our callers.  */
@@ -7424,6 +7511,16 @@ bp_location::bp_location (breakpoint *owner)
 {
 }
 
+/* See breakpoint.h.  */
+
+std::string bp_location::to_string () const
+{
+  string_file stb;
+  ui_out_redirect_pop redir (current_uiout, &stb);
+  print_breakpoint_location (this->owner, this);
+  return stb.string ();
+}
+
 /* Decrement reference count.  If the reference count reaches 0,
    destroy the bp_location.  Sets *BLP to NULL.  */
 
@@ -11157,6 +11254,9 @@ 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 ("insert_mode = %s",
+			   ugll_insert_mode_text (insert_mode));
+
   /* Used in the duplicates detection below.  When iterating over all
      bp_locations, points to the first bp_location of a given address.
      Breakpoints and watchpoints of different types are never
@@ -14899,6 +14999,15 @@ when execution stops."),
 				&breakpoint_set_cmdlist,
 				&breakpoint_show_cmdlist);
 
+  add_setshow_boolean_cmd ("breakpoint", class_maintenance,
+			   &debug_breakpoint, _("\
+Set breakpoint location debugging."), _("\
+Show breakpoint location debugging."), _("\
+When on, breakpoint location specific debugging is enabled."),
+			   NULL,
+			   show_debug_breakpoint,
+			   &setdebuglist, &showdebuglist);
+
   add_setshow_enum_cmd ("condition-evaluation", class_breakpoint,
 			condition_evaluation_enums,
 			&condition_evaluation_mode_1, _("\
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index da150585f73..f4896293bb7 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -509,6 +509,10 @@ class bp_location : public refcounted_object, public intrusive_list_node<bp_loca
 
   /* The objfile the symbol or minimal symbol were found in.  */
   const struct objfile *objfile = NULL;
+
+  /* Return a string representation of the bp_location.
+     This is only meant to be used in debug messages.  */
+  std::string to_string () const;
 };
 
 /* A policy class for bp_location reference counting.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 40d1f244417..ce0444c3c02 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -28356,6 +28356,14 @@ debugging info.
 Turn on or off debugging messages for built-in XML parsers.
 @item show debug xml
 Displays the current state of XML debugging messages.
+
+@item set debug breakpoints
+@cindex breakpoint debugging info
+Turns on or off display of @value{GDBN} debugging info for breakpoint insertion
+and removal.  The default is off.
+@item show debug breakpoints
+Displays the current state of displaying @value{GDBN} debugging info for
+breakpoint insertion and removal.
 @end table
 
 @node Other Misc Settings
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PING][PATCH v5 1/1] gdb, breakpoint: add breakpoint location debugging logs

[Schimpe, Christina]

Kindly pinging for this patch.

Thanks,
Christina

> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+christina.schimpe=intel.com@sourceware.org> On Behalf Of Christina
> Schimpe via Gdb-patches
> Sent: Monday, June 12, 2023 10:15 AM
> To: gdb-patches@sourceware.org
> Cc: aburgess@redhat.com; simark@simark.ca; eliz@gnu.org;
> blarsen@redhat.com
> Subject: [PATCH v5 1/1] gdb, breakpoint: add breakpoint location debugging logs
> 
> From: Mihails Strasuns <mihails.strasuns@intel.com>
> 
> Add new commands:
> 
>   set debug breakpoint on|off
>   show debug breakpoint
> 
> This patch introduces new debugging information that prints breakpoint location
> insertion and removal flow.
> 
> The debug output looks like:
> ~~~
> (gdb) set debug breakpoint 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.
> [breakpoint] update_global_location_list: insert_mode = UGLL_MAY_INSERT
> (gdb) c
> Continuing.
> [breakpoint] update_global_location_list: insert_mode = UGLL_INSERT
> [breakpoint] insert_bp_location: Breakpoint 2 (0x5565daddb1e0) at address
> 0x555555555137 in main at main.c:4 [breakpoint] insert_bp_location: Breakpoint
> -2 (0x5565dab51c10) at address 0x7ffff7fd37b5 [breakpoint] insert_bp_location:
> Breakpoint -5 (0x5565dab68f30) at address 0x7ffff7fe509e [breakpoint]
> insert_bp_location: Breakpoint -7 (0x5565dab694f0) at address 0x7ffff7fe63f4
> [breakpoint] remove_breakpoint_1: Breakpoint 2 (0x5565daddb1e0) at address
> 0x555555555137 in main at main.c:4 due to regular remove [breakpoint]
> remove_breakpoint_1: Breakpoint -2 (0x5565dab51c10) at address
> 0x7ffff7fd37b5 due to regular remove [breakpoint] remove_breakpoint_1:
> Breakpoint -5 (0x5565dab68f30) at address 0x7ffff7fe509e due to regular remove
> [breakpoint] remove_breakpoint_1: Breakpoint -7 (0x5565dab694f0) at address
> 0x7ffff7fe63f4 due to regular remove
> 
> Breakpoint 2, 0x0000555555555137 in main () at main.c:4
> 4	}
> ~~~
> 
> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
> ---
>  gdb/NEWS            |   4 ++
>  gdb/breakpoint.c    | 109 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/breakpoint.h    |   4 ++
>  gdb/doc/gdb.texinfo |   8 ++++
>  4 files changed, 125 insertions(+)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 649a3a9824a..2aafe103dab 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -80,6 +80,10 @@
> 
>  * New commands
> 
> +set debug breakpoint on|off
> +  show debug breakpoint
> +  Print additional debug messages about breakpoint insertion and removal.
> +
>  maintenance print record-instruction [ N ]
>    Print the recorded information for a given instruction.  If N is not given
>    prints how GDB would undo the last instruction executed.  If N is negative, diff --
> git a/gdb/breakpoint.c b/gdb/breakpoint.c index da6c8de9d14..dc502df9d79
> 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -83,6 +83,7 @@
>  #include "progspace-and-thread.h"
>  #include "gdbsupport/array-view.h"
>  #include "gdbsupport/gdb_optional.h"
> +#include "gdbsupport/common-utils.h"
> 
>  /* Prototypes for local functions.  */
> 
> @@ -200,6 +201,68 @@ enum ugll_insert_mode
>    UGLL_INSERT
>  };
> 
> +/* Return a textual version of INSERT_MODE.  */
> +
> +static const char *
> +ugll_insert_mode_text (ugll_insert_mode insert_mode) {
> +/* Make sure the compiler warns if a new ugll_insert_mode enumerator is
> added
> +   but not handled here.  */
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_ERROR_SWITCH
> +  switch (insert_mode)
> +    {
> +    case UGLL_DONT_INSERT:
> +      return "UGLL_DONT_INSERT";
> +    case UGLL_MAY_INSERT:
> +      return "UGLL_MAY_INSERT";
> +    case UGLL_INSERT:
> +      return "UGLL_INSERT";
> +    }
> +DIAGNOSTIC_POP
> +
> +  gdb_assert_not_reached ("must handle all enum values"); }
> +
> +/* Return a textual version of REASON.  */
> +
> +static const char *
> +remove_bp_reason_str (remove_bp_reason reason) {
> +/* Make sure the compiler warns if a new remove_bp_reason enumerator is
> added
> +   but not handled here.  */
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_ERROR_SWITCH
> +  switch (reason)
> +    {
> +    case REMOVE_BREAKPOINT:
> +      return "regular remove";
> +    case DETACH_BREAKPOINT:
> +      return "detach";
> +    }
> +DIAGNOSTIC_POP
> +
> +  gdb_assert_not_reached ("must handle all enum values"); }
> +
> +/* Return a textual version of breakpoint BL describing number, location and
> +   address.  */
> +
> +static std::string
> +breakpoint_location_address_str (const bp_location* bl) {
> +  std::string str = string_printf ("Breakpoint %d (%s) at address %s",
> +				   bl->owner->number,
> +				   host_address_to_string (bl),
> +				   paddress (bl->gdbarch, bl->address));
> +
> +  std::string loc_string = bl->to_string ();  if (!loc_string.empty ())
> +    str += string_printf (" %s", loc_string.c_str ());
> +
> +  return str;
> +}
> +
>  static void update_global_location_list (enum ugll_insert_mode);
> 
>  static void update_global_location_list_nothrow (enum ugll_insert_mode); @@ -
> 510,6 +573,22 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
>  	      value);
>  }
> 
> +/* True if breakpoint debug output is enabled.  */ static bool
> +debug_breakpoint = false;
> +
> +/* Print a "breakpoint" debug statement.  */ #define
> +breakpoint_debug_printf(fmt, ...) \
> +  debug_prefixed_printf_cond (debug_breakpoint, "breakpoint", fmt, \
> +			      ##__VA_ARGS__)
> +
> +/* "show debug breakpoint" implementation.  */ static void
> +show_debug_breakpoint (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); }
> +
>  /* See breakpoint.h.  */
> 
>  int
> @@ -2728,6 +2807,8 @@ insert_bp_location (struct bp_location *bl,
>    if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
>      return 0;
> 
> +  breakpoint_debug_printf ("%s", breakpoint_location_address_str
> + (bl).c_str ());
> +
>    /* Note we don't initialize bl->target_info, as that wipes out
>       the breakpoint location's shadow_contents if the breakpoint
>       is still inserted at that location.  This in turn breaks @@ -3270,6 +3351,8 @@
> remove_breakpoints_inf (inferior *inf)  {
>    int val;
> 
> +  breakpoint_debug_printf ("inf->num = %d", inf->num);
> +
>    for (bp_location *bl : all_bp_locations ())
>      {
>        if (bl->pspace != inf->pspace)
> @@ -3914,6 +3997,10 @@ detach_breakpoints (ptid_t ptid)  static int
>  remove_breakpoint_1 (struct bp_location *bl, enum remove_bp_reason reason)
> {
> +  breakpoint_debug_printf ("%s due to %s",
> +			   breakpoint_location_address_str (bl).c_str (),
> +			   remove_bp_reason_str (reason));
> +
>    int val;
> 
>    /* BL is never in moribund_locations by our callers.  */ @@ -7424,6 +7511,16
> @@ bp_location::bp_location (breakpoint *owner)  {  }
> 
> +/* See breakpoint.h.  */
> +
> +std::string bp_location::to_string () const {
> +  string_file stb;
> +  ui_out_redirect_pop redir (current_uiout, &stb);
> +  print_breakpoint_location (this->owner, this);
> +  return stb.string ();
> +}
> +
>  /* Decrement reference count.  If the reference count reaches 0,
>     destroy the bp_location.  Sets *BLP to NULL.  */
> 
> @@ -11157,6 +11254,9 @@ 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 ("insert_mode = %s",
> +			   ugll_insert_mode_text (insert_mode));
> +
>    /* Used in the duplicates detection below.  When iterating over all
>       bp_locations, points to the first bp_location of a given address.
>       Breakpoints and watchpoints of different types are never @@ -14899,6
> +14999,15 @@ when execution stops."),
>  				&breakpoint_set_cmdlist,
>  				&breakpoint_show_cmdlist);
> 
> +  add_setshow_boolean_cmd ("breakpoint", class_maintenance,
> +			   &debug_breakpoint, _("\
> +Set breakpoint location debugging."), _("\ Show breakpoint location
> +debugging."), _("\ When on, breakpoint location specific debugging is
> +enabled."),
> +			   NULL,
> +			   show_debug_breakpoint,
> +			   &setdebuglist, &showdebuglist);
> +
>    add_setshow_enum_cmd ("condition-evaluation", class_breakpoint,
>  			condition_evaluation_enums,
>  			&condition_evaluation_mode_1, _("\
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index
> da150585f73..f4896293bb7 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -509,6 +509,10 @@ class bp_location : public refcounted_object, public
> intrusive_list_node<bp_loca
> 
>    /* The objfile the symbol or minimal symbol were found in.  */
>    const struct objfile *objfile = NULL;
> +
> +  /* Return a string representation of the bp_location.
> +     This is only meant to be used in debug messages.  */  std::string
> + to_string () const;
>  };
> 
>  /* A policy class for bp_location reference counting.  */ diff --git
> a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 40d1f244417..ce0444c3c02
> 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -28356,6 +28356,14 @@ debugging info.
>  Turn on or off debugging messages for built-in XML parsers.
>  @item show debug xml
>  Displays the current state of XML debugging messages.
> +
> +@item set debug breakpoints
> +@cindex breakpoint debugging info
> +Turns on or off display of @value{GDBN} debugging info for breakpoint
> +insertion and removal.  The default is off.
> +@item show debug breakpoints
> +Displays the current state of displaying @value{GDBN} debugging info
> +for breakpoint insertion and removal.
>  @end table
> 
>  @node Other Misc Settings
> --
> 2.25.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors:
> Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the
> Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register:
> Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> CC: "aburgess@redhat.com" <aburgess@redhat.com>, "simark@simark.ca"
> 	<simark@simark.ca>, "eliz@gnu.org" <eliz@gnu.org>, "blarsen@redhat.com"
> 	<blarsen@redhat.com>
> Date: Mon, 3 Jul 2023 08:33:06 +0000
> 
> Kindly pinging for this patch.

AFAIR, I already approved the documentation parts in v4 of this.
Right?

RE: [PING][PATCH v5 1/1] gdb, breakpoint: add breakpoint location debugging logs

[Schimpe, Christina]

Yes, you did. I should have removed you from cc. Sorry for the noise.

Christina

> -----Original Message-----
> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Monday, July 3, 2023 1:59 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org; aburgess@redhat.com; simark@simark.ca;
> blarsen@redhat.com
> Subject: Re: [PING][PATCH v5 1/1] gdb, breakpoint: add breakpoint location
> debugging logs
> 
> > From: "Schimpe, Christina" <christina.schimpe@intel.com>
> > CC: "aburgess@redhat.com" <aburgess@redhat.com>, "simark@simark.ca"
> > 	<simark@simark.ca>, "eliz@gnu.org" <eliz@gnu.org>,
> "blarsen@redhat.com"
> > 	<blarsen@redhat.com>
> > Date: Mon, 3 Jul 2023 08:33:06 +0000
> >
> > Kindly pinging for this patch.
> 
> AFAIR, I already approved the documentation parts in v4 of this.
> Right?
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Andrew Burgess]

Christina Schimpe <christina.schimpe@intel.com> writes:

> From: Mihails Strasuns <mihails.strasuns@intel.com>
>
> Add new commands:
>
>   set debug breakpoint on|off
>   show debug breakpoint
>
> This patch introduces new debugging information that prints
> breakpoint location insertion and removal flow.
>
> The debug output looks like:
> ~~~
> (gdb) set debug breakpoint 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.
> [breakpoint] update_global_location_list: insert_mode = UGLL_MAY_INSERT
> (gdb) c
> Continuing.
> [breakpoint] update_global_location_list: insert_mode = UGLL_INSERT
> [breakpoint] insert_bp_location: Breakpoint 2 (0x5565daddb1e0) at address 0x555555555137 in main at main.c:4
> [breakpoint] insert_bp_location: Breakpoint -2 (0x5565dab51c10) at address 0x7ffff7fd37b5
> [breakpoint] insert_bp_location: Breakpoint -5 (0x5565dab68f30) at address 0x7ffff7fe509e
> [breakpoint] insert_bp_location: Breakpoint -7 (0x5565dab694f0) at address 0x7ffff7fe63f4
> [breakpoint] remove_breakpoint_1: Breakpoint 2 (0x5565daddb1e0) at address 0x555555555137 in main at main.c:4 due to regular remove
> [breakpoint] remove_breakpoint_1: Breakpoint -2 (0x5565dab51c10) at address 0x7ffff7fd37b5 due to regular remove
> [breakpoint] remove_breakpoint_1: Breakpoint -5 (0x5565dab68f30) at address 0x7ffff7fe509e due to regular remove
> [breakpoint] remove_breakpoint_1: Breakpoint -7 (0x5565dab694f0) at address 0x7ffff7fe63f4 due to regular remove
>
> Breakpoint 2, 0x0000555555555137 in main () at main.c:4
> 4	}
> ~~~
>
> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>

I spotted one minor nit, see below...

> ---
>  gdb/NEWS            |   4 ++
>  gdb/breakpoint.c    | 109 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/breakpoint.h    |   4 ++
>  gdb/doc/gdb.texinfo |   8 ++++
>  4 files changed, 125 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 649a3a9824a..2aafe103dab 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -80,6 +80,10 @@
>  
>  * New commands
>  
> +set debug breakpoint on|off
> +  show debug breakpoint
> +  Print additional debug messages about breakpoint insertion and removal.
> +
>  maintenance print record-instruction [ N ]
>    Print the recorded information for a given instruction.  If N is not given
>    prints how GDB would undo the last instruction executed.  If N is negative,
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index da6c8de9d14..dc502df9d79 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -83,6 +83,7 @@
>  #include "progspace-and-thread.h"
>  #include "gdbsupport/array-view.h"
>  #include "gdbsupport/gdb_optional.h"
> +#include "gdbsupport/common-utils.h"
>  
>  /* Prototypes for local functions.  */
>  
> @@ -200,6 +201,68 @@ enum ugll_insert_mode
>    UGLL_INSERT
>  };
>  
> +/* Return a textual version of INSERT_MODE.  */
> +
> +static const char *
> +ugll_insert_mode_text (ugll_insert_mode insert_mode)
> +{
> +/* Make sure the compiler warns if a new ugll_insert_mode enumerator is added
> +   but not handled here.  */
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_ERROR_SWITCH
> +  switch (insert_mode)
> +    {
> +    case UGLL_DONT_INSERT:
> +      return "UGLL_DONT_INSERT";
> +    case UGLL_MAY_INSERT:
> +      return "UGLL_MAY_INSERT";
> +    case UGLL_INSERT:
> +      return "UGLL_INSERT";
> +    }
> +DIAGNOSTIC_POP
> +
> +  gdb_assert_not_reached ("must handle all enum values");
> +}
> +
> +/* Return a textual version of REASON.  */
> +
> +static const char *
> +remove_bp_reason_str (remove_bp_reason reason)
> +{
> +/* Make sure the compiler warns if a new remove_bp_reason enumerator is added
> +   but not handled here.  */
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_ERROR_SWITCH
> +  switch (reason)
> +    {
> +    case REMOVE_BREAKPOINT:
> +      return "regular remove";
> +    case DETACH_BREAKPOINT:
> +      return "detach";
> +    }
> +DIAGNOSTIC_POP
> +
> +  gdb_assert_not_reached ("must handle all enum values");
> +}
> +
> +/* Return a textual version of breakpoint BL describing number, location and
> +   address.  */
> +
> +static std::string
> +breakpoint_location_address_str (const bp_location* bl)
> +{
> +  std::string str = string_printf ("Breakpoint %d (%s) at address %s",
> +				   bl->owner->number,
> +				   host_address_to_string (bl),
> +				   paddress (bl->gdbarch, bl->address));
> +
> +  std::string loc_string = bl->to_string ();
> +  if (!loc_string.empty ())
> +    str += string_printf (" %s", loc_string.c_str ());
> +
> +  return str;
> +}
> +
>  static void update_global_location_list (enum ugll_insert_mode);
>  
>  static void update_global_location_list_nothrow (enum ugll_insert_mode);
> @@ -510,6 +573,22 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
>  	      value);
>  }
>  
> +/* True if breakpoint debug output is enabled.  */
> +static bool debug_breakpoint = false;
> +
> +/* Print a "breakpoint" debug statement.  */
> +#define breakpoint_debug_printf(fmt, ...) \
> +  debug_prefixed_printf_cond (debug_breakpoint, "breakpoint", fmt, \
> +			      ##__VA_ARGS__)
> +
> +/* "show debug breakpoint" implementation.  */
> +static void
> +show_debug_breakpoint (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);
> +}
> +
>  /* See breakpoint.h.  */
>  
>  int
> @@ -2728,6 +2807,8 @@ insert_bp_location (struct bp_location *bl,
>    if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
>      return 0;
>  
> +  breakpoint_debug_printf ("%s", breakpoint_location_address_str (bl).c_str ());
> +
>    /* Note we don't initialize bl->target_info, as that wipes out
>       the breakpoint location's shadow_contents if the breakpoint
>       is still inserted at that location.  This in turn breaks
> @@ -3270,6 +3351,8 @@ remove_breakpoints_inf (inferior *inf)
>  {
>    int val;
>  
> +  breakpoint_debug_printf ("inf->num = %d", inf->num);
> +
>    for (bp_location *bl : all_bp_locations ())
>      {
>        if (bl->pspace != inf->pspace)
> @@ -3914,6 +3997,10 @@ detach_breakpoints (ptid_t ptid)
>  static int
>  remove_breakpoint_1 (struct bp_location *bl, enum remove_bp_reason reason)
>  {
> +  breakpoint_debug_printf ("%s due to %s",
> +			   breakpoint_location_address_str (bl).c_str (),
> +			   remove_bp_reason_str (reason));
> +
>    int val;
>  
>    /* BL is never in moribund_locations by our callers.  */
> @@ -7424,6 +7511,16 @@ bp_location::bp_location (breakpoint *owner)
>  {
>  }
>  
> +/* See breakpoint.h.  */
> +
> +std::string bp_location::to_string () const

There should be a newline after std::string.

With that fixed, this is OK to merge.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> +{
> +  string_file stb;
> +  ui_out_redirect_pop redir (current_uiout, &stb);
> +  print_breakpoint_location (this->owner, this);
> +  return stb.string ();
> +}
> +
>  /* Decrement reference count.  If the reference count reaches 0,
>     destroy the bp_location.  Sets *BLP to NULL.  */
>  
> @@ -11157,6 +11254,9 @@ 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 ("insert_mode = %s",
> +			   ugll_insert_mode_text (insert_mode));
> +
>    /* Used in the duplicates detection below.  When iterating over all
>       bp_locations, points to the first bp_location of a given address.
>       Breakpoints and watchpoints of different types are never
> @@ -14899,6 +14999,15 @@ when execution stops."),
>  				&breakpoint_set_cmdlist,
>  				&breakpoint_show_cmdlist);
>  
> +  add_setshow_boolean_cmd ("breakpoint", class_maintenance,
> +			   &debug_breakpoint, _("\
> +Set breakpoint location debugging."), _("\
> +Show breakpoint location debugging."), _("\
> +When on, breakpoint location specific debugging is enabled."),
> +			   NULL,
> +			   show_debug_breakpoint,
> +			   &setdebuglist, &showdebuglist);
> +
>    add_setshow_enum_cmd ("condition-evaluation", class_breakpoint,
>  			condition_evaluation_enums,
>  			&condition_evaluation_mode_1, _("\
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index da150585f73..f4896293bb7 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -509,6 +509,10 @@ class bp_location : public refcounted_object, public intrusive_list_node<bp_loca
>  
>    /* The objfile the symbol or minimal symbol were found in.  */
>    const struct objfile *objfile = NULL;
> +
> +  /* Return a string representation of the bp_location.
> +     This is only meant to be used in debug messages.  */
> +  std::string to_string () const;
>  };
>  
>  /* A policy class for bp_location reference counting.  */
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 40d1f244417..ce0444c3c02 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -28356,6 +28356,14 @@ debugging info.
>  Turn on or off debugging messages for built-in XML parsers.
>  @item show debug xml
>  Displays the current state of XML debugging messages.
> +
> +@item set debug breakpoints
> +@cindex breakpoint debugging info
> +Turns on or off display of @value{GDBN} debugging info for breakpoint insertion
> +and removal.  The default is off.
> +@item show debug breakpoints
> +Displays the current state of displaying @value{GDBN} debugging info for
> +breakpoint insertion and removal.
>  @end table
>  
>  @node Other Misc Settings
> -- 
> 2.25.1
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Simon Marchi]

Some more nits, in addition to what Andrew said.

>> +/* Return a textual version of breakpoint BL describing number, location and
>> +   address.  */
>> +
>> +static std::string
>> +breakpoint_location_address_str (const bp_location* bl)

Space before the asterisk.

In the description, it would be more precise to say "of breakpoint
location BL" instead of "of breakpoint BL".  However, you can be more
concise and say just "of BL", it's clear from its type that BL is a
breakpoint location.

The patch LGTM with that (plus what Andrew said) fixed, no need to re-post.

Simon

RE: [PATCH v5 1/1] gdb, breakpoint: add breakpoint location debugging logs

[Schimpe, Christina]

Hi Andrew and Simon, 

Thanks a lot for the review. 
I pushed the patch.

Best Regards,
Christina

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Wednesday, July 5, 2023 6:18 PM
> To: Andrew Burgess <aburgess@redhat.com>; Schimpe, Christina
> <christina.schimpe@intel.com>; gdb-patches@sourceware.org
> Cc: eliz@gnu.org; blarsen@redhat.com
> Subject: Re: [PATCH v5 1/1] gdb, breakpoint: add breakpoint location debugging
> logs
> 
> Some more nits, in addition to what Andrew said.
> 
> >> +/* Return a textual version of breakpoint BL describing number, location and
> >> +   address.  */
> >> +
> >> +static std::string
> >> +breakpoint_location_address_str (const bp_location* bl)
> 
> Space before the asterisk.
> 
> In the description, it would be more precise to say "of breakpoint location BL"
> instead of "of breakpoint BL".  However, you can be more concise and say just "of
> BL", it's clear from its type that BL is a breakpoint location.
> 
> The patch LGTM with that (plus what Andrew said) fixed, no need to re-post.
> 
> Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928