[PATCH 2/4] gdb: Handle requests to print source lines backward

[PATCH 2/4] gdb: Handle requests to print source lines backward

[Andrew Burgess]

...by which I mean from high line number to low, not, actually
backward character by character!

Commit:

  commit 62f29fda90cf1d5a1899f57ef78452471c707fd6
  Date:   Tue Oct 9 22:21:05 2018 -0600

      Highlight source code using GNU Source Highlight

introduced a regression in the test gdb.linespec/explicit.exp, in
which a request is made to GDB to print a reverse sequence of lines,
from +10 to -10 from the current line number.  The expected behaviour
is that GDB prints nothing.  The above commit changed this so that GDB
now prints:

  Line number 32 out of range; /path/to/gdb/testsuite/gdb.linespec/explicit.c has 71 lines.

which is a little confusing.

This commit fixes the regression, and restores the behaviour that GDB
prints nothing.

While I was passing I noticed a call to `back` on a std::string that I
was concerned could be empty if the request for source lines returns
an empty string.  I don't know if it would be possible for a request
for lines to return an empty string, I guess it should be impossible,
in which case, maybe this should be an assertion, but adding a `empty`
check, seems like an easy and cheap safety net.

gdb/ChangeLog:

	* source.c (print_source_lines_base): Handle requests to print
	reverse line number sequences, and guard against empty lines
	string.
---
 gdb/ChangeLog |  6 ++++++
 gdb/source.c  | 12 +++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/source.c b/gdb/source.c
index e77789c0dba..71da396acc8 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1346,6 +1346,16 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 
   last_source_error = 0;
 
+  /* If the user requested a sequence of lines that seems to go backward
+     (from high to low line numbers) then we don't print anything.
+     The use of '- 1' here instead of '<=' is currently critical, we rely
+     on the undefined wrap around behaviour of 'int' for stopline.  When
+     the use has done: 'set listsize unlimited' then stopline can overflow
+     and appear as MIN_INT.  This is a long-standing bug that needs
+     fixing.  */
+  if (stopline - 1 < line)
+    return;
+
   std::string lines;
   if (!g_source_cache.get_source_lines (s, line, stopline - 1, &lines))
     error (_("Line number %d out of range; %s has %d lines."),
@@ -1392,7 +1402,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
       if (c == '\0')
 	break;
     }
-  if (lines.back () != '\n')
+  if (!lines.empty() && lines.back () != '\n')
     uiout->text ("\n");
 }
 \f
-- 
2.14.5

[PATCH 0/4] Fix some regressions caused by source highlighting patch

[Andrew Burgess]

I noticed that the source highlighting patch 62f29fda90cf1d introduced
a couple of regressions.  These are fixed in patches #1 and #2 of this
series.

However, the fix in patch #2 is not great.  I noticed while preparing
this patch that we currently depend on the behaviour of signed integer
overflow in some cases, so...

Patch #3 is some preparation, then patch #4 removes the signed integer
overflow, and cleans up the fix in patch #2.

--

Andrew Burgess (4):
  gdb: Fix skip of `\r` before `\n` in source output
  gdb: Handle requests to print source lines backward
  gdb: Move declarations from symtab.h to source.h
  gdb: Avoid signed integer overflow when printing source lines

 gdb/ChangeLog       | 43 ++++++++++++++++++++++++
 gdb/cli/cli-cmds.c  | 35 +++++++++----------
 gdb/source.c        | 83 ++++++++++++++++++++++++++++-----------------
 gdb/source.h        | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/symtab.h        | 24 --------------
 gdb/tui/tui-hooks.c |  1 +
 6 files changed, 208 insertions(+), 74 deletions(-)

-- 
2.14.5

[PATCH 4/4] gdb: Avoid signed integer overflow when printing source lines

[Andrew Burgess]

When printing source lines with calls to print_source_lines we need to
pass a start line number and an end line number.  The end line number
is calculated by calling get_lines_to_list and adding this value to
the start line number.  For example this code from list_command:

    print_source_lines (cursal.symtab, first,
                        first + get_lines_to_list (), 0);

The problem is that get_lines_to_list returns a value based on the
GDB setting `set listsize LISTSIZE`.  By default LISTSIZE is 10,
however, its also possible to set LISTSIZE to unlimited, in which
case get_lines_to_list will return INT_MAX.

As the parameter signature for print_source_lines is:

  void print_source_lines (struct symtab *, int, int,
                           print_source_lines_flags);

and `first` in the above code is an `int`, then when LISTSIZE is
`unlimited` the above code will result in signed integer overflow,
which is undefined.

