PR gdb/18021 - defend against "static virtual" methods

  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.
---
 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
  

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
 >