[PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file

Shahab Vahedi
From: Shahab Vahedi <[hidden email]>

In TUI mode, when the assembly layout reaches the end of a binary,
GDB wants to disassemle the addresses beyond the last valid ones.
This results in a "MEMORY_ERROR" exception to be thrown when
tui_disasm_window::set_contents() invokes tui_disassemble(). When
that happens set_contents() bails out prematurely without filling
the "content" for the valid addresses. This eventually leads to
no assembly lines or termination of GDB when you scroll down to
the last lines of the program.

With this change, tui_disassemble() catches MEMORY_ERROR exceptions
and ignores them, while filling the rest of "asm_lines" with the
same address (the one just beyond the last PC address).

The issue has been discussed at length in bug 25345 (and 9765).

gdb/ChangeLog:
2020-01-10  Shahab Vahedi  <[hidden email]>

        PR tui/25345
        * tui/tui-disasm.c (tui_disasm_window::tui_disassemble):
        Handle MEMORY_ERROR exceptions gracefully.
---
The behavior of GDB after this fix is illustrated here:
https://sourceware.org/bugzilla/attachment.cgi?id=12178

 gdb/tui/tui-disasm.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 98c691f3387..dffcd257a0d 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -114,7 +114,19 @@ tui_disassemble (struct gdbarch *gdbarch,
   asm_lines[pos + i].addr_size = new_size;
  }
 
-      pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
+      try
+ {
+  pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
+ }
+      catch (const gdb_exception &except)
+ {
+  /* In cases where max_lines is asking tui_disassemble() to fetch
+     too much, like when PC goes past the valid address range, a
+     MEMORY_ERROR is thrown, but it is alright.  */
+  if (except.error != MEMORY_ERROR)
+    throw;
+  /* fall through: let asm_lines still to be filled.  */
+ }
 
       asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
 
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file

Pedro Alves-7
On 1/10/20 11:57 AM, Shahab Vahedi wrote:

> From: Shahab Vahedi <[hidden email]>
>
> In TUI mode, when the assembly layout reaches the end of a binary,
> GDB wants to disassemle the addresses beyond the last valid ones.
> This results in a "MEMORY_ERROR" exception to be thrown when
> tui_disasm_window::set_contents() invokes tui_disassemble(). When
> that happens set_contents() bails out prematurely without filling
> the "content" for the valid addresses. This eventually leads to
> no assembly lines or termination of GDB when you scroll down to
> the last lines of the program.
>
> With this change, tui_disassemble() catches MEMORY_ERROR exceptions
> and ignores them, while filling the rest of "asm_lines" with the
> same address (the one just beyond the last PC address).
>
> The issue has been discussed at length in bug 25345 (and 9765).
>
> gdb/ChangeLog:
> 2020-01-10  Shahab Vahedi  <[hidden email]>
>
> PR tui/25345
> * tui/tui-disasm.c (tui_disasm_window::tui_disassemble):
> Handle MEMORY_ERROR exceptions gracefully.
> ---
> The behavior of GDB after this fix is illustrated here:
> https://sourceware.org/bugzilla/attachment.cgi?id=12178
>
>  gdb/tui/tui-disasm.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
> index 98c691f3387..dffcd257a0d 100644
> --- a/gdb/tui/tui-disasm.c
> +++ b/gdb/tui/tui-disasm.c
> @@ -114,7 +114,19 @@ tui_disassemble (struct gdbarch *gdbarch,
>    asm_lines[pos + i].addr_size = new_size;
>   }
>  
> -      pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
> +      try
> + {
> +  pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
> + }
> +      catch (const gdb_exception &except)
> + {
> +  /* In cases where max_lines is asking tui_disassemble() to fetch
> +     too much, like when PC goes past the valid address range, a
> +     MEMORY_ERROR is thrown, but it is alright.  */
> +  if (except.error != MEMORY_ERROR)
> +    throw;
> +  /* fall through: let asm_lines still to be filled.  */
> + }
>  

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.

>        asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
>  
>

Thanks,
Pedro Alves

Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
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 <[hidden email]>
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  <[hidden email]>

        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(-)

diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 9cb41104fe9..d9f23334f57 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -950,10 +950,12 @@ tui_dispatch_ctrl_char (unsigned int ch)
   return 0;
 }
 
-/* Get a character from the command window.  This is called from the
-   readline package.  */
+/* Main worker for tui_getc.  Get a character from the command window.
+   This is called from the readline package, but wrapped in a
+   try/catch by tui_getc.  */
+
 static int
-tui_getc (FILE *fp)
+tui_getc_1 (FILE *fp)
 {
   int ch;
   WINDOW *w;
@@ -1036,6 +1038,29 @@ tui_getc (FILE *fp)
   return ch;
 }
 
+/* Get a character from the command window.  This is called from the
+   readline package.  */
+
+static int
+tui_getc (FILE *fp)
+{
+  try
+    {
+      return tui_getc_1 (fp);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Just in case, don't ever let an exception escape to readline.
+ This shouldn't ever happen, but if it does, print the
+ exception instead of just crashing GDB.  */
+      exception_print (gdb_stderr, ex);
+
+      /* If we threw an exception, it's because we recognized the
+ character.  */
+      return 0;
+    }
+}
+
 /* See tui-io.h.  */
 
 gdb::unique_xmalloc_ptr<char>

base-commit: ffebb0bbde7deae978ab3e4d3d3d90acf52b7d69
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file

Shahab Vahedi
In reply to this post by Pedro Alves-7
On Fri, Jan 10, 2020 at 12:53:17PM +0000, 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

I have found two different call stacks with this exception. There can be
more.

Call stack 1:
  #0  tui_disassemble (...) at gdb/tui/tui-disasm.c:126
  #1  tui_disasm_window::set_contents (...) at gdb/tui/tui-disasm.c:241
  #2  tui_source_window_base::update_source_window_as_is (...) at
        gdb/tui/tui-winsource.c:184
  #3  tui_source_window_base::update_source_window (...) at
        gdb/tui/tui-winsource.c:173
  #4  tui_update_source_windows_with_addr (...) at
        gdb/tui/tui-winsource.c:207
  #5  tui_set_layout (layout_type=DISASSEM_COMMAND) at
        gdb/tui/tui-layout.c:181
  #6  tui_layout_command (layout_name="asm", from_tty=1) at
        gdb/tui/tui-layout.c:287
  #7  do_const_cfunc (c=, args="asm", from_tty=1) at
        gdb/cli/cli-decode.c:107
  #8  cmd_func (cmd=, args="asm", from_tty=1) at
        gdb/cli/cli-decode.c:1952
  #9  execute_command (p="m", from_tty=1) at gdb/top.c:653
  #10 catch_command_errors (...) at gdb/main.c:401
  #11 captured_main_1 (context=) at gdb/main.c:1168
  #12 captured_main (data=) at gdb/main.c:1193
  #13 gdb_main (args=) at gdb/main.c:1218
  #14 main (argc=4, argv=) at gdb/gdb.c:32

call stack 2:
#0  tui_disassemble (...)
    at gdb/tui/tui-disasm.c:126
#1  tui_find_disassembly_address (...)
    at gdb/tui/tui-disasm.c:157
#2  tui_disasm_window::do_scroll_vertical (this=, num_to_scroll=32)
    at gdb/tui/tui-disasm.c:348
#3  tui_win_info::forward_scroll (this=, num_to_scroll=31)
    at gdb/tui/tui-win.c:476
#4  tui_dispatch_ctrl_char (ch=338) at gdb/tui/tui-io.c:921
#5  tui_getc (fp=<_IO_2_1_stdin_>) at gdb/tui/tui-io.c:1005
#6  rl_read_key () at readline/readline/input.c:495
#7  readline_internal_char () at readline/readline/readline.c:573
#8  rl_callback_read_char () at readline/readline/callback.c:262
#9  gdb_rl_callback_read_char_wrapper_noexcept ()
    at gdb/event-top.c:176
#10 gdb_rl_callback_read_char_wrapper (client_data=)
    at gdb/event-top.c:193
#11 stdin_event_handler (error=0, client_data=) at gdb/event-top.c:515
#12 handle_file_event (file_ptr=, ready_mask=1)
    at gdb/event-loop.c:731