The solution in this patch is a new class source_lines_range that can
be constructed from a single line number and a direction (forward or
backward).  The range is then constructed from the line number and the
value of get_lines_to_list.

gdb/ChangeLog:

	* cli/cli-cmds.c (list_command): Pass a source_lines_range to
	print_source_lines.
	* source.c (print_source_lines_base): Update line number check.
	(print_source_lines): New function.
	(source_lines_range::source_lines_range): New function.
	* source.h (class source_lines_range): New class.
	(print_source_lines): New declaration.
---
 gdb/ChangeLog      | 10 ++++++++++
 gdb/cli/cli-cmds.c | 35 ++++++++++++++++-------------------
 gdb/source.c       | 48 +++++++++++++++++++++++++++++++++++++++++-------
 gdb/source.h       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 26 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 9e09c05513c..70d196776ca 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -895,14 +895,13 @@ list_command (const char *arg, int from_tty)
 	      && get_lines_to_list () == 1 && first > 1)
 	    first -= 1;
 
-	  print_source_lines (cursal.symtab, first,
-			      first + get_lines_to_list (), 0);
+	  print_source_lines (cursal.symtab, source_lines_range (first), 0);
 	}
 
       /* "l" or "l +" lists next ten lines.  */
       else if (arg == NULL || arg[0] == '+')
-	print_source_lines (cursal.symtab, cursal.line,
-			    cursal.line + get_lines_to_list (), 0);
+	print_source_lines (cursal.symtab,
+			    source_lines_range (cursal.line), 0);
 
       /* "l -" lists previous ten lines, the ones before the ten just
 	 listed.  */
@@ -911,10 +910,9 @@ list_command (const char *arg, int from_tty)
 	  if (get_first_line_listed () == 1)
 	    error (_("Already at the start of %s."),
 		   symtab_to_filename_for_display (cursal.symtab));
-	  print_source_lines (cursal.symtab,
-			      std::max (get_first_line_listed ()
-					- get_lines_to_list (), 1),
-			      get_first_line_listed (), 0);
+	  source_lines_range range (get_first_line_listed (),
+				    source_lines_range::direction::backward);
+	  print_source_lines (cursal.symtab, range, 0);
 	}
 
       return;
@@ -1059,9 +1057,11 @@ list_command (const char *arg, int from_tty)
   if (dummy_beg && sal_end.symtab == 0)
     error (_("No default source file yet.  Do \"help list\"."));
   if (dummy_beg)
-    print_source_lines (sal_end.symtab,
-			std::max (sal_end.line - (get_lines_to_list () - 1), 1),
-			sal_end.line + 1, 0);
+    {
+      source_lines_range range (sal_end.line + 1,
+				source_lines_range::direction::backward);
+      print_source_lines (sal_end.symtab, range, 0);
+    }
   else if (sal.symtab == 0)
     error (_("No default source file yet.  Do \"help list\"."));
   else if (no_end)
@@ -1074,17 +1074,14 @@ list_command (const char *arg, int from_tty)
 	    first_line = 1;
 	  if (sals.size () > 1)
 	    print_sal_location (sal);
-	  print_source_lines (sal.symtab,
-			      first_line,
-			      first_line + get_lines_to_list (),
-			      0);
+	  print_source_lines (sal.symtab, source_lines_range (first_line), 0);
 	}
     }
+  else if (dummy_end)
+    print_source_lines (sal.symtab, source_lines_range (sal.line), 0);
   else
-    print_source_lines (sal.symtab, sal.line,
-			(dummy_end
-			 ? sal.line + get_lines_to_list ()
-			 : sal_end.line + 1),
+    print_source_lines (sal.symtab,
+			source_lines_range (sal.line, (sal_end.line + 1)),
 			0);
 }
 
diff --git a/gdb/source.c b/gdb/source.c
index f865c8a9508..f7ce18a0532 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1331,13 +1331,8 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
   last_source_error = 0;
 
   /* If the user requested a sequence of lines that seems to go backward
-     (from high to low line numbers) then we don't print anything.
-     The use of '- 1' here instead of '<=' is currently critical, we rely
-     on the undefined wrap around behaviour of 'int' for stopline.  When
-     the use has done: 'set listsize unlimited' then stopline can overflow
-     and appear as MIN_INT.  This is a long-standing bug that needs
-     fixing.  */
-  if (stopline - 1 < line)
+     (from high to low line numbers) then we don't print anything.  */
+  if (stopline <= line)
     return;
 
   std::string lines;
