diff mbox

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

Message ID 20200206152427.1859681-1-shahab.vahedi@gmail.com
State New
Headers show

Commit Message

Shahab Vahedi Feb. 6, 2020, 3:24 p.m. UTC
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.
---
 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

Comments

Andrew Burgess Feb. 6, 2020, 4:42 p.m. UTC | #1
* 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
>
diff mbox

Patch

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;