[PATCH v3 0/2] [gdb] untie and validate inheritance location

[PATCH v3 0/2] [gdb] untie and validate inheritance location

[Bruno Larsen]

This started with what was thought of as an incorrect inheritance
location expression, but turned out to be a correct, but unusual
expression, and uncovered an incorrect assumption when calculating
offsets of baseclasses. 

This patchset is the new iteration of that solution, that changed
gnuv3_baseclass_offset calculation to allow for non-virtual inheritance
that has a non-trivial location expression and also moved the basic
validation to this function. The check in value_contents_copy_raw was
changed to an assert.
Finally, the test was expanded to have one incorrect trivial location,
and two non-trivial location expressions, one correct and one incorrect.

Bruno Larsen (2):
  [gdb] Untied inheritance virtuality and loc kind
  [gdb] Add Simple offset validation when calculating baseclass_offset

 gdb/gnu-v3-abi.c                              |  23 +-
 .../gdb.dwarf2/dw2-inheritance-locexpr-1.exp  | 233 +++++++++++++++++
 .../gdb.dwarf2/dw2-inheritance-locexpr-2.exp  | 235 ++++++++++++++++++
 .../gdb.dwarf2/dw2-inheritance-locexpr.c      |  69 +++++
 gdb/value.c                                   |   8 +-
 5 files changed, 561 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c

-- 
2.27.0

[PATCH v3 1/2] [gdb] Untied inheritance virtuality and loc kind

[Bruno Larsen]

DWARF specifications, when talking about C++ virtual classes, don't tie
their existance or non-existance to how the location is described, but
the original offset decoding did, assuming that we could use BITPOS for
non-virtual inheritance.

The new code assumes that if there isn't virtuality at play, we probably
have BITPOS, but we may need to calculate an expression, so allows for
fallthrough. Also, the special case on value_primitive_field, on
gdb/value.c, was changed so we call baseclass_offset on non-trivial
expressions.

