On April 25, 2019 11:13:34 AM GMT+02:00, Jakub Jelinek <ja...@redhat.com> wrote:
>Hi!
>
>The introduction of -gstatement-frontiers (and making it the default)
>regressed debug info experience on the attached testcases (not
>including
>in the testsuite, as not sure how to model that in the guality
>testsuite).
>The current behavior is that
>b test
>r
>n
>n
>n
>shows the first iteration and then executes the rest of the loop
>iteration
>in one go and breaks next time on __libc_start_main.
>
>The following patch fixes that by adding DEBUG_BEGIN_STMTs not just at
>the
>start of the loop, but also at the start of the incr-expr and
>cond-expr;
>the DWARF standard says that is_stmt 1 is a recommended breakpoint
>location,
>doesn't necessarily mean full statement, but also a part of it if it is
>a
>recommended breakpoint location there. And the loop expressions
>clearly
>must be recommended breakpoint locations.
>
>The difference in *.gimple dumps on the 3 testcases is:
>--- pr90197-1.c.005t.gimple_ 2019-04-25 10:55:55.087978371 +0200
>+++ pr90197-1.c.005t.gimple 2019-04-25 10:56:08.845759542 +0200
>@@ -7,23 +7,25 @@ test (unsigned int * dst, unsigned int b
>
> # DEBUG BEGIN_STMT
> i = 0;
> goto <D.1912>;
> <D.1911>:
> # DEBUG BEGIN_STMT
> _1 = (long unsigned int) i;
> _2 = _1 * 4;
> _3 = dst + _2;
> *_3 = base;
>+ # DEBUG BEGIN_STMT
> i = i + 1;
> base = base + 15;
> <D.1912>:
>+ # DEBUG BEGIN_STMT
> if (i < count) goto <D.1911>; else goto <D.1913>;
> <D.1913>:
> }
> }
>
>
> main ()
> {
> int D.1919;
>
>--- pr90197-2.c.005t.gimple_ 2019-04-25 10:55:55.192976701 +0200
>+++ pr90197-2.c.005t.gimple 2019-04-25 10:56:08.925758270 +0200
>@@ -10,20 +10,21 @@ test (unsigned int * dst, unsigned int b
> <D.1911>:
> # DEBUG BEGIN_STMT
> base = base + 15;
> i.0_1 = i;
> i = i.0_1 + 1;
> _2 = (long unsigned int) i.0_1;
> _3 = _2 * 4;
> _4 = dst + _3;
> *_4 = base;
> <D.1912>:
>+ # DEBUG BEGIN_STMT
> if (i < count) goto <D.1911>; else goto <D.1913>;
> <D.1913>:
> }
>
>
> main ()
> {
> int D.1919;
>
> {
>--- pr90197-3.c.005t.gimple_ 2019-04-25 10:55:55.298975015 +0200
>+++ pr90197-3.c.005t.gimple 2019-04-25 10:56:09.000757077 +0200
>@@ -8,20 +8,21 @@ test (unsigned int * dst, unsigned int b
> # DEBUG BEGIN_STMT
> <D.1911>:
> # DEBUG BEGIN_STMT
> base = base + 15;
> i.0_1 = i;
> i = i.0_1 + 1;
> _2 = (long unsigned int) i.0_1;
> _3 = _2 * 4;
> _4 = dst + _3;
> *_4 = base;
>+ # DEBUG BEGIN_STMT
> if (i < count) goto <D.1911>; else goto <D.1912>;
> <D.1912>:
> }
>
>
> main ()
> {
> int D.1918;
>
> {
>so it basically adds a DEBUG_BEGIN_STMT for the increment expression
>in for (if not empty) and for the condition in all kinds of loop (both
>if it
>is empty and if it is not).
>
>The assembly changes are:
>--- pr90197-1.s1 2019-04-25 11:09:54.749622921 +0200
>+++ pr90197-1.s2 2019-04-25 11:09:46.569753036 +0200
>@@ -13,6 +13,7 @@ test:
> .loc 1 5 3
> .LBB2:
> .loc 1 5 8
>+ .loc 1 5 19
> .loc 1 5 3 is_stmt 0
> testl %edx, %edx
> jle .L1
>@@ -27,11 +28,13 @@ test:
> .loc 1 6 5 is_stmt 1 discriminator 3
> .loc 1 6 12 is_stmt 0 discriminator 3
> movl %esi, (%rdi)
>- .loc 1 5 40 discriminator 3
>+ .loc 1 5 30 is_stmt 1 discriminator 3
>+ .loc 1 5 40 is_stmt 0 discriminator 3
> addl $15, %esi
> .LVL2:
>+ .loc 1 5 19 is_stmt 1 discriminator 3
> addq $4, %rdi
>- .loc 1 5 3 discriminator 3
>+ .loc 1 5 3 is_stmt 0 discriminator 3
> cmpl %eax, %esi
> jne .L3
> .L1:
>--- pr90197-2.s1 2019-04-25 11:09:54.791622253 +0200
>+++ pr90197-2.s2 2019-04-25 11:09:46.611752367 +0200
>@@ -12,7 +12,7 @@ test:
> .LVL0:
> .loc 1 5 3
> .loc 1 6 3
>- .loc 1 6 9 is_stmt 0
>+ .loc 1 6 9
> testl %edx, %edx
> jle .L1
> movl %edx, %eax
>@@ -23,18 +23,18 @@ test:
> .p2align 4,,10
> .p2align 3
> .L3:
>- .loc 1 7 5 is_stmt 1
>+ .loc 1 7 5
> .loc 1 7 22 is_stmt 0
> addl $15, %esi
> .LVL2:
> addq $4, %rdi
> .loc 1 7 14
> movl %esi, -4(%rdi)
>- .loc 1 6 9
>+ .loc 1 6 9 is_stmt 1
> cmpl %eax, %esi
> jne .L3
> .L1:
>- .loc 1 8 1
>+ .loc 1 8 1 is_stmt 0
> ret
> .cfi_endproc
> .LFE0:
>--- pr90197-3.s1 2019-04-25 11:09:54.828621665 +0200
>+++ pr90197-3.s2 2019-04-25 11:09:46.647751794 +0200
>@@ -24,9 +24,10 @@ test:
> .LVL2:
> .loc 1 7 14 discriminator 1
> movl %esi, (%rdi,%rax,4)
>+ .loc 1 8 9 is_stmt 1 discriminator 1
> addq $1, %rax
> .LVL3:
>- .loc 1 8 3 discriminator 1
>+ .loc 1 8 3 is_stmt 0 discriminator 1
> cmpl %eax, %edx
> jg .L2
> .loc 1 9 1
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
So if you put a breakpoint on the for line is the behavior after the patch now
the same as before stmt frontiers if you do 'continue' repeatedly?
Richard.
>I'll work on a C++ FE version of this next (needed as well).
>
>2019-04-25 Jakub Jelinek <ja...@redhat.com>
>
> PR debug/90197
> * c-tree.h (c_finish_loop): Add 2 further location_t arguments.
> * c-parser.c (c_parser_while_statement): Adjust c_finish_loop caller.
> (c_parser_do_statement): Likewise.
> (c_parser_for_statement): Likewise. Formatting fixes.
> * c-typeck.c (c_finish_loop): Add COND_LOCUS and INCR_LOCUS arguments,
> emit DEBUG_BEGIN_STMTs if needed.
>
>--- gcc/c/c-tree.h.jj 2019-04-08 10:11:26.942246707 +0200
>+++ gcc/c/c-tree.h 2019-04-24 13:35:02.313565354 +0200
>@@ -694,7 +694,8 @@ extern int c_types_compatible_p (tree, t
> extern tree c_begin_compound_stmt (bool);
> extern tree c_end_compound_stmt (location_t, tree, bool);
> extern void c_finish_if_stmt (location_t, tree, tree, tree);
>-extern void c_finish_loop (location_t, tree, tree, tree, tree, tree,
>bool);
>+extern void c_finish_loop (location_t, location_t, tree, location_t,
>tree,
>+ tree, tree, tree, bool);
> extern tree c_begin_stmt_expr (void);
> extern tree c_finish_stmt_expr (location_t, tree);
> extern tree c_process_expr_stmt (location_t, tree);
>--- gcc/c/c-parser.c.jj 2019-03-14 23:44:26.876575932 +0100
>+++ gcc/c/c-parser.c 2019-04-24 13:43:05.136794313 +0200
>@@ -6001,7 +6001,8 @@ c_parser_while_statement (c_parser *pars
> location_t loc_after_labels;
> bool open_brace = c_parser_next_token_is (parser, CPP_OPEN_BRACE);
> body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
>- c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label,
>true);
>+ c_finish_loop (loc, loc, cond, UNKNOWN_LOCATION, NULL, body,
>+ c_break_label, c_cont_label, true);
> add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
> c_parser_maybe_reclassify_token (parser);
>
>@@ -6046,6 +6047,7 @@ c_parser_do_statement (c_parser *parser,
> c_break_label = save_break;
> new_cont = c_cont_label;
> c_cont_label = save_cont;
>+ location_t cond_loc = c_parser_peek_token (parser)->location;
> cond = c_parser_paren_condition (parser);
> if (ivdep && cond != error_mark_node)
> cond = build3 (ANNOTATE_EXPR, TREE_TYPE (cond), cond,
>@@ -6059,7 +6061,8 @@ c_parser_do_statement (c_parser *parser,
> build_int_cst (integer_type_node, unroll));
> if (!c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
> c_parser_skip_to_end_of_block_or_statement (parser);
>- c_finish_loop (loc, cond, NULL, body, new_break, new_cont, false);
>+ c_finish_loop (loc, cond_loc, cond, UNKNOWN_LOCATION, NULL, body,
>+ new_break, new_cont, false);
> add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
> }
>
>@@ -6132,7 +6135,9 @@ c_parser_for_statement (c_parser *parser
> /* Silence the bogus uninitialized warning. */
> tree collection_expression = NULL;
> location_t loc = c_parser_peek_token (parser)->location;
>- location_t for_loc = c_parser_peek_token (parser)->location;
>+ location_t for_loc = loc;
>+ location_t cond_loc = UNKNOWN_LOCATION;
>+ location_t incr_loc = UNKNOWN_LOCATION;
> bool is_foreach_statement = false;
> gcc_assert (c_parser_next_token_is_keyword (parser, RID_FOR));
> token_indent_info for_tinfo
>@@ -6166,7 +6171,8 @@ c_parser_for_statement (c_parser *parser
> c_parser_consume_token (parser);
> is_foreach_statement = true;
> if (check_for_loop_decls (for_loc, true) == NULL_TREE)
>- c_parser_error (parser, "multiple iterating variables in fast
>enumeration");
>+ c_parser_error (parser, "multiple iterating variables in "
>+ "fast enumeration");
> }
> else
> check_for_loop_decls (for_loc, flag_isoc99);
>@@ -6196,7 +6202,8 @@ c_parser_for_statement (c_parser *parser
> c_parser_consume_token (parser);
> is_foreach_statement = true;
> if (check_for_loop_decls (for_loc, true) == NULL_TREE)
>- c_parser_error (parser, "multiple iterating variables in
>fast
>enumeration");
>+ c_parser_error (parser, "multiple iterating variables in "
>+ "fast enumeration");
> }
> else
> check_for_loop_decls (for_loc, flag_isoc99);
>@@ -6218,15 +6225,18 @@ c_parser_for_statement (c_parser *parser
> c_parser_consume_token (parser);
> is_foreach_statement = true;
> if (! lvalue_p (init_expression))
>- c_parser_error (parser, "invalid iterating variable in fast
>enumeration");
>- object_expression = c_fully_fold (init_expression, false, NULL);
>+ c_parser_error (parser, "invalid iterating variable in "
>+ "fast enumeration");
>+ object_expression
>+ = c_fully_fold (init_expression, false, NULL);
> }
> else
> {
> ce = convert_lvalue_to_rvalue (loc, ce, true, false);
> init_expression = ce.value;
> c_finish_expr_stmt (loc, init_expression);
>- c_parser_skip_until_found (parser, CPP_SEMICOLON, "expected
>%<;%>");
>+ c_parser_skip_until_found (parser, CPP_SEMICOLON,
>+ "expected %<;%>");
> }
> }
> }
>@@ -6235,18 +6245,19 @@ c_parser_for_statement (c_parser *parser
> gcc_assert (!parser->objc_could_be_foreach_context);
> if (!is_foreach_statement)
> {
>+ cond_loc = c_parser_peek_token (parser)->location;
> if (c_parser_next_token_is (parser, CPP_SEMICOLON))
> {
> if (ivdep)
> {
>- c_parser_error (parser, "missing loop condition in loop with "
>- "%<GCC ivdep%> pragma");
>+ c_parser_error (parser, "missing loop condition in loop "
>+ "with %<GCC ivdep%> pragma");
> cond = error_mark_node;
> }
> else if (unroll)
> {
>- c_parser_error (parser, "missing loop condition in loop with "
>- "%<GCC unroll%> pragma");
>+ c_parser_error (parser, "missing loop condition in loop "
>+ "with %<GCC unroll%> pragma");
> cond = error_mark_node;
> }
> else
>@@ -6275,11 +6286,13 @@ c_parser_for_statement (c_parser *parser
> /* Parse the increment expression (the third expression in a
> for-statement). In the case of a foreach-statement, this is
> the expression that follows the 'in'. */
>+ loc = incr_loc = c_parser_peek_token (parser)->location;
> if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
> {
> if (is_foreach_statement)
> {
>- c_parser_error (parser, "missing collection in fast
>enumeration");
>+ c_parser_error (parser,
>+ "missing collection in fast enumeration");
> collection_expression = error_mark_node;
> }
> else
>@@ -6288,8 +6301,8 @@ c_parser_for_statement (c_parser *parser
> else
> {
> if (is_foreach_statement)
>- collection_expression = c_fully_fold (c_parser_expression
>(parser).value,
>- false, NULL);
>+ collection_expression
>+ = c_fully_fold (c_parser_expression (parser).value, false,
>NULL);
> else
> {
> struct c_expr ce = c_parser_expression (parser);
>@@ -6312,10 +6325,14 @@ c_parser_for_statement (c_parser *parser
> body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
>
> if (is_foreach_statement)
>- objc_finish_foreach_loop (loc, object_expression,
>collection_expression, body, c_break_label, c_cont_label);
>+ objc_finish_foreach_loop (for_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 ()));
>+ c_finish_loop (for_loc, cond_loc, cond, incr_loc, incr, body,
>+ c_break_label, c_cont_label, true);
>+ add_stmt (c_end_compound_stmt (for_loc, block,
>+ flag_isoc99 || c_dialect_objc ()));
> c_parser_maybe_reclassify_token (parser);
>
> token_indent_info next_tinfo
>--- gcc/c/c-typeck.c.jj 2019-04-19 13:54:36.543027196 +0200
>+++ gcc/c/c-typeck.c 2019-04-24 14:52:26.412862798 +0200
>@@ -10858,11 +10858,14 @@ c_finish_if_stmt (location_t if_locus, t
> the beginning of the loop. COND is the loop condition. COND_IS_FIRST
> is false for DO loops. INCR is the FOR increment expression. BODY is
>the statement controlled by the loop. BLAB is the break label. CLAB
>is
>- the continue label. Everything is allowed to be NULL. */
>+ the continue label. Everything is allowed to be NULL.
>+ COND_LOCUS is the location of the loop condition, INCR_LOCUS is the
>+ location of the FOR increment expression. */
>
> void
>-c_finish_loop (location_t start_locus, tree cond, tree incr, tree
>body,
>- tree blab, tree clab, bool cond_is_first)
>+c_finish_loop (location_t start_locus, location_t cond_locus, tree
>cond,
>+ location_t incr_locus, tree incr, tree body, tree blab,
>+ tree clab, bool cond_is_first)
> {
> tree entry = NULL, exit = NULL, t;
>
>@@ -10904,12 +10907,8 @@ c_finish_loop (location_t start_locus, t
> }
>
> t = build_and_jump (&blab);
>- if (cond_is_first)
>- exit = fold_build3_loc (start_locus,
>- COND_EXPR, void_type_node, cond, exit, t);
>- else
>- exit = fold_build3_loc (input_location,
>- COND_EXPR, void_type_node, cond, exit, t);
>+ exit = fold_build3_loc (cond_is_first ? start_locus :
>input_location,
>+ COND_EXPR, void_type_node, cond, exit, t);
> }
> else
> {
>@@ -10930,9 +10929,23 @@ c_finish_loop (location_t start_locus, t
> if (clab)
> add_stmt (build1 (LABEL_EXPR, void_type_node, clab));
> if (incr)
>- add_stmt (incr);
>+ {
>+ if (MAY_HAVE_DEBUG_MARKER_STMTS && incr_locus !=
>UNKNOWN_LOCATION)
>+ {
>+ t = build0 (DEBUG_BEGIN_STMT, void_type_node);
>+ SET_EXPR_LOCATION (t, incr_locus);
>+ add_stmt (t);
>+ }
>+ add_stmt (incr);
>+ }
> if (entry)
> add_stmt (entry);
>+ if (MAY_HAVE_DEBUG_MARKER_STMTS && cond_locus != UNKNOWN_LOCATION)
>+ {
>+ t = build0 (DEBUG_BEGIN_STMT, void_type_node);
>+ SET_EXPR_LOCATION (t, cond_locus);
>+ add_stmt (t);
>+ }
> if (exit)
> add_stmt (exit);
> if (blab)
>
> Jakub
