From 082284da9d8069290a749218b6aebecab0da2868 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 14 Mar 2018 13:58:13 +0000 Subject: [PATCH] Fix ICE for missing header fix-it hints with overlarge #line directives (PR c/84852) 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). 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. From-SVN: r258526 --- gcc/ChangeLog | 35 ++++++++ gcc/diagnostic-show-locus.c | 105 ++++++++++++++++-------- gcc/input.c | 16 ++++ gcc/selftest.c | 4 + gcc/selftest.h | 38 +++++++++ gcc/testsuite/ChangeLog | 6 ++ gcc/testsuite/gcc.dg/fixits-pr84852-1.c | 25 ++++++ gcc/testsuite/gcc.dg/fixits-pr84852-2.c | 25 ++++++ libcpp/ChangeLog | 4 + libcpp/include/line-map.h | 12 +++ 10 files changed, 237 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/ChangeLog b/gcc/ChangeLog index fcd11eb7db3..8f81cc392aa 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,38 @@ +2018-03-14 David Malcolm + + 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. + 2018-03-14 Segher Boessenkool PR rtl-optimization/84780 diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 5eee3cc1449..bdf608a08bc 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 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 081e7856916..b6675768722 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 0f6c5e4f38c..5709110c291 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 67d55ebe130..e3117c6bfc4 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/ChangeLog b/gcc/testsuite/ChangeLog index f89a29a9c98..14b8e99a04f 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2018-03-14 David Malcolm + + PR c/84852 + * gcc.dg/fixits-pr84852-1.c: New test. + * gcc.dg/fixits-pr84852-2.c: New test. + 2018-03-14 Thomas Preud'homme * lib/scanasm.exp (scan-assembler-times): Move FAIL debug info into a 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 00000000000..ed88434f672 --- /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 '' or provide a declaration of 'strlen'" "" { target *-*-* } -812156810 } */ +#if 0 +{ dg-begin-multiline-output "" } ++#include + /* 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 00000000000..0674ef54689 --- /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 '' or provide a declaration of 'strlen'" "" { target *-*-* } -812156810 } */ +#if 0 +{ dg-begin-multiline-output "" } ++#include + /* 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/ChangeLog b/libcpp/ChangeLog index 791c364f01b..a66948ff1ac 100644 --- a/libcpp/ChangeLog +++ b/libcpp/ChangeLog @@ -1,3 +1,7 @@ +2018-03-14 David Malcolm + + * include/line-map.h (compare): New function on linenum_type. + 2018-02-28 Jonathan Wakely PR preprocessor/84517 diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index f6242fc675d..d6cf81627cc 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