| Message ID | m3wptxzqjf.fsf@oc1027705133.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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-
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.
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; }
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(+)