Re: [PATCH] Implement smart multiple switch expansion algorithms.
Hi. I have watched Martin's talk on switch lowering improvements ( https://slideslive.com/38902416/switch-lowering-improvements), the last slide has a question about benchmarks that can be used for tuning the switch statement optimizer. Martin mentioned one common use case - bytecode interpreters (such as perlbench from spec CPU 2006 and 2017). But there is a caveat with modern bytecode interpreters, such as CPython: they use computed gotos instead of switch statements and also implement the "Threaded code" technique to improve utilization of the CPU's branch predictor. (see this comment for detailed explanation: https://github.com/python/cpython/blob/master/Python/ceval.c#L585) Another common use case involving hot switch statements are various lexers and parsers (either hand-coded or generated by tools such as ragel and re2c). For example, a well-known web server Nginx uses several huge hand-coded switch statements to parse HTTP requests ( http://lxr.nginx.org/source/src/http/ngx_http_parse.c). I found an isolated benchmark for this parser: https://natsys-lab.blogspot. ru/2014/11/the-fast-finite-state-machine-for-http.html (code: https://github.com/natsys/blog/tree/master/http_benchmark). I hope this can be helpful for performance analysis. On Fri, Oct 6, 2017 at 4:46 PM, Wilco Dijkstra <wilco.dijks...@arm.com> wrote: > Martin Liska wrote: > > > There are some numbers for cc1plus: > > > > $ bloaty ./objdir2/gcc/cc1plus -- ./objdir/gcc/cc1plus > > VM SIZE FILE SIZE > > +3.8% +1.11Mi TOTAL +1.03Mi +0.5% > > > insn-attrtab.o: > > VM SIZE FILE SIZE > > +214% +682Ki .rodata +682Ki +214% > > -50.1% -63.3Ki .text -63.3Ki -50.1% > > So is that a 3.8% codesize increase or decrease? If an increase, > I can't see how replacing 63KB of instructions with 682KB of data > is a good tradeoff... There should be an accurate calculation > of the density, taking the switch table width into account (really small > tables can use 1-byte offsets, large tables are typically forced to > use 4-byte offsets). This may need new target callbacks - I changed > PARAM_CASE_VALUES_THRESHOLD on AArch64 to get smaller > code and better performance since the current density calculations > are hardcoded and quite wrong for big tables... > > Also what is the codesize difference on SPEC2006/2017? I don't see > any mention of performance impact either... > > Wilco -- Regards, Mikhail Maltsev
Re: [PATCH 1/5][GIMPLE FE] PR testsuite/80580. Handle missing labels in goto statements
Hi. Sorry for a long delay. On 02.05.2017 17:16, Richard Biener wrote: > Certainly an improvement. I suppose we can do better error recovery > for cases like > > if (1) >goto > else >goto bar; > > but I guess this is better than nothing. I think it's worth spending a bit more time to get this right. > > And yes, we use c_parser_error -- input_location should be ok but here > we just peek which may upset the parser. Maybe it works better > when consuming the token before issueing the error? Thus > > Index: gcc/c/gimple-parser.c > === > --- gcc/c/gimple-parser.c (revision 247498) > +++ gcc/c/gimple-parser.c (working copy) > @@ -1315,8 +1315,8 @@ c_parser_gimple_if_stmt (c_parser *parse >loc = c_parser_peek_token (parser)->location; >c_parser_consume_token (parser); >label = c_parser_peek_token (parser)->value; > - t_label = lookup_label_for_goto (loc, label); >c_parser_consume_token (parser); > + t_label = lookup_label_for_goto (loc, label); >if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) > return; > } > I was more focused on cases with missing labels (i.e. 'label' is NULL), rather than cases with syntactically correct if-statements referencing undefined labels. And, frankly speaking, I'm not sure that swapping 'c_parser_consume_token' with 'lookup_label_for_goto' will help, because 'lookup_label_for_goto' already gets a 'loc' parameter. BTW, unfortunately GIMPLE FE does not handle undefined labels properly. I.e., this test case __GIMPLE() void foo() { bb_0: if (0) goto bb_0; else goto bb_1; } causes an ICE somewhere in build_gimple_cfg/cleanup_dead_labels. But this is a separate issue, of course. I attached a slightly modified patch, which incorporates your changes and also uses if (! c_parser_next_token_is (parser, CPP_NAME)) ... instead of label = c_parser_peek_token (parser)->value; ... if (!label) ... for better readability. This version correctly handles missing labels and semicolons in both branches of the 'if' statement. The only major problem, which I want to fix is error recovery in the following example: __GIMPLE() void foo() { if (1) goto lbl; else goto ; /* { dg-error "expected label" } */ } __GIMPLE() void bar() { if (1) goto lbl; else goto } /* { dg-error "expected label" } */ In this case GCC correctly diagnoses both errors. But if I swap these two functions so that 'bar' comes before 'foo', the error in 'foo' is not diagnosed. I did not dive into details, but my speculation is that the parser enters some strange state and skips 'foo' entirely (-fdump-tree-gimple contains only 'bar'). If I add another function after 'foo', it is handled correctly. Any ideas, why this could happen and how to improve error recovery in this case? -- Regards, Mikhail Maltsev diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index ed9e7c5..44ca738 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -1336,9 +1336,14 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq) { loc = c_parser_peek_token (parser)->location; c_parser_consume_token (parser); + if (! c_parser_next_token_is (parser, CPP_NAME)) + { + c_parser_error (parser, "expected label"); + return; + } label = c_parser_peek_token (parser)->value; - t_label = lookup_label_for_goto (loc, label); c_parser_consume_token (parser); + t_label = lookup_label_for_goto (loc, label); if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) return; } @@ -1360,9 +1365,14 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq) { loc = c_parser_peek_token (parser)->location; c_parser_consume_token (parser); + if (! c_parser_next_token_is (parser, CPP_NAME)) + { + c_parser_error (parser, "expected label"); + return; + } label = c_parser_peek_token (parser)->value; - f_label = lookup_label_for_goto (loc, label); c_parser_consume_token (parser); + f_label = lookup_label_for_goto (loc, label); if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) return; } diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-7.c b/gcc/testsuite/gcc.dg/gimplefe-error-7.c new file mode 100644 index 000..7d5ff37 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-7.c @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void fn1() +{ + if (1) +goto + else /* { dg-error "expected label" } */ +goto lbl; +} + +__GIMPLE() void fn2() +{ + if (1) +goto lbl; + else +goto ; /* { dg-error "expected label" } */ +} + +__GIMPLE() void fn3() +{ + if (1) +goto lbl; + else +goto +} /* { dg-error "expected label" } */ +
[PATCH 5/5][GIMPLE FE] PR testsuite/80580: Handle invalid SSA names
When parsing SSA names, we should check that parent names are scalars. In fact, this patch just uses the condition of a 'gcc_assert' in 'make_ssa_name_fn'. -- Regards, Mikhail Maltsev gcc/testsuite/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gcc.dg/gimplefe-error-11.c: New test. gcc/c/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gimple-parser.c (c_parser_parse_ssa_name): Validate SSA name base. From bae6cf05131c284fc8ae9a02f2ba99d447d04fd2 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Fri, 24 Feb 2017 20:54:40 +0300 Subject: [PATCH 5/5] GIMPLEFE: Handle invalid SSA names --- gcc/c/gimple-parser.c| 8 gcc/testsuite/gcc.dg/gimplefe-error-11.c | 9 + 2 files changed, 17 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-11.c diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index f3af840..ac8e7a7 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -692,6 +692,14 @@ c_parser_parse_ssa_name (c_parser *parser, c_parser_error (parser, "base variable or SSA name undeclared"); return error_mark_node; } + if (!(VAR_P (parent) + || TREE_CODE (parent) == PARM_DECL + || TREE_CODE (parent) == RESULT_DECL + || (TYPE_P (parent) && is_gimple_reg_type (parent + { + error ("invalid SSA name %qE", parent); + return error_mark_node; + } if (VECTOR_TYPE_P (TREE_TYPE (parent)) || TREE_CODE (TREE_TYPE (parent)) == COMPLEX_TYPE) DECL_GIMPLE_REG_P (parent) = 1; diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-11.c b/gcc/testsuite/gcc.dg/gimplefe-error-11.c new file mode 100644 index 000..c73b85c --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-11.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void a(int); + +__GIMPLE() void b() +{ + a_2 = 0; /* { dg-error "invalid" } */ +} -- 2.1.4
[PATCH 4/5][GIMPLE FE] PR testsuite/80580. Handle invalid __MEM
This patch deals with invalid __MEM construct. Before we start building an expression for __MEM, we must check that parsing succeeded and that the __MEM operand is a pointer. -- Regards, Mikhail Maltsev gcc/c/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gimple-parser.c (c_parser_gimple_postfix_expression): Handle invalid __MEM. gcc/testsuite/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gcc.dg/gimplefe-error-9.c: New test. From fc2fe1f2f74ce399f9617fb526668bf1d57b0162 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Fri, 24 Feb 2017 20:45:45 +0300 Subject: [PATCH 4/5] GIMPLEFE: handle invalid __MEM --- gcc/c/gimple-parser.c | 10 ++ gcc/testsuite/gcc.dg/gimplefe-error-9.c | 7 +++ 2 files changed, 17 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-9.c diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 5249e8a..f3af840 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -804,6 +804,16 @@ c_parser_gimple_postfix_expression (c_parser *parser) } } ptr = c_parser_gimple_unary_expression (parser); + if (ptr.value == error_mark_node + || ! POINTER_TYPE_P (TREE_TYPE (ptr.value))) + { + if (ptr.value != error_mark_node) + error_at (ptr.get_start (), + "invalid type of %<__MEM%> operand"); + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, +"expected %<)%>"); + return expr; + } if (! alias_type) alias_type = TREE_TYPE (ptr.value); /* Optional constant offset. */ diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-9.c b/gcc/testsuite/gcc.dg/gimplefe-error-9.c new file mode 100644 index 000..2bdb398 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-9.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void a() +{ + __MEM() = 0; /* { dg-error "expected .<." } */ +} -- 2.1.4
[PATCH 3/5][GIMPLE FE] PR testsuite/80580. Handle invalid unary "*" operand type
This is essentially the same problem as in patch 2, but with unary '*'. We should verify that its argument is a pointer. -- Regards, Mikhail Maltsev gcc/c/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gimple-parser.c (c_parser_gimple_unary_expression): Check argument type of unary '*'. gcc/testsuite/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gcc.dg/gimplefe-error-8.c: New test. From 3e0452e939e42af06365d7d49157c227f53a7522 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Fri, 24 Feb 2017 18:29:57 +0300 Subject: [PATCH 3/5] GIMPLEFE: handle invalid unary "*" operand type --- gcc/c/gimple-parser.c | 5 + gcc/testsuite/gcc.dg/gimplefe-error-8.c | 7 +++ 2 files changed, 12 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-8.c diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 2e11567..5249e8a 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -567,6 +567,11 @@ c_parser_gimple_unary_expression (c_parser *parser) op = c_parser_gimple_postfix_expression (parser); if (op.value == error_mark_node) return ret; + if (! POINTER_TYPE_P (TREE_TYPE (op.value))) + { + error_at (op_loc, "invalid type argument of unary %<*%>"); + return ret; + } finish = op.get_finish (); location_t combined_loc = make_location (op_loc, op_loc, finish); ret.value = build_simple_mem_ref_loc (combined_loc, op.value); diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-8.c b/gcc/testsuite/gcc.dg/gimplefe-error-8.c new file mode 100644 index 000..faf699d --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-8.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void a() +{ + *0 = 1; /* { dg-error "invalid type" } */ +} -- 2.1.4
[PATCH 2/5][GIMPLE FE] PR testsuite/80580: handle invalid types of "->" operands
This bug happens when the LHS of operator '->' is either missing, i.e.: (->a) = 0; or it is not a pointer: int b; b_2->c = 0; LHS should be validated. -- Regards, Mikhail Maltsev gcc/testsuite/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gcc.dg/gimplefe-error-6.c: New test. * gcc.dg/gimplefe-error-7.c: New test. gcc/c/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gimple-parser.c (c_parser_gimple_postfix_expression_after_primary): Check LHS of operator '->' From 8fc6470a428f312bad1dd32d526b51d44d34e16e Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Fri, 24 Feb 2017 18:26:03 +0300 Subject: [PATCH 2/5] GIMPLEFE: handle invalid types of "->" operands --- gcc/c/gimple-parser.c | 9 + gcc/testsuite/gcc.dg/gimplefe-error-6.c | 8 gcc/testsuite/gcc.dg/gimplefe-error-7.c | 7 +++ 3 files changed, 24 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-6.c create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-7.c diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index a99b502..2e11567 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -1052,6 +1052,15 @@ c_parser_gimple_postfix_expression_after_primary (c_parser *parser, start = expr.get_start (); finish = c_parser_peek_token (parser)->get_finish (); c_parser_consume_token (parser); + if (!TREE_TYPE (expr.value) + || !POINTER_TYPE_P (TREE_TYPE (expr.value))) + { + error_at (op_loc, "invalid dereference"); + expr.set_error (); + expr.original_code = ERROR_MARK; + expr.original_type = NULL; + return expr; + } expr.value = build_component_ref (op_loc, build_simple_mem_ref_loc (op_loc, expr.value), diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-6.c b/gcc/testsuite/gcc.dg/gimplefe-error-6.c new file mode 100644 index 000..83ea19e --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-6.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void a() +{ + int b; + b_2->c = 0; /* { dg-error "dereference" } */ +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-7.c b/gcc/testsuite/gcc.dg/gimplefe-error-7.c new file mode 100644 index 000..ac5c1ad --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-7.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void b() +{ + (->a) = 0; /* { dg-error "expected|dereference" } */ +} -- 2.1.4
[PATCH 1/5][GIMPLE FE] PR testsuite/80580. Handle missing labels in goto statements
The first problem happens because we don't check for missing labels when parsing 'goto' statements. I.e.: __GIMPLE() void fn1() { if (1) goto } The fix is pretty obvious: just add a check. My question is: which functions should I use to produce diagnostics? The surrounding code uses 'c_parser_error', but this function does not handle locations very well (in fact, it uses input_location). -- Regards, Mikhail Maltsev gcc/testsuite/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gcc.dg/gimplefe-error-4.c: New test. * gcc.dg/gimplefe-error-5.c: New test. gcc/c/ChangeLog: 2017-05-01 Mikhail Maltsev <malts...@gmail.com> * gimple-parser.c (c_parser_gimple_if_stmt): Check for empty labels. From 07453ba1bf0b1290cef54dcb028cb477b17966df Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Fri, 24 Feb 2017 13:09:10 +0300 Subject: [PATCH 1/5] GIMPLEFE: handle missing labels in goto statements --- gcc/c/gimple-parser.c | 10 ++ gcc/testsuite/gcc.dg/gimplefe-error-4.c | 7 +++ gcc/testsuite/gcc.dg/gimplefe-error-5.c | 9 + 3 files changed, 26 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-4.c create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-5.c diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c index 0d6384b..a99b502 100644 --- a/gcc/c/gimple-parser.c +++ b/gcc/c/gimple-parser.c @@ -1315,6 +1315,11 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq) loc = c_parser_peek_token (parser)->location; c_parser_consume_token (parser); label = c_parser_peek_token (parser)->value; + if (! label) + { + c_parser_error (parser, "expected label"); + return; + } t_label = lookup_label_for_goto (loc, label); c_parser_consume_token (parser); if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) @@ -1339,6 +1344,11 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq) loc = c_parser_peek_token (parser)->location; c_parser_consume_token (parser); label = c_parser_peek_token (parser)->value; + if (! label) + { + c_parser_error (parser, "expected label"); + return; + } f_label = lookup_label_for_goto (loc, label); c_parser_consume_token (parser); if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-4.c b/gcc/testsuite/gcc.dg/gimplefe-error-4.c new file mode 100644 index 000..c61539c --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-4.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void fn1() { + if (1) +goto +} /* { dg-error "expected label" } */ diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-5.c b/gcc/testsuite/gcc.dg/gimplefe-error-5.c new file mode 100644 index 000..7398861 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-error-5.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +__GIMPLE() void fn1() { + if (1) +goto lbl; + else +goto +} /* { dg-error "expected label" } */ -- 2.1.4
[PATCH 0/5][GIMPLE FE] PR testsuite/80580. Fix some ICEs on invalid code
Hi, all. As I have already mentioned in the bug report (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80580), I performed some fuzz testing of the GIMPLE front end. I used a technique proposed by John Regehr in his blog post http://blog.regehr.org/archives/1284 for testing C++ compilers. In short, this technique works as follows: 1. take a valid input file as a starting point (I used the GIMPLE code from the GCC test suite) 2. try to remove several tokens from the current input file in such way that the file remains valid (using CReduce) 3. repeat step 2 while possible +record all ICEs found during this process. As a result I found 46 GIMPLE source files that cause ICEs and produce distinct backtraces (see the attachment in Bugzilla). This series of patches fixes some of these ICEs. I have bootstrapped and regtested the unified patch on x86_64-pc-linux-gnu with no regressions (although, I see some noise in the tree-prof tests). The patches are intended for GCC 8. -- Regards, Mikhail Maltsev
Re: [PATCH 2/2][GCC] Add one more pattern to RTL if-conversion
On 05/23/2016 05:15 PM, Kyrill Tkachov wrote: > > expand_simple_binop may fail. I think you should add a check that diff_rtx is > non-NULL > and bail out early if it is. > Fixed. -- Regards, Mikhail Maltsev diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index a9c146b..e1473eb 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -1260,6 +1260,7 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) { ST_ADD_FLAG, ST_SHIFT_FLAG, +ST_SHIFT_ADD_FLAG, ST_IOR_FLAG }; @@ -1384,6 +1385,12 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) normalize = -1; reversep = true; } + else if (exact_log2 (abs_hwi (diff)) >= 0 + && (STORE_FLAG_VALUE == 1 || if_info->branch_cost >= 2)) + { + strategy = ST_SHIFT_ADD_FLAG; + normalize = 1; + } else return FALSE; @@ -1453,6 +1460,24 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) gen_int_mode (ifalse, mode), if_info->x, 0, OPTAB_WIDEN); break; + case ST_SHIFT_ADD_FLAG: + { + /* if (test) x = 5; else x = 1; + => x = (test != 0) << 2 + 1; */ + HOST_WIDE_INT diff_log = exact_log2 (abs_hwi (diff)); + rtx diff_rtx + = expand_simple_binop (mode, ASHIFT, target, GEN_INT (diff_log), + if_info->x, 0, OPTAB_WIDEN); + if (!diff_rtx) + { + end_sequence (); + return false; + } + target = expand_simple_binop (mode, (diff < 0) ? MINUS : PLUS, + gen_int_mode (ifalse, mode), diff_rtx, + if_info->x, 0, OPTAB_WIDEN); + break; + } } if (! target) diff --git a/gcc/testsuite/gcc.dg/ifcvt-6.c b/gcc/testsuite/gcc.dg/ifcvt-6.c new file mode 100644 index 000..c2cfb17 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ifcvt-6.c @@ -0,0 +1,11 @@ +/* { dg-do compile { target x86_64-*-* } } */ +/* { dg-options "-fdump-rtl-ce1 -O2" } */ + +int +test1 (int a) +{ + return a % 2 != 0 ? 7 : 3; +} + +/* { dg-final { scan-rtl-dump "3 true changes made" "ce1" } } */ +/* { dg-final { scan-assembler-not "sbbl" } } */
[PATCH 2/2][GCC] Add one more pattern to RTL if-conversion
This patch adds a new if-conversion pattern for the following case: if (test) x = A; else x = B; A and B are constants, abs(A - B) == 2^N, A != 0, B != 0 Bootstrapped and regtested on x86_64-linux. OK for trunk? -- Regards, Mikhail Maltsev gcc/testsuite/ChangeLog: 2016-05-23 Mikhail Maltsev <malts...@gmail.com> * gcc.dg/ifcvt-6.c: New test. gcc/ChangeLog: 2016-05-23 Mikhail Maltsev <malts...@gmail.com> * ifcvt.c (noce_try_store_flag_constants): Add new pattern. From 32ef17083d1ca6222e4befb1e1d8bae42d71db3b Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <maltsevm@gmail.com> Date: Thu, 12 May 2016 15:23:03 +0300 Subject: [PATCH 2/2] Add ifcvt pattern diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index a9c146b..f06b05d 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -1260,6 +1260,7 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) { ST_ADD_FLAG, ST_SHIFT_FLAG, +ST_SHIFT_ADD_FLAG, ST_IOR_FLAG }; @@ -1384,6 +1385,12 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) normalize = -1; reversep = true; } + else if (exact_log2 (abs_hwi (diff)) >= 0 + && (STORE_FLAG_VALUE == 1 || if_info->branch_cost >= 2)) + { + strategy = ST_SHIFT_ADD_FLAG; + normalize = 1; + } else return FALSE; @@ -1453,6 +1460,19 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) gen_int_mode (ifalse, mode), if_info->x, 0, OPTAB_WIDEN); break; + case ST_SHIFT_ADD_FLAG: + { + /* if (test) x = 5; else x = 1; + => x = (test != 0) << 2 + 1; */ + HOST_WIDE_INT diff_log = exact_log2 (abs_hwi (diff)); + rtx diff_rtx + = expand_simple_binop (mode, ASHIFT, target, GEN_INT (diff_log), + if_info->x, 0, OPTAB_WIDEN); + target = expand_simple_binop (mode, (diff < 0) ? MINUS : PLUS, + gen_int_mode (ifalse, mode), diff_rtx, + if_info->x, 0, OPTAB_WIDEN); + break; + } } if (! target) diff --git a/gcc/testsuite/gcc.dg/ifcvt-6.c b/gcc/testsuite/gcc.dg/ifcvt-6.c new file mode 100644 index 000..c2cfb17 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ifcvt-6.c @@ -0,0 +1,11 @@ +/* { dg-do compile { target x86_64-*-* } } */ +/* { dg-options "-fdump-rtl-ce1 -O2" } */ + +int +test1 (int a) +{ + return a % 2 != 0 ? 7 : 3; +} + +/* { dg-final { scan-rtl-dump "3 true changes made" "ce1" } } */ +/* { dg-final { scan-assembler-not "sbbl" } } */ -- 2.1.4
[PATCH 1/2][GCC] Refactor noce_try_store_flag_constants
This patch refactors 'noce_try_store_flag_constants' a bit to make it easier to reason about. The function contains two series of conditions, and each branch of the second series corresponds to one or two branches of the first series. The patch introduces a new enumeration strategy_t instead and uses it to select the correct branch. Also, ISTM that the last 'else' branch is unreachable. Bootstrapped and regtested on x86_64-linux. OK for trunk? -- Regards, Mikhail Maltsev gcc/ChangeLog: 2016-05-23 Mikhail Maltsev <malts...@gmail.com> * ifcvt.c (noce_try_store_flag_constants): Refactor. From 847ba5ac9194273c8b51839cfda86bbc399847f4 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <maltsevm@gmail.com> Date: Tue, 10 May 2016 22:53:26 +0300 Subject: [PATCH 1/2] Refactor noce_try_store_flag_constants diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 4949965..a9c146b 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -1256,13 +1256,14 @@ noce_try_inverse_constants (struct noce_if_info *if_info) static int noce_try_store_flag_constants (struct noce_if_info *if_info) { - rtx target; - rtx_insn *seq; - bool reversep; - HOST_WIDE_INT itrue, ifalse, diff, tmp; - int normalize; - bool can_reverse; - machine_mode mode = GET_MODE (if_info->x);; + enum strategy_t + { +ST_ADD_FLAG, +ST_SHIFT_FLAG, +ST_IOR_FLAG + }; + + machine_mode mode = GET_MODE (if_info->x); rtx common = NULL_RTX; rtx a = if_info->a; @@ -1292,11 +1293,11 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) if (CONST_INT_P (a) && CONST_INT_P (b)) { - ifalse = INTVAL (a); - itrue = INTVAL (b); + HOST_WIDE_INT ifalse = INTVAL (a); + HOST_WIDE_INT itrue = INTVAL (b); bool subtract_flag_p = false; - diff = (unsigned HOST_WIDE_INT) itrue - ifalse; + HOST_WIDE_INT diff = (unsigned HOST_WIDE_INT) itrue - ifalse; /* Make sure we can represent the difference between the two values. */ if ((diff > 0) != ((ifalse < 0) != (itrue < 0) ? ifalse < 0 : ifalse < itrue)) @@ -1304,12 +1305,15 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) diff = trunc_int_for_mode (diff, mode); - can_reverse = (reversed_comparison_code (if_info->cond, if_info->jump) - != UNKNOWN); + bool can_reverse + = (reversed_comparison_code (if_info->cond, if_info->jump) != UNKNOWN); - reversep = false; + bool reversep = false; + int normalize; + strategy_t strategy; if (diff == STORE_FLAG_VALUE || diff == -STORE_FLAG_VALUE) { + strategy = ST_ADD_FLAG; normalize = 0; /* We could collapse these cases but it is easier to follow the diff/STORE_FLAG_VALUE combinations when they are listed @@ -1355,20 +1359,28 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) else if (ifalse == 0 && exact_log2 (itrue) >= 0 && (STORE_FLAG_VALUE == 1 || if_info->branch_cost >= 2)) - normalize = 1; + { + strategy = ST_SHIFT_FLAG; + normalize = 1; + } else if (itrue == 0 && exact_log2 (ifalse) >= 0 && can_reverse && (STORE_FLAG_VALUE == 1 || if_info->branch_cost >= 2)) { + strategy = ST_SHIFT_FLAG; normalize = 1; reversep = true; } else if (itrue == -1 && (STORE_FLAG_VALUE == -1 || if_info->branch_cost >= 2)) - normalize = -1; + { + strategy = ST_IOR_FLAG; + normalize = -1; + } else if (ifalse == -1 && can_reverse && (STORE_FLAG_VALUE == -1 || if_info->branch_cost >= 2)) { + strategy = ST_IOR_FLAG; normalize = -1; reversep = true; } @@ -1391,59 +1403,56 @@ noce_try_store_flag_constants (struct noce_if_info *if_info) noce_emit_move_insn (common, if_info->x); } - target = noce_emit_store_flag (if_info, if_info->x, reversep, normalize); + rtx target + = noce_emit_store_flag (if_info, if_info->x, reversep, normalize); if (! target) { end_sequence (); return FALSE; } - /* if (test) x = 3; else x = 4; - => x = 3 + (test == 0); */ - if (diff == STORE_FLAG_VALUE || diff == -STORE_FLAG_VALUE) + if (common && strategy != ST_ADD_FLAG) + { + /* Not beneficial when the original A and B are PLUS expressions. */ + end_sequence (); + return false; + } + + switch (strategy) { + case ST_ADD_FLAG: + /* if (test) x = 3; else x = 4; + => x = 3 + (test == 0); */ + /* Add the common part now. This may allow combine to merge this with the store flag operation earlier into some sort of conditional increment/decrement if the target allows it. */ if (common) - target = expand_simple_binop (mode, PLUS, - target, common, - target, 0, OPTAB_WIDEN); + target = expand_simple_binop (mode, PLUS, target, co
[PATCH 0/2][GCC] Add one more pattern to RTL if-conversion
Hi all! Currently GCC generates rather bad code for the following test case: int test(int a) { return a % 2 != 0 ? 4 : 2; } The code looks like this: test: andl$1, %edi cmpl$1, %edi sbbl%eax, %eax andl$-2, %eax addl$4, %eax ret Clang seems to generate optimal code: test: andl$1, %edi leal2(%rdi,%rdi), %eax retq After applying this series of 2 patches GCC generates: test: movl%edi, %eax andl$1, %eax leal2(%rax,%rax), %eax ret -- Regards, Mikhail Maltsev
Re: [PATCH, GCC] PR middle-end/55299, fold bitnot through ASR and rotates
On 05/17/2016 06:09 PM, Richard Biener wrote: > > The patch is ok. > Committed as r236344. -- Regards, Mikhail Maltsev
Re: [PATCH, GCC] PR middle-end/55299, fold bitnot through ASR and rotates
On 05/17/2016 04:39 PM, Richard Biener wrote: > > Are you sure narrowing conversions are valid for rotates? > > (char)short_var < byte. > Yes, but the transformation leaves conversions as-is. Only bit_not is removed. -- Regards, Mikhail Maltsev
Re: [PATCH, GCC] PR middle-end/55299, fold bitnot through ASR and rotates
On 05/11/2016 10:52 AM, Marc Glisse wrote: > +/* ~((~X) >> Y) -> X >> Y (for arithmetic shift). */ > +(simplify > + (bit_not (convert? (rshift (bit_not @0) @1))) > + (if (!TYPE_UNSIGNED (TREE_TYPE (@0)) > + && TYPE_PRECISION (type) <= TYPE_PRECISION (TREE_TYPE (@0))) > + (convert (rshift @0 @1 > > Is there a particular reason to split the converting / non-converting > cases? For rotate, you managed to merge them nicely. Fixed (i.e., merged two shift simplifications into one). > > + > +(simplify > + (bit_not (convert? (rshift (convert@0 (bit_not @1)) @2))) > + (if (!TYPE_UNSIGNED (TREE_TYPE (@0)) > + && TYPE_PRECISION (TREE_TYPE (@0)) <= TYPE_PRECISION (TREE_TYPE (@1)) > + && TYPE_PRECISION (type) <= TYPE_PRECISION (TREE_TYPE (@0))) > + (with > +{ tree shift_type = TREE_TYPE (@0); } > + (convert (rshift:shift_type (convert @1) @2) > + > +/* Same as above, but for rotates. */ > +(for rotate (lrotate rrotate) > + (simplify > + (bit_not (convert1?@0 (rotate (convert2?@1 (bit_not @2)) @3))) > + (if (TYPE_PRECISION (TREE_TYPE (@1)) <= TYPE_PRECISION (TREE_TYPE (@2)) > +&& TYPE_PRECISION (TREE_TYPE (@0)) <= TYPE_PRECISION (TREE_TYPE > (@1))) > +(with > + { tree operand_type = TREE_TYPE (@2); } > + (convert (rotate:operand_type @2 @3)) > > Is that really safe when the conversion from @2 to @1 is narrowing? I > would expect something closer to > (convert (rotate (convert:type_of_1 @2) @3)) > so the rotation is done in a type of the same precision as the original. > > Or > (convert (rotate:type_of_1 (convert @2) @3)) > if you prefer specifying the type there (I don't), and note that you > need the 'convert' inside or specifying the type on rotate doesn't work. Fixed. > > I have a slight preference for element_precision over TYPE_PRECISION (which > for > vectors is the number of elements), but I don't think it can currently cause > issues for these particular transformations. Fixed. > > I don't know if we might want some :c / single_use restrictions, maybe on the > outer convert and the rshift/rotate. > I don't think :c can be used here. As for :s, I added it, as you suggested. Also, I tried to add some more test cases for rotate with conversions, but unfortunately GCC does not recognize rotate pattern, when narrowing conversions are present. -- Regards, Mikhail Maltsev diff --git a/gcc/match.pd b/gcc/match.pd index 55dd23c..bb4bba6 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -1453,6 +1453,27 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (with { tree mask = int_const_binop (shift, fold_convert (type, @2), @1); } (bit_op (shift (convert @0) @1) { mask; })) +/* ~(~X >> Y) -> X >> Y (for arithmetic shift). */ +(simplify + (bit_not (convert1?:s (rshift:s (convert2?@0 (bit_not @1)) @2))) + (if (!TYPE_UNSIGNED (TREE_TYPE (@0)) + && element_precision (TREE_TYPE (@0)) + <= element_precision (TREE_TYPE (@1)) + && element_precision (type) <= element_precision (TREE_TYPE (@0))) + (with +{ tree shift_type = TREE_TYPE (@0); } + (convert (rshift (convert:shift_type @1) @2) + +/* ~(~X >>r Y) -> X >>r Y + ~(~X < X <> (INT_BITS - (y))) +#define ROR(x, y) ((x) >> (y) | (x) << (INT_BITS - (y))) + +unsigned +rol (unsigned a, unsigned b) +{ + return ~ROL (~a, b); +} + +unsigned int +ror (unsigned a, unsigned b) +{ + return ~ROR (~a, b); +} + +int +rol_conv1 (int a, unsigned b) +{ + return ~(int)ROL((unsigned)~a, b); +} + +int +rol_conv2 (int a, unsigned b) +{ + return ~ROL((unsigned)~a, b); +} + +int +rol_conv3 (unsigned a, unsigned b) +{ + return ~(int)ROL(~a, b); +} + +#define LONG_BITS (sizeof (long) * __CHAR_BIT__) +#define ROLL(x, y) ((x) << (y) | (x) >> (LONG_BITS - (y))) +#define RORL(x, y) ((x) >> (y) | (x) << (LONG_BITS - (y))) + +unsigned long +roll (unsigned long a, unsigned long b) +{ + return ~ROLL (~a, b); +} + +unsigned long +rorl (unsigned long a, unsigned long b) +{ + return ~RORL (~a, b); +} + +/* { dg-final { scan-tree-dump-not "~" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-1.c b/gcc/testsuite/gcc.dg/fold-notshift-1.c new file mode 100644 index 000..2de236f --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notshift-1.c @@ -0,0 +1,77 @@ +/* PR tree-optimization/54579 + PR middle-end/55299 */ + +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-cddce1" } */ + +int +asr1 (int a, int b) +{ + return ~((~a) >> b); +} + +long +asr1l (long a, long b) +{ + return ~((~a) >> b); +} + +int +asr_conv (unsigned a, unsigned b) +{ + return ~((int)~a >> b); +} + +unsigned +asr_conv2 (unsigned a, unsigne
Re: [C PATCH] PR43651: add warning for duplicate qualifier
On 05/10/2016 10:51 PM, Joseph Myers wrote: > On Sat, 9 Apr 2016, Mikhail Maltsev wrote: > >> gcc/c/ChangeLog: >> >> 2016-04-08 Mikhail Maltsev <malts...@gmail.com> >> >> PR c/43651 >> * c-decl.c (declspecs_add_qual): Warn when -Wduplicate-decl-specifier >> is enabled. >> * c-errors.c (pedwarn_c90): Return true if warned. >> * c-tree.h (pedwarn_c90): Change return type to bool. >> (enum c_declspec_word): Add new enumerator cdw_atomic. >> >> gcc/ChangeLog: >> >> 2016-04-08 Mikhail Maltsev <malts...@gmail.com> >> >> PR c/43651 >> * doc/invoke.texi (Wduplicate-decl-specifier): Document new option. >> >> gcc/testsuite/ChangeLog: >> >> 2016-04-08 Mikhail Maltsev <malts...@gmail.com> >> >> PR c/43651 >> * gcc.dg/Wduplicate-decl-specifier-c11.c: New test. >> * gcc.dg/Wduplicate-decl-specifier.c: Likewise. >> >> >> gcc/c-family/ChangeLog: >> >> 2016-04-08 Mikhail Maltsev <malts...@gmail.com> >> >> PR c/43651 >> * c.opt (Wduplicate-decl-specifier): New option. > > OK. > Committed as r236142. -- Regards, Mikhail Maltsev
Re: [PATCH, GCC] PR middle-end/55299, fold bitnot through ASR and rotates
On 05/08/2016 10:57 PM, Marc Glisse wrote: > On Sun, 8 May 2016, Mikhail Maltsev wrote: > >> Hi! >> >> I decided to revive this patch: >> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00999.html. >> I addressed review comments about sign conversions. Bootstrapped and >> regtested >> on x86_64-linux-gnu {,-m32}. OK for trunk? > > Hello, > > are you sure that your transformations are safe for any kind of conversion? > Oops, indeed, only narrowing conversions should be allowed. I updated the patch and added some more test cases. -- Regards, Mikhail Maltsev diff --git a/gcc/match.pd b/gcc/match.pd index 55dd23c..b545cc7 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -1453,6 +1453,31 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (with { tree mask = int_const_binop (shift, fold_convert (type, @2), @1); } (bit_op (shift (convert @0) @1) { mask; })) +/* ~((~X) >> Y) -> X >> Y (for arithmetic shift). */ +(simplify + (bit_not (convert? (rshift (bit_not @0) @1))) + (if (!TYPE_UNSIGNED (TREE_TYPE (@0)) + && TYPE_PRECISION (type) <= TYPE_PRECISION (TREE_TYPE (@0))) + (convert (rshift @0 @1 + +(simplify + (bit_not (convert? (rshift (convert@0 (bit_not @1)) @2))) + (if (!TYPE_UNSIGNED (TREE_TYPE (@0)) + && TYPE_PRECISION (TREE_TYPE (@0)) <= TYPE_PRECISION (TREE_TYPE (@1)) + && TYPE_PRECISION (type) <= TYPE_PRECISION (TREE_TYPE (@0))) + (with +{ tree shift_type = TREE_TYPE (@0); } + (convert (rshift:shift_type (convert @1) @2) + +/* Same as above, but for rotates. */ +(for rotate (lrotate rrotate) + (simplify + (bit_not (convert1?@0 (rotate (convert2?@1 (bit_not @2)) @3))) + (if (TYPE_PRECISION (TREE_TYPE (@1)) <= TYPE_PRECISION (TREE_TYPE (@2)) +&& TYPE_PRECISION (TREE_TYPE (@0)) <= TYPE_PRECISION (TREE_TYPE (@1))) +(with + { tree operand_type = TREE_TYPE (@2); } + (convert (rotate:operand_type @2 @3)) /* Simplifications of conversions. */ diff --git a/gcc/testsuite/gcc.dg/fold-notrotate-1.c b/gcc/testsuite/gcc.dg/fold-notrotate-1.c new file mode 100644 index 000..a9b3804 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notrotate-1.c @@ -0,0 +1,54 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-optimized" } */ + +#define INT_BITS (sizeof (int) * __CHAR_BIT__) +#define ROL(x, y) ((x) << (y) | (x) >> (INT_BITS - (y))) +#define ROR(x, y) ((x) >> (y) | (x) << (INT_BITS - (y))) + +unsigned +rol (unsigned a, unsigned b) +{ + return ~ROL (~a, b); +} + +unsigned int +ror (unsigned a, unsigned b) +{ + return ~ROR (~a, b); +} + +int +rol_conv1 (int a, unsigned b) +{ + return ~(int)ROL((unsigned)~a, b); +} + +int +rol_conv2 (int a, unsigned b) +{ + return ~ROL((unsigned)~a, b); +} + +int +rol_conv3 (unsigned a, unsigned b) +{ + return ~(int)ROL(~a, b); +} + +#define LONG_BITS (sizeof (long) * __CHAR_BIT__) +#define ROLL(x, y) ((x) << (y) | (x) >> (LONG_BITS - (y))) +#define RORL(x, y) ((x) >> (y) | (x) << (LONG_BITS - (y))) + +unsigned long +roll (unsigned long a, unsigned long b) +{ + return ~ROLL (~a, b); +} + +unsigned long +rorl (unsigned long a, unsigned long b) +{ + return ~RORL (~a, b); +} + +/* { dg-final { scan-tree-dump-not "~" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-1.c b/gcc/testsuite/gcc.dg/fold-notshift-1.c new file mode 100644 index 000..2de236f --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notshift-1.c @@ -0,0 +1,77 @@ +/* PR tree-optimization/54579 + PR middle-end/55299 */ + +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-cddce1" } */ + +int +asr1 (int a, int b) +{ + return ~((~a) >> b); +} + +long +asr1l (long a, long b) +{ + return ~((~a) >> b); +} + +int +asr_conv (unsigned a, unsigned b) +{ + return ~((int)~a >> b); +} + +unsigned +asr_conv2 (unsigned a, unsigned b) +{ + return ~(unsigned)((int)~a >> b); +} + +unsigned +asr_conv3 (int a, int b) +{ + return ~(unsigned)(~a >> b); +} + +typedef __INT32_TYPE__ int32_t; +typedef __INT64_TYPE__ int64_t; + +int32_t +asr_conv4 (int64_t a, int b) +{ + return ~((int32_t)~a >> b); +} + +int32_t +asr_conv5 (int64_t a, int b) +{ + return ~(int32_t)(~a >> b); +} + +int +asr2 (int a, int b) +{ + return -((-a - 1) >> b) - 1; +} + +int +asr3 (int a, int b) +{ + return a < 0 ? ~((~a) >> b) : a >> b; +} + +int64_t +asr3l (int64_t a, int b) +{ + return a < 0 ? ~((~a) >> b) : a >> b; +} + +int +asr4 (int a, int b) +{ + return a < 0 ? -((-a - 1) >> b) - 1 : a >> b; +} + +/* { dg-final { scan-tree-dump-times ">>" 11 "cddce1" } } */ +/* { dg-final { scan-tree-dump-not "~" "cddce1" } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-2.c b/gcc/testsuite/g
[PATCH, GCC] PR middle-end/55299, fold bitnot through ASR and rotates
Hi! I decided to revive this patch: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00999.html. I addressed review comments about sign conversions. Bootstrapped and regtested on x86_64-linux-gnu {,-m32}. OK for trunk? -- Regards, Mikhail Maltsev gcc/testsuite/ChangeLog: 2016-05-08 Mikhail Maltsev <malts...@gmail.com> PR tree-optimization/54579 PR middle-end/55299 * gcc.dg/fold-notrotate-1.c: New test. * gcc.dg/fold-notshift-1.c: New test. * gcc.dg/fold-notshift-2.c: New test. gcc/ChangeLog: 2016-05-08 Mikhail Maltsev <malts...@gmail.com> PR tree-optimization/54579 PR middle-end/55299 * match.pd (~(~X >> Y), ~(~X >>r Y), ~(~X <diff --git a/gcc/match.pd b/gcc/match.pd index 55dd23c..cc0d03b 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -1453,6 +1453,25 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (with { tree mask = int_const_binop (shift, fold_convert (type, @2), @1); } (bit_op (shift (convert @0) @1) { mask; })) +/* ~((~X) >> Y) -> X >> Y (for arithmetic shift). */ +(simplify + (bit_not (convert? (rshift (bit_not @0) @1))) + (if (!TYPE_UNSIGNED (TREE_TYPE (@0))) + (convert (rshift @0 @1 +(simplify + (bit_not (convert? (rshift (convert@0 (bit_not @1)) @2))) + (if (!TYPE_UNSIGNED (TREE_TYPE (@0))) + (with +{ tree shift_type = TREE_TYPE (@0); } + (convert (rshift:shift_type (convert @1) @2) + +/* Same as above, but for rotates. */ +(for rotate (lrotate rrotate) + (simplify + (bit_not (convert1? (rotate (convert2? (bit_not @0)) @1))) + (with +{ tree operand_type = TREE_TYPE (@0); } + (convert (rotate:operand_type @0 @1) /* Simplifications of conversions. */ diff --git a/gcc/testsuite/gcc.dg/fold-notrotate-1.c b/gcc/testsuite/gcc.dg/fold-notrotate-1.c new file mode 100644 index 000..a9b3804 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notrotate-1.c @@ -0,0 +1,54 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-optimized" } */ + +#define INT_BITS (sizeof (int) * __CHAR_BIT__) +#define ROL(x, y) ((x) << (y) | (x) >> (INT_BITS - (y))) +#define ROR(x, y) ((x) >> (y) | (x) << (INT_BITS - (y))) + +unsigned +rol (unsigned a, unsigned b) +{ + return ~ROL (~a, b); +} + +unsigned int +ror (unsigned a, unsigned b) +{ + return ~ROR (~a, b); +} + +int +rol_conv1 (int a, unsigned b) +{ + return ~(int)ROL((unsigned)~a, b); +} + +int +rol_conv2 (int a, unsigned b) +{ + return ~ROL((unsigned)~a, b); +} + +int +rol_conv3 (unsigned a, unsigned b) +{ + return ~(int)ROL(~a, b); +} + +#define LONG_BITS (sizeof (long) * __CHAR_BIT__) +#define ROLL(x, y) ((x) << (y) | (x) >> (LONG_BITS - (y))) +#define RORL(x, y) ((x) >> (y) | (x) << (LONG_BITS - (y))) + +unsigned long +roll (unsigned long a, unsigned long b) +{ + return ~ROLL (~a, b); +} + +unsigned long +rorl (unsigned long a, unsigned long b) +{ + return ~RORL (~a, b); +} + +/* { dg-final { scan-tree-dump-not "~" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-1.c b/gcc/testsuite/gcc.dg/fold-notshift-1.c new file mode 100644 index 000..674f3c7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notshift-1.c @@ -0,0 +1,62 @@ +/* PR tree-optimization/54579 + PR middle-end/55299 */ + +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-cddce1" } */ + +int +asr1 (int a, int b) +{ + return ~((~a) >> b); +} + +long +asr1l (long a, long b) +{ + return ~((~a) >> b); +} + +int +asr_conv (unsigned a, unsigned b) +{ + return ~((int)~a >> b); +} + +unsigned +asr_conv2 (unsigned a, unsigned b) +{ + return ~(unsigned)((int)~a >> b); +} + +unsigned +asr_conv3 (int a, int b) +{ + return ~(unsigned)(~a >> b); +} + +int +asr2 (int a, int b) +{ + return -((-a - 1) >> b) - 1; +} + +int +asr3 (int a, int b) +{ + return a < 0 ? ~((~a) >> b) : a >> b; +} + +long +asr3l (long a, int b) +{ + return a < 0 ? ~((~a) >> b) : a >> b; +} + +int +asr4 (int a, int b) +{ + return a < 0 ? -((-a - 1) >> b) - 1 : a >> b; +} + +/* { dg-final { scan-tree-dump-times ">>" 9 "cddce1" } } */ +/* { dg-final { scan-tree-dump-not "~" "cddce1" } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-2.c b/gcc/testsuite/gcc.dg/fold-notshift-2.c new file mode 100644 index 000..5287610 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notshift-2.c @@ -0,0 +1,18 @@ +/* PR middle-end/55299 */ + +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-cddce1" } */ + +unsigned int +lsr (unsigned int a, unsigned int b) +{ + return ~((~a) >> b); +} + +int +sl (int a, int b) +{ + return ~((~a) << b); +} + +/* { dg-final { scan-tree-dump-times "~" 4 "cddce1" } } */
[C PATCH PING] PR43651: add warning for duplicate qualifier
On 04/10/2016 11:12 PM, Martin Sebor wrote: > On 04/09/2016 06:28 AM, Mikhail Maltsev wrote: >> On 04/08/2016 08:54 PM, Martin Sebor wrote: >>>> The name for new option "-Wduplicate-decl-specifier" and wording was >>>> chosen to match the same option in Clang. >>> >>> My version of Clang also warns in C++ mode but if I'm reading >>> the patch right, GCC would warn only C mode. I would find it >>> surprising if GCC provided the same option as Clang but didn't >>> make it available in the same languages. Do you have some >>> reason for leaving it out that I'm not thinking of? >> It is an error in C++ mode. Do we want to change this behavior? > > You're right, G++ does give an error. I missed it in my testing. > Unlike C11, C++ requires a diagnostic for duplicated cv-qualifiers > so by issuing a warning Clang is more permissive. My personal > inclination would be to treat this consistently between C and C++ > but whether or not to change it is something Jason would need to > weigh in on. Ping. -- Regards, Mikhail Maltsev
Re: [PATCH] Fix missed DSE opportunity with operator delete.
On 04/20/2016 05:12 PM, Richard Biener wrote: > You have > > +static tree > +handle_free_attribute (tree *node, tree name, tree /*args*/, int /*flags*/, > + bool *no_add_attrs) > +{ > + tree decl = *node; > + if (TREE_CODE (decl) == FUNCTION_DECL > + && type_num_arguments (TREE_TYPE (decl)) != 0 > + && POINTER_TYPE_P (TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (decl) > +DECL_ALLOC_FN_KIND (decl) = ALLOC_FN_FREE; > + else > +{ > + warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes, > + "%qE attribute ignored", name); > + *no_add_attrs = true; > +} > > so one can happily apply the attribute to > > void foo (void *, void *); > > but then > > @@ -2117,6 +2127,13 @@ call_may_clobber_ref_p_1 (gcall *call, ao_ref *ref) > /* Fallthru to general call handling. */; >} > > + if (callee != NULL_TREE > + && (flags_from_decl_or_type (callee) & ECF_FREE) != 0) > +{ > + tree ptr = gimple_call_arg (call, 0); > + return ptr_deref_may_alias_ref_p_1 (ptr, ref); > +} > > will ignore the 2nd argument. I think it's better to ignore the attribute > if type_num_arguments () != 1. Actually, the C++ standard ([basic.stc.dynamic]/2) defines the following 4 deallocation functions implicitly: void operator delete(void*); void operator delete[](void*); void operator delete(void*, std::size_t) noexcept; void operator delete[](void*, std::size_t) noexcept; And the standard library also has: void operator delete(void*, const std::nothrow_t&); void operator delete[](void*, const std::nothrow_t&); void operator delete(void*, std::size_t, const std::nothrow_t&); void operator delete[](void*, std::size_t, const std::nothrow_t&); IIUC, 'delete(void*, std::size_t)' is used by default in C++14 (https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01266.html). How should we handle this? -- Regards, Mikhail Maltsev
Re: An abridged "Writing C" for the gcc web pages
On 04/22/2016 07:21 PM, Bernd Schmidt wrote: > (Apologies if you get this twice, the mailing list didn't like the html > attachment in the first attempt). > > We frequently get malformatted patches, and it's been brought to my attention > that some people don't even make the effort to read the GNU coding standards > before trying to contribute code. TL;DR seems to be the excuse, and while I > find > that attitude inappropriate, we could probably improve the situation by > spelling > out the most basic rules in an abridged document on our webpages. Below is a > draft I came up with. Thoughts? > Probably contrib/clang-format and https://gcc.gnu.org/wiki/FormattingCodeForGCC are also worth mentioning. -- Regards, Mikhail Maltsev
Re: [PATCH] Fix missed DSE opportunity with operator delete.
On 04/18/2016 12:14 PM, Richard Biener wrote: > > Enlarging tree_function_decl is bad. Probably using 3 bits for malloc_flag, operator_new_flag and free_flag is redundant. I packed the state into 2 bits. > > Passes should get at the info via flags_from_decl_or_type () and a new > ECF_FREE. Fixed. -- Regards, Mikhail Maltsev diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c index 357d26f..00e4f84 100644 --- a/gcc/ada/gcc-interface/trans.c +++ b/gcc/ada/gcc-interface/trans.c @@ -400,7 +400,7 @@ gigi (Node_Id gnat_root, ftype, NULL_TREE, is_disabled, false, true, true, false, true, false, NULL, Empty); - DECL_IS_MALLOC (malloc_decl) = 1; + DECL_SET_MALLOC (malloc_decl); /* free is a function declaration tree for a function to free memory. */ free_decl diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c index d568dff..5b12e3d 100644 --- a/gcc/ada/gcc-interface/utils.c +++ b/gcc/ada/gcc-interface/utils.c @@ -6026,7 +6026,7 @@ handle_malloc_attribute (tree *node, tree name, tree ARG_UNUSED (args), { if (TREE_CODE (*node) == FUNCTION_DECL && POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (*node -DECL_IS_MALLOC (*node) = 1; +DECL_SET_MALLOC (*node); else { warning (OPT_Wattributes, "%qs attribute ignored", diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def index 089817a..ddaf3e6 100644 --- a/gcc/builtin-attrs.def +++ b/gcc/builtin-attrs.def @@ -88,6 +88,7 @@ DEF_ATTR_IDENT (ATTR_CONST, "const") DEF_ATTR_IDENT (ATTR_FORMAT, "format") DEF_ATTR_IDENT (ATTR_FORMAT_ARG, "format_arg") DEF_ATTR_IDENT (ATTR_MALLOC, "malloc") +DEF_ATTR_IDENT (ATTR_FREE, "free") DEF_ATTR_IDENT (ATTR_NONNULL, "nonnull") DEF_ATTR_IDENT (ATTR_NORETURN, "noreturn") DEF_ATTR_IDENT (ATTR_NOTHROW, "nothrow") @@ -141,6 +142,10 @@ DEF_ATTR_TREE_LIST (ATTR_MALLOC_NOTHROW_LIST, ATTR_MALLOC, \ ATTR_NULL, ATTR_NOTHROW_LIST) DEF_ATTR_TREE_LIST (ATTR_MALLOC_NOTHROW_LEAF_LIST, ATTR_MALLOC, \ ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) +DEF_ATTR_TREE_LIST (ATTR_FREE_NOTHROW_LIST, ATTR_FREE, \ + ATTR_NULL, ATTR_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_FREE_NOTHROW_LEAF_LIST, ATTR_FREE, \ + ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_SENTINEL_NOTHROW_LIST, ATTR_SENTINEL, \ ATTR_NULL, ATTR_NOTHROW_LIST) DEF_ATTR_TREE_LIST (ATTR_SENTINEL_NOTHROW_LEAF_LIST, ATTR_SENTINEL, \ @@ -269,8 +274,10 @@ DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_RT_LIST, DEF_ATTR_TREE_LIST (ATTR_TMPURE_MALLOC_NOTHROW_LIST, ATTR_TM_TMPURE, ATTR_NULL, ATTR_MALLOC_NOTHROW_LIST) /* Same attributes used for BUILT_IN_FREE except with TM_PURE thrown in. */ -DEF_ATTR_TREE_LIST (ATTR_TMPURE_NOTHROW_LIST, - ATTR_TM_TMPURE, ATTR_NULL, ATTR_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_TMPURE_FREE_NOTHROW_LIST, + ATTR_TM_TMPURE, ATTR_NULL, ATTR_FREE_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_TMPURE_FREE_NOTHROW_LEAF_LIST, + ATTR_TM_TMPURE, ATTR_NULL, ATTR_FREE_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_TMPURE_NOTHROW_LEAF_LIST, ATTR_TM_TMPURE, ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) diff --git a/gcc/builtins.def b/gcc/builtins.def index 2fc7f65..e3d1614 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -781,7 +781,7 @@ DEF_EXT_LIB_BUILTIN(BUILT_IN_FFSLL, "ffsll", BT_FN_INT_LONGLONG, ATTR_CONST_ DEF_EXT_LIB_BUILTIN(BUILT_IN_FORK, "fork", BT_FN_PID, ATTR_NOTHROW_LIST) DEF_GCC_BUILTIN(BUILT_IN_FRAME_ADDRESS, "frame_address", BT_FN_PTR_UINT, ATTR_NULL) /* [trans-mem]: Adjust BUILT_IN_TM_FREE if BUILT_IN_FREE is changed. */ -DEF_LIB_BUILTIN(BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) +DEF_LIB_BUILTIN(BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_FREE_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN(BUILT_IN_FROB_RETURN_ADDR, "frob_return_addr", BT_FN_PTR_PTR, ATTR_NULL) DEF_EXT_LIB_BUILTIN(BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) DEF_C99_BUILTIN(BUILT_IN_IMAXABS, "imaxabs", BT_FN_INTMAX_INTMAX, ATTR_CONST_NOTHROW_LEAF_LIST) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index cae2faf..12d7924 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -355,6 +355,7 @@ static tree handle_tls_model_attribute (tree *, tree, tree, int, static tree handle_no_instrument_function_attribute (tree *, tree, tree, int, bool *); static tree handle_malloc_attribute (tree *, tree, tree, int, bool *); +static tree handle_free_attribute (tree *, tree, tree, int, bool *); static tree handle_returns_twice_attribute (tree *, tree, tree, int, bool *); static tree handle_no_limit_stack_attribute (tree *, tree, tree, int, bool *); @@ -720,6 +721,8 @@ const struct attribute_spec c_common_attr
[PATCH] Fix missed DSE opportunity with operator delete.
Hi, all! Currently GCC can optimize away the following dead store: void test(char *x) { *x = 1; free(x); } but not this one (Clang handles both cases): void test(char *x) { *x = 1; delete x; } The attached patch fixes this by introducing a new __attribute__((free)). I first tried to add new built-ins for each version of operator delete (there are four of them), but it looked a little clumsy, and would require some special handling for warning about taking address of built-in function. Is such approach (i.e. adding a new attribute) OK? Bootstrapped and regtested on x86_64-pc-linux-gnu. -- Regards, Mikhail Maltsev gcc/c/ChangeLog: 2016-04-16 Mikhail Maltsev <malts...@gmail.com> * c-decl.c (merge_decls): Handle free_flag. gcc/ChangeLog: 2016-04-16 Mikhail Maltsev <malts...@gmail.com> * builtin-attrs.def: Add attribute free. * builtins.def (free): Add attribute free. * doc/extend.texi: Document attribute free. * gtm-builtins.def (_ITM_free): Add attribute free. * tree-core.h (struct tree_function_decl): Add free_flag. * tree-ssa-alias.c (ref_maybe_used_by_call_p_1): Handle free_flag. (call_may_clobber_ref_p_1): Likewise. (stmt_kills_ref_p): Likewise. * tree-streamer-in.c (unpack_ts_function_decl_value_fields): Likewise. * tree-streamer-out.c (pack_ts_function_decl_value_fields): Likewise. * tree.h (DECL_IS_FREE): New accessor macro. gcc/testsuite/ChangeLog: 2016-04-16 Mikhail Maltsev <malts...@gmail.com> * g++.dg/opt/op-delete-dse.C: New test. * gcc.dg/attr-free.c: New test. gcc/c-family/ChangeLog: 2016-04-16 Mikhail Maltsev <malts...@gmail.com> * c-common.c (handle_free_attribute): New function. gcc/cp/ChangeLog: 2016-04-16 Mikhail Maltsev <malts...@gmail.com> * decl.c (cxx_init_decl_processing): Set flag_free for operator delete. diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def index 089817a..ddaf3e6 100644 --- a/gcc/builtin-attrs.def +++ b/gcc/builtin-attrs.def @@ -88,6 +88,7 @@ DEF_ATTR_IDENT (ATTR_CONST, "const") DEF_ATTR_IDENT (ATTR_FORMAT, "format") DEF_ATTR_IDENT (ATTR_FORMAT_ARG, "format_arg") DEF_ATTR_IDENT (ATTR_MALLOC, "malloc") +DEF_ATTR_IDENT (ATTR_FREE, "free") DEF_ATTR_IDENT (ATTR_NONNULL, "nonnull") DEF_ATTR_IDENT (ATTR_NORETURN, "noreturn") DEF_ATTR_IDENT (ATTR_NOTHROW, "nothrow") @@ -141,6 +142,10 @@ DEF_ATTR_TREE_LIST (ATTR_MALLOC_NOTHROW_LIST, ATTR_MALLOC, \ ATTR_NULL, ATTR_NOTHROW_LIST) DEF_ATTR_TREE_LIST (ATTR_MALLOC_NOTHROW_LEAF_LIST, ATTR_MALLOC, \ ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) +DEF_ATTR_TREE_LIST (ATTR_FREE_NOTHROW_LIST, ATTR_FREE, \ + ATTR_NULL, ATTR_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_FREE_NOTHROW_LEAF_LIST, ATTR_FREE, \ + ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_SENTINEL_NOTHROW_LIST, ATTR_SENTINEL, \ ATTR_NULL, ATTR_NOTHROW_LIST) DEF_ATTR_TREE_LIST (ATTR_SENTINEL_NOTHROW_LEAF_LIST, ATTR_SENTINEL, \ @@ -269,8 +274,10 @@ DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_RT_LIST, DEF_ATTR_TREE_LIST (ATTR_TMPURE_MALLOC_NOTHROW_LIST, ATTR_TM_TMPURE, ATTR_NULL, ATTR_MALLOC_NOTHROW_LIST) /* Same attributes used for BUILT_IN_FREE except with TM_PURE thrown in. */ -DEF_ATTR_TREE_LIST (ATTR_TMPURE_NOTHROW_LIST, - ATTR_TM_TMPURE, ATTR_NULL, ATTR_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_TMPURE_FREE_NOTHROW_LIST, + ATTR_TM_TMPURE, ATTR_NULL, ATTR_FREE_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_TMPURE_FREE_NOTHROW_LEAF_LIST, + ATTR_TM_TMPURE, ATTR_NULL, ATTR_FREE_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_TMPURE_NOTHROW_LEAF_LIST, ATTR_TM_TMPURE, ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) diff --git a/gcc/builtins.def b/gcc/builtins.def index 2fc7f65..e3d1614 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -781,7 +781,7 @@ DEF_EXT_LIB_BUILTIN(BUILT_IN_FFSLL, "ffsll", BT_FN_INT_LONGLONG, ATTR_CONST_ DEF_EXT_LIB_BUILTIN(BUILT_IN_FORK, "fork", BT_FN_PID, ATTR_NOTHROW_LIST) DEF_GCC_BUILTIN(BUILT_IN_FRAME_ADDRESS, "frame_address", BT_FN_PTR_UINT, ATTR_NULL) /* [trans-mem]: Adjust BUILT_IN_TM_FREE if BUILT_IN_FREE is changed. */ -DEF_LIB_BUILTIN(BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) +DEF_LIB_BUILTIN(BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_FREE_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN(BUILT_IN_FROB_RETURN_ADDR, "frob_return_addr", BT_FN_PTR_PTR, ATTR_NULL) DEF_EXT_LIB_BUILTIN(BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) DEF_C99_BUILTIN(BUILT_IN_IMAXABS, "imaxabs", BT_FN_INTMAX_INTMAX, ATTR_CONST_NOTHROW_LEAF_LIST) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 30c815d..3840675 100644 --- a/gcc/c-family/c-
Re: [C PATCH] PR43651: add warning for duplicate qualifier
On 04/08/2016 08:54 PM, Martin Sebor wrote: >> The name for new option "-Wduplicate-decl-specifier" and wording was >> chosen to match the same option in Clang. > > My version of Clang also warns in C++ mode but if I'm reading > the patch right, GCC would warn only C mode. I would find it > surprising if GCC provided the same option as Clang but didn't > make it available in the same languages. Do you have some > reason for leaving it out that I'm not thinking of? It is an error in C++ mode. Do we want to change this behavior? > > Also, in C11 mode, Clang issues the warning for duplicated > _Atomic qualifiers but it doesn't look like GCC would with > the patch. Here again, unless there's some reason not to, > I would expect GCC to issue the same warning as Clang for > the same code. > Fixed. -- Regards, Mikhail Maltsev gcc/c/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * c-decl.c (declspecs_add_qual): Warn when -Wduplicate-decl-specifier is enabled. * c-errors.c (pedwarn_c90): Return true if warned. * c-tree.h (pedwarn_c90): Change return type to bool. (enum c_declspec_word): Add new enumerator cdw_atomic. gcc/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * doc/invoke.texi (Wduplicate-decl-specifier): Document new option. gcc/testsuite/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * gcc.dg/Wduplicate-decl-specifier-c11.c: New test. * gcc.dg/Wduplicate-decl-specifier.c: Likewise. gcc/c-family/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * c.opt (Wduplicate-decl-specifier): New option. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 4f86876..f7f844d 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1000,6 +1000,10 @@ Wsubobject-linkage C++ ObjC++ Var(warn_subobject_linkage) Warning Init(1) Warn if a class type has a base or a field whose type uses the anonymous namespace or depends on a type with no linkage. +Wduplicate-decl-specifier +C ObjC Var(warn_duplicate_decl_specifier) Warning LangEnabledBy(C ObjC,Wall) +Warn when a declaration has duplicate const, volatile, restrict or _Atomic specifier. + ansi C ObjC C++ ObjC++ A synonym for -std=c89 (for C) or -std=c++98 (for C++). diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 0dd2121..bc3af02 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -9539,32 +9539,48 @@ declspecs_add_qual (source_location loc, gcc_assert (TREE_CODE (qual) == IDENTIFIER_NODE && C_IS_RESERVED_WORD (qual)); i = C_RID_CODE (qual); + location_t prev_loc = 0; switch (i) { case RID_CONST: dupe = specs->const_p; specs->const_p = true; + prev_loc = specs->locations[cdw_const]; specs->locations[cdw_const] = loc; break; case RID_VOLATILE: dupe = specs->volatile_p; specs->volatile_p = true; + prev_loc = specs->locations[cdw_volatile]; specs->locations[cdw_volatile] = loc; break; case RID_RESTRICT: dupe = specs->restrict_p; specs->restrict_p = true; + prev_loc = specs->locations[cdw_restrict]; specs->locations[cdw_restrict] = loc; break; case RID_ATOMIC: dupe = specs->atomic_p; specs->atomic_p = true; + prev_loc = specs->locations[cdw_atomic]; + specs->locations[cdw_atomic] = loc; break; default: gcc_unreachable (); } if (dupe) -pedwarn_c90 (loc, OPT_Wpedantic, "duplicate %qE", qual); +{ + bool warned = pedwarn_c90 (loc, OPT_Wpedantic, + "duplicate %qE declaration specifier", qual); + if (!warned + && warn_duplicate_decl_specifier + && prev_loc >= RESERVED_LOCATION_COUNT + && !from_macro_expansion_at (prev_loc) + && !from_macro_expansion_at (loc)) + warning_at (loc, OPT_Wduplicate_decl_specifier, + "duplicate %qE declaration specifier", qual); +} return specs; } diff --git a/gcc/c/c-errors.c b/gcc/c/c-errors.c index d5e78b8..af157c0 100644 --- a/gcc/c/c-errors.c +++ b/gcc/c/c-errors.c @@ -71,11 +71,12 @@ pedwarn_c99 (location_t location, int opt, const char *gmsgid, ...) ISO C99 but not supported in ISO C90, thus we explicitly don't pedwarn when C99 is specified. (There is no flag_c90.) */ -void +bool pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) { diagnostic_info diagnostic; va_list ap; + bool warned = false; rich_location richloc (line_table, location); va_start (ap, gmsgid); @@ -92,6 +93,7 @@ pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) ? DK_PEDWARN : DK_WARNING); diagnostic.option_index = opt;
Re: [C PATCH] PR43651: add warning for duplicate qualifier
On 04/08/2016 12:50 AM, Joseph Myers wrote: > New options need documenting in invoke.texi. > Done. -- Regards, Mikhail Maltsev gcc/c/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * c-decl.c (declspecs_add_qual): Warn when -Wduplicate-decl-specifier is enabled. * c-errors.c (pedwarn_c90): Return true if warned. * c-tree.h: Change return type to bool. gcc/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * doc/invoke.texi (Wduplicate-decl-specifier): Document new option. gcc/testsuite/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * gcc.dg/Wduplicate-decl-specifier.c: New test. gcc/c-family/ChangeLog: 2016-04-08 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * c.opt (Wduplicate-decl-specifier): New option. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 4f86876..c45c714 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1000,6 +1000,10 @@ Wsubobject-linkage C++ ObjC++ Var(warn_subobject_linkage) Warning Init(1) Warn if a class type has a base or a field whose type uses the anonymous namespace or depends on a type with no linkage. +Wduplicate-decl-specifier +C ObjC Var(warn_duplicate_decl_specifier) Warning LangEnabledBy(C ObjC,Wall) +Warn when a declaration has duplicate const, volatile or restrict specifier. + ansi C ObjC C++ ObjC++ A synonym for -std=c89 (for C) or -std=c++98 (for C++). diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 0dd2121..9902326 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -9539,21 +9539,25 @@ declspecs_add_qual (source_location loc, gcc_assert (TREE_CODE (qual) == IDENTIFIER_NODE && C_IS_RESERVED_WORD (qual)); i = C_RID_CODE (qual); + location_t prev_loc = 0; switch (i) { case RID_CONST: dupe = specs->const_p; specs->const_p = true; + prev_loc = specs->locations[cdw_const]; specs->locations[cdw_const] = loc; break; case RID_VOLATILE: dupe = specs->volatile_p; specs->volatile_p = true; + prev_loc = specs->locations[cdw_volatile]; specs->locations[cdw_volatile] = loc; break; case RID_RESTRICT: dupe = specs->restrict_p; specs->restrict_p = true; + prev_loc = specs->locations[cdw_restrict]; specs->locations[cdw_restrict] = loc; break; case RID_ATOMIC: @@ -9564,7 +9568,17 @@ declspecs_add_qual (source_location loc, gcc_unreachable (); } if (dupe) -pedwarn_c90 (loc, OPT_Wpedantic, "duplicate %qE", qual); +{ + bool warned = pedwarn_c90 (loc, OPT_Wpedantic, + "duplicate %qE declaration specifier", qual); + if (!warned + && warn_duplicate_decl_specifier + && prev_loc >= RESERVED_LOCATION_COUNT + && !from_macro_expansion_at (prev_loc) + && !from_macro_expansion_at (loc)) + warning_at (loc, OPT_Wduplicate_decl_specifier, + "duplicate %qE declaration specifier", qual); +} return specs; } diff --git a/gcc/c/c-errors.c b/gcc/c/c-errors.c index d5e78b8..af157c0 100644 --- a/gcc/c/c-errors.c +++ b/gcc/c/c-errors.c @@ -71,11 +71,12 @@ pedwarn_c99 (location_t location, int opt, const char *gmsgid, ...) ISO C99 but not supported in ISO C90, thus we explicitly don't pedwarn when C99 is specified. (There is no flag_c90.) */ -void +bool pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) { diagnostic_info diagnostic; va_list ap; + bool warned = false; rich_location richloc (line_table, location); va_start (ap, gmsgid); @@ -92,6 +93,7 @@ pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) ? DK_PEDWARN : DK_WARNING); diagnostic.option_index = opt; report_diagnostic (); + warned = true; goto out; } } @@ -114,7 +116,9 @@ pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) diagnostic_set_info (, gmsgid, , , DK_PEDWARN); diagnostic.option_index = opt; report_diagnostic (); + warned = true; } out: va_end (ap); + return warned; } diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 96ab049..ac72820 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -716,7 +716,7 @@ extern void c_bind (location_t, tree, bool); extern bool tag_exists_p (enum tree_code, tree); /* In c-errors.c */ -extern void pedwarn_c90 (location_t, int opt, const char *, ...) +extern bool pedwarn_c90 (location_t, int opt, const char *, ...) ATTRIBUTE_GCC_DIAG(3,4); extern bool pedwarn_c99 (location_t, int opt, const char *, ...) ATTRIBUTE_GCC_DIAG(3,4); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index e9763d4..02fb10f 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -3538,6 +3538,7 @@ Options} and @ref{Obje
[C PATCH] PR43651: add warning for duplicate qualifier
Hi all! Currently GCC produces pedantic warning, if variable declaration (or typedef) has duplicate qualifier, but only when compiling as C89 (not C99 or C11). The attached patch adds a new warning option to enable the same warning in C99 and C11. It also checks whether qualifiers come from macro expansion, e.g.: #define CT2 const int const CT2 x1; CT2 const x2; and does not warn in this case, but warns for, e.g. void foo(const int const *x) { } (because this probably meant to be "const int *const x") The name for new option "-Wduplicate-decl-specifier" and wording was chosen to match the same option in Clang. Bootstrapped and regtested on x86_64-linux. OK for next stage 1? -- Regards, Mikhail Maltsev gcc/c/ChangeLog: 2016-04-04 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * c-decl.c (declspecs_add_qual): Warn when -Wduplicate-decl-specifier is enabled. * c-errors.c (pedwarn_c90): Return true if warned. * c-tree.h: Change return type to bool. gcc/testsuite/ChangeLog: 2016-04-04 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * gcc.dg/Wduplicate-decl-specifier.c: New test. gcc/c-family/ChangeLog: 2016-04-04 Mikhail Maltsev <malts...@gmail.com> PR c/43651 * c.opt (Wduplicate-decl-specifier): New option. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 4f86876..1650b25 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1000,6 +1000,10 @@ Wsubobject-linkage C++ ObjC++ Var(warn_subobject_linkage) Warning Init(1) Warn if a class type has a base or a field whose type uses the anonymous namespace or depends on a type with no linkage. +Wduplicate-decl-specifier +C ObjC Var(warn_duplicate_decl_specifier) Warning LangEnabledBy(C ObjC,Wall) +Warn when a variable declaration has duplicate const, volatile or restrict specifier. + ansi C ObjC C++ ObjC++ A synonym for -std=c89 (for C) or -std=c++98 (for C++). diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 0dd2121..9902326 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -9539,21 +9539,25 @@ declspecs_add_qual (source_location loc, gcc_assert (TREE_CODE (qual) == IDENTIFIER_NODE && C_IS_RESERVED_WORD (qual)); i = C_RID_CODE (qual); + location_t prev_loc = 0; switch (i) { case RID_CONST: dupe = specs->const_p; specs->const_p = true; + prev_loc = specs->locations[cdw_const]; specs->locations[cdw_const] = loc; break; case RID_VOLATILE: dupe = specs->volatile_p; specs->volatile_p = true; + prev_loc = specs->locations[cdw_volatile]; specs->locations[cdw_volatile] = loc; break; case RID_RESTRICT: dupe = specs->restrict_p; specs->restrict_p = true; + prev_loc = specs->locations[cdw_restrict]; specs->locations[cdw_restrict] = loc; break; case RID_ATOMIC: @@ -9564,7 +9568,17 @@ declspecs_add_qual (source_location loc, gcc_unreachable (); } if (dupe) -pedwarn_c90 (loc, OPT_Wpedantic, "duplicate %qE", qual); +{ + bool warned = pedwarn_c90 (loc, OPT_Wpedantic, + "duplicate %qE declaration specifier", qual); + if (!warned + && warn_duplicate_decl_specifier + && prev_loc >= RESERVED_LOCATION_COUNT + && !from_macro_expansion_at (prev_loc) + && !from_macro_expansion_at (loc)) + warning_at (loc, OPT_Wduplicate_decl_specifier, + "duplicate %qE declaration specifier", qual); +} return specs; } diff --git a/gcc/c/c-errors.c b/gcc/c/c-errors.c index d5e78b8..af157c0 100644 --- a/gcc/c/c-errors.c +++ b/gcc/c/c-errors.c @@ -71,11 +71,12 @@ pedwarn_c99 (location_t location, int opt, const char *gmsgid, ...) ISO C99 but not supported in ISO C90, thus we explicitly don't pedwarn when C99 is specified. (There is no flag_c90.) */ -void +bool pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) { diagnostic_info diagnostic; va_list ap; + bool warned = false; rich_location richloc (line_table, location); va_start (ap, gmsgid); @@ -92,6 +93,7 @@ pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) ? DK_PEDWARN : DK_WARNING); diagnostic.option_index = opt; report_diagnostic (); + warned = true; goto out; } } @@ -114,7 +116,9 @@ pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...) diagnostic_set_info (, gmsgid, , , DK_PEDWARN); diagnostic.option_index = opt; report_diagnostic (); + warned = true; } out: va_end (ap); + return warned; } diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 96ab049..ac72820 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -716,7 +716,7 @@ extern void c_bind (location_t, tree, bool); extern bool tag_exists_p (enum tree_code, tree);
[PATCH, RFC]. Some initial work on coroutines
Hi, all! I decided to start working on coroutines support in C++ (according to P0057R2 proposal: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0057r2.pdf). I did some analysis of how other compilers implement coroutines: https://github.com/miyuki-chan/coroutines This github repo has a small example project for Microsoft Visual Studio and also the relevant part of assembly code generated by MSVC. It also has a link to Clang testcase for coroutines semantic analysis and diagnostics, as well as the actual diagnostic output of Clang for that testcase. In the attached patch I started to work on parsing and semantic analysis. For now it can parse some basic uses of co_await, co_yield and co_return and diagnose some erroneous cases. No support for templates, and no 'for co_await' yet. My main question is: what is the correct way of constructing the AST? I started looking at how range-based for loops are implemented. As far as I understand, GCC converts range-based for into normal for loop immediately, when possible, i.e. when there are no dependent types involved. But when parsing template definition, GCC may create a node for range-based for loop and convert it during instantiation. Should we do the same for co_await/co_yield/co_return? Or would it be better to always construct just AST nodes (perhaps with some meta-information) and delay actual codegen until we start genericising the function (apparently, Clang works this way)? Just it case, what I mean by codegen. The proposal says: "Let e be the operand of the yield-expression and p be an lvalue naming the promise object of the enclosing coroutine (8.4.4), then the yield-expression is equivalent to the expression co_await p.yield_value(e)." So, when we parse "co_yield e", when should we build "co_await p.yield_value(e)" from it and further expand co_await? -- Regards, Mikhail Maltsev diff --git a/.gitignore b/.gitignore index c9a6158..4595d5e 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,9 @@ TAGS.sub .clang-format +.agignore +.ycm_extra_conf.py + .gdbinit .gdb_history diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 22ea7da..9962ab5 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -598,6 +598,11 @@ const struct c_common_resword c_common_reswords[] = { "concept", RID_CONCEPT, D_CXX_CONCEPTS_FLAGS | D_CXXWARN }, { "requires", RID_REQUIRES, D_CXX_CONCEPTS_FLAGS | D_CXXWARN }, + /* C++ coroutines */ + { "co_await", RID_CO_AWAIT, D_CXX_COROUTINES_FLAGS | D_CXXWARN }, + { "co_return", RID_CO_RETURN, D_CXX_COROUTINES_FLAGS | D_CXXWARN }, + { "co_yield", RID_CO_YIELD, D_CXX_COROUTINES_FLAGS | D_CXXWARN }, + /* These Objective-C keywords are recognized only immediately after an '@'. */ { "compatibility_alias", RID_AT_ALIAS, D_OBJC }, diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index fa3746c..4f753c9 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -154,6 +154,9 @@ enum rid /* C++ concepts */ RID_CONCEPT, RID_REQUIRES, + /* C++ coroutines */ + RID_CO_AWAIT, RID_CO_RETURN, RID_CO_YIELD, + /* C++ transactional memory. */ RID_ATOMIC_NOEXCEPT, RID_ATOMIC_CANCEL, RID_SYNCHRONIZED, @@ -383,20 +386,22 @@ extern machine_mode c_default_pointer_mode; mask) is _true_. Thus for keywords which are present in all languages the disable field is zero. */ -#define D_CONLY 0x001 /* C only (not in C++). */ -#define D_CXXONLY 0x002 /* C++ only (not in C). */ -#define D_C99 0x004 /* In C, C99 only. */ -#define D_CXX11 0x008 /* In C++, C++11 only. */ -#define D_EXT 0x010 /* GCC extension. */ -#define D_EXT89 0x020 /* GCC extension incorporated in C99. */ -#define D_ASM 0x040 /* Disabled by -fno-asm. */ -#define D_OBJC 0x080 /* In Objective C and neither C nor C++. */ -#define D_CXX_OBJC 0x100 /* In Objective C, and C++, but not C. */ -#define D_CXXWARN 0x200 /* In C warn with -Wcxx-compat. */ -#define D_CXX_CONCEPTS 0x400 /* In C++, only with concepts. */ -#define D_TRANSMEM 0X800 /* C++ transactional memory TS. */ +#define D_CONLY 0x0001 /* C only (not in C++). */ +#define D_CXXONLY 0x0002 /* C++ only (not in C). */ +#define D_C99 0x0004 /* In C, C99 only. */ +#define D_CXX11 0x0008 /* In C++, C++11 only. */ +#define D_EXT 0x0010 /* GCC extension. */ +#define D_EXT89 0x0020 /* GCC extension incorporated in C99. */ +#define D_ASM 0x0040 /* Disabled by -fno-asm. */ +#define D_OBJC 0x0080 /* In Objective C and neither C nor C++. */ +#define D_CXX_OBJC 0x0100 /* In Objective C, and C++, but not C. */ +#define D_CXXWARN 0x0200 /* In C warn with -Wcxx-compat. */ +#define D_CXX_CONCEPTS 0x0400 /* In C++, only with concepts. */ +#define D_TRANSMEM 0x0800 /* C++ transactional memory TS. */ +#define D_CXX_COROUTINES 0x1000 /*
[PATCH] PR69329 --with-build-config=bootstrap-asan fails because LSAN_OPTIONS is not honored
Hi all! --with-build-config=bootstrap-asan build currently fails, because address sanitizer now has memory leak detection enabled by default. bootstrap-asan contains export LSAN_OPTIONS="detect_leaks=0" but unfortunately this environment variable is set in top-level Makefile and is not propagated to Makefile-s in subdirectories. The attached patch fixes this by adding LSAN_OPTIONS to BASE_FLAGS_TO_PASS. It also incorporates Jakub's comments from bugzilla. Bootstrapped and regtested on x86_64-pc-linux-gnu with default configuration and also with "--with-build-config=bootstrap-asan". OK for trunk? -- Regards, Mikhail Maltsev ChangeLog: 2016-01-24 Mikhail Maltsev <malts...@gmail.com> PR bootstrap/69329 * Makefile.in: Regenerate. * Makefile.tpl (BASE_FLAGS_TO_PASS): Add LSAN_OPTIONS. diff --git a/Makefile.in b/Makefile.in index 2733c4d..20d1c90 100644 --- a/Makefile.in +++ b/Makefile.in @@ -789,7 +789,8 @@ BASE_FLAGS_TO_PASS = \ $(CXX_FOR_TARGET_FLAG_TO_PASS) \ "TFLAGS=$(TFLAGS)" \ "CONFIG_SHELL=$(SHELL)" \ - "MAKEINFO=$(MAKEINFO) $(MAKEINFOFLAGS)" + "MAKEINFO=$(MAKEINFO) $(MAKEINFOFLAGS)" \ + $(if $(LSAN_OPTIONS),"LSAN_OPTIONS=$(LSAN_OPTIONS)") # We leave this in just in case, but it is not needed anymore. RECURSE_FLAGS_TO_PASS = $(BASE_FLAGS_TO_PASS) diff --git a/Makefile.tpl b/Makefile.tpl index f7bb77e..2567365 100644 --- a/Makefile.tpl +++ b/Makefile.tpl @@ -590,7 +590,8 @@ BASE_FLAGS_TO_PASS =[+ FOR flags_to_pass +][+ IF optional +] \ $(CXX_FOR_TARGET_FLAG_TO_PASS) \ "TFLAGS=$(TFLAGS)" \ "CONFIG_SHELL=$(SHELL)" \ - "MAKEINFO=$(MAKEINFO) $(MAKEINFOFLAGS)" + "MAKEINFO=$(MAKEINFO) $(MAKEINFOFLAGS)" \ + $(if $(LSAN_OPTIONS),"LSAN_OPTIONS=$(LSAN_OPTIONS)") # We leave this in just in case, but it is not needed anymore. RECURSE_FLAGS_TO_PASS = $(BASE_FLAGS_TO_PASS)
Re: [wwwdocs] gcc-6/changes.html: diagnostics, Levenshtein, -Wmisleading-indentation, jit (v2)
On 01/19/2016 10:05 PM, Manuel López-Ibáñez wrote: > Am I the only one who doesn't see the colors at > https://gcc.gnu.org/gcc-6/changes.html#c-family nor > https://gcc.gnu.org/gcc-5/changes.html#fortran ? > > Firefox 43.0.4 says "Content Security Policy: The page's settings blocked the > loading of a resource at self ("default-src https://gcc.gnu.org http: > https:")." This problem also reproduces with Chrome Version "47.0.2526.106 unknown (64-bit)". The error message says: "Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' http: https:". Either the 'unsafe-inline' keyword, a hash ('sha256-8f935d27GvUutRyY9yWScUMiFUk4WTdZURISiYfPOeQ='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback." I think, this can be fixed by replacing 'style="color:red"' with 'class="..."' and adding the corresponding CSS class to the stylesheet. -- Regards, Mikhail Maltsev
Re: [doc, 5/n] invoke.texi: add new "Program Instrumentation Options" section
On 01/15/2016 05:17 AM, Sandra Loosemore wrote: > This patch consolidates the documentation of GCC options that add runtime > profiling, error checking, or other instrumentation into a single section. > Currently these are scattered all over, variously classified as debugging > options, code generation options, optimization options, etc. > > Here is the list of options that I moved into the new section: > > @gccoptlist{-p -pg -fprofile-arcs --coverage -ftest-coverage @gol (snip) The list mentions "-fchecking", but this option is not related to instrumentation. In just enables consistency checks of the compiler's internal state, i.e. it is more related to debugging GCC itself. -- Regards, Mikhail Maltsev
Re: [PATCH, i386] PR68497. Fix ICE with -fno-checking
On 11/24/2015 02:43 AM, Bernd Schmidt wrote: > On 11/24/2015 12:09 AM, Mikhail Maltsev wrote: >> The attached patch fixes a problem introduced in r229567: the assertion >> >> gcc_assert (is_sse); >> >> is checked if flag_checking is false, and this causes an ICE when >> compiling with >> -fno-checking. >> > Ok. > > > Bernd > Committed as r230803. -- Regards, Mikhail Maltsev
[PATCH, i386] PR68497. Fix ICE with -fno-checking
Hi! The attached patch fixes a problem introduced in r229567: the assertion gcc_assert (is_sse); is checked if flag_checking is false, and this causes an ICE when compiling with -fno-checking. Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-11-23 Mikhail Maltsev <malts...@gmail.com> PR target/68497 * config/i386/i386.c (output_387_binary_op): Fix assertion for -fno-checking case. gcc/testsuite/ChangeLog: 2015-11-23 Mikhail Maltsev <malts...@gmail.com> PR target/68497 * gcc.target/i386/pr68497.c: New test. diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 83749d5..23dbb3a 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -17675,18 +17675,20 @@ output_387_binary_op (rtx insn, rtx *operands) /* Even if we do not want to check the inputs, this documents input constraints. Which helps in understanding the following code. */ - if (flag_checking - && STACK_REG_P (operands[0]) - && ((REG_P (operands[1]) - && REGNO (operands[0]) == REGNO (operands[1]) - && (STACK_REG_P (operands[2]) || MEM_P (operands[2]))) - || (REG_P (operands[2]) - && REGNO (operands[0]) == REGNO (operands[2]) - && (STACK_REG_P (operands[1]) || MEM_P (operands[1] - && (STACK_TOP_P (operands[1]) || STACK_TOP_P (operands[2]))) -; /* ok */ - else -gcc_checking_assert (is_sse); + if (flag_checking) +{ + if (STACK_REG_P (operands[0]) + && ((REG_P (operands[1]) + && REGNO (operands[0]) == REGNO (operands[1]) + && (STACK_REG_P (operands[2]) || MEM_P (operands[2]))) + || (REG_P (operands[2]) + && REGNO (operands[0]) == REGNO (operands[2]) + && (STACK_REG_P (operands[1]) || MEM_P (operands[1] + && (STACK_TOP_P (operands[1]) || STACK_TOP_P (operands[2]))) + ; /* ok */ + else + gcc_assert (is_sse); +} switch (GET_CODE (operands[3])) { diff --git a/gcc/testsuite/gcc.target/i386/pr68497.c b/gcc/testsuite/gcc.target/i386/pr68497.c new file mode 100644 index 000..0135cda --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr68497.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-fno-checking" } */ + +long double +foo (long double x, long double y) +{ + return x + y; +}
Re: [PATCH 01/15] Selftest framework (unittests v4)
On 11/19/2015 09:44 PM, Bernd Schmidt wrote: > It's one option, but it doesn't seem like the best one either. I was > thinking of something not dissimilar to your approach, but with fewer > bells and whistles. My class registrator would look something like this: > > static list test_callbacks; > > class registrator > { > public: > registrator (void (*)() cb) > { > test_callbacks.push_front (cb); > } > } > > (or use a vec if you can do that at global constructor time) > > and then you just walk the list and run the callbacks when you want to > run tests. The one you have implements both the registration and a > special case linked list, which just doesn't look right to me, and I > think I'd also have avoided the runner class. Google test allows to run tests which match a given regular expression. This is very convenient when you debug a failing test: you just need to run the testsuite under debugger and specify the relevant (failing) test as a filter. I think this feature is worth implementing eventually (maybe regex is an overkill, and matching against a substring will be enough). So, if testcases are implemented as objects, it will easy to add filtering. If testcases are just callbacks, names and possibly some other metainformation need to be stored separately. FWIW, I worked with other unit test frameworks, such as CppUnit (but Google Test is superior, IMHO) and they use the same approach: testcases are objects which store metainformation (such as name) and have methods for running tests. -- Regards, Mikhail Maltsev
Re: [PATCH 10/9] ENABLE_CHECKING refactoring: remove remaining occurrences
On 11/01/2015 11:34 PM, Bernhard Reutner-Fischer wrote: > Mikhail, > > On November 1, 2015 9:19:19 PM GMT+01:00, Mikhail Maltsev > <malts...@gmail.com> wrote: >> This patch cleans up remaining bits related to ENABLE_CHECKING. After >> applying >> this patch (on top of part 9) we will no longer have any references to >> ENABLE_CHECKING in the source code. > > I don't remember if you sent size(1) comparison for the frontends and driver, > BTW. How bad is it? > > TIA, > Here are the results for r228786 "base" (bootstrapped with --enable-checking=release) and for the same revision after applying ENABLE_CHECKING-related patches "head": text data bss dec filename rev cc1 base 21442165 70624 1343960 22856749 head 21489220 70624 1343992 22903836 cc1plus base 22772252 70712 1369624 24212588 head 22820770 70712 1369656 24261138 f951 base 22230769 80136 1349592 23660497 head 22277096 80136 1349624 23706856 lto1 base 20611962 69792 1342008 22023762 head 20658721 69792 1342040 22070553 gcc base 1606938 2840017056 1652394 head 1608434 2840017056 1653890 Relative difference: text data bss dec filename cc1 0.002195 0 0.24 0.002060 cc1plus 0.002131 0 0.23 0.002005 f951 0.002084 0 0.24 0.001959 lto1 0.002269 0 0.24 0.002125 gcc 0.000931 0 0.00 0.000905 -- Regards, Mikhail Maltsev
Re: [PATCH 9/9] ENABLE_CHECKING refactoring: C family front ends
On 11/03/2015 02:34 AM, Jeff Law wrote: > >> @@ -14279,8 +14272,9 @@ tsubst_copy (tree t, tree args, tsubst_flags_t >> complain, tree in_decl) >> return tsubst_binary_right_fold (t, args, complain, in_decl); >> >> default: >> - /* We shouldn't get here, but keep going if !ENABLE_CHECKING. */ >> - gcc_checking_assert (false); >> + /* We shouldn't get here, but keep going if !flag_checking. */ >> + if (!flag_checking) >> +gcc_unreachable (); >> return t; >> } >> } > I think this condition was reversed, right? > > Please fix that and install on the trunk. > > Thanks again! > > jeff Fixed and committed as r229756. -- Regards, Mikhail Maltsev
Re: [PATCH 10/9] ENABLE_CHECKING refactoring: remove remaining occurrences
On 11/03/2015 02:35 AM, Jeff Law wrote: > This is good fore the trunk too. Please install. > > Thanks! > > jeff Committed as r229758. -- Regards, Mikhail Maltsev
[PATCH 9/9] ENABLE_CHECKING refactoring: C family front ends
Hi all! This patch was intended to be the last one in this series (but I'll send one more cleanup patch today). It removes ENABLE_CHECKING macros in the C++ front end (and also touches a small piece of common C family code in OpenMP). I could convert most of "#ifdef ENABLE_CHECKING" conditions into checks performed at run time (i.e. flag_checking), except for GET_NON_DEFAULT_TEMPLATE_ARGS_COUNT logic. The definition of this macro depends on ENABLE_CHECKING (now CHECKING_P) and frankly speaking I don't know how these checks should work (though I did not try hard to understand the details, I hope Jason will give some comments), so I left them as-is, just got rid of conditional compilation (i.e., used CHECKING_P). Also, the patch renames 'check_unstripped_args' into 'verify_unstripped_args' because I think it's more consistent with names of other internal state checking functions. Bootstrapped and regtested on x86_64-pc-linux-gnu with --enable-checking=yes and --enable-checking=release. (there were some problems with languages other than C and C++, not related to the patch, but ISTM they were fixed, so I'm currently running a second check after rebasing). OK for trunk? -- Regards, Mikhail Maltsev gcc/c-family/ChangeLog: 2015-10-31 Mikhail Maltsev <malts...@gmail.com> * c-omp.c (c_omp_split_clauses): Remove conditional compilation. Use flag_checking. gcc/cp/ChangeLog: 2015-10-31 Mikhail Maltsev <malts...@gmail.com> * call.c (validate_conversion_obstack): Define unconditionally. * constexpr.c (maybe_constant_value, fold_non_dependent_expr): Use gcc_checking_assert. * cp-tree.h: Use CHECKING_P instead of ENABLE_CHECKING. * decl2.c (cxx_post_compilation_parsing_cleanups): Use flag_checking. * mangle.c (add_substitution): Likewise. * method.c (maybe_explain_implicit_delete): Likewise. * parser.c (cp_parser_template_argument_list): Remove conditional compilation. * pt.c (check_unstripped_args): Rename to... (verify_unstripped_args): ...this and remove conditional compilation. (retrieve_specialization): Guard call of verify_unstripped_args with flag_checking. (template_parm_to_arg): Remove conditional compilation. (template_parms_to_args, coerce_template_parameter_pack, coerce_template_parms): Likewise. (tsubst_copy): Use flag_checking. (type_unification_real): Remove conditional compilation. (build_non_dependent_expr): Use flag_checking. * tree.c (build_target_expr): Remove conditional compilation, use gcc_checking_assert. * typeck.c (comptypes): Likewise. * typeck2.c (digest_init_r): Likewise. diff --git a/gcc/c-family/c-omp.c b/gcc/c-family/c-omp.c index 133d079..ca64eda 100644 --- a/gcc/c-family/c-omp.c +++ b/gcc/c-family/c-omp.c @@ -1135,7 +1135,10 @@ c_omp_split_clauses (location_t loc, enum tree_code code, OMP_CLAUSE_CHAIN (clauses) = cclauses[s]; cclauses[s] = clauses; } -#ifdef ENABLE_CHECKING + + if (!flag_checking) +return; + if ((mask & (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_MAP)) == 0) gcc_assert (cclauses[C_OMP_CLAUSE_SPLIT_TARGET] == NULL_TREE); if ((mask & (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_NUM_TEAMS)) == 0) @@ -1150,7 +1153,6 @@ c_omp_split_clauses (location_t loc, enum tree_code code, gcc_assert (cclauses[C_OMP_CLAUSE_SPLIT_FOR] == NULL_TREE); if (code != OMP_SIMD) gcc_assert (cclauses[C_OMP_CLAUSE_SPLIT_SIMD] == NULL_TREE); -#endif } diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 9178188..0b7d143 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -724,8 +724,6 @@ alloc_conversion (conversion_kind kind) return c; } -#ifdef ENABLE_CHECKING - /* Make sure that all memory on the conversion obstack has been freed. */ @@ -737,8 +735,6 @@ validate_conversion_obstack (void) == obstack_base (_obstack))); } -#endif /* ENABLE_CHECKING */ - /* Dynamically allocate an array of N conversions. */ static conversion ** diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 038c6f5..51fae5a 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -3856,13 +3856,11 @@ maybe_constant_value (tree t, tree decl) } r = cxx_eval_outermost_constant_expr (t, true, true, decl); -#ifdef ENABLE_CHECKING - gcc_assert (r == t - || CONVERT_EXPR_P (t) - || TREE_CODE (t) == VIEW_CONVERT_EXPR - || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) - || !cp_tree_equal (r, t)); -#endif + gcc_checking_assert (r == t + || CONVERT_EXPR_P (t) + || TREE_CODE (t) == VIEW_CONVERT_EXPR + || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) + || !cp_tree_equal (r, t)); return r; } @@ -3906,14 +3904,12 @@ fold_non_dependent_expr (tree t) } tree r = cxx_eval_outermost_constant_expr (t, true, true, NULL_TREE);
[PATCH 10/9] ENABLE_CHECKING refactoring: remove remaining occurrences
This patch cleans up remaining bits related to ENABLE_CHECKING. After applying this patch (on top of part 9) we will no longer have any references to ENABLE_CHECKING in the source code. Bootstrapped and regtested (on top of part 9) on x86_64-pc-linux-gnu with --enable-checking=yes and --enable-checking=release. libcpp/ChangeLog: 2015-11-01 Mikhail Maltsev <malts...@gmail.com> * config.in: Regenerate. * configure: Regenerate. * configure.ac: Remove ENABLE_CHECKING. gcc/ChangeLog: 2015-11-01 Mikhail Maltsev <malts...@gmail.com> * cfganal.c (inverted_post_order_compute): Remove conditional compilation, use flag_checking. * config.in: Regenerate. * configure: Regenerate. * configure.ac: Remove ENABLE_CHECKING. * genconditions.c: Do not #undef ENABLE_CHECKING. * sese.h (bb_in_region): Comment out broken check. * tree-ssa-loop-manip.c (rewrite_into_loop_closed_ssa_1): Remove conditional compilation, use flag_checking. -- Regards, Mikhail Maltsev diff --git a/gcc/cfganal.c b/gcc/cfganal.c index 6f3e348..0f26038 100644 --- a/gcc/cfganal.c +++ b/gcc/cfganal.c @@ -784,9 +784,8 @@ inverted_post_order_compute (int *post_order) int post_order_num = 0; sbitmap visited; -#if ENABLE_CHECKING - verify_no_unreachable_blocks (); -#endif + if (flag_checking) +verify_no_unreachable_blocks (); /* Allocate stack for back-tracking up CFG. */ stack = XNEWVEC (edge_iterator, n_basic_blocks_for_fn (cfun) + 1); diff --git a/gcc/config.in b/gcc/config.in index 48d7e64..6f46f70 100644 --- a/gcc/config.in +++ b/gcc/config.in @@ -76,13 +76,6 @@ #endif -/* Define if you want more run-time sanity checks. This one gets a grab bag of - miscellaneous but relatively cheap checks. */ -#ifndef USED_FOR_TARGET -#undef ENABLE_CHECKING -#endif - - /* Define to 1 to specify that we are using the BID decimal floating point format instead of DPD */ #ifndef USED_FOR_TARGET diff --git a/gcc/configure b/gcc/configure index 92bda6c..1d2e8f2 100755 --- a/gcc/configure +++ b/gcc/configure @@ -7098,9 +7098,6 @@ IFS="$ac_save_IFS" nocommon_flag="" if test x$ac_checking != x ; then -$as_echo "#define ENABLE_CHECKING 1" >>confdefs.h - - $as_echo "#define CHECKING_P 1" >>confdefs.h nocommon_flag=-fno-common @@ -18405,7 +18402,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 18408 "configure" +#line 18405 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -18511,7 +18508,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 18514 "configure" +#line 18511 "configure" #include "confdefs.h" #if HAVE_DLFCN_H diff --git a/gcc/configure.ac b/gcc/configure.ac index 7e22267..d03a0bd 100644 --- a/gcc/configure.ac +++ b/gcc/configure.ac @@ -566,9 +566,6 @@ IFS="$ac_save_IFS" nocommon_flag="" if test x$ac_checking != x ; then - AC_DEFINE(ENABLE_CHECKING, 1, -[Define if you want more run-time sanity checks. This one gets a grab - bag of miscellaneous but relatively cheap checks.]) AC_DEFINE(CHECKING_P, 1, [Define to 1 if you want more run-time sanity checks. This one gets a grab bag of miscellaneous but relatively cheap checks.]) diff --git a/gcc/genconditions.c b/gcc/genconditions.c index 7481ab4..dd5bc92 100644 --- a/gcc/genconditions.c +++ b/gcc/genconditions.c @@ -59,7 +59,6 @@ write_header (void) #if GCC_VERSION >= 3001\n\ \n\ /* Do not allow checking to confuse the issue. */\n\ -#undef ENABLE_CHECKING\n\ #undef CHECKING_P\n\ #define CHECKING_P 0\n\ #undef ENABLE_TREE_CHECKING\n\ diff --git a/gcc/sese.h b/gcc/sese.h index d2ad9bd..98ab491 100644 --- a/gcc/sese.h +++ b/gcc/sese.h @@ -108,16 +108,18 @@ sese_nb_params (sese_info_p region) static inline bool bb_in_region (basic_block bb, basic_block entry, basic_block exit) { -#ifdef ENABLE_CHECKING - { -edge e; -edge_iterator ei; - -/* Check that there are no edges coming in the region: all the - predecessors of EXIT are dominated by ENTRY. */ -FOR_EACH_EDGE (e, ei, exit->preds) - dominated_by_p (CDI_DOMINATORS, e->src, entry); - } + /* FIXME: PR67842. */ +#if 0 + if (flag_checking) +{ + edge e; + edge_iterator ei; + + /* Check that there are no edges coming in the region: all the + predecessors of EXIT are dominated by ENTRY. */ + FOR_EACH_EDGE (e, ei, exit->preds) + gcc_assert (dominated_by_p (CDI_DOMINATORS, e->src, entry)); +} #endif return dominated_by_p (CDI_DOMINATORS, bb, entry) diff --git a/gcc/tree-ssa-loop-manip.c b/gcc/tree-ssa-loop-manip.c index db797cc..b614412 100644 --- a/gcc/tree-ssa-loop-manip.c +++
Re: [PATCH] Add -fchecking
On 10/27/2015 04:17 PM, Richard Biener wrote: > > This adds -fchecking as a way to enable internal consistency checks > even in release builds (or disable checking with -fno-checking - up to > a certain extent - with checking enabled). I remember that Jakub proposed to use __builtin_expect with flag_checking. I wonder, if it is possible to implement without hacking AWK scripts just for this particular case? For example, to define flag_checking to something like #define flag_checking __builtin_expect (flag_checking_val, CHECKING_P) (provided that flag_checking_val is the actual value got from command-line options). -- Regards, Mikhail Maltsev
Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators
On 10/26/2015 12:47 PM, Richard Biener wrote: > I committed the attached to fix build with the valgrind annotations active. > > Richard. > Doh! Sorry for breakage. -- Regards, Mikhail Maltsev
Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators
On 10/21/2015 01:57 PM, Richard Biener wrote: > Ugh (stupid templates). > > @@ -387,10 +389,10 @@ base_pool_allocator ::allocate () >block = m_virgin_free_list; >header = (allocation_pool_list*) allocation_object::get_data (block); >header->next = NULL; > -#ifdef ENABLE_CHECKING > + >/* Mark the element to be free. */ > - ((allocation_object*) block)->id = 0; > -#endif > + if (flag_checking) > + ((allocation_object*) block)->id = 0; > > just set id to zero unconditionally. That'll be faster than checking > flag_checking. I fixed this and other issues, and committed the attached patch. -- Regards, Mikhail Maltsev diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 81d0e1c..d8a22c3 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2015-10-26 Mikhail Maltsev <malts...@gmail.com> + + * alloc-pool.h (base_pool_allocator::initialize, ::allocate): Remove + conditional compilation. + (base_pool_allocator::remove): Use flag_checking. + 2015-10-25 John David Anglin <dang...@gcc.gnu.org> * config/pa/som.h (EH_FRAME_THROUGH_COLLECT2): Define. diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 70105ba..404b558 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see #define ALLOC_POOL_H #include "memory-block.h" +#include "options.h" // for flag_checking extern void dump_alloc_pool_statistics (void); @@ -275,7 +276,6 @@ base_pool_allocator ::initialize () m_elts_per_block = (TBlockAllocator::block_size - header_size) / size; gcc_checking_assert (m_elts_per_block != 0); -#ifdef ENABLE_CHECKING /* Increase the last used ID and use it for this pool. ID == 0 is used for free elements of pool so skip it. */ last_id++; @@ -283,7 +283,6 @@ base_pool_allocator ::initialize () last_id++; m_id = last_id; -#endif } /* Free all memory allocated for the given memory pool. */ @@ -387,10 +386,9 @@ base_pool_allocator ::allocate () block = m_virgin_free_list; header = (allocation_pool_list*) allocation_object::get_data (block); header->next = NULL; -#ifdef ENABLE_CHECKING + /* Mark the element to be free. */ ((allocation_object*) block)->id = 0; -#endif VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (header,size)); m_returned_free_list = header; m_virgin_free_list += m_elt_size; @@ -404,10 +402,8 @@ base_pool_allocator ::allocate () m_returned_free_list = header->next; m_elts_free--; -#ifdef ENABLE_CHECKING /* Set the ID for element. */ allocation_object::get_instance (header)->id = m_id; -#endif VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size)); return (void *)(header); @@ -418,26 +414,23 @@ template inline void base_pool_allocator ::remove (void *object) { - gcc_checking_assert (m_initialized); - - allocation_pool_list *header; - int size ATTRIBUTE_UNUSED; - size = m_elt_size - offsetof (allocation_object, u.data); - -#ifdef ENABLE_CHECKING - gcc_assert (object + if (flag_checking) +{ + gcc_assert (m_initialized); + gcc_assert (object /* Check if we free more than we allocated, which is Bad (TM). */ && m_elts_free < m_elts_allocated /* Check whether the PTR was allocated from POOL. */ && m_id == allocation_object::get_instance (object)->id); - memset (object, 0xaf, size); + int size = m_elt_size - offsetof (allocation_object, u.data); + memset (object, 0xaf, size); +} /* Mark the element to be free. */ allocation_object::get_instance (object)->id = 0; -#endif - header = (allocation_pool_list*) object; + allocation_pool_list *header = (allocation_pool_list*) object; header->next = m_returned_free_list; m_returned_free_list = header; VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (object, size));
Re: [PATCH 1/9] ENABLE_CHECKING refactoring
On 10/19/2015 02:13 PM, Bernd Schmidt wrote: > But for normal C conditions the patches end up using flag_checking, so > the CHECKING_P macro buys us nothing over ENABLE_CHECKING. Presumably 'if (CHECKING_P)' can be used for performance-critical parts (in this case the condition will be DCE-d) and also for those parts of the compiler which we want to decouple from 'options.h'. IIRC, Jeff's idea was get rid of 'ENABLE_CHECKING' completely and use either 'flag_checking' or 'CHECKING_P'. But I don't know what is the consensus on it (I would like to hear Jeff's and Richard's opinion). Of course it will be easy for me to adjust the patch to whatever the final decision will be. -- Regards, Mikhail Maltsev
[PATCH, committed] PR other/65800. Fix crash in gengtype's internal debug debug dump
Hi! gengtype has an option '-d' which allows to dump it's internal state. I planned to use it in order to create some kind of list of all data which GCC stores in garbage-collected memory. Unfortunately this option was broken. The attached patch fixes it. Because it only affects gengtype's internal debugging option (and is also rather small), I think it's OK to commit it without approve (as obvious). Bootstrapped and regtested on x86_64-pc-linux-gnu. -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-10-18 Mikhail Maltsev <malts...@gmail.com> PR other/65800 * gengtype.c (dump_type): Handle TYPE_UNDEFINED correctly. diff --git a/gcc/gengtype.c b/gcc/gengtype.c index 866d809..8c5c36d 100644 --- a/gcc/gengtype.c +++ b/gcc/gengtype.c @@ -4878,10 +4878,17 @@ dump_type (int indent, type_p t) { PTR *slot; + printf ("%*cType at %p: ", indent, ' ', (void *) t); + if (t->kind == TYPE_UNDEFINED) +{ + gcc_assert (t->gc_used == GC_UNUSED); + printf ("undefined.\n"); + return; +} + if (seen_types == NULL) seen_types = htab_create (100, htab_hash_pointer, htab_eq_pointer, NULL); - printf ("%*cType at %p: ", indent, ' ', (void *) t); slot = htab_find_slot (seen_types, t, INSERT); if (*slot != NULL) {
Re: [PATCH 7/9] ENABLE_CHECKING refactoring: middle-end, LTO FE
On 10/06/2015 03:59 PM, Richard Biener wrote: > On Tue, Oct 6, 2015 at 2:46 PM, Bernd Schmidt <bschm...@redhat.com> wrote: >> On 10/06/2015 01:39 AM, Mikhail Maltsev wrote: >>> >>> void verify_insn_chain (void); >>> +static inline void checking_verify_insn_chain (); >>> static void fixup_fallthru_exit_predecessor (void); >>> static int can_delete_note_p (const rtx_note *); >>> static int can_delete_label_p (const rtx_code_label *); >> >> [...] >>> >>> @@ -3990,6 +3987,16 @@ verify_insn_chain (void) >>> >>> gcc_assert (insn_cnt1 == insn_cnt2); >>> } >>> + >>> +/* Perform checks, if they were requested by corresponding flag. */ >>> + >>> +static inline void >>> +checking_verify_insn_chain () >>> +{ >>> + if (flag_checking) >>> +verify_insn_chain (); >>> +} >>> + >> >> >> There are many new such small inline functions, and I don't think they buy >> us much over just writing out the pattern where they are called. Also, just >> defined the function before its first use rather than writing a forward >> declaration. I agree, that checking_verify_insn_chain is indeed superfluous, so I replaced it with the pattern. But I think some of these functions are still useful (they make the code look cleaner, because we don't need to put those 'if (flag_checking) ...;' checks everywhere). For example, 'checking_verify_flow' is called 11 times and 'checking_verify_loop_structure' is called 9 times. Furthermore, Richard suggested [1] that we might want to split checks into several groups. It will be easier to assign different flags to them in this case. [1] https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01866.html >> >> I think the consensus has been to avoid assert (false), so this would be >> >> if (checking) >> gcc_unreachable (); >> >> but maybe we really just want to do the gcc_unreachable unconditionally. > > I'm fine with unconditional gcc_unreachable - the code is old enough now > that we shouldn't hit this anymore. > > Richard. Fixed. There were one or two other places where I used 'gcc_checking_assert (false)'. I replaced them with if (flag_checking) gcc_unreachable (); -- Regards, Mikhail Maltsev gcc/lto/ChangeLog: 2015-10-19 Mikhail Maltsev <malts...@gmail.com> * lto.c (unify_scc): Use flag_checking (lto_fixup_state): Use CHECKING_P. (do_whole_program_analysis): Use symtab_node::checking_verify_symtab_nodes. gcc/ChangeLog: 2015-10-19 Mikhail Maltsev <malts...@gmail.com> * attribs.c (check_attribute_tables): Define new function. (init_attributes): Use it. * cfgcleanup.c (try_optimize_cfg): Use flag_checking. * cfgexpand.c (expand_goto, expand_debug_expr): Likewise. (pass_expand::execute): Use checking_verify_flow_info. * cfghooks.c (verify_flow_info): Remove DEBUG_FUNCTION. * cfghooks.h (checking_verify_flow_info): Define. * cfgloop.c (verify_loop_structure): Remove DEBUG_FUNCTION. * cfgloop.h (checking_verify_loop_structure): Define. * cfgrtl.c (commit_edge_insertions): Use checking_verify_flow_info. (fixup_reorder_chain): Use checking_verify_insn_chain. (checking_verify_insn_chain): Define. (cfg_layout_finalize): Use checking_verify_insn_chain/flow_info. (rtl_flow_call_edges_add): Use flag_checking. * cgraph.c (symbol_table::create_edge): Remove conditional compilation. (cgraph_edge::redirect_call_stmt_to_callee): Likewise; use flag_checking. * cgraph.h (symtab_node::checking_verify_symtab_nodes): Define. (cgraph_node::checking_verify_cgraph_nodes): Define. * cgraphclones.c (symbol_table::materialize_all_clones): Use checking_verify_cgraph_nodes, remove conditional compilation. * cgraphunit.c (mark_functions_to_output): Use flag_checking. (cgraph_node::expand_thunk): Use checking_verify_flow_info. (symbol_table::compile): Use checking_verify_*, remove conditional compilation. * ddg.c (add_cross_iteration_register_deps): Use flag_checking. (create_ddg_all_sccs): Likewise. * df-core.c (df_finish_pass, df_analyze): Likewise. * diagnostic-core.h: Adjust to use CHECKING_P. * diagnostic.c (diagnostic_report_diagnostic): Likewise. * dominance.c (calculate_dominance_info): Use checking_verify_dominators. * dominance.h (checking_verify_dominators): Define. * dwarf2out.c (add_AT_die_ref, const_ok_for_output_1, mem_loc_descriptor, loc_list_from_tree, gen_lexical_block_die, gen_type_die_with_usage, gen_type_die, dwarf2out_decl): Use flag_che
Re: [PATCH 2/9] ENABLE_CHECKING refactoring: libcpp
On 10/12/2015 11:57 PM, Jeff Law wrote: > On 10/06/2015 06:40 AM, Bernd Schmidt wrote: >> I'm not entirely sure what to make of this series. There seem to be good >> bits in there but also some things I find questionable. I'll add some >> comments on things that occur to me. > Maybe we should start pulling out the bits that we think are ready & good and > start installing them independently. > (snip) >> Probably a good thing, but it looks like libcpp has grown its own >> variant linemap_assert; we should check whether that can be replaced. >> >> Also, the previous patch already introduces a use of gcc_assert, or at >> least a reference to it, and it's only defined here. The two >> modifications of libcpp/system.h should probably be merged into one. > Agreed. > > jeff I moved the 'libcpp/system.h' parts into the first patch of the series. As for replacing linemap_assert, I hope it can be done separately. -- Regards, Mikhail Maltsev libcpp/ChangeLog: 2015-10-19 Mikhail Maltsev <malts...@gmail.com> * include/line-map.h: Use CHECKING_P instead of ENABLE_CHECKING. * init.c: Likewise. * macro.c (struct macro_arg_token_iter, set_arg_token, macro_arg_token_iter_init, macro_arg_token_iter_forward, macro_arg_token_iter_get_token, macro_arg_token_iter_get_location, alloc_expanded_arg_mem, _cpp_backup_tokens): Likewise. >From 5bb52360b2a065015cb081b4528f2f9295c326d6 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Tue, 22 Sep 2015 02:51:31 +0300 Subject: [PATCH 2/7] libcpp - v2 --- libcpp/include/line-map.h | 2 +- libcpp/init.c | 2 +- libcpp/macro.c| 38 +++--- 3 files changed, 17 insertions(+), 25 deletions(-) diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index bc747c1..e718fc2 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -272,7 +272,7 @@ struct GTY((tag ("2"))) line_map_macro : public line_map { source_location expansion; }; -#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007) +#if CHECKING_P && (GCC_VERSION >= 2007) /* Assertion macro to be used in line-map code. */ #define linemap_assert(EXPR) \ diff --git a/libcpp/init.c b/libcpp/init.c index 2d5626f..0419e95 100644 --- a/libcpp/init.c +++ b/libcpp/init.c @@ -535,7 +535,7 @@ cpp_init_builtins (cpp_reader *pfile, int hosted) /* Sanity-checks are dependent on command-line options, so it is called as a subroutine of cpp_read_main_file (). */ -#if ENABLE_CHECKING +#if CHECKING_P static void sanity_checks (cpp_reader *); static void sanity_checks (cpp_reader *pfile) { diff --git a/libcpp/macro.c b/libcpp/macro.c index 786c21b..60a0753 100644 --- a/libcpp/macro.c +++ b/libcpp/macro.c @@ -73,7 +73,7 @@ struct macro_arg_token_iter -ftrack-macro-expansion is used this location tracks loci across macro expansion. */ const source_location *location_ptr; -#ifdef ENABLE_CHECKING +#if CHECKING_P /* The number of times the iterator went forward. This useful only when checking is enabled. */ size_t num_forwards; @@ -1310,14 +1310,11 @@ set_arg_token (macro_arg *arg, const cpp_token *token, if (loc != NULL) { -#ifdef ENABLE_CHECKING - if (kind == MACRO_ARG_TOKEN_STRINGIFIED - || !track_macro_exp_p) - /* We can't set the location of a stringified argument - token and we can't set any location if we aren't tracking - macro expansion locations. */ - abort (); -#endif + /* We can't set the location of a stringified argument + token and we can't set any location if we aren't tracking + macro expansion locations. */ + gcc_checking_assert (kind != MACRO_ARG_TOKEN_STRINGIFIED + && track_macro_exp_p); *loc = location; } } @@ -1403,7 +1400,7 @@ macro_arg_token_iter_init (macro_arg_token_iter *iter, iter->location_ptr = NULL; if (track_macro_exp_p) iter->location_ptr = get_arg_token_location (arg, kind); -#ifdef ENABLE_CHECKING +#if CHECKING_P iter->num_forwards = 0; if (track_macro_exp_p && token_ptr != NULL @@ -1428,14 +1425,14 @@ macro_arg_token_iter_forward (macro_arg_token_iter *it) it->location_ptr++; break; case MACRO_ARG_TOKEN_STRINGIFIED: -#ifdef ENABLE_CHECKING +#if CHECKING_P if (it->num_forwards > 0) abort (); #endif break; } -#ifdef ENABLE_CHECKING +#if CHECKING_P it->num_forwards++; #endif } @@ -1444,7 +1441,7 @@ macro_arg_token_iter_forward (macro_arg_token_iter *it) static const cpp_token * macro_arg_token_iter_get_token (const macro_arg_token_iter *it) { -#ifdef ENABLE_CHECKING +#if CHECKING_P if (it->kind == MACRO_ARG_TOKEN_STRINGIFIED && it->num_forwards > 0) abort (); @@ -1458
Re: [PATCH 6/9] ENABLE_CHECKING refactoring: generators
On 10/06/2015 03:56 PM, Richard Biener wrote: > The generators should simply unconditionally check (not in generated > files, of course). > And the generated code parts should use flag_checking. > > Richard. genautomata has some macros similar to tree checks, so I avoided changing them. genconditions for some reason #undef-s ENABLE_CHECKING in the generated code. I did not look at it in details, but decided to simply #define CHECKING_P to 0 for consistency. As for genextract and gengtype, I followed your recommendations, that is, used flag_checking instead of CHECKING_P in genextract, and always enable debugging functions in gengtype. -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-10-19 Mikhail Maltsev <malts...@gmail.com> * genautomata.c: Use CHECKING_P instead of ENABLE_CHECKING. * genconditions.c: Define CHECKING_P in the generated code. * genextract.c: Use flag_checking in insn_extract. * gengtype.c (main): Remove conditional compilation. * gengtype.h: Likewise. >From df421545d314192e9a4ac2868754f34590d73027 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sun, 4 Oct 2015 22:50:02 +0300 Subject: [PATCH 5/7] Generators - v2 --- gcc/genautomata.c | 2 +- gcc/genconditions.c | 2 ++ gcc/genextract.c| 9 + gcc/gengtype.c | 6 -- gcc/gengtype.h | 5 - 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/gcc/genautomata.c b/gcc/genautomata.c index 5196d68..beae5ef 100644 --- a/gcc/genautomata.c +++ b/gcc/genautomata.c @@ -879,7 +879,7 @@ struct state_ainsn_table /* Macros to access members of unions. Use only them for access to union members of declarations and regexps. */ -#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007) +#if CHECKING_P && (GCC_VERSION >= 2007) #define DECL_UNIT(d) __extension__ \ (({ __typeof (d) const _decl = (d); \ diff --git a/gcc/genconditions.c b/gcc/genconditions.c index 001e58e..7481ab4 100644 --- a/gcc/genconditions.c +++ b/gcc/genconditions.c @@ -60,6 +60,8 @@ write_header (void) \n\ /* Do not allow checking to confuse the issue. */\n\ #undef ENABLE_CHECKING\n\ +#undef CHECKING_P\n\ +#define CHECKING_P 0\n\ #undef ENABLE_TREE_CHECKING\n\ #undef ENABLE_RTL_CHECKING\n\ #undef ENABLE_RTL_FLAG_CHECKING\n\ diff --git a/gcc/genextract.c b/gcc/genextract.c index fe97701..a03ac97 100644 --- a/gcc/genextract.c +++ b/gcc/genextract.c @@ -373,10 +373,11 @@ insn_extract (rtx_insn *insn)\n{\n\ rtx pat = PATTERN (insn);\n\ int i ATTRIBUTE_UNUSED; /* only for peepholes */\n\ \n\ -#ifdef ENABLE_CHECKING\n\ - memset (ro, 0xab, sizeof (*ro) * MAX_RECOG_OPERANDS);\n\ - memset (ro_loc, 0xab, sizeof (*ro_loc) * MAX_RECOG_OPERANDS);\n\ -#endif\n"); + if (flag_checking)\n\ +{\n\ + memset (ro, 0xab, sizeof (*ro) * MAX_RECOG_OPERANDS);\n\ + memset (ro_loc, 0xab, sizeof (*ro_loc) * MAX_RECOG_OPERANDS);\n\ +}\n"); puts ("\ switch (INSN_CODE (insn))\n\ diff --git a/gcc/gengtype.c b/gcc/gengtype.c index 866d809..b7a33c6 100644 --- a/gcc/gengtype.c +++ b/gcc/gengtype.c @@ -158,9 +158,6 @@ size_t num_lang_dirs; BASE_FILES entry for each language. */ static outf_p *base_files; - - -#if ENABLE_CHECKING /* Utility debugging function, printing the various type counts within a list of types. Called through the DBGPRINT_COUNT_TYPE macro. */ void @@ -222,7 +219,6 @@ dbgprint_count_type_at (const char *fil, int lin, const char *msg, type_p t) fprintf (stderr, "@@%%@@ %d undefined types\n", nb_undefined); fprintf (stderr, "\n"); } -#endif /* ENABLE_CHECKING */ /* Scan the input file, LIST, and determine how much space we need to store strings in. Also, count the number of language directories @@ -5181,7 +5177,6 @@ main (int argc, char **argv) parse_program_options (argc, argv); -#if ENABLE_CHECKING if (do_debug) { time_t now = (time_t) 0; @@ -5189,7 +5184,6 @@ main (int argc, char **argv) DBGPRINTF ("gengtype started pid %d at %s", (int) getpid (), ctime ()); } -#endif /* ENABLE_CHECKING */ /* Parse the input list and the input files. */ DBGPRINTF ("inputlist %s", inputlist); diff --git a/gcc/gengtype.h b/gcc/gengtype.h index 83f3632..b8ce7ce 100644 --- a/gcc/gengtype.h +++ b/gcc/gengtype.h @@ -492,17 +492,12 @@ extern int do_dump; /* (-d) program argument. */ gengtype source code). Only useful to debug gengtype itself. */ extern int do_debug; /* (-D) program argument. */ -#if ENABLE_CHECKING #define DBGPRINTF(Fmt,...) do {if (do_debug)\ fprintf (stderr, "%s:%d: " Fmt "\n",\ lbasename (__FILE__),__LINE__, ##__VA_ARGS__);} while (0) void dbgprint_count_type_at (const char *, int, const char *, type_p); #define DBGPRINT_COUNT_TYPE(Msg,Ty) do {if (do_debug) \ dbgprint_c
Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators
On 10/06/2015 03:45 PM, Richard Biener wrote: > On Tue, Oct 6, 2015 at 2:41 PM, Bernd Schmidt <bschm...@redhat.com> wrote: >> On 10/06/2015 01:32 AM, Mikhail Maltsev wrote: >>> >>> gcc/ChangeLog: >>> >>> 2015-10-05 Mikhail Maltsev <malts...@gmail.com> >>> >>> * alloc-pool.h (base_pool_allocator::initialize, ::allocate, >>> ::remove): Adjust to use CHECKING_P. >> >> >> Why CHECKING_P for these and not flag_checking as elsewhere? > > Probably because they are in inline functions and thus possibly would > affect optimization. Not sure why they are inline functions in the > first place... I'd agree to using flag_checking here. > > Richard. > >> >> Bernd Adjusted. Note: I had to include 'options.h' into 'alloc-pool.h' in order to use flag_checking. -- Regards, Mikhail Maltsev 2015-10-19 Mikhail Maltsev <malts...@gmail.com> * alloc-pool.h (base_pool_allocator ::initialize, ::allocate, ::remove): Adjust to use flag_checking. diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 70105ba..a15c25e 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see #define ALLOC_POOL_H #include "memory-block.h" +#include "options.h" // for flag_checking extern void dump_alloc_pool_statistics (void); @@ -275,15 +276,16 @@ base_pool_allocator ::initialize () m_elts_per_block = (TBlockAllocator::block_size - header_size) / size; gcc_checking_assert (m_elts_per_block != 0); -#ifdef ENABLE_CHECKING - /* Increase the last used ID and use it for this pool. - ID == 0 is used for free elements of pool so skip it. */ - last_id++; - if (last_id == 0) -last_id++; + if (flag_checking) +{ + /* Increase the last used ID and use it for this pool. + ID == 0 is used for free elements of pool so skip it. */ + last_id++; + if (last_id == 0) + last_id++; - m_id = last_id; -#endif + m_id = last_id; +} } /* Free all memory allocated for the given memory pool. */ @@ -387,10 +389,10 @@ base_pool_allocator ::allocate () block = m_virgin_free_list; header = (allocation_pool_list*) allocation_object::get_data (block); header->next = NULL; -#ifdef ENABLE_CHECKING + /* Mark the element to be free. */ - ((allocation_object*) block)->id = 0; -#endif + if (flag_checking) + ((allocation_object*) block)->id = 0; VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (header,size)); m_returned_free_list = header; m_virgin_free_list += m_elt_size; @@ -404,10 +406,9 @@ base_pool_allocator ::allocate () m_returned_free_list = header->next; m_elts_free--; -#ifdef ENABLE_CHECKING /* Set the ID for element. */ - allocation_object::get_instance (header)->id = m_id; -#endif + if (flag_checking) +allocation_object::get_instance (header)->id = m_id; VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size)); return (void *)(header); @@ -424,18 +425,18 @@ base_pool_allocator ::remove (void *object) int size ATTRIBUTE_UNUSED; size = m_elt_size - offsetof (allocation_object, u.data); -#ifdef ENABLE_CHECKING - gcc_assert (object + gcc_checking_assert (object /* Check if we free more than we allocated, which is Bad (TM). */ && m_elts_free < m_elts_allocated /* Check whether the PTR was allocated from POOL. */ && m_id == allocation_object::get_instance (object)->id); - memset (object, 0xaf, size); - - /* Mark the element to be free. */ - allocation_object::get_instance (object)->id = 0; -#endif + if (flag_checking) +{ + memset (object, 0xaf, size); + /* Mark the element to be free. */ + allocation_object::get_instance (object)->id = 0; +} header = (allocation_pool_list*) object; header->next = m_returned_free_list;
Re: [PATCH 1/9] ENABLE_CHECKING refactoring
On 10/12/2015 11:57 PM, Jeff Law wrote: >>> -#ifdef ENABLE_CHECKING >>> +#if CHECKING_P >> >> I fail to see the point of this change. > I'm guessing (and Mikhail, please correct me if I'm wrong), but I think he's > trying to get away from ENABLE_CHECKING and instead use a macro which is > always defined to a value. Yes, exactly. Such macro is better because it can be used both for conditional compilation (if needed) and normal if-s (unlike ENABLE_CHECKING). On 10/14/2015 12:33 AM, Jeff Law wrote: >> gcc/common.opt | 5 + >> gcc/system.h| 3 +++ >> libcpp/system.h | 8 >> 3 files changed, 16 insertions(+) > I committed this prerequisite patch to the trunk. > > jeff > There is a typo here: > #ifdef ENABLE_CHECKING > #define gcc_checking_assert(EXPR) gcc_assert (EXPR) >+#define CHECKING_P 1 > #else >+/* N.B.: in release build EXPR is not evaluated. */ > #define gcc_checking_assert(EXPR) ((void)(0 && (EXPR))) >+#define CHECKING_P 1 > #endif In my original patch the second definition actually was '+#define CHECKING_P 0' Also, gcc_checking_assert in libcpp requires gcc_assert to be defined. That was missing in my original patch (and was added in patch 2/9), but I think it would be better to fix it here, as Bernd noticed (in the last paragraph of [1]). Besides, I like Richard's proposal [2] about moving CHECKING_P macros into configure.ac. [1] https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00550.html [2] https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00555.html It required minor tweaking in order to silence autotools' warnings. I attached the modified patch. OK for trunk (after bootstrap/regtest)? P.S. I am planning to post at least some of the other updated parts today, and I also hope to get in time with the whole series (before stage1 ends). -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-10-18 Mikhail Maltsev <malts...@gmail.com> * config.in: Regenerate. * configure: Regenerate. * configure.ac (CHECKING_P): Define. * system.h: Use CHECKING_P. libcpp/ChangeLog: 2015-10-18 Mikhail Maltsev <malts...@gmail.com> * config.in: Regenerate. * configure: Regenerate. * configure.ac (CHECKING_P): Define. * system.h (fancy_abort): Declare. (abort): Define. (gcc_assert): Define. Use CHECKING_P. diff --git a/gcc/config.in b/gcc/config.in index 093478c..48d7e64 100644 --- a/gcc/config.in +++ b/gcc/config.in @@ -30,6 +30,13 @@ #endif +/* Define to 1 if you want more run-time sanity checks. This one gets a grab + bag of miscellaneous but relatively cheap checks. */ +#ifndef USED_FOR_TARGET +#undef CHECKING_P +#endif + + /* Define 0/1 to force the choice for exception handling model. */ #ifndef USED_FOR_TARGET #undef CONFIG_SJLJ_EXCEPTIONS diff --git a/gcc/configure b/gcc/configure index 6b160ae..3122499 100755 --- a/gcc/configure +++ b/gcc/configure @@ -7096,7 +7096,12 @@ if test x$ac_checking != x ; then $as_echo "#define ENABLE_CHECKING 1" >>confdefs.h + $as_echo "#define CHECKING_P 1" >>confdefs.h + nocommon_flag=-fno-common +else + $as_echo "#define CHECKING_P 0" >>confdefs.h + fi if test x$ac_df_checking != x ; then @@ -18385,7 +18390,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 18388 "configure" +#line 18393 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -18491,7 +18496,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 18494 "configure" +#line 18499 "configure" #include "confdefs.h" #if HAVE_DLFCN_H diff --git a/gcc/configure.ac b/gcc/configure.ac index be721e6..a30bb3b 100644 --- a/gcc/configure.ac +++ b/gcc/configure.ac @@ -569,7 +569,12 @@ if test x$ac_checking != x ; then AC_DEFINE(ENABLE_CHECKING, 1, [Define if you want more run-time sanity checks. This one gets a grab bag of miscellaneous but relatively cheap checks.]) + AC_DEFINE(CHECKING_P, 1, +[Define to 1 if you want more run-time sanity checks. This one gets a grab + bag of miscellaneous but relatively cheap checks.]) nocommon_flag=-fno-common +else + AC_DEFINE(CHECKING_P, 0) fi AC_SUBST(nocommon_flag) if test x$ac_df_checking != x ; then diff --git a/gcc/system.h b/gcc/system.h index 61790d7..f9c7e2a 100644 --- a/gcc/system.h +++ b/gcc/system.h @@ -714,13 +714,11 @@ extern void fancy_abort (const char *, int, const char *) ATTRIBUTE_NORETURN; #define gcc_assert(EXPR) ((void)(0 && (EXPR))) #endif -#ifdef ENABLE_CHECKING +#if CHECKING_P #define gcc_checking_assert(EXPR) gcc_assert (EXPR) -#define CHECKING_P 1 #else /* N.B.: in release build
Re: [PATCH][C++] Fix PR67333
On 10/06/2015 04:46 PM, Jason Merrill wrote: > Hi, sorry for the slow response. Please feel free to ping once a week. > > On 08/27/2015 02:27 PM, Mikhail Maltsev wrote: >> + if (TREE_THIS_VOLATILE (t) && (!DECL_P (t) || want_rval)) > > Why the !DECL_P check? Pulling the value out of a volatile INDIRECT_REF is > just > as problematic as from a variable. Hmm... my intent was to check that we are not dealing with an expression, i.e. that should have been 'EXPR_P' rather than '!DECL_P'. I changed the condition to 'TREE_THIS_VOLATILE (t) && (EXPR_P (t) || want_rval)' and also added one more test (the one with 'test_ref' function). The updated patch passes bootstrap & regtest on x86_64-pc-linux-gnu. But now I wonder, if the combination 'TREE_THIS_VOLATILE (t) && EXPR_P (t) && !want_rval' is possible at all and should it be rejected? -- Regards, Mikhail Maltsev diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index ec9710c..b23d52a 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -4006,7 +4006,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, return false; if (t == NULL_TREE) return true; - if (TREE_THIS_VOLATILE (t)) + if (TREE_THIS_VOLATILE (t) && (EXPR_P (t) || want_rval)) { if (flags & tf_error) error ("expression %qE has side-effects", t); diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-67333.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-67333.C new file mode 100644 index 000..885ece6 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-67333.C @@ -0,0 +1,29 @@ +// PR c++/67333 +// { dg-do compile { target c++11 } } + +template +struct integral_constant +{ + static constexpr int value = N; +}; + +template +constexpr int lengthof (const volatile T (&)[S]) +{ + return S; +} + +template +constexpr int valueof (const volatile T ()[S]) // { dg-error "has side-effects" } +{ + return s[0]; +} + +int main () +{ + volatile int meow[4] {}; + integral_constant::value; // OK + integral_constant::value; // { dg-error "in a constant expression" } + return 0; +} + diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-67333.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-67333.C new file mode 100644 index 000..333047a --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-67333.C @@ -0,0 +1,36 @@ +// PR c++/67333 +// { dg-do compile { target c++14 } } + +template +struct integral_constant +{ + static constexpr int value = N; +}; + +constexpr int decl (int x) +{ + volatile int v = x; + return x; +} + +constexpr int use (int x) +{ + volatile int v = x; + return v; +} // { dg-error "has side-effects" } + +constexpr int test_ref (volatile int ) +{ + volatile int _ref = x; + volatile int *vol_ptr = + volatile int _deref = *vol_ptr; + return 0; +} + +int main() +{ + volatile int x = 0; + constexpr int t = test_ref (x); + integral_constant::value; // OK + integral_constant::value; // { dg-error "in a constant expression" } +}
Re: [PATCH 7/9] ENABLE_CHECKING refactoring: middle-end, LTO FE
gcc/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * attribs.c (check_attribute_tables): Define new function. (init_attributes): Use it. * cfgcleanup.c (try_optimize_cfg): Use flag_checking. * cfgexpand.c (expand_goto, expand_debug_expr): Likewise. (pass_expand::execute): Use checking_verify_flow_info. * cfghooks.c (verify_flow_info): Remove DEBUG_FUNCTION. * cfghooks.h (checking_verify_flow_info): Define. * cfgloop.c (verify_loop_structure): Remove DEBUG_FUNCTION. * cfgloop.h (checking_verify_loop_structure): Define. * cfgrtl.c (commit_edge_insertions): Use checking_verify_flow_info. (fixup_reorder_chain): Use checking_verify_insn_chain. (checking_verify_insn_chain): Define. (cfg_layout_finalize): Use checking_verify_insn_chain/flow_info. (rtl_flow_call_edges_add): Use flag_checking. * cgraph.c (symbol_table::create_edge): Remove conditional compilation. (cgraph_edge::redirect_call_stmt_to_callee): Likewise; use flag_checking. * cgraph.h (symtab_node::checking_verify_symtab_nodes): Define. (cgraph_node::checking_verify_cgraph_nodes): Define. * cgraphclones.c (symbol_table::materialize_all_clones): Use checking_verify_cgraph_nodes, remove conditional compilation. * cgraphunit.c (mark_functions_to_output): Use flag_checking. (cgraph_node::expand_thunk): Use checking_verify_flow_info. (symbol_table::compile): Use checking_verify_*, remove conditional compilation. * ddg.c (add_cross_iteration_register_deps): Use flag_checking. (create_ddg_all_sccs): Likewise. * df-core.c (df_finish_pass, df_analyze): Likewise. * diagnostic-core.h: Adjust to use CHECKING_P. * diagnostic.c (diagnostic_report_diagnostic): Likewise. * dominance.c (calculate_dominance_info): Use checking_verify_dominators. * dominance.h (checking_verify_dominators): Define. * dwarf2out.c (add_AT_die_ref, const_ok_for_output_1, mem_loc_descriptor, loc_list_from_tree, gen_lexical_block_die, gen_type_die_with_usage, gen_type_die, dwarf2out_decl): Use flag_checking. * emit-rtl.c (verify_rtx_sharing, reorder_insns_nobb): Likewise. * et-forest.c: Fix comment. * except.c (duplicate_eh_regions): Use flag_checking. * fwprop.c (register_active_defs, update_df_init): Use CHECKING_P. (update_uses): Use gcc_checking_assert. (fwprop_init, fwprop_done): Use CHECKING_P. * ggc-page.c (ggc_grow): Likewise. * gimplify.c (gimplify_body): Use flag_checking. (gimplify_hasher::equal): Use CHECKING_P. * graphite-isl-ast-to-gimple.c (graphite_verify): Use checking_verify_* functions. * graphite-scop-detection.c (canonicalize_loop_closed_ssa_form): Likewise. * graphite-sese-to-poly.c (rewrite_reductions_out_of_ssa): Likewise. (rewrite_cross_bb_scalar_deps_out_of_ssa): Likewise. * hash-table.h (hash_table::find_empty_slot_for_expand): Remove conditional compilation. * ifcvt.c (if_convert): Use checking_verify_flow_info. * ipa-cp.c (ipcp_propagate_stage): Use flag_checking. * ipa-devirt.c (type_in_anonymous_namespace_p, odr_type_p, odr_types_equivalent_p): Use gcc_checking_assert. (add_type_duplicate, get_odr_type): Use flag_checking. * ipa-icf.c (sem_item_optimizer::verify_classes, sem_item_optimizer::traverse_congruence_split, sem_item_optimizer::do_congruence_step_for_index): Use flag_checking. * ipa-inline-analysis.c (compute_inline_parameters): Likewise. * ipa-inline-transform.c (save_inline_function_body): Likewise. * ipa-inline.c (inline_small_functions): Use CHECKING_P. (early_inliner): Use flag_checking. * ipa-inline.h (estimate_edge_growth): Remove conditional compilation. * ipa-visibility.c (function_and_variable_visibility): Use flag_checking. * ipa.c (symbol_table::remove_unreachable_nodes): Likewise. (ipa_single_use): Use gcc_checking_assert. * ira-int.h: Use CHECKING_P. * ira.c (ira): Use flag_checking. * loop-doloop.c (doloop_optimize_loops): Use checking_verify_loop_structure. * loop-init.c (loop_optimizer_init, fix_loop_structure): Likewise. * loop-invariant.c (move_loop_invariants): Use checking_verify_flow_info. * lra-assigns.c (lra_assign): Use CHECKING_P. * lra-constraints.c (lra_constraints): Use flag_checking. * lra-eliminations.c (lra_eliminate): Likewise. * lra-int.h (struct lra_reg): Use CHECKING_P. * lra-lives.c (check_pseudos_live_through_calls, lra_create_live_ranges_1): Use CHECKING_P. * lra-remat.c (create_remat_bb_data): Adjust to use gcc_checking_
[PATCH 8/9] ENABLE_CHECKING refactoring: target-specific parts
gcc/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * config/alpha/alpha.c (alpha_function_arg): Use gcc_checking_assert. * config/arm/arm.c (arm_unwind_emit_sequence): Adjust to use CHECKING_P. * config/bfin/bfin.c (hwloop_optimize): Likewise. * config/i386/i386.c (ix86_print_operand_address, output_387_binary_op): Likewise. * config/ia64/ia64.c (ia64_sched_init, bundling): Likewise. * config/m68k/m68k.c (m68k_sched_md_init_global): Likewise. * config/rs6000/rs6000.c (htm_expand_builtin, rs6000_emit_prologue): Likewise. * config/rs6000/rs6000.h: Likewise. * config/visium/visium.c (visium_setup_incoming_varargs): Likewise. >From 4e7a33c81b2435765b661c189171f2ce9a65c5ac Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sun, 4 Oct 2015 22:51:03 +0300 Subject: [PATCH 8/9] Target-related parts --- gcc/config/alpha/alpha.c | 4 +--- gcc/config/arm/arm.c | 2 +- gcc/config/bfin/bfin.c | 4 +--- gcc/config/i386/i386.c | 9 - gcc/config/ia64/ia64.c | 20 +--- gcc/config/m68k/m68k.c | 2 +- gcc/config/rs6000/rs6000.c | 14 ++ gcc/config/rs6000/rs6000.h | 2 +- gcc/config/visium/visium.c | 2 +- 9 files changed, 25 insertions(+), 34 deletions(-) diff --git a/gcc/config/alpha/alpha.c b/gcc/config/alpha/alpha.c index eb2ae5f..b5e0bc4 100644 --- a/gcc/config/alpha/alpha.c +++ b/gcc/config/alpha/alpha.c @@ -5574,11 +5574,9 @@ alpha_function_arg (cumulative_args_t cum_v, machine_mode mode, basereg = 16; else { -#ifdef ENABLE_CHECKING /* With alpha_split_complex_arg, we shouldn't see any raw complex values here. */ - gcc_assert (!COMPLEX_MODE_P (mode)); -#endif + gcc_checking_assert (!COMPLEX_MODE_P (mode)); /* Set up defaults for FP operands passed in FP registers, and integral operands passed in integer registers. */ diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 02f5dc3..73210bc 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -26854,7 +26854,7 @@ arm_unwind_emit_sequence (FILE * asm_out_file, rtx p) else asm_fprintf (asm_out_file, "%r", reg); -#ifdef ENABLE_CHECKING +#if CHECKING_P /* Check that the addresses are consecutive. */ e = XEXP (SET_DEST (e), 0); if (GET_CODE (e) == PLUS) diff --git a/gcc/config/bfin/bfin.c b/gcc/config/bfin/bfin.c index a131053..e53fe6d 100644 --- a/gcc/config/bfin/bfin.c +++ b/gcc/config/bfin/bfin.c @@ -3811,8 +3811,7 @@ hwloop_optimize (hwloop_info loop) edge e; edge_iterator ei; -#ifdef ENABLE_CHECKING - if (loop->head != loop->incoming_dest) + if (CHECKING_P && loop->head != loop->incoming_dest) { /* We aren't entering the loop at the top. Since we've established that the loop is entered only at one point, this means there @@ -3822,7 +3821,6 @@ hwloop_optimize (hwloop_info loop) FOR_EACH_EDGE (e, ei, loop->head->preds) gcc_assert (!(e->flags & EDGE_FALLTHRU)); } -#endif emit_insn_before (seq, BB_HEAD (loop->head)); seq = emit_label_before (gen_label_rtx (), seq); diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 9c4cfbd..2e98bb1 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -17209,7 +17209,7 @@ ix86_print_operand_address (FILE *file, rtx addr) /* Print SImode register names to force addr32 prefix. */ if (SImode_address_operand (addr, VOIDmode)) { -#ifdef ENABLE_CHECKING +#if CHECKING_P gcc_assert (TARGET_64BIT); switch (GET_CODE (addr)) { @@ -17481,10 +17481,10 @@ output_387_binary_op (rtx insn, rtx *operands) const char *ssep; int is_sse = SSE_REG_P (operands[0]) || SSE_REG_P (operands[1]) || SSE_REG_P (operands[2]); -#ifdef ENABLE_CHECKING /* Even if we do not want to check the inputs, this documents input constraints. Which helps in understanding the following code. */ - if (STACK_REG_P (operands[0]) + if (CHECKING_P + && STACK_REG_P (operands[0]) && ((REG_P (operands[1]) && REGNO (operands[0]) == REGNO (operands[1]) && (STACK_REG_P (operands[2]) || MEM_P (operands[2]))) @@ -17494,8 +17494,7 @@ output_387_binary_op (rtx insn, rtx *operands) && (STACK_TOP_P (operands[1]) || STACK_TOP_P (operands[2]))) ; /* ok */ else -gcc_assert (is_sse); -#endif +gcc_checking_assert (is_sse); switch (GET_CODE (operands[3])) { diff --git a/gcc/config/ia64/ia64.c b/gcc/config/ia64/ia64.c index 779fc58..1f40ba2 100644 --- a/gcc/config/ia64/ia64.c +++ b/gcc/config/ia64/ia64.c @@ -6145,7 +6145,7 @@ struct reg_write_state /* Cumulative info for the current instruction group. */ struct reg_write_state rws_sum[NUM_REGS]; -#ifdef ENABLE_CHECKING +#if CHECKING_P /* Bitm
[PATCH][PING^2][C++] Fix PR67333
PING. On 08/27/2015 09:27 PM, Mikhail Maltsev wrote: > Hi. > This patch fixes a rejects-valid bug related to volatile-qualified arguments > of > constexpr functions. This is essentially a one-line change in constexpr.c. > Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? > > gcc/cp/ChangeLog: > > 2015-08-25 Mikhail Maltsev <malts...@gmail.com> > > PR c++/67333 > * constexpr.c (potential_constant_expression_1): Do not reject > volatile-qualified values, if there is no lvalue-to-rvalue conversion. > > gcc/testsuite/ChangeLog: > > 2015-08-25 Mikhail Maltsev <malts...@gmail.com> > > PR c++/67333 > * g++.dg/cpp0x/constexpr-67333.C: New test. > * g++.dg/cpp1y/constexpr-67333.C: New test. > -- Regards, Mikhail Maltsev
[PATCH 3/9] ENABLE_CHECKING refactoring: Java and Ada
gcc/java/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * decl.c (java_mark_decl_local): Use flag_checking instead of ENABLE_CHECKING. gcc/ada/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * gcc-interface/decl.c (gnat_to_gnu_entity): Use gcc_checking_assert. * gcc-interface/trans.c (assoc_to_constructor): Use flag_checking. * gcc-interface/utils.c (relate_alias_sets): Likewise. * gcc-interface/utils2.c (build_binary_op, build_unary_op): Use gcc_checking_assert >From e8de7d2dc24cff85b6c1e44157dad23c85e435e1 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sun, 20 Sep 2015 05:06:01 +0300 Subject: [PATCH 3/9] Ada and Java FE's --- gcc/ada/gcc-interface/decl.c | 4 +--- gcc/ada/gcc-interface/trans.c | 11 ++- gcc/ada/gcc-interface/utils.c | 4 +--- gcc/ada/gcc-interface/utils2.c | 20 gcc/java/decl.c| 4 +--- 5 files changed, 17 insertions(+), 26 deletions(-) diff --git a/gcc/ada/gcc-interface/decl.c b/gcc/ada/gcc-interface/decl.c index ca36ce5..3922bb8 100644 --- a/gcc/ada/gcc-interface/decl.c +++ b/gcc/ada/gcc-interface/decl.c @@ -2710,10 +2710,8 @@ gnat_to_gnu_entity (Entity_Id gnat_entity, tree gnu_expr, int definition) TYPE_HAS_ACTUAL_BOUNDS_P (gnu_inner) = 1; -#ifdef ENABLE_CHECKING /* Check for other cases of overloading. */ - gcc_assert (!TYPE_ACTUAL_BOUNDS (gnu_inner)); -#endif + gcc_checking_assert (!TYPE_ACTUAL_BOUNDS (gnu_inner)); } for (gnat_index = First_Index (gnat_entity); diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c index 3252ea2..c5f560f 100644 --- a/gcc/ada/gcc-interface/trans.c +++ b/gcc/ada/gcc-interface/trans.c @@ -9411,11 +9411,12 @@ assoc_to_constructor (Entity_Id gnat_entity, Node_Id gnat_assoc, tree gnu_type) gnu_result = extract_values (gnu_list, gnu_type); -#ifdef ENABLE_CHECKING - /* Verify that every entry in GNU_LIST was used. */ - for (; gnu_list; gnu_list = TREE_CHAIN (gnu_list)) -gcc_assert (TREE_ADDRESSABLE (gnu_list)); -#endif + if (flag_checking) +{ + /* Verify that every entry in GNU_LIST was used. */ + for (; gnu_list; gnu_list = TREE_CHAIN (gnu_list)) + gcc_assert (TREE_ADDRESSABLE (gnu_list)); +} return gnu_result; } diff --git a/gcc/ada/gcc-interface/utils.c b/gcc/ada/gcc-interface/utils.c index 0f3087d..7906495 100644 --- a/gcc/ada/gcc-interface/utils.c +++ b/gcc/ada/gcc-interface/utils.c @@ -1499,9 +1499,7 @@ relate_alias_sets (tree gnu_new_type, tree gnu_old_type, enum alias_set_op op) /* The alias set shouldn't be copied between array types with different aliasing settings because this can break the aliasing relationship between the array type and its element type. */ -#ifndef ENABLE_CHECKING - if (flag_strict_aliasing) -#endif + if (flag_checking || flag_strict_aliasing) gcc_assert (!(TREE_CODE (gnu_new_type) == ARRAY_TYPE && TREE_CODE (gnu_old_type) == ARRAY_TYPE && TYPE_NONALIASED_COMPONENT (gnu_new_type) diff --git a/gcc/ada/gcc-interface/utils2.c b/gcc/ada/gcc-interface/utils2.c index 70737a9..13421b4 100644 --- a/gcc/ada/gcc-interface/utils2.c +++ b/gcc/ada/gcc-interface/utils2.c @@ -854,9 +854,8 @@ build_binary_op (enum tree_code op_code, tree result_type, { case INIT_EXPR: case MODIFY_EXPR: -#ifdef ENABLE_CHECKING - gcc_assert (result_type == NULL_TREE); -#endif + gcc_checking_assert (result_type == NULL_TREE); + /* If there were integral or pointer conversions on the LHS, remove them; we'll be putting them back below if needed. Likewise for conversions between array and record types, except for justified @@ -1039,9 +1038,8 @@ build_binary_op (enum tree_code op_code, tree result_type, case TRUTH_AND_EXPR: case TRUTH_OR_EXPR: case TRUTH_XOR_EXPR: -#ifdef ENABLE_CHECKING - gcc_assert (TREE_CODE (get_base_type (result_type)) == BOOLEAN_TYPE); -#endif + gcc_checking_assert (TREE_CODE ( +get_base_type (result_type)) == BOOLEAN_TYPE); operation_type = left_base_type; left_operand = convert (operation_type, left_operand); right_operand = convert (operation_type, right_operand); @@ -1053,9 +1051,8 @@ build_binary_op (enum tree_code op_code, tree result_type, case LT_EXPR: case EQ_EXPR: case NE_EXPR: -#ifdef ENABLE_CHECKING - gcc_assert (TREE_CODE (get_base_type (result_type)) == BOOLEAN_TYPE); -#endif + gcc_checking_assert (TREE_CODE ( +get_base_type (result_type)) == BOOLEAN_TYPE); /* If either operand is a NULL_EXPR, just return a new one. */ if (TREE_CODE (left_operand) == NULL_EXPR) return build2 (op_code, result_type, @@ -1335,9 +1332,8 @@ build_unary_op (enum tree_code op_code, tree result_type, tree operand) break; case TRUTH_NOT_EXPR: -#if
Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators
gcc/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * alloc-pool.h (base_pool_allocator::initialize, ::allocate, ::remove): Adjust to use CHECKING_P. >From ed727b2279dd36e2fbf1ab6956270cbd487b1a01 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sun, 4 Oct 2015 22:50:40 +0300 Subject: [PATCH 5/9] Allocators --- gcc/alloc-pool.h | 45 ++--- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 70105ba..8477ee4 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -275,15 +275,16 @@ base_pool_allocator ::initialize () m_elts_per_block = (TBlockAllocator::block_size - header_size) / size; gcc_checking_assert (m_elts_per_block != 0); -#ifdef ENABLE_CHECKING - /* Increase the last used ID and use it for this pool. - ID == 0 is used for free elements of pool so skip it. */ - last_id++; - if (last_id == 0) -last_id++; - - m_id = last_id; -#endif + if (CHECKING_P) +{ + /* Increase the last used ID and use it for this pool. + ID == 0 is used for free elements of pool so skip it. */ + last_id++; + if (last_id == 0) + last_id++; + + m_id = last_id; +} } /* Free all memory allocated for the given memory pool. */ @@ -387,10 +388,9 @@ base_pool_allocator ::allocate () block = m_virgin_free_list; header = (allocation_pool_list*) allocation_object::get_data (block); header->next = NULL; -#ifdef ENABLE_CHECKING - /* Mark the element to be free. */ - ((allocation_object*) block)->id = 0; -#endif + if (CHECKING_P) + /* Mark the element to be free. */ + ((allocation_object*) block)->id = 0; VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (header,size)); m_returned_free_list = header; m_virgin_free_list += m_elt_size; @@ -404,10 +404,9 @@ base_pool_allocator ::allocate () m_returned_free_list = header->next; m_elts_free--; -#ifdef ENABLE_CHECKING /* Set the ID for element. */ - allocation_object::get_instance (header)->id = m_id; -#endif + if (CHECKING_P) +allocation_object::get_instance (header)->id = m_id; VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size)); return (void *)(header); @@ -424,18 +423,18 @@ base_pool_allocator ::remove (void *object) int size ATTRIBUTE_UNUSED; size = m_elt_size - offsetof (allocation_object, u.data); -#ifdef ENABLE_CHECKING - gcc_assert (object + gcc_checking_assert (object /* Check if we free more than we allocated, which is Bad (TM). */ && m_elts_free < m_elts_allocated /* Check whether the PTR was allocated from POOL. */ && m_id == allocation_object::get_instance (object)->id); - memset (object, 0xaf, size); - - /* Mark the element to be free. */ - allocation_object::get_instance (object)->id = 0; -#endif + if (CHECKING_P) +{ + memset (object, 0xaf, size); + /* Mark the element to be free. */ + allocation_object::get_instance (object)->id = 0; +} header = (allocation_pool_list*) object; header->next = m_returned_free_list; -- 2.1.4
[PATCH 1/9] ENABLE_CHECKING refactoring
Hi! This is an updated series of patches which converts 'ENABLE_CHECKING' macro into a flag, 'flag_checking' (and 'CHECKING_P' macro in several cases). For now flag_checking is always initialized with the value of 'CHECKING_P', but later it can be turned into a proper command-line flag and probably split into several checks. I also added several function which verify internal data structures when flag_checking is enabled (e.g. checking_verify_flow_info which calls verify_flow_info). These functions make their callers look somewhat cleaner. The cases where I left 'CHECKING_P' are: 1. libcpp (turn ICE after an error into fatal error) and pretty-printers (that would require to pass flag_checking to libcpp just for this single case). 2. Code which fills memory in the pools with some predefined patterns in deallocation methods (this would add some overhead to each deallocation), though I have not measured performance impact yet. 3. Generators and generated code. 4. Target-specific code 5. 'struct lra_reg' which has an additional field in checking build 6. Likewise, 'struct moveop_static_params' in insn scheduler and 'cumulative_args_t' in target.h. 7. macro-related code in libcpp (for the same reason) 8. real.c and fwprop.c - I'll profile these and also fix to use flag_checking if there won't be any measurable overhead. There are 9 patches: 1. Add flag_checking and CHECKING_P macros 2. Use CHECKING_P in libcpp 3. Ada and Java frontends 4. Fortran frontend 5. Pool allocators 6. Generator programs 7. Most of middle-end (GIMPLE, IPA, RTL) - it can be split further, if needed. 8. Target-specific code 9. C++ frontend - in progress (I will send this part soon). Some issues related to checking builds: 1. Useless check in graphite: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67842 2. I found a test which fails only on release builds: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58583 (reopened) 3. Another one: gcc.c-torture/compile/pr52073.c which is, I guess, caused by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67816 (the backtrace is the same, at least). Each patch (when applied on top of all the previous ones) compiles in both checking and release builds. The combined patch passes bootstrap and regression tests in checking an release builds (apart from 2 issues mentioned above) on x86_64-linux. I'll also run it through config-list.mk. -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * common.opt: Add flag_checking. * system.h (CHECKING_P): Define. libcpp/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * system.h (CHECKING_P, gcc_checking_assert): Define. >From 8096ea4714b3b7a96b414a70fd0de34e5e5a707a Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sun, 20 Sep 2015 04:30:42 +0300 Subject: [PATCH 1/9] Prerequisites for ENABLE_CHECKING conversion Define CHECKING_P macros. Add flag_checking. Define gcc_checking_assert in libcpp --- gcc/common.opt | 5 + gcc/system.h| 3 +++ libcpp/system.h | 8 3 files changed, 16 insertions(+) diff --git a/gcc/common.opt b/gcc/common.opt index b0f70fb..5060208 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -46,6 +46,11 @@ int optimize_fast Variable bool in_lto_p = false +; Enable additional checks of internal state consistency, which may slow +; the compiler down. +Variable +bool flag_checking = CHECKING_P + ; 0 means straightforward implementation of complex divide acceptable. ; 1 means wide ranges of inputs must work for complex divide. ; 2 means C99-like requirements for complex multiply and divide. diff --git a/gcc/system.h b/gcc/system.h index f1694b9..dc3b96d 100644 --- a/gcc/system.h +++ b/gcc/system.h @@ -716,8 +716,11 @@ extern void fancy_abort (const char *, int, const char *) ATTRIBUTE_NORETURN; #ifdef ENABLE_CHECKING #define gcc_checking_assert(EXPR) gcc_assert (EXPR) +#define CHECKING_P 1 #else +/* N.B.: in release build EXPR is not evaluated. */ #define gcc_checking_assert(EXPR) ((void)(0 && (EXPR))) +#define CHECKING_P 0 #endif /* Use gcc_unreachable() to mark unreachable locations (like an diff --git a/libcpp/system.h b/libcpp/system.h index a2e8c26..e070bc9 100644 --- a/libcpp/system.h +++ b/libcpp/system.h @@ -391,6 +391,14 @@ extern void abort (void); #define __builtin_expect(a, b) (a) #endif +#ifdef ENABLE_CHECKING +#define gcc_checking_assert(EXPR) gcc_assert (EXPR) +#define CHECKING_P 1 +#else +#define gcc_checking_assert(EXPR) ((void)(0 && (EXPR))) +#define CHECKING_P 0 +#endif + /* Provide a fake boolean type. We make no attempt to use the C99 _Bool, as it may not be available in the bootstrap compiler, and even if it is, it is liable to be buggy. -- 2.1.4
Re: [PATCH 2/9] ENABLE_CHECKING refactoring: libcpp
libcpp/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * include/line-map.h: Fix use of ENABLE_CHECKING. * init.c: Likewise. * macro.c (struct macro_arg_token_iter, set_arg_token, macro_arg_token_iter_init, macro_arg_token_iter_forward, macro_arg_token_iter_get_token, macro_arg_token_iter_get_location, alloc_expanded_arg_mem, _cpp_backup_tokens): Likewise. * system.h (gcc_assert): Define. >From e7e3856c2c748cf0c7ed35ed843ec4ce516c9b4f Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Tue, 22 Sep 2015 02:51:31 +0300 Subject: [PATCH 2/9] libcpp --- libcpp/include/line-map.h | 2 +- libcpp/init.c | 2 +- libcpp/macro.c| 38 +++--- libcpp/system.h | 17 + 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index bc747c1..e718fc2 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -272,7 +272,7 @@ struct GTY((tag ("2"))) line_map_macro : public line_map { source_location expansion; }; -#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007) +#if CHECKING_P && (GCC_VERSION >= 2007) /* Assertion macro to be used in line-map code. */ #define linemap_assert(EXPR) \ diff --git a/libcpp/init.c b/libcpp/init.c index 2d5626f..0419e95 100644 --- a/libcpp/init.c +++ b/libcpp/init.c @@ -535,7 +535,7 @@ cpp_init_builtins (cpp_reader *pfile, int hosted) /* Sanity-checks are dependent on command-line options, so it is called as a subroutine of cpp_read_main_file (). */ -#if ENABLE_CHECKING +#if CHECKING_P static void sanity_checks (cpp_reader *); static void sanity_checks (cpp_reader *pfile) { diff --git a/libcpp/macro.c b/libcpp/macro.c index 786c21b..60a0753 100644 --- a/libcpp/macro.c +++ b/libcpp/macro.c @@ -73,7 +73,7 @@ struct macro_arg_token_iter -ftrack-macro-expansion is used this location tracks loci across macro expansion. */ const source_location *location_ptr; -#ifdef ENABLE_CHECKING +#if CHECKING_P /* The number of times the iterator went forward. This useful only when checking is enabled. */ size_t num_forwards; @@ -1310,14 +1310,11 @@ set_arg_token (macro_arg *arg, const cpp_token *token, if (loc != NULL) { -#ifdef ENABLE_CHECKING - if (kind == MACRO_ARG_TOKEN_STRINGIFIED - || !track_macro_exp_p) - /* We can't set the location of a stringified argument - token and we can't set any location if we aren't tracking - macro expansion locations. */ - abort (); -#endif + /* We can't set the location of a stringified argument + token and we can't set any location if we aren't tracking + macro expansion locations. */ + gcc_checking_assert (kind != MACRO_ARG_TOKEN_STRINGIFIED + && track_macro_exp_p); *loc = location; } } @@ -1403,7 +1400,7 @@ macro_arg_token_iter_init (macro_arg_token_iter *iter, iter->location_ptr = NULL; if (track_macro_exp_p) iter->location_ptr = get_arg_token_location (arg, kind); -#ifdef ENABLE_CHECKING +#if CHECKING_P iter->num_forwards = 0; if (track_macro_exp_p && token_ptr != NULL @@ -1428,14 +1425,14 @@ macro_arg_token_iter_forward (macro_arg_token_iter *it) it->location_ptr++; break; case MACRO_ARG_TOKEN_STRINGIFIED: -#ifdef ENABLE_CHECKING +#if CHECKING_P if (it->num_forwards > 0) abort (); #endif break; } -#ifdef ENABLE_CHECKING +#if CHECKING_P it->num_forwards++; #endif } @@ -1444,7 +1441,7 @@ macro_arg_token_iter_forward (macro_arg_token_iter *it) static const cpp_token * macro_arg_token_iter_get_token (const macro_arg_token_iter *it) { -#ifdef ENABLE_CHECKING +#if CHECKING_P if (it->kind == MACRO_ARG_TOKEN_STRINGIFIED && it->num_forwards > 0) abort (); @@ -1458,7 +1455,7 @@ macro_arg_token_iter_get_token (const macro_arg_token_iter *it) static source_location macro_arg_token_iter_get_location (const macro_arg_token_iter *it) { -#ifdef ENABLE_CHECKING +#if CHECKING_P if (it->kind == MACRO_ARG_TOKEN_STRINGIFIED && it->num_forwards > 0) abort (); @@ -2144,11 +2141,9 @@ tokens_buff_add_token (_cpp_buff *buffer, static void alloc_expanded_arg_mem (cpp_reader *pfile, macro_arg *arg, size_t capacity) { -#ifdef ENABLE_CHECKING - if (arg->expanded != NULL - || arg->expanded_virt_locs != NULL) -abort (); -#endif + gcc_checking_assert (arg->expanded == NULL + && arg->expanded_virt_locs == NULL); + arg->expanded = XNEWVEC (const cpp_token *, capacity); if (CPP_OPTION (pfile, track_macro_expansion)) arg->expanded_virt_locs = XNEWVEC (source_location, capacity); @@ -2709,10 +2704,7 @@ _cpp_backup_tokens (cpp_reader *pfile, unsigned int co
[PATCH 4/9] ENABLE_CHECKING refactoring: Fortran
gcc/fortran/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * trans-common.c (create_common): Adjust to use flag_checking. * trans.c (gfc_add_modify_loc): Use gcc_checking_assert. >From 8113b4d5bc943772145abdd6562d4af6093d9718 Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sat, 3 Oct 2015 10:02:21 +0300 Subject: [PATCH 4/9] Fortran --- gcc/fortran/trans-common.c | 15 +++ gcc/fortran/trans.c| 6 ++ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/gcc/fortran/trans-common.c b/gcc/fortran/trans-common.c index 7f4bfe5..ae336c1 100644 --- a/gcc/fortran/trans-common.c +++ b/gcc/fortran/trans-common.c @@ -686,14 +686,13 @@ create_common (gfc_common_head *com, segment_info *head, bool saw_equiv) TREE_STATIC (ctor) = 1; DECL_INITIAL (decl) = ctor; -#ifdef ENABLE_CHECKING - { - tree field, value; - unsigned HOST_WIDE_INT idx; - FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, field, value) - gcc_assert (TREE_CODE (field) == FIELD_DECL); - } -#endif + if (flag_checking) + { + tree field, value; + unsigned HOST_WIDE_INT idx; + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, field, value) + gcc_assert (TREE_CODE (field) == FIELD_DECL); + } } /* Build component reference for each variable. */ diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c index f30809a..4eaea53 100644 --- a/gcc/fortran/trans.c +++ b/gcc/fortran/trans.c @@ -151,7 +151,6 @@ gfc_add_modify_loc (location_t loc, stmtblock_t * pblock, tree lhs, tree rhs) { tree tmp; -#ifdef ENABLE_CHECKING tree t1, t2; t1 = TREE_TYPE (rhs); t2 = TREE_TYPE (lhs); @@ -159,9 +158,8 @@ gfc_add_modify_loc (location_t loc, stmtblock_t * pblock, tree lhs, tree rhs) for scalar assignments. We should probably have something similar for aggregates, but right now removing that check just breaks everything. */ - gcc_assert (t1 == t2 - || AGGREGATE_TYPE_P (TREE_TYPE (lhs))); -#endif + gcc_checking_assert (t1 == t2 + || AGGREGATE_TYPE_P (TREE_TYPE (lhs))); tmp = fold_build2_loc (loc, MODIFY_EXPR, void_type_node, lhs, rhs); -- 2.1.4
[PATCH 6/9] ENABLE_CHECKING refactoring: generators
gcc/ChangeLog: 2015-10-05 Mikhail Maltsev <malts...@gmail.com> * genautomata.c: Use CHECKING_P instead of ENABLE_CHECKING. * genconditions.c: Define CHECKING_P in generated code. * genextract.c: Use CHECKING_P instead of ENABLE_CHECKING. * gengtype.c (main): Likewise. * gengtype.h: Likewise. >From 88463ec3319670875dfe47926f6e55d5fd6219ba Mon Sep 17 00:00:00 2001 From: Mikhail Maltsev <malts...@gmail.com> Date: Sun, 4 Oct 2015 22:50:02 +0300 Subject: [PATCH 6/9] Generators --- gcc/genautomata.c | 2 +- gcc/genconditions.c | 2 ++ gcc/genextract.c| 2 +- gcc/gengtype.c | 8 +++- gcc/gengtype.h | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/gcc/genautomata.c b/gcc/genautomata.c index 5196d68..beae5ef 100644 --- a/gcc/genautomata.c +++ b/gcc/genautomata.c @@ -879,7 +879,7 @@ struct state_ainsn_table /* Macros to access members of unions. Use only them for access to union members of declarations and regexps. */ -#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007) +#if CHECKING_P && (GCC_VERSION >= 2007) #define DECL_UNIT(d) __extension__ \ (({ __typeof (d) const _decl = (d); \ diff --git a/gcc/genconditions.c b/gcc/genconditions.c index 001e58e..7481ab4 100644 --- a/gcc/genconditions.c +++ b/gcc/genconditions.c @@ -60,6 +60,8 @@ write_header (void) \n\ /* Do not allow checking to confuse the issue. */\n\ #undef ENABLE_CHECKING\n\ +#undef CHECKING_P\n\ +#define CHECKING_P 0\n\ #undef ENABLE_TREE_CHECKING\n\ #undef ENABLE_RTL_CHECKING\n\ #undef ENABLE_RTL_FLAG_CHECKING\n\ diff --git a/gcc/genextract.c b/gcc/genextract.c index fe97701..7f1879f 100644 --- a/gcc/genextract.c +++ b/gcc/genextract.c @@ -373,7 +373,7 @@ insn_extract (rtx_insn *insn)\n{\n\ rtx pat = PATTERN (insn);\n\ int i ATTRIBUTE_UNUSED; /* only for peepholes */\n\ \n\ -#ifdef ENABLE_CHECKING\n\ +#if CHECKING_P\n\ memset (ro, 0xab, sizeof (*ro) * MAX_RECOG_OPERANDS);\n\ memset (ro_loc, 0xab, sizeof (*ro_loc) * MAX_RECOG_OPERANDS);\n\ #endif\n"); diff --git a/gcc/gengtype.c b/gcc/gengtype.c index 866d809..777b52f 100644 --- a/gcc/gengtype.c +++ b/gcc/gengtype.c @@ -160,7 +160,7 @@ static outf_p *base_files; -#if ENABLE_CHECKING +#if CHECKING_P /* Utility debugging function, printing the various type counts within a list of types. Called through the DBGPRINT_COUNT_TYPE macro. */ void @@ -222,7 +222,7 @@ dbgprint_count_type_at (const char *fil, int lin, const char *msg, type_p t) fprintf (stderr, "@@%%@@ %d undefined types\n", nb_undefined); fprintf (stderr, "\n"); } -#endif /* ENABLE_CHECKING */ +#endif /* CHECKING_P */ /* Scan the input file, LIST, and determine how much space we need to store strings in. Also, count the number of language directories @@ -5181,15 +5181,13 @@ main (int argc, char **argv) parse_program_options (argc, argv); -#if ENABLE_CHECKING - if (do_debug) + if (CHECKING_P && do_debug) { time_t now = (time_t) 0; time (); DBGPRINTF ("gengtype started pid %d at %s", (int) getpid (), ctime ()); } -#endif /* ENABLE_CHECKING */ /* Parse the input list and the input files. */ DBGPRINTF ("inputlist %s", inputlist); diff --git a/gcc/gengtype.h b/gcc/gengtype.h index 83f3632..9c4eac3 100644 --- a/gcc/gengtype.h +++ b/gcc/gengtype.h @@ -492,7 +492,7 @@ extern int do_dump; /* (-d) program argument. */ gengtype source code). Only useful to debug gengtype itself. */ extern int do_debug; /* (-D) program argument. */ -#if ENABLE_CHECKING +#if CHECKING_P #define DBGPRINTF(Fmt,...) do {if (do_debug)\ fprintf (stderr, "%s:%d: " Fmt "\n",\ lbasename (__FILE__),__LINE__, ##__VA_ARGS__);} while (0) @@ -502,7 +502,7 @@ void dbgprint_count_type_at (const char *, int, const char *, type_p); #else #define DBGPRINTF(Fmt,...) do {/*nodbgrintf*/} while (0) #define DBGPRINT_COUNT_TYPE(Msg,Ty) do{/*nodbgprint_count_type*/}while (0) -#endif /*ENABLE_CHECKING */ +#endif /* CHECKING_P */ #define FOR_ALL_INHERITED_FIELDS(TYPE, FIELD_VAR) \ for (type_p sub = (TYPE); sub; sub = sub->u.s.base_class) \ -- 2.1.4
[PATCH][committed][PR middle-end/67649] Fix use of valgrind API
Hi all. Pool allocators use valgrind API to mark freed memory blocks as unaccessible. Shared memory block pool needs to make them accessible again before returning to other pools (otherwise valgrind will cry wolf). I checked that build passes without valgrind checks and starts fine (i.e. errors disappear during RT libraries build) with valgrind checks on x86_64-linux. Markus also helped with valgrind test on ppc64le-linux. Committed as obvious, r228033. 2015-09-23 Mikhail Maltsev <malts...@gmail.com> PR middle-end/67649 * memory-block.h (memory_block_pool::allocate): Use valgrind API to mark the block as accessible. -- Regards, Mikhail Maltsev diff --git a/gcc/memory-block.h b/gcc/memory-block.h index 1a495ea..8b1202b 100644 --- a/gcc/memory-block.h +++ b/gcc/memory-block.h @@ -57,6 +57,7 @@ memory_block_pool::allocate () void *result = instance.m_blocks; instance.m_blocks = instance.m_blocks->m_next; + VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (result, block_size)); return result; }
Re: [PATCH] Update ENABLE_CHECKING to make it usable in "if" conditions
On 09/10/2015 12:07 AM, Jeff Law wrote: >> On Mon, Aug 31, 2015 at 7:49 AM, Mikhail Maltsev <malts...@gmail.com> >>> In lra.c we have: >>> >>> #ifdef ENABLE_CHECKING >>> >>> /* Function checks RTL for correctness. If FINAL_P is true, it is >>> done at the end of LRA and the check is more rigorous. */ >>> static void >>> check_rtl (bool final_p) >>> ... >>> #ifdef ENABLED_CHECKING >>> extract_constrain_insn (insn); >>> #endif >>> ... >>> #endif /* #ifdef ENABLE_CHECKING */ >>> >>> The comment for extract_constrain_insn says: >>> /* Do uncached extract_insn, constrain_operands and complain about >>> failures. >>> This should be used when extracting a pre-existing constrained >>> instruction >>> if the caller wants to know which alternative was chosen. */ >>> >>> So, as I understand, this additional check can be useful. This patch >>> removes >>> "#ifdef ENABLED_CHECKING" (without regressions). > No strong opinions here. There's other things that would catch this > later. The check merely catches it closer to the point where things go > wrong. So I'd tend to want it to be conditional. > But please notice that the inner condition checks for "ENABLE*D*_CHECKING", so the code guarded by it does not get executed even in "checking" build. Such spelling is not used anywhere in the code (except for this place in lra.c), so for me it looks like a typo (if it is not, then it's really misleading, because it's very easy to confuse ENABLE/ENABLED_CHECKING). Changing the condition to "ENABLE_CHECKING" will make it useless because the code is already guarded by ENABLE_CHECKING, that's why I proposed to remove the "#ifdef". On 09/16/2015 10:10 PM, Jeff Law wrote: >> I'd like to see it as part of the patch, even if the initial >> implementation would be a >> >> static const int flag_checking = CHECKING_P; >> >> in flags.h so we can avoid chaging all uses twice. It requires some >> more careful >> review of what uses are changed to flag_checking and which one to >> CHECKING_P >> of course which means it would make sense to do it as a pre patch >> handling the >> obvious cases first. And then adding -fchecking "properly" wouldn't >> be too hard >> anyway. > That works for me. Mikhail, can you move things in that direction? Yes, I hope to continue working on this patch (i.e. split it into smaller pieces and convert it to use flags instead of macros) during this weekend. -- Regards, Mikhail Maltsev
Re: [PATCH] [ping] Use single shared memory block pool for all pool allocators
On 08/31/2015 02:44 PM, Richard Biener wrote: > Apart from Richards comments: > > +/* Return UNCAST_BLOCK to pool. */ > +inline void > +memory_block_pool::remove (void *uncast_block) > +{ > + block_list *block = reinterpret_cast (uncast_block); > + block->m_next = instance.m_blocks; > + instance.m_blocks = block; > +} > > you need to use placement new > > new (uncast_block) block_list; > > instead of the reinterpret_cast to avoid type-based alias issues (as you > are also inlining this function. Fixed. > > Now some bikeshedding... > > + static inline void *allocate () ATTRIBUTE_MALLOC; > + static inline void remove (void *); > > why is it called 'remove' and not 'release' or 'free'? (ah, release is > already taken) OK, let's name it 'release' and rename 'release' into 'clear_free_list'. Originally I used these names for consistency with corresponding methods of object_pool and pool_allocator. > > Also why virtual-memory.{h,cc} and not memory-block.{h,cc}? I planned to add code for allocating memory directly from the OS (i.e. write wrappers of mmap/VirtualAlloc) to these files. Of course right now memory-block makes much more sense. > > I think the patch is ok with the above correctness fix and whatever > choice you take > for the bikeshedding. Also fixing Richards review comments, of course. Fixed. Bootstrapped and regtested on x86_64-linux and built config-list.mk. Applied to trunk (r227817). -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-09-16 Mikhail Maltsev <malts...@gmail.com> * Makefile.in: Add memory-block.cc (pool_allocator::initialize): Use fixed block size. (pool_allocator::release): Use memory_block_pool. (pool_allocator::allocate): Likewise. * asan.c (asan_mem_ref_pool): Adjust to use common block size in all object pools. * cfg.c (initialize_original_copy_tables): Likewise. * cselib.c (elt_list_pool, elt_loc_list_pool, cselib_val_pool): Likewise. * df-problems.c (df_chain_alloc): Likewise. * df-scan.c (df_scan_alloc): Likewise. * dse.c (cse_store_info_pool, rtx_store_info_pool, read_info_type_pool, insn_info_type_pool, bb_info_pool, group_info_pool, deferred_change_pool): Likewise. * et-forest.c (et_nodes, et_occurrences): Likewise. * ipa-cp.c (ipcp_cst_values_pool, ipcp_sources_pool, ipcp_agg_lattice_pool): Likewise. * ipa-inline-analysis.c (edge_predicate_pool): Likewise. * ipa-profile.c (histogram_pool): Likewise. * ipa-prop.c (ipa_refdesc_pool): Likewise. * ira-build.c (live_range_pool, allocno_pool, object_pool, initiate_cost_vectors, pref_pool, copy_pool): Likewise. * ira-color.c (update_cost_record_pool): Likewise. * lra-lives.c (lra_live_range_pool): Likewise. * lra.c (lra_insn_reg_pool, lra_copy_pool): Likewise. * memory-block.cc: New file. * memory-block.h: New file. * regcprop.c (queued_debug_insn_change_pool): Use common block size. * sched-deps.c (sched_deps_init): Likewise. * sel-sched-ir.c (sched_lists_pool): Likewise. * stmt.c (expand_case, expand_sjlj_dispatch_table): Likewise. * tree-sra.c (access_pool): Likewise. * tree-ssa-math-opts.c (pass_cse_reciprocals::execute): Likewise. * tree-ssa-pre.c (pre_expr_pool, bitmap_set_pool): Likewise. * tree-ssa-reassoc.c (operand_entry_pool): Likewise. * tree-ssa-sccvn.c (allocate_vn_table): Likewise. * tree-ssa-strlen.c (strinfo_pool): Likewise. * tree-ssa-structalias.c (variable_info_pool): Likewise. * var-tracking.c (attrs_def_pool, var_pool, valvar_pool, location_chain_pool, shared_hash_pool, loc_exp_dep_pool): Likewise. gcc/c-family/ChangeLog: 2015-09-16 Mikhail Maltsev <malts...@gmail.com> * c-format.c (check_format_arg): Adjust to use common block size in all object pools. diff --git a/gcc/Makefile.in b/gcc/Makefile.in index b495bd2..254837e 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -1513,7 +1513,7 @@ OBJS = \ # Objects in libcommon.a, potentially used by all host binaries and with # no target dependencies. OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o \ - vec.o input.o version.o hash-table.o ggc-none.o + vec.o input.o version.o hash-table.o ggc-none.o memory-block.o # Objects in libcommon-target.a, used by drivers and by the core # compiler and containing target-dependent code. diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 03bde63..70105ba 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -20,6 +20,7 @@ along with GCC; see the file COPYING3. If not see #ifndef ALLOC_POOL_H #define ALLOC_POOL_H +#include "memory-block.h" extern void dump_alloc_pool_statistics (void);
[PATCH][PING][C++] Fix PR67333
Ping. On 08/27/2015 09:27 PM, Mikhail Maltsev wrote: > Hi. > This patch fixes a rejects-valid bug related to volatile-qualified arguments > of > constexpr functions. This is essentially a one-line change in constexpr.c. > Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? > > gcc/cp/ChangeLog: > > 2015-08-25 Mikhail Maltsev <malts...@gmail.com> > > PR c++/67333 > * constexpr.c (potential_constant_expression_1): Do not reject > volatile-qualified values, if there is no lvalue-to-rvalue conversion. > > gcc/testsuite/ChangeLog: > > 2015-08-25 Mikhail Maltsev <malts...@gmail.com> > > PR c++/67333 > * g++.dg/cpp0x/constexpr-67333.C: New test. > * g++.dg/cpp1y/constexpr-67333.C: New test. > -- Regards, Mikhail Maltsev
[PING^2][PATCH] Use single shared memory block pool for all pool allocators
Ping. On 08/03/2015 11:40 AM, Mikhail Maltsev wrote: > On Jul 26, 2015, at 11:50 AM, Andi Kleen <a...@firstfloor.org> wrote: >> I've been compiling gcc with tcmalloc to do a similar speedup. It would be >> interesting to compare that to your patch. > I repeated the test with TCMalloc and jemalloc. TCMalloc shows nice results, > though it required some tweaks: this allocator has a threshold block size > equal > to 32 KB, larger blocks are allocated from global heap, rather than thread > cache > (and this operation is expensive), so the original patch shows worse > performance > when used with TCMalloc. In order to fix this, I reduced the block size to 8 > KB. > Here there are 5 columns for each value: pristine version, pristine version + > TCMalloc (and the difference in parenthesis), and patched version with > TCMalloc > (difference is relative to pristine version). Likewise, for memory usage. > > 400.perlbench26.86 26.17 ( -2.57%) 26.17 ( -2.57%) user > 0.56 0.64 ( +14.29%) 0.61 ( +8.93%) sys > 27.45 26.84 ( -2.22%) 26.81 ( -2.33%) real > 401.bzip2 2.532.5 ( -1.19%) 2.48 ( -1.98%) user > 0.07 0.09 ( +28.57%)0.1 ( +42.86%) sys > 2.612.6 ( -0.38%) 2.59 ( -0.77%) real > 403.gcc 73.59 72.62 ( -1.32%) 71.72 ( -2.54%) user > 1.59 1.88 ( +18.24%) 1.88 ( +18.24%) sys > 75.27 74.58 ( -0.92%) 73.67 ( -2.13%) real > 429.mcf0.4 0.41 ( +2.50%)0.4 ( +0.00%) user > 0.03 0.05 ( +66.67%) 0.05 ( +66.67%) sys > 0.44 0.47 ( +6.82%) 0.47 ( +6.82%) real > 433.milc 3.22 3.24 ( +0.62%) 3.25 ( +0.93%) user > 0.22 0.32 ( +45.45%)0.3 ( +36.36%) sys > 3.48 3.59 ( +3.16%) 3.59 ( +3.16%) real > 444.namd 7.54 7.41 ( -1.72%) 7.37 ( -2.25%) user >0.1 0.15 ( +50.00%) 0.15 ( +50.00%) sys > 7.66 7.58 ( -1.04%) 7.54 ( -1.57%) real > 445.gobmk20.24 19.59 ( -3.21%) 19.6 ( -3.16%) user > 0.52 0.67 ( +28.85%) 0.59 ( +13.46%) sys > 20.8 20.29 ( -2.45%) 20.23 ( -2.74%) real > 450.soplex 19.08 18.47 ( -3.20%) 18.51 ( -2.99%) user > 0.87 1.11 ( +27.59%) 1.06 ( +21.84%) sys > 19.99 19.62 ( -1.85%) 19.6 ( -1.95%) real > 453.povray 42.27 41.42 ( -2.01%) 41.32 ( -2.25%) user > 2.71 3.11 ( +14.76%) 3.09 ( +14.02%) sys > 45.04 44.58 ( -1.02%) 44.47 ( -1.27%) real > 456.hmmer 7.27 7.22 ( -0.69%) 7.15 ( -1.65%) user > 0.31 0.36 ( +16.13%) 0.39 ( +25.81%) sys > 7.61 7.61 ( +0.00%) 7.57 ( -0.53%) real > 458.sjeng 3.22 3.14 ( -2.48%) 3.15 ( -2.17%) user > 0.09 0.16 ( +77.78%) 0.14 ( +55.56%) sys > 3.32 3.32 ( +0.00%)3.3 ( -0.60%) real > 462.libquantum0.86 0.87 ( +1.16%) 0.85 ( -1.16%) user > 0.05 0.08 ( +60.00%) 0.08 ( +60.00%) sys > 0.92 0.96 ( +4.35%) 0.94 ( +2.17%) real > 464.h264ref 27.62 27.27 ( -1.27%) 27.16 ( -1.67%) user > 0.63 0.73 ( +15.87%) 0.75 ( +19.05%) sys > 28.28 28.03 ( -0.88%) 27.95 ( -1.17%) real > 470.lbm 0.27 0.27 ( +0.00%) 0.27 ( +0.00%) user > 0.01 0.01 ( +0.00%) 0.01 ( +0.00%) sys > 0.29 0.29 ( +0.00%) 0.29 ( +0.00%) real > 471.omnetpp 28.29 27.63 ( -2.33%) 27.54 ( -2.65%) user >1.5 1.57 ( +4.67%) 1.62 ( +8.00%) sys > 29.84 29.25 ( -1.98%) 29.21 ( -2.11%) real > 473.astar 1.14 1.12 ( -1.75%) 1.11 ( -2.63%) user > 0.05 0.07 ( +40.00%) 0.09 ( +80.00%) sys > 1.21 1.21 ( +0.00%)1.2 ( -0.83%) real > 482.sphinx3 4.65 4.57 ( -1.72%) 4.59 ( -1.29%) user >0.20.3 ( +50.00%) 0.26 ( +30.00%) sys > 4.88 4.89 ( +0.20%) 4.88 ( +0.00%) real > 483.xalancbmk284.5 276.4 ( -2.85%) 276.48 ( -2.82%) user > 20.29 23.03 ( +13.50%) 22.82 ( +12.47%) sys > 305.19 299.79 ( -1.77%) 299.67 ( -1.81%) real > > 400.perlbench 102308kB123004kB ( +20696kB)116104kB ( +13796k
[PATCH][C++] Fix PR67333
Hi. This patch fixes a rejects-valid bug related to volatile-qualified arguments of constexpr functions. This is essentially a one-line change in constexpr.c. Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? gcc/cp/ChangeLog: 2015-08-25 Mikhail Maltsev malts...@gmail.com PR c++/67333 * constexpr.c (potential_constant_expression_1): Do not reject volatile-qualified values, if there is no lvalue-to-rvalue conversion. gcc/testsuite/ChangeLog: 2015-08-25 Mikhail Maltsev malts...@gmail.com PR c++/67333 * g++.dg/cpp0x/constexpr-67333.C: New test. * g++.dg/cpp1y/constexpr-67333.C: New test. -- Regards, Mikhail Maltsev diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 1eacb8b..f4ee727 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -4006,7 +4006,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, return false; if (t == NULL_TREE) return true; - if (TREE_THIS_VOLATILE (t)) + if (TREE_THIS_VOLATILE (t) (!DECL_P (t) || want_rval)) { if (flags tf_error) error (expression %qE has side-effects, t); diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-67333.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-67333.C new file mode 100644 index 000..885ece6 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-67333.C @@ -0,0 +1,29 @@ +// PR c++/67333 +// { dg-do compile { target c++11 } } + +template int N +struct integral_constant +{ + static constexpr int value = N; +}; + +template typename T, int S +constexpr int lengthof (const volatile T ()[S]) +{ + return S; +} + +template typename T, int S +constexpr int valueof (const volatile T (s)[S]) // { dg-error has side-effects } +{ + return s[0]; +} + +int main () +{ + volatile int meow[4] {}; + integral_constantlengthof (meow)::value; // OK + integral_constantvalueof (meow)::value; // { dg-error in a constant expression } + return 0; +} + diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-67333.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-67333.C new file mode 100644 index 000..7e3ef21 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-67333.C @@ -0,0 +1,26 @@ +// PR c++/67333 +// { dg-do compile { target c++14 } } + +template int N +struct integral_constant +{ + static constexpr int value = N; +}; + +constexpr int decl (int x) +{ + volatile int v = x; + return x; +} + +constexpr int use (int x) +{ + volatile int v = x; + return v; +} // { dg-error has side-effects } + +int main() +{ + integral_constantdecl (2)::value; // OK + integral_constantuse (2)::value; // { dg-error in a constant expression } +}
Re: [PATCH 1/2] C++-ify dominance.c
On 08/18/2015 10:00 PM, Jeff Law wrote: On 08/14/2015 10:02 PM, Mikhail Maltsev wrote: gcc/ChangeLog: 2015-08-15 Mikhail Maltsev malts...@gmail.com * dominance.c (new_zero_array): Define. (dom_info): Redefine as class with proper encapsulation. (dom_info::m_n_basic_blocks, m_reverse, m_start_block, m_end_block): Add new members. (dom_info::dom_info, ~dom_info): Define. Use new/delete for memory allocations/deallocations. Pass function as parameter (instead of using cfun). (dom_info::get_idom): Define accessor method. (dom_info::calc_dfs_tree_nonrec, calc_dfs_tree, compress, eval, link_roots, calc_idoms): Redefine as class members. Do not use cfun. (calculate_dominance_info): Adjust to use dom_info class. (verify_dominators): Likewise. OK for the trunk. Thanks, Jeff Committed as r227093. -- Regards, Mikhail Maltsev
[PATCH] [ping] Use single shared memory block pool for all pool allocators
Hi, all. I'm pinging this patch: https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00030.html And adding some more changes to it (they are not complete yet - so they are more like an RFC). In this patch I also try to make obstacks use the same block pool as object pools. This seems rather easy to implement because obstacks already have obstack_chunk_alloc/obstack_chunk_free callbacks, and they are easily replaced. I also change the default chunk size to memory_block_pool::block_size. This still works for object sizes greater than memory_block_pool::block_size: in this case I simply call xmalloc to allocate the requested chunk, and the deallocation function uses obstack chunk header to determine chunk's size (and depending on it calls either free or memory_block_pool::remove). The result is visible on the profile. For example, for tramp3d build _obstack_begin is the top caller of xmalloc (it constitutes 107 ms into xmalloc self time, which is 375 ms - according to one of my runs). After applying the patch it is gone from profile. This patch adds new files virtual-memory.h and virtual-memory.cc, which currently contain memory_block_pool class and related obstack functions (I plan to factor out part of ggc-page.c into this file in order to use mmap/VirtualAlloc directly, hence the name virtual-memory). Currently this file is linked into libcommon, and this seems somewhat wrong to me, because the driver and other command line tools don't use all memory management machinery of the compiler proper. But obstacks are used by diagnostics.c and this file is already in libcommon, so there is probably no easy way to make it use xmalloc instead of memory_block_pool::allocate. A possible solution is to create an additional file with definitions of mempool_obstack_chunk_alloc/free and use it for generators and drivers (or somehow make mempool_obstack_chunk_alloc alias to malloc). Do you have any suggestions, how this could be done in a better way? Another problem is headers. I included virtual-memory.h into coretypes.h, because it defines a macro gcc_obstack_init, which now uses functions from virtual-memory.h. Alternatively I could just declare those functions in coretypes.h. Would that be better? -- Regards, Mikhail Maltsev diff --git a/gcc/Makefile.in b/gcc/Makefile.in index c1cb4ce..1b4198d 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -1516,7 +1516,7 @@ OBJS = \ # Objects in libcommon.a, potentially used by all host binaries and with # no target dependencies. OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o \ - vec.o input.o version.o hash-table.o ggc-none.o + vec.o input.o version.o hash-table.o ggc-none.o virtual-memory.o # Objects in libcommon-target.a, used by drivers and by the core # compiler and containing target-dependent code. diff --git a/gcc/alloc-pool.c b/gcc/alloc-pool.c index f8c1351..7e25915 100644 --- a/gcc/alloc-pool.c +++ b/gcc/alloc-pool.c @@ -35,25 +35,3 @@ dump_alloc_pool_statistics (void) pool_allocator_usage.dump (ALLOC_POOL_ORIGIN); } - -/* Global singleton-like instance. */ -memory_block_pool memory_block_pool::instance; - -memory_block_pool::memory_block_pool () : m_blocks (NULL) {} - -memory_block_pool::~memory_block_pool () -{ - release (); -} - -/* Free all memory allocated by memory_block_pool. */ -void -memory_block_pool::release () -{ - while (m_blocks) -{ - block_list *next = m_blocks-m_next; - XDELETEVEC (m_blocks); - m_blocks = next; -} -} diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index dccc41a..eccdf0d 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -20,6 +20,7 @@ along with GCC; see the file COPYING3. If not see #ifndef ALLOC_POOL_H #define ALLOC_POOL_H +#include virtual-memory.h extern void dump_alloc_pool_statistics (void); @@ -95,55 +96,6 @@ struct pool_usage: public mem_usage extern mem_alloc_descriptionpool_usage pool_allocator_usage; -/* Shared pool which allows other memory pools to reuse each others' allocated - memory blocks instead of calling free/malloc again. */ -class memory_block_pool -{ -public: - /* Blocks have fixed size. This is necessary for sharing. */ - static const size_t block_size = 64 * 1024; - - memory_block_pool (); - ~memory_block_pool (); - - static inline void *allocate () ATTRIBUTE_MALLOC; - static inline void remove (void *); - void release (); - -private: - /* memory_block_pool singleton instance, defined in alloc-pool.c. */ - static memory_block_pool instance; - - struct block_list - { -block_list *m_next; - }; - - /* Free list. */ - block_list *m_blocks; -}; - -/* Allocate single block. Reuse previously returned block, if possible. */ -inline void * -memory_block_pool::allocate () -{ - if (instance.m_blocks == NULL) -return XNEWVEC (char, block_size); - - void *result = instance.m_blocks; - instance.m_blocks = instance.m_blocks-m_next; - return result; -} - -/* Return UNCAST_BLOCK to pool. */ -inline void
Re: [PATCH 2/2] Get rid of global state accesses in dominance.c
On 08/14/2015 11:02 AM, Richard Biener wrote: So the last time I did similar refactoring I wondered if we can somehow avoid the noise in non-IPA passes. Ideas I came up with are a) Inherit gimple/rtl pass classes from a class which is initialized with the function the pass operates on and provides methods like bool dom_info_available_p (..) { return dom_info_available_p (fn, ...); } thus wraps APIs working on a specific function. b) Do sth similar but make it work with overloads and clever (no idea what!) C++ that disables them if this_fn cannot be looked up template disable-me-if-this_fn-cannot_be_lookedup-at-instantiation-place bool dom_info_available_p (..., struct function *fn = this_fn); I attached some sketch of what this clever C++ could look like. It still requires some changes in classes which would use this new base class, but they are not very significant compared to converting pass functions into members of these classes. all of the above would of course require that passes make all their implementation be methods of their pass class. So even more refactoring. The good thing is that this can be done incrementally, i.e. changing one pass at a time. Refactoring APIs that are used from IPA and make push/pop_cfun necessary for them are another thing. (there are still plenty left I think) What I could find is: 1. ipa_analyze_node 2. some transactional memory-related stuff: ipa_tm_scan_irr_function, ipa_tm_transform_transaction, ipa_tm_transform_clone, ipa_tm_execute. 3. tree_profiling Presumably there are no more calculate_dominance_info/free_dominance_info call sites with push_cfun/pop_cfun nearby (except for passes.c, but they are not related to IPA). So now I need to figure out, what other global state (which is set up by push_cfun) can these functions use? Could you give some advice about that, in sense, what should I look for (e.g., push_cfun calls some target hooks, but I did not try to look in detail, what kind of actions are performed by it)? As for the API. I propose to, at least, remove uses of cfun from dominance.c itself, but provide helper functions which will allow to leave the callers unchanged, but make it possible to use all dominance-related API on functions other than cfun. Does such change sound reasonable to you? On 08/14/2015 11:11 PM, David Malcolm wrote: The JIT guards all access to GCC's state in a big mutex (jit_mutex, in gcc/jit/jit-playback.c). Yeah, I know, I looked through JIT documentation briefly. For example, this includes everything to do with GC and GTY, since there's implicitly a single set of GC roots and the GC code isn't thread safe. And GGC is a stop-the-world collector, which is just not designed for multithreaded environment. (sigh) As for the patch, I think reviewing this variant does not make much sense, because of Richard suggestion on how to avoid changing the callers. But still thanks for looking. I've been tackling things on an as-needed basis - for example, the recent timevar global-state removal was motivated by wanting to expose a profiling API to jit client code. By the way, speaking of timevars. I wrote about my concerns w.r.t. them, but did not get any feedback: https://gcc.gnu.org/ml/gcc/2015-07/msg00165.html Briefly speaking, I noticed that our timers introduce rather significant measurement errors and proposed some ideas about reducing them. What do you think? -- Regards, Mikhail Maltsev #include stdio.h //===- various headers - Existing GCC code ===// // // No changes required //===--===// enum cdi_direction { CDI_DOMINATORS, CDI_POSTDOMINATORS }; struct basic_block_def { }; typedef basic_block_def *basic_block; struct function { }; function *cfun; class gimple_opt_pass { public: virtual unsigned int execute(function *) = 0; }; //===- dominance.h - Refactored version ---===// // // Allow passing function as an additional parameter (with default value), so // that dominaince.c does not directly use cfun any more. //===--===// // We will need cfun to be in scope. Alternatively, we can put a 3-argument // overload of domainted_by_p in another header, where cfun is available. extern function *cfun; bool dominated_by_p(cdi_direction, basic_block, basic_block, function *fun = cfun) { (void)fun; printf(\t4-argument overload\n); return false; } //===- old_pass.c - This pass uses cfun ---===// // // We don't need to change anything here //===--===// class old_pass : public gimple_opt_pass { public: virtual unsigned int execute(function *); }; void old_pass_do_stuff() { basic_block bb1 = 0, bb2 = 0
Re: [PATCH 1/2] C++-ify dominance.c
On 08/14/2015 10:54 AM, Richard Biener wrote: Putting in m_fn looks backwards to me - it looks like we only need to remember the entry and exit BBs and the number of blocks. Fixed. In fact initializing dom_info from that would allow it to work on SESE regions as well? Can't tell for sure. We would need a way to iterate through region's basic blocks (I'm not familiar with that code, but it seems to me that they are not contiguous: build_sese_loop_nests function iterates through the region by iterating through all BBs and filtering out ones which are not in region). Also we have a mapping from some global BB index to it's number in DFS-order walk, i.e. we will either need to allocate enough space for it, or use a hash map instead of an array. Never the less, initializing start/end blocks and reverse flag in constructor makes the code look cleaner because we avoid repeating these initializations. if you are here please fix the 'extern' vs. w/o 'extern' inconsistencies as well (we prefer 'extern'). Reverted (see later). On 08/14/2015 09:20 PM, Jeff Law wrote: It looks like your patch is primarily concerned with converting all the internal stuff into a C++ style and not exposing a class to the users of dominance.h. Correct? Well, sort of. But I think this class also provides some API (though it's used internally in dominance.c): it computes some initial dominance tree and other functions either query it or update incrementally. And, as Richard said, this class could be modified to be used on SESE regions. I could argue that those kind of changes are independent of turning dom_info into a real class and if they're going to go forward, they would have to stand alone on their merits and go in independently if turning dom_info into a class (which AFIACT is the meat of this patch). Actually I thought that putting all these non-functional changes into a single patch would make the churn less (after all, it's a single commit). But I understand this rather obvious cue, that these changes are, well, unlikely to be accepted. So, the updated patch contains only the dom_info-related changes. Umm, isn't ENABLE_CHECKING defined in auto-host.h (as set up by configure?) What's the reason for this change? Is the issue that auto-host.h won't define checking at all for --disable-checking? Yes, it does not get defined even for --enable-checking=release. I think that the ENABLE_CHECKING conversion from #ifdef testing to testing for a value should probably be done separately. It also probably has higher value than this refactoring. Well, I'll try to work on that. After all, it's the most frequently used condition for conditional compilation in GCC. And removing part of #ifdef-s will probably help to avoid some errors (e.g. warnings which only appear in release builds). + + /* The function being processed. */ + function *m_fn; So presumably the idea here is to avoid explicitly hitting cfun which in theory we could recompute the dominance tree for another function. But is that really all that useful? No, I was thinking more of moving toward having a self-contained compilation context and being able to run compilation in multiple threads. I know that we are far away from this goal but never the less. BTW, do we actually have such goal or not? :) I'm a bit torn here. Richi mentioned the idea of stuffing away a pointer to cfun looked backwards to him and he'd pretty stuffing away the entry, exit # blocks and perhaps take us a step towards the ability to compute dominance on sub-graphs. The problem I see with Richi's idea now that I think about it more is keeping that information up-to-date. Ie, if we've stuffed away those pointers, what happens if (for example) a block gets deleted from the graph. What if that block happens to be the exit block we've recorded? All this information is discarded after we have the dominator tree computed. The tree itself is stored in et_node-s (which are attached to basic blocks). dom_info is not used for incremental updates. gcc/ChangeLog: 2015-08-15 Mikhail Maltsev malts...@gmail.com * dominance.c (new_zero_array): Define. (dom_info): Redefine as class with proper encapsulation. (dom_info::m_n_basic_blocks, m_reverse, m_start_block, m_end_block): Add new members. (dom_info::dom_info, ~dom_info): Define. Use new/delete for memory allocations/deallocations. Pass function as parameter (instead of using cfun). (dom_info::get_idom): Define accessor method. (dom_info::calc_dfs_tree_nonrec, calc_dfs_tree, compress, eval, link_roots, calc_idoms): Redefine as class members. Do not use cfun. (calculate_dominance_info): Adjust to use dom_info class. (verify_dominators): Likewise. -- Regards, Mikhail Maltsev diff --git a/gcc/dominance.c b/gcc/dominance.c index d8d87ca..e9b24ef 100644 --- a/gcc/dominance.c +++ b/gcc/dominance.c @@ -53,139
Re: [PATCH 2/2] replace several uses of the anon namespace with GCC_FINAL
On 08/12/2015 11:36 PM, Trevor Saunders wrote: In many places gcc puts classes in the anon namespace so the compiler can tell they do not get inheritted from to enable better devirtualization. C++ does not allow class members to have static linkage, so there is no way to tell the compiler that a member function will not be called from another translation unit except for placing it into an anonymous namespace. AFAIK this affects inliner decisions rather significantly (i.e. devirtualization is not the only reason). are they actually all that common? I think gcc is the only C++ with which I'm familiar that uses them much. LLVM is an example of another large project, which uses anonymous namespaces extensively: http://llvm.org/docs/CodingStandards.html#anonymous-namespaces BTW, I noticed that LLVM developers put small inline functions into anon. namespaces (instead of just declaring them static). I wonder, whether it is just a matter of style or there is some other, more important reason... The problem is you can't get to stuff in the anonymous namespace easily in the debugger. There was talk of fixing that, but I don't think it ever went forward on the gdb side. I think, a possible solution is to define a macro which expands to namespace { during normal build and expands to nothing in some sort of debug build (for example, when ENABLE_CHECKING is defined). -- Regards, Mikhail Maltsev
[PATCH 1/2] C++-ify dominance.c
Hi all. These two patches are refactoring of dominator-related code. The comment in dominance.c says: We work in a poor-mans object oriented fashion, and carry an instance of this structure through all our 'methods'. So, the first patch converts the mentioned structure (dom_info) into a class with proper encapsulation. It also adds a new member - m_fn (the function currently being compiled) to this structure and replaces some uses of cfun with m_fn. It also contains some fixes, related to current coding standards: move variable declarations to place of first use, replace elaborated type specifiers (i.e. struct/enum foo) by simple ones (i.e., just foo) in function prototypes. Bootstrapped and regtested on x86_64-linux. Tested build of config-list.mk. gcc/ChangeLog: 2015-08-14 Mikhail Maltsev malts...@gmail.com * (ENABLE_CHECKING): Define as 0 by default. dominance.c (new_zero_array): Define. (dom_info): Define as class instead of struct. (dom_info::dom_info, ~dom_info): Define. Use new/delete for memory allocations/deallocations. Pass function as parameter (instead of using cfun). (dom_info::get_idom): Define accessor method. (dom_info::calc_dfs_tree_nonrec, calc_dfs_tree, compress, eval, link_roots, calc_idoms): Redefine as class members. Use m_fn instead of cfun. (init_dom_info, free_dom_info): Remove (use dom_info ctor/dtor). (dom_convert_dir_to_idx): Fix prototype. (assign_dfs_numbers): Move variable declarations to their first uses. (calculate_dominance_info): Remove conditional compilation, move variables. (free_dominance_info, get_immediate_dominator, set_immediate_dominator, get_dominated_b, get_dominated_by_region, get_dominated_to_depth, redirect_immediate_dominators, nearest_common_dominator_for_set, dominated_by_p, bb_dom_dfs_in, bb_dom_dfs_out, verify_dominators, determine_dominators_for_sons, iterate_fix_dominators, first_dom_son, next_dom_son, debug_dominance_info, debug_dominance_tree_1): Adjust to use class dom_info. Move variable declarations to the place of first use. Fix prototypes (remove struct/enum). * dominance.h: Fix prototypes (remove struct/enum). -- Regards, Mikhail Maltsev diff --git a/gcc/dominance.c b/gcc/dominance.c index d8d87ca..3c4f228 100644 --- a/gcc/dominance.c +++ b/gcc/dominance.c @@ -44,6 +44,10 @@ #include timevar.h #include graphds.h +#ifndef ENABLE_CHECKING +# define ENABLE_CHECKING 0 +#endif + /* We name our nodes with integers, beginning with 1. Zero is reserved for 'undefined' or 'end of list'. The name of each node is given by the dfs number of the corresponding basic block. Please note, that we include the @@ -53,139 +57,154 @@ /* Type of Basic Block aka. TBB */ typedef unsigned int TBB; -/* We work in a poor-mans object oriented fashion, and carry an instance of - this structure through all our 'methods'. It holds various arrays - reflecting the (sub)structure of the flowgraph. Most of them are of type - TBB and are also indexed by TBB. */ +namespace { + +/* This class holds various arrays reflecting the (sub)structure of the + flowgraph. Most of them are of type TBB and are also indexed by TBB. */ -struct dom_info +class dom_info { +public: + dom_info (function *, cdi_direction); + ~dom_info (); + void calc_dfs_tree (bool); + void calc_idoms (bool); + + inline basic_block get_idom (basic_block); +private: + void calc_dfs_tree_nonrec (basic_block, bool); + void compress (TBB); + TBB eval (TBB); + void link_roots (TBB, TBB); + /* The parent of a node in the DFS tree. */ - TBB *dfs_parent; - /* For a node x key[x] is roughly the node nearest to the root from which + TBB *m_dfs_parent; + /* For a node x m_key[x] is roughly the node nearest to the root from which exists a way to x only over nodes behind x. Such a node is also called semidominator. */ - TBB *key; - /* The value in path_min[x] is the node y on the path from x to the root of - the tree x is in with the smallest key[y]. */ - TBB *path_min; - /* bucket[x] points to the first node of the set of nodes having x as key. */ - TBB *bucket; - /* And next_bucket[x] points to the next node. */ - TBB *next_bucket; - /* After the algorithm is done, dom[x] contains the immediate dominator + TBB *m_key; + /* The value in m_path_min[x] is the node y on the path from x to the root of + the tree x is in with the smallest m_key[y]. */ + TBB *m_path_min; + /* m_bucket[x] points to the first node of the set of nodes having x as + key. */ + TBB *m_bucket; + /* And m_next_bucket[x] points to the next node. */ + TBB *m_next_bucket; + /* After the algorithm is done, m_dom[x] contains the immediate dominator of x. */ - TBB *dom; + TBB *m_dom; /* The following few fields implement the structures needed
Re: [PATCH] Use single shared memory block pool for all pool allocators
( +5068kB) 79192kB ( +5468kB) 450.soplex 62076kB 67596kB ( +5520kB) 66856kB ( +4780kB) 453.povray180620kB208480kB ( +27860kB)207576kB ( +26956kB) 456.hmmer 39544kB 47380kB ( +7836kB) 46776kB ( +7232kB) 458.sjeng 40144kB 48652kB ( +8508kB) 47608kB ( +7464kB) 462.libquantum 23464kB 28576kB ( +5112kB) 28260kB ( +4796kB) 464.h264ref 708760kB738400kB ( +29640kB)734224kB ( +25464kB) 470.lbm26552kB 31684kB ( +5132kB) 31348kB ( +4796kB) 471.omnetpp 152000kB172924kB ( +20924kB)167204kB ( +15204kB) 473.astar 27036kB 31472kB ( +4436kB) 31380kB ( +4344kB) 482.sphinx333100kB 40812kB ( +7712kB) 39496kB ( +6396kB) 483.xalancbmk 368844kB393292kB ( +24448kB)393032kB ( +24188kB) jemalloc causes regression (and that is rather surprising, because my previous tests showed the opposite result, but those tests had very small workload - in fact, a single file). On 07/27/2015 12:13 PM, Richard Biener wrote: On Jul 26, 2015, at 11:50 AM, Andi Kleen a...@firstfloor.org wrote: Another useful optimization is to adjust the allocation size to be = 2MB. Then modern Linux kernels often can give you a large page, which cuts down TLB overhead. I did similar changes some time ago for the garbage collector. Unless you are running with 64k pages which I do all the time on my armv8 system. This can be a host configurable value of course. Yes, I actually mentioned that among possible enhancements. I think that code from ggc-page.c can be reused (it already implements querying page size from OS). But first of all (without looking at the patch but just reading the description) this sounds like a good idea. Maybe still allow pools to use their own backing if the object size is larger than the block size of the caching pool? Yes, I though about it, but I hesitated, whether this should be implemented in advance. I attached the updated patch. -- Regards, Mikhail Maltsev diff --git a/gcc/alloc-pool.c b/gcc/alloc-pool.c index 7e25915..f8c1351 100644 --- a/gcc/alloc-pool.c +++ b/gcc/alloc-pool.c @@ -35,3 +35,25 @@ dump_alloc_pool_statistics (void) pool_allocator_usage.dump (ALLOC_POOL_ORIGIN); } + +/* Global singleton-like instance. */ +memory_block_pool memory_block_pool::instance; + +memory_block_pool::memory_block_pool () : m_blocks (NULL) {} + +memory_block_pool::~memory_block_pool () +{ + release (); +} + +/* Free all memory allocated by memory_block_pool. */ +void +memory_block_pool::release () +{ + while (m_blocks) +{ + block_list *next = m_blocks-m_next; + XDELETEVEC (m_blocks); + m_blocks = next; +} +} diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 03bde63..dccc41a 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -95,18 +95,102 @@ struct pool_usage: public mem_usage extern mem_alloc_descriptionpool_usage pool_allocator_usage; +/* Shared pool which allows other memory pools to reuse each others' allocated + memory blocks instead of calling free/malloc again. */ +class memory_block_pool +{ +public: + /* Blocks have fixed size. This is necessary for sharing. */ + static const size_t block_size = 64 * 1024; + + memory_block_pool (); + ~memory_block_pool (); + + static inline void *allocate () ATTRIBUTE_MALLOC; + static inline void remove (void *); + void release (); + +private: + /* memory_block_pool singleton instance, defined in alloc-pool.c. */ + static memory_block_pool instance; + + struct block_list + { +block_list *m_next; + }; + + /* Free list. */ + block_list *m_blocks; +}; + +/* Allocate single block. Reuse previously returned block, if possible. */ +inline void * +memory_block_pool::allocate () +{ + if (instance.m_blocks == NULL) +return XNEWVEC (char, block_size); + + void *result = instance.m_blocks; + instance.m_blocks = instance.m_blocks-m_next; + return result; +} + +/* Return UNCAST_BLOCK to pool. */ +inline void +memory_block_pool::remove (void *uncast_block) +{ + block_list *block = reinterpret_castblock_list * (uncast_block); + block-m_next = instance.m_blocks; + instance.m_blocks = block; +} + +#if 0 +/* If a pool with custom block size is needed, one might use the following + template. An instance of this template can be used as a parameter for + instantiating base_pool_allocator template: + + typedef custom_block_allocator 128*1024 huge_block_allocator; + ... + static base_pool_allocator huge_block_allocator + value_pool (value, 16384); + + Right now it's not used anywhere in the code, and is given here as an + example). */ + +template size_t BlockSize +class custom_block_allocator +{ +public: + static const size_t block_size = BlockSize; + + static inline void * + allocate () ATTRIBUTE_MALLOC + { +return XNEWVEC (char, BlockSize); + } + + static inline void
[PATCH] Use single shared memory block pool for all pool allocators
, because obstacks can be used for unlimited object sizes (large objects can be allocated using malloc, while small blocks will be allocated from block pool). 2. Do not destroy and re-initialize the pools themselves (instead call release to free allocated blocks). 3. Find out the cause of memory usage growth in omnetpp (it seems that system time also increased because of it, though overall effect is still improvement). 4. ??? Factor out the code for virtual memory management (i.e. mmap wrapper) from ggc-page.c, reuse it for pools. 5. Move all pools inside gcc::context. Eventually this will be needed for multi-threaded compilation in GCCJIT (is it planned?). 6. Use ASAN poisoning in pool allocators. Add red zones between allocated objects when GCC is configured to use ASAN checks. gcc/ChangeLog: 2015-07-26 Mikhail Maltsev malts...@gmail.com * alloc-pool.c (memory_block_pool): New class. (pool_allocator::initialize): Use fixed block size. (pool_allocator::release): Use memory_block_pool. (pool_allocator::allocate): Likewise. * asan.c (asan_mem_ref_pool): Adjust to use common block size in all object pools. * cfg.c (initialize_original_copy_tables): Likewise. * cselib.c (elt_list_pool, elt_loc_list_pool, cselib_val_pool): Likewise. * df-problems.c (df_chain_alloc): Likewise. * df-scan.c (df_scan_alloc): Likewise. * dse.c (cse_store_info_pool, rtx_store_info_pool, read_info_type_pool, insn_info_type_pool, bb_info_pool, group_info_pool, deferred_change_pool): Likewise. * et-forest.c (et_nodes, et_occurrences): Likewise. * ipa-cp.c (ipcp_cst_values_pool, ipcp_sources_pool, ipcp_agg_lattice_pool): Likewise. * ipa-inline-analysis.c (edge_predicate_pool): Likewise. * ipa-profile.c (histogram_pool): Likewise. * ipa-prop.c (ipa_refdesc_pool): Likewise. * ira-build.c (live_range_pool, allocno_pool, object_pool, initiate_cost_vectors, pref_pool, copy_pool): Likewise. * ira-color.c (update_cost_record_pool): Likewise. * lra-lives.c (lra_live_range_pool): Likewise. * lra.c (lra_insn_reg_pool, lra_copy_pool): Likewise. * regcprop.c (queued_debug_insn_change_pool): Likewise. * sched-deps.c (sched_deps_init): Likewise. * sel-sched-ir.c (sched_lists_pool): Likewise. * stmt.c (expand_case, expand_sjlj_dispatch_table): Likewise. * tree-sra.c (access_pool): Likewise. * tree-ssa-math-opts.c (pass_cse_reciprocals::execute): Likewise. * tree-ssa-pre.c (pre_expr_pool, bitmap_set_pool): Likewise. * tree-ssa-reassoc.c (operand_entry_pool): Likewise. * tree-ssa-sccvn.c (allocate_vn_table): Likewise. * tree-ssa-strlen.c (strinfo_pool): Likewise. * tree-ssa-structalias.c (variable_info_pool): Likewise. * var-tracking.c (attrs_def_pool, var_pool, valvar_pool, location_chain_def_pool, shared_hash_def_pool, loc_exp_dep_pool): Likewise. gcc/c-family/ChangeLog: 2015-07-26 Mikhail Maltsev malts...@gmail.com * c-format.c (check_format_arg): Adjust to use common block size in all object pools. -- Regards, Mikhail Maltsev diff --git a/gcc/alloc-pool.c b/gcc/alloc-pool.c index 7e25915..29d4f66 100644 --- a/gcc/alloc-pool.c +++ b/gcc/alloc-pool.c @@ -35,3 +35,25 @@ dump_alloc_pool_statistics (void) pool_allocator_usage.dump (ALLOC_POOL_ORIGIN); } + +/* Global singleton-like instance. */ +memory_block_pool block_pool; + +memory_block_pool::memory_block_pool () : m_blocks (NULL) {} + +memory_block_pool::~memory_block_pool () +{ + release (); +} + +/* Free all memory allocated by memory_block_pool. */ +void +memory_block_pool::release () +{ + while (m_blocks) +{ + block_list *next = m_blocks-m_next; + XDELETEVEC (m_blocks); + m_blocks = next; +} +} diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 03bde63..f79d8bd 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -95,13 +95,60 @@ struct pool_usage: public mem_usage extern mem_alloc_descriptionpool_usage pool_allocator_usage; +/* Shared pool which allows other memory pools to reuse each others' allocated + memory blocks instead of calling free/malloc again. */ +class memory_block_pool +{ +public: + /* Blocks have fixed size. This is necessary for sharing. */ + static const size_t block_size = 64 * 1024; + + memory_block_pool (); + ~memory_block_pool (); + + inline void *allocate () ATTRIBUTE_MALLOC; + inline void remove (void *); + void release (); +private: + struct block_list + { +block_list *m_next; + }; + + /* Free list. */ + block_list *m_blocks; +}; + +/* Allocate single block. Reuse previously returned block, if possible. */ +inline void * +memory_block_pool::allocate () +{ + if (m_blocks == NULL) +return XNEWVEC (char, block_size); + + void *result = m_blocks; + m_blocks
Re: [PATCH] Fix partial template specialization syntax in wide-int.h
On 07/17/2015 07:46 PM, Mike Stump wrote: On Jul 17, 2015, at 2:28 AM, Mikhail Maltsev malts...@gmail.com wrote: The following code (reduced from wide-int.h) is rejected by Intel C++ Compiler (EDG-based): So, could you test this with the top of the tree compiler and file a bug report against g++ for it, if it seems to not work right. If that bug report is rejected, then I’d say file a bug report against clang and EDG. In addition to usual bootstrap+regtest, I also checked that build succeeds with GCC 4.3.6 (IIRC, this is now the minimal required version) as well as with recent GCC snapshot used as stage 0. Committed as r225993. I also filed this bugreport: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66941 I think that the warning is correct, and template should not be used here. The attached patch should fix this issue. Bootstrapped and regtested on x86_64-linux. OK for trunk? Ok. Does this need to go into the gcc-5 release branch as well? If so, ok there too. Thanks. I think there is no need for it. -- Regards, Mikhail Maltsev
[PATCH] Fix partial template specialization syntax in wide-int.h
Hi, all! The following code (reduced from wide-int.h) is rejected by Intel C++ Compiler (EDG-based): $ cat genpreds1_min.cc template typename class A; template int struct B; template typename struct C; template template int N struct C B N { templatetypename T A B N m_fn(T); }; template int N template typename T A B N C B N ::m_fn (T) { } $ /opt/intel/bin/icpc -c genpreds1_min.cc genpreds1_min.cc(22): error: incomplete type is not allowed C B N ::m_fn (T) ^ genpreds1_min.cc(22): error: template argument list must match the parameter list C B N ::m_fn (T) Clang gives the following warning: $ clang++ -c genpreds1_min.cc genpreds1_min.cc:10:1: warning: extraneous template parameter list in template specialization template I think that the warning is correct, and template should not be used here. The attached patch should fix this issue. Bootstrapped and regtested on x86_64-linux. OK for trunk? -- Regards, Mikhail Maltsev diff --git a/gcc/wide-int.h b/gcc/wide-int.h index d8f7b46..6e0275f 100644 --- a/gcc/wide-int.h +++ b/gcc/wide-int.h @@ -360,21 +360,18 @@ namespace wi inputs. Note that CONST_PRECISION and VAR_PRECISION cannot be mixed, in order to give stronger type checking. When both inputs are CONST_PRECISION, they must have the same precision. */ - template template typename T1, typename T2 struct binary_traits T1, T2, FLEXIBLE_PRECISION, FLEXIBLE_PRECISION { typedef widest_int result_type; }; - template template typename T1, typename T2 struct binary_traits T1, T2, FLEXIBLE_PRECISION, VAR_PRECISION { typedef wide_int result_type; }; - template template typename T1, typename T2 struct binary_traits T1, T2, FLEXIBLE_PRECISION, CONST_PRECISION { @@ -384,14 +381,12 @@ namespace wi int_traits T2::precision result_type; }; - template template typename T1, typename T2 struct binary_traits T1, T2, VAR_PRECISION, FLEXIBLE_PRECISION { typedef wide_int result_type; }; - template template typename T1, typename T2 struct binary_traits T1, T2, CONST_PRECISION, FLEXIBLE_PRECISION { @@ -401,7 +396,6 @@ namespace wi int_traits T1::precision result_type; }; - template template typename T1, typename T2 struct binary_traits T1, T2, CONST_PRECISION, CONST_PRECISION { @@ -412,7 +406,6 @@ namespace wi int_traits T1::precision result_type; }; - template template typename T1, typename T2 struct binary_traits T1, T2, VAR_PRECISION, VAR_PRECISION { @@ -876,7 +869,6 @@ generic_wide_int storage::dump () const namespace wi { - template template typename storage struct int_traits generic_wide_int storage : public wi::int_traits storage @@ -955,7 +947,6 @@ inline wide_int_ref_storage SE::wide_int_ref_storage (const T x, namespace wi { - template template bool SE struct int_traits wide_int_ref_storage SE { @@ -1142,7 +1133,6 @@ public: namespace wi { - template template int N struct int_traits fixed_wide_int_storage N { gcc/ChangeLog: 2015-07-17 Mikhail Maltsev malts...@gmail.com * wide-int.h (struct binary_traits): Fix partial specialization syntax. (struct int_traits): Likewise.
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
On 07/14/2015 07:38 PM, Marek Polacek wrote: Ok, in that case I think easiest would the following (I hit the same issue when writing the -Wtautological-compare patch): Thanks for taking care of this issue. -- Regards, Mikhail Maltsev
Re: [PATCH 3/7] Fix trinary op
On 07/10/2015 11:44 PM, Jeff Law wrote: OK after regression testing. jeff Bootstrapped and regtested on x86_64-unknown-linux-gnu. Applied as r225727. -- Regards, Mikhail Maltsev
Re: [PATCH 3/7] Fix trinary op
On 08.07.2015 13:55, Ian Lance Taylor wrote: I don't know of anybody who actually uses the DMGL_TYPES support. I don't know why anybody would. Ian Thanks for pointing that out. I updated the testcases, so that now they don't depend on DMGL_TYPES being used. But better still is to consider the larger context. We want the demangler to work the same on all hosts, if at all possible. d_identifier is called exactly once. Change it to take a parameter of type long. Don't worry about changing d_source_name. Fixed. Then look at the fact that d_number does not check for overflow. We should consider changing d_number to limit itself to 32-bit integers, and to return an error indication on overflow. From a quick glance I don't see any need for the demangler to support numbers larger than 32 bits. I think it's OK if we fail to demangle symbol names that are more than 2 billion characters long. OK, but I think it'll be better to fix that in a separate patch. The attached patch includes the changes mentioned above, there is also a small change: I moved the comment for CHECK_DEMANGLER macro to cp-demangle.c (it already contains a comment for other similar macros) and replaced __builtin_abort() with abort(). For some reason I thought that it might need an additional #include, but in reality libiberty (and the demangler too) already use abort(). The changelog is also attached. OK for trunk after regtest? -- Regards, Mikhail Maltsev libiberty/ChangeLog: 2015-07-10 Mikhail Maltsev malts...@gmail.com * cp-demangle.c (d_dump): Fix syntax error. (d_identifier): Adjust type of len to match d_source_name. (d_expression_1): Fix out-of-bounds access. Check code variable for NULL before dereferencing it. (d_find_pack): Do not recurse for FIXED_TYPE, DEFAULT_ARG and NUMBER. (d_print_comp_inner): Add NULL pointer check. * cp-demangle.h (d_peek_next_char): Define as inline function when CHECK_DEMANGLER is defined. (d_advance): Likewise. * testsuite/demangle-expected: Add new testcases. diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 2988b6b..8254100 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -93,7 +93,11 @@ CP_DEMANGLE_DEBUG If defined, turns on debugging mode, which prints information on stdout about the mangled string. This is not generally useful. -*/ + + CHECK_DEMANGLER + If defined, additional sanity checks will be performed. It will + cause some slowdown, but will allow to catch out-of-bound access + errors earlier. This macro is intended for testing and debugging. */ #if defined (_AIX) !defined (__GNUC__) #pragma alloca @@ -419,7 +423,7 @@ static struct demangle_component *d_source_name (struct d_info *); static long d_number (struct d_info *); -static struct demangle_component *d_identifier (struct d_info *, int); +static struct demangle_component *d_identifier (struct d_info *, long); static struct demangle_component *d_operator_name (struct d_info *); @@ -715,7 +719,7 @@ d_dump (struct demangle_component *dc, int indent) case DEMANGLE_COMPONENT_FIXED_TYPE: printf (fixed-point type, accum? %d, sat? %d\n, dc-u.s_fixed.accum, dc-u.s_fixed.sat); - d_dump (dc-u.s_fixed.length, indent + 2) + d_dump (dc-u.s_fixed.length, indent + 2); break; case DEMANGLE_COMPONENT_ARGLIST: printf (argument list\n); @@ -1656,7 +1660,7 @@ d_number_component (struct d_info *di) /* identifier ::= (unqualified source code identifier) */ static struct demangle_component * -d_identifier (struct d_info *di, int len) +d_identifier (struct d_info *di, long len) { const char *name; @@ -1677,7 +1681,7 @@ d_identifier (struct d_info *di, int len) /* Look for something which looks like a gcc encoding of an anonymous namespace, and replace it with a more user friendly name. */ - if (len = (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 + if (len = (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 memcmp (name, ANONYMOUS_NAMESPACE_PREFIX, ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0) { @@ -3166,6 +3170,8 @@ d_expression_1 (struct d_info *di) struct demangle_component *type = NULL; if (peek == 't') type = cplus_demangle_type (di); + if (!d_peek_next_char (di)) + return NULL; d_advance (di, 2); return d_make_comp (di, DEMANGLE_COMPONENT_INITIALIZER_LIST, type, d_exprlist (di, 'E')); @@ -3240,6 +3246,8 @@ d_expression_1 (struct d_info *di) struct demangle_component *left; struct demangle_component *right; + if (code == NULL) + return NULL; if (op_is_new_cast (op)) left = cplus_demangle_type (di); else @@ -3267,7 +3275,9 @@ d_expression_1 (struct d_info *di) struct demangle_component *second
Re: [PATCH 4/7] Fix int overflow
On 08.07.2015 1:48, Jeff Law wrote: I'm not questioning whether or not the test will cause a problem, but instead questioning if the test does what you expect it to do on a 32bit host. On a host where sizeof (int) == sizeof (long), that len INT_MAX test is always going to be false. Yes, but in this case the call d_identifier (di, len) is also safe, because implicit conversion from long (the type of len variable) to int (the type of d_identifier's second argument) will never cause overflow. I first wanted to change one of those types to match the other, but it turned out that they are used rather consistently: all the code that works with demangle_component.u.s_number.number uses type long (because strtol was used for string-to-integer conversion before r73788), and ints are used for lengths of various identifiers. Should I try to refactor it somehow? If you want to do overflow testing, you have to compute len in a wider type. You might consider using long long or int64_t depending on the outcome of a configure test. Falling back to a simple long if the host compiler doesn't have long long or int64_t. So, it's probably vice-versa: the test is only needed if long is wider than int. And a generic question on the testsuite -- presumably it turns on type demangling? I wanted to verify the flow through d_expression_1 was what I expected it to be and it took a while to realize that c++filt doesn't demangle types by default, thus Av32_f would demangle to Av32_f without ever getting into d_expression_1. Yes, the testsuite is based on libiberty/testsuite/test-demangle.c (OMG, there are still KR-style definitions there), and it uses (DMGL_PARAMS | DMGL_ANSI | DMGL_TYPES) by default. (snip) FWIW, demangler.com doesn't give any results for that case. It just returns DpDv1_c But _Z1fDpDv1_c makes it crash :) -- Regards, Mikhail Maltsev
[PATCH 5/7] Fix braced-init-list demangling
--- libiberty/cp-demangle.c | 2 ++ libiberty/testsuite/demangle-expected | 4 2 files changed, 6 insertions(+) diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index befa6b6..424b1c5 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -3167,6 +3167,8 @@ d_expression_1 (struct d_info *di) struct demangle_component *type = NULL; if (peek == 't') type = cplus_demangle_type (di); + if (!d_peek_next_char (di)) + return NULL; d_advance (di, 2); return d_make_comp (di, DEMANGLE_COMPONENT_INITIALIZER_LIST, type, d_exprlist (di, 'E')); diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected index 9a8d3db..2dbab14 100644 --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -4100,6 +4100,10 @@ Av32_f --format=gnu-v3 _Z111 _Z111 +# Check out-of-bounds access when decoding braced initializer list +--format=gnu-v3 +_ZDTtl +_ZDTtl # # Ada (GNAT) tests. # -- 1.8.3.1
[PATCH 1/7] Add CHECK_DEMANGLER
--- libiberty/cp-demangle.h | 33 +++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/libiberty/cp-demangle.h b/libiberty/cp-demangle.h index 6fce025..c37a91f 100644 --- a/libiberty/cp-demangle.h +++ b/libiberty/cp-demangle.h @@ -135,12 +135,41 @@ struct d_info - call d_check_char(di, '\0') Everything else is safe. */ #define d_peek_char(di) (*((di)-n)) -#define d_peek_next_char(di) ((di)-n[1]) -#define d_advance(di, i) ((di)-n += (i)) +#ifndef CHECK_DEMANGLER +# define d_peek_next_char(di) ((di)-n[1]) +# define d_advance(di, i) ((di)-n += (i)) +#endif #define d_check_char(di, c) (d_peek_char(di) == c ? ((di)-n++, 1) : 0) #define d_next_char(di) (d_peek_char(di) == '\0' ? '\0' : *((di)-n++)) #define d_str(di) ((di)-n) +/* Define CHECK_DEMANGLER to perform additional sanity checks (i.e., when + debugging the demangler). It will cause some slowdown, but will allow to + catch out-of-bound access errors earlier. + Note: CHECK_DEMANGLER is not compatible with compilers other than GCC. */ +#ifdef CHECK_DEMANGLER +static inline char +d_peek_next_char (const struct d_info *di) +{ + if (!di-n[0]) +__builtin_abort (); + return di-n[1]; +} + +static inline void +d_advance (struct d_info *di, int i) +{ + if (i 0) +__builtin_abort (); + while (i--) +{ + if (!di-n[0]) + __builtin_abort (); + di-n++; +} +} +#endif + /* Functions and arrays in cp-demangle.c which are referenced by functions in cp-demint.c. */ #ifdef IN_GLIBCPP_V3 -- 1.8.3.1
[PATCH 4/7] Fix int overflow
--- libiberty/cp-demangle.c | 3 ++- libiberty/testsuite/demangle-expected | 4 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 44a0a9b..befa6b6 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -103,6 +103,7 @@ #include config.h #endif +#include limits.h #include stdio.h #ifdef HAVE_STDLIB_H @@ -1599,7 +1600,7 @@ d_source_name (struct d_info *di) struct demangle_component *ret; len = d_number (di); - if (len = 0) + if (len = 0 || len INT_MAX) return NULL; ret = d_identifier (di, len); di-last_name = ret; diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected index 47ca8f5..9a8d3db 100644 --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -4096,6 +4096,10 @@ std::complexint::real[abi:cxx11]() const --format=gnu-v3 Av32_f Av32_f +# Check range when converting from long to int +--format=gnu-v3 +_Z111 +_Z111 # # Ada (GNAT) tests. # -- 1.8.3.1
[PATCH 0/7] Fix bugs found during demangler fuzz-testing
Hi, all! I performed some fuzz-testing of C++ demangler (libiberty/cp-demangle.c). The test revealed several bugs, which are addressed by this series of patches. Here is a brief description of them: First one adds a macro CHECK_DEMANGLER. When this macro is defined, d_peek_next_char and d_advance macros are replaced by inline functions which perform additional sanity checks and call __builtin_abort when out-of-bound access is detected. The second patch fixes a syntax error in debug dump code (it is normally disabled, unless CP_DEMANGLE_DEBUG is defined). All other parts fix some errors on invalid input. The attached files contain a cumulative patch and changelog record. Bootstrapped and regtested on x86_64-linux. Some notes: * Patch 4 adds #include limits.h to demangler (because of INT_MAX). I noticed that this file is checked by configury scripts. Do we have hosts, which do not provide this header? If so, what is the appropriate replacement for it? * Testcase _ZDTtl (fixed in patch 5) did not actually cause segfault, but it still invoked undefined behavior (read 1 byte past buffer end). * Testcase DpDv1_c (from patch 7) is now demangled as (char __vector(1))... (it used to segfault). I'm not sure, whether it is correct or should be rejected. I have some more test cases, lots of them cause infinite recursion, because of conversion operator being used as template parameter. Some are fixed in PR61321, some are not. For example, there are cases when conversion operator is used as a nested (qualified) name: _Z1fIN1CcvT_EEvv - segfault Presumably this means: templatetypename T void fC::operator T() I wonder, if it is possible in valid C++ code? Notice that the following template instantiation is demangled correctly: void fC::operator int() _Z1fIN1CcviEEvv - OK -- Regards, Mikhail Maltsev diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 2988b6b..4ca285e 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -103,6 +103,7 @@ #include config.h #endif +#include limits.h #include stdio.h #ifdef HAVE_STDLIB_H @@ -715,7 +716,7 @@ d_dump (struct demangle_component *dc, int indent) case DEMANGLE_COMPONENT_FIXED_TYPE: printf (fixed-point type, accum? %d, sat? %d\n, dc-u.s_fixed.accum, dc-u.s_fixed.sat); - d_dump (dc-u.s_fixed.length, indent + 2) + d_dump (dc-u.s_fixed.length, indent + 2); break; case DEMANGLE_COMPONENT_ARGLIST: printf (argument list\n); @@ -1599,7 +1600,7 @@ d_source_name (struct d_info *di) struct demangle_component *ret; len = d_number (di); - if (len = 0) + if (len = 0 || len INT_MAX) return NULL; ret = d_identifier (di, len); di-last_name = ret; @@ -3166,6 +3167,8 @@ d_expression_1 (struct d_info *di) struct demangle_component *type = NULL; if (peek == 't') type = cplus_demangle_type (di); + if (!d_peek_next_char (di)) + return NULL; d_advance (di, 2); return d_make_comp (di, DEMANGLE_COMPONENT_INITIALIZER_LIST, type, d_exprlist (di, 'E')); @@ -3240,6 +3243,8 @@ d_expression_1 (struct d_info *di) struct demangle_component *left; struct demangle_component *right; + if (code == NULL) + return NULL; if (op_is_new_cast (op)) left = cplus_demangle_type (di); else @@ -3267,7 +3272,9 @@ d_expression_1 (struct d_info *di) struct demangle_component *second; struct demangle_component *third; - if (!strcmp (code, qu)) + if (code == NULL) + return NULL; + else if (!strcmp (code, qu)) { /* ?: expression. */ first = d_expression_1 (di); @@ -4196,6 +4203,9 @@ d_find_pack (struct d_print_info *dpi, case DEMANGLE_COMPONENT_CHARACTER: case DEMANGLE_COMPONENT_FUNCTION_PARAM: case DEMANGLE_COMPONENT_UNNAMED_TYPE: +case DEMANGLE_COMPONENT_FIXED_TYPE: +case DEMANGLE_COMPONENT_DEFAULT_ARG: +case DEMANGLE_COMPONENT_NUMBER: return NULL; case DEMANGLE_COMPONENT_EXTENDED_OPERATOR: @@ -4431,6 +4441,11 @@ d_print_comp_inner (struct d_print_info *dpi, int options, local_name = d_right (typed_name); if (local_name-type == DEMANGLE_COMPONENT_DEFAULT_ARG) local_name = local_name-u.s_unary_num.sub; + if (local_name == NULL) + { + d_print_error (dpi); + return; + } while (local_name-type == DEMANGLE_COMPONENT_RESTRICT_THIS || local_name-type == DEMANGLE_COMPONENT_VOLATILE_THIS || local_name-type == DEMANGLE_COMPONENT_CONST_THIS diff --git a/libiberty/cp-demangle.h b/libiberty/cp-demangle.h index 6fce025..c37a91f 100644 --- a/libiberty/cp-demangle.h +++ b/libiberty/cp-demangle.h @@ -135,12 +135,41 @@ struct d_info - call d_check_char(di, '\0') Everything else is safe. */ #define d_peek_char(di) (*((di)-n)) -#define d_peek_next_char(di) ((di)-n[1]) -#define d_advance(di, i) ((di)-n += (i)) +#ifndef CHECK_DEMANGLER +# define d_peek_next_char(di) ((di)-n[1]) +# define
[PATCH 2/7] Fix build with CP_DEMANGLE_DEBUG
--- libiberty/cp-demangle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 2988b6b..12093cc 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -715,7 +715,7 @@ d_dump (struct demangle_component *dc, int indent) case DEMANGLE_COMPONENT_FIXED_TYPE: printf (fixed-point type, accum? %d, sat? %d\n, dc-u.s_fixed.accum, dc-u.s_fixed.sat); - d_dump (dc-u.s_fixed.length, indent + 2) + d_dump (dc-u.s_fixed.length, indent + 2); break; case DEMANGLE_COMPONENT_ARGLIST: printf (argument list\n); -- 1.8.3.1
[PATCH 3/7] Fix trinary op
--- libiberty/cp-demangle.c | 4 +++- libiberty/testsuite/demangle-expected | 6 ++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 12093cc..44a0a9b 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -3267,7 +3267,9 @@ d_expression_1 (struct d_info *di) struct demangle_component *second; struct demangle_component *third; - if (!strcmp (code, qu)) + if (code == NULL) + return NULL; + else if (!strcmp (code, qu)) { /* ?: expression. */ first = d_expression_1 (di); diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected index 6ea64ae..47ca8f5 100644 --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -4091,6 +4091,12 @@ void g1(A1, Bstatic_castbool(1)) _ZNKSt7complexIiE4realB5cxx11Ev std::complexint::real[abi:cxx11]() const # +# Some more crashes revealed by fuzz-testing: +# Check for NULL pointer when demangling trinary operators +--format=gnu-v3 +Av32_f +Av32_f +# # Ada (GNAT) tests. # # Simple test. -- 1.8.3.1
[PATCH 7/7] Fix several crashes in d_find_pack
--- libiberty/cp-demangle.c | 3 +++ libiberty/testsuite/demangle-expected | 12 2 files changed, 15 insertions(+) diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 289a704..4ca285e 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -4203,6 +4203,9 @@ d_find_pack (struct d_print_info *dpi, case DEMANGLE_COMPONENT_CHARACTER: case DEMANGLE_COMPONENT_FUNCTION_PARAM: case DEMANGLE_COMPONENT_UNNAMED_TYPE: +case DEMANGLE_COMPONENT_FIXED_TYPE: +case DEMANGLE_COMPONENT_DEFAULT_ARG: +case DEMANGLE_COMPONENT_NUMBER: return NULL; case DEMANGLE_COMPONENT_EXTENDED_OPERATOR: diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected index cfa2691..b58cea2 100644 --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -4108,6 +4108,18 @@ _ZDTtl --format=gnu-v3 _ZZN1fEEd_lEv _ZZN1fEEd_lEv +# Handle DEMANGLE_COMPONENT_FIXED_TYPE in d_find_pack +--format=gnu-v3 +DpDFT_ +DpDFT_ +# Likewise, DEMANGLE_COMPONENT_DEFAULT_ARG +--format=gnu-v3 +DpZ1fEd_ +DpZ1fEd_ +# Likewise, DEMANGLE_COMPONENT_NUMBER (??? result is probably still wrong) +--format=gnu-v3 +DpDv1_c +(char __vector(1))... # # Ada (GNAT) tests. # -- 1.8.3.1
[PATCH 6/7] Fix DEMANGLE_COMPONENT_LOCAL_NAME
--- libiberty/cp-demangle.c | 7 +++ libiberty/testsuite/demangle-expected | 4 2 files changed, 11 insertions(+) diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 424b1c5..289a704 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -3243,6 +3243,8 @@ d_expression_1 (struct d_info *di) struct demangle_component *left; struct demangle_component *right; + if (code == NULL) + return NULL; if (op_is_new_cast (op)) left = cplus_demangle_type (di); else @@ -4436,6 +4438,11 @@ d_print_comp_inner (struct d_print_info *dpi, int options, local_name = d_right (typed_name); if (local_name-type == DEMANGLE_COMPONENT_DEFAULT_ARG) local_name = local_name-u.s_unary_num.sub; + if (local_name == NULL) + { + d_print_error (dpi); + return; + } while (local_name-type == DEMANGLE_COMPONENT_RESTRICT_THIS || local_name-type == DEMANGLE_COMPONENT_VOLATILE_THIS || local_name-type == DEMANGLE_COMPONENT_CONST_THIS diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected index 2dbab14..cfa2691 100644 --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -4104,6 +4104,10 @@ _Z111 --format=gnu-v3 _ZDTtl _ZDTtl +# Check for NULL pointer when demangling DEMANGLE_COMPONENT_LOCAL_NAME +--format=gnu-v3 +_ZZN1fEEd_lEv +_ZZN1fEEd_lEv # # Ada (GNAT) tests. # -- 1.8.3.1
Re: [PATCH 4/7] Fix int overflow
On 07.07.2015 1:55, Jeff Law wrote: len = d_number (di); - if (len = 0) + if (len = 0 || len INT_MAX) return NULL; ret = d_identifier (di, len); di-last_name = ret; Isn't this only helpful if sizeof (long) sizeof (int)? Otherwise the compiler is going to eliminate that newly added test, right? So with that in mind, what happens on i686-unknown-linux with this test? Jeff Probably it should be fine, because the problem occurred when len became negative after implicit conversion to int (d_identifier does not check for negative length, but it does check that length does not exceed total string length). In this case (i.e. on ILP32 targets) len will not change sign after conversion to int (because it's a no-op). I'm not completely sure about compiler warnings, but AFAIR, in multilib build libiberty is also built for 32-bit target, and I did not get any additional warnings. -- Regards, Mikhail Maltsev
Re: [12/12] Simplify uses of hash_map
On 06/26/2015 04:37 PM, Rainer Orth wrote: /vol/gcc/src/hg/trunk/local/gcc/hash-map.h:173:8: note: templateclass Arg, bool (* f)(tree_node*, tree_node* const, Arg) void hash_map::traverse(Arg) const [with Arg = Arg; bool (* f)(typename Traits::key_type, const Value, Arg) = f; KeyId = tree_node*; Value = tree_node*; Traits = simple_hashmap_traitsdefault_hash_traitstree_node* ] /vol/gcc/src/hg/trunk/local/gcc/hash-map.h:173:8: note: template argument deduction/substitution failed: /vol/gcc/src/hg/trunk/local/gcc/c-family/cilk.c:747:76: error: could not convert template argument 'fill_decls_vec' to 'bool (*)(tree_node*, tree_node* const, auto_veccilk_decls*)' In file included from /vol/gcc/src/hg/trunk/local/gcc/hash-table.h:553:0, from /vol/gcc/src/hg/trunk/local/gcc/coretypes.h:317, from /vol/gcc/src/hg/trunk/local/gcc/c-family/cilk.c:25: /vol/gcc/src/hg/trunk/local/gcc/hash-map.h:181:8: note: templateclass Arg, bool (* f)(tree_node*, tree_node**, Arg) void hash_map::traverse(Arg) const [with Arg = Arg; bool (* f)(typename Traits::key_type, Value*, Arg) = f; KeyId = tree_node*; Value = tree_node*; Traits = simple_hashmap_traitsdefault_hash_traitstree_node* ] /vol/gcc/src/hg/trunk/local/gcc/hash-map.h:181:8: note: template argument deduction/substitution failed: /vol/gcc/src/hg/trunk/local/gcc/c-family/cilk.c:747:76: error: could not convert template argument 'fill_decls_vec' to 'bool (*)(tree_node*, tree_node**, auto_veccilk_decls*)' make: *** [c-family/cilk.o] Error 1 Rainer It seems that I can also reproduce this issue. The following code was reduced from GCC r224910, genmatch.c: $ cat ./test2.ii template typename Descriptor, template typename class class hash_table { typedef typename Descriptor::value_type value_type; template typename Argument, int (*)(value_type *, Argument) void traverse(Argument); }; template typename Descriptor, template typename class Allocator template typename Argument, int (*)(typename hash_tableDescriptor, Allocator::value_type *, Argument) void hash_tableDescriptor, Allocator::traverse(Argument) {} It is accepted by current GCC and Clang but rejected by EDG: $ /opt/intel/bin/icpc -V Intel(R) C++ Intel(R) 64 Compiler XE for applications running on Intel(R) 64, Version 15.0.3.187 Build 20150407 Copyright (C) 1985-2015 Intel Corporation. All rights reserved. FOR NON-COMMERCIAL USE ONLY $ /opt/intel/bin/icpc -c ./test2.ii ./test2.ii(11): error: declaration is incompatible with function template void hash_tableDescriptor, unnamed::traverseArgument,unnamed(Argument) (declared at line 4) void hash_tableDescriptor, Allocator::traverse(Argument) {} ^ compilation aborted for ./test2.ii (code 2) -- Regards, Mikhail Maltsev
Re: [06/12] Consolidate string hashers
On 23.06.2015 17:49, Richard Sandiford wrote: This patch replaces various string hashers with a single copy in hash-traits.h. (snip) Index: gcc/config/alpha/alpha.c === --- gcc/config/alpha/alpha.c 2015-06-23 15:48:30.751788389 +0100 +++ gcc/config/alpha/alpha.c 2015-06-23 15:48:30.747788453 +0100 @@ -4808,13 +4808,7 @@ alpha_multipass_dfa_lookahead (void) struct GTY(()) alpha_links; -struct string_traits : default_hashmap_traits -{ - static bool equal_keys (const char *const a, const char *const b) - { -return strcmp (a, b) == 0; - } -}; +typedef simple_hashmap_traits nofree_string_hash string_traits; I remember that when we briefly discussed unification of string traits, a looked through GCC code and this one seemed weird to me: it does not reimplement the hash function. I.e. the pointer value is used as hash. I wonder, is it intentional or not? This could actually work if strings are interned (but in that case there is no need to compare them, because comparing pointers would be enough). -- Regards, Mikhail Maltsev
Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1
On 06/24/2015 06:52 PM, Christophe Lyon wrote: Hi Mikhail, In the gcc-5-branch, I can see that your new inhibit-warn-2.C test fails (targets ARM and AArch64). I can see this error message in g++.log: /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C: In function 'void fn1()': /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3: error: 'typename A(Ftypename C template-parameter-1-1 ::type::value || B:: value)::type D::operator=(Expr) [with Expr = int; typename A(Ftypename C template-parameter-1-1 ::type::value || B:: value)::type = int]' is private /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7: error: within this context Christophe. Oops. Sorry for that, it seems that I messed up with my testing box and the backport did not actually get regtested :(. The problem is caused by difference in wording of diagnostics. GCC 6 gives an error on line 35 and a note on line 29: $ ./cc1plus ~/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C void fn1() /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7: error: 'typename A(Ftypename C template-parameter-1-1 ::type::value || B:: value)::type D::operator=(Expr) [with Expr = int; typename A(Ftypename C template-parameter-1-1 ::type::value || B:: value)::type = int]' is private within this context opt = 0; /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3: note: declared private here operator=(Expr); GCC 5 gives two errors: /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3: error: 'typename A(Ftypename C template-parameter-1-1 ::type::value || B:: value)::type D::operator=(Expr) [with Expr = int; typename A(Ftypename C template-parameter-1-1 ::type::value || B:: value)::type = int]' is private operator=(Expr); /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7: error: within this context opt = 0; It can probably be fixed like this: diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C index cb16b4c..f658c1d 100644 --- a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C +++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C @@ -26,11 +26,11 @@ class D { template class Expr typename AFtypename CExpr::type::value || B::value::type - operator=(Expr); // { dg-message declared } + operator=(Expr); // { dg-message private } }; void fn1() { D opt; - opt = 0; // { dg-error private } + opt = 0; // { dg-error this context } } But I am not sure, what should I do in this case. Maybe it is better to remove the failing testcase from GCC 5 branch (provided that inhibit-warn-1.C tests a fix for the same bug and does not fail)? -- Regards, Mikhail Maltsev
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
On 23.06.2015 22:49, Marek Polacek wrote: On Sat, Jun 20, 2015 at 03:02:06AM +0300, Mikhail Maltsev wrote: - /* We do not warn for constants because they are typical of macro - expansions that test for features. */ - if (CONSTANT_CLASS_P (op_left) || CONSTANT_CLASS_P (op_right)) + /* We do not warn for literal constants because they are typical of macro + expansions that test for features. Likewise, we do not warn for + const-qualified and constexpr variables which are initialized by constant + expressions, because they can come from e.g. type_traits or similar user + code. */ + if (TREE_CONSTANT (op_left) || TREE_CONSTANT (op_right)) return; That looks wrong, because with TREE_CONSTANT we'd warn in C but not in C++ for the following: const int a = 4; void f (void) { const int b = 4; static const int c = 5; if (a a) {} if (b b) {} if (c c) {} } Actually for this case the patch silences the warning both for C and C++. It's interesting that Clang warns like this: test.c:7:10: warning: use of logical '' with constant operand [-Wconstant-logical-operand] It does not warn for my testcase with templates. It also does not warn about: void bar (const int parm_a) { const bool a = parm_a; if (a a) {} if (a || a) {} if (parm_a parm_a) {} if (parm_a || parm_a) {} } EDG does not give any warnings at all (in all 3 testcases). Note that const-qualified types are checked using TYPE_READONLY. Yes, but I think we should warn about const-qualified types like in the example above (and in your recent patch). But I'm not even sure that the warning in the original testcase in the PR is bogus; you won't get any warning when using e.g. foounsigned, signed(); in main(). Maybe my snippet does not express clearly enough what it was supposed to express. I actually meant something like this: templateclass _U1, class _U2, class = typename enable_if__and_is_convertible_U1, _T1, is_convertible_U2, _T2::value::type constexpr pair(pair_U1, _U2 __p) : first(std::forward_U1(__p.first)), second(std::forward_U2(__p.second)) { } (it's std::pair move constructor) It is perfectly possible that the user will construct an std::pairT, T object from an std::pairU, U. In this case we get an and of two identical is_convertible instantiations. The difference is that here there is a clever __and_ template which helps to avoid warnings. Well, at least I now know a good way to suppress them in my code :). Though I still think that this warning is bogus. Probably the correct (and the hard) way to check templates is to compare ASTs of the operands before any substitutions. But for now I could try to implement an idea, which I mentioned in the bug report: add a new flag to enum tsubst_flags, and set it when we check ASTs which depend on parameters of a template being instantiated (we already have similar checks for macro expansions). What do you think about such approach? -- Regards, Mikhail Maltsev
Re: [PATCH] Refactoring: use std::swap instead of manual swaps (part 2)
On 16.06.2015 9:09, Uros Bizjak wrote: According to [1], patches that convert to std::swap are considered as obvious and don't need approval. If there is a non-trivial part, please split it to a separate patch for an eventual discussion. [1] https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01620.html Uros. OK. I think that the whole patch is rather obvious, thus committing. It applied smoothly, so I hope that the previous test result is still valid (x86_64-linux bootstrap/regtest + build for full target list). And just in case, I also tested it on arm-eabi simulator today. The applied patch is identical to the one I have posted in my original message, so I'm not repeating it here. -- Regards, Mikhail Maltsev
[C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
Hi. In current implementation we avoid giving logical and/or of equal expressions warning for literal constant operands. The attached patch fixes the check to make it treat const-qualified VAR_DECLs with constant initializers the same way. Bootstrapped and regtested on x86_64-linux. OK for trunk? -- Regards, Mikhail Maltsev gcc/c-family/ChangeLog: 2015-06-19 Mikhail Maltsev malts...@gmail.com PR c++/66572 * c-common.c (warn_logical_operator): Treat constant-initialized VAR_DECLs like literal constants. gcc/testsuite/ChangeLog: 2015-06-19 Mikhail Maltsev malts...@gmail.com PR c++/66572 * c-c++-common/Wlogical-op-2.c: New test. * g++.dg/warn/Wlogical-op-2.C: New test. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index dc2bf00..38c7be9 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -1766,9 +1766,12 @@ warn_logical_operator (location_t location, enum tree_code code, tree type, return; } - /* We do not warn for constants because they are typical of macro - expansions that test for features. */ - if (CONSTANT_CLASS_P (op_left) || CONSTANT_CLASS_P (op_right)) + /* We do not warn for literal constants because they are typical of macro + expansions that test for features. Likewise, we do not warn for + const-qualified and constexpr variables which are initialized by constant + expressions, because they can come from e.g. type_traits or similar user + code. */ + if (TREE_CONSTANT (op_left) || TREE_CONSTANT (op_right)) return; /* This warning only makes sense with logical operands. */ diff --git a/gcc/testsuite/c-c++-common/Wlogical-op-2.c b/gcc/testsuite/c-c++-common/Wlogical-op-2.c new file mode 100644 index 000..47f5c28 --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wlogical-op-2.c @@ -0,0 +1,24 @@ +/* PR c++/66572 */ +/* { dg-do compile } */ +/* { dg-options -Wlogical-op } */ + +#ifndef __cplusplus +# define bool _Bool +# define true 1 +# define false 0 +#endif + +void +no_warn () +{ + const bool cst_a = true; + const bool cst_b = false; + + if (cst_a || cst_b) {} + if (cst_a cst_b) {} + if (true cst_a) {} + if (true || cst_a) {} + if (false cst_a) {} + if (false || cst_a) {} +} + diff --git a/gcc/testsuite/g++.dg/warn/Wlogical-op-2.C b/gcc/testsuite/g++.dg/warn/Wlogical-op-2.C new file mode 100644 index 000..252592c6 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wlogical-op-2.C @@ -0,0 +1,59 @@ +// PR c++/66572 +// { dg-do compile } +// { dg-options -Wlogical-op } + +#if __cplusplus = 201103L +# define CONSTEXPR constexpr +#else +# define CONSTEXPR const +#endif + +struct true_type +{ + static CONSTEXPR bool value = true; +}; + +struct false_type +{ + static CONSTEXPR bool value = true; +}; + +templatetypename T +struct is_unsigned : false_type { }; + +template +struct is_unsignedunsigned int : true_type { }; + +templatetypename T1, typename T2 +bool both_are_unsigned () +{ + return is_unsigned T1::value is_unsigned T2::value; +} + +templatetypename T1, typename T2 +bool one_of_is_unsigned () +{ + return is_unsigned T1::value || is_unsigned T2::value; +} + +void +foo () +{ + both_are_unsigned unsigned int, unsigned int (); + both_are_unsigned int, unsigned int (); + both_are_unsigned int, int (); + one_of_is_unsigned unsigned int, unsigned int (); + one_of_is_unsigned int, unsigned int (); + one_of_is_unsigned int, int (); +} + +void +bar (const int parm_a) +{ + const bool a = parm_a; + if (a a) {}/* { dg-warning logical .and. of equal expressions } */ + if (a || a) {}/* { dg-warning logical .or. of equal expressions } */ + if (parm_a parm_a) {} /* { dg-warning logical .and. of equal expressions } */ + if (parm_a || parm_a) {} /* { dg-warning logical .or. of equal expressions } */ +} +
Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1
On 19.06.2015 19:35, Jason Merrill wrote: OK, thanks. Sorry this took so long to review; please feel free to ping me every week. Jason I added the testcase from PR66467, bootstrapped and regtested on x86_64-linux. The final variant is attached. I applied it to trunk. I see that version 5.2 is set as target milestone for this bug. Should I backport the patch? -- Regards, Mikhail Maltsev diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index eb5e4c5..6656441 100644 --- a/gcc/cp/ChangeLog +++ b/gcc/cp/ChangeLog @@ -1,3 +1,8 @@ +2015-06-20 Mikhail Maltsev malts...@gmail.com + + PR c++/65882 + * call.c (build_new_op_1): Check tf_warning flag in all cases. + 2015-06-19 Jason Merrill ja...@redhat.com PR c++/66585 diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 5d1891d..ba5da4c 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -5640,8 +5640,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1, case TRUTH_ORIF_EXPR: case TRUTH_AND_EXPR: case TRUTH_OR_EXPR: - warn_logical_operator (loc, code, boolean_type_node, -code_orig_arg1, arg1, code_orig_arg2, arg2); + if (complain tf_warning) + warn_logical_operator (loc, code, boolean_type_node, + code_orig_arg1, arg1, code_orig_arg2, arg2); /* Fall through. */ case GT_EXPR: case LT_EXPR: @@ -5649,8 +5650,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1, case LE_EXPR: case EQ_EXPR: case NE_EXPR: - if ((code_orig_arg1 == BOOLEAN_TYPE) - ^ (code_orig_arg2 == BOOLEAN_TYPE)) + if ((complain tf_warning) + ((code_orig_arg1 == BOOLEAN_TYPE) + ^ (code_orig_arg2 == BOOLEAN_TYPE))) maybe_warn_bool_compare (loc, code, arg1, arg2); /* Fall through. */ case PLUS_EXPR: diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 42a0ee9d..89b859f 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2015-06-20 Mikhail Maltsev malts...@gmail.com + + PR c++/65882 + * g++.dg/diagnostic/inhibit-warn-1.C: New test. + * g++.dg/diagnostic/inhibit-warn-2.C: New test. + 2015-06-19 Eric Botcazou ebotca...@adacore.com * gnat.dg/specs/debug1.ads: Adjust. diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-1.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-1.C new file mode 100644 index 000..5655eb4 --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-1.C @@ -0,0 +1,32 @@ +// PR c++/65882 +// { dg-do compile { target c++11 } } +// { dg-options -Wbool-compare } + +// Check that we don't ICE because of reentering error reporting routines while +// evaluating template parameters + +templatetypename +struct type_function { + static constexpr bool value = false; +}; + +templatebool +struct dependent_type { + typedef int type; +}; + +templatetypename T +typename dependent_type(5 type_functionT::value)::type +bar(); + +templatetypename T +typename dependent_type(5 type_functionT::value)::type +foo() +{ + return barint(); +} + +int main() +{ + fooint(); +} diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C new file mode 100644 index 000..cb16b4c --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C @@ -0,0 +1,36 @@ +// PR c++/65882 +// PR c++/66467 +// { dg-do compile } + +template bool +struct A +{ + typedef int type; +}; + +struct B +{ + static const int value = 0; +}; + +template class +struct C +{ + typedef int type; +}; + +template class +struct F : B {}; + +class D +{ + template class Expr + typename AFtypename CExpr::type::value || B::value::type + operator=(Expr); // { dg-message declared } +}; + +void fn1() +{ + D opt; + opt = 0; // { dg-error private } +}
[PATCH, RFC] PR middle-end/55299, contract bitnot through ASR and rotations
Hi. The attached patch adds new match-and-simplify patterns, which fold ~((~a) b) into (a b) for arithmetic shifts (i.e. when A is signed) and perform similar folds for rotations. It also fixes PR tree-optimization/54579 (because we already fold (-a - 1) into ~a). A couple of questions: 1. Should we limit folding to this special case or rather introduce some canonical order of bitnot and shifts (when they are commutative)? In the latter case, which order is better: bitnot as shift/rotate operand or vise-versa? 2. I noticed that some rotation patterns are folded on tree, while other are folded rather late (during second forward propagation). For example on LP64: #define INT_BITS (sizeof (int) * 8) unsigned int rol(unsigned int a, unsigned int b) { return a b | a (INT_BITS - b); } INT_BITS has type unsigned long, so b and (INT_BITS - b) have different types and tree folding fails (if I change int to long, everything is OK). Should this be addressed somehow? 3. Do the new patterns require any special handling of nop-conversions? -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-06-15 Mikhail Maltsev malts...@gmail.com * match.pd: (~((~X) Y) - X Y): New pattern. (~((~X) r Y) - X r Y): New pattern. (~((~X) r Y) - X r Y): New pattern. gcc/testsuite/ChangeLog: 2015-06-15 Mikhail Maltsev malts...@gmail.com * gcc.dg/fold-notrotate-1.c: New test. * gcc.dg/fold-notshift-1.c: New test. * gcc.dg/fold-notshift-2.c: New test. diff --git a/gcc/match.pd b/gcc/match.pd index 1ab2b1c..487af72 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -696,6 +696,21 @@ along with GCC; see the file COPYING3. If not see wi::eq_p (wi::lshift (@0, cand), @2)) (cmp @1 { build_int_cst (TREE_TYPE (@1), cand); }) +/* ~((~X) Y) - X Y (for arithmetic shift). */ +(simplify + (bit_not (rshift (bit_not @0) @1)) + (if (!TYPE_UNSIGNED (TREE_TYPE (@0))) + (rshift @0 @1))) + +/* Same as above, but for rotations. */ +(for rotate (lrotate rrotate) + (simplify + (bit_not (rotate (bit_not @0) @1)) + (rotate @0 @1))) + +/* TODO: ~((-X + CST) Y) - (X - (CST + 1)) Y, + if overflow does not trap. */ + /* Simplifications of conversions. */ /* Basic strip-useless-type-conversions / strip_nops. */ diff --git a/gcc/testsuite/gcc.dg/fold-notrotate-1.c b/gcc/testsuite/gcc.dg/fold-notrotate-1.c new file mode 100644 index 000..7fc43d4 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notrotate-1.c @@ -0,0 +1,36 @@ +/* { dg-do compile } */ +/* { dg-options -O -fdump-tree-optimized } */ + +#define INT_BITS (sizeof (int) * __CHAR_BIT__) +#define ROL(x, y) ((x) (y) | (x) (INT_BITS - (y))) +#define ROR(x, y) ((x) (y) | (x) (INT_BITS - (y))) + +unsigned int +rol (unsigned int a, unsigned int b) +{ + return ~ROL (~a, b); +} + +unsigned int +ror (unsigned int a, unsigned int b) +{ + return ~ROR (~a, b); +} + +#define LONG_BITS (sizeof (long) * __CHAR_BIT__) +#define ROLL(x, y) ((x) (y) | (x) (LONG_BITS - (y))) +#define RORL(x, y) ((x) (y) | (x) (LONG_BITS - (y))) + +unsigned long +roll (unsigned long a, unsigned long b) +{ + return ~ROLL (~a, b); +} + +unsigned long +rorl (unsigned long a, unsigned long b) +{ + return ~RORL (~a, b); +} + +/* { dg-final { scan-tree-dump-not ~ optimized } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-1.c b/gcc/testsuite/gcc.dg/fold-notshift-1.c new file mode 100644 index 000..32a55a0 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notshift-1.c @@ -0,0 +1,44 @@ +/* PR tree-optimization/54579 + PR middle-end/55299 */ + +/* { dg-do compile } */ +/* { dg-options -O -fdump-tree-cddce1 } */ + +int +asr1 (int a, int b) +{ + return ~((~a) b); +} + +long +asr1l (long a, long b) +{ + return ~((~a) b); +} + +int +asr2 (int a, int b) +{ + return -((-a - 1) b) - 1; +} + +int +asr3 (int a, int b) +{ + return a 0 ? ~((~a) b) : a b; +} + +long +asr3l (long a, int b) +{ + return a 0 ? ~((~a) b) : a b; +} + +int +asr4 (int a, int b) +{ + return a 0 ? -(-a - 1 b) - 1 : a b; +} + +/* { dg-final { scan-tree-dump-times 6 cddce1 } } */ +/* { dg-final { scan-tree-dump-not ~ cddce1 } } */ diff --git a/gcc/testsuite/gcc.dg/fold-notshift-2.c b/gcc/testsuite/gcc.dg/fold-notshift-2.c new file mode 100644 index 000..5287610 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-notshift-2.c @@ -0,0 +1,18 @@ +/* PR middle-end/55299 */ + +/* { dg-do compile } */ +/* { dg-options -O -fdump-tree-cddce1 } */ + +unsigned int +lsr (unsigned int a, unsigned int b) +{ + return ~((~a) b); +} + +int +sl (int a, int b) +{ + return ~((~a) b); +} + +/* { dg-final { scan-tree-dump-times ~ 4 cddce1 } } */
[PATCH] Refactoring: use std::swap instead of manual swaps (part 2)
Hi all. The attached patch replaces manual swaps (i.e. tmp = a; a = b; b = tmp;) with std::swap. It also removes a couple of static functions which were used only for implementing such swaps. Bootstrapped/regtested on x86_64-linux; full target list build in progress. OK for trunk? -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-06-14 Mikhail Maltsev malts...@gmail.com * auto-inc-dec.c (reverse_mem): Remove. (reverse_inc): Remove. (parse_add_or_inc): Use std::swap instead of reverse_{mem,inc}. (find_inc): Likewise. * combine.c (combine_simplify_rtx): Use std::swap instead of manual swaps. * df-core.c (df_worklist_dataflow_doublequeue): Likewise. * df-scan.c (df_swap_refs): Remove. (df_sort_and_compress_refs): Use std::swap instead of df_swap_refs. * dominance.c (link_roots): Use std::swap instead of manual swaps. * expr.c (expand_expr_real_2, do_store_flag): Likewise. * fold-const.c (fold_relational_const): Likewise. * genattrtab.c (simplify_test_exp): Likewise. * gimple-match-head.c (gimple_resimplify2 gimple_resimplify3, gimple_simplify): Likewise. * ifcvt.c (noce_try_abs, find_if_header): Likewise. * internal-fn.c (expand_addsub_overflow, expand_mul_overflow): Likewise. * ipa-devirt.c (add_type_duplicate): Likewise. * loop-iv.c (get_biv_step_1, iv_number_of_iterations): Likewise. * lra-lives.c (lra_setup_reload_pseudo_preferenced_hard_reg): Likewise. * lra.c (lra_create_copy): Likewise. * lto-streamer-out.c (DFS::DFS): Likewise. * modulo-sched.c (get_sched_window): Likewise. * omega.c (omega_pretty_print_problem): Likewise. * optabs.c (prepare_float_lib_cmp, expand_mult_highpart): Likewise. * reload1.c (reloads_unique_chain_p): Likewise. * sel-sched-ir.c (exchange_lv_sets, exchange_av_sets): Remove. (exchange_data_sets): Move logic from exchange_{av,lv}_sets here and use std::swap. * simplify-rtx.c (simplify_unary_operation_1): Use std::swap instead of manual swaps. * tree-if-conv.c (is_cond_scalar_reduction, predicate_scalar_phi, predicate_mem_writes): Likewise. * tree-loop-distribution.c (pg_add_dependence_edges): Likewise. * tree-predcom.c (combine_chains): Likewise. * tree-ssa-alias.c (nonoverlapping_component_refs_p, refs_may_alias_p_1): Likewise. * tree-ssa-ifcombine.c (recognize_if_then_else): Likewise. * tree-ssa-loop-ivopts.c (extract_cond_operands): Likewise. * tree-ssa-loop-niter.c (refine_bounds_using_guard, number_of_iterations_cond): Likewise. * tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Likewise. * tree-ssa-sccvn.c (vn_nary_op_compute_hash): Likewise. * tree-vect-slp.c (vect_build_slp_tree): Likewise. * tree-vect-stmts.c (supportable_widening_operation): Likewise. * tree-vrp.c (extract_range_from_binary_expr_1, extract_range_from_unary_expr_1): Likewise. gcc/cp/ChangeLog: 2015-06-14 Mikhail Maltsev malts...@gmail.com * pt.c (maybe_adjust_types_for_deduction): Use std::swap instead of manual swaps. * semantics.c (finish_omp_atomic): Likewise. * typeck.c (cp_build_array_ref): Likewise. gcc/c-family/ChangeLog: 2015-06-14 Mikhail Maltsev malts...@gmail.com * c-common.c (scalar_to_vector): Use std::swap instead of manual swaps. diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c index 0bdf91a..1e93e7e 100644 --- a/gcc/auto-inc-dec.c +++ b/gcc/auto-inc-dec.c @@ -768,28 +768,6 @@ get_next_ref (int regno, basic_block bb, rtx_insn **next_array) } -/* Reverse the operands in a mem insn. */ - -static void -reverse_mem (void) -{ - rtx tmp = mem_insn.reg1; - mem_insn.reg1 = mem_insn.reg0; - mem_insn.reg0 = tmp; -} - - -/* Reverse the operands in a inc insn. */ - -static void -reverse_inc (void) -{ - rtx tmp = inc_insn.reg1; - inc_insn.reg1 = inc_insn.reg0; - inc_insn.reg0 = tmp; -} - - /* Return true if INSN is of a form a = b op c where a and b are regs. op is + if c is a reg and +|- if c is a const. Fill in INC_INSN with what is found. @@ -858,7 +836,7 @@ parse_add_or_inc (rtx_insn *insn, bool before_mem) { /* Reverse the two operands and turn *_ADD into *_INC since a = c + a. */ - reverse_inc (); + std::swap (inc_insn.reg0, inc_insn.reg1); inc_insn.form = before_mem ? FORM_PRE_INC : FORM_POST_INC; return true; } @@ -1018,7 +996,7 @@ find_inc (bool first_try) find this. Only try it once though. */ if (first_try !mem_insn.reg1_is_const) { - reverse_mem (); + std::swap (mem_insn.reg0, mem_insn.reg1); return find_inc (false); } else @@ -1119,7 +1097,7 @@ find_inc (bool first_try
Re: [PATCH] Move gen_* stubs from defaults.h to genflags
On 10.06.2015 10:05, Richard Sandiford wrote: +/* Structure which holds data, required for generating stub gen_* function. */ No comma after data +/* These instructions require default stub function. Stubs are never called. require a default [snip] Seems like this is more naturally a hash_table rather than a hash_map. I think there's also a preference to avoid static constructor-based initialisation. Fixed. There again, this is a generator, so those kinds of concerns aren't particularly important. If we do keep the above though, I think we should put the hasher in hash-map-table.h now. Otherwise these FIXMEs are just going to accumulate, and each time makes it less likely that any consolidation will actually happen. Well, after changing hash_map to hash_table, the hasher class is no longer identical to other hash traits classes. As for fixing other occurrences, I think I'd better leave it for another patch. On 10.06.2015 17:55, Trevor Saunders wrote: + /* Number of arguments (i.e., instruction operands). */ + int opno; unsigned? Fixed. + /* Set to true when generator is output, so no stub is needed. */ + bool done; +}; + +/* These instructions require default stub function. Stubs are never called. are the ones that don't call gcc_unreachable () called? Well, bootstrap on x86_64 passes without such calls, but in general case, I think they may get called (comment from genflags.c:main): /* We need to see all the possibilities. Elided insns may have direct calls to their generators in C code. */ For example, this would work if result of gen_* function is passed directly to some emit_pattern_* function (they can handle NULL pointers). +/* Print out a dummy for generator for instruction NAME with NUM arguments + which either does nothing, or aborts (depending on UNREACHABLE). */ I believe you should drop the first for in this sentence. Fixed. -- Regards, Mikhail Maltsev diff --git a/gcc/defaults.h b/gcc/defaults.h index 057b646..5beddea 100644 --- a/gcc/defaults.h +++ b/gcc/defaults.h @@ -1426,96 +1426,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #define TARGET_VTABLE_USES_DESCRIPTORS 0 #endif -#ifndef HAVE_simple_return -#define HAVE_simple_return 0 -static inline rtx -gen_simple_return () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_return -#define HAVE_return 0 -static inline rtx -gen_return () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_epilogue -#define HAVE_epilogue 0 -static inline rtx -gen_epilogue () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_mem_thread_fence -#define HAVE_mem_thread_fence 0 -static inline rtx -gen_mem_thread_fence (rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_memory_barrier -#define HAVE_memory_barrier 0 -static inline rtx -gen_memory_barrier () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_mem_signal_fence -#define HAVE_mem_signal_fence 0 -static inline rtx -gen_mem_signal_fence (rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_load_multiple -#define HAVE_load_multiple 0 -static inline rtx -gen_load_multiple (rtx, rtx, rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_store_multiple -#define HAVE_store_multiple 0 -static inline rtx -gen_store_multiple (rtx, rtx, rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_tablejump -#define HAVE_tablejump 0 -static inline rtx -gen_tablejump (rtx, rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - #endif /* GCC_INSN_FLAGS_H */ #endif /* ! GCC_DEFAULTS_H */ diff --git a/gcc/genflags.c b/gcc/genflags.c index 0da15f1..2fdf54f 100644 --- a/gcc/genflags.c +++ b/gcc/genflags.c @@ -26,10 +26,70 @@ along with GCC; see the file COPYING3. If not see #include tm.h #include rtl.h #include obstack.h +#include hash-table.h #include errors.h #include read-md.h #include gensupport.h +/* Structure which holds data required for generating stub gen_* function. */ + +struct stub_info_t : typed_noop_removestub_info_t +{ + stub_info_t (const char *, unsigned); + + /* Instruction name. */ + const char *name; + /* Number of arguments (i.e., instruction operands). */ + unsigned opno; + /* Set to true when generator is output, so no stub is needed. */ + bool done; + + /* Helpers for hash_table. */ + typedef stub_info_t *value_type; + typedef const char *compare_type; + static inline hashval_t hash (const stub_info_t *); + static inline bool equal (const stub_info_t *, const char *); +}; + +stub_info_t::stub_info_t (const char *name_, unsigned opno_) : name (name_), + opno (opno_), + done (false) +{ +} + +inline hashval_t +stub_info_t::hash (const stub_info_t *elem) +{ + return htab_hash_string (elem
[PATCH] Move gen_* stubs from defaults.h to genflags
Hi, all. I noticed that defaults.h file contains stub generator functions which simply call gcc_unreachable. FWIW, Trevor added them to remove some conditional compilation which depends on HAVE_insn_name macros (I mean something like r223624). Because we still have ~80 more such conditions in GCC, and probably some of them will be fixed in the same way, I propose a patch, which allows to generate required stubs in genflags. Bootstrapped and regtested on x86_64-unknown-linux-gnu, testing for full target list in progress. OK for trunk? -- Regards, Mikhail Maltsev gcc/ChangeLog: 2015-06-10 Mikhail Maltsev malts...@gmail.com * defaults.h (gen_simple_return, gen_return, gen_mem_thread_fence, gen_memory_barrier, gen_mem_signal_fence, gen_load_multiple, gen_store_multiple, gen_tablejump): Remove. Generate by genflags. (HAVE_*): Likewise. * genflags.c (struct stub_info_t): Define. (stubs): Define variable. (struct gflg_string_hasher): Define. (stubs_map): Define variable. (mark_as_done): New function. (gen_dummy): New function (move code out of gen_proto, generalize). (gen_proto): Use gen_dummy. (gen_insn): Call mark_as_done. (gen_stub): New function. (main): Initialize stubs_map, call gen_stubs for missing stubs. diff --git a/gcc/defaults.h b/gcc/defaults.h index 057b646..5beddea 100644 --- a/gcc/defaults.h +++ b/gcc/defaults.h @@ -1426,96 +1426,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #define TARGET_VTABLE_USES_DESCRIPTORS 0 #endif -#ifndef HAVE_simple_return -#define HAVE_simple_return 0 -static inline rtx -gen_simple_return () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_return -#define HAVE_return 0 -static inline rtx -gen_return () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_epilogue -#define HAVE_epilogue 0 -static inline rtx -gen_epilogue () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_mem_thread_fence -#define HAVE_mem_thread_fence 0 -static inline rtx -gen_mem_thread_fence (rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_memory_barrier -#define HAVE_memory_barrier 0 -static inline rtx -gen_memory_barrier () -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_mem_signal_fence -#define HAVE_mem_signal_fence 0 -static inline rtx -gen_mem_signal_fence (rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_load_multiple -#define HAVE_load_multiple 0 -static inline rtx -gen_load_multiple (rtx, rtx, rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_store_multiple -#define HAVE_store_multiple 0 -static inline rtx -gen_store_multiple (rtx, rtx, rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - -#ifndef HAVE_tablejump -#define HAVE_tablejump 0 -static inline rtx -gen_tablejump (rtx, rtx) -{ - gcc_unreachable (); - return NULL; -} -#endif - #endif /* GCC_INSN_FLAGS_H */ #endif /* ! GCC_DEFAULTS_H */ diff --git a/gcc/genflags.c b/gcc/genflags.c index 0da15f1..2a70b56 100644 --- a/gcc/genflags.c +++ b/gcc/genflags.c @@ -26,10 +26,65 @@ along with GCC; see the file COPYING3. If not see #include tm.h #include rtl.h #include obstack.h +#include hash-map.h #include errors.h #include read-md.h #include gensupport.h +/* Structure which holds data, required for generating stub gen_* function. */ +struct stub_info_t +{ + /* Instruction name. */ + const char *name; + /* Number of arguments (i.e., instruction operands). */ + int opno; + /* Set to true when generator is output, so no stub is needed. */ + bool done; +}; + +/* These instructions require default stub function. Stubs are never called. + They allow to reduce the amount of conditionally-compiled code: if a stub is + defined there is no need to guard the call by #ifdef (plain if statement can + be used instead). */ + +#define DEF_STUB(name, opno) { name, opno, false } + +static stub_info_t stubs[] = { +DEF_STUB (simple_return, 0), +DEF_STUB (return, 0), +DEF_STUB (epilogue, 0), +DEF_STUB (mem_thread_fence,1), +DEF_STUB (memory_barrier, 0), +DEF_STUB (mem_signal_fence,1), +DEF_STUB (load_multiple, 3), +DEF_STUB (store_multiple, 3), +DEF_STUB (tablejump, 2) +}; + +#undef DEF_STUB + +/* Helper traits for using null-terminated strings as keys in hash map. + FIXME: Unify various string hashers and move them to hash-map-traits.h. */ +struct gflg_string_hasher : default_hashmap_traits +{ + typedef const char *value_type; + typedef const char *compare_type; + + static inline hashval_t hash (const char *s) + { +return htab_hash_string (s); + } + + static inline bool equal_keys (const char *p1, const char *p2) + { +return strcmp (p1, p2) == 0; + } +}; + +/* Mapping from insn name
Re: [wwwdocs] Move gcc/README.Portability to wwwdocs codingconventions.html
05.06.2015 23:45, Ciro Santilli writes: [snip] +high, consider using code__builtin_expect/code, e.g., like this:/p + +blockquoteprecode +if (__builtin_expect (ptr != NULL, 0)) + free (ptr); +/code/pre/blockquote It's strange, that we don't have macros for this, e.g.: #define likely(x) __builtin_expect(!!(x), 1) #define unlikely(x) __builtin_expect((x), 0) Is it worth adding them to, say, system.h or ansidecl.h? -- Regards, Mikhail Maltsev
Re: [PATCH, RFC]: Next stage1, refactoring: propagating rtx subclasses
09.05.2015 1:54, Jeff Law wrote: On 05/04/2015 02:18 PM, Mikhail Maltsev wrote: [snip] I'm trying to continue and the next patch (peep_split.patch, peep_split.cl) is addressing the same task in some of the generated code (namely, gen_peephole2_* and gen_split_* series of functions). And that looks good. If it's bootstrapping and regression testing then it's good to go too. If you're going to continue this work, you should probably get write-after-approval access so that you can commit your own approved changes. Is it OK to mention you as a maintainer who can approve my request for write access? Yes, absolutely. If you haven't already done so, go ahead and get this going because... Both patches are approved. Please install onto the trunk. jeff Though this patch was approved about a month ago and I spent some time while fixing the first patch related to rtx class hierarchy, I suppose that it is still OK to apply it without additional approval. I rebased the patch, and it required ~1 line change (which is rather obvious). I also performed the complete test (bootstrapped and regtested on x86_64-linux multilib, checked build of targets in contrib/config-list.mk and ran regtests on several simulators: sh, mips and arm). Commited to trunk as r224183. -- Regards, Mikhail Maltsev diff --git a/gcc/ChangeLog b/gcc/ChangeLog index c388eb5..5c8d6c4 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,21 @@ +2015-06-06 Mikhail Maltsev malts...@gmail.com + + * combine.c (combine_split_insns): Remove cast. + * config/bfin/bfin.c (hwloop_fail): Add cast in try_split call. + * config/sh/sh.c (sh_try_split_insn_simple): Remove cast. + * config/sh/sh_treg_combine.cc (sh_treg_combine::execute): Add cast. + * emit-rtl.c (try_split): Promote type of trial argument to rtx_insn. + * genemit.c (gen_split): Change return type of generated functions to + rtx_insn. + * genrecog.c (get_failure_return): Use NULL instead of NULL_RTX. + (print_subroutine_start): Promote rtx to rtx_insn in gen_split_* and + gen_peephole2_* functions. + (print_subroutine, main): Likewise. + * recog.c (peephole2_optimize): Remove cast. + (peep2_next_insn): Promote return type to rtx_insn. + * recog.h (peep2_next_insn): Fix prototype. + * rtl.h (try_split, split_insns): Likewise. + 2015-06-05 Kaz Kojima kkoj...@gcc.gnu.org PR target/66410 diff --git a/gcc/combine.c b/gcc/combine.c index 01f43b1..8a9ab7a 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -554,7 +554,7 @@ combine_split_insns (rtx pattern, rtx_insn *insn) rtx_insn *ret; unsigned int nregs; - ret = safe_as_a rtx_insn * (split_insns (pattern, insn)); + ret = split_insns (pattern, insn); nregs = max_reg_num (); if (nregs reg_stat.length ()) reg_stat.safe_grow_cleared (nregs); diff --git a/gcc/config/bfin/bfin.c b/gcc/config/bfin/bfin.c index 914a024..7b570cd 100644 --- a/gcc/config/bfin/bfin.c +++ b/gcc/config/bfin/bfin.c @@ -3877,7 +3877,7 @@ hwloop_fail (hwloop_info loop) else { splitting_loops = 1; - try_split (PATTERN (insn), insn, 1); + try_split (PATTERN (insn), safe_as_a rtx_insn * (insn), 1); splitting_loops = 0; } } diff --git a/gcc/config/sh/sh.c b/gcc/config/sh/sh.c index d77154c..3b63014 100644 --- a/gcc/config/sh/sh.c +++ b/gcc/config/sh/sh.c @@ -14236,7 +14236,7 @@ sh_try_split_insn_simple (rtx_insn* i, rtx_insn* curr_insn, int n = 0) fprintf (dump_file, \n); } - rtx_insn* seq = safe_as_artx_insn* (split_insns (PATTERN (i), curr_insn)); + rtx_insn* seq = split_insns (PATTERN (i), curr_insn); if (seq == NULL) return std::make_pair (i, i); diff --git a/gcc/config/sh/sh_treg_combine.cc b/gcc/config/sh/sh_treg_combine.cc index 02e13e8..c09a4c3 100644 --- a/gcc/config/sh/sh_treg_combine.cc +++ b/gcc/config/sh/sh_treg_combine.cc @@ -1612,7 +1612,7 @@ sh_treg_combine::execute (function *fun) log_msg (trying to split insn:\n); log_insn (*i); log_msg (\n); - try_split (PATTERN (*i), *i, 0); + try_split (PATTERN (*i), safe_as_a rtx_insn * (*i), 0); } m_touched_insns.clear (); diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index e632710..7bb2c77 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -3653,9 +3653,8 @@ mark_label_nuses (rtx x) returns TRIAL. If the insn to be returned can be split, it will be. */ rtx_insn * -try_split (rtx pat, rtx uncast_trial, int last) +try_split (rtx pat, rtx_insn *trial, int last) { - rtx_insn *trial = as_a rtx_insn * (uncast_trial); rtx_insn *before = PREV_INSN (trial); rtx_insn *after = NEXT_INSN (trial); rtx note; @@ -3674,7 +3673,7 @@ try_split (rtx pat, rtx uncast_trial, int last) split_branch_probability = XINT (note, 0); probability = split_branch_probability; - seq = safe_as_a rtx_insn * (split_insns (pat, trial)); + seq = split_insns (pat, trial