[PATCH v3 0/2] Fix C loops' back-jump location

[PATCH v3 0/2] Fix C loops' back-jump location

[Andreas Arnez]

Another iteration of trying to fix the regression caused by r223098
("Implement -Wmisleading-indentation").  Patch #1 is the same as v1,
except for some minor changes to the test case.  Patch #2 fixes some
additional cases where the back-jump's location was set wrongly, and
it removes the dependency on input_location for this purpose.

Tested on s390x without regressions.

Previous versions:
 * v1: https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01132.html
 * v2: https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02393.html

Andreas Arnez (2):
  [PR debug/67192] Fix C loops' back-jump location
  [PR debug/67192] Further fix C loops' back-jump location

 gcc/c/c-parser.c                       | 13 +++---
 gcc/c/c-typeck.c                       | 10 +++++
 gcc/testsuite/gcc.dg/guality/pr67192.c | 79 ++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/guality/pr67192.c

-- 
2.3.0

[PATCH v3 2/2] [PR debug/67192] Further 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 occurred after r223098 ("Implement
-Wmisleading-indentation"), even with a loop body enclosed in braces.
This was because the check for misleading indentation always peeks ahead
one token as well.

This patch avoids the use of input_location and sets the location of the
backward-goto to the start of the loop body instead, or, if there is no
loop body, to the start of the loop.

gcc/c/ChangeLog:

	PR debug/67192
	* c-typeck.c (c_finish_loop): For unconditional loops, set the
	location of the backward-goto to the start of the loop body.

gcc/testsuite/ChangeLog:

	PR debug/67192
	* gcc.dg/guality/pr67192.c (f3, f4): New functions.
	(main): Invoke them.
---
 gcc/c/c-typeck.c                       | 10 ++++++++++
 gcc/testsuite/gcc.dg/guality/pr67192.c | 26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 2363b9b..e4c3720 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -9878,6 +9878,16 @@ c_finish_loop (location_t start_locus, tree cond, tree incr, tree body,
 	    exit = fold_build3_loc (input_location,
 				COND_EXPR, void_type_node, cond, exit, t);
 	}
+      else
+	{
+	  /* For the backward-goto's location of an unconditional loop
+	     use the beginning of the body, or, if there is none, the
+	     top of the loop.  */
+	  location_t loc = EXPR_LOCATION (expr_first (body));
+	  if (loc == UNKNOWN_LOCATION)
+	    loc = start_locus;
+	  SET_EXPR_LOCATION (exit, loc);
+	}
 
       add_stmt (top);
     }
diff --git a/gcc/testsuite/gcc.dg/guality/pr67192.c b/gcc/testsuite/gcc.dg/guality/pr67192.c
index f6382ef..946e68f 100644
--- a/gcc/testsuite/gcc.dg/guality/pr67192.c
+++ b/gcc/testsuite/gcc.dg/guality/pr67192.c
@@ -39,15 +39,41 @@ f2 (void)
   do_it (); /* { dg-final { gdb-test 39 "cnt" "10" } } */
 }
 
+__attribute__((noinline, noclone)) static void
+f3 (void)
+{
+  for (;; do_it())
+    if (last ())
+      break;
+  do_it (); /* { dg-final { gdb-test 48 "cnt" "15" } } */
+}
+
+__attribute__((noinline, noclone)) static void
+f4 (void)
+{
+  while (1) /* { dg-final { gdb-test 54 "cnt" "15" } } */
+    if (last ())
+      break;
+    else
+      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

[PATCH v3 1/2] [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().  Note that this does not fully
cover all cases where the back-jump gets the wrong location.

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/testsuite/ChangeLog:

	PR debug/67192
	* gcc.dg/guality/pr67192.c: New test.
---
 gcc/c/c-parser.c                       | 13 +++++----
 gcc/testsuite/gcc.dg/guality/pr67192.c | 53 ++++++++++++++++++++++++++++++++++
 2 files changed, 60 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 ec88c65..0c5e4e9 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -5435,13 +5435,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;
 }
@@ -5725,15 +5725,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..f6382ef
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr67192.c
@@ -0,0 +1,53 @@
+/* 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 27 "cnt" "5" } } */
+}
+
+__attribute__((noinline, noclone)) static void
+f2 (void)
+{
+  while (1)
+    {
+      if (last ())
+	break;
+      do_it ();
+    }
+  do_it (); /* { dg-final { gdb-test 39 "cnt" "10" } } */
+}
+
+void (*volatile fnp1) (void) = f1;
+void (*volatile fnp2) (void) = f2;
+
+int
+main ()
+{
+  asm volatile ("" : : "r" (&fnp1) : "memory");
+  asm volatile ("" : : "r" (&fnp2) : "memory");
+  fnp1 ();
+  fnp2 ();
+  return 0;
+}
-- 
2.3.0

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

