From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 8766F3858D39 for ; Tue, 14 Mar 2023 14:22:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8766F3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 0EFC81E110; Tue, 14 Mar 2023 10:22:31 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1678803752; bh=LfiVYnAKCQyEv7cebFQ7mq1GyhOGxxHC+0Eiy5oduPE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=fEHPAJSj5/fSDSg46xZ6xEG3HKUidEqRgNw7LuvKV1rJgQ2crNGwrQEP8GS17G5kB FYqd/tiStH5DLukDh85Psmmg6IpJlT0ekgL6+oAJAM/UUD9PaTkA6RB8ai7JQBkhpq cj0BCPkY5DQAI/+yBKbeYScm2SkLUPlzXD+iZn1g= Message-ID: <958242df-7f89-f3e2-bbcb-ef3092fba24a@simark.ca> Date: Tue, 14 Mar 2023 10:22:31 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH v2 1/1] gdb, breakpoint: add breakpoint location debugging logs To: Christina Schimpe , gdb-patches@sourceware.org Cc: eliz@gnu.org, blarsen@redhat.com References: <20230313183427.2735278-1-christina.schimpe@intel.com> <20230313183427.2735278-2-christina.schimpe@intel.com> Content-Language: fr From: Simon Marchi In-Reply-To: <20230313183427.2735278-2-christina.schimpe@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/13/23 14:34, Christina Schimpe via Gdb-patches wrote: > From: Mihails Strasuns > > 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 > [breakpoints] insert_bp_location: bp_location (0x56288179ea60) at address 0x7ffff7fe509e > [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 > [breakpoints] remove_breakpoint_1: bp_location (0x56288179ea60) due to regular remove at address 0x7ffff7fe509e > [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 > 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