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

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

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

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  <shahab@synopsys.com>

	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

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

[Pedro Alves]

On 1/10/20 11:57 AM, Shahab Vahedi wrote:
> From: Shahab Vahedi <shahab@synopsys.com>
> 
> 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  <shahab@synopsys.com>
> 
> 	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

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

[Pedro Alves]

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

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

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

[Shahab Vahedi]

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
> 

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

[Shahab Vahedi]

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

Cheers,
--
Shahab

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

[Tom Tromey]

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

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

[Andrew Burgess]

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 <andrew.burgess@embecosm.com>
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;
 }

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

[Andrew Burgess]

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

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

[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

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

[Andrew Burgess]

From: Pedro Alves <palves@redhat.com>

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.

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

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

[Shahab Vahedi]

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

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

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

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

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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

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

[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

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

[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

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

[Andrew Burgess]

From: Pedro Alves <palves@redhat.com>

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.

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

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

[Shahab Vahedi]

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

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

[Shahab Vahedi]

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

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

[Andrew Burgess]

* Shahab Vahedi <Shahab.Vahedi@synopsys.com> [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

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

[Pedro Alves]

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

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

[Shahab Vahedi]

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

[PATCH 1/2] gdb/tui: Update help text for scroll commands

[Andrew Burgess]

GDB has some commands ('+', '-', '<', and '>') for scrolling the SRC
and ASM TUI windows from the CMD window, however the help text for
these commands lists the arguments in the wrong order.

This commit updates the help text to match how GDB actually works, and
also extends the text to describe what the arguments mean, and what
the defaults are.

There should be no change in GDBs functionality after this commit.

gdb/ChangeLog:

	* tui/tui-win.c (_initialize_tui_win): Update help text for '+',
	'-', '<', and '>' commands.

Change-Id: Ib2624891de1f4ba983838822206304e4c3ed982e
---
 gdb/ChangeLog     |  5 +++++
 gdb/tui/tui-win.c | 16 ++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 8b7c39916a3..185ae26f698 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -1063,16 +1063,24 @@ FOCUS_USAGE
   set_cmd_completer (cmd, focus_completer);
   add_com ("+", class_tui, tui_scroll_forward_command, _("\
 Scroll window forward.\n\
-Usage: + [WIN] [N]"));
+Usage: + [N] [WIN]\n\
+Scroll window WIN N lines forwards.  Both WIN and N are optional, N\n\
+defaults to 1, and WIN defaults to the currently focused window."));
   add_com ("-", class_tui, tui_scroll_backward_command, _("\
 Scroll window backward.\n\
-Usage: - [WIN] [N]"));
+Usage: - [N] [WIN]\n\
+Scroll window WIN N lines backwards.  Both WIN and N are optional, N\n\
+defaults to 1, and WIN defaults to the currently focused window."));
   add_com ("<", class_tui, tui_scroll_left_command, _("\
 Scroll window text to the left.\n\
-Usage: < [WIN] [N]"));
+Usage: < [N] [WIN]\n\
+Scroll window WIN N characters left.  Both WIN and N are optional, N\n\
+defaults to 1, and WIN defaults to the currently focused window."));
   add_com (">", class_tui, tui_scroll_right_command, _("\
 Scroll window text to the right.\n\
-Usage: > [WIN] [N]"));
+Usage: > [N] [WIN]\n\
+Scroll window WIN N characters right.  Both WIN and N are optional, N\n\
+defaults to 1, and WIN defaults to the currently focused window."));
 
   /* Define the tui control variables.  */
   add_setshow_enum_cmd ("border-kind", no_class, tui_border_kind_enums,
-- 
2.14.5

[PATCH 0/2] Further Assembler Scrolling Fixes

[Andrew Burgess]

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

-- 
2.14.5

[PATCH 2/2] gdb/tui: Disassembler scrolling of very small programs

[Andrew Burgess]

In TUI mode, if the disassembly output for the program is less than
one screen long, then currently if the user scrolls down until on the
last assembly instruction is displayed and then tries to scroll up
using Page-Up, the display doesn't update - they are stuck viewing the
last line.

If the user tries to scroll up using the Up-Arrow, then the display
scrolls normally.

What is happening is on the Page-Up we ask GDB to scroll backward the
same number of lines as the height of the TUI ASM window.  The back
scanner, which looks for a good place to start disassembling, fails to
find a starting address which will provide the requested number of new
lines before we get back to the original starting address (which is
not surprising, our whole program contains less than a screen height
of instructions), as a result the back scanner gives up and returns
the original starting address.

When we scroll with Up-Arrow we only ask the back scanner to find 1
new instruction, which it manages to do, so this scroll works.

The solution here is, when we fail to find enough instructions, to
return the lowest address we did manage to find.  This will ensure we
jump to the lowest possible address in the disassembly output.

gdb/ChangeLog:

	PR tui/9765
	* tui/tui-disasm.c (tui_find_disassembly_address): If we don't
	have enough lines to fill the screen, still return the lowest
	address we found.

gdb/testsuite/ChangeLog:

	PR tui/9765
	* gdb.tui/tui-layout-asm-short-prog.S: New file.
	* gdb.tui/tui-layout-asm-short-prog.exp: New file.

Change-Id: I6a6a7972c68a0559e9717fd8d82870b669a40af3
---
 gdb/ChangeLog                                      |  7 +++
 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 +-
 5 files changed, 87 insertions(+), 1 deletion(-)
 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

diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S
new file mode 100644
index 00000000000..7705ef139aa
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.global _start
+_start:
+	.rept 5
+	nop
+	.endr
diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp
new file mode 100644
index 00000000000..d0b871ff762
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp
@@ -0,0 +1,51 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Ensure that 'layout asm' can scroll away from the last line of a
+# very short program using a page up sized scroll.
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout-asm-short-prog.S
+
+if {[build_executable "failed to prepare" ${testfile} ${srcfile} \
+	 {debug additional_flags=-nostdlib \
+	      additional_flags=-nostartfiles}] == -1} {
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::prepare_for_tui]} {
+    unsupported "TUI not supported"
+}
+
+# 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 "<_start>"
+
+# Record the first line of output, we'll need this later.
+set first_line [Term::get_line 1]
+
+# Scroll forward a large amount, this should take us to the last
+# instruction in the program.
+Term::command "+ 13"
+Term::check_box_contents "check asm box contents again" 0 0 80 15 \
+    "^ *$hex\[^\n\]+\n +\n"
+
+# Now scroll backward again, we should return to the start of the
+# program.
+Term::command "- 13"
+gdb_assert {[string eq "$first_line" [Term::get_line 1]]} \
+    "check first line is back"
diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c
index 726b7c27d60..547d2c9e6d6 100644
--- a/gdb/tui/tui-disasm.c
+++ b/gdb/tui/tui-disasm.c
@@ -268,7 +268,7 @@ tui_find_disassembly_address (struct gdbarch *gdbarch, CORE_ADDR pc, int from)
       if (asm_lines.size () < max_lines)
 	{
 	  if (!possible_new_low.has_value ())
-	    return pc;
+	    return new_low;
 
 	  /* Take the best possible match we have.  */
 	  new_low = *possible_new_low;
-- 
2.14.5

Re: [PATCH 1/2] gdb/tui: Update help text for scroll commands

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> GDB has some commands ('+', '-', '<', and '>') for scrolling the SRC
Andrew> and ASM TUI windows from the CMD window, however the help text for
Andrew> these commands lists the arguments in the wrong order.

Andrew> This commit updates the help text to match how GDB actually works, and
Andrew> also extends the text to describe what the arguments mean, and what
Andrew> the defaults are.

Andrew> There should be no change in GDBs functionality after this commit.

Andrew> gdb/ChangeLog:

Andrew> 	* tui/tui-win.c (_initialize_tui_win): Update help text for '+',
Andrew> 	'-', '<', and '>' commands.

Thanks, this is ok.

I notice now that these commands aren't in gdb.texinfo.

Tom

Re: [PATCH 2/2] gdb/tui: Disassembler scrolling of very small programs

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	PR tui/9765
Andrew> 	* tui/tui-disasm.c (tui_find_disassembly_address): If we don't
Andrew> 	have enough lines to fill the screen, still return the lowest
Andrew> 	address we found.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	PR tui/9765
Andrew> 	* gdb.tui/tui-layout-asm-short-prog.S: New file.
Andrew> 	* gdb.tui/tui-layout-asm-short-prog.exp: New file.

Looks good to me.  Thank you for doing this.

Tom

Re: [PATCH 2/2] gdb/tui: Disassembler scrolling of very small programs

[Shahab Vahedi]

Hi Andrew,

The fix is very subtle and works flawlessly. Great job on catching all
the corner cases and putting them in tests.

Cheers,
Shahab


P.S.: For the record, the fix is already merged:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=42330a681af23a80d3e1f201d2a65886875e74bd