Re: [PATCH v3 1/2] [PR debug/67192] Fix C loops' back-jump location
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 1/2] [PR debug/67192] Fix C loops' back-jump location
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 1/2] [PR debug/67192] Fix C loops' back-jump location
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
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
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
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, _list);
--
2.3.0
[PATCH v3 1/2] [PR debug/67192] Fix C loops' back-jump location
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 000..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" () : "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" () : "memory");
+ asm volatile ("" : : "r" () : "memory");
+ fnp1 ();
+ fnp2 ();
+ return 0;
+}
--
2.3.0
