[PATCH] GDB/MI: crash printing "_task" (Ada) argument

[PATCH] GDB/MI: crash printing "_task" (Ada) argument

[Joel Brobecker]

Hello,

In GDB/MI mode, trying to print the arguments of the frame corresponding
to the body of a task ("-stack-list-arguments 1") causes the debugger to
crash.

This is because the compiler adds an implicit argument to that task body
called "_task". mi/mi-cmd-stack.c:list_args_or_locals, which is
responsible for printing the value of our arguments, finds that our
"_task" symbol is an argument, and thus tries to fing the non-argument
equivalent:

              if (SYMBOL_IS_ARGUMENT (sym))
                sym2 = lookup_symbol (SYMBOL_NATURAL_NAME (sym),
                                      block, VAR_DOMAIN,
                                      (int *) NULL);

Unfortunately, it tries using the natural name instead of the search
name when doing the lookup.  The correct name to use for symbol lookups
is the search name. In C/C++, it's the same, but not in Ada.  In Ada,
"_task"'s natural name is "<_task>", and that symbol does not exist,
so sym2 is NULL, thus causing the crash a little later on when trying
to dereference it.

gdb/ChangeLog:

        * mi/mi-cmd-stack.c (list_args_or_locals): For argument symbols,
        use SYMBOL_SEARCH_NAME to find the corresponding non-argument
        symbol.  Add assertion that sym2 is never NULL.

gdb/testsuite/ChangeLog:

        gdb.ada/mi_task_arg: New testcase.

Tested on x86_64-linux, no regression.  I will commit in a few days
unless there are objections.

For the record, I also audited the rest of the code, searching for
lookups obviously using SYMBOL_NATURAL_NAME, but did not find any.

Thanks,
-- 
Joel

---
 gdb/mi/mi-cmd-stack.c                             |    3 +-
 gdb/testsuite/gdb.ada/mi_task_arg.exp             |   51 +++++++++++++++
 gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb |   70 +++++++++++++++++++++
 3 files changed, 123 insertions(+), 1 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/mi_task_arg.exp
 create mode 100644 gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index c0c17bc..b414844 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -389,11 +389,12 @@ list_args_or_locals (enum what_to_list what, enum print_values values,
 	      struct frame_arg arg, entryarg;
 
 	      if (SYMBOL_IS_ARGUMENT (sym))
-		sym2 = lookup_symbol (SYMBOL_NATURAL_NAME (sym),
+		sym2 = lookup_symbol (SYMBOL_SEARCH_NAME (sym),
 				      block, VAR_DOMAIN,
 				      (int *) NULL);
 	      else
 		sym2 = sym;
+	      gdb_assert (sym2 != NULL);
 
 	      memset (&arg, 0, sizeof (arg));
 	      arg.sym = sym2;
diff --git a/gdb/testsuite/gdb.ada/mi_task_arg.exp b/gdb/testsuite/gdb.ada/mi_task_arg.exp
new file mode 100644
index 0000000..a8cc59b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_task_arg.exp
@@ -0,0 +1,51 @@
+# Copyright 2011-2012 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"
+
+set testdir "mi_task_arg"
+set testfile "${testdir}/task_switch"
+set srcfile ${srcdir}/${subdir}/${testfile}.adb
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug additional_flags=-gnata ]] != "" } {
+  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_runto "task_switch.break_me"] then {
+   fail "Cannot run to main, testcase aborted"
+   return 0
+}
+
+# Verify that "-stack-list-arguments" does not cause the debugger to
+# crash when printing the arguments of frame 1 (due to the internally-
+# generated argument "_task").
+mi_gdb_test "-stack-list-arguments 1" \
+            "\\^done,stack-args=\\\[frame=\{level=\"0\",args=\\\[\\\]\},frame=\{level=\"1\",args=\\\[\{name=\"<_task>\",value=\"$hex\"\}\\\]\},frame=\{level=\"2\",args=\\\[\\\]\}.*" \
+            "-stack-list-arguments 1"
+
diff --git a/gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb b/gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb
new file mode 100644
index 0000000..8ee1a2d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb
@@ -0,0 +1,70 @@
+--  Copyright 2011-2012 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 Task_Switch is
+
+   -------------------
+   -- Declaractions --
+   -------------------
+
+   task type Callee is
+      entry Finito;
+   end Callee;
+   type Callee_Ptr is access Callee;
+
+   task type Caller is
+   end Caller;
+   type Caller_Ptr is access Caller;
+
+   procedure Break_Me;
+
+   My_Caller : Caller_Ptr;
+   My_Callee : Callee_Ptr;
+
+   ------------
+   -- Bodies --
+   ------------
+
+   task body Callee is
+   begin
+      --  Just wait until we are told to terminate this task.
+      --  This is just to maintain this task alive.
+      accept Finito do
+         null;
+      end Finito;
+   end Callee;
+
+   task body Caller is
+   begin
+      Break_Me;
+      My_Callee.Finito;
+   end Caller;
+
+   procedure Break_Me is
+   begin
+      null;
+   end Break_Me;
+
+begin
+
+   --  Make sure to create the Callee task first... And then give it
+   --  enough time to complete its activation phase before we start
+   --  the Caller task.
+   My_Callee := new Callee;
+   delay 0.1;
+
+   My_Caller := new Caller;
+
+end Task_Switch;
-- 
1.7.1

