| Message ID | ed397bcd-0562-d18d-b7e8-365bbf8e7b37@redhat.com |
|---|---|
| State | New |
| Headers | show |
This patch must be in as well! It takes care of the "call stack 2" from the other e-mail. Cheers, -- Shahab
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> gdb/ChangeLog:
Pedro> yyyy-mm-dd Pedro Alves <palves@redhat.com>
Pedro> PR tui/9765
Pedro> * tui/tui-io.c (tui_getc): Rename to ...
Pedro> (tui_getc_1): ... this.
Pedro> (tui_get): New, reimplent as try/catch wrapper around tui_getc_1.
Pedro> ---
Pedro> gdb/tui/tui-io.c | 31 ++++++++++++++++++++++++++++---
Pedro> 1 file changed, 28 insertions(+), 3 deletions(-)
This seems like a good idea measure to me.
Ideally I suppose these TUI functions shouldn't throw. However, in gdb
it can be difficult to ensure that, so this provides some defense.
thanks,
Tom
Patch #1 is Pedro's patch to stop exceptions trying to cross readline which was posted elsewhere in this thread and really should be included before its lost. Patch #2 is a reworking of assembler window scrolling that allows it to handle trying to disassemble invalid memory, and also makes the whole scrolling experience more robust (I believe). -- Andrew Burgess (1): gdb/tui: asm window handles invalid memory and scrolls better Pedro Alves (1): gdb/tui: Prevent exceptions from trying to cross readline gdb/ChangeLog | 12 ++ gdb/testsuite/ChangeLog | 4 + gdb/testsuite/gdb.tui/tui-layout-asm.exp | 41 ++++++ gdb/tui/tui-disasm.c | 216 +++++++++++++++++++++++-------- gdb/tui/tui-io.c | 31 ++++- 5 files changed, 245 insertions(+), 59 deletions(-)
Hello Andrew,
I have tested the patch series against the "hello world"
program that I have [1]. Here comes my observations:
1. As you can see in this gif file:
https://sourceware.org/bugzilla/attachment.cgi?id=12200
"Going up" sometimes does not work. In this case, it is
between "main" and "_start" (garbage?). Nevertheless, I
manged to have it working for me by:
@@ -161,7 +161,7 @@ tui_find_symbol_backward (CORE_ADDR addr)
{
struct bound_minimal_symbol msym;
- for (int offset = 1; offset <= 1024; offset *= 2)
+ for (int offset = 1; offset <= 1024; ++offset)
{
CORE_ADDR tmp = addr - offset;
msym = lookup_minimal_symbol_by_pc_section (tmp, 0);
2. I run into a problem that "page-up" does not work when we
reach the end of file:
https://sourceware.org/bugzilla/attachment.cgi?id=12201
The following fixes that, but breaks elsewhere (see 3):
@@ -232,14 +232,14 @@ tui_find_disassembly_address ...
/* Disassemble forward a few lines and see ...
next_addr = tui_disassemble (gdbarch, asm_lines, ...
last_addr = asm_lines.back ().addr;
- if (last_addr > pc && msymbol.minsym != nullptr
+ if (last_addr >= pc && msymbol.minsym != nullptr
&& asm_lines.size () >= max_lines)
{
/* This will do if we can't find anything... */
possible_new_low.found = true;
possible_new_low.new_low = new_low;
}
- } while (last_addr > pc && msymbol.minsym != nullptr);
+ } while (last_addr >= pc && msymbol.minsym != nullptr);
3. With the changes from above, "arrow up" stops working near
beginning of the file:
https://sourceware.org/bugzilla/attachment.cgi?id=12202
I can summarise what I have learned in the following table:
,-------------.----------.----------------------------------.
| usecase | action | condition that works |
|-------------+----------+----------------------------------|
| very bottom | page-up | as long as last_addr >= pc ..., |
| | | go higher (try reducing new_low) |
|-------------+----------+----------------------------------|
| second line | arrow up | as long as last_addr > pc ..., |
| | | go higher (try reducing new_low) |
`-------------^----------^----------------------------------'
This table portrays contradictory conditions for corner case
scenarios. I believe the real solution in the end should be
much simpler and ideally as such that it covers the corner
cases naturally. I will try to cook something up in my free
time.
I suggest that this patch series should be in along with
the changes proposed at point 1. So we only end-up with
none-working page-up at the end of assembly output.
Cheers,
Shahab
[1]
that x86_64 elf program is archived here:
https://sourceware.org/bugzilla/attachment.cgi?id=12199
This revision addresses the issues raised by Shahab, as well as making the improvements Tom pointed out. I looked at changing the TERM type from ansi to xterm as Tom suggested, but figuring out all of the extra control sequences that are sent was taking too much effort. I might try to revisit this when I have more time, but I don't plan to do this in the immediate future. I did start adding a mechanism to try and detect when the user tries to scroll and we're already at the end of the output (or the beginning), and this helped in the scroll down case, but I still need to figure out how to use this in the scroll up case, so for now I've not included this work in this patch set. There's still some things I think could be improved with the assembler scrolling - the user is currenly "trapped" inside the continuous memory region that the $pc starts in, they can't scroll to any disjoint code region, but this never worked before either, so this isn't a regression. I do have an idea for how to fix this, but I'm hoping to merge this set first, and work on the multi-section support when I can find some time later. Comments/feedback welcome as always, Thanks, Andrew --- Andrew Burgess (1): gdb/tui: asm window handles invalid memory and scrolls better Pedro Alves (1): gdb/tui: Prevent exceptions from trying to cross readline gdb/ChangeLog | 17 +++ gdb/minsyms.c | 41 ++++-- gdb/minsyms.h | 17 ++- gdb/testsuite/ChangeLog | 4 + gdb/testsuite/gdb.tui/tui-layout-asm.exp | 41 ++++++ gdb/tui/tui-disasm.c | 243 +++++++++++++++++++++++-------- gdb/tui/tui-io.c | 31 +++- 7 files changed, 313 insertions(+), 81 deletions(-)
These patches are in now: gdb/tui: Prevent exceptions from trying to cross readline https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2f267673f0fdee9287e6d404ecd4f2d29da0d2f2 gdb/tui: asm window handles invalid memory and scrolls better https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=733d0a679536628eb1be4b4b8aa6384de24ff1f1 However, for the record, I must add that I have found one failing case. That happens when you have a small program that fits in one window, then you do a page down and reach the end of it. Now, the page up does not work anymore. A sample usecase is attached for your amusement. In case you do not like attachments, just save the base64 code below ... H4sIAAAAAAAAA+2WbW/bNhDH/db8FAclgGOgsiU/FgI2oEtSb0Aeijh9MSxFQUm0rJUWDZJqZBT9 7jtK8kPWtDXSwu0w/l5YJvk/HsnTiScy5nKqtMuFUMxlWezqPHWpWrgqkoLzNEu6jW/DQ8bDoXn6 46FXPft+2V/T8Psjvzcc+P6o38DRnjdowPAb/e5FrjSVAA01p3MafknHpDrEgg6L2Cf+N+cvzi7P n+zDBHg0GHw2/sNxbx1/b2Ti38P3BePvfcd9fpb/efxnqVQa9BzDrKBsBITGdGn6GDiRyGZp4oBi kU5FBmlW9i/oOzZLOSNEsqUUcR6hPSjNliogvwKL5gKc4w+n1xevL6+mH4vjDxd/XJ1PPzoE4LlX 9AYoMnOQkyQO2/AXLCVTCpY0YW4s7jMQWcTgE95s9Jm4LxdSWuRLiAVT2KnhXsh3QLPVQkiGehKz ME8Sszq0hFQrxmdB7R13Jd8z+dDHkdkjZgVoJhdpRvlaXM4Ej4gp+p3jNBsDEqeKKsUWocmecp0h DsjVxjEKyI+OfMVe+U8X8WjQefLb/7X873v9Ov/7Y3/QM/nf93yb/4eg2Ukzzfhbtco0LTCFMBNn aUGaHc0KjY+Ei5A335pT0qR6BKRZCNlktHgG+HPwFgA2gcWmGRY/SSL9R9kr/9ef+yf6+Er++wO/ t63/hmOT/2NvaPP/ENyeT2/NTRb8AuVXnpAjqC59Mjn7Deqh7pLqeVeLLt5lXbxIu3jpJ5IujFoL wRWZvLq+ua3Efq8/IJOXFy8m07LtsgJaimnYXosrd8bpe0zi8uPTqiScrkSuAV+9FplMK/va/M75 kv2dU4u2M9w5hFDOA3PrExLmKY8DOD4xu213FGken5yetsGNtn3gJn1wxaZDGNHFWducwKavPI8d ESuwBEIXAZQujAmeWhvHq/23HyqPQOaZWVJdeJw4+N+B+5RzCJmpTnBDMyZTIduk0mxm3loFOLs5 7dJN7W36uLvdKuffZc3RblnzWBlTjuz6X/tbRwuUjFp7h9dIqMQq8/Hl1ipMxgSFki2EZtuttsqi arMYEf4d54sluDG4l5WXh1vvvPr9+urPACLOaEZI+QhMeG4u2zvh3LX50ZlosVgsFovFYrFYLBaL xWKxWCwWi8XyffgH4PT6ZQAoAAA= ... and then: base64 -d <saved_code_from_email> > lingering-bug.tgz Last but not least, I cannot care less about this now, but I wanted it documented. Cheers, Shahab
Shahab, Thanks for reporting this regression. Here's a fix. I'll give this a few days for feedback, then push it. Thanks, Andrew --- Andrew Burgess (2): gdb/tui: Update help text for scroll commands gdb/tui: Disassembler scrolling of very small programs gdb/ChangeLog | 12 +++++ gdb/testsuite/ChangeLog | 6 +++ gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S | 22 ++++++++++ .../gdb.tui/tui-layout-asm-short-prog.exp | 51 ++++++++++++++++++++++ gdb/tui/tui-disasm.c | 2 +- gdb/tui/tui-win.c | 16 +++++-- 6 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.S create mode 100644 gdb/testsuite/gdb.tui/tui-layout-asm-short-prog.exp
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>