#13 gdb_wait_for_event (block=1) at gdb/event-loop.c:857
#14 gdb_do_one_event () at gdb/event-loop.c:346
#15 start_event_loop () at gdb/event-loop.c:370
#16 captured_command_loop () at gdb/main.c:360
#17 captured_main (data=) at gdb/main.c:1203
#18 gdb_main (args=) at gdb/main.c:1218
#19 main (argc=4, argv=) at gdb/gdb.c:32

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

For "call stack 1", other exceptions end up here:
gdb/main.c:
 catch_command_errors (...)
 {
   try
     {
       ...
     }
   catch (const gdb_exception &e)
     {
       return handle_command_errors (e);
     }
   ...
 }

"call stack 2" is doomed. Probably in do_scroll_vertical () it aborts.

>
>
> Thanks,
> Pedro Alves
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't let TUI exceptions escape to readline (PR tui/9765)

Shahab Vahedi
In reply to this post by Pedro Alves-7
This patch must be in as well! It takes care of the "call stack 2"
from the other e-mail.

Cheers,
--
Shahab
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't let TUI exceptions escape to readline (PR tui/9765)

Tom Tromey-2
In reply to this post by Pedro Alves-7
>>>>> "Pedro" == Pedro Alves <[hidden email]> writes:

Pedro> gdb/ChangeLog:
Pedro> yyyy-mm-dd  Pedro Alves  <[hidden email]>

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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2][PR tui/9765] Fix segfault in asm TUI when reaching end of file

Andrew Burgess
In reply to this post by Shahab Vahedi
Shahab,

I took a look at you patch last night, and there were a couple of
things I thought should be tweaked.  I wanted to remove the trailing
addresses with no content, and ideally have the window stop scrolling
at the last valid instruction - much like the source window does with
its content.  I also saw in a test here that I couldn't scroll back to
the first instruction (test file was single asm file containing 35
1-byte nop instructions).

I initially intended to just suggest how you might do that, but in
trying to figure out what to suggest I ended up with the patch below.

This still needs some more work - I was initially testing this with an
asm file containing ~35 nop instructions, and just scrolling that up
and down, and this worked fine.

But then I took a look at:
  https://sourceware.org/bugzilla/show_bug.cgi?id=9765
and tried scrolling up and down in a helloworld binary.  For some
reason on _that_ test the scrolling seems to get "stuck" at the end,
and when I use Page-Up to scroll up I see the test get "stuck"
alternating between two addresses and not actually making progress at
scrolling up.

In summary, I think the patch below would be a good starting point,
but it needs some more work to fix the last few nits.  I'll take a
look at this tomorrow or Sunday.

[ NOTE: I've been using Pedro's patch to prevent exceptions entering
  readline in my tree too. ]

Feedback welcome,

Thanks,
Andrew

---

commit 354f0a9f7a9c0edfb862d43656c21119fe9007e2
Author: Andrew Burgess <[hidden email]>
Date:   Sat Jan 11 01:38:28 2020 +0000

    gdb/tui: Make asm window handle reading invalid memory
   
    When scrolling the asm window, if we try to disassemble invalid memory
    this should not prevent the window displaying something sane for those
    addresses that can be disassembled.x
   
    gdb/ChangeLog:
   
            * tui/tui-disasm.c (tui_disassemble): Update header comment,
            remove unneeded parameter, add try/catch around gdb_print_insn,
            rewrite to add items to asm_lines vector.
            (tui_find_disassembly_address): Don't assume the length of
            asm_lines after calling tui_disassemble, update for changes in
            tui_disassemble API.
            (tui_disasm_window::set_contents): Likewise.
   
    Change-Id: I292f4b8d571516ca8b25d9d60c85228c98222086

diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 98c691f3387..eeecaec43bf 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -81,13 +81,21 @@ len_without_escapes (const std::string &str)
   return len;
 }
 
-/* Function to set the disassembly window's content.
-   Disassemble count lines starting at pc.
-   Return address of the count'th instruction after pc.  */
+/* Function to disassemble up to COUNT instructions starting from address
+   PC into the ASM_LINES vector.  Return the address of the count'th
+   instruction after pc.  When ADDR_SIZE is non-null then place the
+   maximum size of an address and label into the value pointed to by
+   ADDR_SIZE, and set the addr_size field on each item in ASM_LINES,
+   otherwise the addr_size fields within asm_lines are undefined.
+
+   It is worth noting that ASM_LINES might not have COUNT entries when
+   this function returns.  If the disassembly is truncated for some other
+   reason, for example, we hit invalid memory, then ASM_LINES can have
+   fewer entries than requested.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
  std::vector<tui_asm_line> &asm_lines,
- CORE_ADDR pc, int pos, int count,
+ CORE_ADDR pc, int count,
  size_t *addr_size = nullptr)
 {
   bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
@@ -96,10 +104,31 @@ tui_disassemble (struct gdbarch *gdbarch,
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
     {
-      print_address (gdbarch, pc, &gdb_dis_out);
-      asm_lines[pos + i].addr = pc;
-      asm_lines[pos + i].addr_string = std::move (gdb_dis_out.string ());
+      tui_asm_line tal;
+      CORE_ADDR orig_pc = pc;
 
+      try
+ {
+  pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
+ }
+      catch (const gdb_exception &except)
+ {
+  /* If pc points to an invalid address then we'll catch a
+     MEMORY_ERROR here, this should stop the disassembly, but
+     otherwise is fine.  */
+  if (except.error != MEMORY_ERROR)
+    throw;
+  return pc;
+ }
+
+      /* Capture the disassembled instruction.  */
+      tal.insn = std::move (gdb_dis_out.string ());
+      gdb_dis_out.clear ();
+
+      /* And capture the address the instruction is at.  */
+      tal.addr = orig_pc;
+      print_address (gdbarch, orig_pc, &gdb_dis_out);
+      tal.addr_string = std::move (gdb_dis_out.string ());
       gdb_dis_out.clear ();
 
       if (addr_size != nullptr)
@@ -107,19 +136,14 @@ tui_disassemble (struct gdbarch *gdbarch,
   size_t new_size;
 
   if (term_out)
-    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+    new_size = len_without_escapes (tal.addr_string);
   else
-    new_size = asm_lines[pos + i].addr_string.size ();
+    new_size = tal.addr_string.size ();
   *addr_size = std::max (*addr_size, new_size);
-  asm_lines[pos + i].addr_size = new_size;
+  tal.addr_size = new_size;
  }
 
-      pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
-
-      asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
-
-      /* Reset the buffer to empty.  */
-      gdb_dis_out.clear ();
+      asm_lines.push_back (tal);
     }
   return pc;
 }
@@ -137,41 +161,54 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
   if (max_lines <= 1)
     return pc;
 
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
 
   new_low = pc;
   if (from > 0)
     {
-      tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-      new_low = asm_lines[max_lines - 1].addr;
+      tui_disassemble (gdbarch, asm_lines, pc, max_lines);
+      new_low = asm_lines.back ().addr;
     }
   else
     {
       CORE_ADDR last_addr;
-      int pos;
       struct bound_minimal_symbol msymbol;
 
       /* Find backward an address which is a symbol and for which
          disassembling from that address will fill completely the
          window.  */
-      pos = max_lines - 1;
-      do {
-         new_low -= 1 * max_lines;
-         msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0);
+      do
+ {
+  /* We want to move back at least one byte.  */
+  new_low -= 1;
+
+  /* If there's a symbol covering this address then jump back to
+     the address of this symbol.  */
+  msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0);
+  if (msymbol.minsym)
+    new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
+  else
+    new_low += 1;
+
+  /* Disassemble forward a few lines and see where we got to.  */
+  tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+  last_addr = asm_lines.back ().addr;
 
-         if (msymbol.minsym)
-            new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
-         else
-            new_low += 1 * max_lines;
+ } while (last_addr >= pc && msymbol.minsym != nullptr);
 
-         tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines);
-         last_addr = asm_lines[pos].addr;
-      } while (last_addr > pc && msymbol.minsym);
+      /* If we failed to disassemble the required number of lines then the
+ following walk forward is not going to work.  And what would it
+ mean to try and walk forward through memory we know can't be
+ disassembled?  Just return the original address which should
+ indicate we can't move backward in this case.  */
+      if (asm_lines.size () < max_lines)
+ return pc;
 
       /* Scan forward disassembling one instruction at a time until
          the last visible instruction of the window matches the pc.
          We keep the disassembled instructions in the 'lines' window
          and shift it downward (increasing its addresses).  */
