[PATCH] Do not consider reference types as dynamic

[PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

Hello,

I'm working on a GCC patch that fixes the type of Ada functions that 
return their result by reference: the debugging information currently 
says they return a pointer to the result and my patch turns these into 
references. The intent is that when evaluating a call to such functions 
from debuggers, the returned value is automatically dereferenced:

    # Currently we have:
    (gdb) print my_function("foo")
    $1 = (access ...) 0x...

    # We would like to have instead:
    (gdb) print my_function("foo")
    $1 = <referenced object>

Doing so in GDB is currently not possible when the referenced object 
type is dynamic. Indeed, the return value for such calls is not in 
memory and GDB considers reference types as dynamic iff the referenced 
type is dynamic. As a consequence, the type resolving process ends up 
trying to read memory at address 0x0, fails to do so and aborts printing 
the final value:

     # With my locally patched GCC, I have:
     (gdb) print my_function("foo")
     $1 = Cannot access memory at address 0x0

The attached patch fixes GDB so that the above works. It just makes GDB 
never consider reference types as dynamic (which makes sense per se 
IMHO) so this memory problem does not occurs.

I tested this patch on x86_64-linux with C, C++, Ada, Fortran and Java 
compilers: it triggers no regression. Note that the testcase it adds 
(gdb.ada/funcall_ref.exp) cannot pass without my local GCC patch and 
relies on my previous submitted patch 
(https://www.sourceware.org/ml/gdb-patches/2015-03/msg00241.html).

Ok to push?
Thank you in advance!

     gdb/ChangeLog:
     2015-03-10  Pierre-Marie de Rodat  <derodat@adacore.com>

         * gdbtypes.c (is_dynamic_type_internal): Remove special handling
         of TYPE_CODE_REF types so that they are not considered as
         dynamic depending on the referenced type.

     gdb/testsuite/ChangeLog:
     2015-03-10  Pierre-Marie de Rodat  <derodat@adacore.com>

         * gdb.ada/funcall_ref.exp: New file.
         * gdb.ada/funcall_ref/foo.adb: New file.

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-Do-not-consider-reference-types-as-dynamic.patch --]
[-- Type: text/x-diff, Size: 4155 bytes --]

From fbd03f077be85a8c455aa82cf247cc12cc9dea5f Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Tue, 10 Mar 2015 09:18:45 +0100
Subject: [PATCH] Do not consider reference types as dynamic

Even when referenced types are dynamic, the corresponding referencing
type should not be considered as dynamic: it's only a pointer.  This
prevents reference type for values not in memory to be resolved.

gdb/ChangeLog:
2015-03-10  Pierre-Marie de Rodat  <derodat@adacore.com>

	* gdbtypes.c (is_dynamic_type_internal): Remove special handling
	of TYPE_CODE_REF types so that they are not considered as
	dynamic depending on the referenced type.

gdb/testsuite/ChangeLog:
2015-03-10  Pierre-Marie de Rodat  <derodat@adacore.com>

	* gdb.ada/funcall_ref.exp: New file.
	* gdb.ada/funcall_ref/foo.adb: New file.
---
 gdb/gdbtypes.c                            |  4 ---
 gdb/testsuite/gdb.ada/funcall_ref.exp     | 42 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/funcall_ref/foo.adb | 19 ++++++++++++++
 3 files changed, 61 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/funcall_ref.exp
 create mode 100644 gdb/testsuite/gdb.ada/funcall_ref/foo.adb

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index a80151c..f75eac5 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1748,10 +1748,6 @@ is_dynamic_type_internal (struct type *type, int top_level)
 {
   type = check_typedef (type);
 
-  /* We only want to recognize references at the outermost level.  */
-  if (top_level && TYPE_CODE (type) == TYPE_CODE_REF)
-    type = check_typedef (TYPE_TARGET_TYPE (type));
-
   /* Types that have a dynamic TYPE_DATA_LOCATION are considered
      dynamic, even if the type itself is statically defined.
      From a user's point of view, this may appear counter-intuitive;
diff --git a/gdb/testsuite/gdb.ada/funcall_ref.exp b/gdb/testsuite/gdb.ada/funcall_ref.exp
new file mode 100644
index 0000000..b754e59
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/funcall_ref.exp
@@ -0,0 +1,42 @@
+# Copyright 2008-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 "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable \
+                     [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+# Test printing and type-printing of a discriminated record that a function
+# returns by reference.
+
+gdb_test "p get (\"Hello world!\")" \
+         "= \\(n => 12, s => \"Hello world!\"\\)" \
+         "p get (\"Hello world!\")"
+
+gdb_test "ptype get (\"Hello world!\")" \
+         [multi_line "type = <ref> record" \
+                     "    n: natural;" \
+                     "    s: access array \\(1 \\.\\. n\\) of character;" \
+                     "end record"] \
+         "ptype get (\"Hello world!\")"
diff --git a/gdb/testsuite/gdb.ada/funcall_ref/foo.adb b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
new file mode 100644
index 0000000..17d7641
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
@@ -0,0 +1,19 @@
+procedure Foo is
+   type Bar (N : Natural) is record
+      S : String (1 .. N);
+   end record;
+
+   function Get (S : String) return Bar is
+   begin
+      return (N => S'Length, S => S);
+   end Get;
+
+   procedure Do_Nothing (B : Bar) is
+   begin
+      null;
+   end Do_Nothing;
+
+   B : Bar := Get ("Foo");
+begin
+   Do_Nothing (B); --  STOP
+end Foo;
-- 
2.3.1

Re: [PATCH] Do not consider reference types as dynamic

[Joel Brobecker]

Hello Pierre-Marie,

> I'm working on a GCC patch that fixes the type of Ada functions that
> return their result by reference: the debugging information
> currently says they return a pointer to the result and my patch
> turns these into references. The intent is that when evaluating a
> call to such functions from debuggers, the returned value is
> automatically dereferenced:
> 
>    # Currently we have:
>    (gdb) print my_function("foo")
>    $1 = (access ...) 0x...
> 
>    # We would like to have instead:
>    (gdb) print my_function("foo")
>    $1 = <referenced object>
> 
> Doing so in GDB is currently not possible when the referenced object
> type is dynamic. Indeed, the return value for such calls is not in
> memory and GDB considers reference types as dynamic iff the
> referenced type is dynamic. As a consequence, the type resolving
> process ends up trying to read memory at address 0x0, fails to do so
> and aborts printing the final value:
> 
>     # With my locally patched GCC, I have:
>     (gdb) print my_function("foo")
>     $1 = Cannot access memory at address 0x0
> 
> The attached patch fixes GDB so that the above works. It just makes
> GDB never consider reference types as dynamic (which makes sense per
> se IMHO) so this memory problem does not occurs.
> 
> I tested this patch on x86_64-linux with C, C++, Ada, Fortran and
> Java compilers: it triggers no regression. Note that the testcase it
> adds (gdb.ada/funcall_ref.exp) cannot pass without my local GCC
> patch and relies on my previous submitted patch
> (https://www.sourceware.org/ml/gdb-patches/2015-03/msg00241.html).

If it cannot pass without your GCC patch, let's add a setup_xfail.

> Ok to push?
> Thank you in advance!
> 
>     gdb/ChangeLog:
>     2015-03-10  Pierre-Marie de Rodat  <derodat@adacore.com>
> 
>         * gdbtypes.c (is_dynamic_type_internal): Remove special handling
>         of TYPE_CODE_REF types so that they are not considered as
>         dynamic depending on the referenced type.
> 
>     gdb/testsuite/ChangeLog:
>     2015-03-10  Pierre-Marie de Rodat  <derodat@adacore.com>
> 
>         * gdb.ada/funcall_ref.exp: New file.
>         * gdb.ada/funcall_ref/foo.adb: New file.

Mostly OK - a couple of minor comments below.

> +# Test printing and type-printing of a discriminated record that a function
> +# returns by reference.
> +
> +gdb_test "p get (\"Hello world!\")" \
> +         "= \\(n => 12, s => \"Hello world!\"\\)" \
> +         "p get (\"Hello world!\")"

If the test "label" (3rd argument) is the same as the command being
sent, you do not need to explicitly specify it. So, the following
is equivalent to your version:

        gdb_test "p get (\"Hello world!\")" \
                 "= \\(n => 12, s => \"Hello world!\"\\)"

> +gdb_test "ptype get (\"Hello world!\")" \
> +         [multi_line "type = <ref> record" \
> +                     "    n: natural;" \
> +                     "    s: access array \\(1 \\.\\. n\\) of character;" \
> +                     "end record"] \
> +         "ptype get (\"Hello world!\")"

Same here.

> +++ b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
> @@ -0,0 +1,19 @@
> +procedure Foo is

Missing copyright header.

Thank you,
-- 
Joel

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

Joel,

On 03/23/2015 02:36 PM, Joel Brobecker wrote:
> If it cannot pass without your GCC patch, let's add a setup_xfail.

Done.

> Mostly OK - a couple of minor comments below.
> [...]
> If the test "label" (3rd argument) is the same as the command being
> sent, you do not need to explicitly specify it. So, the following
> is equivalent to your version:
>
>          gdb_test "p get (\"Hello world!\")" \
>                   "= \\(n => 12, s => \"Hello world!\"\\)"
>
>> +gdb_test "ptype get (\"Hello world!\")" \
>> +         [multi_line "type = <ref> record" \
>> +                     "    n: natural;" \
>> +                     "    s: access array \\(1 \\.\\. n\\) of character;" \
>> +                     "end record"] \
>> +         "ptype get (\"Hello world!\")"
>
> Same here.

Fixed both.

>> +++ b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
>> @@ -0,0 +1,19 @@
>> +procedure Foo is
>
> Missing copyright header.

Likewise.

Here's the updated patch. Thank you for reviewing!

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-Do-not-consider-reference-types-as-dynamic.patch --]
[-- Type: text/x-diff, Size: 4948 bytes --]

From 5c456519c3651360120739d1ee6cf01d4a61adac Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Tue, 10 Mar 2015 09:18:45 +0100
Subject: [PATCH] Do not consider reference types as dynamic

Even when referenced types are dynamic, the corresponding referencing
type should not be considered as dynamic: it's only a pointer.  This
prevents reference type for values not in memory to be resolved.

gdb/ChangeLog:
2015-03-23  Pierre-Marie de Rodat  <derodat@adacore.com>

	* gdbtypes.c (is_dynamic_type_internal): Remove special handling
	of TYPE_CODE_REF types so that they are not considered as
	dynamic depending on the referenced type.

gdb/testsuite/ChangeLog:
2015-03-23  Pierre-Marie de Rodat  <derodat@adacore.com>

	* gdb.ada/funcall_ref.exp: New file.
	* gdb.ada/funcall_ref/foo.adb: New file.
---
 gdb/gdbtypes.c                            |  4 ---
 gdb/testsuite/gdb.ada/funcall_ref.exp     | 44 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/funcall_ref/foo.adb | 34 ++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/funcall_ref.exp
 create mode 100644 gdb/testsuite/gdb.ada/funcall_ref/foo.adb

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 217ec70..6c5a3bb 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1752,10 +1752,6 @@ is_dynamic_type_internal (struct type *type, int top_level)
 {
   type = check_typedef (type);
 
-  /* We only want to recognize references at the outermost level.  */
-  if (top_level && TYPE_CODE (type) == TYPE_CODE_REF)
-    type = check_typedef (TYPE_TARGET_TYPE (type));
-
   /* Types that have a dynamic TYPE_DATA_LOCATION are considered
      dynamic, even if the type itself is statically defined.
      From a user's point of view, this may appear counter-intuitive;
diff --git a/gdb/testsuite/gdb.ada/funcall_ref.exp b/gdb/testsuite/gdb.ada/funcall_ref.exp
new file mode 100644
index 0000000..444e9d2
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/funcall_ref.exp
@@ -0,0 +1,44 @@
+# Copyright 2008-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 "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable \
+                     [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+# Test printing and type-printing of a discriminated record that a function
+# returns by reference.
+
+# Currently, GCC describes such functions as returning pointers (instead of
+# references).
+setup_xfail *-*-*
+gdb_test "p get (\"Hello world!\")" \
+         "= \\(n => 12, s => \"Hello world!\"\\)" \
+
+setup_xfail *-*-*
+gdb_test "ptype get (\"Hello world!\")" \
+         [multi_line "type = <ref> record" \
+                     "    n: natural;" \
+                     "    s: access array \\(1 \\.\\. n\\) of character;" \
+                     "end record"] \
diff --git a/gdb/testsuite/gdb.ada/funcall_ref/foo.adb b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
new file mode 100644
index 0000000..84655d3
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
@@ -0,0 +1,34 @@
+--  Copyright 2008-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/>.
+
+procedure Foo is
+   type Bar (N : Natural) is record
+      S : String (1 .. N);
+   end record;
+
+   function Get (S : String) return Bar is
+   begin
+      return (N => S'Length, S => S);
+   end Get;
+
+   procedure Do_Nothing (B : Bar) is
+   begin
+      null;
+   end Do_Nothing;
+
+   B : Bar := Get ("Foo");
+begin
+   Do_Nothing (B); --  STOP
+end Foo;
-- 
2.3.4

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

Joel suggested in private that if we don't consider reference types as 
dynamic in is_dynamic_type_internal, maybe the corresponding handling of 
reference types in resolve_dynamic_type_internal could be removed too.

I've tried to do this and this still triggers no regression on 
x86_64-linux. It's not surprising actually, since 
resolve_dynamic_type_internal does nothing on a type when 
is_dynamic_type_internal returns false for it.

So here's the updated patch. Thanks Joel for the idea! :-)

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-Do-not-consider-reference-types-as-dynamic.patch --]
[-- Type: text/x-diff, Size: 5643 bytes --]

From f3fd1a7e81fad7a0b8e4f483f79b2470a3cf1499 Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Fri, 3 Apr 2015 10:40:52 +0200
Subject: [PATCH] Do not consider reference types as dynamic

Even when referenced types are dynamic, the corresponding referencing
type should not be considered as dynamic: it's only a pointer.  This
prevents reference type for values not in memory to be resolved.

gdb/ChangeLog:
2015-04-03  Pierre-Marie de Rodat  <derodat@adacore.com>

	* gdbtypes.c (is_dynamic_type_internal): Remove special handling
	of TYPE_CODE_REF types so that they are not considered as
	dynamic depending on the referenced type.
	(resolve_dynamic_type_internal): Likewise.

gdb/testsuite/ChangeLog:
2015-04-03  Pierre-Marie de Rodat  <derodat@adacore.com>

	* gdb.ada/funcall_ref.exp: New file.
	* gdb.ada/funcall_ref/foo.adb: New file.
---
 gdb/gdbtypes.c                            | 19 -------------
 gdb/testsuite/gdb.ada/funcall_ref.exp     | 44 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/funcall_ref/foo.adb | 34 ++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 19 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/funcall_ref.exp
 create mode 100644 gdb/testsuite/gdb.ada/funcall_ref/foo.adb

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 217ec70..6fb2e9a 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1752,10 +1752,6 @@ is_dynamic_type_internal (struct type *type, int top_level)
 {
   type = check_typedef (type);
 
-  /* We only want to recognize references at the outermost level.  */
-  if (top_level && TYPE_CODE (type) == TYPE_CODE_REF)
-    type = check_typedef (TYPE_TARGET_TYPE (type));
-
   /* Types that have a dynamic TYPE_DATA_LOCATION are considered
      dynamic, even if the type itself is statically defined.
      From a user's point of view, this may appear counter-intuitive;
@@ -2045,21 +2041,6 @@ resolve_dynamic_type_internal (struct type *type,
 
       switch (TYPE_CODE (type))
 	{
-	case TYPE_CODE_REF:
-	  {
-	    struct property_addr_info pinfo;
-
-	    pinfo.type = check_typedef (TYPE_TARGET_TYPE (type));
-	    pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
-	    pinfo.next = addr_stack;
-
-	    resolved_type = copy_type (type);
-	    TYPE_TARGET_TYPE (resolved_type)
-	      = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type),
-					       &pinfo, top_level);
-	    break;
-	  }
-
 	case TYPE_CODE_ARRAY:
 	  resolved_type = resolve_dynamic_array (type, addr_stack);
 	  break;
diff --git a/gdb/testsuite/gdb.ada/funcall_ref.exp b/gdb/testsuite/gdb.ada/funcall_ref.exp
new file mode 100644
index 0000000..444e9d2
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/funcall_ref.exp
@@ -0,0 +1,44 @@
+# Copyright 2008-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 "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable \
+                     [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+# Test printing and type-printing of a discriminated record that a function
+# returns by reference.
+
+# Currently, GCC describes such functions as returning pointers (instead of
+# references).
+setup_xfail *-*-*
+gdb_test "p get (\"Hello world!\")" \
+         "= \\(n => 12, s => \"Hello world!\"\\)" \
+
+setup_xfail *-*-*
+gdb_test "ptype get (\"Hello world!\")" \
+         [multi_line "type = <ref> record" \
+                     "    n: natural;" \
+                     "    s: access array \\(1 \\.\\. n\\) of character;" \
+                     "end record"] \
diff --git a/gdb/testsuite/gdb.ada/funcall_ref/foo.adb b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
new file mode 100644
index 0000000..84655d3
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/funcall_ref/foo.adb
@@ -0,0 +1,34 @@
+--  Copyright 2008-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/>.
+
+procedure Foo is
+   type Bar (N : Natural) is record
+      S : String (1 .. N);
+   end record;
+
+   function Get (S : String) return Bar is
+   begin
+      return (N => S'Length, S => S);
+   end Get;
+
+   procedure Do_Nothing (B : Bar) is
+   begin
+      null;
+   end Do_Nothing;
+
+   B : Bar := Get ("Foo");
+begin
+   Do_Nothing (B); --  STOP
+end Foo;
-- 
2.3.4

Re: [PATCH] Do not consider reference types as dynamic

[Joel Brobecker]

> >From f3fd1a7e81fad7a0b8e4f483f79b2470a3cf1499 Mon Sep 17 00:00:00 2001
> From: Pierre-Marie de Rodat <derodat@adacore.com>
> Date: Fri, 3 Apr 2015 10:40:52 +0200
> Subject: [PATCH] Do not consider reference types as dynamic
> 
> Even when referenced types are dynamic, the corresponding referencing
> type should not be considered as dynamic: it's only a pointer.  This
> prevents reference type for values not in memory to be resolved.
> 
> gdb/ChangeLog:
> 2015-04-03  Pierre-Marie de Rodat  <derodat@adacore.com>
> 
> 	* gdbtypes.c (is_dynamic_type_internal): Remove special handling
> 	of TYPE_CODE_REF types so that they are not considered as
> 	dynamic depending on the referenced type.
> 	(resolve_dynamic_type_internal): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 2015-04-03  Pierre-Marie de Rodat  <derodat@adacore.com>
> 
> 	* gdb.ada/funcall_ref.exp: New file.
> 	* gdb.ada/funcall_ref/foo.adb: New file.

This is OK, thank you, Pierre-Marie.

Note that the parameter "top_level" is no longer useful after
this patch is applied, which is actually quite a nice side-effect
of this patch. I hum'ed and ah'ed about whether to ask that it be
removed as part of this patch, but in the end, I decided that this patch
was fine without the cleanup, as "top_level" is used a several places,
and I felt that cleaning it up here would dilute a bit the essence of
this patch.  So, I think we can clean it up separately.

> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 217ec70..6fb2e9a 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1752,10 +1752,6 @@ is_dynamic_type_internal (struct type *type, int top_level)

Thank you,
-- 
Joel

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

On 04/03/2015 02:47 PM, Joel Brobecker wrote:
> This is OK, thank you, Pierre-Marie.

Cool! Thank you for the review!

> Note that the parameter "top_level" is no longer useful after
> this patch is applied, which is actually quite a nice side-effect
> of this patch. I hum'ed and ah'ed about whether to ask that it be
> removed as part of this patch, but in the end, I decided that this patch
> was fine without the cleanup, as "top_level" is used a several places,
> and I felt that cleaning it up here would dilute a bit the essence of
> this patch.  So, I think we can clean it up separately.

Oh, you are right, I did not notice this. The attached patch does this 
cleanup. No regression on x86_64-linux. Better to push these two in the 
same time. ;-)

[PATCH] gdbtypes.c: remove the usuned "top_level" parameter

This paramater is no longer useful after the previous commit, so remove
it as a cleanup.

gdb/ChangeLog:

         * gdbtypes.c (is_dynamic_type_internal): Remove the unused
         "top_level" parameter.
         (resolve_dynamic_type_internal): Remove the unused "top_level"
         parameter.  Update call to is_dynamic_type_internal.
         (is_dynamic_type): Update call to is_dynamic_type_internal.
         (resolve_dynamic_range): Update call to
         resolve_dynamic_type_internal.
         (resolve_dynamic_union): Likewise.
         (resolve_dynamic_struct): Likewise.
         (resolve_dynamic_type): Likewise.

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-gdbtypes.c-remove-the-usuned-top_level-parameter.patch --]
[-- Type: text/x-diff, Size: 5196 bytes --]

From 3e0a648d8c966e902a3d1c2779b499535d3f443b Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Fri, 3 Apr 2015 14:53:12 +0200
Subject: [PATCH] gdbtypes.c: remove the usuned "top_level" parameter

This paramater is no longer useful after the previous commit, so remove
it as a cleanup.

gdb/ChangeLog:

	* gdbtypes.c (is_dynamic_type_internal): Remove the unused
	"top_level" parameter.
	(resolve_dynamic_type_internal): Remove the unused "top_level"
	parameter.  Update call to is_dynamic_type_internal.
	(is_dynamic_type): Update call to is_dynamic_type_internal.
	(resolve_dynamic_range): Update call to
	resolve_dynamic_type_internal.
	(resolve_dynamic_union): Likewise.
	(resolve_dynamic_struct): Likewise.
	(resolve_dynamic_type): Likewise.
---
 gdb/gdbtypes.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 6fb2e9a..4aec5ec 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1748,7 +1748,7 @@ stub_noname_complaint (void)
 /* Worker for is_dynamic_type.  */
 
 static int
-is_dynamic_type_internal (struct type *type, int top_level)
+is_dynamic_type_internal (struct type *type)
 {
   type = check_typedef (type);
 
@@ -1773,7 +1773,7 @@ is_dynamic_type_internal (struct type *type, int top_level)
 	   of the range type are static.  It allows us to assume that
 	   the subtype of a static range type is also static.  */
 	return (!has_static_range (TYPE_RANGE_DATA (type))
-		|| is_dynamic_type_internal (TYPE_TARGET_TYPE (type), 0));
+		|| is_dynamic_type_internal (TYPE_TARGET_TYPE (type)));
       }
 
     case TYPE_CODE_ARRAY:
@@ -1782,9 +1782,9 @@ is_dynamic_type_internal (struct type *type, int top_level)
 
 	/* The array is dynamic if either the bounds are dynamic,
 	   or the elements it contains have a dynamic contents.  */
-	if (is_dynamic_type_internal (TYPE_INDEX_TYPE (type), 0))
+	if (is_dynamic_type_internal (TYPE_INDEX_TYPE (type)))
 	  return 1;
-	return is_dynamic_type_internal (TYPE_TARGET_TYPE (type), 0);
+	return is_dynamic_type_internal (TYPE_TARGET_TYPE (type));
       }
 
     case TYPE_CODE_STRUCT:
@@ -1794,7 +1794,7 @@ is_dynamic_type_internal (struct type *type, int top_level)
 
 	for (i = 0; i < TYPE_NFIELDS (type); ++i)
 	  if (!field_is_static (&TYPE_FIELD (type, i))
-	      && is_dynamic_type_internal (TYPE_FIELD_TYPE (type, i), 0))
+	      && is_dynamic_type_internal (TYPE_FIELD_TYPE (type, i)))
 	    return 1;
       }
       break;
@@ -1808,11 +1808,11 @@ is_dynamic_type_internal (struct type *type, int top_level)
 int
 is_dynamic_type (struct type *type)
 {
-  return is_dynamic_type_internal (type, 1);
+  return is_dynamic_type_internal (type);
 }
 
 static struct type *resolve_dynamic_type_internal
-  (struct type *type, struct property_addr_info *addr_stack, int top_level);
+  (struct type *type, struct property_addr_info *addr_stack);
 
 /* Given a dynamic range type (dyn_range_type) and a stack of
    struct property_addr_info elements, return a static version
@@ -1860,7 +1860,7 @@ resolve_dynamic_range (struct type *dyn_range_type,
 
   static_target_type
     = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (dyn_range_type),
-				     addr_stack, 0);
+				     addr_stack);
   static_range_type = create_range_type (copy_type (dyn_range_type),
 					 static_target_type,
 					 &low_bound, &high_bound);
@@ -1928,7 +1928,7 @@ resolve_dynamic_union (struct type *type,
 	continue;
 
       t = resolve_dynamic_type_internal (TYPE_FIELD_TYPE (resolved_type, i),
-					 addr_stack, 0);
+					 addr_stack);
       TYPE_FIELD_TYPE (resolved_type, i) = t;
       if (TYPE_LENGTH (t) > max_len)
 	max_len = TYPE_LENGTH (t);
@@ -1985,7 +1985,7 @@ resolve_dynamic_struct (struct type *type,
 
       TYPE_FIELD_TYPE (resolved_type, i)
 	= resolve_dynamic_type_internal (TYPE_FIELD_TYPE (resolved_type, i),
-					 &pinfo, 0);
+					 &pinfo);
       gdb_assert (TYPE_FIELD_LOC_KIND (resolved_type, i)
 		  == FIELD_LOC_KIND_BITPOS);
 
@@ -2016,23 +2016,21 @@ resolve_dynamic_struct (struct type *type,
 
 static struct type *
 resolve_dynamic_type_internal (struct type *type,
-			       struct property_addr_info *addr_stack,
-			       int top_level)
+			       struct property_addr_info *addr_stack)
 {
   struct type *real_type = check_typedef (type);
   struct type *resolved_type = type;
   struct dynamic_prop *prop;
   CORE_ADDR value;
 
-  if (!is_dynamic_type_internal (real_type, top_level))
+  if (!is_dynamic_type_internal (real_type))
     return type;
 
   if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
     {
       resolved_type = copy_type (type);
       TYPE_TARGET_TYPE (resolved_type)
-	= resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type), addr_stack,
-					 top_level);
+	= resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type), addr_stack);
     }
   else 
     {
@@ -2077,7 +2075,7 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
 {
   struct property_addr_info pinfo = {check_typedef (type), addr, NULL};
 
-  return resolve_dynamic_type_internal (type, &pinfo, 1);
+  return resolve_dynamic_type_internal (type, &pinfo);
 }
 
 /* See gdbtypes.h  */
