FYI: [varobj] false type-changed status for reference to Ada array

FYI: [varobj] false type-changed status for reference to Ada array

[Joel Brobecker]

Hello,

Given the following variable...

   BT : Bounded := New_Bounded (Low => 1, High => 3);

... where type Bounded is defined as a simple unconstrained array:

   type Bounded is array (Integer range <>) of Integer;

Creating a varobj for that variable, and immediately asking for
varobj updates, GDB says that our varobj changed types!

    (gdb)
    -var-create bt * bt
    ^done,name="bt",numchild="3",value="[3]",type="<ref> array (1 .. 3) of integer",has_more="0"
    (gdb)
    -var-update 1 *
    ^done,changelist=[{name="bt",value="[3]",in_scope="true",type_changed="true",new_type="<ref> array (1 .. 3) of integer",new_num_children="3",has_more="0"}]

The expected output for the -var-update command is, in this case:

    (gdb)
    -var-update 1 *
    ^done,changelist=[]

The problem occurs because the ada-varobj module does not handle
references, and while the references gets stripped when the varobj
gets created, it doesn't when computing varobj updates.

More specifically, when creating the varobj, varobj_create creates
a new value which is a reference to a TYPE_CODE_ARRAY. It then calls
install_new_value which calls coerce_ref with the following comment:

    /* We are not interested in the address of references, and given
       that in C++ a reference is not rebindable, it cannot
       meaningfully change.  So, get hold of the real value.  */
    if (value)
      value = coerce_ref (value);

This leaves the varobj's type component still a ref, while
the varobj's value is now our array, without the ref. This explains
why the "value" field in the varobj indicates an array with 3 elements
"[3]" while the "type" field shows a ref to an array. Generally
speaking, most users have said that showing the ref was a useful
piece of information, so this patch is not touching this part.

Next, when the user issues the -var-update request, varobj_update
calls value_of_root to compute the varobj's new value as well as
determine whether the value's type has changed or not. What happens
in a nutshell is that it calls value_of_root_1 (which re-evaluates
the expression and returns the corresponding new value), finds that
the new value is not NULL, and thus asks whether it has mutated:

    else if (varobj_value_has_mutated (var, value, value_type (value)))

This then indirectly delegates the determination to the language-specific
callback, which fails, because it does not handle references.

This patch fixes the issue by adjusting varobj_value_has_mutated to
expect references, and strip them when seen. This allows the various
language-specific implementations to remain unaware of references.

gdb/ChangeLog:

        * varobj.c (varobj_value_has_mutated): If NEW_VALUE is
        a reference, strip the reference layer before calling
        the lang_ops value_has_mutated callback.

gdb/testsuite/ChangeLog:

        * gdb.ada/mi_dyn_arr: New testcase.

Tested on x86_64-linux. I will commit in a couple of days unless
there are comments/objections.

Thank you,
-- 
Joel

---
 gdb/testsuite/gdb.ada/mi_dyn_arr.exp     | 52 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/mi_dyn_arr/foo.adb | 24 +++++++++++++++
 gdb/testsuite/gdb.ada/mi_dyn_arr/pck.adb | 30 ++++++++++++++++++
 gdb/testsuite/gdb.ada/mi_dyn_arr/pck.ads | 21 +++++++++++++
 gdb/varobj.c                             | 11 ++++++-
 5 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.ada/mi_dyn_arr.exp
 create mode 100644 gdb/testsuite/gdb.ada/mi_dyn_arr/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/mi_dyn_arr/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/mi_dyn_arr/pck.ads

diff --git a/gdb/testsuite/gdb.ada/mi_dyn_arr.exp b/gdb/testsuite/gdb.ada/mi_dyn_arr.exp
new file mode 100644
index 0000000..434c21d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_dyn_arr.exp
@@ -0,0 +1,52 @@
+# Copyright 2014 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
+}
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+
+if ![mi_run_to_main] then {
+   fail "Cannot run to main, testcase aborted"
+   return 0
+}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.adb]
+mi_continue_to_line \
+    "foo.adb:$bp_location" \
+    "stop at start of main Ada procedure"
+
+mi_gdb_test "-var-create bt * bt" \
+    "\\^done,name=\"bt\",numchild=\"3\",.*" \
+    "Create bt varobj"
+
+mi_gdb_test "-var-update 1 *" \
+    "\\^done,changelist=\\\[\\\]" \
+    "list ggg1's children"
diff --git a/gdb/testsuite/gdb.ada/mi_dyn_arr/foo.adb b/gdb/testsuite/gdb.ada/mi_dyn_arr/foo.adb
new file mode 100644
index 0000000..4a9f358
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_dyn_arr/foo.adb
@@ -0,0 +1,24 @@
+--  Copyright 2014 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/>.
+
+with Pck; use Pck;
+procedure Foo is
+   --  The goal here is to have an array whose bounds are not
+   --  known at compile time.
+   BT : Bounded := New_Bounded (Low => 1, High => 3);
+begin
+   Do_Nothing (BT'Address);  -- STOP
+end Foo;
+
diff --git a/gdb/testsuite/gdb.ada/mi_dyn_arr/pck.adb b/gdb/testsuite/gdb.ada/mi_dyn_arr/pck.adb
new file mode 100644
index 0000000..c77c31e
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_dyn_arr/pck.adb
@@ -0,0 +1,30 @@
+--  Copyright 2014 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/>.
+
+package body Pck is
+   function New_Bounded (Low, High : Integer) return Bounded is
+      Result : Bounded (Low .. High);
+   begin
+      for J in Low .. High loop
+         Result (J) := J;
+      end loop;
+      return Result;
+   end New_Bounded;
+
+   procedure Do_Nothing (A : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/mi_dyn_arr/pck.ads b/gdb/testsuite/gdb.ada/mi_dyn_arr/pck.ads
new file mode 100644
index 0000000..2519fdd
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_dyn_arr/pck.ads
@@ -0,0 +1,21 @@
+--  Copyright 2014 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/>.
+
+with System;
+package Pck is
+   type Bounded is array (Integer range <>) of Integer;
+   function New_Bounded (Low, High : Integer) return Bounded;
+   procedure Do_Nothing (A : System.Address);
+end Pck;
diff --git a/gdb/varobj.c b/gdb/varobj.c
index bbe4213..10ef5b7 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1649,7 +1649,16 @@ varobj_value_has_mutated (struct varobj *var, struct value *new_value,
     return 0;
 
   if (var->root->lang_ops->value_has_mutated)
-    return var->root->lang_ops->value_has_mutated (var, new_value, new_type);
+    {
+      /* The varobj module, when installing new values, explicitly strips
+	 references, saying that we're not interested in those addresses.
+	 But detection of mutation happens before installing the new
+	 value, so our value may be a reference that we need to strip
+	 in order to remain consistent.  */
+      if (new_value != NULL)
+	new_value = coerce_ref (new_value);
+      return var->root->lang_ops->value_has_mutated (var, new_value, new_type);
+    }
   else
     return 0;
 }
-- 
1.8.3.2

pushed: [varobj] false type-changed status for reference to Ada array

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * varobj.c (varobj_value_has_mutated): If NEW_VALUE is
>         a reference, strip the reference layer before calling
>         the lang_ops value_has_mutated callback.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.ada/mi_dyn_arr: New testcase.
> 

For the record, I just pushed this patch.

Thanks :)
-- 
Joel