@@ -1399,6 +1394,18 @@ print_source_lines (struct symtab *s, int line, int stopline,
 {
   print_source_lines_base (s, line, stopline, flags);
 }
+
+/* See source.h.  */
+
+void
+print_source_lines (struct symtab *s, source_lines_range line_range,
+		    print_source_lines_flags flags)
+{
+  print_source_lines_base (s, line_range.startline (),
+			   line_range.stopline (), flags);
+}
+
+
 \f
 /* Print info on range of pc's in a specified line.  */
 
@@ -1822,6 +1829,33 @@ set_substitute_path_command (const char *args, int from_tty)
   forget_cached_source_info ();
 }
 
+/* See source.h.  */
+
+source_lines_range::source_lines_range (int startline,
+					source_lines_range::direction dir)
+{
+  if (dir == source_lines_range::direction::forward)
+    {
+      LONGEST end = static_cast <LONGEST> (startline) + get_lines_to_list ();
+
+      if (end > INT_MAX)
+	end = INT_MAX;
+
+      m_startline = startline;
+      m_stopline = static_cast <int> (end);
+    }
+  else
+    {
+      LONGEST start = static_cast <LONGEST> (startline) - get_lines_to_list ();
+
+      if (start < 1)
+	start = 1;
+
+      m_startline = static_cast <int> (start);
+      m_stopline = startline;
+    }
+}
+
 \f
 void
 _initialize_source (void)
diff --git a/gdb/source.h b/gdb/source.h
index fcd83daafcd..3e08238e883 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -157,6 +157,54 @@ DEF_ENUM_FLAGS_TYPE (enum print_source_lines_flag, print_source_lines_flags);
 extern void print_source_lines (struct symtab *s, int line, int stopline,
 				print_source_lines_flags flags);
 
+/* Wrap up the logic to build a line number range for passing to
+   print_source_lines when using get_lines_to_list.  An instance of this
+   class can be built from a single line number and a direction (forward or
+   backward) the range is then computed using get_lines_to_list.  */
+class source_lines_range
+{
+public:
+  /* When constructing the range from a single line number, does the line
+     range extend forward, or backward.  */
+  enum class direction
+  {
+   forward,
+   backward
+  };
+
+  /* Construct a SOURCE_LINES_RANGE starting at STARTLINE and extending in
+   direction DIR.  The number of lines is from GET_LINES_TO_LIST.  If the
+   direction is backward then the start is actually (STARTLINE -
+   GET_LINES_TO_LIST).  There is also logic in place to ensure the start
+   is always 1 or more, and the end will be at most INT_MAX.  */
+  source_lines_range (int startline, direction dir = direction::forward);
+
+  /* Construct a SOURCE_LINES_RANGE from STARTLINE to STOPLINE.  */
+  source_lines_range (int startline, int stopline)
+    : m_startline (startline),
+      m_stopline (stopline)
+  { /* Nothing.  */ }
+
+  /* Return the line to start listing from.  */
+  int startline () const
+  { return m_startline; }
+
+  /* Return the line after the last line that should be listed.  */
+  int stopline () const
+  { return m_stopline; }
+
+private:
+
+  /* The start and end of the range.  */
+  int m_startline;
+  int m_stopline;
+};
+
+/* Variation of previous print_source_lines that takes a range instead of a
+   start and end line number.  */
+extern void print_source_lines (struct symtab *s, source_lines_range r,
+				print_source_lines_flags flags);
+
 /* Forget line positions and file names for the symtabs in a
    particular objfile.  */
 extern void forget_cached_source_info_for_objfile (struct objfile *);
-- 
2.14.5

[PATCH 3/4] gdb: Move declarations from symtab.h to source.h

[Andrew Burgess]

Declarations for functions in source.c are split between source.h and
symtab.h.  This commit moves the small number that are in symtab.h
into source.h.  There's just one file that needs to add an include of
source.h in order to build.

I've moved the function header comments from source.c to source.h
inline with the recommended GDB style.

gdb/ChangeLog:

	* source.c (select_source_symtab): Move header comment to
	declaration in source.h.
	(forget_cached_source_info_for_objfile): Likewise.
	(forget_cached_source_info): Likewise.
	(identify_source_line): Likewise.
	* source.h (identify_source_line): Move declaration from symtab.h
	and add comment from source.c
	(print_source_lines): Likewise.
	(forget_cached_source_info_for_objfile): Likewise.
	(forget_cached_source_info): Likewise.
	(select_source_symtab): Likewise.
	(enum print_source_lines_flag): Move definition from symtab.h.
	* symtab.h (identify_source_line): Move declaration to source.h.
	(print_source_lines): Likewise.
	(forget_cached_source_info_for_objfile): Likewise.
	(forget_cached_source_info): Likewise.
	(select_source_symtab): Likewise.
	(enum print_source_lines_flag): Move definition to source.h.
	* tui/tui-hooks.c: Add 'source.h' include.