Re: [PATCH] GDB/MI: crash printing "_task" (Ada) argument

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>               if (SYMBOL_IS_ARGUMENT (sym))
Joel>                 sym2 = lookup_symbol (SYMBOL_NATURAL_NAME (sym),
Joel>                                       block, VAR_DOMAIN,
Joel>                                       (int *) NULL);

FWIW the correct version is in stack.c:iterate_over_block_arg_vars.

It would be nice not to have separate code in MI.  I didn't look to see
why it is done this way.

Joel> Tested on x86_64-linux, no regression.  I will commit in a few days
Joel> unless there are objections.

I agree it is correct.

Tom

Re: [PATCH] GDB/MI: crash printing "_task" (Ada) argument

[Tom Tromey]

Joel> -		sym2 = lookup_symbol (SYMBOL_NATURAL_NAME (sym),
Joel> +		sym2 = lookup_symbol (SYMBOL_SEARCH_NAME (sym),

Oops.  Actually, I looked closer, and iterate_over_block_arg_vars uses
SYMBOL_LINKAGE_NAME.

Either that is a latent bug, or I think your patch should follow it in
the same of consistency.

Tom

Re: [PATCH] GDB/MI: crash printing "_task" (Ada) argument

[Joel Brobecker]

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

> Joel> -		sym2 = lookup_symbol (SYMBOL_NATURAL_NAME (sym),
> Joel> +		sym2 = lookup_symbol (SYMBOL_SEARCH_NAME (sym),
> 
> Oops.  Actually, I looked closer, and iterate_over_block_arg_vars uses
> SYMBOL_LINKAGE_NAME.
> 
> Either that is a latent bug, or I think your patch should follow it in
> the same of consistency.

Ah, yes, indeed. SYMBOL_LINKAGE_NAME is even better, I think, as it
requires no name transformation, and we're sure to get an exact match.

This is what I checked in (after re-testing). Thanks for the review!

-- 
Joel



[-- Attachment #2: mi-ada-tasks-v2.diff --]
[-- Type: text/x-diff, Size: 5994 bytes --]

commit 5780a060decabfb1b33e04118b283c2a23545261
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Mon Jan 30 09:14:39 2012 +0400

    gdb/ChangeLog:
    
            * mi/mi-cmd-stack.c (list_args_or_locals): For argument symbols,
            use SYMBOL_LINKAGE_NAME to find the corresponding non-argument
            symbol.  Add assertion that sym2 is never NULL.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.ada/mi_task_arg: New testcase.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 871278f..a2151e5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2012-02-03  Joel Brobecker  <brobecker@adacore.com>
+
+	* mi/mi-cmd-stack.c (list_args_or_locals): For argument symbols,
+	use SYMBOL_LINKAGE_NAME to find the corresponding non-argument
+	symbol.  Add assertion that sym2 is never NULL.
+
 2012-02-02  Doug Evans  <dje@google.com>
 
 	* blockframe.c (find_pc_partial_function_gnu_ifunc): Change type of
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index c0c17bc..4ad9f7c 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -389,11 +389,12 @@ list_args_or_locals (enum what_to_list what, enum print_values values,
 	      struct frame_arg arg, entryarg;
 
 	      if (SYMBOL_IS_ARGUMENT (sym))
-		sym2 = lookup_symbol (SYMBOL_NATURAL_NAME (sym),
+		sym2 = lookup_symbol (SYMBOL_LINKAGE_NAME (sym),
 				      block, VAR_DOMAIN,
 				      (int *) NULL);
 	      else
 		sym2 = sym;
+	      gdb_assert (sym2 != NULL);
 
 	      memset (&arg, 0, sizeof (arg));
 	      arg.sym = sym2;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 143dae1..cbaca89 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2012-02-03  Joel Brobecker  <brobecker@adacore.com>
+
+	* gdb.ada/mi_task_arg: New testcase.
+
 2012-02-02  Pedro Alves  <palves@redhat.com>
 
 	* gdb.reverse/until-precsave.exp: Also put "record save" under the
diff --git a/gdb/testsuite/gdb.ada/mi_task_arg.exp b/gdb/testsuite/gdb.ada/mi_task_arg.exp
new file mode 100644
index 0000000..a8cc59b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_task_arg.exp
@@ -0,0 +1,51 @@
+# Copyright 2011-2012 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"
+
+set testdir "mi_task_arg"
+set testfile "${testdir}/task_switch"
+set srcfile ${srcdir}/${subdir}/${testfile}.adb
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug additional_flags=-gnata ]] != "" } {
+  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_runto "task_switch.break_me"] then {
+   fail "Cannot run to main, testcase aborted"
+   return 0
+}
+
+# Verify that "-stack-list-arguments" does not cause the debugger to
+# crash when printing the arguments of frame 1 (due to the internally-
+# generated argument "_task").
+mi_gdb_test "-stack-list-arguments 1" \
+            "\\^done,stack-args=\\\[frame=\{level=\"0\",args=\\\[\\\]\},frame=\{level=\"1\",args=\\\[\{name=\"<_task>\",value=\"$hex\"\}\\\]\},frame=\{level=\"2\",args=\\\[\\\]\}.*" \
+            "-stack-list-arguments 1"
+
diff --git a/gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb b/gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb
new file mode 100644
index 0000000..8ee1a2d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/mi_task_arg/task_switch.adb
@@ -0,0 +1,70 @@
+--  Copyright 2011-2012 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 Task_Switch is
+
+   -------------------
+   -- Declaractions --
+   -------------------
+
+   task type Callee is
+      entry Finito;
+   end Callee;
+   type Callee_Ptr is access Callee;
+
+   task type Caller is
+   end Caller;
+   type Caller_Ptr is access Caller;
+
+   procedure Break_Me;
+
+   My_Caller : Caller_Ptr;
+   My_Callee : Callee_Ptr;
+
+   ------------
+   -- Bodies --
+   ------------
+
+   task body Callee is
+   begin
+      --  Just wait until we are told to terminate this task.
+      --  This is just to maintain this task alive.
+      accept Finito do
+         null;
+      end Finito;
+   end Callee;
+
+   task body Caller is
+   begin
+      Break_Me;
+      My_Callee.Finito;
+   end Caller;
+
+   procedure Break_Me is
+   begin
+      null;
+   end Break_Me;
+
+begin
+
+   --  Make sure to create the Callee task first... And then give it
+   --  enough time to complete its activation phase before we start
+   --  the Caller task.
+   My_Callee := new Callee;
+   delay 0.1;
+
+   My_Caller := new Caller;
+
+end Task_Switch;

Re: [PATCH] GDB/MI: crash printing "_task" (Ada) argument

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

> +# Verify that "-stack-list-arguments" does not cause the debugger to
> +# crash when printing the arguments of frame 1 (due to the internally-
> +# generated argument "_task").
> +mi_gdb_test "-stack-list-arguments 1" \
> +            "\\^done,stack-args=\\\[frame=\{level=\"0\",args=\\\[\\\]\},frame=\{level=\"1\",args=\\\[\{name=\"<_task>\",value=\"$hex\"\}\\\]\},frame=\{level=\"2\",args=\\\[\\\]\}.*" \
> +            "-stack-list-arguments 1"
> +

Expecting: ^(-stack-list-arguments 1[
]+)?(\^done,stack-args=\[frame={level="0",args=\[\]},frame={level="1",args=\[{name="<_task>",value="0x[0-9A-Fa-f]+"}\]},frame={level="2",args=\[\]}.*[
]+[(]gdb[)] 
[ ]*)
-stack-list-arguments 1
^done,stack-args=[frame={level="0",args=[]},frame={level="1",args=[{name="<_task>",value="0x1005a9b8"}]},frame={level="2",args=[{name="self_id",value="0x1005a9c8"}]},frame={level="3",args=[]},frame={level="4",args=[]}]
(gdb) 
FAIL: gdb.ada/mi_task_arg.exp: -stack-list-arguments 1

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."