Re: [PATCH v4] gdb: Catch exceptions if the source file is not found

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Shahab Vahedi <shahab.vahedi@gmail.com> [2020-02-06 16:24:27 +0100]:

> From: Shahab Vahedi <shahab@synopsys.com>
> 
> The source_cache::ensure method may throw an exception through
> the invocation of source_cache::get_plain_source_lines. This
> happens when the source file is not found. The expected behaviour
> of "ensure" is only returning "true" or "false" according to the
> documentation in the header file.
> 
> So far, if gdb is in source layout and a file is missing, you see
> some outputs like below:
> 
>  ,---------------------------------------------.
>  | test.c file is loaded in the source window. |
>  |                                             |
>  | int main()                                  |
>  | ...                                         |
>  |---------------------------------------------|
>  | Remote debugging using :1234                |
>  | __start () at /path/to/crt0.S:141           |
>  | /path/to/crt0.S: No such file or directory. |
>  | (gdb) p/x $pc                               |
>  | $1 = 0x124                                  |
>  | (gdb) n                                     |
>  | /path/to/crt0.S: No such file or directory. |
>  | (gdb) p/x $pc                               |
>  | $2 = 0x128                                  |
>  | (gdb) [pressing arrow-down key]             |
>  | (gdb) terminate called after throwing an    |
>  |       instance of 'gdb_exception_error'     |
>  `---------------------------------------------'
> Other issues have been encountered as well [1].
> 
> The patch from Pedro [2] which is about preventing exceptions
> from crossing the "readline" mitigates the situation by not
> causing gdb crash, but still there are lots of errors printed:
> 
>  ,---------------------------------------------.
>  | test.c file is loaded in the source window. |
>  |                                             |
>  | int main()                                  |
>  | ...                                         |
>  |---------------------------------------------|
>  | Remote debugging using :1234                |
>  | __start () at /path/to/crt0.S:141           |
>  | /path/to/crt0.S: No such file or directory. |
>  | (gdb) [pressing arrow-down key]             |
>  | /path/to/crt0.S: No such file or directory. |
>  | (gdb) [pressing arrow-down key]             |
>  | /path/to/crt0.S: No such file or directory. |
>  | (gdb) [pressing arrow-up key]               |
>  | /path/to/crt0.S: No such file or directory. |
>  `---------------------------------------------'
> 
> With the changes of this patch, the behavior is like:
>  ,---------------------------------------------.
>  | initially, source window is empty because   |
>  | crt0.S is not found and according to the    |
>  | program counter that is the piece of code   |
>  | being executed.                             |
>  |                                             |
>  | later, when we break at main (see commands  |
>  | below), this window will be filled with the |
>  | the contents of test.c file.                |
>  |---------------------------------------------|
>  | Remote debugging using :1234                |
>  | __start () at /path/to/crt0.S:141           |
>  | (gdb) p/x $pc                               |
>  | $1 = 0x124                                  |
>  | (gdb) n                                     |
>  | (gdb) p/x $pc                               |
>  | $2 = 0x128                                  |
>  | (gdb) b main                                |
>  | Breakpoint 1 at 0x334: file test.c, line 8. |
>  | (gdb) cont                                  |
>  | Continuing.                                 |
>  | Breakpoint 1, main () at hello.c:8          |
>  | (gdb) n                                     |
>  | (gdb)                                       |
>  `---------------------------------------------'
> 
> There is no crash and the error message is completely
> gone. Maybe it is good practice that the error is
> shown inside the source window.
> 
> I tested this change against gdb.base/list-missing-source.exp
> and there was no regression.
> 
> [1]
> It has also been observed in the past that the register
> values are not transferred from qemu's gdb stub, see:
> https://github.com/foss-for-synopsys-dwc-arc-processors/toolchain/issues/226
> 
> [2]
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2f267673f0fdee9287e6d404ecd4f2d29da0d2f2
> 
> gdb/ChangeLog:
> 
> 	* source-cache.c (source_cache::ensure): Surround
> 	get_plain_source_lines with a try/catch.
> 	(source_cache::get_line_charpos): Get rid of try/catch
> 	and only check for the return value of "ensure".
> 	* tui/tui-source.c (tui_source_window::set_contents):
> 	Simplify "nlines" calculation.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.tui/tui-missing-src.exp: Add the "missing source
> 	file" test for the TUI.

LGTM.  I see you have write access now, so feel free to go ahead and
push this.

Thanks,
Andrew



> ---
>  gdb/source-cache.c                        | 39 ++++-----
>  gdb/testsuite/gdb.tui/tui-missing-src.exp | 97 +++++++++++++++++++++++
>  gdb/tui/tui-source.c                      |  2 +-
>  3 files changed, 119 insertions(+), 19 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.tui/tui-missing-src.exp
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 71277ecc9b..9196e3a19e 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -176,7 +176,16 @@ source_cache::ensure (struct symtab *s)
>  	}
>      }
>  
> -  std::string contents = get_plain_source_lines (s, fullname);
> +  std::string contents;
> +  try
> +    {
> +      contents = get_plain_source_lines (s, fullname);
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      /* If 's' is not found, an exception is thrown.  */
> +      return false;
> +    }
>  
>    if (source_styling && gdb_stdout->can_emit_style_escape ())
>      {
> @@ -241,26 +250,20 @@ bool
>  source_cache::get_line_charpos (struct symtab *s,
>  				const std::vector<off_t> **offsets)
>  {
> -  try
> -    {
> -      std::string fullname = symtab_to_fullname (s);
> -
> -      auto iter = m_offset_cache.find (fullname);
> -      if (iter == m_offset_cache.end ())
> -	{
> -	  ensure (s);
> -	  iter = m_offset_cache.find (fullname);
> -	  /* cache_source_text ensured this was entered.  */
> -	  gdb_assert (iter != m_offset_cache.end ());
> -	}
> +  std::string fullname = symtab_to_fullname (s);
>  
> -      *offsets = &iter->second;
> -      return true;
> -    }
> -  catch (const gdb_exception_error &e)
> +  auto iter = m_offset_cache.find (fullname);
> +  if (iter == m_offset_cache.end ())
>      {
> -      return false;
> +      if (!ensure (s))
> +	return false;
> +      iter = m_offset_cache.find (fullname);
> +      /* cache_source_text ensured this was entered.  */
> +      gdb_assert (iter != m_offset_cache.end ());
>      }
> +
> +  *offsets = &iter->second;
> +  return true;
>  }
>  
>  /* A helper function that extracts the desired source lines from TEXT,
> diff --git a/gdb/testsuite/gdb.tui/tui-missing-src.exp b/gdb/testsuite/gdb.tui/tui-missing-src.exp
> new file mode 100644
> index 0000000000..2d9a851bec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.tui/tui-missing-src.exp
> @@ -0,0 +1,97 @@
> +# 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/>.
> +
> +# This test checks if gdb can handle missing source files gracefully.
> +# Testing steps are:
> +# 1. Have a main() in main.c that calls an external function f2().
> +# 2. Have f2() implemented in f2.c.
> +# 3. Build the two files into one executable.
> +# 4. Remove main.c.
> +# 5. Open the executable inside gdb while having gdb in source layout.
> +#    No source is found for the moment.
> +# 6. After a little bit of playing, we enter f2() and now the source
> +#    layout must show the contents of f2.c.
> +# 7. Going back to main() shall result in no contents again.
> +
> +load_lib "tuiterm.exp"
> +
> +standard_testfile
> +
> +set mainfile [standard_output_file main.c]
> +set f2file   [standard_output_file f2.c]
> +set srcfiles [list $mainfile $f2file]
> +
> +# Step 1: Write the main.c file into the output directory.
> +# This file will be removed after compilation.
> +set fd [open "$mainfile" w]
> +puts $fd {
> +extern int f2(int);
> +int
> +main ()
> +{
> +  int a = 4;
> +  a = f2(a);
> +  return a - a;
> +}
> +}
> +close $fd
> +
> +# Step 2: Write the f2.c file into the output directory.
> +set fd [open "$f2file" w]
> +puts $fd {
> +int
> +f2 (int x)
> +{
> +  x <<= 1;
> +  return x+5;
> +}
> +}
> +close $fd
> +
> +# Step 3: Compile the source files.
> +if  { [gdb_compile "${srcfiles}" "${binfile}" \
> +	   executable {debug additional_flags=-O0}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +# Step 4: Remove the main.c file.
> +file delete $mainfile
> +
> +# Step 5: Load the executable into GDB.
> +# There shall be no source content.
> +Term::clean_restart 24 80 $testfile
> +if {![Term::enter_tui]} {
> +    unsupported "TUI not supported"
> +}
> +# There must exist a source layout with the size 80x15 and
> +# there should be nothing in it.
> +Term::check_box_contents "check source box is empty" \
> +    0 0 80 15 "No Source Available"
> +
> +# Step 6: Go to main and after one next, enter f2().
> +Term::command "set pagination off"
> +Term::command "start"
> +Term::command "next"
> +Term::command "step"
> +Term::check_contents "checking if inside f2 ()" "f2 \\(x=4\\)"
> +Term::check_box_contents "f2.c must be displayed in source window" \
> +    0 0 80 15 "return x\\+5"
> +
> +# Step 7: Back in main
> +Term::command "finish"
> +Term::check_box_contents "check source box is empty after return" \
> +    0 0 80 15 "No Source Available"
> +Term::check_contents "Back in main" "Value returned is .* 13"
> diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
> index 912eaa4544..3c7a8e1000 100644
> --- a/gdb/tui/tui-source.c
> +++ b/gdb/tui/tui-source.c
> @@ -55,7 +55,7 @@ tui_source_window::set_contents (struct gdbarch *arch,
>    line_width = width - TUI_EXECINFO_SIZE - 1;
>    /* Take hilite (window border) into account, when
>       calculating the number of lines.  */
> -  nlines = (line_no + (height - 2)) - line_no;
> +  nlines = height - 2;
>  
>    std::string srclines;
>    const std::vector<off_t> *offsets;
> -- 
> 2.25.0
>