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

[Keith Seitz <keiths at redhat dot com>]

On 03/02/2015 03:28 PM, Doug Evans wrote:
> 1) Include PR number in changelog entry.
>
Fixed. Grr! I seem to still occasionally forget about that.

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

We cannot predict the problem (or at least I cannot), only deal with what we're given. In this case, we have a DIE that describes a method that is both virtual and static (for the definition of static used/assumed by dwarf2read.c).

If we didn't know that it was for a destructor, we would have *no* way to know whether the compiler omitted `this' or added virtuality/vtable_elem_location.

> If GDB wanted to be forgiving it could even manufacture a "this",
> but that's not a requirement for this patch of course.

Yeah, I thought about that, but in the absence of this bug, and given its existence in some unknown compiler, I question how useful such a change would be. So I have chosen to simply fix the (crashing) bug.

> So how about just rewording the text of the complaint to
> "virtual member function missing \"this\"." or some such.
>

I've selected (the more concise?) 'cannot determine context for virtual member function \"%s\" (offset %d)'

Which is, after all, the real problem. [YMMV :-)]

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

Right, but as you point out, that option was explored earlier in the function. In the context of where the complaint is added, the statement is completely accurate.

Nonetheless, the following is still more complete, so I've changed it to this:

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

Keith

gdb/ChangeLog
	PR gdb/18021
	* dwarf2read.c (dwarf2_add_member_fn): Issue a complaint
	if we find a static method with DW_AT_vtable_elem_location.

gdb/testsuite/ChangeLog
	PR gdb/18021
	* gdb.dwarf2/staticvirtual.exp: New test.
---
 gdb/dwarf2read.c                           | 19 ++++++++++-
 gdb/testsuite/gdb.dwarf2/staticvirtual.exp | 54 ++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/staticvirtual.exp

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index c185d51..264421f 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -12918,7 +12918,24 @@ 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 and no DW_AT_containing_type,
+		 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,
+			     _("cannot determine context for virtual member "
+			       "function \"%s\" (offset %d)"),
+			     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