---
 gdb/ChangeLog       | 22 ++++++++++++++++++++++
 gdb/source.c        | 30 ++++++------------------------
 gdb/source.h        | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/symtab.h        | 24 ------------------------
 gdb/tui/tui-hooks.c |  1 +
 5 files changed, 77 insertions(+), 48 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index 71da396acc8..f865c8a9508 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -233,13 +233,7 @@ clear_current_source_symtab_and_line (void)
   current_source_line = 0;
 }
 
-/* Set the source file default for the "list" command to be S.
-
-   If S is NULL, and we don't have a default, find one.  This
-   should only be called when the user actually tries to use the
-   default, since we produce an error if we can't find a reasonable
-   default.  Also, since this can cause symbols to be read, doing it
-   before we need to would make things slower than necessary.  */
+/* See source.h.  */
 
 void
 select_source_symtab (struct symtab *s)
@@ -351,8 +345,7 @@ show_directories_command (struct ui_file *file, int from_tty,
   show_directories_1 (NULL, from_tty);
 }
 
-/* Forget line positions and file names for the symtabs in a
-   particular objfile.  */
+/* See source.h.  */
 
 void
 forget_cached_source_info_for_objfile (struct objfile *objfile)
@@ -378,9 +371,7 @@ forget_cached_source_info_for_objfile (struct objfile *objfile)
     objfile->sf->qf->forget_cached_source_info (objfile);
 }
 
-/* Forget what we learned about line positions in source files, and
-   which directories contain them; must check again now since files
-   may be found in a different directory now.  */
+/* See source.h.  */
 
 void
 forget_cached_source_info (void)
@@ -1226,14 +1217,7 @@ get_filename_and_charpos (struct symtab *s, char **fullname)
     find_source_lines (s, desc.get ());
 }
 
-/* Print text describing the full name of the source file S
-   and the line number LINE and its corresponding character position.
-   The text starts with two Ctrl-z so that the Emacs-GDB interface
-   can easily find it.
-
-   MID_STATEMENT is nonzero if the PC is not at the beginning of that line.
-
-   Return 1 if successful, 0 if could not find the file.  */
+/* See source.h.  */
 
 int
 identify_source_line (struct symtab *s, int line, int mid_statement,
@@ -1406,10 +1390,8 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
     uiout->text ("\n");
 }
 \f
