[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().
gcc/testsuite/ChangeLog:
PR debug/67192
* gcc.dg/guality/pr67192.c: New test.
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/c/c-parser.c | 13 +
gcc/testsuite/gcc.dg/guality/pr67192.c | 50 ++
2 files changed, 57 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 2d24c21..8740922 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -5438,13 +5438,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;
}
@@ -5728,15 +5728,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..73d4e44
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr67192.c
@@ -0,0 +1,50 @@
+/* PR debug/67192 */
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+static volatile int cnt = 0;
+
+__attribute__((noinline)) int
+f1 (void)
+{
+ return ++cnt % 5 == 0;
+}
+
+__attribute__((noinline)) void
+f2 (void)
+{
+}
+
+__attribute__((noinline)) void
+f3 (int (*last) (void), void (*do_it) (void))
+{
+ for (;;)
+{
+ if (last ())
+ break;
+ do_it ();
+}
+ do_it (); /* { dg-final { gdb-test 27 "cnt" "5" } } */
+
+ while (1)
+{
+ if (last ())
+ break;
+ do_it ();
+}
+ do_it (); /* { dg-final { gdb-test 35 "cnt" "10" } } */
+}
+
+int (*volatile fnp1) (void) = f1;
+void (*volatile fnp2) (void) = f2;
+void (*volatile fnp3) (int (*) (void), void (*) (void)) = f3;
+
+int
+main (int argc, char *argv[])
+{
+ asm volatile ("" : : "r" (&fnp1) : "memory");
+ asm volatile ("" : : "r" (&fnp2) : "memory");
+ asm volatile ("" : : "r" (&fnp3) : "memory");
+ fnp3 (fnp1, fnp2);
+ return 0;
+}
--
2.3.0
Re: [PR debug/67192] Fix C loops' back-jump location
On Tue, Oct 13 2015, Bernd Schmidt wrote: > One could argue that peek_token should not have an effect on > input_location, and in fact cpp_peek_token seems to take steps that > this does not happen, but it looks like c_parser_peek_token does not > use that mechanism. Yes, the C/C++ parsers differ quite significantly in this regard. The C parser invokes the lexer in peek_token and advances input_location upon each newline. The C++ parser usually lexes everything in advance and updates input_location on each *consumed* token. By advancing input_location in peek_token upon each newline, diagnostics emitted with warning() and friends point to the beginning of the line of the peeked-at token. This is probably somewhat intended, so I'd rather not touch that right now. A different aspect is the implicit use of input_location for the location of generated statements. This usage is what causes the problem at hand, and IMHO it should generally be rooted out. >> Still, >> >> gcc/testsuite/ChangeLog: >> >> PR debug/67192 >> * gcc.dg/guality/pr67192.c: New test. >> >> 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 fix looks simple enough. Ok. (Might want to add noclone to the > testcase attributes). Thanks for reviewing! Unfortunately, after investigating this some more, I realized that my solution is incomplete. E.g., consider this: while (1) if (foo ()) break; else do_something (); bar (); /* break here */ Interestingly, line number information for such code has been broken in GCC for a long time. I'll send an updated version.
[PATCH v2] [PR debug/67192] Fix C loops' back-jump location
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 occurs since r223098 ("Implement -Wmisleading-indentation"),
even with a loop body enclosed in braces. This is because the check for
misleading indentation always peeks ahead one token as well.
This patch adds a new parameter to c_finish_loop that expclitly
specifies the location to be used for the loop iteration. All calls to
c_finish_loop are adjusted accordingly.
gcc/c/ChangeLog:
PR debug/67192
* c-typeck.c (c_finish_loop): Replace implicit use of
input_location by new parameter iter_locus.
* c-tree.h (c_finish_loop): Adjust prototype.
* c-array-notation.c (fix_builtin_array_notation_fn): Explicitly
pass input_location as the new parameter to c_finish_loop.
(build_array_notation_expr): Likewise.
(fix_conditional_array_notations_1): Likewise.
(fix_array_notation_expr): Likewise.
(fix_array_notation_call_expr): Likewise.
* c-parser.c (c_parser_while_statement): Choose iter_locus
depending on whether the loop body is enclosed in braces, and pass
it to c_finish_loop.
(c_parser_for_statement): Likewise.
(c_parser_do_statement): Use the final semicolon's location for
iter_locus and pass it to c_finish_loop.
gcc/testsuite/ChangeLog:
PR debug/67192
* gcc.dg/guality/pr67192.c: New test.
---
gcc/c/c-array-notation.c | 15 ---
gcc/c/c-parser.c | 13 --
gcc/c/c-tree.h | 2 +-
gcc/c/c-typeck.c | 17 +---
gcc/testsuite/gcc.dg/guality/pr67192.c | 79 ++
5 files changed, 109 insertions(+), 17 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/guality/pr67192.c
diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
index 3de7569..c457eee2 100644
--- a/gcc/c/c-array-notation.c
+++ b/gcc/c/c-array-notation.c
@@ -591,7 +591,8 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree
*new_var)
for (ii = 0; ii < rank; ii++)
{
tree new_loop = push_stmt_list ();
- c_finish_loop (location, an_loop_info[ii].cmp, an_loop_info[ii].incr,
+ c_finish_loop (location, input_location, an_loop_info[ii].cmp,
+an_loop_info[ii].incr,
body, NULL_TREE, NULL_TREE, true);
body = pop_stmt_list (new_loop);
}
@@ -879,8 +880,8 @@ build_array_notation_expr (location_t location, tree lhs,
tree lhs_origtype,
append_to_statement_list_force (lhs_an_loop_info[ii].incr, &incr_list);
if (rhs_rank && rhs_an_loop_info[ii].incr)
append_to_statement_list_force (rhs_an_loop_info[ii].incr, &incr_list);
- c_finish_loop (location, cond_expr[ii], incr_list, body, NULL_TREE,
-NULL_TREE, true);
+ c_finish_loop (location, input_location, cond_expr[ii],
+incr_list, body, NULL_TREE, NULL_TREE, true);
body = pop_stmt_list (new_loop);
}
append_to_statement_list_force (body, &loop_with_init);
@@ -1004,7 +1005,8 @@ fix_conditional_array_notations_1 (tree stmt)
{
tree new_loop = push_stmt_list ();
add_stmt (an_loop_info[ii].ind_init);
- c_finish_loop (location, an_loop_info[ii].cmp, an_loop_info[ii].incr,
+ c_finish_loop (location, input_location, an_loop_info[ii].cmp,
+an_loop_info[ii].incr,
body, NULL_TREE, NULL_TREE, true);
body = pop_stmt_list (new_loop);
}
@@ -1107,7 +1109,7 @@ fix_array_notation_expr (location_t location, enum
tree_code code,
{
tree new_loop = push_stmt_list ();
add_stmt (an_loop_info[ii].ind_init);
- c_finish_loop (location, an_loop_info[ii].cmp,
+ c_finish_loop (location, input_location, an_loop_info[ii].cmp,
an_loop_info[ii].incr, body, NULL_TREE,
NULL_TREE, true);
body = pop_stmt_list (new_loop);
@@ -1193,7 +1195,8 @@ fix_array_notation_call_expr (tree arg)
{
tree new_loop = push_stmt_list ();
add_stmt (an_loop_info[ii].ind_init);
- c_finish_loop (location, an_loop_info[ii].cmp, an_loop_info[ii].incr,
+ c_finish_loop (location, input_location, an_loop_info[ii].cmp,
+an_loop_info[ii].incr,
body, NULL_TREE, NULL_TREE, true);
body = pop_stmt_list (new_loop);
}
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 704ebc6..69e0a9c 100644
--- a/gcc/c/c-p
Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location
Ping:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02393.html
On Fri, Oct 23 2015, 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 occurs since r223098 ("Implement -Wmisleading-indentation"),
> even with a loop body enclosed in braces. This is because the check for
> misleading indentation always peeks ahead one token as well.
>
> This patch adds a new parameter to c_finish_loop that expclitly
> specifies the location to be used for the loop iteration. All calls to
> c_finish_loop are adjusted accordingly.
>
> gcc/c/ChangeLog:
>
> PR debug/67192
> * c-typeck.c (c_finish_loop): Replace implicit use of
> input_location by new parameter iter_locus.
> * c-tree.h (c_finish_loop): Adjust prototype.
> * c-array-notation.c (fix_builtin_array_notation_fn): Explicitly
> pass input_location as the new parameter to c_finish_loop.
> (build_array_notation_expr): Likewise.
> (fix_conditional_array_notations_1): Likewise.
> (fix_array_notation_expr): Likewise.
> (fix_array_notation_call_expr): Likewise.
> * c-parser.c (c_parser_while_statement): Choose iter_locus
> depending on whether the loop body is enclosed in braces, and pass
> it to c_finish_loop.
> (c_parser_for_statement): Likewise.
> (c_parser_do_statement): Use the final semicolon's location for
> iter_locus and pass it to c_finish_loop.
>
> gcc/testsuite/ChangeLog:
>
> PR debug/67192
> * gcc.dg/guality/pr67192.c: New test.
Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location
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 > 32if (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
Re: [PATCH v2] [PR debug/67192] Fix C loops' back-jump location
On Thu, Oct 29 2015, Andreas Arnez wrote: > On Thu, Oct 29 2015, Bernd Schmidt wrote: >> [...] >> 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. > > [...] > 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? I've basically tried the latter, and it seems to work pretty well. It solves all the issues discussed in this mail thread, and I haven't found any other issues with it. I'll post the patch separately. >> 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. I've slightly changed the test case for that one, so I'll repost it as well before committing it. -- Andreas
[PATCH v3 0/2] Fix C loops' back-jump location
Another iteration of trying to fix the regression caused by r223098
("Implement -Wmisleading-indentation"). Patch #1 is the same as v1,
except for some minor changes to the test case. Patch #2 fixes some
additional cases where the back-jump's location was set wrongly, and
it removes the dependency on input_location for this purpose.
Tested on s390x without regressions.
Previous versions:
* v1: https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01132.html
* v2: https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02393.html
Andreas Arnez (2):
[PR debug/67192] Fix C loops' back-jump location
[PR debug/67192] Further fix C loops' back-jump location
gcc/c/c-parser.c | 13 +++---
gcc/c/c-typeck.c | 10 +
gcc/testsuite/gcc.dg/guality/pr67192.c | 79 ++
3 files changed, 96 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/guality/pr67192.c
--
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" (&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
[PATCH v3 2/2] [PR debug/67192] Further fix C loops' back-jump location
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(+)
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;
}
--
2.3.0
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, &stmt_list);
--
2.3.0
Re: [PATCH v3 2/2] [PR debug/67192] Further fix C loops' back-jump location
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.
[PATCH] Improve C++ loop's backward-jump location
While fixing PR67192 it turned out that there are multiple potential
issues with the location of the backward-jump statement generated by the
C and C++ front-ends for an unconditional loop.
First, due to a bug the C front-end had sometimes set the location to
the token following the loop. This is what triggered PR67192.
The C front-end's intention was to set the location to the last line of
the loop instead. This is not perfect either; it may be slightly
confusing in the special case where 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.
The C++ front-end does not have these issues; it sets the location to
the "while" or "for" token. But this can cause confusion when setting a
breakpoint on "while (1)": When hitting it for the first time, one loop
iteration will already have executed.
To fix this as well, this patch mimics the fix applied to the C
front-end, making the front-ends behave identically in this regard.
gcc/cp/ChangeLog:
* cp-gimplify.c (genericize_cp_loop): Change LOOP_EXPR's location
to start of loop body instead of start of loop.
gcc/testsuite/ChangeLog:
* g++.dg/guality/pr67192.C: New test.
---
gcc/cp/cp-gimplify.c | 7 ++-
gcc/testsuite/g++.dg/guality/pr67192.C | 79 ++
2 files changed, 85 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/guality/pr67192.C
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);
diff --git a/gcc/testsuite/g++.dg/guality/pr67192.C
b/gcc/testsuite/g++.dg/guality/pr67192.C
new file mode 100644
index 000..c09ecf8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/guality/pr67192.C
@@ -0,0 +1,79 @@
+/* PR debug/67192 */
+/* { dg-do run } */
+/* { dg-options "-x c++ -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" } } */
+}
+
+__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;
+}
--
2.3.0
[PING] [PATCH] Improve C++ loop's backward-jump location
Ping: https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01192.html > gcc/cp/ChangeLog: > > * cp-gimplify.c (genericize_cp_loop): Change LOOP_EXPR's location > to start of loop body instead of start of loop. > > gcc/testsuite/ChangeLog: > > * g++.dg/guality/pr67192.C: New test.
[PING^2][PATCH] Improve C++ loop's backward-jump location
Ping? https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01192.html I guess we want C and C++ behave the same here? > gcc/cp/ChangeLog: > > * cp-gimplify.c (genericize_cp_loop): Change LOOP_EXPR's location > to start of loop body instead of start of loop. > > gcc/testsuite/ChangeLog: > > * g++.dg/guality/pr67192.C: New test.
[PATCH] [PR68603] Associate conditional C++ loop's back-jump with start, not body
SVN commit r230979 always associates a loop's back-jump with the start
of the loop body. This caused a regression for gcov with conditional
loops, because then the loop body appears to be covered twice per
iteration.
gcc/cp/ChangeLog:
PR gcov-profile/68603
* cp-gimplify.c (genericize_cp_loop): For the back-jump's location
use the start of the loop body only if the loop is unconditional.
---
gcc/cp/cp-gimplify.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index a9a34cd..3c89f1b 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -264,7 +264,9 @@ genericize_cp_loop (tree *stmt_p, location_t start_locus,
tree cond, tree body,
}
else
{
- location_t loc = EXPR_LOCATION (expr_first (body));
+ location_t loc = start_locus;
+ if (!cond || integer_nonzerop (cond))
+ loc = EXPR_LOCATION (expr_first (body));
if (loc == UNKNOWN_LOCATION)
loc = start_locus;
loop = build1_loc (loc, LOOP_EXPR, void_type_node, stmt_list);
--
2.5.0
Re: [PING^2][PATCH] Improve C++ loop's backward-jump location
On Wed, Dec 02 2015, Alan Lawrence wrote: [...] > Since this, we've been seeing these tests fail natively on AArch64 and ARM: > > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++11 gcov: 3 failures in line > counts, 0 in branch percentages, 0 in return percentages, 0 in > intermediate format > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++11 line 115: is 27:should be 14 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++11 line 58: is 18:should be 9 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++11 line 73: is 162:should be 81 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++14 gcov: 3 failures in line > counts, 0 in branch percentages, 0 in return percentages, 0 in > intermediate format > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++14 line 115: is 27:should be 14 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++14 line 58: is 18:should be 9 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++14 line 73: is 162:should be 81 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++98 gcov: 3 failures in line > counts, 0 in branch percentages, 0 in return percentages, 0 in > intermediate format > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++98 line 115: is 27:should be 14 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++98 line 58: is 18:should be 9 > FAIL: g++.dg/gcov/gcov-1.C -std=gnu++98 line 73: is 162:should be 81 Right, sorry about that. See also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68603 This should be fixed now. Let me know if you still see problems. -- Andreas
PR67192 (Was: Re: [PATCH 1/3] Implement -Wmisleading-indentation (v4))
On Tue, May 12 2015, David Malcolm wrote: > On Tue, 2015-05-12 at 17:21 -0400, David Malcolm wrote: > [...] >> Thanks; I've removed the new warning from -Wall, making the appropriate >> trivial doc changes, and committed it to trunk (r223098; attached). > > ...having bootstrapped®rtested it on x86_64-unknown-linux-gnu > (Fedora 20). This patch introduces a regression that can cause a wrong line number to be emitted for the backward-goto in a loop. See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67192 Some new FAILs in the GDB test suite (when using upstream GCC) are due to this. Any chance that this gets fixed soon?
Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.
On Sat, Sep 20 2014, Mark Wielaard wrote:
> When adding DW_TAG_restrict_type I made a mistake when updating the
> code that handled types with multiple modifiers. This patch fixes it
> by putting the logic for finding the "sub-qualified" type in a separate
> function and fall back to adding the modifiers separately if there is
> no such existing type. The old tests didn't catch this case because
> there always was an existing sub-qualified type already. The new testcase
> fails before and succeeds after this patch.
>
> gcc/ChangeLog
>
> * dwarf2out.c (existing_sub_qualified_type): New function.
> (modified_type_die): Use existing_sub_qualified_type. Fall
> back to adding modifiers one by one of there is no existing
> sub-qualified type.
>
> gcc/testsuite/ChangeLog
>
> * gcc.dg/guality/pr63300-const-volatile.c: New testcase.
> ---
> gcc/dwarf2out.c| 85
> ++
> .../gcc.dg/guality/pr63300-const-volatile.c| 12 +++
> 2 files changed, 84 insertions(+), 13 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index e87ade2..0cbc316 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -10461,6 +10461,51 @@ decl_quals (const_tree decl)
>? TYPE_QUAL_VOLATILE : TYPE_UNQUALIFIED));
> }
>
> +/* Returns true if CV_QUALS contains QUAL and we have a qualified
> + variant of TYPE that has at least one other qualifier found in
> + CV_QUALS. Returns false if CV_QUALS doesn't contain QUAL, if
> + CV_QUALS is empty after subtracting QUAL, or if we don't have a
> + TYPE that has at least one qualifier from CV_QUALS minus QUAL. */
> +static bool
> +existing_sub_qualified_type (tree type, int cv_quals, int qual)
> +{
> + int sub_qual, sub_quals = cv_quals & ~qual;
> + if ((cv_quals & qual) == TYPE_UNQUALIFIED || sub_quals == TYPE_UNQUALIFIED)
> +return false;
> +
> + sub_qual = TYPE_QUAL_CONST;
> + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +return true;
> +
> + sub_qual = TYPE_QUAL_VOLATILE;
> + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +return true;
> +
> + sub_qual = TYPE_QUAL_RESTRICT;
> + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +return true;
> +
> + sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_VOLATILE;
You probably mean '|' instead of '&' here.
> + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +return true;
> +
> + sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_RESTRICT;
See above.
> + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +return true;
> +
> + sub_qual = TYPE_QUAL_VOLATILE & TYPE_QUAL_RESTRICT;
See above.
> + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> +return true;
> +
> + return false;
> +}
IIUC, 'sub_qual' above represents the qualifiers to *omit* from the ones
we're interested in, right? Maybe it would be more straightforward to
reverse the logic, i.e., start with
sub_qual = TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT;
and then always use sub_qual instead of ~sub_qual.
Also note that the logic wouldn't scale too well for yet more
qualifiers...
> +
> /* Given a pointer to an arbitrary ..._TYPE tree node, return a debugging
> entry that chains various modifiers in front of the given type. */
>
> @@ -10543,34 +10588,48 @@ modified_type_die (tree type, int cv_quals,
> dw_die_ref context_die)
>
>mod_scope = scope_die_for (type, context_die);
>
> - if ((cv_quals & TYPE_QUAL_CONST)
> - /* If there are multiple type modifiers, prefer a path which
> - leads to a qualified type. */
> - && (((cv_quals & ~TYPE_QUAL_CONST) == TYPE_UNQUALIFIED)
> - || get_qualified_type (type, cv_quals) == NULL_TREE
> - || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_CONST)
> - != NULL_TREE)))
> + /* If there are multiple type modifiers, prefer a path which
> + leads to a qualified type. */
> + if (existing_sub_qualified_type (type, cv_quals, TYPE_QUAL_CONST))
> {
>mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
>sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_CONST,
> context_die);
> }
> - else if ((cv_quals & TYPE_QUAL_VOLATILE)
> -&& (((cv_quals & ~TYPE_QUAL_VOLATILE) == TYPE_UNQUALIFIED)
> -|| get_qualified_type (type, cv_quals) == NULL_TREE
> -|| (get_qualified_type (type, cv_quals & ~TYPE_QUAL_VOLATILE)
> -
Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.
On Mon, Sep 22 2014, Andreas Arnez wrote:
> On Sat, Sep 20 2014, Mark Wielaard wrote:
>
>> When adding DW_TAG_restrict_type I made a mistake when updating the
>> code that handled types with multiple modifiers. This patch fixes it
>> by putting the logic for finding the "sub-qualified" type in a separate
>> function and fall back to adding the modifiers separately if there is
>> no such existing type. The old tests didn't catch this case because
>> there always was an existing sub-qualified type already. The new testcase
>> fails before and succeeds after this patch.
>>
>> gcc/ChangeLog
>>
>> * dwarf2out.c (existing_sub_qualified_type): New function.
>> (modified_type_die): Use existing_sub_qualified_type. Fall
>> back to adding modifiers one by one of there is no existing
>> sub-qualified type.
>>
> [...]
>
> Also note that the logic wouldn't scale too well for yet more
> qualifiers...
Considering this, I've tried a different approach below. What do you
think?
-- >8 --
Subject: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.
When adding DW_TAG_restrict_type the handling of multiple modifiers
was adjusted incorrectly. This patch fixes it with the help of a new
tree function get_nearest_type_subqualifiers.
gcc/ChangeLog
* tree.c (check_base_type): New.
(check_qualified_type): Exploit new helper function above.
(get_nearest_type_subqualifiers): New.
* tree.h (get_nearest_type_subqualifiers): New prototype.
* dwarf2out.c (modified_type_die): Fix handling for qualifiers.
Next qualifier to "peel off" is now determined with the help of
get_nearest_type_subqualifiers.
---
gcc/dwarf2out.c | 61 +
gcc/tree.c | 51 ++-
gcc/tree.h | 7 +++
3 files changed, 84 insertions(+), 35 deletions(-)
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index e87ade2..ec881d1 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -10474,12 +10474,14 @@ modified_type_die (tree type, int cv_quals,
dw_die_ref context_die)
tree qualified_type;
tree name, low, high;
dw_die_ref mod_scope;
+ /* Only these cv-qualifiers are currently handled. */
+ const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
+ | TYPE_QUAL_RESTRICT);
if (code == ERROR_MARK)
return NULL;
- /* Only these cv-qualifiers are currently handled. */
- cv_quals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+ cv_quals &= cv_qual_mask;
/* Don't emit DW_TAG_restrict_type for DWARFv2, since it is a type
tag modifier (and not an attribute) old consumers won't be able
@@ -10530,7 +10532,7 @@ modified_type_die (tree type, int cv_quals, dw_die_ref
context_die)
else
{
int dquals = TYPE_QUALS_NO_ADDR_SPACE (dtype);
- dquals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+ dquals &= cv_qual_mask;
if ((dquals & ~cv_quals) != TYPE_UNQUALIFIED
|| (cv_quals == dquals && DECL_ORIGINAL_TYPE (name) != type))
/* cv-unqualified version of named type. Just use
@@ -10543,33 +10545,32 @@ modified_type_die (tree type, int cv_quals,
dw_die_ref context_die)
mod_scope = scope_die_for (type, context_die);
- if ((cv_quals & TYPE_QUAL_CONST)
- /* If there are multiple type modifiers, prefer a path which
-leads to a qualified type. */
- && (((cv_quals & ~TYPE_QUAL_CONST) == TYPE_UNQUALIFIED)
- || get_qualified_type (type, cv_quals) == NULL_TREE
- || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_CONST)
- != NULL_TREE)))
-{
- mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
- sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_CONST,
- context_die);
-}
- else if ((cv_quals & TYPE_QUAL_VOLATILE)
- && (((cv_quals & ~TYPE_QUAL_VOLATILE) == TYPE_UNQUALIFIED)
- || get_qualified_type (type, cv_quals) == NULL_TREE
- || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_VOLATILE)
- != NULL_TREE)))
-{
- mod_type_die = new_die (DW_TAG_volatile_type, mod_scope, type);
- sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_VOLATILE,
- context_die);
-}
- else if (cv_quals & TYPE_QUAL_RESTRICT)
-{
- mod_type_die = new_die (DW_TAG_restrict_type, mod_scope, type);
- sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_RESTRICT,
- context_die);
+ if (cv_quals)
+{
+ int q;
+ enum
Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.
On Tue, Sep 23 2014, Mark Wielaard wrote: > On Mon, Sep 22, 2014 at 10:59:38AM +0200, Andreas Arnez wrote: >> > + sub_qual = TYPE_QUAL_RESTRICT; >> > + if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED >> > + && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE) >> > +return true; >> > + >> > + sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_VOLATILE; >> >> You probably mean '|' instead of '&' here. > > Eep. Yes (3x). > > But then I tried to write a testcase to show the cases that fail > because of this typo. And failed, everything works fine. That is > because these checks are bogus. sub_quals contains at most 2 qualifiers. > const, volatile and restrict, minus the qual we are interested in. > And subtracting two always results in the empty set. So I removed > those 3 cases. Right, the checks were bogus. But from looking at the code before adding restrict, I guess it was intended to *minimze* the number of generated DIEs. If we keep that goal, maybe the function should actually return the "rank" of the found sub-qualified type (i.e., the number of matching sub-qualifiers) and the caller should follow the path with the highest rank. Then the checks would be needed again, and they would even have to be doubled for 'atomic'. Without such handling there are cases where more DIEs than necessary are created, e.g. if we have the following types: some_base_t *const some_base_t *volatile restrict some_base_t *const volatile restrict Then the latter is based on the first instead of the second, although this requires one more DIE.
Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.
On Tue, Sep 23 2014, Mark Wielaard wrote:
> This certainly looks nicer than how I wrote it. It took me a while
> (again) to realize why this works. We rely on the fact that earlier in
> the function a match would have been found if there was already a fully
> qualified type available. So here we know some subset will be found and
> at least one qualifier we need will not be in the result returned by
> get_nearest_type_subqualifiers. Maybe add a comment saying that to the
> code?
>
> Could you add the testcases I wrote for my variant of the fix to your
> patch and make sure they PASS?
OK, here's the adjusted patch with a comment added and your testcases
included. I changed the patch a bit further, to reduce unnecessary
iterations and recursions, and tested it again.
A few style aspects I'm not sure about:
* Is it OK to use __builtin_popcount in tree.c?
* Is it acceptable to add such a specialized function as
get_nearest_type_subqualifiers to the tree interface? Or would it be
preferable to move it as a static function to dwarf2out.c, since
that's the only user right now?
-- >8 --
Subject: [PATCH v3] PR63300 'const volatile' sometimes stripped in debug info.
When adding DW_TAG_restrict_type the handling of multiple modifiers
was adjusted incorrectly. This patch fixes it with the help of a new
tree function get_nearest_type_subqualifiers. The old tests didn't
catch this case because there always was an existing sub-qualified
type already. The new guality testcase fails before and succeeds
after this patch. The new dwarf2 testcases make sure the optimization
works and doesn't introduce unnecessary type tags.
gcc/ChangeLog
* tree.c (check_base_type): New.
(check_qualified_type): Exploit new helper function above.
(get_nearest_type_subqualifiers): New.
* tree.h (get_nearest_type_subqualifiers): New prototype.
* dwarf2out.c (modified_type_die): Fix handling for qualifiers.
Next qualifier to "peel off" is now determined with the help of
get_nearest_type_subqualifiers.
gcc/testsuite/ChangeLog
* gcc.dg/debug/dwarf2/stacked-qualified-types-1.c: New testcase.
* gcc.dg/debug/dwarf2/stacked-qualified-types-2.c: Likewise.
* gcc.dg/guality/pr63300-const-volatile.c: New testcase.
---
gcc/dwarf2out.c| 62 +++---
.../debug/dwarf2/stacked-qualified-types-1.c | 18 +++
.../debug/dwarf2/stacked-qualified-types-2.c | 19 +++
.../gcc.dg/guality/pr63300-const-volatile.c| 12 +
gcc/tree.c | 52 --
gcc/tree.h | 7 +++
6 files changed, 135 insertions(+), 35 deletions(-)
create mode 100644
gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c
create mode 100644
gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c
create mode 100644 gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index e87ade2..abd9df9 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -10474,12 +10474,14 @@ modified_type_die (tree type, int cv_quals,
dw_die_ref context_die)
tree qualified_type;
tree name, low, high;
dw_die_ref mod_scope;
+ /* Only these cv-qualifiers are currently handled. */
+ const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
+ | TYPE_QUAL_RESTRICT);
if (code == ERROR_MARK)
return NULL;
- /* Only these cv-qualifiers are currently handled. */
- cv_quals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+ cv_quals &= cv_qual_mask;
/* Don't emit DW_TAG_restrict_type for DWARFv2, since it is a type
tag modifier (and not an attribute) old consumers won't be able
@@ -10530,7 +10532,7 @@ modified_type_die (tree type, int cv_quals, dw_die_ref
context_die)
else
{
int dquals = TYPE_QUALS_NO_ADDR_SPACE (dtype);
- dquals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+ dquals &= cv_qual_mask;
if ((dquals & ~cv_quals) != TYPE_UNQUALIFIED
|| (cv_quals == dquals && DECL_ORIGINAL_TYPE (name) != type))
/* cv-unqualified version of named type. Just use
@@ -10543,33 +10545,33 @@ modified_type_die (tree type, int cv_quals,
dw_die_ref context_die)
mod_scope = scope_die_for (type, context_die);
- if ((cv_quals & TYPE_QUAL_CONST)
- /* If there are multiple type modifiers, prefer a path which
-leads to a qualified type. */
- && (((cv_quals & ~TYPE_QUAL_CONST) == TYPE_UNQUALIFIED)
- || get_qualified_type (type, cv_quals) == NULL_TREE
- || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_CONST)
- != NULL_TREE)))
-{
- mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
- sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_CONST,
-
Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.
On Wed, Sep 24 2014, Jakub Jelinek wrote:
> On Wed, Sep 24, 2014 at 02:40:14PM +0200, Andreas Arnez wrote:
>> A few style aspects I'm not sure about:
>>
>> * Is it OK to use __builtin_popcount in tree.c?
>
> Definitely not, you can use popcount_hwi instead, which for GCC
> host compiler (>= 3.4) will use __builtin_popcount*, otherwise
> fallback to a library function.
>
>> * Is it acceptable to add such a specialized function as
>> get_nearest_type_subqualifiers to the tree interface? Or would it be
>> preferable to move it as a static function to dwarf2out.c, since
>> that's the only user right now?
>
> I agree it should be kept in dwarf2out.c, it is too specialized.
>
> Jakub
OK, I'm using popcount_hwi now and moved get_nearest_type_subqualifiers
to dwarf2out.c. Does this look OK?
-- >8 --
Subject: [PATCH v4] PR63300 'const volatile' sometimes stripped in debug info.
When adding DW_TAG_restrict_type the handling of multiple modifiers
was adjusted incorrectly. This patch fixes it with the help of a new
tree function get_nearest_type_subqualifiers. The old tests didn't
catch this case because there always was an existing sub-qualified
type already. The new guality testcase fails before and succeeds
after this patch. The new dwarf2 testcases make sure the optimization
works and doesn't introduce unnecessary type tags.
gcc/ChangeLog
* tree.c (check_base_type): New.
(check_qualified_type): Exploit new helper function above.
* tree.h (check_base_type): New prototype.
* dwarf2out.c (get_nearest_type_subqualifiers): New.
(modified_type_die): Fix handling for qualifiers. Qualifiers to
"peel off" are now determined using get_nearest_type_subqualifiers.
gcc/testsuite/ChangeLog
* gcc.dg/debug/dwarf2/stacked-qualified-types-1.c: New testcase.
* gcc.dg/debug/dwarf2/stacked-qualified-types-2.c: Likewise.
* gcc.dg/guality/pr63300-const-volatile.c: New testcase.
---
gcc/dwarf2out.c| 96 +++---
.../debug/dwarf2/stacked-qualified-types-1.c | 18
.../debug/dwarf2/stacked-qualified-types-2.c | 19 +
.../gcc.dg/guality/pr63300-const-volatile.c| 12 +++
gcc/tree.c | 16 +++-
gcc/tree.h | 4 +
6 files changed, 131 insertions(+), 34 deletions(-)
create mode 100644
gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c
create mode 100644
gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c
create mode 100644 gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index e87ade2..e15b42b 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -10461,6 +10461,40 @@ decl_quals (const_tree decl)
? TYPE_QUAL_VOLATILE : TYPE_UNQUALIFIED));
}
+/* Determine the TYPE whose qualifiers match the largest strict subset
+ of the given TYPE_QUALS, and return its qualifiers. Ignore all
+ qualifiers outside QUAL_MASK. */
+
+static int
+get_nearest_type_subqualifiers (tree type, int type_quals, int qual_mask)
+{
+ tree t;
+ int best_rank = 0, best_qual = 0, max_rank;
+
+ type_quals &= qual_mask;
+ max_rank = popcount_hwi (type_quals) - 1;
+
+ for (t = TYPE_MAIN_VARIANT (type); t && best_rank < max_rank;
+ t = TYPE_NEXT_VARIANT (t))
+{
+ int q = TYPE_QUALS (t) & qual_mask;
+
+ if ((q & type_quals) == q && q != type_quals
+ && check_base_type (t, type))
+ {
+ int rank = popcount_hwi (q);
+
+ if (rank > best_rank)
+ {
+ best_rank = rank;
+ best_qual = q;
+ }
+ }
+}
+
+ return best_qual;
+}
+
/* Given a pointer to an arbitrary ..._TYPE tree node, return a debugging
entry that chains various modifiers in front of the given type. */
@@ -10474,12 +10508,14 @@ modified_type_die (tree type, int cv_quals,
dw_die_ref context_die)
tree qualified_type;
tree name, low, high;
dw_die_ref mod_scope;
+ /* Only these cv-qualifiers are currently handled. */
+ const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
+ | TYPE_QUAL_RESTRICT);
if (code == ERROR_MARK)
return NULL;
- /* Only these cv-qualifiers are currently handled. */
- cv_quals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+ cv_quals &= cv_qual_mask;
/* Don't emit DW_TAG_restrict_type for DWARFv2, since it is a type
tag modifier (and not an attribute) old consumers won't be able
@@ -10530,7 +10566,7 @@ modified_type_die (tree type, int cv_quals, dw_die_ref
context_die)
else
{
int dquals = TYPE_QUALS_NO_ADDR_SPACE (dtype);
- dquals &=