+      int pos = max_lines - 1;
       if (last_addr < pc)
         do
           {
@@ -181,12 +218,15 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
             if (pos >= max_lines)
               pos = 0;
 
-            next_addr = tui_disassemble (gdbarch, asm_lines,
- last_addr, pos, 1);
+    std::vector<tui_asm_line> single_asm_line;
+    next_addr = tui_disassemble (gdbarch, single_asm_line,
+ last_addr, 1);
 
             /* If there are some problems while disassembling exit.  */
             if (next_addr <= last_addr)
               break;
+    gdb_assert (single_asm_line.size () == 1);
+    asm_lines[pos] = single_asm_line[0];
             last_addr = next_addr;
           } while (last_addr <= pc);
       pos++;
@@ -224,9 +264,9 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
   line_width = width - TUI_EXECINFO_SIZE - 2;
 
   /* Get temporary table that will hold all strings (addr & insn).  */
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
   size_t addr_size = 0;
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
+  tui_disassemble (gdbarch, asm_lines, pc, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -237,17 +277,29 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
     {
       tui_source_element *src = &content[i];
 
-      std::string line
- = (asm_lines[i].addr_string
-   + n_spaces (insn_pos - asm_lines[i].addr_size)
-   + asm_lines[i].insn);
+      std::string line;
+      CORE_ADDR addr;
+
+      if (i < asm_lines.size ())
+ {
+  line
+    = (asm_lines[i].addr_string
+       + n_spaces (insn_pos - asm_lines[i].addr_size)
+       + asm_lines[i].insn);
+  addr = asm_lines[i].addr;
+ }
+      else
+ {
+  line = "";
+  addr = 0;
+ }
 
       const char *ptr = line.c_str ();
       src->line = tui_copy_source_line (&ptr, -1, offset, line_width, 0);
 
       src->line_or_addr.loa = LOA_ADDRESS;
-      src->line_or_addr.u.addr = asm_lines[i].addr;
-      src->is_exec_point = asm_lines[i].addr == cur_pc;
+      src->line_or_addr.u.addr = addr;
+      src->is_exec_point = (addr == cur_pc && line.size () > 0);
     }
   return true;
 }
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] gdb/tui: Assembler window scrolling fixes

Andrew Burgess
In reply to this post by Shahab Vahedi
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(-)

--
2.14.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] gdb/tui: Prevent exceptions from trying to cross readline

Andrew Burgess
From: Pedro Alves <[hidden email]>

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  <[hidden email]>

        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.

Change-Id: I2e32a401ab34404b2132ec82a3e1c17b9b723e41
---
 gdb/tui/tui-io.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 9cb41104fe9..d9f23334f57 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -950,10 +950,12 @@ tui_dispatch_ctrl_char (unsigned int ch)
   return 0;
 }
 
-/* Get a character from the command window.  This is called from the
-   readline package.  */
+/* Main worker for tui_getc.  Get a character from the command window.
+   This is called from the readline package, but wrapped in a
+   try/catch by tui_getc.  */
+
 static int
-tui_getc (FILE *fp)
+tui_getc_1 (FILE *fp)
 {
   int ch;
   WINDOW *w;
@@ -1036,6 +1038,29 @@ tui_getc (FILE *fp)
   return ch;
 }
 
+/* Get a character from the command window.  This is called from the
+   readline package.  */
+
+static int
+tui_getc (FILE *fp)
+{
+  try
+    {
+      return tui_getc_1 (fp);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Just in case, don't ever let an exception escape to readline.
+ This shouldn't ever happen, but if it does, print the
+ exception instead of just crashing GDB.  */
+      exception_print (gdb_stderr, ex);
+
+      /* If we threw an exception, it's because we recognized the
+ character.  */
+      return 0;
+    }
+}
+
 /* See tui-io.h.  */
 
 gdb::unique_xmalloc_ptr<char>
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Andrew Burgess
In reply to this post by Andrew Burgess
This started as a patch to enable the asm window to handle attempts to
disassemble invalid memory, but it ended up expanding into a
significant rewrite of how the asm window handles scrolling.  These
two things ended up being tied together as it was impossible to
correctly test scrolling into invalid memory when the asm window would
randomly behave weirdly while scrolling.

Things that should work nicely now, scroll to the bottom or top of the
listing with PageUp, PageDown, Up Arrow, Down Arrow and we should be
able to scroll past small areas of memory that don't have symbols
associated with them now.

Adding tests for this scrolling was a little bit of a problem.  First
I would have liked to add tests for PageUp / PageDown, but the tuiterm
library we use doesn't support these commands.

Next, I would have liked to test scrolling to the start or end of the
assembler listing and then trying to scroll even more, however, this
is a problem because in a well behaving GDB a scroll at the start/end
has no effect.  What we need to do is:

  - Move to start of assembler listing,
  - Send scroll up command,
  - Wait for all curses output,
  - Ensure the assembler listing is unchanged, we're still at the
    start of the listing.

The problem is that there is no curses output, so how long do we wait
at step 3?  The same problem exists for scrolling to the bottom of the
assembler listing.  However, when scrolling down you can at least see
the end coming, so I added a test for this case, however, this feels
like an area of code that is massively undertested.

gdb/ChangeLog:

        * tui/tui-disasm.c (tui_disassemble): Update header comment,
        remove unneeded parameter, add try/catch around gdb_print_insn,
        rewrite to add items to asm_lines vector.
        (tui_find_symbol_backward): New function.
        (tui_find_disassembly_address): Updated throughout.
        (tui_disasm_window::set_contents): Update for changes to
        tui_disassemble.
        (tui_disasm_window::do_scroll_vertical): No need to adjust the
        number of lines to scroll.

gdb/testsuite/ChangeLog:

        * gdb.tui/tui-layout-asm.exp: Add scrolling test for asm window.

Change-Id: I2114f6cca5cd89fb8ef5dfdacd5f751248c461e0
---
 gdb/ChangeLog                            |  12 ++
 gdb/testsuite/ChangeLog                  |   4 +
 gdb/testsuite/gdb.tui/tui-layout-asm.exp |  41 ++++++
 gdb/tui/tui-disasm.c                     | 216 +++++++++++++++++++++++--------
 4 files changed, 217 insertions(+), 56 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index cec2735764e..f78baab1081 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -32,3 +32,44 @@ if {![Term::prepare_for_tui]} {
 # This puts us into TUI mode, and should display the ASM window.
 Term::command "layout asm"
 Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
+
+# Scroll the ASM window down using the down arrow key.  In an ideal
+# world I'd like to use PageDown here, but currently our terminal
+# library doesn't support such advanced things.
+set testname "scroll to end of assembler"
+set down_count 0
+while (1) {
+    # Grab the second line, this is about to become the first line.
+    set line [Term::get_line 2]
+
+    # Except, if the second line is blank then we are at the end of
+    # the available asm output.  Pressing down again _shouldn't_
+    # change the output, however, if GDB is working, and we press down
+    # then the screen wont change, so the call to Term::wait_for below
+    # will just timeout.  So for now we avoid testing the edge case.
+    if {[regexp -- "^\\| +\\|$" $line]} {
+ # Second line is blank, we're at the end of the assembler.
+ pass $testname
+ break
+    }
+
+    # Send the down key to GDB.
+    send_gdb "\033\[B"
+    incr down_count
+    if {[Term::wait_for [string_to_regexp $line]] \
+    && [Term::get_line 1] == $line} {
+ # We scrolled successfully.
+    } else {
+ fail "$testname (scroll failed)"
+ Term::dump_screen
+ break
+    }
+
+    if { $down_count > 250 } {
+ # Maybe we should accept this as a pass in case a target
+ # really does have loads of assembler to scroll through.
+ fail "$testname (too much assembler)"
+ Term::dump_screen
+ break
+    }
+}
diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 98c691f3387..c3d66bf348b 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -81,25 +81,58 @@ len_without_escapes (const std::string &str)
   return len;
 }
 