-/* Show source lines from the file of symtab S, starting with line
-   number LINE and stopping before line number STOPLINE.  If this is
-   not the command line version, then the source is shown in the source
-   window otherwise it is simply printed.  */
+
+/* See source.h.  */
 
 void
 print_source_lines (struct symtab *s, int line, int stopline,
diff --git a/gdb/source.h b/gdb/source.h
index 05148f37c75..fcd83daafcd 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -127,4 +127,52 @@ extern void clear_current_source_symtab_and_line (void);
 
 /* Add a source path substitution rule.  */
 extern void add_substitute_path_rule (char *, char *);
+
+/* Print text describing the full name of the source file S
+   and the line number LINE and its corresponding character position.
+   The text starts with two Ctrl-z so that the Emacs-GDB interface
+   can easily find it.
+
+   MID_STATEMENT is nonzero if the PC is not at the beginning of that line.
+
+   Return 1 if successful, 0 if could not find the file.  */
+extern int identify_source_line (struct symtab *s, int line,
+				 int mid_statement, CORE_ADDR pc);
+
+/* Flags passed as 4th argument to print_source_lines.  */
+enum print_source_lines_flag
+  {
+    /* Do not print an error message.  */
+    PRINT_SOURCE_LINES_NOERROR = (1 << 0),
+
+    /* Print the filename in front of the source lines.  */
+    PRINT_SOURCE_LINES_FILENAME = (1 << 1)
+  };
+DEF_ENUM_FLAGS_TYPE (enum print_source_lines_flag, print_source_lines_flags);
+
+/* Show source lines from the file of symtab S, starting with line
+   number LINE and stopping before line number STOPLINE.  If this is
+   not the command line version, then the source is shown in the source
+   window otherwise it is simply printed.  */
+extern void print_source_lines (struct symtab *s, int line, int stopline,
+				print_source_lines_flags flags);
+
+/* Forget line positions and file names for the symtabs in a
+   particular objfile.  */
+extern void forget_cached_source_info_for_objfile (struct objfile *);
+
+/* Forget what we learned about line positions in source files, and
+   which directories contain them; must check again now since files
+   may be found in a different directory now.  */
+extern void forget_cached_source_info (void);
+
+/* Set the source file default for the "list" command to be S.
+
+   If S is NULL, and we don't have a default, find one.  This
+   should only be called when the user actually tries to use the
+   default, since we produce an error if we can't find a reasonable
+   default.  Also, since this can cause symbols to be read, doing it
+   before we need to would make things slower than necessary.  */
+extern void select_source_symtab (struct symtab *s);
+
 #endif
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 414d167f3e9..c2d8a69cf9f 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1882,30 +1882,6 @@ extern void resolve_sal_pc (struct symtab_and_line *);
 
 extern void clear_solib (void);
 
-/* source.c */
-
-extern int identify_source_line (struct symtab *, int, int, CORE_ADDR);
-
-/* Flags passed as 4th argument to print_source_lines.  */
-
-enum print_source_lines_flag
-  {
-    /* Do not print an error message.  */
-    PRINT_SOURCE_LINES_NOERROR = (1 << 0),
-
-    /* Print the filename in front of the source lines.  */
-    PRINT_SOURCE_LINES_FILENAME = (1 << 1)
-  };
-DEF_ENUM_FLAGS_TYPE (enum print_source_lines_flag, print_source_lines_flags);
-
-extern void print_source_lines (struct symtab *, int, int,
-				print_source_lines_flags);
-
-extern void forget_cached_source_info_for_objfile (struct objfile *);
-extern void forget_cached_source_info (void);
-
-extern void select_source_symtab (struct symtab *);
-
 /* The reason we're calling into a completion match list collector
    function.  */
 enum class complete_symbol_mode
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index b8c18a3216d..98c6fd651fa 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -33,6 +33,7 @@
 #include "ui-out.h"
 #include "top.h"
 #include "observable.h"
+#include "source.h"
 #include <unistd.h>
 #include <fcntl.h>
 
-- 
2.14.5

[PATCH 1/4] gdb: Fix skip of `\r` before `\n` in source output

[Andrew Burgess]

In this commit:

  commit 62f29fda90cf1d5a1899f57ef78452471c707fd6
  Date:   Tue Oct 9 22:21:05 2018 -0600

      Highlight source code using GNU Source Highlight

A bug was introduced such that when displaying source code from a file
with lines `\r\n` GDB would print `^M` at the end of each line.

This caused a regression on the test gdb.fortran/nested-funcs.exp,
which happens to have `\r\n` line endings.

gdb/ChangeLog:

	* source.c (print_source_lines_base): Fix skip of '\r' if next
	character is '\n'.
---
 gdb/ChangeLog | 5 +++++
 gdb/source.c  | 7 +------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index ad6c6466b44..e77789c0dba 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1379,12 +1379,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 	  else if (c == '\r')
 	    {
 	      /* Skip a \r character, but only before a \n.  */
-	      if (iter[1] == '\n')
-		{
-		  ++iter;
-		  c = '\n';
-		}
-	      else
+	      if (*iter != '\n')
 		printf_filtered ("^%c", c + 0100);
 	    }
 	  else
-- 
2.14.5

Re: [PATCH 1/4] gdb: Fix skip of `\r` before `\n` in source output

[Tom Tromey]

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

Andrew> This caused a regression on the test gdb.fortran/nested-funcs.exp,
Andrew> which happens to have `\r\n` line endings.

Andrew> gdb/ChangeLog:

Andrew> 	* source.c (print_source_lines_base): Fix skip of '\r' if next
Andrew> 	character is '\n'.

Thanks,  This is ok.

Tom

Re: [PATCH 3/4] gdb: Move declarations from symtab.h to source.h

[Tom Tromey]

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

Andrew> Declarations for functions in source.c are split between source.h and
Andrew> symtab.h.  This commit moves the small number that are in symtab.h
Andrew> into source.h.  There's just one file that needs to add an include of
Andrew> source.h in order to build.

Andrew> I've moved the function header comments from source.c to source.h
Andrew> inline with the recommended GDB style.

Thanks.  This is a nice cleanup.  Please check it in.

Tom

Re: [PATCH 2/4] gdb: Handle requests to print source lines backward

[Tom Tromey]

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

Andrew> This commit fixes the regression, and restores the behaviour that GDB
Andrew> prints nothing.

Andrew> While I was passing I noticed a call to `back` on a std::string that I
Andrew> was concerned could be empty if the request for source lines returns
Andrew> an empty string.  I don't know if it would be possible for a request
Andrew> for lines to return an empty string, I guess it should be impossible,
Andrew> in which case, maybe this should be an assertion, but adding a `empty`
Andrew> check, seems like an easy and cheap safety net.

Seems reasonable.

Andrew> 	* source.c (print_source_lines_base): Handle requests to print
Andrew> 	reverse line number sequences, and guard against empty lines
Andrew> 	string.

This is ok...

Andrew> +  /* If the user requested a sequence of lines that seems to go backward
Andrew> +     (from high to low line numbers) then we don't print anything.
Andrew> +     The use of '- 1' here instead of '<=' is currently critical, we rely
Andrew> +     on the undefined wrap around behaviour of 'int' for stopline.  When
Andrew> +     the use has done: 'set listsize unlimited' then stopline can overflow
Andrew> +     and appear as MIN_INT.  This is a long-standing bug that needs
Andrew> +     fixing.  */

... assuming this was the previous status quo.

Tom

Re: [PATCH 4/4] gdb: Avoid signed integer overflow when printing source lines

[Tom Tromey]

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

Andrew> The solution in this patch is a new class source_lines_range that can
Andrew> be constructed from a single line number and a direction (forward or
Andrew> backward).  The range is then constructed from the line number and the
Andrew> value of get_lines_to_list.

Sorry I didn't get to this the other night.

Andrew> +      source_lines_range range (sal_end.line + 1,
Andrew> +				source_lines_range::direction::backward);

Normally I think gdb uses upper case for enumerators.

Andrew> +  /* When constructing the range from a single line number, does the line
Andrew> +     range extend forward, or backward.  */
Andrew> +  enum class direction
Andrew> +  {
Andrew> +   forward,
Andrew> +   backward
Andrew> +  };

For an enum that's already nested in a class, I would probably just use
plain "enum" and not "enum class", since IMO the extra namespacing just
results in noise -- "source_lines_range::backward" seems just as clear,
but shorter, than "source_lines_range::direction::backward".

I don't insist on it though, I just thought I'd throw this out there to
see what others think.  As far as I know we don't have a rule about
this.

Andrew> +  /* Construct a SOURCE_LINES_RANGE starting at STARTLINE and extending in
Andrew> +   direction DIR.  The number of lines is from GET_LINES_TO_LIST.  If the
Andrew> +   direction is backward then the start is actually (STARTLINE -
Andrew> +   GET_LINES_TO_LIST).  There is also logic in place to ensure the start
Andrew> +   is always 1 or more, and the end will be at most INT_MAX.  */
Andrew> +  source_lines_range (int startline, direction dir = direction::forward);

I think this should be marked "explicit".  I guess it could go either
way but seeing as the patch uses it in an explicit way, and since
explicit is generally better...

Tom

Re: [PATCH 4/4] gdb: Avoid signed integer overflow when printing source lines

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2019-01-08 18:08:27 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> The solution in this patch is a new class source_lines_range that can
> Andrew> be constructed from a single line number and a direction (forward or
> Andrew> backward).  The range is then constructed from the line number and the
> Andrew> value of get_lines_to_list.
> 
> Sorry I didn't get to this the other night.
> 
> Andrew> +      source_lines_range range (sal_end.line + 1,
> Andrew> +				source_lines_range::direction::backward);
> 
> Normally I think gdb uses upper case for enumerators.
> 
> Andrew> +  /* When constructing the range from a single line number, does the line
> Andrew> +     range extend forward, or backward.  */
> Andrew> +  enum class direction
> Andrew> +  {
> Andrew> +   forward,
> Andrew> +   backward
> Andrew> +  };
> 
> For an enum that's already nested in a class, I would probably just use
> plain "enum" and not "enum class", since IMO the extra namespacing just
> results in noise -- "source_lines_range::backward" seems just as clear,
> but shorter, than "source_lines_range::direction::backward".
> 
> I don't insist on it though, I just thought I'd throw this out there to
> see what others think.  As far as I know we don't have a rule about
> this.
> 
> Andrew> +  /* Construct a SOURCE_LINES_RANGE starting at STARTLINE and extending in
> Andrew> +   direction DIR.  The number of lines is from GET_LINES_TO_LIST.  If the
> Andrew> +   direction is backward then the start is actually (STARTLINE -
> Andrew> +   GET_LINES_TO_LIST).  There is also logic in place to ensure the start
> Andrew> +   is always 1 or more, and the end will be at most INT_MAX.  */
> Andrew> +  source_lines_range (int startline, direction dir = direction::forward);
> 
> I think this should be marked "explicit".  I guess it could go either
> way but seeing as the patch uses it in an explicit way, and since
> explicit is generally better...

Thanks for the review.

This iteration has:

  - Enum values with CAPITAL letters,
  - No longer using an enum class,
  - Added use of explicit on constructors.

Thanks,
Andrew

--

gdb: Avoid signed integer overflow when printing source lines

When printing source lines with calls to print_source_lines we need to
pass a start line number and an end line number.  The end line number
is calculated by calling get_lines_to_list and adding this value to
the start line number.  For example this code from list_command:

    print_source_lines (cursal.symtab, first,
                        first + get_lines_to_list (), 0);

The problem is that get_lines_to_list returns a value based on the
GDB setting `set listsize LISTSIZE`.  By default LISTSIZE is 10,
however, its also possible to set LISTSIZE to unlimited, in which
case get_lines_to_list will return INT_MAX.

As the parameter signature for print_source_lines is:

  void print_source_lines (struct symtab *, int, int,
                           print_source_lines_flags);

and `first` in the above code is an `int`, then when LISTSIZE is
`unlimited` the above code will result in signed integer overflow,
which is undefined.

The solution in this patch is a new class source_lines_range that can
be constructed from a single line number and a direction (forward or
backward).  The range is then constructed from the line number and the
value of get_lines_to_list.

gdb/ChangeLog:

	* cli/cli-cmds.c (list_command): Pass a source_lines_range to
	print_source_lines.
	* source.c (print_source_lines_base): Update line number check.
	(print_source_lines): New function.
	(source_lines_range::source_lines_range): New function.
	* source.h (class source_lines_range): New class.
	(print_source_lines): New declaration.
---
 gdb/ChangeLog      | 10 ++++++++++
 gdb/cli/cli-cmds.c | 35 ++++++++++++++++-------------------
 gdb/source.c       | 48 +++++++++++++++++++++++++++++++++++++++++-------
 gdb/source.h       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 26 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 9e09c05513c..57cfad441c9 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -895,14 +895,13 @@ list_command (const char *arg, int from_tty)
 	      && get_lines_to_list () == 1 && first > 1)
 	    first -= 1;
 
