Re: [PATCH] PR gdb/18021 - defend against "static virtual" methods

[Doug Evans <dje at google dot com>]

Keith Seitz writes:
 > This bug appears to be caused by bad debuginfo. The method
 > causing the sefault in the reporter's test case is marked both static
 > and virtual.
 > 
 > This patch simply safegaurds against this case in dwarf2_add_member_fn,
 > where the code assumes that there is a `this' pointer when a virtual method
 > is seen (more specifically, when DW_AT_vtable_elem is seen).
 > 
 > It previously dereferenced the first formal parameter
 > (`this' pointer), which in this case doesn't exist. GDB consequently
 > segfaulted dereferencing a NULL pointer.
 > 
 > gdb/ChangeLog
 > 	* dwarf2read.c (dwarf2_add_member_fn): Issue a complaint
 > 	if we find a static method with DW_AT_vtable_elem_location.
 > 
 > gdb/testsuite/ChangeLog
 > 	* gdb.dwarf2/staticvirtual.exp: New test.

Hi.
A couple of nits.

1) Include PR number in changelog entry.

2) The text of the warning assumes a particular kind of failure, but
I think there can be multiple kinds of failures here.
E.g., just because "this" isn't present doesn't mean the function is
static: maybe the compiler forgot to emit the DIE for "this".

The provided testcase for 18021 has this:

    <174b>   DW_AT_virtuality  : 1	(virtual)

even though there is no DIE for "this".

[Interestingly, the same DIE also has this:
    <174f>   Unknown AT value: 3fe1: 1	
but dwarf2.def has this:
DW_AT (DW_AT_APPLE_optimized, 0x3fe1)
and yet the producer is gcc. Heh.]

The logic of the surrounding code is a bit confusing, but that's not your
fault. I would have expected the code to first do a test of DW_AT_virtuality
to determine virtuality, and *then* check DW_AT_vtable_elem_location.
E.g., we don't verify that if virtuality is zero then
DW_AT_vtable_elem_location is not present.

In this case, given that DW_AT_virtuality is one, I'd say this is a
case of a missing "this" rather than a static function marked virtual.
If GDB wanted to be forgiving it could even manufacture a "this",
but that's not a requirement for this patch of course.

OTOH, complaints are hardly ever paid attention to, so we don't need
to put much time into being fancy. Given that we have
DW_AT_vtable_elem_location I think assuming that in the warning would
be better than claiming a static function is marked virtual.
So how about just rewording the text of the complaint to
"virtual member function missing \"this\"." or some such.

3) This comment

+	      /* If there is no `this' field, we cannot actually find a
+		 base class context for the vtable!  */

isn't entirely accurate. E.g., there could be a DW_AT_containing_type
attribute (which we check for earlier in the function). I'd rephrase it to:

+	      /* If there is no `this' field, and no DW_AT_containing_type,
+		 we cannot actually find a base class context for the
+		 vtable!  */

 > ---
 >  gdb/dwarf2read.c                           | 18 +++++++++-
 >  gdb/testsuite/gdb.dwarf2/staticvirtual.exp | 54 ++++++++++++++++++++++++++++++
 >  2 files changed, 71 insertions(+), 1 deletion(-)
 >  create mode 100644 gdb/testsuite/gdb.dwarf2/staticvirtual.exp
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index 071f97b..d4d65de 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -12916,7 +12916,23 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
 >  	    dwarf2_complex_location_expr_complaint ();
 >  
 >  	  if (!fnp->fcontext)
 > -	    fnp->fcontext = TYPE_TARGET_TYPE (TYPE_FIELD_TYPE (this_type, 0));
 > +	    {
 > +	      /* If there is no `this' field, we cannot actually find a
 > +		 base class context for the vtable!  */
 > +	      if (TYPE_NFIELDS (this_type) == 0
 > +		  || !TYPE_FIELD_ARTIFICIAL (this_type, 0))
 > +		{
 > +		  complaint (&symfile_complaints,
 > +			     _("Static member function \"%s\" (offset %d) "
 > +			       "is declared virtual"),
 > +			     fieldname, die->offset.sect_off);
 > +		}
 > +	      else
 > +		{
 > +		  fnp->fcontext
 > +		    = TYPE_TARGET_TYPE (TYPE_FIELD_TYPE (this_type, 0));
 > +		}
 > +	    }
 >  	}
 >        else if (attr_form_is_section_offset (attr))
 >          {
 > diff --git a/gdb/testsuite/gdb.dwarf2/staticvirtual.exp b/gdb/testsuite/gdb.dwarf2/staticvirtual.exp
 > new file mode 100644
 > index 0000000..06d46e1
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.dwarf2/staticvirtual.exp
 > @@ -0,0 +1,54 @@
 > +# Copyright 2015 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/>.
 > +load_lib dwarf.exp
 > +
 > +# This test can only be run on targets which support DWARF-2 and use gas.
 > +if {![dwarf2_support]} {
 > +    return 0
 > +}
 > +
 > +if { [skip_cplus_tests] } { continue }
 > +
 > +standard_testfile main.c staticvirtual-dw.S
 > +
 > +# Make DWARF for the test.
 > +set asm_file [standard_output_file $srcfile2]
 > +Dwarf::assemble $asm_file {
 > +    cu {} {
 > +	compile_unit {{language @DW_LANG_C_plus_plus}} {
 > +	    structure_type {
 > +		{name S}
 > +		{byte_size 1 DW_FORM_sdata}
 > +	    } {
 > +		subprogram {
 > +		    {low_pc 0x1000 addr}
 > +		    {high_pc 0x2000 addr}
 > +		    {name ~S}
 > +		    {external 1 flag}
 > +		    {virtuality @DW_VIRTUALITY_virtual}
 > +		    {vtable_elem_location {const1u 1} SPECIAL_expr}
 > +		}
 > +	    }
 > +	}
 > +    }
 > +}
 > +
 > +if { [prepare_for_testing ${testfile}.exp ${testfile} \
 > +	  [list $srcfile $asm_file] {nodebug}] } {
 > +    return -1
 > +}
 > +
 > +# gdb/18021: The test below would cause GDB to crash.
 > +gdb_test "p S::~S" "0x1000"
 > -- 
 > 2.1.0
 >