-/* Function to set the disassembly window's content.
-   Disassemble count lines starting at pc.
-   Return address of the count'th instruction after pc.  */
+/* Function to disassemble up to COUNT instructions starting from address
+   PC into the ASM_LINES vector (which will be emptied of any previous
+   contents).  Return the address of the count'th instruction after pc.
+   When ADDR_SIZE is non-null then place the maximum size of an address and
+   label into the value pointed to by ADDR_SIZE, and set the addr_size
+   field on each item in ASM_LINES, otherwise the addr_size fields within
+   asm_lines are undefined.
+
+   It is worth noting that ASM_LINES might not have COUNT entries when this
+   function returns.  If the disassembly is truncated for some other
+   reason, for example, we hit invalid memory, then ASM_LINES can have
+   fewer entries than requested.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
  std::vector<tui_asm_line> &asm_lines,
- CORE_ADDR pc, int pos, int count,
+ CORE_ADDR pc, int count,
  size_t *addr_size = nullptr)
 {
   bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
   string_file gdb_dis_out (term_out);
 
+  /* Must start with an empty list.  */
+  asm_lines.clear ();
+
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
     {
-      print_address (gdbarch, pc, &gdb_dis_out);
-      asm_lines[pos + i].addr = pc;
-      asm_lines[pos + i].addr_string = std::move (gdb_dis_out.string ());
+      tui_asm_line tal;
+      CORE_ADDR orig_pc = pc;
 
+      try
+ {
+  pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
+ }
+      catch (const gdb_exception &except)
+ {
+  /* If pc points to an invalid address then we'll catch a
+     MEMORY_ERROR here, this should stop the disassembly, but
+     otherwise is fine.  */
+  if (except.error != MEMORY_ERROR)
+    throw;
+  return pc;
+ }
+
+      /* Capture the disassembled instruction.  */
+      tal.insn = std::move (gdb_dis_out.string ());
+      gdb_dis_out.clear ();
+
+      /* And capture the address the instruction is at.  */
+      tal.addr = orig_pc;
+      print_address (gdbarch, orig_pc, &gdb_dis_out);
+      tal.addr_string = std::move (gdb_dis_out.string ());
       gdb_dis_out.clear ();
 
       if (addr_size != nullptr)
@@ -107,21 +140,36 @@ tui_disassemble (struct gdbarch *gdbarch,
   size_t new_size;
 
   if (term_out)
-    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+    new_size = len_without_escapes (tal.addr_string);
   else
-    new_size = asm_lines[pos + i].addr_string.size ();
+    new_size = tal.addr_string.size ();
   *addr_size = std::max (*addr_size, new_size);
-  asm_lines[pos + i].addr_size = new_size;
+  tal.addr_size = new_size;
  }
 
-      pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
+      asm_lines.push_back (tal);
+    }
+  return pc;
+}
 
-      asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
+/* Look backward from ADDR for a bound_minimal_symbol.  If no symbol can be
+   found then return a null bound_minimal_symbol.  This is used when
+   disassembling backwards.  */
 
-      /* Reset the buffer to empty.  */
-      gdb_dis_out.clear ();
+static struct bound_minimal_symbol
+tui_find_symbol_backward (CORE_ADDR addr)
+{
+  struct bound_minimal_symbol msym;
+
+  for (int offset = 1; offset <= 1024; offset *= 2)
+    {
+      CORE_ADDR tmp = addr - offset;
+      msym = lookup_minimal_symbol_by_pc_section (tmp, 0);
+      if (msym.minsym)
+ return msym;
     }
-  return pc;
+
+  return { nullptr, nullptr };
 }
 
 /* Find the disassembly address that corresponds to FROM lines above
@@ -134,65 +182,113 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
   int max_lines;
 
   max_lines = (from > 0) ? from : - from;
-  if (max_lines <= 1)
+  if (max_lines == 0)
     return pc;
 
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
 
   new_low = pc;
   if (from > 0)
     {
-      tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-      new_low = asm_lines[max_lines - 1].addr;
+      /* Always disassemble 1 extra instruction here, then if the last
+ instruction fails to disassemble we will take the address of the
+ previous instruction that did disassemble as the result.  */
+      tui_disassemble (gdbarch, asm_lines, pc, max_lines + 1);
+      new_low = asm_lines.back ().addr;
     }
   else
     {
       CORE_ADDR last_addr;
-      int pos;
       struct bound_minimal_symbol msymbol;
 
+      max_lines++;
+
       /* Find backward an address which is a symbol and for which
-         disassembling from that address will fill completely the
-         window.  */
-      pos = max_lines - 1;
-      do {
-         new_low -= 1 * max_lines;
-         msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0);
-
-         if (msymbol.minsym)
-            new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
-         else
-            new_low += 1 * max_lines;
-
-         tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines);
-         last_addr = asm_lines[pos].addr;
-      } while (last_addr > pc && msymbol.minsym);
+ disassembling from that address will give us MAX_LINES of
+ disassembly.  The variable NEXT_ADDR will be set to the next
+ address to disassemble after the MAX_LINES of disassembly.  */
+      CORE_ADDR next_addr;
+
+      /* As we search backward if we find an address that looks promising
+ then we record it in this structure.  If the next address we try
+ is not suitable then we fall back to the previous good address.
+ Otherwise, if the next address is also good it gets recorded here
+ instead, and then we try the next address.  */
+      struct
+      {
+ bool found = false;
+ CORE_ADDR new_low;
+      } possible_new_low;
+
+      do
+ {
+  /* Search backward for a symbol, this allows us to find a good
+     spot to start disassembling from which should, we hope, be
+     the start of an instruction.  */
+  msymbol = tui_find_symbol_backward (new_low);
+  if (msymbol.minsym != nullptr)
+    new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
+
+  /* Disassemble forward a few lines and see where we got to.  */
+  next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+  last_addr = asm_lines.back ().addr;
+  if (last_addr > pc && msymbol.minsym != nullptr
+      && asm_lines.size () >= max_lines)
+    {
+      /* This will do if we can't find anything better.  */
+      possible_new_low.found = true;
+      possible_new_low.new_low = new_low;
+    }
+ } while (last_addr > pc && msymbol.minsym != nullptr);
+
+      /* If we failed to disassemble the required number of lines then the
+ following walk forward is not going to work, it assumes that
+ ASM_LINES contains exactly MAX_LINES entries.  Instead we should
+ consider falling back to a previous possible start address in
+ POSSIBLE_NEW_LOW.  */
+      if (asm_lines.size () < max_lines)
+ {
+  if (!possible_new_low.found)
+    return pc;
+
+  /* Take the best possible match we have.  */
+  new_low = possible_new_low.new_low;
+  next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+  last_addr = asm_lines.back ().addr;
+  gdb_assert (asm_lines.size () >= max_lines);
+ }
 
       /* Scan forward disassembling one instruction at a time until
          the last visible instruction of the window matches the pc.
          We keep the disassembled instructions in the 'lines' window
          and shift it downward (increasing its addresses).  */
+      int pos = max_lines - 1;
       if (last_addr < pc)
         do
           {
-            CORE_ADDR next_addr;
-
             pos++;
             if (pos >= max_lines)
               pos = 0;
 
-            next_addr = tui_disassemble (gdbarch, asm_lines,
- last_addr, pos, 1);
-
+    CORE_ADDR old_next_addr = next_addr;
+    std::vector<tui_asm_line> single_asm_line;
+    next_addr = tui_disassemble (gdbarch, single_asm_line,
+ next_addr, 1);
             /* If there are some problems while disassembling exit.  */
-            if (next_addr <= last_addr)
-              break;
-            last_addr = next_addr;
-          } while (last_addr <= pc);
+    if (next_addr <= old_next_addr)
+      return pc;
+    gdb_assert (single_asm_line.size () == 1);
+    asm_lines[pos] = single_asm_line[0];
+  } while (next_addr <= pc);
       pos++;
       if (pos >= max_lines)
          pos = 0;
       new_low = asm_lines[pos].addr;
+
+      /* When scrolling backward the addresses should move backward, or at
+ the very least stay the same if we are at the first address that
+ can be disassembled.  */
+      gdb_assert (new_low <= pc);
     }
   return new_low;
 }
@@ -224,9 +320,9 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
   line_width = width - TUI_EXECINFO_SIZE - 2;
 
   /* Get temporary table that will hold all strings (addr & insn).  */
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
   size_t addr_size = 0;
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
+  tui_disassemble (gdbarch, asm_lines, pc, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -237,17 +333,29 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
     {
       tui_source_element *src = &content[i];
 
-      std::string line
- = (asm_lines[i].addr_string
-   + n_spaces (insn_pos - asm_lines[i].addr_size)
-   + asm_lines[i].insn);
+      std::string line;
+      CORE_ADDR addr;
+
+      if (i < asm_lines.size ())
+ {
+  line
+    = (asm_lines[i].addr_string
+       + n_spaces (insn_pos - asm_lines[i].addr_size)
+       + asm_lines[i].insn);
+  addr = asm_lines[i].addr;
+ }
+      else
+ {
+  line = "";
+  addr = 0;
+ }
 
       const char *ptr = line.c_str ();
       src->line = tui_copy_source_line (&ptr, -1, offset, line_width, 0);
 
       src->line_or_addr.loa = LOA_ADDRESS;
-      src->line_or_addr.u.addr = asm_lines[i].addr;
-      src->is_exec_point = asm_lines[i].addr == cur_pc;
+      src->line_or_addr.u.addr = addr;
+      src->is_exec_point = (addr == cur_pc && line.size () > 0);
     }
   return true;
 }
