Don't let TUI exceptions escape to readline (PR tui/9765)

  On 1/10/20 12:53 PM, Pedro Alves wrote:

> I didn't delve deep into the patch, but, I should point out one
> thing -- as described in the PR, it's a problem to let exceptions
> cross ncurses.  Any kind of C++ exception.  So which ncurses callback/entry
> point in gdb were we at?  We need to look into it and make sure that
> no exceptions are thrown from it back into ncurses.  Above, you're rethrowing
> non-memory exceptions, which is what made me wonder, since it sounds like
> for  example a Ctrl-C at some "wrong" time may bring down GDB.
> For readline, we ended up with TRY_SJLJ/CATCH_SJLJ.


There's actually a backtrace in the PR.  And I can still (*) reproduce it.
Here's the current backtrace I get:

(top-gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff4633d31 in __GI_abort () at abort.c:79
#2  0x00007ffff4c5d2a5 in __gnu_cxx::__verbose_terminate_handler () at ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
#3  0x00007ffff4c5ae96 in __cxxabiv1::__terminate (handler=<optimized out>) at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:47
#4  0x00007ffff4c59d99 in __cxa_call_terminate (During symbol reading, Child DIE 0x2cee6 and its abstract origin 0x5a17 have different parents.
ue_header=ue_header@entry=0x1a011a0) at ../../../../libstdc++-v3/libsupc++/eh_call.cc:54
#5  0x00007ffff4c5a788 in __cxxabiv1::__gxx_personality_v0 (version=<optimized out>, actions=<optimized out>, exception_class=5138137972254386944, ue_header=0x1a011a0, context=<optimized out>) at ../../../../libstdc++-v3/libsupc++/eh_personality.cc:676
#6  0x00007ffff49c3f33 in _Unwind_RaiseException_Phase2 (exc=exc@entry=0x1a011a0, context=context@entry=0x7fffffffcfa0) at ../../../libgcc/unwind.inc:62
#7  0x00007ffff49c475e in _Unwind_Resume (exc=0x1a011a0) at ../../../libgcc/unwind.inc:230
#8  0x000000000093313e in tui_disasm_window::set_contents (this=0x1c00f10, arch=0x1a3f2a0, sal=...) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-disasm.c:227
#9  0x000000000094c8fd in tui_source_window_base::update_source_window_as_is (this=0x1c00f10, gdbarch=0x1a3f2a0, sal=...) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-winsource.c:184
#10 0x0000000000933428 in tui_disasm_window::do_scroll_vertical (this=0x1c00f10, num_to_scroll=13) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-disasm.c:337
#11 0x000000000094a322 in tui_win_info::forward_scroll (this=0x1c00f10, num_to_scroll=12) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-win.c:476
#12 0x000000000093deac in tui_dispatch_ctrl_char (ch=338) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-io.c:921
#13 0x000000000093e05c in tui_getc (fp=0x7ffff49ae9e0 <_IO_2_1_stdin_>) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-io.c:1005
#14 0x00000000009dbedd in rl_read_key () at /home/pedro/gdb/binutils-gdb/src/readline/readline/input.c:495
#15 0x00000000009be41c in readline_internal_char () at /home/pedro/gdb/binutils-gdb/src/readline/readline/readline.c:573
#16 0x00000000009dca7b in rl_callback_read_char () at /home/pedro/gdb/binutils-gdb/src/readline/readline/callback.c:262
...

The issue is actually crossing readline here, tui_getc -> rl_read_key, not ncurses.

* - eh, I was the one who filed this, I forgot, lol.

I think we should add this patch, in addition to your fix, or something
like it.

From abb77826ee2ea565282d675d5c82a98e55601c41 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 10 Jan 2020 13:32:07 +0000
Subject: [PATCH] Don't let TUI exceptions escape to readline (PR tui/9765)

PR tui/9765 shows a use case where GDB dies from an uncaught exception,
here:

