[RFA/commit] error/internal-error printing local variable during "bt full".

[Joel Brobecker <brobecker at adacore dot com>]

One of our users reported an internal error using the "bt full"
command. In their situation, reproducing involved the following
scenario:

    (gdb) frame 1
    (gdb) bt full
    #0  0xf7783430 in __kernel_vsyscall ()
    No symbol table info available.
    #1  0xf5550aeb in waitpid () at ../sysdeps/unix/syscall-template.S:81
    No locals.
    [...]
    #6  0x0fe83139 in xxxx (arg=...)
    [...some locals printed, and then...]
    <S17b> =
    [...]/dwarf2loc.c:364: internal-error: dwarf_expr_frame_base: Assertion
    `framefunc != NULL' failed.

As shown above, the error happens while GDB is trying to print the value
of <S17b>, which is a local string internally generated by the compiler.
For that, it finds that the array lives in memory, and therefore tries
to create a struct value for it via:

        case DWARF_VALUE_MEMORY:
          {
            CORE_ADDR address = dwarf_expr_fetch_address (ctx, 0);
            [...]
            retval = value_at_lazy (type, address + byte_offset);

Unfortunately for us, TYPE happens to be an array whose bounds
are dynamic. More precisely, the bounds of our arrays are described
in the debugging info as being...

 <4><2c1985e>: Abbrev Number: 33 (DW_TAG_subrange_type)
    <2c1985f>   DW_AT_type        : <0x2c1989c>
    <2c19863>   DW_AT_lower_bound : <0x2c19835>
    <2c19867>   DW_AT_upper_bound : <0x2c19841>

... which are references to a pair of local variables. For instance,
the lower bound is a reference to the following DIE

 <3><2c19835>: Abbrev Number: 32 (DW_TAG_variable)
    <2c19836>   DW_AT_name        : [...]
    <2c1983a>   DW_AT_type        : <0x2c198b4>
    <2c1983e>   DW_AT_artificial  : 1
    <2c1983e>   DW_AT_location    : 2 byte block: 91 58         (DW_OP_fbreg: -40)

As a result of the above, value_at_lazy indirectly triggers
a resolution of TYPE (via value_from_contents_and_address),
which means a resolution of TYPE's bounds, and as seen in
the DW_AT_location attribute above for our bounds, computing
the bound's location requires the frame (its location expression
uses DW_OP_fbreg).

Unfortunately for us, value_at_lazy does not get passed a frame,
we've lost the relevant frame when we try to resolve the array's
bounds. Instead, resolve_dynamic_range gets calls dwarf2_evaluate_property
with NULL as the frame:

    static struct type *
    resolve_dynamic_range (struct type *dyn_range_type,
                           struct property_addr_info *addr_stack)
    {
      [...]
      if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
                                          ^^^^

... which then handles this by using the selected frame instead:

    if (frame == NULL && has_stack_frames ())
      frame = get_selected_frame (NULL);

In our case, the selected frame happens to be frame #1, which is
a frame where we have a minimal amount of debugging info, and in
particular, no debug info for the function itself. And because of that,
when we try to determine the frame's base...

    static void
    dwarf_expr_frame_base (void *baton, const gdb_byte **start,
                           size_t * length)
    {
      struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
      const struct block *bl = get_frame_block (debaton->frame, NULL);
      [...]
      framefunc = block_linkage_function (bl);

... framefunc ends up being NULL, which triggers the assert
in that same function:

      gdb_assert (framefunc != NULL);

This patches avoids the issue by temporarily setting the selected_frame
before printing the locals of each frames.

This patch also adds a small testcase, which reproduces the same
issue, but with a slightly different outcome:

    (gdb) bt full
    #0  0x000000000040049a in opaque_routine ()
    No symbol table info available.
    #1  0x0000000000400532 in main () at wrong_frame_bt_full-main.c:20
            my_table_size = 3
            my_table = <error reading variable my_table (frame address is not available.)>

With this patch, the output becomes:

    (gdb) bt full
    [...]
            my_table = {0, 1, 2}

gdb/ChangeLog:

        * stack.c (print_frame_local_vars): Temporarily set the selected
        frame to FRAME while printing the frame's local variables.

Thinking longer term, I think the best approach is to add a "frame"
parameter to value_at_lazy, and make sure it gets passed down all
the way to the type resolver.  But the number of locations that this
change impacts is quite large, so I just wanted to receive the OK from
the rest of the group before embarking on such a large change, knowing
that once done, the patch would ideally be pushed quickly to avoid
having to handle rebasing conflicts.

In the meantime, OK to push this patch? (tested on x86_64-linux)

Thanks,
-- 
Joel

---
 gdb/stack.c                                        | 25 ++++++++--
 gdb/testsuite/gdb.base/wrong_frame_bt_full-main.c  | 34 +++++++++++++
 .../gdb.base/wrong_frame_bt_full-opaque.c          | 22 +++++++++
 gdb/testsuite/gdb.base/wrong_frame_bt_full.exp     | 55 ++++++++++++++++++++++
 4 files changed, 133 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/wrong_frame_bt_full-main.c
 create mode 100644 gdb/testsuite/gdb.base/wrong_frame_bt_full-opaque.c
 create mode 100644 gdb/testsuite/gdb.base/wrong_frame_bt_full.exp

diff --git a/gdb/stack.c b/gdb/stack.c
index b825bdf..163b72d 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2082,6 +2082,7 @@ print_frame_local_vars (struct frame_info *frame, int num_tabs,
   struct print_variable_and_value_data cb_data;
   const struct block *block;
   CORE_ADDR pc;
+  struct gdb_exception except = exception_none;
 
   if (!get_frame_pc_if_available (frame, &pc))
     {
@@ -2102,9 +2103,27 @@ print_frame_local_vars (struct frame_info *frame, int num_tabs,
   cb_data.stream = stream;
   cb_data.values_printed = 0;
 
-  iterate_over_block_local_vars (block,
-				 do_print_variable_and_value,
-				 &cb_data);
+  /* Temporarily change the selected frame to the given FRAME.
+     This allows routines that rely on the selected frame instead
+     of being given a frame as parameter to use the correct frame.  */
+  select_frame (frame);
+
+  TRY
+    {
+      iterate_over_block_local_vars (block,
+				     do_print_variable_and_value,
+				     &cb_data);
+    }
+  CATCH (ex, RETURN_MASK_ALL)
+    {
+      except = ex;
+    }
+  END_CATCH
+
+  /* Restore the selected frame, and then rethrow if there was a problem.  */
+  select_frame (frame_find_by_id (cb_data.frame_id));
+  if (except.reason < 0)
+    throw_exception (except);
 
   /* do_print_variable_and_value invalidates FRAME.  */
   frame = NULL;
diff --git a/gdb/testsuite/gdb.base/wrong_frame_bt_full-main.c b/gdb/testsuite/gdb.base/wrong_frame_bt_full-main.c
new file mode 100644
index 0000000..73ab34c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/wrong_frame_bt_full-main.c
@@ -0,0 +1,34 @@
+/* Copyright (C) 2015 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/>.  */
+
+extern void opaque_routine (void);
+
+int dyn_arr_size = 4;
+
+int
+main (void)
+{
+  int i;
+  int my_table_size = dyn_arr_size - 1;
+  int my_table [my_table_size];
+
+  for (i = 0; i < my_table_size; i++)
+    my_table[i] = i;
+
+  opaque_routine ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/wrong_frame_bt_full-opaque.c b/gdb/testsuite/gdb.base/wrong_frame_bt_full-opaque.c
new file mode 100644
index 0000000..19b8770
--- /dev/null
+++ b/gdb/testsuite/gdb.base/wrong_frame_bt_full-opaque.c
@@ -0,0 +1,22 @@
+/* Copyright (C) 2015 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/>.  */
+
+void
+opaque_routine (void)
+{
+  /* Do nothing.  */
+}
diff --git a/gdb/testsuite/gdb.base/wrong_frame_bt_full.exp b/gdb/testsuite/gdb.base/wrong_frame_bt_full.exp
new file mode 100644
index 0000000..863cc1b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/wrong_frame_bt_full.exp
@@ -0,0 +1,55 @@
+# Copyright (C) 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/>.
+
+# Build wrong_frame_bt_full-main using two C files:
+#   - wrong_frame_bt_full-opaque.c, which needs to be built without
+#     debugging info;
+#   - wrong_frame_bt_full-main.c, which needs to be built with
+#     debugging info.
+# This is why we use gdb_compile instead of relying on he usual call
+# to prepare_for_testing.
+
+set main_testfile wrong_frame_bt_full-main
+set opaque_testfile wrong_frame_bt_full-opaque
+
+if {[gdb_compile "${srcdir}/${subdir}/$opaque_testfile.c" \
+                 $opaque_testfile.o \
+                 object {}] != ""} {
+  untested "failed to compile $opaque_testfile.c"
+  return -1
+}
+
+if {[gdb_compile \
+      [list ${srcdir}/${subdir}/$main_testfile.c $opaque_testfile.o] \
+      [standard_output_file ${main_testfile}] \
+      executable {debug}] != ""} {
+    untested "failed to build $main_testfile"
+    return -1
+}
+
+clean_restart ${main_testfile}
+
+if ![runto opaque_routine] {
+    untested "could not run to opaque_routine"
+    return -1
+}
+
+# Make sure that "bt full" command is capable of displaying MY_TABLE
+# correctly when frame #0 (the frame which does not have any debugging
+# info) is the selected frame.
+
+gdb_test "bt full" \
+         ".*\[\r\n\]+ *my_table = \\{0, 1, 2\\}\[\r\n\]+.*"
+
-- 
2.1.4