diff mbox series

c-family: Fix regression in location-overflow-test-1.c [PR97117]

Message ID 20201014155625.2067298-1-ppalka@redhat.com
State New
Headers show
Series c-family: Fix regression in location-overflow-test-1.c [PR97117] | expand

Commit Message

Patrick Palka Oct. 14, 2020, 3:56 p.m. UTC 
The r11-3266 patch that added macro support to -Wmisleading-indentation
accidentally suppressed the column-tracking diagnostic in
get_visual_column in some cases, e.g. in the location-overflow-test-1.c
testcase.

More generally, when all three tokens are on the same line and we've run
out of locations with column info, then their location_t values will be
equal, and we exit early from should_warn_for_misleading_indentation due
to the new check

  /* Give up if the loci are not all distinct.  */
  if (guard_loc == body_loc || body_loc == next_stmt_loc)
    return false;

before we ever call get_visual_column.

[ This new check is needed to detect and give up on analyzing code
  fragments where exactly two out of the three tokens come from the same
  macro expansion, e.g.

    #define MACRO \
      if (a)      \
        foo ();

    MACRO; bar ();

  Here, guard_loc and body_loc will be equal and point to the macro
  expansion point.  The heuristics the warning uses are not really valid
  in scenarios like these.  ]

In order to restore the column-tracking diagnostic, this patch moves the
the diagnostic code out from get_visual_column to earlier in
should_warn_for_misleading_indentation.  Moreover, it tests the three
location_t values for a zero column all at once, which I suppose should
make us issue the diagnostic more consistently.

Tested on x86_64-pc-linux-gnu, does this look OK to commit?

gcc/c-family/ChangeLog:

	PR testsuite/97117
	* c-indentation.c (get_visual_column): Remove location_t
	parameter.  Move the column-tracking diagnostic code from here
	to ...
	(should_warn_for_misleading_indentation): ... here, before the
	early exit for when the loci are not all distinct.  Don't pass a
	location_t argument to get_visual_column.
	(assert_get_visual_column_succeeds): Don't pass a location_t
	argument to get_visual_column.
	(assert_get_visual_column_fails): Likewise.
---
 gcc/c-family/c-indentation.c | 70 ++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

Comments

Jeff Law Nov. 6, 2020, 5:48 p.m. UTC | #1 
On 10/14/20 9:56 AM, Patrick Palka via Gcc-patches wrote:
> The r11-3266 patch that added macro support to -Wmisleading-indentation
> accidentally suppressed the column-tracking diagnostic in
> get_visual_column in some cases, e.g. in the location-overflow-test-1.c
> testcase.
>
> More generally, when all three tokens are on the same line and we've run
> out of locations with column info, then their location_t values will be
> equal, and we exit early from should_warn_for_misleading_indentation due
> to the new check
>
>   /* Give up if the loci are not all distinct.  */
>   if (guard_loc == body_loc || body_loc == next_stmt_loc)
>     return false;
>
> before we ever call get_visual_column.
>
> [ This new check is needed to detect and give up on analyzing code
>   fragments where exactly two out of the three tokens come from the same
>   macro expansion, e.g.
>
>     #define MACRO \
>       if (a)      \
>         foo ();
>
>     MACRO; bar ();
>
>   Here, guard_loc and body_loc will be equal and point to the macro
>   expansion point.  The heuristics the warning uses are not really valid
>   in scenarios like these.  ]
>
> In order to restore the column-tracking diagnostic, this patch moves the
> the diagnostic code out from get_visual_column to earlier in
> should_warn_for_misleading_indentation.  Moreover, it tests the three
> location_t values for a zero column all at once, which I suppose should
> make us issue the diagnostic more consistently.
>
> Tested on x86_64-pc-linux-gnu, does this look OK to commit?
>
> gcc/c-family/ChangeLog:
>
> 	PR testsuite/97117
> 	* c-indentation.c (get_visual_column): Remove location_t
> 	parameter.  Move the column-tracking diagnostic code from here
> 	to ...
> 	(should_warn_for_misleading_indentation): ... here, before the
> 	early exit for when the loci are not all distinct.  Don't pass a
> 	location_t argument to get_visual_column.
> 	(assert_get_visual_column_succeeds): Don't pass a location_t
> 	argument to get_visual_column.
> 	(assert_get_visual_column_fails): Likewise.

OK.


jeff
diff mbox series

Patch

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 8b88a8adc7c..836a524f266 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -45,36 +45,11 @@  next_tab_stop (unsigned int vis_column, unsigned int tab_width)
    on the line (up to or before EXPLOC).  */
 
 static bool
