From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66324 invoked by alias); 18 Jul 2017 19:05:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 66296 invoked by uid 89); 18 Jul 2017 19:05:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Jul 2017 19:05:49 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7519A806A6 for ; Tue, 18 Jul 2017 19:05:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7519A806A6 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 7519A806A6 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id CDAB2627DB; Tue, 18 Jul 2017 19:05:46 +0000 (UTC) Subject: Re: [PATCH 1/2] Report call site for inlined functions To: Keith Seitz , gdb-patches@sourceware.org References: <1499740601-15957-1-git-send-email-keiths@redhat.com> From: Pedro Alves Message-ID: Date: Tue, 18 Jul 2017 19:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1499740601-15957-1-git-send-email-keiths@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-07/txt/msg00263.txt.bz2 On 07/11/2017 03:36 AM, Keith Seitz wrote: > I have chosen to record the actual symbol that we found during the parse > in the SaL. Later that information is copied into the bp_location. At some point I think we'll just make bp_location hold a SaL. :-) > From > there, print_breakpoint_location can use this information to ascertain > that the symbol is really an inlined function, and it can report the > real symbol (foo) and inform the user of the actual call site (where the > breakpoint is really set): > > (gdb) i b > Num Type Disp Enb Address What > 1 breakpoint keep y 0x0000000000400434 in foo at 1228556.c:3 > inlined in main > at 1228556.c:10 > > I have reported this information through to MI so that UI writers can inform > their users as well: > > (gdb) interpreter-exec mi -break-info > ^done,BreakpointTable={nr_rows="1",nr_cols="6",hdr=[{width="7",alignment="-1",col_name="number",colhdr="Num"},{width="14",alignment="-1",col_name="type",colhdr="Type"},{width="4",alignment="-1",col_name="disp",colhdr="Disp"},{width="3",alignment="-1",col_name="enabled",colhdr="Enb"},{width="18",alignment="-1",col_name="addr",colhdr="Address"},{width="40",alignment="2",col_name="what",colhdr="What"}],body=[bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000400434",func="foo",file="1228556.c",fullname="/home/keiths/tmp/1228556.c",line="3",call-site-func="main",call-site-file="1228556.c",call-site-fullname="/home/keiths/tmp/1228556.c",call-site-line="10",thread-groups=["i1"],times="0",original-location="foo"}]} > > Here you can see the new call-site-func, call-site-file, call-site-fullname, > and call-site-line. The non-inlined call site seems sufficient info to me, at least for the CLI. I'm not sure whether "call-site" is sufficiently clear that it's referring to that vs the immediate potential inlined-too caller though. I think at least the docs need such a clarification. > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -6153,24 +6153,73 @@ print_breakpoint_location (struct breakpoint *b, > uiout->field_string ("what", event_location_to_string (b->location.get ())); > else if (loc && loc->symtab) > { > - struct symbol *sym > - = find_pc_sect_function (loc->address, loc->section); > - if (sym) > + /* If a location is associated with an inlined symbol, print out > + information about its call site, too. */ > + if (loc->symbol != NULL && SYMBOL_INLINED (loc->symbol)) > { > + struct inlined_symbol *isym = (struct inlined_symbol *) (loc->symbol); > + struct symbol *sym > + = find_pc_sect_function (loc->address, loc->section); > + > + /* ISYM contains information about the inlined function, and > + LOC->SYMBOL describes the call site. */ > uiout->text ("in "); > - uiout->field_string ("func", SYMBOL_PRINT_NAME (sym)); > + uiout->field_string ("func", SYMBOL_PRINT_NAME (loc->symbol)); > uiout->text (" "); > uiout->wrap_hint (wrap_indent_at_field (uiout, "what")); > uiout->text ("at "); > + > + const char *s = symtab_to_filename_for_display (isym->decl_file); > + uiout->field_string ("file", s); > + > + uiout->text (":"); > + if (uiout->is_mi_like_p ()) > + { > + uiout->field_string ("fullname", > + symtab_to_fullname (isym->decl_file)); > + } Isn't there a good deal of duplication between the above and the else branch? > + uiout->field_int ("line", isym->decl_line); > + uiout->text (" "); > + uiout->wrap_hint (wrap_indent_at_field (uiout, "what")); > + uiout->text ("inlined in "); > + if (sym != NULL) > + uiout->field_string ("call-site-func", SYMBOL_PRINT_NAME (sym)); > + uiout->text (" "); > + uiout->wrap_hint (wrap_indent_at_field (uiout, "what")); > + uiout->text ("at "); > + s = symtab_to_filename_for_display (symbol_symtab (loc->symbol)); > + uiout->field_string ("call-site-file", s); > + > + uiout->text (":"); > + if (uiout->is_mi_like_p ()) > + { > + s = symtab_to_fullname (symbol_symtab (loc->symbol)); > + uiout->field_string ("call-site-fullname", s); > + } > + uiout->field_int ("call-site-line", SYMBOL_LINE (loc->symbol)); > } > - uiout->field_string ("file", > - symtab_to_filename_for_display (loc->symtab)); > - uiout->text (":"); > + else > + { > + struct symbol *sym > + = find_pc_sect_function (loc->address, loc->section); > I wonder whether the else branch could use loc->symbol now that you have it, instead of looking it up. > - if (uiout->is_mi_like_p ()) > - uiout->field_string ("fullname", symtab_to_fullname (loc->symtab)); > + if (sym) > + { > + uiout->text ("in "); > + uiout->field_string ("func", SYMBOL_PRINT_NAME (sym)); > + uiout->text (" "); > + uiout->wrap_hint (wrap_indent_at_field (uiout, "what")); > + uiout->text ("at "); > + } > + uiout->field_string ("file", > + symtab_to_filename_for_display (loc->symtab)); > + uiout->text (":"); > + > + if (uiout->is_mi_like_p ()) > + uiout->field_string ("fullname", symtab_to_fullname (loc->symtab)); > > - uiout->field_int ("line", loc->line_number); > + uiout->field_int ("line", loc->line_number); > + } > } > else if (loc) > { > @@ -8986,6 +9035,7 @@ add_location_to_breakpoint (struct breakpoint *b, > loc->gdbarch = loc_gdbarch; > loc->line_number = sal->line; > loc->symtab = sal->symtab; > + loc->symbol = sal->symbol; > > set_breakpoint_location_function (loc, > sal->explicit_pc || sal->explicit_line); > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h > index d955184..2f10c3b 100644 > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -484,6 +484,11 @@ public: > to find the corresponding source file name. */ > > struct symtab *symtab = NULL; > + > + /* The symbol found by the location parser, if any. This may be used to > + ascertain when an event location was set at a different location than > + the one originally selected by parsing, e.g., inlined symbols. */ > + const struct symbol *symbol = NULL; > }; > > /* The possible return values for print_bpstat, print_it_normal, > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index c167a86..58cdc1a 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -26755,6 +26755,14 @@ known. If not known, this field is not present. > The line number at which this breakpoint appears, if known. > If not known, this field is not present. > > +@item call-site-func > +@item call-site-file > +@item call-site-fullname > +@item call-site-line > +These fields describe the call site for a breakpoint set on an inlined function. > +The fields are analogous to those fields of the same name for normal breakpoint > +locations. IMO, the non-inlined call site is sufficient info, at least for the CLI. I'm not sure whether "call-site" is sufficiently clear that it's referring to that vs the immediate potential inlined-too caller though. I think at least the docs need that clarified. > + > @item at > If the source file is not known, this field may be provided. If > provided, this holds the address of the breakpoint, possibly followed > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 0fdcd42..3b3193b 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -11470,6 +11470,35 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu) > do_cleanups (cleanups); > } > > +/* Get the symtab representing the KIND of file of DIE in CU or NULL > + if no such file exists. KIND is any valid DWARF file attribute such as > + DW_AT_decl_file or DW_AT_call_file. */ > + > +static struct symtab * > +dwarf2_file_symtab (unsigned int kind, struct die_info *die, > + struct dwarf2_cu *cu) > +{ > + struct attribute *attr = dwarf2_attr (die, kind, cu); > + > + if (attr != NULL) > + { > + file_name_index file_index = (file_name_index) DW_UNSND (attr); > + struct file_entry *fe; > + > + if (cu->line_header != NULL) > + fe = cu->line_header->file_name_at (file_index); > + else > + fe = NULL; > + > + if (fe == NULL) > + complaint (&symfile_complaints, _("file index out of range")); > + else > + return fe->symtab; > + } > + > + return NULL; > +} > + > static void > read_func_scope (struct die_info *die, struct dwarf2_cu *cu) > { > @@ -11486,6 +11515,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu) > int inlined_func = (die->tag == DW_TAG_inlined_subroutine); > VEC (symbolp) *template_args = NULL; > struct template_symbol *templ_func = NULL; > + struct inlined_symbol *isym = NULL; > + struct symbol *symbol_storage = NULL; > > if (inlined_func) > { > @@ -11540,13 +11571,28 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu) > { > templ_func = allocate_template_symbol (objfile); > templ_func->base.is_cplus_template_function = 1; > + symbol_storage = (struct symbol *) templ_func; > break; > } > } > > + /* If we have an inlined symbol, we must also allocate a different > + symbol. */ How does this work when you have an (inlined) template function? Like e.g.,: template static T inline_func (T val) { return val * 2; } int not_inline_func (int val) { return inline_func (val * 2); } <1><31>: Abbrev Number: 2 (DW_TAG_subprogram) <32> DW_AT_name : (indirect string, offset: 0x929): inline_func <36> DW_AT_decl_file : 1 <37> DW_AT_decl_line : 2 <38> DW_AT_type : <0x54> <3c> DW_AT_inline : 1 (inlined) <3d> DW_AT_sibling : <0x54> > + if (inlined_func) > + { > + isym = allocate_inlined_symbol (objfile); > + isym->decl_file = dwarf2_file_symtab (DW_AT_decl_file, die, cu); > + > + attr = dwarf2_attr (die, DW_AT_decl_line, cu); > + if (attr != NULL) > + isym->decl_line = DW_UNSND (attr); > + > + symbol_storage = (struct symbol *) isym; > + } > + > newobj = push_context (0, lowpc); > newobj->name = new_symbol_full (die, read_type_die (die, cu), cu, > - (struct symbol *) templ_func); > + symbol_storage); > > /* If there is a location expression for DW_AT_frame_base, record > it. */ > @@ -18948,25 +18994,11 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu, > SYMBOL_LINE (sym) = DW_UNSND (attr); > } > > /* See symtab.h. */ > > struct objfile * > diff --git a/gdb/symtab.h b/gdb/symtab.h > index 341deca..a70ed54 100644 > --- a/gdb/symtab.h > +++ b/gdb/symtab.h > @@ -910,6 +910,21 @@ struct template_symbol > struct symbol **template_arguments; > }; > > +/* A superclass of struct symbol used to represent an inlined symbol. > + A symbol is really of this type if SYMBOL_INLINED is true. */ > + > +struct inlined_symbol > +{ > + /* The base class. */ > + struct symbol base; Couldn't this be real C++ inheritance? struct inlined_symbol : symbol { ... }; I ran out of time ... more later. Thanks, Pedro Alves