* 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(-)
@@ -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);
}
@@ -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);
+}
+
+
/* 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;
+ }
+}
+
void
_initialize_source (void)
@@ -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 *);