On Thu, Oct 29 2015, Bernd Schmidt wrote:
> On 10/23/2015 11:14 AM, Andreas Arnez wrote:
>> + bool is_braced = c_parser_next_token_is (parser, CPP_OPEN_BRACE);
>> body = c_parser_c99_block_statement (parser);
>> + location_t iter_loc = is_braced ? input_location : loc;
>>
>> 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);
>> + c_finish_loop (loc, iter_loc, cond, NULL, body, c_break_label,
>> + c_cont_label, true);
>
> I'm not entirely sure I understand what the is_braced logic is trying
> to achieve.
>
> I tried the patch out with the debugger on the testcase
> you provided, and I get slightly strange behaviour in f2:
>
> Starting program: /local/src/egcs/bwork-git/gcc/a.out
>
> Breakpoint 7, f2 () at pr67192.c:32
> 32 if (last ())
> (gdb) cont
> Continuing.
>
> Breakpoint 6, f2 () at pr67192.c:31
> 31 while (1)
> (gdb)
>
> i.e. the breakpoint on the code inside the loop is reached before the
> while statement itself. This may be the expected behaviour with your
> patch, but I'm not sure it's really desirable for debugging.
Good point. This happens because no code is generated for "while (1)",
except the backward-goto.
If the loop body is enclosed in braces, then the patch associates the
backward-goto with the line of the closing brace (which usually stands
on a line by itself), otherwise with the while- or for-token. By
contrast, the approach that seems to have been intended before is to
generally associate the backward-goto with the loop body's last line.
But firstly, this may cause confusion as well, e.g. the following could
then stop on every loop iteration, even if the else-branch is never
executed:
while (1)
if (do_foo ())
foo ();
else
bar (); /* <- break here */
Secondly, AFAIK, c_parser_c99_block_statement currently provides no way
of retrieving the last token's location. One way of getting there might
be to update input_location on each *consumed* token instead of every
peeked-at token, similar to what the C++ parser does. But that would
probably be a fairly large change and might have lots of other pitfalls.
Or we could pass it around as an extra parameter? *Or* add an end_locus
to the tree_exp struct?
Maybe we could also improve the behavior of breaking on "while (1)" by
generating a NOP for it? Or by using the first loop body's token
instead?
Ideas/thoughts?
> In f4 placing a breakpoint on the while (1) just places it on the if
> (last ()) line. The same behaviour appears to occur for both while
> loops with the system gcc-4.8.5. So I think there are some strange
> inconsistencies going on here, and for debugging the old behaviour may
> well be better.
Possibly in some cases. But not where the old behavior had real bugs.
For instance, even with gcc-4.8.5 the breakpoint in f1 is set
incorrectly, right?
Also note that the "last loop body line" approach has inconsistencies as
well. E.g. in my example above the meaning of the breakpoint on the
else-branch then depends on whether the loop body is enclosed in braces
or not.
> I'd suggest you commit your original patch to fix the
> misleading-indent problem while we sort this out.
I can certainly do that. But note that the original patch does not
solve the misleading-indent regression caused for f2() in the new
version of pr67192.c. Thus the PR is not really fixed by it.
--
Andreas
