[PATCH] Resolve dynamic types before computing pointer size

[PATCH] Resolve dynamic types before computing pointer size

[Keith Seitz]

Consider the following code:

int
get(int n, int m, int arr[n][m], int x, int y)
{
	arr[x][y] += 100;
	return arr[x][y];
}

If we break in this function and attempt to print arr[x][y],
we get an error: "Cannot perform pointer math on incomplete types, try
casting to a known type, or void *."

The DWARF information for this contains the array bounds, bound to
a local variable:

 <1><1aa>: Abbrev Number: 3 (DW_TAG_array_type)
    <1ab>   DW_AT_type        : <0x6d>
    <1af>   DW_AT_sibling     : <0x1bd>
 <2><1b3>: Abbrev Number: 4 (DW_TAG_subrange_type)
    <1b4>   DW_AT_type        : <0x8a>
    <1b8>   DW_AT_upper_bound : 3 byte block: 91 68 6   (DW_OP_fbreg: -24; DW_OP_deref)

In value_ptradd, where we attempt to do the pointer math, we completely
ignore the fact that the pointer's target type is an unresolved dynamic type.

This patch addresses this by changing the API for find_size_for_pointer
so that it takes the actual struct value of the pointer variable,
resolves the pointer's target type if it is dynamic, and returns
a size based on this resolved type.

As a consequence of this dynamic type resolution, we must now also pass this
new resolved type to the caller via an out parameter.
---
 gdb/testsuite/gdb.base/vla-multi-array.c   | 58 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/vla-multi-array.exp | 53 ++++++++++++++++++++
 gdb/valarith.c                             | 18 +++++--
 3 files changed, 125 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/vla-multi-array.c
 create mode 100644 gdb/testsuite/gdb.base/vla-multi-array.exp

diff --git a/gdb/testsuite/gdb.base/vla-multi-array.c b/gdb/testsuite/gdb.base/vla-multi-array.c
new file mode 100644
index 00000000000..c4ee223a6f1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/vla-multi-array.c
@@ -0,0 +1,58 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+int
+get2 (int n, int m, int arr[n][m], int x, int y)
+{
+  return arr[x][y];		/* break-here 2  */
+}
+
+int
+get3 (int n, int m, int o, int arr[n][m][o], int x, int y, int z)
+{
+  return arr[x][y][z];		/* break-here 3  */
+}
+
+int
+main (void)
+{
+  int array2[3][4];
+  int array3[2][2][2];
+
+  array2[0][0] = 0;
+  array2[0][1] = 1;
+  array2[0][2] = 2;
+  array2[0][3] = 3;
+  array2[1][0] = 10;
+  array2[1][1] = 11;
+  array2[1][2] = 12;
+  array2[1][3] = 13;
+  array2[2][0] = 20;
+  array2[2][1] = 21;
+  array2[2][2] = 22;
+  array2[2][3] = 23;
+  array3[0][0][0] = 0;
+  array3[0][0][1] = 1;
+  array3[0][1][0] = 10;
+  array3[0][1][1] = 11;
+  array3[1][0][0] = 100;
+  array3[1][0][1] = 101;
+  array3[1][1][0] = 110;
+  array3[1][1][1] = 111;
+
+  return get2 (3, 4, array2, 0, 0) + get3 (2, 2, 2, array3, 1, 1, 1);
+}
diff --git a/gdb/testsuite/gdb.base/vla-multi-array.exp b/gdb/testsuite/gdb.base/vla-multi-array.exp
new file mode 100644
index 00000000000..c93cf013055
--- /dev/null
+++ b/gdb/testsuite/gdb.base/vla-multi-array.exp
@@ -0,0 +1,53 @@
+# Copyright 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/>.
+
+# Test multi-dimensional VLA arrays.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return
+}
+
+if {![runto_main]} {
+    untested "couldn't run to main"
+    return
+}
+
+# A note on column values.  It would be easiest to use "expr ${i}${j}${k}",
+# but when I is 0, Tcl interprets this as octal input.  Tcl < 8.7 does not
+# support a decimal prefix (0d), so work around this by using regsub.
+
+# Check all values of array2.
+gdb_breakpoint [gdb_get_line_number "break-here 2"]
+gdb_continue_to_breakpoint "break-here 2"
+for {set i 0} {$i < 3} {incr i} {
+    for {set j 0} {$j < 4} {incr j} {
+	set pattern [regsub "^(0)" "${i}${j}" ""]
+	gdb_test "p arr\[$i\]\[$j\]" "= $pattern"
+    }
+}
+
+# Check all values of array3.
+gdb_breakpoint [gdb_get_line_number "break-here 3"]
+gdb_continue_to_breakpoint "break-here 3"
+for {set i 0} {$i < 2} {incr i} {
+    for {set j 0} {$j < 2} {incr j} {
+	for {set k 0} {$k < 2} {incr k} {
+	    set pattern [regsub "^(0|00)" "${i}${j}${k}" ""]
+	    gdb_test "p arr\[$i\]\[$j\]\[$k\]" "= $pattern"
+	}
+    }
+}
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 9ebad648b27..b7f4f3a71a6 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -36,21 +36,31 @@
 #define TRUNCATION_TOWARDS_ZERO ((-5 / 2) == -2)
 #endif
 
