[PATCH v2] [PR debug/67192] Fix C loops' back-jump location

[PATCH v2] [PR debug/67192] Fix C loops' back-jump location

[Andreas Arnez]

After parsing an unconditional "while"- or "for"-loop, the C front-end
generates a backward-goto statement and implicitly sets its location to
the current input_location.  But in some cases the parser peeks ahead
first, such that input_location already points to the line after the
loop and the generated backward-goto gets the wrong line number.

One way this can occur is with a loop body consisting of an "if"
statement, because then the parser peeks for an optional "else" before
finishing the loop.

Another way occurs since r223098 ("Implement -Wmisleading-indentation"),
even with a loop body enclosed in braces.  This is because the check for
misleading indentation always peeks ahead one token as well.

This patch adds a new parameter to c_finish_loop that expclitly
specifies the location to be used for the loop iteration.  All calls to
c_finish_loop are adjusted accordingly.

gcc/c/ChangeLog:

	PR debug/67192
	* c-typeck.c (c_finish_loop): Replace implicit use of
	input_location by new parameter iter_locus.
	* c-tree.h (c_finish_loop): Adjust prototype.
	* c-array-notation.c (fix_builtin_array_notation_fn): Explicitly
	pass input_location as the new parameter to c_finish_loop.
	(build_array_notation_expr): Likewise.
	(fix_conditional_array_notations_1): Likewise.
	(fix_array_notation_expr): Likewise.
	(fix_array_notation_call_expr): Likewise.
	* c-parser.c (c_parser_while_statement): Choose iter_locus
	depending on whether the loop body is enclosed in braces, and pass
	it to c_finish_loop.
	(c_parser_for_statement): Likewise.
	(c_parser_do_statement): Use the final semicolon's location for
	iter_locus and pass it to c_finish_loop.

gcc/testsuite/ChangeLog:

	PR debug/67192
	* gcc.dg/guality/pr67192.c: New test.
---
 gcc/c/c-array-notation.c               | 15 ++++---
 gcc/c/c-parser.c                       | 13 ++++--
 gcc/c/c-tree.h                         |  2 +-
 gcc/c/c-typeck.c                       | 17 +++++---
 gcc/testsuite/gcc.dg/guality/pr67192.c | 79 ++++++++++++++++++++++++++++++++++
 5 files changed, 109 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/guality/pr67192.c

diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
index 3de7569..c457eee2 100644
--- a/gcc/c/c-array-notation.c
+++ b/gcc/c/c-array-notation.c
@@ -591,7 +591,8 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var)
   for (ii = 0; ii < rank; ii++)
     {
       tree new_loop = push_stmt_list ();
-      c_finish_loop (location, an_loop_info[ii].cmp, an_loop_info[ii].incr,
+      c_finish_loop (location, input_location, an_loop_info[ii].cmp,
+		     an_loop_info[ii].incr,
 		     body, NULL_TREE, NULL_TREE, true);
       body = pop_stmt_list (new_loop);
     }
@@ -879,8 +880,8 @@ build_array_notation_expr (location_t location, tree lhs, tree lhs_origtype,
 	append_to_statement_list_force (lhs_an_loop_info[ii].incr, &incr_list);
       if (rhs_rank && rhs_an_loop_info[ii].incr)
 	append_to_statement_list_force (rhs_an_loop_info[ii].incr, &incr_list);
-      c_finish_loop (location, cond_expr[ii], incr_list, body, NULL_TREE,
-		     NULL_TREE, true);
+      c_finish_loop (location, input_location, cond_expr[ii],
+		     incr_list, body, NULL_TREE, NULL_TREE, true);
       body = pop_stmt_list (new_loop);
     }
   append_to_statement_list_force (body, &loop_with_init);
@@ -1004,7 +1005,8 @@ fix_conditional_array_notations_1 (tree stmt)
     {
       tree new_loop = push_stmt_list ();
       add_stmt (an_loop_info[ii].ind_init);
-      c_finish_loop (location, an_loop_info[ii].cmp, an_loop_info[ii].incr,
+      c_finish_loop (location, input_location, an_loop_info[ii].cmp,
+		     an_loop_info[ii].incr,
 		     body, NULL_TREE, NULL_TREE, true);
       body = pop_stmt_list (new_loop);
     }
@@ -1107,7 +1109,7 @@ fix_array_notation_expr (location_t location, enum tree_code code,
     {
       tree new_loop = push_stmt_list ();
       add_stmt (an_loop_info[ii].ind_init);
-      c_finish_loop (location, an_loop_info[ii].cmp,
+      c_finish_loop (location, input_location, an_loop_info[ii].cmp,
 		     an_loop_info[ii].incr, body, NULL_TREE,
 		     NULL_TREE, true);
       body = pop_stmt_list (new_loop);
@@ -1193,7 +1195,8 @@ fix_array_notation_call_expr (tree arg)
     {
       tree new_loop = push_stmt_list ();
       add_stmt (an_loop_info[ii].ind_init);
-      c_finish_loop (location, an_loop_info[ii].cmp, an_loop_info[ii].incr,
+      c_finish_loop (location, input_location, an_loop_info[ii].cmp,
+		     an_loop_info[ii].incr,
 		     body, NULL_TREE, NULL_TREE, true);
       body = pop_stmt_list (new_loop);
     }
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 704ebc6..69e0a9c 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -5438,13 +5438,16 @@ c_parser_while_statement (c_parser *parser, bool ivdep)
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
+  bool is_braced = c_parser_next_token_is (parser, CPP_OPEN_BRACE);
   body = c_parser_c99_block_statement (parser);
+  location_t iter_loc = is_braced ? input_location : loc;
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo);
 
-  c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
+  c_finish_loop (loc, iter_loc, cond, NULL, body, c_break_label,
+		 c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
   c_break_label = save_break;
   c_cont_label = save_cont;
@@ -5490,7 +5493,8 @@ c_parser_do_statement (c_parser *parser, bool ivdep)
 		   annot_expr_ivdep_kind));
   if (!c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
     c_parser_skip_to_end_of_block_or_statement (parser);
-  c_finish_loop (loc, cond, NULL, body, new_break, new_cont, false);
+  c_finish_loop (loc, input_location, cond, NULL, body, new_break,
+		 new_cont, false);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
 }
 
@@ -5727,7 +5731,9 @@ c_parser_for_statement (c_parser *parser, bool ivdep)
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
+  bool is_braced = c_parser_next_token_is (parser, CPP_OPEN_BRACE);
   body = c_parser_c99_block_statement (parser);
+  location_t iter_loc = is_braced ? input_location : loc;
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
@@ -5736,7 +5742,8 @@ c_parser_for_statement (c_parser *parser, bool ivdep)
   if (is_foreach_statement)
     objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label);
   else
-    c_finish_loop (loc, cond, incr, body, c_break_label, c_cont_label, true);
+    c_finish_loop (loc, iter_loc, cond, incr, body, c_break_label,
+		   c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99 || c_dialect_objc ()));
   c_break_label = save_break;
   c_cont_label = save_cont;
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index bee03d3..9c13672 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -630,7 +630,7 @@ extern int c_types_compatible_p (tree, tree);
 extern tree c_begin_compound_stmt (bool);
 extern tree c_end_compound_stmt (location_t, tree, bool);
 extern void c_finish_if_stmt (location_t, tree, tree, tree, bool);
-extern void c_finish_loop (location_t, tree, tree, tree, tree, tree, bool);
+extern void c_finish_loop (location_t, location_t, tree, tree, tree, tree, tree, bool);
 extern tree c_begin_stmt_expr (void);
 extern tree c_finish_stmt_expr (location_t, tree);
 extern tree c_process_expr_stmt (location_t, tree);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index bc43602..e4b5471 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -9810,14 +9810,16 @@ c_finish_if_stmt (location_t if_locus, tree cond, tree then_block,
   add_stmt (stmt);
 }
 
-/* Emit a general-purpose loop construct.  START_LOCUS is the location of
-   the beginning of the loop.  COND is the loop condition.  COND_IS_FIRST
-   is false for DO loops.  INCR is the FOR increment expression.  BODY is
-   the statement controlled by the loop.  BLAB is the break label.  CLAB is
-   the continue label.  Everything is allowed to be NULL.  */
+/* Emit a general-purpose loop construct.  START_LOCUS is the location
+   of the beginning of the loop.  ITER_LOCUS is the location of the
+   loop iteration.  COND is the loop condition.  COND_IS_FIRST is
+   false for DO loops.  INCR is the FOR increment expression.  BODY is
+   the statement controlled by the loop.  BLAB is the break label.
+   CLAB is the continue label.  Everything is allowed to be NULL.  */
 
 void
-c_finish_loop (location_t start_locus, tree cond, tree incr, tree body,
+c_finish_loop (location_t start_locus, location_t iter_locus,
+	       tree cond, tree incr, tree body,
 	       tree blab, tree clab, bool cond_is_first)
 {
   tree entry = NULL, exit = NULL, t;
@@ -9843,6 +9845,7 @@ c_finish_loop (location_t start_locus, tree cond, tree incr, tree body,
 	 out of the loop, or to the top of it.  If there's no exit condition,
 	 then we just build a jump back to the top.  */
       exit = build_and_jump (&LABEL_EXPR_LABEL (top));
+      SET_EXPR_LOCATION (exit, iter_locus);
 
       if (cond && !integer_nonzerop (cond))
 	{
@@ -9867,7 +9870,7 @@ c_finish_loop (location_t start_locus, tree cond, tree incr, tree body,
 	    exit = fold_build3_loc (start_locus,
 				COND_EXPR, void_type_node, cond, exit, t);
 	  else
-	    exit = fold_build3_loc (input_location,
+	    exit = fold_build3_loc (iter_locus,
 				COND_EXPR, void_type_node, cond, exit, t);
 	}
 
diff --git a/gcc/testsuite/gcc.dg/guality/pr67192.c b/gcc/testsuite/gcc.dg/guality/pr67192.c
new file mode 100644
index 0000000..faef853
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr67192.c
@@ -0,0 +1,79 @@
+/* PR debug/67192 */
+/* { dg-do run } */
+/* { dg-options "-g -Wmisleading-indentation" } */
+
+volatile int cnt = 0;
+
+__attribute__((noinline, noclone)) static int
+last (void)
+{
+  return ++cnt % 5 == 0;
+}
+
+__attribute__((noinline, noclone)) static void
+do_it (void)
+{
+  asm volatile ("" : : "r" (&cnt) : "memory");
+}
+
+__attribute__((noinline, noclone)) static void
+f1 (void)
+{
+  for (;; do_it())
+    if (last ())
+      break;
+  do_it (); /* { dg-final { gdb-test 25 "cnt" "5" } } */
+}
+
+__attribute__((noinline, noclone)) static void
+f2 (void)
+{
+  while (1)
+    if (last ())
+      break;
+    else
+      do_it ();
+  do_it (); /* { dg-final { gdb-test 36 "cnt" "10" } } */
+}
+
+__attribute__((noinline, noclone)) static void
+f3 (void)
+{
+  for (;; do_it())
+    {
+      if (last ())
+	break;
+    }
+  do_it (); /* { dg-final { gdb-test 47 "cnt" "15" } } */
+}
+
+__attribute__((noinline, noclone)) static void
+f4 (void)
+{
+  while (1)
+    {
+      if (last ())
+	break;
+      do_it ();
+    }
+  do_it (); /* { dg-final { gdb-test 59 "cnt" "20" } } */
+}
+
+void (*volatile fnp1) (void) = f1;
+void (*volatile fnp2) (void) = f2;
+void (*volatile fnp3) (void) = f3;
+void (*volatile fnp4) (void) = f4;
+
+int
+main ()
+{
+  asm volatile ("" : : "r" (&fnp1) : "memory");
+  asm volatile ("" : : "r" (&fnp2) : "memory");
+  asm volatile ("" : : "r" (&fnp3) : "memory");
+  asm volatile ("" : : "r" (&fnp4) : "memory");
+  fnp1 ();
+  fnp2 ();
+  fnp3 ();
+  fnp4 ();
+  return 0;
+}
-- 
2.3.0

Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location

[Andreas Arnez]

Ping:

  https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02393.html

On Fri, Oct 23 2015, Andreas Arnez wrote:

> After parsing an unconditional "while"- or "for"-loop, the C front-end
> generates a backward-goto statement and implicitly sets its location to
> the current input_location.  But in some cases the parser peeks ahead
> first, such that input_location already points to the line after the
> loop and the generated backward-goto gets the wrong line number.
>
> One way this can occur is with a loop body consisting of an "if"
> statement, because then the parser peeks for an optional "else" before
> finishing the loop.
>
> Another way occurs since r223098 ("Implement -Wmisleading-indentation"),
> even with a loop body enclosed in braces.  This is because the check for
> misleading indentation always peeks ahead one token as well.
>
> This patch adds a new parameter to c_finish_loop that expclitly
> specifies the location to be used for the loop iteration.  All calls to
> c_finish_loop are adjusted accordingly.
>
> gcc/c/ChangeLog:
>
> 	PR debug/67192
> 	* c-typeck.c (c_finish_loop): Replace implicit use of
> 	input_location by new parameter iter_locus.
> 	* c-tree.h (c_finish_loop): Adjust prototype.
> 	* c-array-notation.c (fix_builtin_array_notation_fn): Explicitly
> 	pass input_location as the new parameter to c_finish_loop.
> 	(build_array_notation_expr): Likewise.
> 	(fix_conditional_array_notations_1): Likewise.
> 	(fix_array_notation_expr): Likewise.
> 	(fix_array_notation_call_expr): Likewise.
> 	* c-parser.c (c_parser_while_statement): Choose iter_locus
> 	depending on whether the loop body is enclosed in braces, and pass
> 	it to c_finish_loop.
> 	(c_parser_for_statement): Likewise.
> 	(c_parser_do_statement): Use the final semicolon's location for
> 	iter_locus and pass it to c_finish_loop.
>
> gcc/testsuite/ChangeLog:
>
> 	PR debug/67192
> 	* gcc.dg/guality/pr67192.c: New test.

Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location

[Bernd Schmidt]

On 10/23/2015 11:14 AM, Andreas Arnez wrote:
> This patch adds a new parameter to c_finish_loop that expclitly
> specifies the location to be used for the loop iteration.  All calls to
> c_finish_loop are adjusted accordingly.

I think the general principle of this is probably ok.

> +  bool is_braced = c_parser_next_token_is (parser, CPP_OPEN_BRACE);
>     body = c_parser_c99_block_statement (parser);
> +  location_t iter_loc = is_braced ? input_location : loc;
>
>     token_indent_info next_tinfo
>       = get_token_indent_info (c_parser_peek_token (parser));
>     warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo);
>
> -  c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
> +  c_finish_loop (loc, iter_loc, cond, NULL, body, c_break_label,
> +		 c_cont_label, true);

I'm not entirely sure I understand what the is_braced logic is trying to 
achieve. I tried the patch out with the debugger on the testcase you 
provided, and I get slightly strange behaviour in f2:

Starting program: /local/src/egcs/bwork-git/gcc/a.out

Breakpoint 7, f2 () at pr67192.c:32
32	    if (last ())
(gdb) cont
Continuing.

Breakpoint 6, f2 () at pr67192.c:31
31	  while (1)
(gdb)

i.e. the breakpoint on the code inside the loop is reached before the 
while statement itself. This may be the expected behaviour with your 
patch, but I'm not sure it's really desirable for debugging.

In f4 placing a breakpoint on the while (1) just places it on the if 
(last ()) line. The same behaviour appears to occur for both while loops 
with the system gcc-4.8.5. So I think there are some strange 
inconsistencies going on here, and for debugging the old behaviour may 
well be better.

I'd suggest you commit your original patch to fix the misleading-indent 
problem while we sort this out.


Bernd

Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location

[Andreas Arnez]

On Thu, Oct 29 2015, Bernd Schmidt wrote:

> On 10/23/2015 11:14 AM, Andreas Arnez wrote:
>> +  bool is_braced = c_parser_next_token_is (parser, CPP_OPEN_BRACE);
>>     body = c_parser_c99_block_statement (parser);
>> +  location_t iter_loc = is_braced ? input_location : loc;
>>
>>     token_indent_info next_tinfo
>>       = get_token_indent_info (c_parser_peek_token (parser));
>>     warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo);
>>
>> -  c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
>> +  c_finish_loop (loc, iter_loc, cond, NULL, body, c_break_label,
>> +		 c_cont_label, true);
>
> I'm not entirely sure I understand what the is_braced logic is trying
> to achieve.
>
> I tried the patch out with the debugger on the testcase
> you provided, and I get slightly strange behaviour in f2:
>
> Starting program: /local/src/egcs/bwork-git/gcc/a.out
>
> Breakpoint 7, f2 () at pr67192.c:32
> 32	    if (last ())
> (gdb) cont
> Continuing.
>
> Breakpoint 6, f2 () at pr67192.c:31
> 31	  while (1)
> (gdb)
>
> i.e. the breakpoint on the code inside the loop is reached before the
> while statement itself. This may be the expected behaviour with your
> patch, but I'm not sure it's really desirable for debugging.

Good point.  This happens because no code is generated for "while (1)",
except the backward-goto.

If the loop body is enclosed in braces, then the patch associates the
backward-goto with the line of the closing brace (which usually stands
on a line by itself), otherwise with the while- or for-token.  By
contrast, the approach that seems to have been intended before is to
generally associate the backward-goto with the loop body's last line.
But firstly, this may cause confusion as well, e.g. the following could
then stop on every loop iteration, even if the else-branch is never
executed:

  while (1)
    if (do_foo ())
      foo ();
    else
      bar ();  /* <- break here */

Secondly, AFAIK, c_parser_c99_block_statement currently provides no way
of retrieving the last token's location.  One way of getting there might
be to update input_location on each *consumed* token instead of every
peeked-at token, similar to what the C++ parser does.  But that would
probably be a fairly large change and might have lots of other pitfalls.
Or we could pass it around as an extra parameter?  *Or* add an end_locus
to the tree_exp struct?

Maybe we could also improve the behavior of breaking on "while (1)" by
generating a NOP for it?  Or by using the first loop body's token
instead?

Ideas/thoughts?

> In f4 placing a breakpoint on the while (1) just places it on the if
> (last ()) line. The same behaviour appears to occur for both while
> loops with the system gcc-4.8.5. So I think there are some strange
> inconsistencies going on here, and for debugging the old behaviour may
> well be better.

Possibly in some cases.  But not where the old behavior had real bugs.
For instance, even with gcc-4.8.5 the breakpoint in f1 is set
incorrectly, right?

Also note that the "last loop body line" approach has inconsistencies as
well.  E.g. in my example above the meaning of the breakpoint on the
else-branch then depends on whether the loop body is enclosed in braces
or not.

> I'd suggest you commit your original patch to fix the
> misleading-indent problem while we sort this out.

I can certainly do that.  But note that the original patch does not
solve the misleading-indent regression caused for f2() in the new
version of pr67192.c.  Thus the PR is not really fixed by it.

--
Andreas

Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location

[Andreas Arnez]

On Thu, Oct 29 2015, Andreas Arnez wrote:

> On Thu, Oct 29 2015, Bernd Schmidt wrote:
>> [...]
>> i.e. the breakpoint on the code inside the loop is reached before the
>> while statement itself. This may be the expected behaviour with your
>> patch, but I'm not sure it's really desirable for debugging.
>
> [...]
> Maybe we could also improve the behavior of breaking on "while (1)" by
> generating a NOP for it?  Or by using the first loop body's token
> instead?

I've basically tried the latter, and it seems to work pretty well.  It
solves all the issues discussed in this mail thread, and I haven't found
any other issues with it.  I'll post the patch separately.

>> I'd suggest you commit your original patch to fix the
>> misleading-indent problem while we sort this out.
>
> I can certainly do that.  But note that the original patch does not
> solve the misleading-indent regression caused for f2() in the new
> version of pr67192.c.  Thus the PR is not really fixed by it.

I've slightly changed the test case for that one, so I'll repost it as
well before committing it.

--
Andreas