(top-gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff4633d31 in __GI_abort () at abort.c:79
#2  0x00007ffff4c5d2a5 in __gnu_cxx::__verbose_terminate_handler () at ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
#3  0x00007ffff4c5ae96 in __cxxabiv1::__terminate (handler=<optimized out>) at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:47
#4  0x00007ffff4c59d99 in __cxa_call_terminate (During symbol reading, Child DIE 0x2cee6 and its abstract origin 0x5a17 have different parents.
ue_header=ue_header@entry=0x1a011a0) at ../../../../libstdc++-v3/libsupc++/eh_call.cc:54
#5  0x00007ffff4c5a788 in __cxxabiv1::__gxx_personality_v0 (version=<optimized out>, actions=<optimized out>, exception_class=5138137972254386944, ue_header=0x1a011a0, context=<optimized out>) at ../../../../libstdc++-v3/libsupc++/eh_personality.cc:676
#6  0x00007ffff49c3f33 in _Unwind_RaiseException_Phase2 (exc=exc@entry=0x1a011a0, context=context@entry=0x7fffffffcfa0) at ../../../libgcc/unwind.inc:62
#7  0x00007ffff49c475e in _Unwind_Resume (exc=0x1a011a0) at ../../../libgcc/unwind.inc:230
#8  0x000000000093313e in tui_disasm_window::set_contents (this=0x1c00f10, arch=0x1a3f2a0, sal=...) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-disasm.c:227
#9  0x000000000094c8fd in tui_source_window_base::update_source_window_as_is (this=0x1c00f10, gdbarch=0x1a3f2a0, sal=...) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-winsource.c:184
#10 0x0000000000933428 in tui_disasm_window::do_scroll_vertical (this=0x1c00f10, num_to_scroll=13) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-disasm.c:337
#11 0x000000000094a322 in tui_win_info::forward_scroll (this=0x1c00f10, num_to_scroll=12) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-win.c:476
#12 0x000000000093deac in tui_dispatch_ctrl_char (ch=338) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-io.c:921
#13 0x000000000093e05c in tui_getc (fp=0x7ffff49ae9e0 <_IO_2_1_stdin_>) at /home/pedro/gdb/binutils-gdb/src/gdb/tui/tui-io.c:1005
#14 0x00000000009dbedd in rl_read_key () at /home/pedro/gdb/binutils-gdb/src/readline/readline/input.c:495
#15 0x00000000009be41c in readline_internal_char () at /home/pedro/gdb/binutils-gdb/src/readline/readline/readline.c:573
#16 0x00000000009dca7b in rl_callback_read_char () at /home/pedro/gdb/binutils-gdb/src/readline/readline/callback.c:262
...

This is triggered by simply scrolling off the end of the dissasembly
window.  This commit doesn't fix the actual exception that is being
thrown, which will still need to be fixed, but makes sure that we
don't ever throw an exception out to readline.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR tui/9765
	* tui/tui-io.c (tui_getc): Rename to ...
	(tui_getc_1): ... this.
	(tui_get): New, reimplent as try/catch wrapper around tui_getc_1.
---
 gdb/tui/tui-io.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)


base-commit: ffebb0bbde7deae978ab3e4d3d3d90acf52b7d69
  

This patch must be in as well! It takes care of the "call stack 2"
from the other e-mail.

Cheers,
--
Shahab
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> gdb/ChangeLog:
Pedro> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

Pedro> 	PR tui/9765
Pedro> 	* tui/tui-io.c (tui_getc): Rename to ...
Pedro> 	(tui_getc_1): ... this.
Pedro> 	(tui_get): New, reimplent as try/catch wrapper around tui_getc_1.
Pedro> ---
Pedro>  gdb/tui/tui-io.c | 31 ++++++++++++++++++++++++++++---
Pedro>  1 file changed, 28 insertions(+), 3 deletions(-)

This seems like a good idea measure to me.
Ideally I suppose these TUI functions shouldn't throw.  However, in gdb
it can be difficult to ensure that, so this provides some defense.

thanks,
Tom
  

Patch #1 is Pedro's patch to stop exceptions trying to cross readline
which was posted elsewhere in this thread and really should be
included before its lost.

Patch #2 is a reworking of assembler window scrolling that allows it
to handle trying to disassemble invalid memory, and also makes the
whole scrolling experience more robust (I believe).

--

Andrew Burgess (1):
  gdb/tui: asm window handles invalid memory and scrolls better

Pedro Alves (1):
  gdb/tui: Prevent exceptions from trying to cross readline

 gdb/ChangeLog                            |  12 ++
 gdb/testsuite/ChangeLog                  |   4 +
 gdb/testsuite/gdb.tui/tui-layout-asm.exp |  41 ++++++
 gdb/tui/tui-disasm.c                     | 216 +++++++++++++++++++++++--------
 gdb/tui/tui-io.c                         |  31 ++++-
 5 files changed, 245 insertions(+), 59 deletions(-)
  