-/* Given a pointer, return the size of its target.
+/* Given a pointer variable, return the size of its target.
    If the pointer type is void *, then return 1.
    If the target type is incomplete, then error out.
+   PTR_TYPE_OUT will contain the the actual pointer target type,
+   which may be a resolved dynamic type.
    This isn't a general purpose function, but just a 
    helper for value_ptradd.  */
 
 static LONGEST
-find_size_for_pointer_math (struct type *ptr_type)
+find_size_for_pointer_math (struct value *ptr_var, struct type **ptr_type_out)
 {
   LONGEST sz = -1;
   struct type *ptr_target;
+  struct type *ptr_type = check_typedef (value_type (ptr_var));
 
   gdb_assert (ptr_type->code () == TYPE_CODE_PTR);
   ptr_target = check_typedef (TYPE_TARGET_TYPE (ptr_type));
 
+  if (is_dynamic_type (ptr_target))
+    {
+      ptr_target
+	= resolve_dynamic_type (ptr_target, {}, value_address (ptr_var));
+      ptr_type = make_pointer_type (ptr_target, nullptr);
+    }
+
   sz = type_length_units (ptr_target);
   if (sz == 0)
     {
@@ -69,6 +79,7 @@ find_size_for_pointer_math (struct type *ptr_type)
 		   "try casting to a known type, or void *."), name);
 	}
     }
+  *ptr_type_out = ptr_type;
   return sz;
 }
 
@@ -83,8 +94,7 @@ value_ptradd (struct value *arg1, LONGEST arg2)
   struct value *result;
 
   arg1 = coerce_array (arg1);
-  valptrtype = check_typedef (value_type (arg1));
-  sz = find_size_for_pointer_math (valptrtype);
+  sz = find_size_for_pointer_math (arg1, &valptrtype);
 
   result = value_from_pointer (valptrtype,
 			       value_as_address (arg1) + sz * arg2);
-- 
2.31.1

Re: [PATCH] Resolve dynamic types before computing pointer size

[Tom Tromey]

>>>>> "Keith" == Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:

Keith> This patch addresses this by changing the API for find_size_for_pointer
Keith> so that it takes the actual struct value of the pointer variable,
Keith> resolves the pointer's target type if it is dynamic, and returns
Keith> a size based on this resolved type.

Thanks, this looks good to me.

Tom

Re: [PATCH] Resolve dynamic types before computing pointer size

[Keith Seitz]

On 10/19/21 09:21, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Keith> This patch addresses this by changing the API for find_size_for_pointer
> Keith> so that it takes the actual struct value of the pointer variable,
> Keith> resolves the pointer's target type if it is dynamic, and returns
> Keith> a size based on this resolved type.
> 
> Thanks, this looks good to me.

Unfortunately, my patch to find_size_for_pointer_math interferes with your recently committed
patch ["Fix bug in dynamic type resolution"], which you "snuck" [:-)] in before I could finish
rebasing and retesting my patch.

With my patch applied:

+FAIL: gdb.ada/array_of_variant.exp: scenario=minimal: print first element of another_array
+FAIL: gdb.ada/array_of_variant.exp: scenario=minimal: print second element of another_array

In both cases, we see "Cannot access memory at address 0x4".

So until I figure this out, I'll have to put this patch into a holding pattern.

Thank you for the review!

Keith

Re: [PATCH] Resolve dynamic types before computing pointer size

[Tom Tromey]

>>>>> "Keith" == Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:

Keith> Unfortunately, my patch to find_size_for_pointer_math interferes with your recently committed
Keith> patch ["Fix bug in dynamic type resolution"], which you "snuck" [:-)] in before I could finish
Keith> rebasing and retesting my patch.

Sorry about that.

Keith> With my patch applied:

Keith> +FAIL: gdb.ada/array_of_variant.exp: scenario=minimal: print first element of another_array
Keith> +FAIL: gdb.ada/array_of_variant.exp: scenario=minimal: print second element of another_array

Keith> In both cases, we see "Cannot access memory at address 0x4".

Does the test pass without your patch?
I wonder if there's some other problem -- the Ada tests tend to be
fairly compiler-dependent.

Tom

Re: [PATCH] Resolve dynamic types before computing pointer size

[Tom Tromey]

Keith> Unfortunately, my patch to find_size_for_pointer_math interferes with your recently committed
Keith> patch ["Fix bug in dynamic type resolution"], which you "snuck" [:-)] in before I could finish
Keith> rebasing and retesting my patch.

Tom> Sorry about that.

I debugged this today and came up with the appended.

The ada-lang.c hunk is the important bit.  I don't see a reason to call
value_addr here, and this call results in a value that has no address.

It seems wrong to have a not_lval object with a 0 address that can then
be treated as a pointer.  Though in this case, the value has
contents... so I wonder if that's something your patch needs to account
for.

The other hunk just changes the failure from 0x4 to 0x0, which seems
more correct in this case.  Though I wonder if this should really go in,
since it seems like it could do the wrong thing on platforms where 0 is
a valid address.

Tom

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 8b9e94e25d8..b9c693e277c 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10685,9 +10685,6 @@ ada_funcall_operation::evaluate (struct type *expect_type,
 	 well.  */
       callee = ada_to_fixed_value (coerce_ref (callee));
     }
-  else if (value_type (callee)->code () == TYPE_CODE_ARRAY
-	   && VALUE_LVAL (callee) == lval_memory)
-    callee = value_addr (callee);
 
   struct type *type = ada_check_typedef (value_type (callee));
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b39d2b72348..b162e8fe20c 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2619,7 +2619,9 @@ resolve_dynamic_struct (struct type *type,
       pinfo.valaddr = addr_stack->valaddr;
       if (!pinfo.valaddr.empty ())
 	pinfo.valaddr = pinfo.valaddr.slice (offset);
-      pinfo.addr = addr_stack->addr + offset;
+      pinfo.addr = addr_stack->addr;
+      if (pinfo.addr != 0)
+	pinfo.addr += offset;
       pinfo.next = addr_stack;
 
       resolved_type->field (i).set_type

Re: [PATCH] Resolve dynamic types before computing pointer size

[Keith Seitz]

On 10/21/21 11:19, Tom Tromey wrote:
> Keith> Unfortunately, my patch to find_size_for_pointer_math interferes with your recently committed
> Keith> patch ["Fix bug in dynamic type resolution"], which you "snuck" [:-)] in before I could finish
> Keith> rebasing and retesting my patch.
> 
> Tom> Sorry about that.

Don't be. I apologize that it has taken me several weeks to get back
to this!

> I debugged this today and came up with the appended.

I appreciate this, but I do have some simple questions. I am
*quite* rusty when it comes to working on gdb nowadays...

> The ada-lang.c hunk is the important bit.  I don't see a reason to call
> value_addr here, and this call results in a value that has no address.
> 
> It seems wrong to have a not_lval object with a 0 address that can then
> be treated as a pointer.  Though in this case, the value has
> contents... so I wonder if that's something your patch needs to account
> for.

I can certainly call value_contents here instead. While
gdb.ada/array_of_variant.exp still fails, it does so without
erroring out:

Expected:
print another_array(1)
$6 = (initial => 0, rest => (tag => unused, cval => 88 'X'))

With my (+ value_contents) patch:
$6 = (initial => 0, rest => (tag => unused, ival => 88))

[This is where my previous patch used to error.] While I think this
is an improvement, there is obviously something amiss. Your
patch to ada-lang.c (ada_funcall_operation::evaluate) fixes this
failure. While I may agree that value_addr seems out-of-place here,
I will be the first to admit that the value system is probably the one area
of gdb that I understand the least. [Ada excepted, of course. I
definitely understand that the least!]