[Bernd Schmidt]

On 11/04/2015 05:17 PM, Andreas Arnez wrote:
>
> 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 is OK. Does C++ have similar issues?


Bernd

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

[Andreas Arnez]

On Thu, Nov 05 2015, Bernd Schmidt wrote:

> On 11/04/2015 05:17 PM, Andreas Arnez wrote:
>>
>> 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 is OK.

Thanks again for reviewing.  Are you going to look at patch #2 as well?

> Does C++ have similar issues?

Not this particular issue, AFAIK.  But I've just looked at how C++ fares
with the enhanced version of pr67192.c from patch #2.  There I see the
following:

  Breakpoint 2, f4 () at pr67192.cc:54
  (gdb) p cnt
  $1 = 16

I.e., when breaking on "while (1)" the first loop iteration has already
executed.  This is because the C++ parser assigns the backward-goto to
the 'while' token.  It's the same issue you pointed at with version 2 of
my patch.

Shall I open a bug for that?

--
Andreas

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

[Bernd Schmidt]

On 11/05/2015 12:33 PM, Andreas Arnez wrote:

> Thanks again for reviewing.  Are you going to look at patch #2 as well?

Yeah, still thinking about that one.

>> Does C++ have similar issues?
>
> Not this particular issue, AFAIK.  But I've just looked at how C++ fares
> with the enhanced version of pr67192.c from patch #2.  There I see the
> following:
>
>    Breakpoint 2, f4 () at pr67192.cc:54
>    (gdb) p cnt
>    $1 = 16
>
> I.e., when breaking on "while (1)" the first loop iteration has already
> executed.  This is because the C++ parser assigns the backward-goto to
> the 'while' token.  It's the same issue you pointed at with version 2 of
> my patch.
>
> Shall I open a bug for that?

I'd obviously prefer if you'd manage to get the two frontends behave 
identically. The alternative would be to open a bug.


Bernd

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

[Andreas Arnez]

On Thu, Nov 05 2015, Bernd Schmidt wrote:

> On 11/05/2015 12:33 PM, Andreas Arnez wrote:
>
>> Thanks again for reviewing.  Are you going to look at patch #2 as well?
>
> Yeah, still thinking about that one.
>
>>> Does C++ have similar issues?
>>
>> Not this particular issue, AFAIK.  But I've just looked at how C++ fares
>> with the enhanced version of pr67192.c from patch #2.  There I see the
>> following:
>>
>>    Breakpoint 2, f4 () at pr67192.cc:54
>>    (gdb) p cnt
>>    $1 = 16
>>
>> I.e., when breaking on "while (1)" the first loop iteration has already
>> executed.  This is because the C++ parser assigns the backward-goto to
>> the 'while' token.  It's the same issue you pointed at with version 2 of
>> my patch.
>>
>> Shall I open a bug for that?
>
> I'd obviously prefer if you'd manage to get the two frontends behave
> identically. The alternative would be to open a bug.

OK, I guess it depends on whether we want to go the route of patch #2.
If so, it seems we can do the same for the C++ parser, like in the patch
below.  Note that I've not tested this very much.

-- >8 --
Subject: [PATCH] C++: Fix location of loop statement

---
 gcc/cp/cp-gimplify.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index e4b50e5..d9bb708 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -266,7 +266,12 @@ genericize_cp_loop (tree *stmt_p, location_t start_locus, tree cond, tree body,
 	loop = stmt_list;
     }
   else
-    loop = build1_loc (start_locus, LOOP_EXPR, void_type_node, stmt_list);
+    {
+      location_t loc = EXPR_LOCATION (expr_first (body));
+      if (loc == UNKNOWN_LOCATION)
+	loc = start_locus;
+      loop = build1_loc (loc, LOOP_EXPR, void_type_node, stmt_list);
+    }
 
   stmt_list = NULL;
   append_to_statement_list (loop, &stmt_list);
-- 
2.3.0

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

[Jeff Law]

On 11/04/2015 09:17 AM, 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().  Note that this does not fully
> cover all cases where the back-jump gets the wrong location.
>
> 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/testsuite/ChangeLog:
>
> 	PR debug/67192
> 	* gcc.dg/guality/pr67192.c: New test.
OK.

You might consider testing C++ on the same tests.  I wouldn't be 
surprised if it shows the same problem.

jeff

Re: [PATCH v3 2/2] [PR debug/67192] Further fix C loops' back-jump location

[Jeff Law]

On 11/04/2015 09:18 AM, 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 occurred after r223098 ("Implement
> -Wmisleading-indentation"), even with a loop body enclosed in braces.
> This was because the check for misleading indentation always peeks ahead
> one token as well.
>
> This patch avoids the use of input_location and sets the location of the
> backward-goto to the start of the loop body instead, or, if there is no
> loop body, to the start of the loop.
>
> gcc/c/ChangeLog:
>
> 	PR debug/67192
> 	* c-typeck.c (c_finish_loop): For unconditional loops, set the
> 	location of the backward-goto to the start of the loop body.
>
> gcc/testsuite/ChangeLog:
>
> 	PR debug/67192
> 	* gcc.dg/guality/pr67192.c (f3, f4): New functions.
> 	(main): Invoke them.
Also OK.  And please consider using those tests with the C++ compiler to 
see if it's suffering from the same problem.

Thanks again!

jeff

Re: [PATCH v3 2/2] [PR debug/67192] Further fix C loops' back-jump location

[Andreas Krebbel]

On 11/04/2015 05:18 PM, 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 occurred after r223098 ("Implement
> -Wmisleading-indentation"), even with a loop body enclosed in braces.
> This was because the check for misleading indentation always peeks ahead
> one token as well.
> 
> This patch avoids the use of input_location and sets the location of the
> backward-goto to the start of the loop body instead, or, if there is no
> loop body, to the start of the loop.
> 
> gcc/c/ChangeLog:
> 
> 	PR debug/67192
> 	* c-typeck.c (c_finish_loop): For unconditional loops, set the
> 	location of the backward-goto to the start of the loop body.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR debug/67192
> 	* gcc.dg/guality/pr67192.c (f3, f4): New functions.
> 	(main): Invoke them.

Applied. Thanks!

-Andreas-

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

[Andreas Krebbel]

On 11/04/2015 05:17 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().  Note that this does not fully
> cover all cases where the back-jump gets the wrong location.
> 
> 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/testsuite/ChangeLog:
> 
> 	PR debug/67192
> 	* gcc.dg/guality/pr67192.c: New test.

Applied. Thanks!

-Andreas-

Re: [PATCH v3 2/2] [PR debug/67192] Further fix C loops' back-jump location

[Andreas Arnez]

On Sat, Nov 07 2015, Jeff Law wrote:

> Also OK.  And please consider using those tests with the C++ compiler
> to see if it's suffering from the same problem.

Not really, but there's still an issue.

In the C front-end the back-jump's location of an unconditional loop was
sometimes set to the token after the loop, particularly after the
misleading-indent patch.  This does *not* apply to C++.

Before the misleading-indent patch the location was usually set to the
last line of the loop instead.  This may be slightly confusing when the
loop body consists of an if-else statement: Breaking on that line then
causes a breakpoint hit on every iteration even if the else-path is
never executed.  This issue does *not* apply to C++ either.

But the C++ front-end always sets the location to the "while" or "for"
token.  This can cause confusion when setting a breakpoint there: When
hitting it for the first time, one loop iteration will already have
executed.

For that issue I included an informal patch in my earlier post.  It
mimics the C patch and seems to fix the issue:

  https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00478.html

I'll go ahead and prepare a full patch (with test case, ChangeLog, etc.)
for this.