* [PATCH 0/2] Fix BZ 25065 (LTO related GDB segfault) @ 2019-10-14 0:19 Kevin Buettner 2019-10-14 0:19 ` [PATCH 2/2] Test case for BZ 25065 Kevin Buettner 2019-10-14 0:19 ` [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Kevin Buettner 0 siblings, 2 replies; 15+ messages in thread From: Kevin Buettner @ 2019-10-14 0:19 UTC (permalink / raw) To: gdb-patches; +Cc: Kevin Buettner This two part patch consists of a fix for an LTO related GDB segfault plus a test case. Kevin Buettner (2): Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Test case for BZ 25065 gdb/dwarf2read.c | 11 ++ gdb/testsuite/gdb.dwarf2/imported-unit.c | 56 ++++++++ gdb/testsuite/gdb.dwarf2/imported-unit.exp | 157 +++++++++++++++++++++ 3 files changed, 224 insertions(+) create mode 100644 gdb/testsuite/gdb.dwarf2/imported-unit.c create mode 100644 gdb/testsuite/gdb.dwarf2/imported-unit.exp -- 2.21.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] Test case for BZ 25065 2019-10-14 0:19 [PATCH 0/2] Fix BZ 25065 (LTO related GDB segfault) Kevin Buettner @ 2019-10-14 0:19 ` Kevin Buettner 2019-12-08 10:29 ` [committed] Fix inter-CU references using intra-CU form in imported-unit Tom de Vries 2019-10-14 0:19 ` [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Kevin Buettner 1 sibling, 1 reply; 15+ messages in thread From: Kevin Buettner @ 2019-10-14 0:19 UTC (permalink / raw) To: gdb-patches; +Cc: Kevin Buettner Running a GDB with the fix for BZ 25065 should cause these new tests to all pass. When run against a GDB without the fix, there will be 2 unresolved testcases. This is what I see in the gdb.sum file when I try it using a GDB without the fix: ERROR: GDB process no longer exists UNRESOLVED: gdb.dwarf2/imported-unit.exp: ptype main::Foo ERROR: Couldn't send ptype main::foo to GDB. UNRESOLVED: gdb.dwarf2/imported-unit.exp: ptype main::foo These are "unresolved" versus outright failures due to the fact that GDB dies (segfaults) during the running of the test. gdb/testsuite/ChangeLog: * gdb.dwarf2/imported-unit.exp: New file. * gdb.dwarf2/imported-unit.c: New file. --- gdb/testsuite/gdb.dwarf2/imported-unit.c | 56 ++++++++ gdb/testsuite/gdb.dwarf2/imported-unit.exp | 157 +++++++++++++++++++++ 2 files changed, 213 insertions(+) create mode 100644 gdb/testsuite/gdb.dwarf2/imported-unit.c create mode 100644 gdb/testsuite/gdb.dwarf2/imported-unit.exp diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.c b/gdb/testsuite/gdb.dwarf2/imported-unit.c new file mode 100644 index 0000000000..3c665c507e --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/imported-unit.c @@ -0,0 +1,56 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2019 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +/* Using the DWARF assembler - see imported-unit.exp - we'll be constructing + debug info corresponding to the following C++ code that also might have + been compiled using -flto... + + int main() + { + class Foo { + public: + int doit () + { + return 0; + } + }; + + Foo foo; + + return foo.doit (); + } + + An attempt was made to try to use the above code directly, but + finding the start and end address of doit turned out to be + difficult. +*/ + + +int doit (void) +{ + asm ("doit_label: .globl doit_label"); + + return 0; +} + +int +main (int argc, char *argv[]) +{ + asm ("main_label: .globl main_label"); + + return doit (); +} diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp new file mode 100644 index 0000000000..25d44874c1 --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp @@ -0,0 +1,157 @@ +# Copyright 2019 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Import a CU into an "artificial" CU. For each DW_TAG DIE in the +# artificial CU, use DW_AT_abstract_origin to refer to a DIE in the +# imported CU. This DWARF file organization is frequently found in +# programs compiled with -flto (and -g) using GCC. +# +# This test reproduces the bug described in BZ 25065 without relying +# on specific compiler versions or use of optimization switches, in +# this case -flto. + +if [skip_cplus_tests] { + continue +} + +load_lib dwarf.exp + +# This test can only be run on targets which support DWARF-2 and use gas. +if {![dwarf2_support]} { + return 0 +}; + +standard_testfile .c .S + +# ${testfile} is now "implref-struct". srcfile2 is "implref-struct.S". +set executable ${testfile} +set asm_file [standard_output_file ${srcfile2}] + +# We need to know the size of integer and address types in order +# to write some of the debugging info we'd like to generate. +if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} {debug c++}] { + return -1 +} + +# Create the DWARF. +Dwarf::assemble $asm_file { + declare_labels cu_label main_label doit_label int_label + declare_labels Foo_label Foo_pointer_type doit_self_label + declare_labels foo_label Foo_destructor_obj_pointer_label + declare_labels Foo_constructor_obj_pointer_label + set int_size [get_sizeof "int" 4] + set addr_size [get_sizeof "void *" 8] + + global srcdir subdir srcfile + + extern main + extern doit + + set main_range [function_range main ${srcdir}/${subdir}/${srcfile}] + set main_start [lindex $main_range 0] + set main_length [lindex $main_range 1] + + set doit_range [function_range doit ${srcdir}/${subdir}/${srcfile}] + set doit_start [lindex $doit_range 0] + set doit_length [lindex $doit_range 1] + + cu {} { + compile_unit { + {language @DW_LANG_C_plus_plus} + {name "<artificial>"} + } { + imported_unit { + {import :$cu_label ref_addr} + } + subprogram { + {abstract_origin :$main_label} + {low_pc $main_start addr} + {high_pc "$main_start + $main_length" addr} + } { + subprogram { + {abstract_origin :$doit_label} + {low_pc $doit_start addr} + {high_pc "$doit_start + $doit_length" addr} + } { + formal_parameter { + {abstract_origin :$doit_self_label} + } + } + DW_TAG_variable { + {abstract_origin :$foo_label} + {location 4 data1} + } + } + } + } + + cu {} { + cu_label: compile_unit { + {language @DW_LANG_C_plus_plus} + {name "imported_unit.c"} + } { + int_label: base_type { + {byte_size $int_size sdata} + {encoding @DW_ATE_signed} + {name int} + } + + main_label: subprogram { + {name main} + {type :$int_label} + {external 1 flag} + } { + Foo_label: class_type { + {name Foo} + {byte_size 1 sdata} + } { + doit_label: subprogram { + {name doit} + {type :$int_label} + {accessibility 1 DW_FORM_data1} + } { + doit_self_label: formal_parameter { + {name this} + {artificial 1 DW_FORM_flag_present} + {type :$Foo_pointer_type} + } + } + Foo_pointer_type: pointer_type { + {byte_size $addr_size sdata} + {type :$Foo_label} + } + } + foo_label: DW_TAG_variable { + {name foo} + {type :$Foo_label} + } + } + } + } +} + +if { [prepare_for_testing "failed to prepare" ${testfile} \ + [list $srcfile $asm_file] {nodebug}] } { + return -1 +} + +gdb_test_no_output "set language c++" + +# Sanity check +gdb_test "ptype main" "= int \\(void\\)" + +# Each of these tests caused a segfault prior to fixing BZ 25065. +gdb_test "ptype main::Foo" "= class Foo \{\\s+public:\\s+int doit\\(void\\);\\s+\}" +gdb_test "ptype main::foo" "= class Foo \{\\s+public:\\s+int doit\\(void\\);\\s+\}" -- 2.21.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [committed] Fix inter-CU references using intra-CU form in imported-unit 2019-10-14 0:19 ` [PATCH 2/2] Test case for BZ 25065 Kevin Buettner @ 2019-12-08 10:29 ` Tom de Vries 0 siblings, 0 replies; 15+ messages in thread From: Tom de Vries @ 2019-12-08 10:29 UTC (permalink / raw) To: Kevin Buettner, gdb-patches [-- Attachment #1: Type: text/plain, Size: 1845 bytes --] [ was: Re: [PATCH 2/2] Test case for BZ 25065 ] On 14-10-2019 02:18, Kevin Buettner wrote: > + cu {} { > + compile_unit { > + {language @DW_LANG_C_plus_plus} > + {name "<artificial>"} > + } { > + imported_unit { > + {import :$cu_label ref_addr} > + } > + subprogram { > + {abstract_origin :$main_label} > + {low_pc $main_start addr} > + {high_pc "$main_start + $main_length" addr} > + } { > + subprogram { > + {abstract_origin :$doit_label} > + {low_pc $doit_start addr} > + {high_pc "$doit_start + $doit_length" addr} > + } { > + formal_parameter { > + {abstract_origin :$doit_self_label} > + } > + } > + DW_TAG_variable { > + {abstract_origin :$foo_label} > + {location 4 data1} > + } > + } > + } > + } > + > + cu {} { > + cu_label: compile_unit { > + {language @DW_LANG_C_plus_plus} > + {name "imported_unit.c"} > + } { > + int_label: base_type { > + {byte_size $int_size sdata} > + {encoding @DW_ATE_signed} > + {name int} > + } > + > + main_label: subprogram { > + {name main} > + {type :$int_label} > + {external 1 flag} > + } { > + Foo_label: class_type { > + {name Foo} > + {byte_size 1 sdata} > + } { > + doit_label: subprogram { > + {name doit} > + {type :$int_label} > + {accessibility 1 DW_FORM_data1} > + } { > + doit_self_label: formal_parameter { > + {name this} > + {artificial 1 DW_FORM_flag_present} > + {type :$Foo_pointer_type} > + } > + } > + Foo_pointer_type: pointer_type { > + {byte_size $addr_size sdata} > + {type :$Foo_label} > + } > + } > + foo_label: DW_TAG_variable { > + {name foo} > + {type :$Foo_label} > + } > + } > + } > + } > +} Hi, I've committed the test-case fix attached below. Thanks, - Tom [-- Attachment #2: 0001-Fix-inter-CU-references-using-intra-CU-form-in-imported-unit.patch --] [-- Type: text/x-patch, Size: 2703 bytes --] Fix inter-CU references using intra-CU form in imported-unit When running the gdb testsuite with the cc-with-dwz board, I run into: ... Running gdb/testsuite/gdb.dwarf2/imported-unit.exp ... gdb compile failed, dwz: gdb.dwarf2/imported-unit/imported-unit: \ Couldn't find DIE referenced by DW_AT_abstract_origin cc-with-tweaks.sh: dwz did not modify gdb.dwarf2/imported-unit/imported-unit. ... The problem is that the DW_AT_abstract_origin reference here: ... <0><d2>: Abbrev Number: 2 (DW_TAG_compile_unit) <1><e6>: Abbrev Number: 4 (DW_TAG_subprogram) <e7> DW_AT_abstract_origin: <0x142> <eb> DW_AT_low_pc : 0x4004b2 <f3> DW_AT_high_pc : 0x4004c8 ... referring to a DIE in another compilation unit here: ... <0><129>: Abbrev Number: 2 (DW_TAG_compile_unit) <1><142>: Abbrev Number: 4 (DW_TAG_subprogram) <143> DW_AT_name : main <148> DW_AT_type : <0x13b> <14c> DW_AT_external : 1 ... is encoded using intra-CU reference form DW_FORM_ref4 instead of intra-CU reference DW_FORM_ref_addr: ... 4 DW_TAG_subprogram [has children] DW_AT_abstract_origin DW_FORM_ref4 DW_AT_low_pc DW_FORM_addr DW_AT_high_pc DW_FORM_addr DW_AT value: 0 DW_FORM value: 0 ... Fix this in the DWARF assembler by making all inter-CU references use the '%' label prefix. Tested on x86_64-linux. gdb/testsuite/ChangeLog: 2019-12-08 Tom de Vries <tdevries@suse.de> * gdb.dwarf2/imported-unit.exp: Fix inter-CU references. Change-Id: I690ff18c3943705ed478453531b176ff74700f3c --- gdb/testsuite/gdb.dwarf2/imported-unit.exp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp index 25d44874c12..bf535120349 100644 --- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp +++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp @@ -73,24 +73,24 @@ Dwarf::assemble $asm_file { {name "<artificial>"} } { imported_unit { - {import :$cu_label ref_addr} + {import %$cu_label} } subprogram { - {abstract_origin :$main_label} + {abstract_origin %$main_label} {low_pc $main_start addr} {high_pc "$main_start + $main_length" addr} } { subprogram { - {abstract_origin :$doit_label} + {abstract_origin %$doit_label} {low_pc $doit_start addr} {high_pc "$doit_start + $doit_length" addr} } { formal_parameter { - {abstract_origin :$doit_self_label} + {abstract_origin %$doit_self_label} } } DW_TAG_variable { - {abstract_origin :$foo_label} + {abstract_origin %$foo_label} {location 4 data1} } } ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-14 0:19 [PATCH 0/2] Fix BZ 25065 (LTO related GDB segfault) Kevin Buettner 2019-10-14 0:19 ` [PATCH 2/2] Test case for BZ 25065 Kevin Buettner @ 2019-10-14 0:19 ` Kevin Buettner 2019-10-14 3:02 ` Simon Marchi 1 sibling, 1 reply; 15+ messages in thread From: Kevin Buettner @ 2019-10-14 0:19 UTC (permalink / raw) To: gdb-patches; +Cc: Kevin Buettner This is a fix for BZ 25065. GDB segfaults when running either gdb.cp/subtypes.exp or gdb.cp/local.exp in conjunction with using the -flto compiler/linker flag. A much simpler program, which was used to help create the test for this fix, is: -- doit.cc -- int main() { class Foo { public: int doit () { return 0; } }; Foo foo; return foo.doit (); } -- end doit.cc -- gcc -o doit -flto -g doit.cc gdb -q doit Reading symbols from doit... (gdb) ptype main::Foo type = class Foo { Segmentation fault (core dumped) The segfault occurs due to a NULL physname in c_type_print_base_struct_union in c-typeprint.c. Specifically, calling is_constructor_name() eventually causes the SIGSEGV is this code in c-typeprint.c: const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j); int is_full_physname_constructor = TYPE_FN_FIELD_CONSTRUCTOR (f, j) || is_constructor_name (physname) || is_destructor_name (physname) || method_name[0] == '~'; However, looking at compute_delayed_physnames(), we see that the TYPE_FN_FIELD_PHYSNAME field should never be NULL. This field will be set to "" for NULL physnames: physname = dwarf2_physname (mi.name, mi.die, cu); TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index) = physname ? physname : ""; For this particular case, it turns out that compute_delayed_physnames wasn't being called, which left TYPE_FN_FIELD_PHYSNAME set to the NULL value that it started with when that data structure was allocated. The place to fix it, I think, is towards the end of inherit_abstract_dies(). My fix causes the origin CU's method_list (which is simply the list of methods whose physnames still need to be computed) to be added to the CU which is doing the inheriting. Another way to fix it might be call compute_delayed_physnames() from inherit_abstract_dies(), but I wasn't confident that all needed types would be known at that point. It seemed safer to defer the physname computation until the inheriting CU is completely processed. gdb/ChangeLog: * dwarf2read.c (inherit_abstract_dies): Ensure that delayed physnames are computed for inherited DIEs. --- gdb/dwarf2read.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index ee9df34ed2..f7ef122933 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -13666,6 +13666,17 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu) origin_child_die = sibling_die (origin_child_die); } origin_cu->list_in_scope = origin_previous_list_in_scope; + + if (cu != origin_cu && !origin_cu->method_list.empty ()) + { + /* Add list of methods found in origin_cu to the list in cu. These + methods still need to have their physnames computed in + compute_delayed_physnames(). */ + cu->method_list.insert (cu->method_list.end (), + origin_cu->method_list.begin (), + origin_cu->method_list.end ()); + origin_cu->method_list.clear (); + } } static void -- 2.21.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-14 0:19 ` [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Kevin Buettner @ 2019-10-14 3:02 ` Simon Marchi 2019-10-15 16:27 ` Kevin Buettner 0 siblings, 1 reply; 15+ messages in thread From: Simon Marchi @ 2019-10-14 3:02 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb-patches Hi Kevin, I don't really have the big picture of these advanced DWARF use cases, so I can't really weigh on this, but I have a question: > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index ee9df34ed2..f7ef122933 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -13666,6 +13666,17 @@ inherit_abstract_dies (struct die_info *die, > struct dwarf2_cu *cu) > origin_child_die = sibling_die (origin_child_die); > } > origin_cu->list_in_scope = origin_previous_list_in_scope; > + > + if (cu != origin_cu && !origin_cu->method_list.empty ()) > + { > + /* Add list of methods found in origin_cu to the list in cu. > These > + methods still need to have their physnames computed in > + compute_delayed_physnames(). */ > + cu->method_list.insert (cu->method_list.end (), > + origin_cu->method_list.begin (), > + origin_cu->method_list.end ()); > + origin_cu->method_list.clear (); > + } So, this effectively makes the inheriting CU steal the method_list content from the inherited from CU? Is it possible for a CU to be inherited from multiple times? If so, what happens the second time we process something that inherits from this CU, the method_list vector will then be empty? Is it that we want? Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-14 3:02 ` Simon Marchi @ 2019-10-15 16:27 ` Kevin Buettner 2019-10-17 3:54 ` Simon Marchi 0 siblings, 1 reply; 15+ messages in thread From: Kevin Buettner @ 2019-10-15 16:27 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi On Sun, 13 Oct 2019 23:02:01 -0400 Simon Marchi <simon.marchi@polymtl.ca> wrote: > Hi Kevin, > > I don't really have the big picture of these advanced DWARF use cases, > so I can't really weigh on this, but I have a question: > > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > > index ee9df34ed2..f7ef122933 100644 > > --- a/gdb/dwarf2read.c > > +++ b/gdb/dwarf2read.c > > @@ -13666,6 +13666,17 @@ inherit_abstract_dies (struct die_info *die, > > struct dwarf2_cu *cu) > > origin_child_die = sibling_die (origin_child_die); > > } > > origin_cu->list_in_scope = origin_previous_list_in_scope; > > + > > + if (cu != origin_cu && !origin_cu->method_list.empty ()) > > + { > > + /* Add list of methods found in origin_cu to the list in cu. > > These > > + methods still need to have their physnames computed in > > + compute_delayed_physnames(). */ > > + cu->method_list.insert (cu->method_list.end (), > > + origin_cu->method_list.begin (), > > + origin_cu->method_list.end ()); > > + origin_cu->method_list.clear (); > > + } > > So, this effectively makes the inheriting CU steal the method_list > content from the inherited from CU? Is it possible for a CU to be > inherited from multiple times? If so, what happens the second time we > process something that inherits from this CU, the method_list vector > will then be empty? Is it that we want? Hi Simon, You raise some good questions. I modified the test associated with this bug so that two different CUs attempt to inherit a third CU. This has turned up another bug in GDB which I spent the rest of the day looking at. (GDB goes into an infinite loop!) I'll make the following observations, however... - method_list is used solely for keeping track of C++ methods for delayed physname computations. - method_list is cleared in process_full_comp_unit(), process_full_type_unit(), and compute_delayed_physnames(). That latter function(), compute_delayed_physnames(), is called after DIE processing in the first two functions. So method_list is made to be empty prior to DIE processing and then made empty (as a result of delayed physname computation) again after DIE processing (in the above mentioned functions). So, what is the right thing to do with regard to method_list for inherit_abstract_dies()? Yesterday, as part of my investigations, I made inherit_abstract_dies() call compute_delayed_physnames in place of the code in my posted patch. That also works, at least for my test case. I'll note that for my original (posted) patch, compute_delayed_physnames will be called with the inheriting CU. In the alternate approach (in which compute_delayed_physnames is called from inherit_abstract_dies), compute_delayed_physnames is called with the origin CU. I don't yet know which is more correct or if there are cases where it'll make a difference. So... I'm continuing my investigations and will let you know when I have some answers. In the interim, if anyone has some insights about these matters, I'd very much like to hear them. Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-15 16:27 ` Kevin Buettner @ 2019-10-17 3:54 ` Simon Marchi 2019-10-17 5:30 ` Simon Marchi 0 siblings, 1 reply; 15+ messages in thread From: Simon Marchi @ 2019-10-17 3:54 UTC (permalink / raw) To: Kevin Buettner, gdb-patches On 2019-10-15 12:27 p.m., Kevin Buettner wrote: > Hi Simon, > > You raise some good questions. I modified the test associated with > this bug so that two different CUs attempt to inherit a third CU. > This has turned up another bug in GDB which I spent the rest of the > day looking at. (GDB goes into an infinite loop!) > > I'll make the following observations, however... > > - method_list is used solely for keeping track of C++ methods for > delayed physname computations. > > - method_list is cleared in process_full_comp_unit(), > process_full_type_unit(), and compute_delayed_physnames(). > That latter function(), compute_delayed_physnames(), is called > after DIE processing in the first two functions. So method_list > is made to be empty prior to DIE processing and then made empty > (as a result of delayed physname computation) again after DIE > processing (in the above mentioned functions). > > So, what is the right thing to do with regard to method_list for > inherit_abstract_dies()? > > Yesterday, as part of my investigations, I made inherit_abstract_dies() > call compute_delayed_physnames in place of the code in my posted > patch. That also works, at least for my test case. I'll note that > for my original (posted) patch, compute_delayed_physnames will be > called with the inheriting CU. In the alternate approach (in which > compute_delayed_physnames is called from inherit_abstract_dies), > compute_delayed_physnames is called with the origin CU. I don't > yet know which is more correct or if there are cases where it'll > make a difference. > > So... I'm continuing my investigations and will let you know when > I have some answers. In the interim, if anyone has some insights > about these matters, I'd very much like to hear them. > > Kevin > Hi Kevin, I've spent a bit of time to better understand what inherited abstract DIEs are and to step in that code. I observed that when a DIE A inherits from another DIE X, we process DIE X completely from scratch, going through process_die, creating the struct type instances, adding items to method_list for delayed name computation. If another DIE B inherits from DIE X, then we'll go through process_die again, creating another struct type, adding items to method_list for delayed name computation again. There is no (and there shouldn't be) any state shared between the processing of X while inherited by A and the processing of X while inherited by B. In theory, the compiler could have decided to have two completely separate DIE trees under A and under B. But to reduce duplication, it decided to use that abstract origin thing, so A and B could share some children. However, we should still conceptually treat them as separate trees. Btw, I'm writing all this so I assimilate it better, but also so you can tell me if you think I'm wrong. I am confident that what you do in this patch is right. Let's say A, B and X, the same DIEs as above, are all in different CUs. Before your patch, while processing A, the delayed method info for things described in X would be put in X's CU's method_info list and stay there, and it would never get used. When processing B, the delayed method info for things described in X would also be put in X's CU's method_info list and never get used. If, we then happen to process the CU that contains X, we'll start by clearing that CU's method_info list in process_full_comp_unit and start from scratch. With your patch, the entries generated while parsing X as inherited by A get moved to A's CU, and the entries generated while parsing X as inherited by B get transferred to B's CU. That's as if A and B had their own independent subtree, which is what we want. If we then happen to process X's CU, that CU's method_list will be empty, as expected. I think that what's confusing in all this is the fact that the method_info list is currently attached to a particular CU. Instead, I think it should be attached to the operation of processing of a CU, and used to collect all delayed method infos while processing that CU, even if some of these infos come from inherited DIEs from another CU. Concretely, it would mean to have a local instance of std::vector<delayed_method_info> in process_full_comp_unit/process_full_type_unit and to pass it by pointer/reference through the call stack to any code who might want to append to it. We wouldn't have to do anything special in inherit_abstract_dies, just pass this reference to the list down the stack. I don't know how feasible it would be in practice to do that change, maybe it's too much work or would end up ugly. I'll give it a try. But your patch gives essentially the same result, and works with what we have today. Also, note this in inherit_abstract_dies: /* We're inheriting ORIGIN's children into the scope we'd put DIE's symbols in. */ origin_previous_list_in_scope = origin_cu->list_in_scope; origin_cu->list_in_scope = cu->list_in_scope; ... origin_cu->list_in_scope = origin_previous_list_in_scope; I think this code is solving the same kind of problem for `list_in_scope`, but differently: it temporarily moves A's CU's list in X's CU. Here too, I think it would be clearer if `list_in_scope` wasn't attached to a CU, but something passed down the stack. Again, we wouldn't have to do anything special in inherit_abstract_dies, just pass that list down. One last thing, I think that those method_list.clear() calls in process_full_comp_unit and process_full_type_unit are unnecessary, and are confusing because they suggest that there might be something in there. When we start processing a CU with either of these functions, I don't think there should ever be some content. I'd try to change them to gdb_assert (cu->method_list.empty ()); Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-17 3:54 ` Simon Marchi @ 2019-10-17 5:30 ` Simon Marchi 2019-10-18 1:08 ` Kevin Buettner 0 siblings, 1 reply; 15+ messages in thread From: Simon Marchi @ 2019-10-17 5:30 UTC (permalink / raw) To: Kevin Buettner, gdb-patches On 2019-10-16 11:54 p.m., Simon Marchi wrote: > I think that what's confusing in all this is the fact that the method_info list is > currently attached to a particular CU. Instead, I think it should be attached to the > operation of processing of a CU, and used to collect all delayed method infos while > processing that CU, even if some of these infos come from inherited DIEs from another > CU. Concretely, it would mean to have a local instance of > std::vector<delayed_method_info> in process_full_comp_unit/process_full_type_unit and to > pass it by pointer/reference through the call stack to any code who might want to append > to it. We wouldn't have to do anything special in inherit_abstract_dies, just pass this > reference to the list down the stack. I don't know how feasible it would be in practice > to do that change, maybe it's too much work or would end up ugly. I'll give it a try. > But your patch gives essentially the same result, and works with what we have today. A little follow up to the above. I prototyped that change here in a WIP patch (so, not intended to be reviewed): https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/128 I got no regression in gdb.dwarf2. However, it's a bit invasive. If we want to pass other objects in the same fashion, it will quickly become very heavy. What we could do though, is to introduce a new type (e.g. struct dwarf2_cu_processing_context) and pass that around instead. Its lifetime would be the duration of process_full_comp_unit / process_full_type_unit, just like std::vector in the patch above, but could contain many fields. I found something potentially problematic though (applies to both your and my patch). When we process the delayed_method_info objects in compute_delayed_physnames, we call: dwarf2_physname (mi.name, mi.die, cu); mi.die could be a die coming from X's CU (to keep the example from my previous message), but the cu in the call above is A's CU (the CU we are processing). I am pretty sure that this function (and what it calls) expect the passed DIE to come from the passed CU. If they don't match, I guess we could have some bad surprises. Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-17 5:30 ` Simon Marchi @ 2019-10-18 1:08 ` Kevin Buettner 2019-10-18 15:07 ` Simon Marchi 0 siblings, 1 reply; 15+ messages in thread From: Kevin Buettner @ 2019-10-18 1:08 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi On Thu, 17 Oct 2019 01:30:12 -0400 Simon Marchi <simon.marchi@polymtl.ca> wrote: > On 2019-10-16 11:54 p.m., Simon Marchi wrote: > > I think that what's confusing in all this is the fact that the > > method_info list is currently attached to a particular CU. > > Instead, I think it should be attached to the operation of > > processing of a CU, and used to collect all delayed method infos > > while processing that CU, even if some of these infos come from > > inherited DIEs from another CU. Concretely, it would mean to have > > a local instance of std::vector<delayed_method_info> in > > process_full_comp_unit/process_full_type_unit and to pass it by > > pointer/reference through the call stack to any code who might > > want to append to it. We wouldn't have to do anything special in > > inherit_abstract_dies, just pass this reference to the list down > > the stack. I don't know how feasible it would be in practice to > > do that change, maybe it's too much work or would end up ugly. > > I'll give it a try. But your patch gives essentially the same > > result, and works with what we have today. > > A little follow up to the above. > > I prototyped that change here in a WIP patch (so, not intended to be > reviewed): > > https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/128 > > I got no regression in gdb.dwarf2. However, it's a bit invasive. > If we want to pass other objects in the same fashion, it will > quickly become very heavy. > > What we could do though, is to introduce a new type (e.g. struct > dwarf2_cu_processing_context) and pass that around instead. Its > lifetime would be the duration of process_full_comp_unit / > process_full_type_unit, just like std::vector in the patch above, > but could contain many fields. I looked over your patch; it looks reasonable to me. If we go that route, I like the idea of introducing a dwarf2_cu_processing_context struct. (But see below for some later misgivings that I have/had.) > I found something potentially problematic though (applies to both > your and my patch). When we process the delayed_method_info objects > in compute_delayed_physnames, we call: > > dwarf2_physname (mi.name, mi.die, cu); > > mi.die could be a die coming from X's CU (to keep the example from > my previous message), but the cu in the call above is A's CU (the CU > we are processing). I am pretty sure that this function (and what > it calls) expect the passed DIE to come from the passed CU. If they > don't match, I guess we could have some bad surprises. I spent a little time trying to figure out what CU is used for in dwarf2_physname() and its callees. What I noticed most is that cu->language is used to figure out some appropriate thing to do. I seem to remember Keith running into problems with mismatched languages in different CUs. So there might be problems if the languages associated with the CUs are different. There are also obstacks associated with each CU. I noticed this call in dwarf2_compute_name(): dwarf2_const_value_attr (attr, type, name, &cu->comp_unit_obstack, cu, &value, &bytes, &baton); For this matter, I think that we want the inheriting CU's obstack to be used, so this might be okay. However, due to potentially differing lifetimes, there could be problems if data structures allocated in one obstack point to data structures in another; I don't know if that could happen or not. There are also errors which fetch the objfile name via... objfile_name (cu->per_cu->dwarf2_per_objfile->objfile) I don't think this will cause a crash or anything, but it has the potential to misidentify the problematic objfile. There may be other concerns too; I'm certain that I didn't look at all of the ways that CU is used in dwarf2_physname and its callees. Also, while looking at all of that, it occurred to me that struct dwarf2_cu already contains elements of a CU processing context. E.g. the rust_unions member looks very similar to method_list. Plus the comment for dwarf2_cu says "Internal state when decoding a particular compilation unit." That led me to wonder if it truly makes sense to pass around both a struct dwarf2_cu and a struct dwarf2_cu_processing_context. (I don't know the answer, but it's something to mull over...) Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-18 1:08 ` Kevin Buettner @ 2019-10-18 15:07 ` Simon Marchi 2019-10-21 20:05 ` Keith Seitz 2019-10-22 22:23 ` Kevin Buettner 0 siblings, 2 replies; 15+ messages in thread From: Simon Marchi @ 2019-10-18 15:07 UTC (permalink / raw) To: Kevin Buettner, gdb-patches, Keith Seitz On 2019-10-17 9:08 p.m., Kevin Buettner wrote: > On Thu, 17 Oct 2019 01:30:12 -0400 > Simon Marchi <simon.marchi@polymtl.ca> wrote: > >> On 2019-10-16 11:54 p.m., Simon Marchi wrote: >>> I think that what's confusing in all this is the fact that the >>> method_info list is currently attached to a particular CU. >>> Instead, I think it should be attached to the operation of >>> processing of a CU, and used to collect all delayed method infos >>> while processing that CU, even if some of these infos come from >>> inherited DIEs from another CU. Concretely, it would mean to have >>> a local instance of std::vector<delayed_method_info> in >>> process_full_comp_unit/process_full_type_unit and to pass it by >>> pointer/reference through the call stack to any code who might >>> want to append to it. We wouldn't have to do anything special in >>> inherit_abstract_dies, just pass this reference to the list down >>> the stack. I don't know how feasible it would be in practice to >>> do that change, maybe it's too much work or would end up ugly. >>> I'll give it a try. But your patch gives essentially the same >>> result, and works with what we have today. >> >> A little follow up to the above. >> >> I prototyped that change here in a WIP patch (so, not intended to be >> reviewed): >> >> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/128 >> >> I got no regression in gdb.dwarf2. However, it's a bit invasive. >> If we want to pass other objects in the same fashion, it will >> quickly become very heavy. >> >> What we could do though, is to introduce a new type (e.g. struct >> dwarf2_cu_processing_context) and pass that around instead. Its >> lifetime would be the duration of process_full_comp_unit / >> process_full_type_unit, just like std::vector in the patch above, >> but could contain many fields. > > I looked over your patch; it looks reasonable to me. If we go that > route, I like the idea of introducing a dwarf2_cu_processing_context > struct. (But see below for some later misgivings that I have/had.) Ok, I can try to make something cleaner, but I don't know when it would be ready, and I wouldn't want that to block the GDB 9.1 branch creation (or release) for that. Would you like to still push your patch (or a perhaps updated version of it) so that we have the fix in GDB 9.1? >> I found something potentially problematic though (applies to both >> your and my patch). When we process the delayed_method_info objects >> in compute_delayed_physnames, we call: >> >> dwarf2_physname (mi.name, mi.die, cu); >> >> mi.die could be a die coming from X's CU (to keep the example from >> my previous message), but the cu in the call above is A's CU (the CU >> we are processing). I am pretty sure that this function (and what >> it calls) expect the passed DIE to come from the passed CU. If they >> don't match, I guess we could have some bad surprises. > > I spent a little time trying to figure out what CU is used for in > dwarf2_physname() and its callees. What I noticed most is that > cu->language is used to figure out some appropriate thing to do. > I seem to remember Keith running into problems with mismatched > languages in different CUs. So there might be problems if the > languages associated with the CUs are different. Yeah, I remember this saga, but I did not really follow it. Keith, would you have an idea of what would be the right CU to pass here? > There are also obstacks associated with each CU. I noticed this > call in dwarf2_compute_name(): > > dwarf2_const_value_attr (attr, type, name, > &cu->comp_unit_obstack, cu, > &value, &bytes, &baton); > > For this matter, I think that we want the inheriting CU's obstack to > be used, so this might be okay. However, due to potentially differing > lifetimes, there could be problems if data structures allocated in one > obstack point to data structures in another; I don't know if that could > happen or not. Hmm right. Maybe it's fine in practice since the lifetimes of the two dwarf_cu objects are probably similar. But I agree it's not ideal. > > There are also errors which fetch the objfile name via... > > objfile_name (cu->per_cu->dwarf2_per_objfile->objfile) > > I don't think this will cause a crash or anything, but it has > the potential to misidentify the problematic objfile. Aren't the two CUs necessarily in the same objfile? > There may be other concerns too; I'm certain that I didn't look at all > of the ways that CU is used in dwarf2_physname and its callees. I don't think it's humanly possible to manually check all the possible branches this code can take. I say, let's do a quick pass to check for the obvious (like what you found above), but otherwise I'm fine with this patch, it already makes things better than they are now. > Also, while looking at all of that, it occurred to me that struct > dwarf2_cu already contains elements of a CU processing context. E.g. > the rust_unions member looks very similar to method_list. Plus the > comment for dwarf2_cu says "Internal state when decoding a particular > compilation unit." That led me to wonder if it truly makes sense to > pass around both a struct dwarf2_cu and a struct > dwarf2_cu_processing_context. (I don't know the answer, but it's > something to mull over...) I'll check closer when actually trying to write that patch, but for now I believe that it's fine to pass both objects, as they have different meanings. The dwarf2_cu_processing_ctx would contain things related to the root CU we are parsing. The dwarf2_cu would represent things related to the CU we are in right now. So when crossing a CU boundary (like in the abstract origin case that started this thread), the passed dwarf2_cu_processing_ctx wouldn't change, but the passed dwarf2_cu would change. Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-18 15:07 ` Simon Marchi @ 2019-10-21 20:05 ` Keith Seitz 2019-10-22 22:23 ` Kevin Buettner 1 sibling, 0 replies; 15+ messages in thread From: Keith Seitz @ 2019-10-21 20:05 UTC (permalink / raw) To: Simon Marchi, Kevin Buettner, gdb-patches On 10/18/19 8:07 AM, Simon Marchi wrote: >> I spent a little time trying to figure out what CU is used for in >> dwarf2_physname() and its callees. What I noticed most is that >> cu->language is used to figure out some appropriate thing to do. >> I seem to remember Keith running into problems with mismatched >> languages in different CUs. So there might be problems if the >> languages associated with the CUs are different. > > Yeah, I remember this saga, but I did not really follow it. Keith, would > you have an idea of what would be the right CU to pass here? The language mismatch that Kevin refers to was a problem that appeared with LTO. The primary DIE would be marked artificial and having language C99. That DIE would then import other DIEs of different languages (mostly C++), and that would cause problems because all these symbols are supposed to live in the same dictionary. That was resolved by moving to multidictionary a while back. [Reminder, a symbol's language is used for searching. Different languages now have different searching algorithms. This was introduced to help with Pedro's multi-breakpoint/linespec work to deal with namespaces, ABI tags, etc.] As far as the correct DIEs to use when passing to dwarf2_physname, I don't think/remember it being anything particularly unexpected. If the physname for a DIE is computed, the DIE/CU passed must be able to give us this information. Since dwarf2_physname largely uses linkage names nowadays (there was only one or two instances where we actually needed to compute the physname), the correct CU/DIE would be the pair containing DW_AT_linkage_name for the symbol. It is debatable whether we even still need delayed physnames. I don't think we actually need to compute physnames much anymore. [As I recall, one place we've had to do this with was dtors. GCC did not emit linkage names for them.] Keith ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-18 15:07 ` Simon Marchi 2019-10-21 20:05 ` Keith Seitz @ 2019-10-22 22:23 ` Kevin Buettner 2019-11-04 20:42 ` Kevin Buettner 2019-11-04 20:49 ` Simon Marchi 1 sibling, 2 replies; 15+ messages in thread From: Kevin Buettner @ 2019-10-22 22:23 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches, Keith Seitz On Fri, 18 Oct 2019 11:07:31 -0400 Simon Marchi <simon.marchi@polymtl.ca> wrote: > > I looked over your patch; it looks reasonable to me. If we go that > > route, I like the idea of introducing a dwarf2_cu_processing_context > > struct. (But see below for some later misgivings that I have/had.) > > Ok, I can try to make something cleaner, but I don't know when it > would be ready, and I wouldn't want that to block the GDB 9.1 branch > creation (or release) for that. Would you like to still push your > patch (or a perhaps updated version of it) so that we have the fix > in GDB 9.1? [...] > > There may be other concerns too; I'm certain that I didn't look at all > > of the ways that CU is used in dwarf2_physname and its callees. > > I don't think it's humanly possible to manually check all the > possible branches this code can take. I say, let's do a quick pass > to check for the obvious (like what you found above), but otherwise > I'm fine with this patch, it already makes things better than they > are now. My testing shows that the patch below still fixes the problem while also avoiding the poential problems of passing a CU to compute_delayed_physnames() which is different from the CU of the methods for which want to compute physnames. I think that this patch is safer than the one I originally proposed and is, therefore, a better short term solution. What do you think? - - - Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs This is a fix for BZ 25065. GDB segfaults when running either gdb.cp/subtypes.exp or gdb.cp/local.exp in conjunction with using the -flto compiler/linker flag. A much simpler program, which was used to help create the test for this fix, is: -- doit.cc -- int main() { class Foo { public: int doit () { return 0; } }; Foo foo; return foo.doit (); } -- end doit.cc -- gcc -o doit -flto -g doit.cc gdb -q doit Reading symbols from doit... (gdb) ptype main::Foo type = class Foo { Segmentation fault (core dumped) The segfault occurs due to a NULL physname in c_type_print_base_struct_union in c-typeprint.c. Specifically, calling is_constructor_name() eventually causes the SIGSEGV is this code in c-typeprint.c: const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j); int is_full_physname_constructor = TYPE_FN_FIELD_CONSTRUCTOR (f, j) || is_constructor_name (physname) || is_destructor_name (physname) || method_name[0] == '~'; However, looking at compute_delayed_physnames(), we see that the TYPE_FN_FIELD_PHYSNAME field should never be NULL. This field will be set to "" for NULL physnames: physname = dwarf2_physname (mi.name, mi.die, cu); TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index) = physname ? physname : ""; For this particular case, it turns out that compute_delayed_physnames wasn't being called, which left TYPE_FN_FIELD_PHYSNAME set to the NULL value that it started with when that data structure was allocated. The place to fix it, I think, is towards the end of inherit_abstract_dies(). My first attempt at fix caused the origin CU's method_list (which is simply the list of methods whose physnames still need to be computed) to be added to the CU which is doing the inheriting. One drawback with this approach is that compute_delayed_physnames is (eventually) called with a CU that's different than the CU in which the methods were found. It's not clear whether this will cause problems or not. A safer approach, which is what I ultimately settled on, is to call compute_delayed_physnames() from inherit_abstract_dies(). One potential drawback is that all needed types might not be known at that point. However, in my testing, I haven't seen a problem along these lines. gdb/ChangeLog: * dwarf2read.c (inherit_abstract_dies): Ensure that delayed physnames are computed for inherited DIEs. Change-Id: I6c6ffe96b301a9daab9f653956b89e3a33fa9445 --- gdb/dwarf2read.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index ee9df34ed2..976153640a 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -13666,6 +13666,9 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu) origin_child_die = sibling_die (origin_child_die); } origin_cu->list_in_scope = origin_previous_list_in_scope; + + if (cu != origin_cu) + compute_delayed_physnames (origin_cu); } static void ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-22 22:23 ` Kevin Buettner @ 2019-11-04 20:42 ` Kevin Buettner 2019-11-04 20:49 ` Simon Marchi 1 sibling, 0 replies; 15+ messages in thread From: Kevin Buettner @ 2019-11-04 20:42 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Ping. On Tue, 22 Oct 2019 15:23:07 -0700 Kevin Buettner <kevinb@redhat.com> wrote: > On Fri, 18 Oct 2019 11:07:31 -0400 > Simon Marchi <simon.marchi@polymtl.ca> wrote: > > > > I looked over your patch; it looks reasonable to me. If we go that > > > route, I like the idea of introducing a dwarf2_cu_processing_context > > > struct. (But see below for some later misgivings that I have/had.) > > > > Ok, I can try to make something cleaner, but I don't know when it > > would be ready, and I wouldn't want that to block the GDB 9.1 branch > > creation (or release) for that. Would you like to still push your > > patch (or a perhaps updated version of it) so that we have the fix > > in GDB 9.1? > > [...] > > > > There may be other concerns too; I'm certain that I didn't look at all > > > of the ways that CU is used in dwarf2_physname and its callees. > > > > I don't think it's humanly possible to manually check all the > > possible branches this code can take. I say, let's do a quick pass > > to check for the obvious (like what you found above), but otherwise > > I'm fine with this patch, it already makes things better than they > > are now. > > My testing shows that the patch below still fixes the problem while > also avoiding the poential problems of passing a CU to > compute_delayed_physnames() which is different from the CU of the > methods for which want to compute physnames. > > I think that this patch is safer than the one I originally proposed > and is, therefore, a better short term solution. > > What do you think? > > - - - > > Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs > > This is a fix for BZ 25065. > > GDB segfaults when running either gdb.cp/subtypes.exp or > gdb.cp/local.exp in conjunction with using the -flto compiler/linker > flag. > > A much simpler program, which was used to help create the test for > this fix, is: > > -- doit.cc -- > int main() > { > class Foo { > public: > int doit () > { > return 0; > } > }; > > Foo foo; > > return foo.doit (); > } > -- end doit.cc -- > > gcc -o doit -flto -g doit.cc > gdb -q doit > Reading symbols from doit... > (gdb) ptype main::Foo > type = class Foo { > Segmentation fault (core dumped) > > The segfault occurs due to a NULL physname in > c_type_print_base_struct_union in c-typeprint.c. Specifically, > calling is_constructor_name() eventually causes the SIGSEGV is this > code in c-typeprint.c: > > const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j); > int is_full_physname_constructor = > TYPE_FN_FIELD_CONSTRUCTOR (f, j) > || is_constructor_name (physname) > || is_destructor_name (physname) > || method_name[0] == '~'; > > However, looking at compute_delayed_physnames(), we see that > the TYPE_FN_FIELD_PHYSNAME field should never be NULL. This > field will be set to "" for NULL physnames: > > physname = dwarf2_physname (mi.name, mi.die, cu); > TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index) > = physname ? physname : ""; > > For this particular case, it turns out that compute_delayed_physnames > wasn't being called, which left TYPE_FN_FIELD_PHYSNAME set to the NULL > value that it started with when that data structure was allocated. > > The place to fix it, I think, is towards the end of > inherit_abstract_dies(). > > My first attempt at fix caused the origin CU's method_list (which is > simply the list of methods whose physnames still need to be computed) > to be added to the CU which is doing the inheriting. One drawback > with this approach is that compute_delayed_physnames is (eventually) > called with a CU that's different than the CU in which the methods > were found. It's not clear whether this will cause problems or not. > > A safer approach, which is what I ultimately settled on, is to call > compute_delayed_physnames() from inherit_abstract_dies(). One > potential drawback is that all needed types might not be known at that > point. However, in my testing, I haven't seen a problem along these > lines. > > gdb/ChangeLog: > > * dwarf2read.c (inherit_abstract_dies): Ensure that delayed > physnames are computed for inherited DIEs. > > Change-Id: I6c6ffe96b301a9daab9f653956b89e3a33fa9445 > --- > gdb/dwarf2read.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index ee9df34ed2..976153640a 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -13666,6 +13666,9 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu) > origin_child_die = sibling_die (origin_child_die); > } > origin_cu->list_in_scope = origin_previous_list_in_scope; > + > + if (cu != origin_cu) > + compute_delayed_physnames (origin_cu); > } > > static void > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-10-22 22:23 ` Kevin Buettner 2019-11-04 20:42 ` Kevin Buettner @ 2019-11-04 20:49 ` Simon Marchi 2019-11-27 20:17 ` Kevin Buettner 1 sibling, 1 reply; 15+ messages in thread From: Simon Marchi @ 2019-11-04 20:49 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb-patches, Keith Seitz On 2019-10-22 6:23 p.m., Kevin Buettner wrote: > On Fri, 18 Oct 2019 11:07:31 -0400 > Simon Marchi <simon.marchi@polymtl.ca> wrote: > >>> I looked over your patch; it looks reasonable to me. If we go that >>> route, I like the idea of introducing a dwarf2_cu_processing_context >>> struct. (But see below for some later misgivings that I have/had.) >> >> Ok, I can try to make something cleaner, but I don't know when it >> would be ready, and I wouldn't want that to block the GDB 9.1 branch >> creation (or release) for that. Would you like to still push your >> patch (or a perhaps updated version of it) so that we have the fix >> in GDB 9.1? > > [...] > >>> There may be other concerns too; I'm certain that I didn't look at all >>> of the ways that CU is used in dwarf2_physname and its callees. >> >> I don't think it's humanly possible to manually check all the >> possible branches this code can take. I say, let's do a quick pass >> to check for the obvious (like what you found above), but otherwise >> I'm fine with this patch, it already makes things better than they >> are now. > > My testing shows that the patch below still fixes the problem while > also avoiding the poential problems of passing a CU to > compute_delayed_physnames() which is different from the CU of the > methods for which want to compute physnames. > > I think that this patch is safer than the one I originally proposed > and is, therefore, a better short term solution. > > What do you think? Yep, this looks good and relatively safe to me. Thanks! Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs 2019-11-04 20:49 ` Simon Marchi @ 2019-11-27 20:17 ` Kevin Buettner 0 siblings, 0 replies; 15+ messages in thread From: Kevin Buettner @ 2019-11-27 20:17 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi On Mon, 4 Nov 2019 15:49:21 -0500 Simon Marchi <simon.marchi@polymtl.ca> wrote: > > I think that this patch is safer than the one I originally proposed > > and is, therefore, a better short term solution. > > > > What do you think? > > Yep, this looks good and relatively safe to me. > > Thanks! I've (finally) pushed this patch along with the test case. Thanks again for the review. Kevin ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-12-08 10:29 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-14 0:19 [PATCH 0/2] Fix BZ 25065 (LTO related GDB segfault) Kevin Buettner 2019-10-14 0:19 ` [PATCH 2/2] Test case for BZ 25065 Kevin Buettner 2019-12-08 10:29 ` [committed] Fix inter-CU references using intra-CU form in imported-unit Tom de Vries 2019-10-14 0:19 ` [PATCH 1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs Kevin Buettner 2019-10-14 3:02 ` Simon Marchi 2019-10-15 16:27 ` Kevin Buettner 2019-10-17 3:54 ` Simon Marchi 2019-10-17 5:30 ` Simon Marchi 2019-10-18 1:08 ` Kevin Buettner 2019-10-18 15:07 ` Simon Marchi 2019-10-21 20:05 ` Keith Seitz 2019-10-22 22:23 ` Kevin Buettner 2019-11-04 20:42 ` Kevin Buettner 2019-11-04 20:49 ` Simon Marchi 2019-11-27 20:17 ` Kevin Buettner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).