> The other hunk just changes the failure from 0x4 to 0x0, which seems
> more correct in this case.  Though I wonder if this should really go in,
> since it seems like it could do the wrong thing on platforms where 0 is
> a valid address.

Yeah, this whole 0 address dichotomy is always concerning. Nonetheless,
please allow me to ask, when you say "I wonder if this should really go
in," do you mean this particular hunk or both these hunks? [AFAICT,
this last hunk is unnecessary from my proposed patch's perspective.]

Keith

For the record, the proposed patch, including "necessary" bits of yours,
is:

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index e76a3000f61..4bbeee240f2 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10692,9 +10692,6 @@ ada_funcall_operation::evaluate (struct type *expect_type,
  	 well.  */
        callee = ada_to_fixed_value (coerce_ref (callee));
      }
-  else if (value_type (callee)->code () == TYPE_CODE_ARRAY
-	   && VALUE_LVAL (callee) == lval_memory)
-    callee = value_addr (callee);
  
    struct type *type = ada_check_typedef (value_type (callee));

diff --git a/gdb/valarith.c b/gdb/valarith.c
index 140ef448137..fb6b3c0819a 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -36,21 +36,32 @@
  #define TRUNCATION_TOWARDS_ZERO ((-5 / 2) == -2)
  #endif
  
-/* Given a pointer, return the size of its target.
+/* Given a pointer variable, return the size of its target.
     If the pointer type is void *, then return 1.
     If the target type is incomplete, then error out.
+   PTR_TYPE_OUT will contain the the actual pointer target type,
+   which may be a resolved dynamic type.
     This isn't a general purpose function, but just a
     helper for value_ptradd.  */
  
  static LONGEST
-find_size_for_pointer_math (struct type *ptr_type)
+find_size_for_pointer_math (struct value *ptr_var, struct type **ptr_type_out)
  {
    LONGEST sz = -1;
    struct type *ptr_target;
+  struct type *ptr_type = check_typedef (value_type (ptr_var));
  
    gdb_assert (ptr_type->code () == TYPE_CODE_PTR);
    ptr_target = check_typedef (TYPE_TARGET_TYPE (ptr_type));
  
+  if (is_dynamic_type (ptr_target))
+    {
+      gdb::array_view<const gdb_byte> view = value_contents (ptr_var);
+      ptr_target
+	= resolve_dynamic_type (ptr_target, view, value_address (ptr_var));
+      ptr_type = make_pointer_type (ptr_target, nullptr);
+    }
+
    sz = type_length_units (ptr_target);
    if (sz == 0)
      {
@@ -69,6 +80,7 @@ find_size_for_pointer_math (struct type *ptr_type)
  		   "try casting to a known type, or void *."), name);
  	}
      }
+  *ptr_type_out = ptr_type;
    return sz;
  }
  
@@ -83,8 +95,7 @@ value_ptradd (struct value *arg1, LONGEST arg2)
    struct value *result;
  
    arg1 = coerce_array (arg1);
-  valptrtype = check_typedef (value_type (arg1));
-  sz = find_size_for_pointer_math (valptrtype);
+  sz = find_size_for_pointer_math (arg1, &valptrtype);
  
    result = value_from_pointer (valptrtype,
  			       value_as_address (arg1) + sz * arg2);

Re: [PATCH] Resolve dynamic types before computing pointer size

[Keith Seitz]

Ping?

[The big remaining question for me is whether your proposed
ada-lang.c patch is correct. I have not been able to work around
that regression otherwise.]

Keith