@@ -326,10 +434,6 @@ tui_disasm_window::do_scroll_vertical (int num_to_scroll)
       CORE_ADDR pc;
 
       pc = start_line_or_addr.u.addr;
-      if (num_to_scroll >= 0)
- num_to_scroll++;
-      else
- --num_to_scroll;
 
       symtab_and_line sal {};
       sal.pspace = current_program_space;
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] gdb/tui: Assembler window scrolling fixes

Shahab Vahedi-2
In reply to this post by Andrew Burgess
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] gdb/tui: Prevent exceptions from trying to cross readline

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> writes:

Andrew> gdb/ChangeLog:
Andrew> yyyy-mm-dd  Pedro Alves  <[hidden email]>

Andrew>         PR tui/9765
Andrew>         * tui/tui-io.c (tui_getc): Rename to ...
Andrew>         (tui_getc_1): ... this.
Andrew>         (tui_get): New, reimplent as try/catch wrapper around tui_getc_1.

This still seems like a good idea to me.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> writes:

Andrew> Adding tests for this scrolling was a little bit of a problem.  First
Andrew> I would have liked to add tests for PageUp / PageDown, but the tuiterm
Andrew> library we use doesn't support these commands.

I wonder if setting TERM to xterm or vt100 would let this work without
too much effort.

Andrew> Next, I would have liked to test scrolling to the start or end of the
Andrew> assembler listing and then trying to scroll even more [...]

Andrew> The problem is that there is no curses output, so how long do we wait
Andrew> at step 3?

Resizing had the same problem (how to tell when the resize is finished),
so I added a special mode to the TUI for this.  So, if you really
wanted, in this case you could have the TUI debug mode print something
or ring the bell when scrolling isn't possible.

Andrew> +      asm_lines.push_back (tal);

This should probably use push_back (std::move (tal)), to avoid copying
the string.

Andrew> +      /* As we search backward if we find an address that looks promising
Andrew> + then we record it in this structure.  If the next address we try
Andrew> + is not suitable then we fall back to the previous good address.
Andrew> + Otherwise, if the next address is also good it gets recorded here
Andrew> + instead, and then we try the next address.  */
Andrew> +      struct
Andrew> +      {
Andrew> + bool found = false;
Andrew> + CORE_ADDR new_low;
Andrew> +      } possible_new_low;

This can be gdb::optional<CORE_ADDR>, which would seem clearer in this
case to me.


Aside from these nits, this seems fine to me.  I didn't try it, but if
you can reproduce the problems Shahab saw, I think it would be good to
incorporate his suggested change and also file a TUI bug for the
remaining problem (unless you feel like fixing it as well...)

Thanks for doing this.  This is a tricky area.

Tom
Reply | Threaded
Open this post in threaded view
|

[PATCHv2 0/2] gdb/tui: Assembler window scrolling fixes

Andrew Burgess
In reply to this post by Andrew Burgess
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(-)

--
2.14.5

Reply | Threaded
Open this post in threaded view
|

[PATCHv2 1/2] gdb/tui: Prevent exceptions from trying to cross readline

Andrew Burgess
From: Pedro Alves <[hidden email]>

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  <[hidden email]>

        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.

Change-Id: I2e32a401ab34404b2132ec82a3e1c17b9b723e41
---
 gdb/tui/tui-io.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 9cb41104fe9..d9f23334f57 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -950,10 +950,12 @@ tui_dispatch_ctrl_char (unsigned int ch)
   return 0;
 }
 
-/* Get a character from the command window.  This is called from the
-   readline package.  */
+/* Main worker for tui_getc.  Get a character from the command window.
+   This is called from the readline package, but wrapped in a
+   try/catch by tui_getc.  */
+
 static int
-tui_getc (FILE *fp)
+tui_getc_1 (FILE *fp)
 {
   int ch;
   WINDOW *w;
@@ -1036,6 +1038,29 @@ tui_getc (FILE *fp)
   return ch;
 }
 
+/* Get a character from the command window.  This is called from the
+   readline package.  */
+
+static int
+tui_getc (FILE *fp)
+{
+  try
+    {
+      return tui_getc_1 (fp);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Just in case, don't ever let an exception escape to readline.
+ This shouldn't ever happen, but if it does, print the
+ exception instead of just crashing GDB.  */
+      exception_print (gdb_stderr, ex);
+
+      /* If we threw an exception, it's because we recognized the
+ character.  */
+      return 0;
+    }
+}
+
 /* See tui-io.h.  */
 
 gdb::unique_xmalloc_ptr<char>
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

[PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Andrew Burgess
In reply to this post by Andrew Burgess
This started as a patch to enable the asm window to handle attempts to
disassemble invalid memory, but it ended up expanding into a
significant rewrite of how the asm window handles scrolling.  These
two things ended up being tied together as it was impossible to
correctly test scrolling into invalid memory when the asm window would
randomly behave weirdly while scrolling.

Things that should work nicely now; scrolling to the bottom or top of
the listing with PageUp, PageDown, Up Arrow, Down Arrow and we should
be able to scroll past small areas of memory that don't have symbols
associated with them.  It should also be possible to scroll to the
start of a section even if there's no symbol at the start of the
section.

Adding tests for this scrolling was a little bit of a problem.  First
I would have liked to add tests for PageUp / PageDown, but the tuiterm
library we use doesn't support these commands right now due to only
emulating a basic ascii terminal.  Changing this to emulate a more
complex terminal would require adding support for more escape sequence
control codes, so I've not tried to tackle that in this patch.

Next, I would have liked to test scrolling to the start or end of the
assembler listing and then trying to scroll even more, however, this
is a problem because in a well behaving GDB a scroll at the start/end
has no effect.  What we need to do is:

  - Move to start of assembler listing,
  - Send scroll up command,
  - Wait for all curses output,
  - Ensure the assembler listing is unchanged, we're still at the
    start of the listing.

The problem is that there is no curses output, so how long do we wait
at step 3?  The same problem exists for scrolling to the bottom of the
assembler listing.  However, when scrolling down you can at least see
the end coming, so I added a test for this case, however, this feels
like an area of code that is massively under tested.

gdb/ChangeLog:

        * minsyms.c (lookup_minimal_symbol_by_pc_section): Update header
        comment, add extra parameter, and update to store previous symbol
        when appropriate.
        * minsyms.h (lookup_minimal_symbol_by_pc_section): Update comment,
        add extra parameter.
        * tui/tui-disasm.c (tui_disassemble): Update header comment,
        remove unneeded parameter, add try/catch around gdb_print_insn,
        rewrite to add items to asm_lines vector.
        (tui_find_backward_disassembly_start_address): New function.
        (tui_find_disassembly_address): Updated throughout.
        (tui_disasm_window::set_contents): Update for changes to
        tui_disassemble.
        (tui_disasm_window::do_scroll_vertical): No need to adjust the
        number of lines to scroll.

gdb/testsuite/ChangeLog:

        * gdb.tui/tui-layout-asm.exp: Add scrolling test for asm window.

Change-Id: I323987c8fd316962c937e73c17d952ccd3cfa66c
---
 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 +++++++++++++++++++++++--------
 6 files changed, 285 insertions(+), 78 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 21335080d31..e238355dc11 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -666,24 +666,18 @@ msym_prefer_to_msym_type (lookup_msym_prefer prefer)
   gdb_assert_not_reached ("unhandled lookup_msym_prefer");
 }
 
