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

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

[Andreas Arnez]

Since r223098 ("Implement -Wmisleading-indentation") the backward-jump
generated for a C while- or for-loop can get the wrong line number.
This is because the check for misleading indentation peeks ahead one
token, advancing input_location to after the loop, and then
c_finish_loop() creates the back-jump and calls add_stmt(), which
assigns input_location to the statement by default.

This patch swaps the check for misleading indentation with the finishing
of the loop, such that input_location still has the right value at the
time of any invocations of add_stmt().

gcc/testsuite/ChangeLog:

	PR debug/67192
	* gcc.dg/guality/pr67192.c: New test.

gcc/c/ChangeLog:

	PR debug/67192
	* c-parser.c (c_parser_while_statement): Finish the loop before
	parsing ahead for misleading indentation.
	(c_parser_for_statement): Likewise.
---
 gcc/c/c-parser.c                       | 13 +++++----
 gcc/testsuite/gcc.dg/guality/pr67192.c | 50 ++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/guality/pr67192.c

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 2d24c21..8740922 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -5438,13 +5438,13 @@ c_parser_while_statement (c_parser *parser, bool ivdep)
     = get_token_indent_info (c_parser_peek_token (parser));
 
   body = c_parser_c99_block_statement (parser);
+  c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
+  add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
 
   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);
-  add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
@@ -5728,15 +5728,16 @@ c_parser_for_statement (c_parser *parser, bool ivdep)
 
   body = c_parser_c99_block_statement (parser);
 
-  token_indent_info next_tinfo
-    = get_token_indent_info (c_parser_peek_token (parser));
-  warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo);
-
   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);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99 || c_dialect_objc ()));
+
+  token_indent_info next_tinfo
+    = get_token_indent_info (c_parser_peek_token (parser));
+  warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo);
+
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
diff --git a/gcc/testsuite/gcc.dg/guality/pr67192.c b/gcc/testsuite/gcc.dg/guality/pr67192.c
new file mode 100644
index 0000000..73d4e44
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr67192.c
@@ -0,0 +1,50 @@
+/* PR debug/67192 */
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+static volatile int cnt = 0;
+
+__attribute__((noinline)) int
+f1 (void)
+{
+  return ++cnt % 5 == 0;
+}
+
+__attribute__((noinline)) void
+f2 (void)
+{
+}
+
+__attribute__((noinline)) void
+f3 (int (*last) (void), void (*do_it) (void))
+{
+  for (;;)
+    {
+      if (last ())
+	break;
+      do_it ();
+    }
+  do_it (); /* { dg-final { gdb-test 27 "cnt" "5" } } */
+
+  while (1)
+    {
+      if (last ())
+	break;
+      do_it ();
+    }
+  do_it (); /* { dg-final { gdb-test 35 "cnt" "10" } } */
+}
+
+int (*volatile fnp1) (void) = f1;
+void (*volatile fnp2) (void) = f2;
+void (*volatile fnp3) (int (*) (void), void (*) (void)) = f3;
+
+int
+main (int argc, char *argv[])
+{
+  asm volatile ("" : : "r" (&fnp1) : "memory");
+  asm volatile ("" : : "r" (&fnp2) : "memory");
+  asm volatile ("" : : "r" (&fnp3) : "memory");
+  fnp3 (fnp1, fnp2);
+  return 0;
+}
-- 
2.3.0

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

[Bernd Schmidt]

On 10/12/2015 04:04 PM, Andreas Arnez wrote:
> Since r223098 ("Implement -Wmisleading-indentation") the backward-jump
> generated for a C while- or for-loop can get the wrong line number.
> This is because the check for misleading indentation peeks ahead one
> token, advancing input_location to after the loop, and then
> c_finish_loop() creates the back-jump and calls add_stmt(), which
> assigns input_location to the statement by default.
>
> This patch swaps the check for misleading indentation with the finishing
> of the loop, such that input_location still has the right value at the
> time of any invocations of add_stmt().

One could argue that peek_token should not have an effect on 
input_location, and in fact cpp_peek_token seems to take steps that this 
does not happen, but it looks like c_parser_peek_token does not use that 
mechanism. Still,
>
> gcc/testsuite/ChangeLog:
>
> 	PR debug/67192
> 	* gcc.dg/guality/pr67192.c: New test.
>
> gcc/c/ChangeLog:
>
> 	PR debug/67192
> 	* c-parser.c (c_parser_while_statement): Finish the loop before
> 	parsing ahead for misleading indentation.
> 	(c_parser_for_statement): Likewise.

This fix looks simple enough. Ok. (Might want to add noclone to the 
testcase attributes).


Bernd

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

[Andreas Arnez]

On Tue, Oct 13 2015, Bernd Schmidt wrote:

> One could argue that peek_token should not have an effect on
> input_location, and in fact cpp_peek_token seems to take steps that
> this does not happen, but it looks like c_parser_peek_token does not
> use that mechanism.

Yes, the C/C++ parsers differ quite significantly in this regard.  The C
parser invokes the lexer in peek_token and advances input_location upon
each newline.  The C++ parser usually lexes everything in advance and
updates input_location on each *consumed* token.

By advancing input_location in peek_token upon each newline, diagnostics
emitted with warning() and friends point to the beginning of the line of
the peeked-at token.  This is probably somewhat intended, so I'd rather
not touch that right now.

A different aspect is the implicit use of input_location for the
location of generated statements.  This usage is what causes the problem
at hand, and IMHO it should generally be rooted out.

>> Still,
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR debug/67192
>> 	* gcc.dg/guality/pr67192.c: New test.
>>
>> gcc/c/ChangeLog:
>>
>> 	PR debug/67192
>> 	* c-parser.c (c_parser_while_statement): Finish the loop before
>> 	parsing ahead for misleading indentation.
>> 	(c_parser_for_statement): Likewise.
>
> This fix looks simple enough. Ok. (Might want to add noclone to the
> testcase attributes).

Thanks for reviewing!  Unfortunately, after investigating this some
more, I realized that my solution is incomplete.  E.g., consider this:

  while (1)
    if (foo ())
       break;
    else
       do_something ();
  bar ();      /* break here */

Interestingly, line number information for such code has been broken in
GCC for a long time.

I'll send an updated version.