-	  print_source_lines (cursal.symtab, first,
-			      first + get_lines_to_list (), 0);
+	  print_source_lines (cursal.symtab, source_lines_range (first), 0);
 	}
 
       /* "l" or "l +" lists next ten lines.  */
       else if (arg == NULL || arg[0] == '+')
-	print_source_lines (cursal.symtab, cursal.line,
-			    cursal.line + get_lines_to_list (), 0);
+	print_source_lines (cursal.symtab,
+			    source_lines_range (cursal.line), 0);
 
       /* "l -" lists previous ten lines, the ones before the ten just
 	 listed.  */
@@ -911,10 +910,9 @@ list_command (const char *arg, int from_tty)
 	  if (get_first_line_listed () == 1)
 	    error (_("Already at the start of %s."),
 		   symtab_to_filename_for_display (cursal.symtab));
-	  print_source_lines (cursal.symtab,
-			      std::max (get_first_line_listed ()
-					- get_lines_to_list (), 1),
-			      get_first_line_listed (), 0);
+	  source_lines_range range (get_first_line_listed (),
+				    source_lines_range::BACKWARD);
+	  print_source_lines (cursal.symtab, range, 0);
 	}
 
       return;
@@ -1059,9 +1057,11 @@ list_command (const char *arg, int from_tty)
   if (dummy_beg && sal_end.symtab == 0)
     error (_("No default source file yet.  Do \"help list\"."));
   if (dummy_beg)