A new testcase was added, where we use a trivial locexpr instead of a
bitpos, to showcase one of this instances
---
 gdb/gnu-v3-abi.c                              |  15 +-
 .../gdb.dwarf2/dw2-inheritance-locexpr-1.exp  | 233 ++++++++++++++++++
 .../gdb.dwarf2/dw2-inheritance-locexpr.c      |  69 ++++++
 gdb/value.c                                   |   8 +-
 4 files changed, 319 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 45e57c210cb..55e0cf24deb 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -458,10 +458,17 @@ gnuv3_baseclass_offset (struct type *type, int index,
   gdbarch = type->arch ();
   ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
 
-  /* If it isn't a virtual base, this is easy.  The offset is in the
-     type definition.  */
-  if (!BASETYPE_VIA_VIRTUAL (type, index))
-    return TYPE_BASECLASS_BITPOS (type, index) / 8;
+  if (!BASETYPE_VIA_VIRTUAL (type, index)){
+      /* If it isn't a virtual base, this is easy.
+         The offset is probably in the type definition.
+	 However, the DWARF specification doesn't tie virtuality with location
+	 so we may need to resolve location expressions.
+	 To reuse code, we just fall-through in that case */
+      if (TYPE_FIELD_LOC_KIND (type, index) == FIELD_LOC_KIND_BITPOS){
+	  cur_base_offset = TYPE_BASECLASS_BITPOS (type, index) / 8;
+	return TYPE_BASECLASS_BITPOS (type, index) / 8;
+      }
+  }
 
   /* If we have a DWARF expression for the offset, evaluate it.  */
   if (TYPE_FIELD_LOC_KIND (type, index) == FIELD_LOC_KIND_DWARF_BLOCK)
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
new file mode 100644
index 00000000000..c1a7bea1b88
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
@@ -0,0 +1,233 @@
+# Copyright 2018-2021 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
+}
+
+# We'll place the output of Dwarf::assemble in pr28030.S.
+standard_testfile dw2-inheritance-locexpr.c .S
+
+# ${testfile} is now "pr28030".  srcfile2 is "pr28030.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.
+#
+# For that, we ask GDB by debugging our pr28030 program.
+# Any program would do, but since we already have pr28030
+# specifically for this testcase, might as well use that.
+if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
+    return -1
+}
+set long_size [get_sizeof "long" -1]
+# gdb always assumes references are implemented as pointers.
+set addr_size [get_sizeof "void *" -1]
+set struct_A_size [get_sizeof "g_A" 4]
+set struct_B_size [get_sizeof "g_B" 4]
+set struct_C_size [get_sizeof "g_C" 4]
+
+# Create the DWARF.
+Dwarf::assemble ${asm_file} {
+    global srcdir subdir srcfile srcfile2
+    global long_size addr_size struct_A_size struct_B_size struct_C_size
+    declare_labels L
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_C_plus_plus}
+	    {name pr28030.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+        } {
+	    declare_labels int_label class_A_label class_B_label class_C_label
+
+	    int_label: DW_TAG_base_type {
+		{DW_AT_byte_size ${long_size} DW_FORM_udata}
+		{DW_AT_encoding @DW_ATE_signed}
+		{DW_AT_name "int"}
+	    }
+
+	    class_A_label: DW_TAG_class_type {
+		{DW_AT_name "A"}
+		{DW_AT_byte_size $struct_A_size DW_FORM_sdata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 0*$long_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "x"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 1*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_B_label: DW_TAG_class_type {
+		{DW_AT_name "B"}
+		{DW_AT_byte_size $struct_B_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location {
+			DW_OP_constu 0
+			DW_OP_plus
+			DW_OP_stack_value } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_C_label: DW_TAG_class_type {
+		{DW_AT_name "C"}
+		{DW_AT_byte_size $struct_C_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location { DW_OP_constu 9000 } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_A"}
+		{DW_AT_type :$class_A_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_A"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_B"}
+		{DW_AT_type :$class_B_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_B"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_C"}
+		{DW_AT_type :$class_C_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_C"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "foo" }}
+		{DW_AT_type :${class_B_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "bar" }}
+		{DW_AT_type :${class_C_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "main" }}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    }
+	}
+    }
+
+    lines {version 2} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	    main_start main_len
+	set main_end "$main_start + $main_len"
+	lassign [function_range foo [list ${srcdir}/${subdir}/$srcfile]] \
+	    foo_start foo_len
+	set foo_end "$foo_start + $foo_len"
+	lassign [function_range bar [list ${srcdir}/${subdir}/$srcfile]] \
+	    bar_start bar_len
+	set bar_end "$bar_start + $bar_len"
+
+	# Generate a line table program.  An attempt was made to make it
+	# reasonably accurate as it made debugging the test case easier.
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {line [gdb_get_line_number "main prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label}
+	    {line [gdb_get_line_number "main foo call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_bar_label}
+	    {line [gdb_get_line_number "main bar call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {line [gdb_get_line_number "main return"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {line [expr [gdb_get_line_number "main end"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $foo_start}
+	    {line [gdb_get_line_number "foo prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label}
+	    {line [gdb_get_line_number "foo return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "foo end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $foo_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $bar_start}
+	    {line [gdb_get_line_number "bar prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address bar_label}
+	    {line [gdb_get_line_number "bar return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "bar end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $bar_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	}
+    }
+}
+
+if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "step" "foo .. at .* foo end.*" "step into foo"
+gdb_test "finish" "= {<A> = {a = 5, x = 6}, b = 7}" "finish out of foo"
+
+if ![runto_main] {
+    return -1
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c
new file mode 100644
index 00000000000..0de0cb04b34
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c
@@ -0,0 +1,69 @@
+/* Copyright (C) 2018-2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+typedef struct A {
+    long a;
+    long x;
+} A;
+
+struct A g_A = {3, 4};
+
+typedef struct B {
+    long a;
+    long x;
+    long b;
+} B;
+
+struct B g_B = { 5, 6, 7 };
+
+typedef struct C {
+    long a;
+    long x;
+    long b;
+} C;
+
+struct C g_C = { 8, 9, 10 };
+
+B
+foo ()
+{						/* foo prologue */
+  asm ("foo_label: .globl foo_label");
+  return g_B;					/* foo return */
+}						/* foo end */
+
+C
+bar()
+{						/* bar prologue */
+  asm ("bar_label: .globl bar_label");
+  return g_C;					/* bar return */
+}						/* bar end */
+
+int
+main (void)
+{						/* main prologue */
+  B v;
+  C ret;
+  asm ("main_label: .globl main_label");
+
+  v = foo ();					/* main foo call */
+
+  asm ("main_bar_label: .globl main_bar_label");
+  ret = bar();					/* main bar call */
+
+  asm ("main_label2: .globl main_label2");
+  return 0;					/* main return */
+}						/* main end */
diff --git a/gdb/value.c b/gdb/value.c
index 3a2bc13985e..c3f4b263e69 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3046,8 +3046,12 @@ value_primitive_field (struct value *arg1, LONGEST offset,
 
       /* We special case virtual inheritance here because this
 	 requires access to the contents, which we would rather avoid
-	 for references to ordinary fields of unavailable values.  */
-      if (BASETYPE_VIA_VIRTUAL (arg_type, fieldno))
+	 for references to ordinary fields of unavailable values.  
+	 TODO: we should probably try to change this special case, we are
+	 starting to add validation to baseclass_offset.
+	 */
+      if (BASETYPE_VIA_VIRTUAL (arg_type, fieldno) || 
+	  TYPE_FIELD_LOC_KIND(arg_type, fieldno) != FIELD_LOC_KIND_BITPOS)
 	boffset = baseclass_offset (arg_type, fieldno,
 				    value_contents (arg1),
 				    value_embedded_offset (arg1),
-- 
2.27.0

[PATCH v3 2/2] [gdb] Add Simple offset validation when calculating baseclass_offset

[Bruno Larsen]

Add a sinple validation to gnuv3_baseclass_offset when not dealing with
virtual inheritance. The validation is simply checking if the offset is
bigger than the size of the variable.

An assert was also added to value_contents_copy_raw, in gdb/value.c, to
ensure that no data from outside the src value is copied, and that we
dont copy to outside the dst variable.

A test case was added that has incorrect location expressions and tests
that the addresses are correctly identified as invalid.
---
 gdb/gnu-v3-abi.c                              |   8 +-
 .../gdb.dwarf2/dw2-inheritance-locexpr-2.exp  | 235 ++++++++++++++++++
 2 files changed, 242 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 55e0cf24deb..52e8d2f4a37 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -466,6 +466,8 @@ gnuv3_baseclass_offset (struct type *type, int index,
 	 To reuse code, we just fall-through in that case */
       if (TYPE_FIELD_LOC_KIND (type, index) == FIELD_LOC_KIND_BITPOS){
 	  cur_base_offset = TYPE_BASECLASS_BITPOS (type, index) / 8;
+	  if (cur_base_offset >= TYPE_LENGTH (type))
+	    error (_("Incorrect offset of baseclass"));
 	return TYPE_BASECLASS_BITPOS (type, index) / 8;
       }
   }
@@ -490,8 +492,12 @@ gnuv3_baseclass_offset (struct type *type, int index,
 
       CORE_ADDR result;
       if (dwarf2_evaluate_property (&prop, nullptr, &addr_stack, &result,
-				    true))
+				    true)){
+	if (!BASETYPE_VIA_VIRTUAL (type, index) &&
+	    (result - addr_stack.addr) >= TYPE_LENGTH (type))
+	    error (_("overly large offset in non-virtual member"));
 	return (int) (result - addr_stack.addr);
+      }
     }
 
   /* To access a virtual base, we need to use the vbase offset stored in
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
new file mode 100644
index 00000000000..485829dc28b
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
@@ -0,0 +1,235 @@
+# Copyright 2018-2021 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
+}
+
+# We'll place the output of Dwarf::assemble in pr28030.S.
+standard_testfile dw2-inheritance-locexpr.c .S
+
+# ${testfile} is now "pr28030".  srcfile2 is "pr28030.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.
+#
+# For that, we ask GDB by debugging our pr28030 program.
+# Any program would do, but since we already have pr28030
+# specifically for this testcase, might as well use that.
+if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
+    return -1
+}
+set long_size [get_sizeof "long" -1]
+# gdb always assumes references are implemented as pointers.
+set addr_size [get_sizeof "void *" -1]
+set struct_A_size [get_sizeof "g_A" 4]
+set struct_B_size [get_sizeof "g_B" 4]
+set struct_C_size [get_sizeof "g_C" 4]
+
+# Create the DWARF.
+Dwarf::assemble ${asm_file} {
+    global srcdir subdir srcfile srcfile2
+    global long_size addr_size struct_A_size struct_B_size struct_C_size
+    declare_labels L
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_C_plus_plus}
+	    {name pr28030.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+        } {
+	    declare_labels int_label class_A_label class_B_label class_C_label
+
+	    int_label: DW_TAG_base_type {
+		{DW_AT_byte_size ${long_size} DW_FORM_udata}
+		{DW_AT_encoding @DW_ATE_signed}
+		{DW_AT_name "int"}
+	    }
+
+	    class_A_label: DW_TAG_class_type {
+		{DW_AT_name "A"}
+		{DW_AT_byte_size $struct_A_size DW_FORM_sdata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 0*$long_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "x"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 1*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_B_label: DW_TAG_class_type {
+		{DW_AT_name "B"}
+		{DW_AT_byte_size $struct_B_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location {
+			DW_OP_constu 9000
+			DW_OP_plus
+			DW_OP_stack_value } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_C_label: DW_TAG_class_type {
+		{DW_AT_name "C"}
+		{DW_AT_byte_size $struct_C_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location { DW_OP_constu 9000 } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_A"}
+		{DW_AT_type :$class_A_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_A"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_B"}
+		{DW_AT_type :$class_B_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_B"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_C"}
+		{DW_AT_type :$class_C_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_C"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "foo" }}
+		{DW_AT_type :${class_B_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "bar" }}
+		{DW_AT_type :${class_C_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "main" }}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    }
+	}
+    }
+
+    lines {version 2} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	    main_start main_len
+	set main_end "$main_start + $main_len"
+	lassign [function_range foo [list ${srcdir}/${subdir}/$srcfile]] \
+	    foo_start foo_len
+	set foo_end "$foo_start + $foo_len"
+	lassign [function_range bar [list ${srcdir}/${subdir}/$srcfile]] \
+	    bar_start bar_len
+	set bar_end "$bar_start + $bar_len"
+
+	# Generate a line table program.  An attempt was made to make it
+	# reasonably accurate as it made debugging the test case easier.
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {line [gdb_get_line_number "main prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label}
+	    {line [gdb_get_line_number "main foo call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_bar_label}
+	    {line [gdb_get_line_number "main bar call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {line [gdb_get_line_number "main return"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {line [expr [gdb_get_line_number "main end"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $foo_start}
+	    {line [gdb_get_line_number "foo prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label}
+	    {line [gdb_get_line_number "foo return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "foo end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $foo_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $bar_start}
+	    {line [gdb_get_line_number "bar prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address bar_label}
+	    {line [gdb_get_line_number "bar return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "bar end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $bar_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	}
+    }
+}
+
+if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "step" "foo .. at .* foo end.*" "step into foo"
+gdb_test "finish" "= {<A> = <invalid address>, b = 7}" "finish out of foo"
+gdb_test "step" "bar .. at .* bar end.*" "step into bar"
+gdb_test "finish" "= {<A> = <invalid address>, b = 10}" "finish out of bar"
+
+if ![runto_main] {
+    return -1
+}
-- 
2.27.0

[PATCH v3 2/2 RESEND] [gdb] Add Simple offset validation when calculating baseclass_offset

[Bruno Larsen]

Add a sinple validation to gnuv3_baseclass_offset when not dealing with
virtual inheritance. The validation is simply checking if the offset is
bigger than the size of the variable.

An assert was also added to value_contents_copy_raw, in gdb/value.c, to
ensure that no data from outside the src value is copied, and that we
dont copy to outside the dst variable.

A test case was added that has incorrect location expressions and tests
that the addresses are correctly identified as invalid.
---
 gdb/gnu-v3-abi.c                              |   8 +-
 .../gdb.dwarf2/dw2-inheritance-locexpr-2.exp  | 235 ++++++++++++++++++
 gdb/value.c                                   |   5 +
 3 files changed, 247 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 55e0cf24deb..52e8d2f4a37 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -466,6 +466,8 @@ gnuv3_baseclass_offset (struct type *type, int index,
 	 To reuse code, we just fall-through in that case */
       if (TYPE_FIELD_LOC_KIND (type, index) == FIELD_LOC_KIND_BITPOS){
 	  cur_base_offset = TYPE_BASECLASS_BITPOS (type, index) / 8;
+	  if (cur_base_offset >= TYPE_LENGTH (type))
+	    error (_("Incorrect offset of baseclass"));
 	return TYPE_BASECLASS_BITPOS (type, index) / 8;
       }
   }
@@ -490,8 +492,12 @@ gnuv3_baseclass_offset (struct type *type, int index,
 
       CORE_ADDR result;
       if (dwarf2_evaluate_property (&prop, nullptr, &addr_stack, &result,
-				    true))
+				    true)){
+	if (!BASETYPE_VIA_VIRTUAL (type, index) &&
+	    (result - addr_stack.addr) >= TYPE_LENGTH (type))
+	    error (_("overly large offset in non-virtual member"));
 	return (int) (result - addr_stack.addr);
+      }
     }
 
   /* To access a virtual base, we need to use the vbase offset stored in
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
new file mode 100644
index 00000000000..485829dc28b
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
@@ -0,0 +1,235 @@
+# Copyright 2018-2021 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
+}
+
+# We'll place the output of Dwarf::assemble in pr28030.S.
+standard_testfile dw2-inheritance-locexpr.c .S
+
+# ${testfile} is now "pr28030".  srcfile2 is "pr28030.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.
+#
+# For that, we ask GDB by debugging our pr28030 program.
+# Any program would do, but since we already have pr28030
+# specifically for this testcase, might as well use that.
+if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
+    return -1
+}
+set long_size [get_sizeof "long" -1]
+# gdb always assumes references are implemented as pointers.
+set addr_size [get_sizeof "void *" -1]
+set struct_A_size [get_sizeof "g_A" 4]
+set struct_B_size [get_sizeof "g_B" 4]
+set struct_C_size [get_sizeof "g_C" 4]
+
+# Create the DWARF.
+Dwarf::assemble ${asm_file} {
+    global srcdir subdir srcfile srcfile2
+    global long_size addr_size struct_A_size struct_B_size struct_C_size
+    declare_labels L
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_C_plus_plus}
+	    {name pr28030.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+        } {
+	    declare_labels int_label class_A_label class_B_label class_C_label
+
+	    int_label: DW_TAG_base_type {
+		{DW_AT_byte_size ${long_size} DW_FORM_udata}
+		{DW_AT_encoding @DW_ATE_signed}
+		{DW_AT_name "int"}
+	    }
+
+	    class_A_label: DW_TAG_class_type {
+		{DW_AT_name "A"}
+		{DW_AT_byte_size $struct_A_size DW_FORM_sdata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 0*$long_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "x"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 1*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_B_label: DW_TAG_class_type {
+		{DW_AT_name "B"}
+		{DW_AT_byte_size $struct_B_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location {
+			DW_OP_constu 9000
+			DW_OP_plus
+			DW_OP_stack_value } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_C_label: DW_TAG_class_type {
+		{DW_AT_name "C"}
+		{DW_AT_byte_size $struct_C_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location { DW_OP_constu 9000 } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_A"}
+		{DW_AT_type :$class_A_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_A"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_B"}
+		{DW_AT_type :$class_B_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_B"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_C"}
+		{DW_AT_type :$class_C_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_C"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "foo" }}
+		{DW_AT_type :${class_B_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "bar" }}
+		{DW_AT_type :${class_C_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "main" }}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    }
+	}
+    }
+
+    lines {version 2} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	    main_start main_len
+	set main_end "$main_start + $main_len"
+	lassign [function_range foo [list ${srcdir}/${subdir}/$srcfile]] \
+	    foo_start foo_len
+	set foo_end "$foo_start + $foo_len"
+	lassign [function_range bar [list ${srcdir}/${subdir}/$srcfile]] \
+	    bar_start bar_len
+	set bar_end "$bar_start + $bar_len"
+
+	# Generate a line table program.  An attempt was made to make it
+	# reasonably accurate as it made debugging the test case easier.
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {line [gdb_get_line_number "main prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label}
+	    {line [gdb_get_line_number "main foo call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_bar_label}
+	    {line [gdb_get_line_number "main bar call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {line [gdb_get_line_number "main return"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {line [expr [gdb_get_line_number "main end"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $foo_start}
+	    {line [gdb_get_line_number "foo prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label}
+	    {line [gdb_get_line_number "foo return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "foo end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $foo_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $bar_start}
+	    {line [gdb_get_line_number "bar prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address bar_label}
+	    {line [gdb_get_line_number "bar return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "bar end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $bar_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	}
+    }
+}
+
+if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "step" "foo .. at .* foo end.*" "step into foo"
+gdb_test "finish" "= {<A> = <invalid address>, b = 7}" "finish out of foo"
+gdb_test "step" "bar .. at .* bar end.*" "step into bar"
+gdb_test "finish" "= {<A> = <invalid address>, b = 10}" "finish out of bar"
+
+if ![runto_main] {
+    return -1
+}
diff --git a/gdb/value.c b/gdb/value.c
index c3f4b263e69..348a1498c15 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1320,6 +1320,11 @@ value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
      mean we'd be copying garbage.  */
   gdb_assert (!dst->lazy && !src->lazy);
 
+  /* Confirm that, if we are copying something, the copy is contained
+     to the desired variables */
+  gdb_assert (((dst_offset + length) <= TYPE_LENGTH (VALUE_TYPE (dst))) &&
+	      ((src_offset + length) <= TYPE_LENGTH (BALUE_TYPE (src))));
+
   /* The overwritten DST range gets unavailability ORed in, not
      replaced.  Make sure to remember to implement replacing if it
      turns out actually necessary.  */
-- 
2.27.0

[PING][PATCH v3 0/2] [gdb] untie and validate inheritance location

[Bruno Larsen]

ping

On 9/30/21 16:26, Bruno Larsen wrote:
> This started with what was thought of as an incorrect inheritance
> location expression, but turned out to be a correct, but unusual
> expression, and uncovered an incorrect assumption when calculating
> offsets of baseclasses.
> 
> This patchset is the new iteration of that solution, that changed
> gnuv3_baseclass_offset calculation to allow for non-virtual inheritance
> that has a non-trivial location expression and also moved the basic
> validation to this function. The check in value_contents_copy_raw was
> changed to an assert.
> Finally, the test was expanded to have one incorrect trivial location,
> and two non-trivial location expressions, one correct and one incorrect.
> 
> Bruno Larsen (2):
>    [gdb] Untied inheritance virtuality and loc kind
>    [gdb] Add Simple offset validation when calculating baseclass_offset
> 
>   gdb/gnu-v3-abi.c                              |  23 +-
>   .../gdb.dwarf2/dw2-inheritance-locexpr-1.exp  | 233 +++++++++++++++++
>   .../gdb.dwarf2/dw2-inheritance-locexpr-2.exp  | 235 ++++++++++++++++++
>   .../gdb.dwarf2/dw2-inheritance-locexpr.c      |  69 +++++
>   gdb/value.c                                   |   8 +-
>   5 files changed, 561 insertions(+), 7 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c
> 


-- 
Cheers!
Bruno Larsen

Re: [PING*2][PATCH v3 0/2] [gdb] untie and validate inheritance location

[Bruno Larsen]

ping

On 10/19/21 16:15, Bruno Larsen wrote:
> ping
> 
> On 9/30/21 16:26, Bruno Larsen wrote:
>> This started with what was thought of as an incorrect inheritance
>> location expression, but turned out to be a correct, but unusual
>> expression, and uncovered an incorrect assumption when calculating
>> offsets of baseclasses.
>>
>> This patchset is the new iteration of that solution, that changed
>> gnuv3_baseclass_offset calculation to allow for non-virtual inheritance
>> that has a non-trivial location expression and also moved the basic
>> validation to this function. The check in value_contents_copy_raw was
>> changed to an assert.
>> Finally, the test was expanded to have one incorrect trivial location,
>> and two non-trivial location expressions, one correct and one incorrect.
>>
>> Bruno Larsen (2):
>>    [gdb] Untied inheritance virtuality and loc kind
>>    [gdb] Add Simple offset validation when calculating baseclass_offset
>>
>>   gdb/gnu-v3-abi.c                              |  23 +-
>>   .../gdb.dwarf2/dw2-inheritance-locexpr-1.exp  | 233 +++++++++++++++++
>>   .../gdb.dwarf2/dw2-inheritance-locexpr-2.exp  | 235 ++++++++++++++++++
>>   .../gdb.dwarf2/dw2-inheritance-locexpr.c      |  69 +++++
>>   gdb/value.c                                   |   8 +-
>>   5 files changed, 561 insertions(+), 7 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
>>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
>>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c
>>
> 
> 


-- 
Cheers!
Bruno Larsen

Re: [PING*3][PATCH v3 0/2] [gdb] untie and validate inheritance location

[Bruno Larsen]

Ping

On 11/1/21 10:17, Bruno Larsen wrote:
> ping
> 
> On 10/19/21 16:15, Bruno Larsen wrote:
>> ping
>>
>> On 9/30/21 16:26, Bruno Larsen wrote:
>>> This started with what was thought of as an incorrect inheritance
>>> location expression, but turned out to be a correct, but unusual
>>> expression, and uncovered an incorrect assumption when calculating
>>> offsets of baseclasses.
>>>
>>> This patchset is the new iteration of that solution, that changed
>>> gnuv3_baseclass_offset calculation to allow for non-virtual inheritance
>>> that has a non-trivial location expression and also moved the basic
>>> validation to this function. The check in value_contents_copy_raw was
>>> changed to an assert.
>>> Finally, the test was expanded to have one incorrect trivial location,
>>> and two non-trivial location expressions, one correct and one incorrect.
>>>
>>> Bruno Larsen (2):
>>>    [gdb] Untied inheritance virtuality and loc kind
>>>    [gdb] Add Simple offset validation when calculating baseclass_offset
>>>
>>>   gdb/gnu-v3-abi.c                              |  23 +-
>>>   .../gdb.dwarf2/dw2-inheritance-locexpr-1.exp  | 233 +++++++++++++++++
>>>   .../gdb.dwarf2/dw2-inheritance-locexpr-2.exp  | 235 ++++++++++++++++++
>>>   .../gdb.dwarf2/dw2-inheritance-locexpr.c      |  69 +++++
>>>   gdb/value.c                                   |   8 +-
>>>   5 files changed, 561 insertions(+), 7 deletions(-)
>>>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-1.exp
>>>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr-2.exp
>>>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-locexpr.c
>>>
>>
>>
> 
> 


-- 
Cheers!
Bruno Larsen

Re: [PING*3][PATCH v3 0/2] [gdb] untie and validate inheritance location

[Simon Marchi]

On 2021-11-08 1:33 p.m., Bruno Larsen via Gdb-patches wrote:
> Ping

Hi Bruno,

When fixing a bug, I like to add infrastructure and checks in place that
would have caught that bug, hoping that it will help catch more bugs of
the same class.  While reviewing your patch, I went into the rabbit hole
of making the value_contents & co functions return an array_view, and
then make it possible to copy from an array view to another, verifying
that the array views have the same size.  That would have caught the bug
in patch 2 I think.

Here's my series: https://sourceware.org/pipermail/gdb-patches/2021-November/183251.html

I hope for it to be merged soon, and then when we rebase your series on
top of it, the assertion in value_contents_copy_raw would no longer be
needed, since it would implicitly be done by the array_view copy.

Simon