-- 
2.3.4

Re: [PATCH] Do not consider reference types as dynamic

[Joel Brobecker]

> [PATCH] gdbtypes.c: remove the usuned "top_level" parameter
> 
> This paramater is no longer useful after the previous commit, so remove
> it as a cleanup.
> 
> gdb/ChangeLog:
> 
>         * gdbtypes.c (is_dynamic_type_internal): Remove the unused
>         "top_level" parameter.
>         (resolve_dynamic_type_internal): Remove the unused "top_level"
>         parameter.  Update call to is_dynamic_type_internal.
>         (is_dynamic_type): Update call to is_dynamic_type_internal.
>         (resolve_dynamic_range): Update call to
>         resolve_dynamic_type_internal.
>         (resolve_dynamic_union): Likewise.
>         (resolve_dynamic_struct): Likewise.
>         (resolve_dynamic_type): Likewise.

Looks good to me!

Thank you :)
-- 
Joel

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

On 04/03/2015 03:11 PM, Joel Brobecker wrote:
> Looks good to me!
>
> Thank you :)

Great! Both are pushed, now. Thank *you* again for reviewing!

-- 
Pierre-Marie de Rodat

Re: [PATCH] Do not consider reference types as dynamic

[Yao Qi]

Pierre-Marie de Rodat <derodat@adacore.com> writes:

> I've tried to do this and this still triggers no regression on
> x86_64-linux. It's not surprising actually, since
> resolve_dynamic_type_internal does nothing on a type when
> is_dynamic_type_internal returns false for it.

Hi, Pierre,
This patch causes a regression in gdb.cp/vla-cxx.exp unfortunately,

 print vlaref^M
 $2 = (int (&)[variable length]) @0x3fffffffe900: {5, 7, 9}^M
 (gdb) FAIL: gdb.cp/vla-cxx.exp: print vlaref

which is shown by buildbot
https://sourceware.org/ml/gdb-testers/2015-q2/msg00360.html

I am not familiar with gdb type and dynamic type, so can't give anything
useful on how to fix this regression.  Any thoughts?

-- 
Yao (齐尧)

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

Yao,

On 04/17/2015 12:47 PM, Yao Qi wrote:
> Hi, Pierre,

(My first name is "Pierre-Marie" ;-))

> This patch causes a regression in gdb.cp/vla-cxx.exp unfortunately,
>
>   print vlaref^M
>   $2 = (int (&)[variable length]) @0x3fffffffe900: {5, 7, 9}^M
>   (gdb) FAIL: gdb.cp/vla-cxx.exp: print vlaref
>
> which is shown by buildbot
> https://sourceware.org/ml/gdb-testers/2015-q2/msg00360.html
>
> I am not familiar with gdb type and dynamic type, so can't give anything
> useful on how to fix this regression.  Any thoughts?

Thank you for the heads up! I don't know what happens, so I will 
investigate and keep you updated.

-- 
Pierre-Marie de Rodat

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

On 04/17/2015 01:20 PM, Pierre-Marie de Rodat wrote:
>> This patch causes a regression in gdb.cp/vla-cxx.exp unfortunately,
>>
>>   print vlaref^M
>>   $2 = (int (&)[variable length]) @0x3fffffffe900: {5, 7, 9}^M
>>   (gdb) FAIL: gdb.cp/vla-cxx.exp: print vlaref
>>
>> which is shown by buildbot
>> https://sourceware.org/ml/gdb-testers/2015-q2/msg00360.html

This is tough!

For the record, I did not notice this regression because I ran the 
testsuite with GCC's development branch, and this testcase was already 
failing with it. It works with GCC 4.9.2, though.

So my understanding was that types are dynamic when we need runtime 
information to decode them properly (memory layout, range bounds, etc). 
With this definition, reference types are never dynamic since they are 
actually mere pointers (static size, no dynamic semantics).

Printing a reference value is supposed to automatically fetch and print 
the referenced object... and its type. This testcase also tells us that 
it's the "fully resolved type" that must be printed (I mean referenced 
types must be resolved, too).

Here, we have "vlaref": a reference to a VLA (variable-length array), 
hence a reference to a dynamic type (the bounds and thus the size of the 
array are dynamic). Because of the "dynamic type" definition above, the 
referenced type is kept as-is during evaluation. And then at 
type-printing time, we use the dynamic referenced type, hence the 
"variable length" in the output.