-/* Search through the minimal symbol table for each objfile and find
-   the symbol whose address is the largest address that is still less
-   than or equal to PC, and matches SECTION (which is not NULL).
-   Returns a pointer to the minimal symbol if such a symbol is found,
-   or NULL if PC is not in a suitable range.
+/* See minsyms.h.
+
    Note that we need to look through ALL the minimal symbol tables
    before deciding on the symbol that comes closest to the specified PC.
    This is because objfiles can overlap, for example objfile A has .text
    at 0x100 and .data at 0x40000 and objfile B has .text at 0x234 and
-   .data at 0x40048.
-
-   If WANT_TRAMPOLINE is set, prefer mst_solib_trampoline symbols when
-   there are text and trampoline symbols at the same address.
-   Otherwise prefer mst_text symbols.  */
+   .data at 0x40048.  */
 
 bound_minimal_symbol
 lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *section,
-     lookup_msym_prefer prefer)
+     lookup_msym_prefer prefer,
+     bound_minimal_symbol *previous)
 {
   int lo;
   int hi;
@@ -693,6 +687,12 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
   struct objfile *best_objfile = NULL;
   struct bound_minimal_symbol result;
 
+  if (previous != nullptr)
+    {
+      previous->minsym = nullptr;
+      previous->objfile = nullptr;
+    }
+
   if (section == NULL)
     {
       section = find_pc_section (pc_in);
@@ -886,8 +886,23 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
   if (best_zero_sized != -1)
     hi = best_zero_sized;
   else
-    /* Go on to the next object file.  */
-    continue;
+    {
+      /* If needed record this symbol as the closest
+ previous symbol.  */
+      if (previous != nullptr)
+ {
+  if (previous->minsym == nullptr
+      || (MSYMBOL_VALUE_RAW_ADDRESS (&msymbol[hi])
+  > MSYMBOL_VALUE_RAW_ADDRESS
+ (previous->minsym)))
+    {
+      previous->minsym = &msymbol[hi];
+      previous->objfile = objfile;
+    }
+ }
+      /* Go on to the next object file.  */
+      continue;
+    }
  }
 
       /* The minimal symbol indexed by hi now is the best one in this
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 98735e75e33..a8d50a463ba 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -240,7 +240,9 @@ enum class lookup_msym_prefer
 
 /* Search through the minimal symbol table for each objfile and find
    the symbol whose address is the largest address that is still less
-   than or equal to PC, and which matches SECTION.
+   than or equal to PC_IN, and which matches SECTION.  A matching symbol
+   must either by zero sized and have address PC_IN, or PC_IN must fall
+   within the range of addresses covered by the matching symbol.
 
    If SECTION is NULL, this uses the result of find_pc_section
    instead.
@@ -249,12 +251,17 @@ enum class lookup_msym_prefer
    found, or NULL if PC is not in a suitable range.
 
    See definition of lookup_msym_prefer for description of PREFER.  By
-   default mst_text symbols are preferred.  */
+   default mst_text symbols are preferred.
+
+   If the PREVIOUS pointer is non-NULL, and no matching symbol is found,
+   then the contents will be set to reference the closest symbol before
+   PC_IN.  */
 
 struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section
-  (CORE_ADDR,
-   struct obj_section *,
-   lookup_msym_prefer prefer = lookup_msym_prefer::TEXT);
+  (CORE_ADDR pc_in,
+   struct obj_section *section,
+   lookup_msym_prefer prefer = lookup_msym_prefer::TEXT,
+   bound_minimal_symbol *previous = nullptr);
 
 /* Backward compatibility: search through the minimal symbol table
    for a matching PC (no section given).
diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
index cec2735764e..f78baab1081 100644
--- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -32,3 +32,44 @@ if {![Term::prepare_for_tui]} {
 # This puts us into TUI mode, and should display the ASM window.
 Term::command "layout asm"
 Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
+
+# Scroll the ASM window down using the down arrow key.  In an ideal
+# world I'd like to use PageDown here, but currently our terminal
+# library doesn't support such advanced things.
+set testname "scroll to end of assembler"
+set down_count 0
+while (1) {
+    # Grab the second line, this is about to become the first line.
+    set line [Term::get_line 2]
+
+    # Except, if the second line is blank then we are at the end of
+    # the available asm output.  Pressing down again _shouldn't_
+    # change the output, however, if GDB is working, and we press down
+    # then the screen wont change, so the call to Term::wait_for below
+    # will just timeout.  So for now we avoid testing the edge case.
+    if {[regexp -- "^\\| +\\|$" $line]} {
+ # Second line is blank, we're at the end of the assembler.
+ pass $testname
+ break
+    }
+
+    # Send the down key to GDB.
+    send_gdb "\033\[B"
+    incr down_count
+    if {[Term::wait_for [string_to_regexp $line]] \
+    && [Term::get_line 1] == $line} {
+ # We scrolled successfully.
+    } else {
+ fail "$testname (scroll failed)"
+ Term::dump_screen
+ break
+    }
+
+    if { $down_count > 250 } {
+ # Maybe we should accept this as a pass in case a target
+ # really does have loads of assembler to scroll through.
+ fail "$testname (too much assembler)"
+ Term::dump_screen
+ break
+    }
+}
diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 98c691f3387..5b606dcf696 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -81,25 +81,58 @@ len_without_escapes (const std::string &str)
   return len;
 }
 
-/* Function to set the disassembly window's content.
-   Disassemble count lines starting at pc.
-   Return address of the count'th instruction after pc.  */
+/* Function to disassemble up to COUNT instructions starting from address
+   PC into the ASM_LINES vector (which will be emptied of any previous
+   contents).  Return the address of the count'th instruction after pc.
+   When ADDR_SIZE is non-null then place the maximum size of an address and
+   label into the value pointed to by ADDR_SIZE, and set the addr_size
+   field on each item in ASM_LINES, otherwise the addr_size fields within
+   asm_lines are undefined.
+
+   It is worth noting that ASM_LINES might not have COUNT entries when this
+   function returns.  If the disassembly is truncated for some other
+   reason, for example, we hit invalid memory, then ASM_LINES can have
+   fewer entries than requested.  */
 static CORE_ADDR
 tui_disassemble (struct gdbarch *gdbarch,
  std::vector<tui_asm_line> &asm_lines,
- CORE_ADDR pc, int pos, int count,
+ CORE_ADDR pc, int count,
  size_t *addr_size = nullptr)
 {
   bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
   string_file gdb_dis_out (term_out);
 
+  /* Must start with an empty list.  */
+  asm_lines.clear ();
+
   /* Now construct each line.  */
   for (int i = 0; i < count; ++i)
     {
-      print_address (gdbarch, pc, &gdb_dis_out);
-      asm_lines[pos + i].addr = pc;
-      asm_lines[pos + i].addr_string = std::move (gdb_dis_out.string ());
+      tui_asm_line tal;
+      CORE_ADDR orig_pc = pc;
 
+      try
+ {
+  pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
+ }
+      catch (const gdb_exception &except)
+ {
+  /* If pc points to an invalid address then we'll catch a
+     MEMORY_ERROR here, this should stop the disassembly, but
+     otherwise is fine.  */
+  if (except.error != MEMORY_ERROR)
+    throw;
+  return pc;
+ }
+
+      /* Capture the disassembled instruction.  */
+      tal.insn = std::move (gdb_dis_out.string ());
+      gdb_dis_out.clear ();
+
+      /* And capture the address the instruction is at.  */
+      tal.addr = orig_pc;
+      print_address (gdbarch, orig_pc, &gdb_dis_out);
+      tal.addr_string = std::move (gdb_dis_out.string ());
       gdb_dis_out.clear ();
 
       if (addr_size != nullptr)
@@ -107,23 +140,45 @@ tui_disassemble (struct gdbarch *gdbarch,
   size_t new_size;
 
   if (term_out)
-    new_size = len_without_escapes (asm_lines[pos + i].addr_string);
+    new_size = len_without_escapes (tal.addr_string);
   else
-    new_size = asm_lines[pos + i].addr_string.size ();
+    new_size = tal.addr_string.size ();
   *addr_size = std::max (*addr_size, new_size);
-  asm_lines[pos + i].addr_size = new_size;
+  tal.addr_size = new_size;
  }
 
-      pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
-
-      asm_lines[pos + i].insn = std::move (gdb_dis_out.string ());
-
-      /* Reset the buffer to empty.  */
-      gdb_dis_out.clear ();
+      asm_lines.push_back (std::move (tal));
     }
   return pc;
 }
 
+/* Look backward from ADDR for an address from which we can start
+   disassembling, this needs to be something we can be reasonably
+   confident will fall on an instruction boundary.  We use msymbol
+   addresses, or the start of a section.  */
+
+static CORE_ADDR
+tui_find_backward_disassembly_start_address (CORE_ADDR addr)
+{
+  struct bound_minimal_symbol msym, msym_prev;
+
+  msym = lookup_minimal_symbol_by_pc_section (addr - 1, nullptr,
+      lookup_msym_prefer::TEXT,
+      &msym_prev);
+  if (msym.minsym != nullptr)
+    return BMSYMBOL_VALUE_ADDRESS (msym);
+  else if (msym_prev.minsym != nullptr)
+    return BMSYMBOL_VALUE_ADDRESS (msym_prev);
+
+  /* Find the section that ADDR is in, and look for the start of the
+     section.  */
+  struct obj_section *section = find_pc_section (addr);
+  if (section != NULL)
+    return obj_section_addr (section);
+
+  return addr;
+}
+
 /* Find the disassembly address that corresponds to FROM lines above
    or below the PC.  Variable sized instructions are taken into
    account by the algorithm.  */
@@ -134,65 +189,125 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
   int max_lines;
 
   max_lines = (from > 0) ? from : - from;
-  if (max_lines <= 1)
+  if (max_lines == 0)
     return pc;
 
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
 
   new_low = pc;
   if (from > 0)
     {
-      tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines);
-      new_low = asm_lines[max_lines - 1].addr;
+      /* Always disassemble 1 extra instruction here, then if the last
+ instruction fails to disassemble we will take the address of the
+ previous instruction that did disassemble as the result.  */
+      tui_disassemble (gdbarch, asm_lines, pc, max_lines + 1);
+      new_low = asm_lines.back ().addr;
     }
   else
     {
+      /* In order to disassemble backwards we need to find a suitable
+ address to start disassembling from and then work forward until we
+ re-find the address we're currently at.  We can then figure out
+ which address will be at the top of the TUI window after our
+ backward scroll.  During our backward disassemble we need to be
+ able to distinguish between the case where the last address we
+ _can_ disassemble is ADDR, and the case where the disassembly
+ just happens to stop at ADDR, for this reason we increase
+ MAX_LINES by one.  */
+      max_lines++;
+
+      /* When we disassemble a series of instructions this will hold the
+ address of the last instruction disassembled.  */
       CORE_ADDR last_addr;
-      int pos;
-      struct bound_minimal_symbol msymbol;
-
-      /* Find backward an address which is a symbol and for which
-         disassembling from that address will fill completely the
-         window.  */
-      pos = max_lines - 1;
-      do {
-         new_low -= 1 * max_lines;
-         msymbol = lookup_minimal_symbol_by_pc_section (new_low, 0);
-
-         if (msymbol.minsym)
-            new_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
-         else
-            new_low += 1 * max_lines;
-
-         tui_disassemble (gdbarch, asm_lines, new_low, 0, max_lines);
-         last_addr = asm_lines[pos].addr;
-      } while (last_addr > pc && msymbol.minsym);
+
+      /* And this will hold the address of the next instruction that would
+ have been disassembled.  */
+      CORE_ADDR next_addr;
+
+      /* As we search backward if we find an address that looks like a
+ promising starting point then we record it in this structure.  If
+ the next address we try is not a suitable starting point then we
+ will fall back to the address held here.  */
+      gdb::optional<CORE_ADDR> possible_new_low;
+
+      /* The previous value of NEW_LOW so we know if the new value is
+ different or not.  */
+      CORE_ADDR prev_low;
+
+      do
+ {
+  /* Find an address from which we can start disassembling.  */
+  prev_low = new_low;
+  new_low = tui_find_backward_disassembly_start_address (new_low);
+
+  /* Disassemble forward.  */
+  next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+  last_addr = asm_lines.back ().addr;
+
+  /* If disassembling from the current value of NEW_LOW reached PC
+     (or went past it) then this would do as a starting point if we
+     can't find anything better, so remember it.  */
+  if (last_addr >= pc && new_low != prev_low
+      && asm_lines.size () >= max_lines)
+    possible_new_low.emplace (new_low);
+
+  /* Continue searching until we find a value of NEW_LOW from which
+     disassembling MAX_LINES instructions doesn't reach PC.  We
+     know this means we can find the required number of previous
+     instructions then.  */
+ }
+      while ((last_addr > pc
+      || (last_addr == pc && asm_lines.size () < max_lines))
+     && new_low != prev_low);
+
+      /* If we failed to disassemble the required number of lines then the
+ following walk forward is not going to work, it assumes that
+ ASM_LINES contains exactly MAX_LINES entries.  Instead we should
+ consider falling back to a previous possible start address in
+ POSSIBLE_NEW_LOW.  */
+      if (asm_lines.size () < max_lines)
+ {
+  if (!possible_new_low.has_value ())
+    return pc;
+
+  /* Take the best possible match we have.  */
+  new_low = *possible_new_low;
+  next_addr = tui_disassemble (gdbarch, asm_lines, new_low, max_lines);
+  last_addr = asm_lines.back ().addr;
+  gdb_assert (asm_lines.size () >= max_lines);
+ }
 
       /* Scan forward disassembling one instruction at a time until
          the last visible instruction of the window matches the pc.
          We keep the disassembled instructions in the 'lines' window
          and shift it downward (increasing its addresses).  */
+      int pos = max_lines - 1;
       if (last_addr < pc)
         do
           {
-            CORE_ADDR next_addr;
-
             pos++;
             if (pos >= max_lines)
               pos = 0;
 
-            next_addr = tui_disassemble (gdbarch, asm_lines,
- last_addr, pos, 1);
-
+    CORE_ADDR old_next_addr = next_addr;
+    std::vector<tui_asm_line> single_asm_line;
+    next_addr = tui_disassemble (gdbarch, single_asm_line,
+ next_addr, 1);
             /* If there are some problems while disassembling exit.  */
-            if (next_addr <= last_addr)
-              break;
-            last_addr = next_addr;
-          } while (last_addr <= pc);
+    if (next_addr <= old_next_addr)
+      return pc;
+    gdb_assert (single_asm_line.size () == 1);
+    asm_lines[pos] = single_asm_line[0];
+  } while (next_addr <= pc);
       pos++;
       if (pos >= max_lines)
          pos = 0;
       new_low = asm_lines[pos].addr;