-    print_source_lines (sal_end.symtab,
-			std::max (sal_end.line - (get_lines_to_list () - 1), 1),
-			sal_end.line + 1, 0);
+    {
+      source_lines_range range (sal_end.line + 1,
+				source_lines_range::BACKWARD);
+      print_source_lines (sal_end.symtab, range, 0);
+    }
   else if (sal.symtab == 0)
     error (_("No default source file yet.  Do \"help list\"."));
   else if (no_end)
@@ -1074,17 +1074,14 @@ list_command (const char *arg, int from_tty)
 	    first_line = 1;
 	  if (sals.size () > 1)
 	    print_sal_location (sal);
-	  print_source_lines (sal.symtab,
-			      first_line,
-			      first_line + get_lines_to_list (),
-			      0);
+	  print_source_lines (sal.symtab, source_lines_range (first_line), 0);
 	}
     }
+  else if (dummy_end)
+    print_source_lines (sal.symtab, source_lines_range (sal.line), 0);
   else
-    print_source_lines (sal.symtab, sal.line,
-			(dummy_end
-			 ? sal.line + get_lines_to_list ()
-			 : sal_end.line + 1),
+    print_source_lines (sal.symtab,
+			source_lines_range (sal.line, (sal_end.line + 1)),
 			0);
 }
 
