This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[committed] Fix ICE for missing header fix-it hints with overlarge #line directives (PR c/84852)
- From: David Malcolm <dmalcolm at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org
- Cc: David Malcolm <dmalcolm at redhat dot com>
- Date: Wed, 14 Mar 2018 10:10:29 -0400
- Subject: [committed] Fix ICE for missing header fix-it hints with overlarge #line directives (PR c/84852)
- Authentication-results: sourceware.org; auth=none
PR c/84852 reports an ICE inside diagnostic_show_locus when printing
a diagnostic for a source file with a #line >= 2^31:
#line 7777777777
int foo (void) { return strlen(""); }
where we're attempting to print a fix-it hint at the top of the file
and underline the "strlen" (two "line spans").
The
#line 7777777777
won't fix within the 32-bit linenum_type, and is truncated from
0x1cf977871
to
0xcf977871
i.e. 3482810481 in decimal.
Such a #line is reported by -pedantic and -pedantic-errors, but we
shouldn't ICE.
The ICE is an assertion failure within layout::calculate_line_spans,
where the line spans have not been properly sorted.
The layout_ranges are stored as int, rather than linenum_type,
giving line -812156815 for the error, and line 1 for the fix-it hint.
However, line_span uses linenum_type rather than int.
line_span::comparator compares these values as int, and hence
decides that (linenum_type)3482810481 aka (int)-812156815 is less
than line 1.
This leads to this assertion failing in layout::calculate_line_spans:
1105 gcc_assert (next->m_first_line >= current->m_first_line);
since it isn't the case that 1 >= 3482810481.
The underlying problem is the mix of types for storing line numbers:
in parts of libcpp and diagnostic-show-locus.c we use linenum_type;
in other places (including libcpp's expanded_location) we use int.
I looked at using linenum_type throughout, but doing so turned into
a large patch, so this patch fixes the ICE in a less invasive way
by merely using linenum_type more consistently just within
diagnostic-show-locus.c, and fixing line_span::comparator to properly
handle line numbers (and line number differences) >= 2^31, by using
a new helper function for linenum_type differences, computing the
difference using long long, and using the sign of the difference
(as the difference might not fit in the "int" return type imposed
by qsort).
(The new testcases assume the host's "unsigned int" is 32 bits; is
there anything we support where that isn't the case?)
I can self-approve the libcpp, diagnostic-show-locus.c and input.c
changes.
As part of the selftests, I needed to add ASSERT_GT and ASSERT_LT
to selftest.h; I'm treating those parts of the patch as "obvious".
Successfully bootstrapped and regression-tested on x86_64-pc-linux-gnu;
adds 14 PASS results to gcc.sum.
Committed to trunk as r258526.
gcc/ChangeLog:
PR c/84852
* diagnostic-show-locus.c (class layout_point): Convert m_line
from int to linenum_type.
(line_span::comparator): Use linenum "compare" function when
comparing line numbers.
(test_line_span): New function.
(layout_range::contains_point): Convert param "row" from int to
linenum_type.
(layout_range::intersects_line_p): Likewise.
(layout::will_show_line_p): Likewise.
(layout::print_source_line): Likewise.
(layout::should_print_annotation_line_p): Likewise.
(layout::print_annotation_line): Likewise.
(layout::print_leading_fixits): Likewise.
(layout::annotation_line_showed_range_p): Likewise.
(struct line_corrections): Likewise for field m_row.
(line_corrections::line_corrections): Likewise for param "row".
(layout::print_trailing_fixits): Likewise.
(layout::get_state_at_point): Likewise.
(layout::get_x_bound_for_row): Likewise.
(layout::print_line): Likewise.
(diagnostic_show_locus): Likewise for locals "last_line" and
"row".
(selftest::diagnostic_show_locus_c_tests): Call test_line_span.
* input.c (selftest::test_linenum_comparisons): New function.
(selftest::input_c_tests): Call it.
* selftest.c (selftest::test_assertions): Test ASSERT_GT,
ASSERT_GT_AT, ASSERT_LT, and ASSERT_LT_AT.
* selftest.h (ASSERT_GT): New macro.
(ASSERT_GT_AT): New macro.
(ASSERT_LT): New macro.
(ASSERT_LT_AT): New macro.
gcc/testsuite/ChangeLog:
PR c/84852
* gcc.dg/fixits-pr84852-1.c: New test.
* gcc.dg/fixits-pr84852-2.c: New test.
libcpp/ChangeLog:
* include/line-map.h (compare): New function on linenum_type.
---
gcc/diagnostic-show-locus.c | 105 ++++++++++++++++++++++----------
gcc/input.c | 16 +++++
gcc/selftest.c | 4 ++
gcc/selftest.h | 38 ++++++++++++
gcc/testsuite/gcc.dg/fixits-pr84852-1.c | 25 ++++++++
gcc/testsuite/gcc.dg/fixits-pr84852-2.c | 25 ++++++++
libcpp/include/line-map.h | 12 ++++
7 files changed, 192 insertions(+), 33 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/fixits-pr84852-1.c
create mode 100644 gcc/testsuite/gcc.dg/fixits-pr84852-2.c
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 5eee3cc..bdf608a 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -115,7 +115,7 @@ class layout_point
: m_line (exploc.line),
m_column (exploc.column) {}
- int m_line;
+ linenum_type m_line;
int m_column;
};
@@ -129,8 +129,8 @@ class layout_range
bool show_caret_p,
const expanded_location *caret_exploc);
- bool contains_point (int row, int column) const;
- bool intersects_line_p (int row) const;
+ bool contains_point (linenum_type row, int column) const;
+ bool intersects_line_p (linenum_type row) const;
layout_point m_start;
layout_point m_finish;
@@ -172,16 +172,52 @@ struct line_span
{
const line_span *ls1 = (const line_span *)p1;
const line_span *ls2 = (const line_span *)p2;
- int first_line_diff = (int)ls1->m_first_line - (int)ls2->m_first_line;
- if (first_line_diff)
- return first_line_diff;
- return (int)ls1->m_last_line - (int)ls2->m_last_line;
+ int first_line_cmp = compare (ls1->m_first_line, ls2->m_first_line);
+ if (first_line_cmp)
+ return first_line_cmp;
+ return compare (ls1->m_last_line, ls2->m_last_line);
}
linenum_type m_first_line;
linenum_type m_last_line;
};
+#if CHECKING_P
+
+/* Selftests for line_span. */
+
+static void
+test_line_span ()
+{
+ line_span line_one (1, 1);
+ ASSERT_EQ (1, line_one.get_first_line ());
+ ASSERT_EQ (1, line_one.get_last_line ());
+ ASSERT_FALSE (line_one.contains_line_p (0));
+ ASSERT_TRUE (line_one.contains_line_p (1));
+ ASSERT_FALSE (line_one.contains_line_p (2));
+
+ line_span lines_1_to_3 (1, 3);
+ ASSERT_EQ (1, lines_1_to_3.get_first_line ());
+ ASSERT_EQ (3, lines_1_to_3.get_last_line ());
+ ASSERT_TRUE (lines_1_to_3.contains_line_p (1));
+ ASSERT_TRUE (lines_1_to_3.contains_line_p (3));
+
+ ASSERT_EQ (0, line_span::comparator (&line_one, &line_one));
+ ASSERT_GT (line_span::comparator (&lines_1_to_3, &line_one), 0);
+ ASSERT_LT (line_span::comparator (&line_one, &lines_1_to_3), 0);
+
+ /* A linenum > 2^31. */
+ const linenum_type LARGEST_LINE = 0xffffffff;
+ line_span largest_line (LARGEST_LINE, LARGEST_LINE);
+ ASSERT_EQ (LARGEST_LINE, largest_line.get_first_line ());
+ ASSERT_EQ (LARGEST_LINE, largest_line.get_last_line ());
+
+ ASSERT_GT (line_span::comparator (&largest_line, &line_one), 0);
+ ASSERT_LT (line_span::comparator (&line_one, &largest_line), 0);
+}
+
+#endif /* #if CHECKING_P */
+
/* A class to control the overall layout when printing a diagnostic.
The layout is determined within the constructor.
@@ -207,18 +243,18 @@ class layout
expanded_location get_expanded_location (const line_span *) const;
- void print_line (int row);
+ void print_line (linenum_type row);
private:
- bool will_show_line_p (int row) const;
- void print_leading_fixits (int row);
- void print_source_line (int row, const char *line, int line_width,
+ bool will_show_line_p (linenum_type row) const;
+ void print_leading_fixits (linenum_type row);
+ void print_source_line (linenum_type row, const char *line, int line_width,
line_bounds *lbounds_out);
- bool should_print_annotation_line_p (int row) const;
- void print_annotation_line (int row, const line_bounds lbounds);
- void print_trailing_fixits (int row);
+ bool should_print_annotation_line_p (linenum_type row) const;
+ void print_annotation_line (linenum_type row, const line_bounds lbounds);
+ void print_trailing_fixits (linenum_type row);
- bool annotation_line_showed_range_p (int line, int start_column,
+ bool annotation_line_showed_range_p (linenum_type line, int start_column,
int finish_column) const;
void show_ruler (int max_column) const;
@@ -230,13 +266,13 @@ class layout
bool
get_state_at_point (/* Inputs. */
- int row, int column,
+ linenum_type row, int column,
int first_non_ws, int last_non_ws,
/* Outputs. */
point_state *out_state);
int
- get_x_bound_for_row (int row, int caret_column,
+ get_x_bound_for_row (linenum_type row, int caret_column,
int last_non_ws);
void
@@ -417,7 +453,7 @@ layout_range::layout_range (const expanded_location *start_exploc,
- 'a' indicates a subsequent point *after* the range. */
bool
-layout_range::contains_point (int row, int column) const
+layout_range::contains_point (linenum_type row, int column) const
{
gcc_assert (m_start.m_line <= m_finish.m_line);
/* ...but the equivalent isn't true for the columns;
@@ -478,7 +514,7 @@ layout_range::contains_point (int row, int column) const
/* Does this layout_range contain any part of line ROW? */
bool
-layout_range::intersects_line_p (int row) const
+layout_range::intersects_line_p (linenum_type row) const
{
gcc_assert (m_start.m_line <= m_finish.m_line);
if (row < m_start.m_line)
@@ -940,7 +976,7 @@ layout::maybe_add_location_range (const location_range *loc_range,
/* Return true iff ROW is within one of the line spans for this layout. */
bool
-layout::will_show_line_p (int row) const
+layout::will_show_line_p (linenum_type row) const
{
for (int line_span_idx = 0; line_span_idx < get_num_line_spans ();
line_span_idx++)
@@ -1138,7 +1174,7 @@ layout::calculate_line_spans ()
is its width. */
void
-layout::print_source_line (int row, const char *line, int line_width,
+layout::print_source_line (linenum_type row, const char *line, int line_width,
line_bounds *lbounds_out)
{
m_colorizer.set_normal_text ();
@@ -1201,7 +1237,7 @@ layout::print_source_line (int row, const char *line, int line_width,
i.e. if any of m_layout_ranges contains ROW. */
bool
-layout::should_print_annotation_line_p (int row) const
+layout::should_print_annotation_line_p (linenum_type row) const
{
layout_range *range;
int i;
@@ -1215,7 +1251,7 @@ layout::should_print_annotation_line_p (int row) const
source line. */
void
-layout::print_annotation_line (int row, const line_bounds lbounds)
+layout::print_annotation_line (linenum_type row, const line_bounds lbounds)
{
int x_bound = get_x_bound_for_row (row, m_exploc.column,
lbounds.m_last_non_ws);
@@ -1263,7 +1299,7 @@ layout::print_annotation_line (int row, const line_bounds lbounds)
itself, with a leading '+'. */
void
-layout::print_leading_fixits (int row)
+layout::print_leading_fixits (linenum_type row)
{
for (unsigned int i = 0; i < m_fixit_hints.length (); i++)
{
@@ -1301,7 +1337,7 @@ layout::print_leading_fixits (int row)
the exact range from START_COLUMN to FINISH_COLUMN. */
bool
-layout::annotation_line_showed_range_p (int line, int start_column,
+layout::annotation_line_showed_range_p (linenum_type line, int start_column,
int finish_column) const
{
layout_range *range;
@@ -1552,7 +1588,7 @@ correction::ensure_terminated ()
struct line_corrections
{
- line_corrections (const char *filename, int row)
+ line_corrections (const char *filename, linenum_type row)
: m_filename (filename), m_row (row)
{}
~line_corrections ();
@@ -1560,7 +1596,7 @@ struct line_corrections
void add_hint (const fixit_hint *hint);
const char *m_filename;
- int m_row;
+ linenum_type m_row;
auto_vec <correction *> m_corrections;
};
@@ -1674,7 +1710,7 @@ line_corrections::add_hint (const fixit_hint *hint)
in layout::print_leading_fixits. */
void
-layout::print_trailing_fixits (int row)
+layout::print_trailing_fixits (linenum_type row)
{
/* Build a list of correction instances for the line,
potentially consolidating hints (for the sake of readability). */
@@ -1761,7 +1797,7 @@ layout::print_newline ()
bool
layout::get_state_at_point (/* Inputs. */
- int row, int column,
+ linenum_type row, int column,
int first_non_ws, int last_non_ws,
/* Outputs. */
point_state *out_state)
@@ -1806,7 +1842,7 @@ layout::get_state_at_point (/* Inputs. */
character of source (as determined when printing the source line). */
int
-layout::get_x_bound_for_row (int row, int caret_column,
+layout::get_x_bound_for_row (linenum_type row, int caret_column,
int last_non_ws_column)
{
int result = caret_column + 1;
@@ -1897,7 +1933,7 @@ layout::show_ruler (int max_column) const
consisting of any caret/underlines, then any fixits.
If the source line can't be read, print nothing. */
void
-layout::print_line (int row)
+layout::print_line (linenum_type row)
{
int line_width;
const char *line = location_get_source_line (m_exploc.file, row,
@@ -1977,8 +2013,9 @@ diagnostic_show_locus (diagnostic_context * context,
expanded_location exploc = layout.get_expanded_location (line_span);
context->start_span (context, exploc);
}
- int last_line = line_span->get_last_line ();
- for (int row = line_span->get_first_line (); row <= last_line; row++)
+ linenum_type last_line = line_span->get_last_line ();
+ for (linenum_type row = line_span->get_first_line ();
+ row <= last_line; row++)
layout.print_line (row);
}
@@ -3129,6 +3166,8 @@ test_fixit_deletion_affecting_newline (const line_table_case &case_)
void
diagnostic_show_locus_c_tests ()
{
+ test_line_span ();
+
test_layout_range_for_single_point ();
test_layout_range_for_single_line ();
test_layout_range_for_multiple_lines ();
diff --git a/gcc/input.c b/gcc/input.c
index 081e785..b667576 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1595,6 +1595,21 @@ get_num_source_ranges_for_substring (cpp_reader *pfile,
/* Selftests of location handling. */
+/* Verify that compare() on linenum_type handles comparisons over the full
+ range of the type. */
+
+static void
+test_linenum_comparisons ()
+{
+ linenum_type min_line (0);
+ linenum_type max_line (0xffffffff);
+ ASSERT_EQ (0, compare (min_line, min_line));
+ ASSERT_EQ (0, compare (max_line, max_line));
+
+ ASSERT_GT (compare (max_line, min_line), 0);
+ ASSERT_LT (compare (min_line, max_line), 0);
+}
+
/* Helper function for verifying location data: when location_t
values are > LINE_MAP_MAX_LOCATION_WITH_COLS, they are treated
as having column 0. */
@@ -3528,6 +3543,7 @@ for_each_line_table_case (void (*testcase) (const line_table_case &))
void
input_c_tests ()
{
+ test_linenum_comparisons ();
test_should_have_column_data_p ();
test_unknown_location ();
test_builtins ();
diff --git a/gcc/selftest.c b/gcc/selftest.c
index 0f6c5e4..5709110 100644
--- a/gcc/selftest.c
+++ b/gcc/selftest.c
@@ -288,6 +288,10 @@ test_assertions ()
ASSERT_EQ (1, 1);
ASSERT_EQ_AT (SELFTEST_LOCATION, 1, 1);
ASSERT_NE (1, 2);
+ ASSERT_GT (2, 1);
+ ASSERT_GT_AT (SELFTEST_LOCATION, 2, 1);
+ ASSERT_LT (1, 2);
+ ASSERT_LT_AT (SELFTEST_LOCATION, 1, 2);
ASSERT_STREQ ("test", "test");
ASSERT_STREQ_AT (SELFTEST_LOCATION, "test", "test");
ASSERT_STR_CONTAINS ("foo bar baz", "bar");
diff --git a/gcc/selftest.h b/gcc/selftest.h
index 67d55eb..e3117c6 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -333,6 +333,44 @@ extern int num_passes;
::selftest::fail ((LOC), desc); \
SELFTEST_END_STMT
+/* Evaluate LHS and RHS and compare them with >, calling
+ ::selftest::pass if LHS > RHS,
+ ::selftest::fail otherwise. */
+
+#define ASSERT_GT(LHS, RHS) \
+ ASSERT_GT_AT ((SELFTEST_LOCATION), (LHS), (RHS))
+
+/* Like ASSERT_GT, but treat LOC as the effective location of the
+ selftest. */
+
+#define ASSERT_GT_AT(LOC, LHS, RHS) \
+ SELFTEST_BEGIN_STMT \
+ const char *desc_ = "ASSERT_GT (" #LHS ", " #RHS ")"; \
+ if ((LHS) > (RHS)) \
+ ::selftest::pass ((LOC), desc_); \
+ else \
+ ::selftest::fail ((LOC), desc_); \
+ SELFTEST_END_STMT
+
+/* Evaluate LHS and RHS and compare them with <, calling
+ ::selftest::pass if LHS < RHS,
+ ::selftest::fail otherwise. */
+
+#define ASSERT_LT(LHS, RHS) \
+ ASSERT_LT_AT ((SELFTEST_LOCATION), (LHS), (RHS))
+
+/* Like ASSERT_LT, but treat LOC as the effective location of the
+ selftest. */
+
+#define ASSERT_LT_AT(LOC, LHS, RHS) \
+ SELFTEST_BEGIN_STMT \
+ const char *desc_ = "ASSERT_LT (" #LHS ", " #RHS ")"; \
+ if ((LHS) < (RHS)) \
+ ::selftest::pass ((LOC), desc_); \
+ else \
+ ::selftest::fail ((LOC), desc_); \
+ SELFTEST_END_STMT
+
/* Evaluate EXPECTED and ACTUAL and compare them with strcmp, calling
::selftest::pass if they are equal,
::selftest::fail if they are non-equal. */
diff --git a/gcc/testsuite/gcc.dg/fixits-pr84852-1.c b/gcc/testsuite/gcc.dg/fixits-pr84852-1.c
new file mode 100644
index 0000000..ed88434
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fixits-pr84852-1.c
@@ -0,0 +1,25 @@
+/* This is padding (to avoid the output containing DejaGnu directives). */
+
+/* We need -fdiagnostics-show-caret to trigger the ICE. */
+
+/* { dg-options "-fdiagnostics-show-caret -pedantic-errors -Wno-implicit-function-declaration" } */
+
+#line 3482810481 /* { dg-error "line number out of range" } */
+/* { dg-begin-multiline-output "" }
+ #line 3482810481
+ ^~~~~~~~~~
+ { dg-end-multiline-output "" } */
+
+int foo (void) { return strlen(""); }
+
+/* { dg-warning "incompatible implicit declaration of built-in function 'strlen'" "" { target *-*-* } -812156810 } */
+/* { dg-message "include '<string.h>' or provide a declaration of 'strlen'" "" { target *-*-* } -812156810 } */
+#if 0
+{ dg-begin-multiline-output "" }
++#include <string.h>
+ /* This is padding (to avoid the output containing DejaGnu directives). */
+{ dg-end-multiline-output "" }
+#endif
+
+/* We need this, to consume a stray line marker for the bogus line. */
+/* { dg-regexp ".*fixits-pr84852.c:-812156810:25:" } */
diff --git a/gcc/testsuite/gcc.dg/fixits-pr84852-2.c b/gcc/testsuite/gcc.dg/fixits-pr84852-2.c
new file mode 100644
index 0000000..0674ef5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fixits-pr84852-2.c
@@ -0,0 +1,25 @@
+/* This is padding (to avoid the output containing DejaGnu directives). */
+
+/* We need -fdiagnostics-show-caret to trigger the ICE. */
+
+/* { dg-options "-fdiagnostics-show-caret -pedantic-errors -Wno-implicit-function-declaration" } */
+
+#line 7777777777 /* { dg-error "line number out of range" } */
+/* { dg-begin-multiline-output "" }
+ #line 7777777777
+ ^~~~~~~~~~
+ { dg-end-multiline-output "" } */
+
+int foo (void) { return strlen(""); }
+
+/* { dg-warning "incompatible implicit declaration of built-in function 'strlen'" "" { target *-*-* } -812156810 } */
+/* { dg-message "include '<string.h>' or provide a declaration of 'strlen'" "" { target *-*-* } -812156810 } */
+#if 0
+{ dg-begin-multiline-output "" }
++#include <string.h>
+ /* This is padding (to avoid the output containing DejaGnu directives). */
+{ dg-end-multiline-output "" }
+#endif
+
+/* We need this, to consume a stray line marker for the bogus line. */
+/* { dg-regexp ".*fixits-pr84852-2.c:-812156810:25:" } */
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index f6242fc..d6cf816 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -49,6 +49,18 @@ along with this program; see the file COPYING3. If not see
/* The type of line numbers. */
typedef unsigned int linenum_type;
+/* A function for for use by qsort for comparing line numbers. */
+
+inline int compare (linenum_type lhs, linenum_type rhs)
+{
+ /* Avoid truncation issues by using long long for the comparison,
+ and only consider the sign of the result. */
+ long long diff = (long long)lhs - (long long)rhs;
+ if (diff)
+ return diff > 0 ? 1 : -1;
+ return 0;
+}
+
/* Reason for creating a new line map with linemap_add. LC_ENTER is
when including a new file, e.g. a #include directive in C.
LC_LEAVE is when reaching a file's end. LC_RENAME is when a file
--
1.8.5.3