+
+      /* When scrolling backward the addresses should move backward, or at
+ the very least stay the same if we are at the first address that
+ can be disassembled.  */
+      gdb_assert (new_low <= pc);
     }
   return new_low;
 }
@@ -224,9 +339,9 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
   line_width = width - TUI_EXECINFO_SIZE - 2;
 
   /* Get temporary table that will hold all strings (addr & insn).  */
-  std::vector<tui_asm_line> asm_lines (max_lines);
+  std::vector<tui_asm_line> asm_lines;
   size_t addr_size = 0;
-  tui_disassemble (gdbarch, asm_lines, pc, 0, max_lines, &addr_size);
+  tui_disassemble (gdbarch, asm_lines, pc, max_lines, &addr_size);
 
   /* Align instructions to the same column.  */
   insn_pos = (1 + (addr_size / tab_len)) * tab_len;
@@ -237,17 +352,29 @@ tui_disasm_window::set_contents (struct gdbarch *arch,
     {
       tui_source_element *src = &content[i];
 
-      std::string line
- = (asm_lines[i].addr_string
-   + n_spaces (insn_pos - asm_lines[i].addr_size)
-   + asm_lines[i].insn);
+      std::string line;
+      CORE_ADDR addr;
+
+      if (i < asm_lines.size ())
+ {
+  line
+    = (asm_lines[i].addr_string
+       + n_spaces (insn_pos - asm_lines[i].addr_size)
+       + asm_lines[i].insn);
+  addr = asm_lines[i].addr;
+ }
+      else
+ {
+  line = "";
+  addr = 0;
+ }
 
       const char *ptr = line.c_str ();
       src->line = tui_copy_source_line (&ptr, -1, offset, line_width, 0);
 
       src->line_or_addr.loa = LOA_ADDRESS;
-      src->line_or_addr.u.addr = asm_lines[i].addr;
-      src->is_exec_point = asm_lines[i].addr == cur_pc;
+      src->line_or_addr.u.addr = addr;
+      src->is_exec_point = (addr == cur_pc && line.size () > 0);
     }
   return true;
 }
@@ -326,10 +453,6 @@ tui_disasm_window::do_scroll_vertical (int num_to_scroll)
       CORE_ADDR pc;
 
       pc = start_line_or_addr.u.addr;