If the definition above is correct, then we should enhance 
typeprint.c/c-typeprint.c/etc. to resolve dynamic type as type printing 
goes in order to get the actual array bounds in the output. This means: 
propagating a (possibly null) value in a lot of functions and use it for 
type resolution when possible. I guess this would involve a big patch 
but would also cover the "ptype" case.

I guess another way to "just get things working" would be to revert my 
previous patch and to enhance the dynamic property mechanism 
(dwarf2loc.h) to handle reference values that are not present in memory. 
For instance: extend the property_addr_info structure to hold either the 
address of objects (as today) or the address of the referenced object 
(for reference types).

Thoughts?

-- 
Pierre-Marie de Rodat

Re: [PATCH] Do not consider reference types as dynamic

[Joel Brobecker]

> If the definition above is correct, then we should enhance
> typeprint.c/c-typeprint.c/etc. to resolve dynamic type as type
> printing goes in order to get the actual array bounds in the output.
> This means: propagating a (possibly null) value in a lot of
> functions and use it for type resolution when possible. I guess this
> would involve a big patch but would also cover the "ptype" case.
> 
> I guess another way to "just get things working" would be to revert
> my previous patch and to enhance the dynamic property mechanism
> (dwarf2loc.h) to handle reference values that are not present in
> memory. For instance: extend the property_addr_info structure to
> hold either the address of objects (as today) or the address of the
> referenced object (for reference types).
> 
> Thoughts?