-get_visual_column (expanded_location exploc, location_t loc,
+get_visual_column (expanded_location exploc,
 		   unsigned int *out,
 		   unsigned int *first_nws,
 		   unsigned int tab_width)
 {
-  /* PR c++/68819: if the column number is zero, we presumably
-     had a location_t > LINE_MAP_MAX_LOCATION_WITH_COLS, and so
-     we have no column information.
-     Act as if no conversion was possible, triggering the
-     error-handling path in the caller.  */
-  if (!exploc.column)
-    {
-      static bool issued_note = false;
-      if (!issued_note)
-	{
-	  /* Notify the user the first time this happens.  */
-	  issued_note = true;
-	  inform (loc,
-		  "%<-Wmisleading-indentation%> is disabled from this point"
-		  " onwards, since column-tracking was disabled due to"
-		  " the size of the code/headers");
-	  if (!flag_large_source_files)
-	    inform (loc,
-		    "adding %<-flarge-source-files%> will allow for more" 
-		    " column-tracking support, at the expense of compilation"
-		    " time and memory");
-	}
-      return false;
-    }
-
   char_span line = location_get_source_line (exploc.file, exploc.line);
   if (!line)
     return false;
@@ -325,14 +300,37 @@  should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 						NULL);
     }
 
-  /* Give up if the loci are not all distinct.  */
-  if (guard_loc == body_loc || body_loc == next_stmt_loc)
-    return false;
-
   expanded_location body_exploc = expand_location (body_loc);
   expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
   expanded_location guard_exploc = expand_location (guard_loc);
 
+  /* PR c++/68819: if the column number is zero, we presumably
+     had a location_t > LINE_MAP_MAX_LOCATION_WITH_COLS, and so
+     we have no column information.  */
+  if (!guard_exploc.column || !body_exploc.column || !next_stmt_exploc.column)
+    {
+      static bool issued_note = false;
+      if (!issued_note)
+	{
+	  /* Notify the user the first time this happens.  */
+	  issued_note = true;
+	  inform (guard_loc,
+		  "%<-Wmisleading-indentation%> is disabled from this point"
+		  " onwards, since column-tracking was disabled due to"
+		  " the size of the code/headers");
+	  if (!flag_large_source_files)
+	    inform (guard_loc,
+		    "adding %<-flarge-source-files%> will allow for more" 
+		    " column-tracking support, at the expense of compilation"
+		    " time and memory");
+	}
+      return false;
+    }
+
+  /* Give up if the loci are not all distinct.  */
+  if (guard_loc == body_loc || body_loc == next_stmt_loc)
+    return false;
+
   const unsigned int tab_width = global_dc->tabstop;
 
   /* They must be in the same file.  */
@@ -378,7 +376,7 @@  should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	  gcc_assert (guard_exploc.line == next_stmt_exploc.line);
 	  unsigned int guard_vis_column;
 	  unsigned int guard_line_first_nws;
-	  if (!get_visual_column (guard_exploc, guard_loc,
+	  if (!get_visual_column (guard_exploc,
 				  &guard_vis_column,
 				  &guard_line_first_nws, tab_width))
 	    return false;
@@ -438,15 +436,15 @@  should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	 the case for input files containing #line directives, and these
 	 are often for autogenerated sources (e.g. from .md files), where
 	 it's not clear that it's meaningful to look at indentation.  */
-      if (!get_visual_column (next_stmt_exploc, next_stmt_loc,
+      if (!get_visual_column (next_stmt_exploc,
 			      &next_stmt_vis_column,
 			      &next_stmt_line_first_nws, tab_width))
 	return false;
-      if (!get_visual_column (body_exploc, body_loc,
+      if (!get_visual_column (body_exploc,
 			      &body_vis_column,
 			      &body_line_first_nws, tab_width))
 	return false;
-      if (!get_visual_column (guard_exploc, guard_loc,
+      if (!get_visual_column (guard_exploc,
 			      &guard_vis_column,
 			      &guard_line_first_nws, tab_width))
 	return false;
@@ -701,7 +699,7 @@  assert_get_visual_column_succeeds (const location &loc,
   exploc.sysp = false;
   unsigned int actual_visual_column;
   unsigned int actual_first_nws;
-  bool result = get_visual_column (exploc, UNKNOWN_LOCATION,
+  bool result = get_visual_column (exploc,
 				   &actual_visual_column,
 				   &actual_first_nws, tab_width);
   ASSERT_TRUE_AT (loc, result);
@@ -739,7 +737,7 @@  assert_get_visual_column_fails (const location &loc,
   exploc.sysp = false;
   unsigned int actual_visual_column;
   unsigned int actual_first_nws;
-  bool result = get_visual_column (exploc, UNKNOWN_LOCATION,
+  bool result = get_visual_column (exploc,
 				   &actual_visual_column,
 				   &actual_first_nws, tab_width);
   ASSERT_FALSE_AT (loc, result);