Hello Andrew,

I have tested the patch series against the "hello world"
program that I have [1]. Here comes my observations:

1. As you can see in this gif file:
https://sourceware.org/bugzilla/attachment.cgi?id=12200
"Going up" sometimes does not work. In this case, it is
between "main" and "_start" (garbage?). Nevertheless, I
manged to have it working for me by:

   @@ -161,7 +161,7 @@ tui_find_symbol_backward (CORE_ADDR addr)
 {
   struct bound_minimal_symbol msym;
 
-  for (int offset = 1; offset <= 1024; offset *= 2)
+  for (int offset = 1; offset <= 1024; ++offset)
     {
       CORE_ADDR tmp = addr - offset;
       msym = lookup_minimal_symbol_by_pc_section (tmp, 0);

2. I run into a problem that "page-up" does not work when we
reach the end of file:
https://sourceware.org/bugzilla/attachment.cgi?id=12201
The following fixes that, but breaks elsewhere (see 3):

@@ -232,14 +232,14 @@ tui_find_disassembly_address ...
          /* Disassemble forward a few lines and see ...
          next_addr = tui_disassemble (gdbarch, asm_lines, ...
          last_addr = asm_lines.back ().addr;
-         if (last_addr > pc && msymbol.minsym != nullptr
+         if (last_addr >= pc && msymbol.minsym != nullptr
              && asm_lines.size () >= max_lines)
            {
              /* This will do if we can't find anything... */
              possible_new_low.found = true;
              possible_new_low.new_low = new_low;
            }
-       } while (last_addr > pc && msymbol.minsym != nullptr);
+       } while (last_addr >= pc && msymbol.minsym != nullptr);

3. With the changes from above, "arrow up" stops working near
beginning of the file:
https://sourceware.org/bugzilla/attachment.cgi?id=12202

I can summarise what I have learned in the following table:

,-------------.----------.----------------------------------.
| usecase     | action   | condition that works             |
|-------------+----------+----------------------------------|
| very bottom | page-up  | as long as last_addr >= pc ...,  |
|             |          | go higher (try reducing new_low) |
|-------------+----------+----------------------------------|
| second line | arrow up | as long as last_addr > pc ...,   |
|             |          | go higher (try reducing new_low) |
`-------------^----------^----------------------------------'

This table portrays contradictory conditions for corner case
scenarios. I believe the real solution in the end should be
much simpler and ideally as such that it covers the corner
cases naturally. I will try to cook something up in my free
time.

I suggest that this patch series should be in along with
the changes proposed at point 1. So we only end-up with
none-working page-up at the end of assembly output.


Cheers,
Shahab


[1]
that x86_64 elf program is archived here:
https://sourceware.org/bugzilla/attachment.cgi?id=12199
  

This revision addresses the issues raised by Shahab, as well as making
the improvements Tom pointed out.

I looked at changing the TERM type from ansi to xterm as Tom
suggested, but figuring out all of the extra control sequences that
are sent was taking too much effort.  I might try to revisit this when
I have more time, but I don't plan to do this in the immediate future.

I did start adding a mechanism to try and detect when the user tries
to scroll and we're already at the end of the output (or the
beginning), and this helped in the scroll down case, but I still need
to figure out how to use this in the scroll up case, so for now I've
not included this work in this patch set.

There's still some things I think could be improved with the assembler
scrolling - the user is currenly "trapped" inside the continuous
memory region that the $pc starts in, they can't scroll to any
disjoint code region, but this never worked before either, so this
isn't a regression.  I do have an idea for how to fix this, but I'm
hoping to merge this set first, and work on the multi-section support
when I can find some time later.

Comments/feedback welcome as always,

Thanks,
Andrew

---

Andrew Burgess (1):
  gdb/tui: asm window handles invalid memory and scrolls better

Pedro Alves (1):
  gdb/tui: Prevent exceptions from trying to cross readline

 gdb/ChangeLog                            |  17 +++
 gdb/minsyms.c                            |  41 ++++--
 gdb/minsyms.h                            |  17 ++-
 gdb/testsuite/ChangeLog                  |   4 +
 gdb/testsuite/gdb.tui/tui-layout-asm.exp |  41 ++++++
 gdb/tui/tui-disasm.c                     | 243 +++++++++++++++++++++++--------
 gdb/tui/tui-io.c                         |  31 +++-
 7 files changed, 313 insertions(+), 81 deletions(-)
  

These patches are in now:

gdb/tui: Prevent exceptions from trying to cross readline
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2f267673f0fdee9287e6d404ecd4f2d29da0d2f2

gdb/tui: asm window handles invalid memory and scrolls better
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=733d0a679536628eb1be4b4b8aa6384de24ff1f1

However, for the record, I must add that I have found one failing
case. That happens when you have a small program that fits in one
window, then you do a page down and reach the end of it. Now, the
page up does not work anymore.

A sample usecase is attached for your amusement. In case you do
not like attachments, just save the base64 code below ...


H4sIAAAAAAAAA+2WbW/bNhDH/db8FAclgGOgsiU/FgI2oEtSb0Aeijh9MSxFQUm0rJUWDZJqZBT9
7jtK8kPWtDXSwu0w/l5YJvk/HsnTiScy5nKqtMuFUMxlWezqPHWpWrgqkoLzNEu6jW/DQ8bDoXn6
46FXPft+2V/T8Psjvzcc+P6o38DRnjdowPAb/e5FrjSVAA01p3MafknHpDrEgg6L2Cf+N+cvzi7P
n+zDBHg0GHw2/sNxbx1/b2Ti38P3BePvfcd9fpb/efxnqVQa9BzDrKBsBITGdGn6GDiRyGZp4oBi
kU5FBmlW9i/oOzZLOSNEsqUUcR6hPSjNliogvwKL5gKc4w+n1xevL6+mH4vjDxd/XJ1PPzoE4LlX
9AYoMnOQkyQO2/AXLCVTCpY0YW4s7jMQWcTgE95s9Jm4LxdSWuRLiAVT2KnhXsh3QLPVQkiGehKz
ME8Sszq0hFQrxmdB7R13Jd8z+dDHkdkjZgVoJhdpRvlaXM4Ej4gp+p3jNBsDEqeKKsUWocmecp0h
DsjVxjEKyI+OfMVe+U8X8WjQefLb/7X873v9Ov/7Y3/QM/nf93yb/4eg2Ukzzfhbtco0LTCFMBNn
aUGaHc0KjY+Ei5A335pT0qR6BKRZCNlktHgG+HPwFgA2gcWmGRY/SSL9R9kr/9ef+yf6+Er++wO/
t63/hmOT/2NvaPP/ENyeT2/NTRb8AuVXnpAjqC59Mjn7Deqh7pLqeVeLLt5lXbxIu3jpJ5IujFoL
wRWZvLq+ua3Efq8/IJOXFy8m07LtsgJaimnYXosrd8bpe0zi8uPTqiScrkSuAV+9FplMK/va/M75
kv2dU4u2M9w5hFDOA3PrExLmKY8DOD4xu213FGken5yetsGNtn3gJn1wxaZDGNHFWducwKavPI8d
ESuwBEIXAZQujAmeWhvHq/23HyqPQOaZWVJdeJw4+N+B+5RzCJmpTnBDMyZTIduk0mxm3loFOLs5
7dJN7W36uLvdKuffZc3RblnzWBlTjuz6X/tbRwuUjFp7h9dIqMQq8/Hl1ipMxgSFki2EZtuttsqi
arMYEf4d54sluDG4l5WXh1vvvPr9+urPACLOaEZI+QhMeG4u2zvh3LX50ZlosVgsFovFYrFYLBaL
xWKxWCwWi8XyffgH4PT6ZQAoAAA=

... and then: base64 -d <saved_code_from_email> > lingering-bug.tgz

Last but not least, I cannot care less about this now, but I wanted it
documented.

Cheers,
Shahab
  

Shahab,

Thanks for reporting this regression.  Here's a fix.  I'll give this a
few days for feedback, then push it.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb/tui: Update help text for scroll commands
  gdb/tui: Disassembler scrolling of very small programs

 gdb/ChangeLog                                      | 12 +++++
 gdb/testsuite/ChangeLog                            |  6 +++
 gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S  | 22 ++++++++++
 .../gdb.tui/tui-layout-asm-short-prog.exp          | 51 ++++++++++++++++++++++
 gdb/tui/tui-disasm.c                               |  2 +-
 gdb/tui/tui-win.c                                  | 16 +++++--
 6 files changed, 104 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S
 create mode 100644 gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp