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 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
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 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
[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" (&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