On 11/11/21 10:21, Keith Seitz via Gdb-patches wrote:
> On 10/21/21 11:19, Tom Tromey wrote:
>> Keith> Unfortunately, my patch to find_size_for_pointer_math interferes with your recently committed
>> Keith> patch ["Fix bug in dynamic type resolution"], which you "snuck" [:-)] in before I could finish
>> Keith> rebasing and retesting my patch.
>>
>> Tom> Sorry about that.
> 
> Don't be. I apologize that it has taken me several weeks to get back
> to this!
> 
>> I debugged this today and came up with the appended.
> 
> I appreciate this, but I do have some simple questions. I am
> *quite* rusty when it comes to working on gdb nowadays...
> 
>> The ada-lang.c hunk is the important bit.  I don't see a reason to call
>> value_addr here, and this call results in a value that has no address.
>>
>> It seems wrong to have a not_lval object with a 0 address that can then
>> be treated as a pointer.  Though in this case, the value has
>> contents... so I wonder if that's something your patch needs to account
>> for.
> 
> I can certainly call value_contents here instead. While
> gdb.ada/array_of_variant.exp still fails, it does so without
> erroring out:
> 
> Expected:
> print another_array(1)
> $6 = (initial => 0, rest => (tag => unused, cval => 88 'X'))
> 
> With my (+ value_contents) patch:
> $6 = (initial => 0, rest => (tag => unused, ival => 88))
> 
> [This is where my previous patch used to error.] While I think this
> is an improvement, there is obviously something amiss. Your
> patch to ada-lang.c (ada_funcall_operation::evaluate) fixes this
> failure. While I may agree that value_addr seems out-of-place here,
> I will be the first to admit that the value system is probably the one area
> of gdb that I understand the least. [Ada excepted, of course. I
> definitely understand that the least!]
> 
>> The other hunk just changes the failure from 0x4 to 0x0, which seems
>> more correct in this case.  Though I wonder if this should really go in,
>> since it seems like it could do the wrong thing on platforms where 0 is
>> a valid address.
> 
> Yeah, this whole 0 address dichotomy is always concerning. Nonetheless,
> please allow me to ask, when you say "I wonder if this should really go
> in," do you mean this particular hunk or both these hunks? [AFAICT,
> this last hunk is unnecessary from my proposed patch's perspective.]
> 
> Keith
> 
> For the record, the proposed patch, including "necessary" bits of yours,
> is:
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index e76a3000f61..4bbeee240f2 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -10692,9 +10692,6 @@ ada_funcall_operation::evaluate (struct type *expect_type,
>        well.  */
>         callee = ada_to_fixed_value (coerce_ref (callee));
>       }
> -  else if (value_type (callee)->code () == TYPE_CODE_ARRAY
> -       && VALUE_LVAL (callee) == lval_memory)
> -    callee = value_addr (callee);
> 
>     struct type *type = ada_check_typedef (value_type (callee));
> 
> diff --git a/gdb/valarith.c b/gdb/valarith.c
> index 140ef448137..fb6b3c0819a 100644
> --- a/gdb/valarith.c
> +++ b/gdb/valarith.c
> @@ -36,21 +36,32 @@
>   #define TRUNCATION_TOWARDS_ZERO ((-5 / 2) == -2)
>   #endif
> 
> -/* Given a pointer, return the size of its target.
> +/* Given a pointer variable, return the size of its target.
>      If the pointer type is void *, then return 1.
>      If the target type is incomplete, then error out.
> +   PTR_TYPE_OUT will contain the the actual pointer target type,
> +   which may be a resolved dynamic type.
>      This isn't a general purpose function, but just a
>      helper for value_ptradd.  */
> 
>   static LONGEST
> -find_size_for_pointer_math (struct type *ptr_type)
> +find_size_for_pointer_math (struct value *ptr_var, struct type **ptr_type_out)
>   {
>     LONGEST sz = -1;
>     struct type *ptr_target;
> +  struct type *ptr_type = check_typedef (value_type (ptr_var));
> 
>     gdb_assert (ptr_type->code () == TYPE_CODE_PTR);
>     ptr_target = check_typedef (TYPE_TARGET_TYPE (ptr_type));
> 
> +  if (is_dynamic_type (ptr_target))
> +    {
> +      gdb::array_view<const gdb_byte> view = value_contents (ptr_var);
> +      ptr_target
> +    = resolve_dynamic_type (ptr_target, view, value_address (ptr_var));
> +      ptr_type = make_pointer_type (ptr_target, nullptr);
> +    }
> +
>     sz = type_length_units (ptr_target);
>     if (sz == 0)
>       {
> @@ -69,6 +80,7 @@ find_size_for_pointer_math (struct type *ptr_type)
>              "try casting to a known type, or void *."), name);
>       }
>       }
> +  *ptr_type_out = ptr_type;
>     return sz;
>   }
> 
> @@ -83,8 +95,7 @@ value_ptradd (struct value *arg1, LONGEST arg2)
>     struct value *result;
> 
>     arg1 = coerce_array (arg1);
> -  valptrtype = check_typedef (value_type (arg1));
> -  sz = find_size_for_pointer_math (valptrtype);
> +  sz = find_size_for_pointer_math (arg1, &valptrtype);
> 
>     result = value_from_pointer (valptrtype,
>                      value_as_address (arg1) + sz * arg2);
>