Thanks for looking into that, Pierre-Marie.

My 2 cents, based on the little amount of experience we've had dealing
with dyanmic types in Ada, and the amount of experience we've had
dealing with the GNAT encodings, I have a feeling that it's best to
maintain reference types as being non-dynamic, and enhance the functions
that print reference objects instead. We should probably compare what
Ada does (ada_val_print_ref) compared to C, for instance, as we handle
those reference values correctly, I believe.

-- 
Joel

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

On 04/17/2015 05:30 PM, Joel Brobecker wrote:
> My 2 cents, based on the little amount of experience we've had dealing
> with dyanmic types in Ada, and the amount of experience we've had
> dealing with the GNAT encodings, I have a feeling that it's best to
> maintain reference types as being non-dynamic, and enhance the functions
> that print reference objects instead. We should probably compare what
> Ada does (ada_val_print_ref) compared to C, for instance, as we handle
> those reference values correctly, I believe.

Joel and I discussed this live: doing the type printing change is 
probably the best way in terms of "implementation consistency", but it's 
also going to be a lot of work.

So the plan is instead to try to enhance type resolution to handle 
references that are not addressable. In the meantime, I've reverted my 
patches so that the regression disappears. I'll submit another patch...

-- 
Pierre-Marie de Rodat

Re: [PATCH] Do not consider reference types as dynamic

[Pierre-Marie de Rodat]

On 04/20/2015 04:27 PM, Pierre-Marie de Rodat wrote:
> Joel and I discussed this live: doing the type printing change is
> probably the best way in terms of "implementation consistency", but it's
> also going to be a lot of work.
>
> So the plan is instead to try to enhance type resolution to handle
> references that are not addressable. In the meantime, I've reverted my
> patches so that the regression disappears. I'll submit another patch...

The patch series Joel submitted last Tuesday[1] solves the original 
issue: the plan I was talking about is actually implemented by these 
patches! So there's no need to go further on this matter.

[1] <https://www.sourceware.org/ml/gdb-patches/2015-04/msg00768.html>

-- 
Pierre-Marie de Rodat