-      if (num_to_scroll >= 0)
- num_to_scroll++;
-      else
- --num_to_scroll;
 
       symtab_and_line sal {};
       sal.pspace = current_program_space;
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Shahab Vahedi-2
Great job Andrew!

Functionality testing:
I have tested this patch and it is impossible to break it.

Code review:
The comments that have been added are very self explanatory.
The logic that is used seems sound and thorough. I have one
small remark though. For most part of the
tui_find_disassembly_address() function, there are usages of
new_low, prev_low, and possible_new_low variables. What these
variables are actually used for is finding a new _high_. I
suggest replacing every instance of "low/LOW" with "high/HIGH".
The region of code I am talking about is listed below:

region of code begins
  else
    {
      ...
      gdb::optional<CORE_ADDR> possible_new_low;
      ...
      CORE_ADDR prev_low;

      do
        {
          ...
        }
      while (...)
      ...
      if (asm_lines.size () < max_lines)
        {
          if (!possible_new_low.has_value ())
            return pc;

          new_low = *possible_new_low;
          next_addr = tui_disassemble (gdbarch, asm_lines,
                                       new_low, max_lines);
          ...
        }
region of code ends

Nevertheless, the code looks (very) fine to me as it is.


Cheers,
Shahab
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Shahab Vahedi-2
I just figured out that the "low" means the lower address
than the current one, which happens to be _visually_
printed higher.

Please ignore my remark regarding that and sorry for the
confusion.


Cheers,
Shahab
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Andrew Burgess
* Shahab Vahedi <[hidden email]> [2020-01-22 10:19:03 +0000]:

> I just figured out that the "low" means the lower address
> than the current one, which happens to be _visually_
> printed higher.
>
> Please ignore my remark regarding that and sorry for the
> confusion.

Not a problem!

Thanks,
Andrew
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2 2/2] gdb/tui: asm window handles invalid memory and scrolls better

Pedro Alves-7
In reply to this post by Andrew Burgess
Hi!

I think this should be filed under PR tui/9765 too?

I didn't try to grok the code very deeply.  I did try it out, and
saw that it works as described.  I'm happy with that.
Thanks for doing all this.

Tiny nits below, mostly comments issues I noticed while
skimming the patch.

> index 98735e75e33..a8d50a463ba 100644
> --- a/gdb/minsyms.h
> +++ b/gdb/minsyms.h
> @@ -240,7 +240,9 @@ enum class lookup_msym_prefer
>  
>  /* Search through the minimal symbol table for each objfile and find
>     the symbol whose address is the largest address that is still less
> -   than or equal to PC, and which matches SECTION.
> +   than or equal to PC_IN, and which matches SECTION.  A matching symbol
> +   must either by zero sized and have address PC_IN, or PC_IN must fall

"BE zero sized", I think.

> +   within the range of addresses covered by the matching symbol.
>  
>     If SECTION is NULL, this uses the result of find_pc_section
>     instead.
> @@ -249,12 +251,17 @@ enum class lookup_msym_prefer
>     found, or NULL if PC is not in a suitable range.
>  
>     See definition of lookup_msym_prefer for description of PREFER.  By
> -   default mst_text symbols are preferred.  */
> +   default mst_text symbols are preferred.
> +
> +   If the PREVIOUS pointer is non-NULL, and no matching symbol is found,
> +   then the contents will be set to reference the closest symbol before
> +   PC_IN.  */
>  


> --- a/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> +++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
> @@ -32,3 +32,44 @@ if {![Term::prepare_for_tui]} {
>  # This puts us into TUI mode, and should display the ASM window.
>  Term::command "layout asm"
>  Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
> +
> +# Scroll the ASM window down using the down arrow key.  In an ideal
> +# world I'd like to use PageDown here, but currently our terminal

Please avoid "I" in comments.  

For example, you could write "In an ideal world we'd use".

> +# library doesn't support such advanced things.
> +set testname "scroll to end of assembler"
> +set down_count 0
> +while (1) {
> +    # Grab the second line, this is about to become the first line.
> +    set line [Term::get_line 2]
> +
> +    # Except, if the second line is blank then we are at the end of
> +    # the available asm output.  Pressing down again _shouldn't_
> +    # change the output, however, if GDB is working, and we press down
> +    # then the screen wont change, so the call to Term::wait_for below

wont -> won't

> +    # will just timeout.  So for now we avoid testing the edge case.
> +    if {[regexp -- "^\\| +\\|$" $line]} {
> + # Second line is blank, we're at the end of the assembler.
> + pass $testname
> + break
> +    }
> +
> +    # Send the down key to GDB.
> +    send_gdb "\033\[B"
> +    incr down_count
> +    if {[Term::wait_for [string_to_regexp $line]] \
> +    && [Term::get_line 1] == $line} {
> + # We scrolled successfully.
> +    } else {
> + fail "$testname (scroll failed)"
> + Term::dump_screen
> + break
> +    }
> +
> +    if { $down_count > 250 } {
> + # Maybe we should accept this as a pass in case a target
> + # really does have loads of assembler to scroll through.
> + fail "$testname (too much assembler)"
> + Term::dump_screen
> + break
> +    }
> +}
> diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
> index 98c691f3387..5b606dcf696 100644
> --- a/gdb/tui/tui-disasm.c
> +++ b/gdb/tui/tui-disasm.c
> @@ -81,25 +81,58 @@ len_without_escapes (const std::string &str)
>    return len;
>  }
>  
> -/* Function to set the disassembly window's content.
> -   Disassemble count lines starting at pc.
> -   Return address of the count'th instruction after pc.  */
> +/* Function to disassemble up to COUNT instructions starting from address
> +   PC into the ASM_LINES vector (which will be emptied of any previous
> +   contents).  Return the address of the count'th instruction after pc.

Uppercase COUNT in "COUNT'th" ?

( I'm not sure I'm able to pronounce that.  :-) )


> +   When ADDR_SIZE is non-null then place the maximum size of an address and
> +   label into the value pointed to by ADDR_SIZE, and set the addr_size
> +   field on each item in ASM_LINES, otherwise the addr_size fields within
> +   asm_lines are undefined.

Uppercase last ASM_LINES ?

> +
> +   It is worth noting that ASM_LINES might not have COUNT entries when this
> +   function returns.  If the disassembly is truncated for some other
> +   reason, for example, we hit invalid memory, then ASM_LINES can have
> +   fewer entries than requested.  */
>  static CORE_ADDR
>  tui_disassemble (struct gdbarch *gdbarch,
>   std::vector<tui_asm_line> &asm_lines,
> - CORE_ADDR pc, int pos, int count,
> + CORE_ADDR pc, int count,
>   size_t *addr_size = nullptr)
>  {
>    bool term_out = source_styling && gdb_stdout->can_emit_style_escape ();
>    string_file gdb_dis_out (term_out);
>  
> +  /* Must start with an empty list.  */
> +  asm_lines.clear ();
> +
>    /* Now construct each line.  */
>    for (int i = 0; i < count; ++i)
>      {
> -      print_address (gdbarch, pc, &gdb_dis_out);
> -      asm_lines[pos + i].addr = pc;
> -      asm_lines[pos + i].addr_string = std::move (gdb_dis_out.string ());
> +      tui_asm_line tal;
> +      CORE_ADDR orig_pc = pc;
>  
> +      try
> + {
> +  pc = pc + gdb_print_insn (gdbarch, pc, &gdb_dis_out, NULL);
> + }
> +      catch (const gdb_exception &except)

This can be gdb_exception_error.


> + {
> +  /* If pc points to an invalid address then we'll catch a

Uppercase PC.

> +     MEMORY_ERROR here, this should stop the disassembly, but
> +     otherwise is fine.  */
> +  if (except.error != MEMORY_ERROR)
> +    throw;
> +  return pc;
> + }
> +
> +      /* Capture the disassembled instruction.  */
> +      tal.insn = std::move (gdb_dis_out.string ());
> +      gdb_dis_out.clear ();
> +
> +      /* And capture the address the instruction is at.  */
> +      tal.addr = orig_pc;
> +      print_address (gdbarch, orig_pc, &gdb_dis_out);
> +      tal.addr_string = std::move (gdb_dis_out.string ());
>        gdb_dis_out.clear ();
>  
>        if (addr_size != nullptr)

Thanks,
Pedro Alves

12