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

[Schimpe, Christina]

Hi,

Thank you for the feedback! I just have a question to one of your
comments (see below).

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Tuesday, March 14, 2023 3:23 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: eliz@gnu.org; blarsen@redhat.com
> Subject: Re: [PATCH v2 1/1] gdb, breakpoint: add breakpoint location
> debugging logs
> 
> 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.

I don't think I can do this due to the dependency to the function print_breakpoint_location.
So if I move it up above the function which first uses it (insert_bp_location) and remove
the forward declaration it won't compile or did I miss something?

Regards,
Christina
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