Re: [PATCH] Fix assertion fi->level

[Mark Williams <mark@myosotissp.com>]

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

The problem seems to be that jit_frame_prev_register returns a value
with lval == not_lval, so the value returned from
get_frame_register_value ends up with lval == not_lval, rather than
lval == lval_register.

This is hidden by value_of_register_lazy/value_fetch_lazy, because
value_of_register_lazy creates a value like this:

  reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
  VALUE_LVAL (reg_val) = lval_register;
  VALUE_REGNUM (reg_val) = regnum;

and then value_fetch_lazy gets a new_val with lval==not_lval, but then
copies its contents into the reg_val created by value_of_register_lazy
without overwriting the lval field.

This seems wrong... if an unwinder says its returning a value thats
not an lval, it doesn't seem correct for value_fetch_lazy to turn it
into one. On the other hand, it does seem to work as an lval, so the
bug is probably in jit_frame_prev_register? I'm just not familiar
enough with the code to know what the correct fix is...

In any case, the simple fix for my patch is to call
value_of_register_lazy/value_fetch_lazy when there *is* a valid
frame_id, and get_frame_register_value when there isn't. That way it
only changes the behavior when it was going to crash anyway. I'll go
with that unless someone has a better suggestion...

New patch attached.

On Fri, Feb 7, 2020 at 1:08 PM Mark Williams <mark@myosotissp.com> wrote:
>
> Thanks, I'll take a look.
>
> On Fri, Feb 7, 2020 at 1:05 PM Tom Tromey <tom@tromey.com> wrote:
> >
> > >>>>> "Mark" == Mark Williams <mark@myosotissp.com> writes:
> >
> > Mark> Apparently I should ping the thread after 2 weeks with no response...
> >
> > Yeah.
> >
> > Mark> Note that python unwinders are completely unusable in gdb-8.1 and
> > Mark> later without this fix...
> >
> > Thanks.
> >
> > I read the patch, and I think it looks reasonable.  However, since we
> > were considering it for gdb 9 in another thread, I applied it and ran
> > the test suite.  It caused a few regressions, like:
> >
> > gdb.base/jit-reader.exp: with jit-reader: after mangling: caller frame: cannot a
> > ssign to register: PASS => FAIL
> > gdb.mi/mi-reg-undefined.exp: register values, format r, frame 1: PASS => FAIL
> > gdb.mi/mi-reg-undefined.exp: register values, format r, frame 2: PASS => FAIL
> > gdb.mi/mi-reg-undefined.exp: register values, format x, frame 1: PASS => FAIL
> > gdb.mi/mi-reg-undefined.exp: register values, format x, frame 2: PASS => FAIL
> >
> >
> > Here's some stuff from gdb.log:
> >
> > me: print pseudo register
> > print $rbp = -1
> > Left operand of assignment is not an lvalue.
> > (gdb) FAIL: gdb.base/jit-reader.exp: with jit-reader: after mangling: caller frame: cannot assign to register
> >
> >
> > 221-data-list-register-values --thread 1 --frame 1 x  0 1 2 8 9
> > ~"../../binutils-gdb/gdb/value.c:389: internal-error: int value_bits_any_optimiz
> > ed_out(const value*, int, int): Assertion `!value->lazy' failed.\nA problem inte
> > rnal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit th
> > is debugging session? (y or n) "
> > FAIL: gdb.mi/mi-reg-undefined.exp: register values, format x, frame 1 (timeout)
> >
> >
> > I didn't investigate any further but there's still some issue to be
> > worked out.
> >
> > thanks,
> > Tom

[-- Attachment #2: 0001-gdb-Fix-Assertion-fi-level-0-failed.patch --]
[-- Type: application/octet-stream, Size: 5777 bytes --]

From 2604c15697ecb25e39fa7b049193bfdc263c40cc Mon Sep 17 00:00:00 2001
From: mwilliams <mwilliams@fb.com>
Date: Sun, 9 Feb 2020 10:12:24 -0800
Subject: [PATCH] gdb: Fix "Assertion 'fi->level == 0' failed."

At the point that python unwinders are called to determine if they
want to handle a frame, calling value_of_register_lazy can fail in
the caller of an inlined function, because the inlined function's
frame id has not yet been computed.

However, there's no need to call value_of_register_lazy because the
very next statement is value_fetch_lazy, and the two calls together
are identical to a call to get_frame_register_value, except that the
latter doesn't need a frame id.

An existing test case already stopped in an inlined function and
inspected various frames, so just add a dummy unwinder to it. The
presence of the dummy unwinder should not affect the existing test,
because all it does is tell gdb that it does not want to handle
the current frame. But without this fix, gdb crashes, breaking the
test.

gdb/Changelog
2020-02-09 Mark Williams <mark@myosotissp.com>

	PR gdb/22748
	* findvar.c call get_frame_register_value instead of
	value_of_register_lazy/value_fetch_lazy when there is
	no frame id.
	* frame.c add has_frame_id implementation
        * frame.h add has_frame_id declaration

gdb/testsuite/Changelog
2020-02-09 Mark Williams <mark@myosotissp.com>

	PR gdb/22748
	* gdb.python/py-frame-inline.exp modified to load
	the new dummy unwinder to expose the bug
	* gdb.python/py-frame-inline.py new file, implementing
	a dummy unwinder.
---
 gdb/findvar.c                                |  9 ++++++--
 gdb/frame.c                                  |  6 +++++
 gdb/frame.h                                  |  1 +
 gdb/testsuite/gdb.python/py-frame-inline.exp |  4 ++++
 gdb/testsuite/gdb.python/py-frame-inline.py  | 24 ++++++++++++++++++++
 5 files changed, 42 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-frame-inline.py

diff --git a/gdb/findvar.c b/gdb/findvar.c
index a836c63dc5..12d4e0361c 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -264,14 +264,19 @@ value_of_register (int regnum, struct frame_info *frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct value *reg_val;
+  struct frame_info *next_frame;
 
   /* User registers lie completely outside of the range of normal
      registers.  Catch them early so that the target never sees them.  */
   if (regnum >= gdbarch_num_cooked_regs (gdbarch))
     return value_of_user_reg (regnum, frame);
 
-  reg_val = value_of_register_lazy (frame, regnum);
-  value_fetch_lazy (reg_val);
+  next_frame = get_next_frame_sentinel_okay (frame);
+  if (has_frame_id(next_frame)) {
+    reg_val = value_of_register_lazy(frame, regnum);
+    value_fetch_lazy(reg_val);
+  } else
+    reg_val = get_frame_register_value (frame, regnum);
   return reg_val;
 }
 
diff --git a/gdb/frame.c b/gdb/frame.c
index d74d1d5c7c..34789e56cc 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -558,6 +558,12 @@ compute_frame_id (struct frame_info *fi)
     }
 }
 
+bool
+has_frame_id (struct frame_info *fi)
+{
+  return fi->this_id.p;
+}
+
 /* Return a frame uniq ID that can be used to, later, re-find the
    frame.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index cfc15022ed..0a4d748df7 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -478,6 +478,7 @@ extern CORE_ADDR get_frame_base (struct frame_info *);
    if (frame_id_eq (id, get_frame_id (r)))
 
    instead, since that avoids the bug.  */
+extern bool            has_frame_id (struct frame_info *fi);
 extern struct frame_id get_frame_id (struct frame_info *fi);
 extern struct frame_id get_stack_frame_id (struct frame_info *fi);
 extern struct frame_id frame_unwind_caller_id (struct frame_info *next_frame);
diff --git a/gdb/testsuite/gdb.python/py-frame-inline.exp b/gdb/testsuite/gdb.python/py-frame-inline.exp
index 71bffd375d..488346e651 100644
--- a/gdb/testsuite/gdb.python/py-frame-inline.exp
+++ b/gdb/testsuite/gdb.python/py-frame-inline.exp
@@ -24,6 +24,10 @@ if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
 # Skip all tests if Python scripting is not enabled.
 if { [skip_python_tests] } { continue }
 
+set remote_python_file [gdb_remote_download host \
+			    ${srcdir}/${subdir}/${testfile}.py]
+gdb_test_no_output "source ${remote_python_file}" "load python file"
+
 if ![runto main] then {
     fail "can't run to function f"
     return 0
diff --git a/gdb/testsuite/gdb.python/py-frame-inline.py b/gdb/testsuite/gdb.python/py-frame-inline.py
new file mode 100644
index 0000000000..56be6d3523
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-frame-inline.py
@@ -0,0 +1,24 @@
+# Copyright (C) 2020 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/>.
+
+# This file is part of the GDB testsuite.
+from gdb.unwinder import Unwinder, register_unwinder
+class DummyUnwinder(Unwinder):
+    def __init__(self):
+        super(DummyUnwinder, self).__init__('dummy_unwinder')
+    def __call__(self, pending_frame):
+        fp = pending_frame.read_register('pc')
+        return None
+register_unwinder(None, DummyUnwinder())
-- 
2.17.1