diff --git a/gdb/source.c b/gdb/source.c
index f865c8a9508..1f10379a071 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1331,13 +1331,8 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
   last_source_error = 0;
 
   /* If the user requested a sequence of lines that seems to go backward
-     (from high to low line numbers) then we don't print anything.
-     The use of '- 1' here instead of '<=' is currently critical, we rely
-     on the undefined wrap around behaviour of 'int' for stopline.  When
-     the use has done: 'set listsize unlimited' then stopline can overflow
-     and appear as MIN_INT.  This is a long-standing bug that needs
-     fixing.  */
-  if (stopline - 1 < line)
+     (from high to low line numbers) then we don't print anything.  */
+  if (stopline <= line)
     return;
 
   std::string lines;
@@ -1399,6 +1394,18 @@ print_source_lines (struct symtab *s, int line, int stopline,
 {
   print_source_lines_base (s, line, stopline, flags);
 }
+
+/* See source.h.  */
+
+void
+print_source_lines (struct symtab *s, source_lines_range line_range,
+		    print_source_lines_flags flags)
+{
+  print_source_lines_base (s, line_range.startline (),
+			   line_range.stopline (), flags);
+}
+
+
 \f
 /* Print info on range of pc's in a specified line.  */
 
@@ -1822,6 +1829,33 @@ set_substitute_path_command (const char *args, int from_tty)
   forget_cached_source_info ();
 }
 
+/* See source.h.  */
+
+source_lines_range::source_lines_range (int startline,
+					source_lines_range::direction dir)
+{
+  if (dir == source_lines_range::FORWARD)
+    {
+      LONGEST end = static_cast <LONGEST> (startline) + get_lines_to_list ();
+
+      if (end > INT_MAX)
+	end = INT_MAX;
+
+      m_startline = startline;
+      m_stopline = static_cast <int> (end);
+    }
+  else
+    {
+      LONGEST start = static_cast <LONGEST> (startline) - get_lines_to_list ();
+
+      if (start < 1)
+	start = 1;
+
+      m_startline = static_cast <int> (start);
+      m_stopline = startline;
+    }
+}
+
 \f
 void
 _initialize_source (void)
diff --git a/gdb/source.h b/gdb/source.h
index fcd83daafcd..f1b5f6e8f7f 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -157,6 +157,54 @@ DEF_ENUM_FLAGS_TYPE (enum print_source_lines_flag, print_source_lines_flags);
 extern void print_source_lines (struct symtab *s, int line, int stopline,
 				print_source_lines_flags flags);
 
+/* Wrap up the logic to build a line number range for passing to
+   print_source_lines when using get_lines_to_list.  An instance of this
+   class can be built from a single line number and a direction (forward or
+   backward) the range is then computed using get_lines_to_list.  */
+class source_lines_range
+{
+public:
+  /* When constructing the range from a single line number, does the line
+     range extend forward, or backward.  */
+  enum direction
+  {
+   FORWARD,
+   BACKWARD
+  };
+
+  /* Construct a SOURCE_LINES_RANGE starting at STARTLINE and extending in
+   direction DIR.  The number of lines is from GET_LINES_TO_LIST.  If the
+   direction is backward then the start is actually (STARTLINE -
+   GET_LINES_TO_LIST).  There is also logic in place to ensure the start
+   is always 1 or more, and the end will be at most INT_MAX.  */
+  explicit source_lines_range (int startline, direction dir = FORWARD);
+
+  /* Construct a SOURCE_LINES_RANGE from STARTLINE to STOPLINE.  */
+  explicit source_lines_range (int startline, int stopline)
+    : m_startline (startline),
+      m_stopline (stopline)
+  { /* Nothing.  */ }
+
+  /* Return the line to start listing from.  */
+  int startline () const
+  { return m_startline; }
+
+  /* Return the line after the last line that should be listed.  */
+  int stopline () const
+  { return m_stopline; }
+
+private:
+
+  /* The start and end of the range.  */
+  int m_startline;
+  int m_stopline;
+};
+
+/* Variation of previous print_source_lines that takes a range instead of a
+   start and end line number.  */
+extern void print_source_lines (struct symtab *s, source_lines_range r,
+				print_source_lines_flags flags);
+
 /* Forget line positions and file names for the symtabs in a
    particular objfile.  */
 extern void forget_cached_source_info_for_objfile (struct objfile *);
-- 
2.14.5

Re: [PATCH 4/4] gdb: Avoid signed integer overflow when printing source lines

[Tom Tromey]

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

Andrew> This iteration has:
Andrew>   - Enum values with CAPITAL letters,
Andrew>   - No longer using an enum class,
Andrew>   - Added use of explicit on constructors.

This is ok.  Thanks for doing this.

Tom