[PATCH] Fix assertion fi->level

[PATCH] Fix assertion fi->level

[Mark Williams]

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

This is a fix for https://sourceware.org/bugzilla/show_bug.cgi?id=22748.

It avoids calling value_of_register_lazy, since that requires a frame
id, and there may not be one yet for inlined frames.

Mark

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

From 28221e7158acb4b3129e57fcb8bbf262b8d4ab3d Mon Sep 17 00:00:00 2001
From: mwilliams <mwilliams@fb.com>
Date: Tue, 7 Jan 2020 07:02:31 -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
because the next frame's id has not yet been computed (if its inline).

Rather than calling value_of_register_lazy, then immediately calling
value_fetch_lazy, just call get_frame_register_value directly.

gdb/Changelog
2020-01-18 Mark Williams <mark@myosotissp.com>

	PR gdb/22748
	* findvar.c call get_frame_register_value instead of
	value_of_register_lazy/value_fetch_lazy so it works
	without a frame id.
---
 gdb/findvar.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 5cf1cd4137..0cddebc12b 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -263,16 +263,13 @@ struct value *
 value_of_register (int regnum, struct frame_info *frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  struct value *reg_val;
 
   /* 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);
-  return reg_val;
+  return get_frame_register_value (frame, regnum);
 }
 
 /* Return a `value' with the contents of (virtual or cooked) register
-- 
2.17.1

Re: [PATCH] Fix assertion fi->level

[Mark Williams]

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

Add a test case, and more detail to the commit message

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

From ee4c0dafb5495c1492709cc606a0542771315fe2 Mon Sep 17 00:00:00 2001
From: mwilliams <mwilliams@fb.com>
Date: Sun, 19 Jan 2020 09:14:12 -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-01-19 Mark Williams <mark@myosotissp.com>

	PR gdb/22748
	* findvar.c call get_frame_register_value instead of
	value_of_register_lazy/value_fetch_lazy so it works
	without a frame id.

gdb/testsuite/Changelog
2020-01-19 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                                |  5 +---
 gdb/testsuite/gdb.python/py-frame-inline.exp |  4 ++++
 gdb/testsuite/gdb.python/py-frame-inline.py  | 24 ++++++++++++++++++++
 3 files changed, 29 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-frame-inline.py

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 5cf1cd4137..0cddebc12b 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -263,16 +263,13 @@ struct value *
 value_of_register (int regnum, struct frame_info *frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  struct value *reg_val;
 
   /* 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);
-  return reg_val;
+  return get_frame_register_value (frame, regnum);
 }
 
 /* Return a `value' with the contents of (virtual or cooked) register
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

Re: [PATCH] Fix assertion fi->level

[Mark Williams]

Apparently I should ping the thread after 2 weeks with no response...

Note that python unwinders are completely unusable in gdb-8.1 and
later without this fix...

On Sun, Jan 19, 2020 at 9:20 AM Mark Williams <mark@myosotissp.com> wrote:
>
> Add a test case, and more detail to the commit message

Re: [PATCH] Fix assertion fi->level

[Tom Tromey]

>>>>> "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

Re: [PATCH] Fix assertion fi->level

[Mark Williams]

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

Re: [PATCH] Fix assertion fi->level

[Mark Williams]

[-- 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