Re: Patch ping
On 03/04/2016 12:30 AM, Jakub Jelinek wrote: Hi! I'd like to ping a texinfo fix for __builtin_alloca*: http://gcc.gnu.org/ml/gcc-patches/2016-02/msg01842.html OK. jeff
Re: [PATCH] S/390: Set GOARCH to the current target when testing multiarch.
On 03/02/2016 02:05 PM, Dominik Vogt wrote: > gcc/testsuite/ChangeLog > > * go.test/go-test.exp: S/390: Set GOARCH to the current target when > testing multiarch. Applied. Thanks! -Andreas-
Patch ping
Hi! I'd like to ping a texinfo fix for __builtin_alloca*: http://gcc.gnu.org/ml/gcc-patches/2016-02/msg01842.html Jakub
Re: [PATCH] Fix PR70054
On 03/03/2016 05:35 AM, Richard Biener wrote: The following patch adjusts strict_aliasing_warning to use proper alias_set_subset_of instead of relying on alias_sets_conflict_p as after the PR66110 fix aggregates with a char[] member do not automatically behave like having alias-set zero. As a side-effect the test will be somewhat stricter as well accessing a 'long' with a struct { int i; long l; } * will now warn while it previously didn't: struct S { int i; long l; }; long x; struct S foo () { return *(struct S *) } now warns: t.c: In function ‘foo’: t.c:3:35: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] struct S foo () { return *(struct S *) } ^ Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk? Thanks, Richard. 2016-03-03 Richard BienerPR c++/70054 * c-common.c (strict_aliasing_warning): Use alias_set_subset_of instead of alias_sets_conflict_p. * g++.dg/warn/Wstrict-aliasing-bogus-union-2.C: New testcase. * gcc.dg/Wstrict-aliasing-struct-member.c: New testcase. The PR doesn't have a regression marker, but from reading the PR it's clearly a regression. OK for the trunk, Thanks, Jeff
Re: Patch ping
On Fri, Mar 04, 2016 at 12:10:26AM -0700, Jeff Law wrote: > On 03/03/2016 07:35 AM, Jakub Jelinek wrote: > >Hi! > > > >I'd like to ping fix for P1 PR69947: > >https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01743.html > So essentially this is just marking more things so that we don't prune them > away, right? > > It's similar conceptually to one of Pierre-Marie's patches where he removed > the switch and recursed anytime the operand's val_class matched > dw_val_class_die_ref and was !external. Yours just explicitly adds the new > DW_OP_ things to the switch and has a slightly looser check (dropping the > !external part of the check). The !external part is IMHO not needed, as we only set external for attributes, not for operands of DWARF expression opcodes. And, while checking both operands for dw_val_class_die_ref is possible, it is not enough, it doesn't cover the DW_OP_GNU_entry_value case where it is a val_loc, and that case really depends on the context, other val_locs shouldn't be traversed. So, I think it is better to list the opcodes explicitly, as that gives the needed context to what the arguments are, perhaps at some point we'll refer to DIEs that we want to do something different for. When adding new DW_OP_* values, one has to change dozens of other places anyway, so one further one doesn't matter, one has to search for all the spots anyway. Jakub
Re: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)
On 03/03/2016 08:21 AM, David Malcolm wrote: Comment #1 of PR c/68187 identified another overzealous warning from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly indented code: 115if (locked) 116i = CRYPTO_add(>struct_ref, -1, CRYPTO_LOCK_ENGINE); 117else 118i = --e->struct_ref; 119engine_ref_debug(e, 0, -1) 120if (i > 0) 121return 1; Egad. How do people read this code when they have to understand it and make changes.What a steaming pile of . Root cause is that "engine_ref_debug" is actually a debugging macro that was empty in the given configuration, so the code effectively was: 117else // GUARD 118i = --e->struct_ref; // BODY 119 120if (i > 0)// NEXT hence the warning. No surprise we triggered seeing that. OK for trunk? Note: one of the new test cases adds a dg-warning/dg-message pair, and so would require updating if/when the wording change posted here: https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00068.html ("[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation") is applied. gcc/c-family/ChangeLog: PR c/68187 * c-indentation.c (get_visual_column): Move code to determine next tab stop to... (next_tab_stop): ...this new function. (line_contains_hash_if): Delete function. (detect_preprocessor_logic): Delete function. (get_first_nws_vis_column): New function. (detect_intervening_unindent): New function. (should_warn_for_misleading_indentation): Replace call to detect_preprocessor_logic with a call to detect_intervening_unindent. gcc/testsuite/ChangeLog: PR c/68187 * c-c++-common/Wmisleading-indentation.c (fn_42_a): New test function. (fn_42_b): Likewise. (fn_42_c): Likewise. OK. Jeff
Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)
On 03/03/2016 08:21 AM, David Malcolm wrote: PR c/68187 covers two cases involving poor indentation where the indentation is arguably not misleading, but for which -Wmisleading-indentation emits a warning. The two cases appear to be different in nature; one in comment #0 and the other in comment #1. Richi marked the bug as a whole as a P1 regression; it's not clear to me if he meant one or both of these cases, so the following two patches fix both. The rest of this post addresses the case in comment #0 of the PR; the followup post addresses the other case, in comment #1 of the PR. Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64 led to this diagnostic from -Wmisleading-indentation: ../stdlib/strtol_l.c: In function 'strtoul_l_internal': ../stdlib/strtol_l.c:356:9: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation] cnt < thousands_len; }) ^ ../stdlib/strtol_l.c:353:9: note: ...this 'for' clause, but it is not && ({ for (cnt = 0; cnt < thousands_len; ++cnt) ^ The code is question looks like this: 348for (c = *end; c != L_('\0'); c = *++end) 349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) c > L_('9')) 350 # ifdef USE_WIDE_CHAR 351 && (wchar_t) c != thousands 352 # else 353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) 354if (thousands[cnt] != end[cnt]) 355 break; 356cnt < thousands_len; }) 357 # endif 358 && (!ISALPHA (c) 359 || (int) (TOUPPER (c) - L_('A') + 10) >= base)) 360break; Lines 354 and 355 are poorly indented, leading to the warning from -Wmisleading-indentation at line 356. The wording of the warning is clearly wrong: line 356 isn't indented as if guarded by line 353, it's more that lines 354 and 355 *aren't* indented. What's happening is that should_warn_for_misleading_indentation has a heuristic for handling "} else", such as: if (p) foo (1); } else // GUARD foo (2); // BODY foo (3); // NEXT and this heuristic uses the first non-whitespace character in the line containing GUARD as the column of interest: the "}" character. In this case we have: 353&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD 354 if (thousands[cnt] != end[cnt])// BODY 355break; 356 cnt < thousands_len; })// NEXT and so it uses the column of the "&&", and treats it as if it were indented thus: 353for (cnt = 0; cnt < thousands_len; ++cnt)// GUARD 354 if (thousands[cnt] != end[cnt])// BODY 355break; 356 cnt < thousands_len; })// NEXT and thus issues a warning. As far as I can tell the heuristic in question only makes sense for "else" clauses, so the following patch updates it to only use the special column when handling "else" clauses, eliminating the overzealous warning. Doing so led to a nonsensical warning for libstdc++-v3/src/c++11/random.cc:random_device::_M_init: random.cc: In member function ‘void std::random_device::_M_init(const string&)’: random.cc:102:10: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] else if (token != "/dev/urandom" && token != "/dev/random") ^~ random.cc:107:5: note: ...this statement, but the latter is indented as if it does _M_file = static_cast(std::fopen(fname, "rb")); ^~~ so the patch addresses this by tweaking the heuristic that rejects aligned BODY and NEXT so that it doesn't require them to be aligned with the first non-whitespace of the GUARD, simply that they not be indented relative to it. Successfully bootstrapped on x86_64-pc-linux-gnu in combination with the following patch; standalone bootstrap is in progress. OK for trunk if the latter is successful? gcc/c-family/ChangeLog: PR c/68187 * c-indentation.c (should_warn_for_misleading_indentation): When suppressing warnings about cases where the guard and body are on the same column, only use the first non-whitespace column in place of the guard token column when dealing with "else" clauses. When rejecting aligned BODY and NEXT, loosen the requirement from equality with the first non-whitespace of guard to simply that they not be indented relative to it. gcc/testsuite/ChangeLog: PR c/68187 * c-c++-common/Wmisleading-indentation.c (fn_40_a): New test function. (fn_40_b): Likewise. (fn_41_a): Likewise. (fn_41_b): Likewise. OK. FWIW, I think all of c-indentation would fall under the diagnostics maintainership you picked up recently. jeff
Re: Patch ping
On 03/03/2016 07:35 AM, Jakub Jelinek wrote: Hi! I'd like to ping fix for P1 PR69947: https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01743.html So essentially this is just marking more things so that we don't prune them away, right? It's similar conceptually to one of Pierre-Marie's patches where he removed the switch and recursed anytime the operand's val_class matched dw_val_class_die_ref and was !external. Yours just explicitly adds the new DW_OP_ things to the switch and has a slightly looser check (dropping the !external part of the check). I could argue for either approach. Yours AFAICT is safer in that it won't recurse on unexpected DW_OP_ things. Of course, it may require more long term maintenance to keep the list of things to recurse on up-to-date. Either approach is OK with me, given you're a lot more familiar with our dwarf writer than I, I'll go with your judgment on which is the best approach to address the problem. jeff
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On Thu, Mar 03, 2016 at 11:31:08PM -0700, Jeff Law wrote: > >2016-03-03 Marek Polacek> > > > PR c/69798 > > * c-parser.c (c_parser_postfix_expression): Call > > c_parser_cast_expression instead of c_parser_postfix_expression. > > > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > > * gcc.dg/cilk-plus/pr69798-2.c: New test. > Do we need to do anything for the call into c_parser_postfix_expression that > occurs between the two you changed in this patch. > > I think you can get into that code with something like > > _Cilk_spawn _Cilk_spawn ()); The _Cilk_spawn _Cilk_spawn (struct S); case needs the same treatment as the other 2 spots, sure, and should be also covered by a testcase. Perhaps another testcase should test the various other cases of the postfix vs. cast expression I've mentioned in the other mail, just make sure the diagnostics is reasonable and we don't ICE. Jakub
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On 03/03/2016 09:15 AM, Marek Polacek wrote: On Thu, Mar 03, 2016 at 03:28:01PM +0100, Jakub Jelinek wrote: On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword is parsed using recursive call in c_parser_postfix_expression (case RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a typename, so it thinks we're inside a compound literal, which means it expects '{', but that isn't there, so we crash on the assert in c_parser_braced_init: gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); But as the comment in c_parser_postfix_expression says, the code for parsing a compound literal here is likely dead. I made an experiment and added gcc_unreachable () in that block, ran regtest, and there were no failures. Thus it should be safe to just remove the code, which also fixes this ICE; with the patch we just give a proper error and don't crash anymore. Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly nervous about the change, so maybe better table until gcc7? This reminds me of PR67495. Perhaps the answer here is also during the _Cilk* stuff parsing don't call c_parser_postfix_expression, but call c_parser_cast_expression instead and then verify what it got? Alternatively this one works as well. I don't know if any verification of the result should be done (in the second call, the first one is invalid anyway). 2016-03-03 Marek PolacekPR c/69798 * c-parser.c (c_parser_postfix_expression): Call c_parser_cast_expression instead of c_parser_postfix_expression. * gcc.dg/cilk-plus/pr69798-1.c: New test. * gcc.dg/cilk-plus/pr69798-2.c: New test. Do we need to do anything for the call into c_parser_postfix_expression that occurs between the two you changed in this patch. I think you can get into that code with something like _Cilk_spawn _Cilk_spawn ()); For the non-error case (the second one you changed), I think we do want to verify that the expression we got back was a function call and issue an appropriate error otherwise. You could make an argument that we should issue a diagnostic for the other cases as well, but it's less obviously correct. jeff
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On Thu, Mar 03, 2016 at 10:52:20PM -0700, Jeff Law wrote: > On 03/03/2016 07:28 AM, Jakub Jelinek wrote: > >On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: > >>This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so > >>e.g. > >>_Cilk_spawn (void) is invalid. The function call after the cilk_spawn > >>keyword > >>is parsed using recursive call in c_parser_postfix_expression (case > >>RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a > >>typename, so it thinks we're inside a compound literal, which means it > >>expects > >>'{', but that isn't there, so we crash on the assert in > >>c_parser_braced_init: > >>gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); > >>But as the comment in c_parser_postfix_expression says, the code for parsing > >>a compound literal here is likely dead. I made an experiment and added > >>gcc_unreachable () in that block, ran regtest, and there were no failures. > >>Thus it should be safe to just remove the code, which also fixes this ICE; > >>with > >>the patch we just give a proper error and don't crash anymore. > >> > >>Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly > >>nervous about the change, so maybe better table until gcc7? > > > >This reminds me of PR67495. Perhaps the answer here is also during the > >_Cilk* stuff parsing don't call c_parser_postfix_expression, but call > >c_parser_cast_expression instead and then verify what it got? > That sounds more sensible to me -- as long as there aren't unintended side > effects of calling c_parser_cast_expression. The difference is that c_parser_cast_expression can parse (type)..., ++..., --..., &..., +..., -..., *..., ~..., !..., &&..., sizeof/alignof/__extension__/__real/__imag/__transaction_{atomic,relaxed}... Perhaps even safer version would be: if (c_parser_next_token_is (parser, CPP_OPEN_PAREN) && c_token_starts_typename (c_parser_peek_2nd_token (parser))) { ... error handling ... } else ... c_parser_postfix_expression (parser); But, even c_parser_postfix_expression parses tons of expressions that aren't allowed there, so I think it is overkill. It needs to check afterwards, and from what I can see that happens in cilk_set_spawn_marker. Thus I think c_parser_cast_expression is safe, it parses perhaps more unexpected expressions, but will reject them all anyway, because they aren't CALL_EXPR after parsing. Jakub
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On 03/03/2016 07:28 AM, Jakub Jelinek wrote: On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword is parsed using recursive call in c_parser_postfix_expression (case RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a typename, so it thinks we're inside a compound literal, which means it expects '{', but that isn't there, so we crash on the assert in c_parser_braced_init: gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); But as the comment in c_parser_postfix_expression says, the code for parsing a compound literal here is likely dead. I made an experiment and added gcc_unreachable () in that block, ran regtest, and there were no failures. Thus it should be safe to just remove the code, which also fixes this ICE; with the patch we just give a proper error and don't crash anymore. Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly nervous about the change, so maybe better table until gcc7? This reminds me of PR67495. Perhaps the answer here is also during the _Cilk* stuff parsing don't call c_parser_postfix_expression, but call c_parser_cast_expression instead and then verify what it got? That sounds more sensible to me -- as long as there aren't unintended side effects of calling c_parser_cast_expression. Jeff
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On 03/03/2016 07:15 AM, Marek Polacek wrote: This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword is parsed using recursive call in c_parser_postfix_expression (case RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a typename, so it thinks we're inside a compound literal, which means it expects '{', but that isn't there, so we crash on the assert in c_parser_braced_init: gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); But as the comment in c_parser_postfix_expression says, the code for parsing a compound literal here is likely dead. I made an experiment and added gcc_unreachable () in that block, ran regtest, and there were no failures. Thus it should be safe to just remove the code, which also fixes this ICE; with the patch we just give a proper error and don't crash anymore. Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly nervous about the change, so maybe better table until gcc7? 2016-03-03 Marek PolacekPR c/69798 * c-parser.c (c_parser_postfix_expression): Remove code dealing with compound literals. * gcc.dg/cilk-plus/pr69798-1.c: New test. * gcc.dg/cilk-plus/pr69798-2.c: New test. FWIW, I don't particularly like this version. My worry about just removing this code is that I expect our testsuite doesn't cover parsing issues all that well. I'd feel better if it'd been run through one of the commercial suites. Even then those suites don't cover the GNU extensions, but at least the scope of things that might go wrong is significantly diminished. Jeff
Re: Proposed Patch for Bug 69687
On 4 Mar 2016, at 1:43 AM, Bernd Schmidtwrote: > > On 03/03/2016 04:18 PM, Mike Stump wrote: >> On Mar 3, 2016, at 6:55 AM, Marcel Böhme wrote: >>> I have revised the patch and removed the limits. >> >> I looked at the patch, I can find no more unreasonable limits! Wonderful. >> Hope someone will finish off the review and approve. > > Marcel, if you haven't contributed before, we'll need a copyright assignment > from you before we can accept the patch. Do you already have one on file? > > > Bernd > Hi Bernd, I have requested the disclaimer form as per /gd/gnuorg/Copyright/request-disclaim.changes Thanks, - Marcel
C++ PATCH for constexpr operator=
One thing I overlooked in my implementation of C++14 constexpr is that it changed operator= to be potentially constexpr. Tested x86_64-pc-linux-gnu, applying to trunk. commit 445b5c0054f338e6c1904ae4ff09fa0761222fd3 Author: Jason MerrillDate: Tue Mar 1 23:06:54 2016 -0500 * method.c (synthesized_method_walk): operator= can also be constexpr. diff --git a/gcc/cp/method.c b/gcc/cp/method.c index 0235e6a..38f2a54 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -1379,9 +1379,18 @@ synthesized_method_walk (tree ctype, special_function_kind sfk, bool const_p, /* If that user-written default constructor would satisfy the requirements of a constexpr constructor (7.1.5), the - implicitly-defined default constructor is constexpr. */ + implicitly-defined default constructor is constexpr. + + The implicitly-defined copy/move assignment operator is constexpr if + - X is a literal type, and + - the assignment operator selected to copy/move each direct base class + subobject is a constexpr function, and + - for each non-static data member of X that is of class type (or array + thereof), the assignment operator selected to copy/move that member is a + constexpr function. */ if (constexpr_p) -*constexpr_p = ctor_p; +*constexpr_p = ctor_p + || (assign_p && cxx_dialect >= cxx14); move_p = false; switch (sfk) diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-assign1.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-assign1.C new file mode 100644 index 000..4583b64 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-assign1.C @@ -0,0 +1,9 @@ +// { dg-do compile { target c++14 } } + +struct A { }; + +struct B +{ + A a; + constexpr B& operator=(const B&) = default; +};
C++ PATCH to instantiation of generic lambda
In this testcase, instantiating the return type doesn't work before we've instantiated the declaration of the operator(). Furthermore, instantiating the operator() declaration necessarily instantiates the return type, so we can wait and look it up from there. Tested x86_64-pc-linux-gnu, applying to trunk. commit 178af5b2e30a6efb8e49638c37dfea7320586c62 Author: Jason MerrillDate: Wed Mar 2 14:53:31 2016 -0500 * pt.c (tsubst_copy_and_build) [LAMBDA_EXPR]: Get LAMBDA_EXPR_RETURN_TYPE from the instantiated closure. diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index c5b9201..e8cd736 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -17103,8 +17103,6 @@ tsubst_copy_and_build (tree t, else gcc_unreachable (); LAMBDA_EXPR_EXTRA_SCOPE (r) = scope; - LAMBDA_EXPR_RETURN_TYPE (r) - = tsubst (LAMBDA_EXPR_RETURN_TYPE (t), args, complain, in_decl); gcc_assert (LAMBDA_EXPR_THIS_CAPTURE (t) == NULL_TREE && LAMBDA_EXPR_PENDING_PROXIES (t) == NULL); @@ -17115,6 +17113,9 @@ tsubst_copy_and_build (tree t, declaration of the op() for later calls to lambda_function. */ complete_type (type); + if (tree fn = lambda_function (type)) + LAMBDA_EXPR_RETURN_TYPE (r) = TREE_TYPE (TREE_TYPE (fn)); + LAMBDA_EXPR_THIS_CAPTURE (r) = NULL_TREE; insert_pending_capture_proxies (); diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-generic-trailing1.C b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-trailing1.C new file mode 100644 index 000..96755b1 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-trailing1.C @@ -0,0 +1,12 @@ +// { dg-do compile { target c++14 } } + +template +void f() +{ + auto lam = [](auto a)->decltype(++a) { return a; }; +} + +int main() +{ + f(); +}
C++ PATCH for c++/67164 (error with variadic templates)
When we instantiate an element of a pack expansion, we replace the argument pack in the template argument vec with an ARGUMENT_PACK_SELECT which indicates the desired element of the vec. If the args have been used to instantiate other templates as well, the args of those instances get modified as well, which can lead to strange results when we run into ARGUMENT_PACK_SELECT in inappropriate places. This patch fixes this issue by making a copy of the template args before we start messing with them. Tested x86_64-pc-linux-gnu, applying to trunk. commit e32e331a8f6ada0c20ff13240bac030020f627bf Author: Jason MerrillDate: Wed Mar 2 09:17:20 2016 -0500 PR c++/67164 * pt.c (copy_template_args): New. (tsubst_pack_expansion): Use it. diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index b3681be..c5b9201 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -178,6 +178,7 @@ static int check_cv_quals_for_unify (int, tree, tree); static void template_parm_level_and_index (tree, int*, int*); static int unify_pack_expansion (tree, tree, tree, tree, unification_kind_t, bool, bool); +static tree copy_template_args (tree); static tree tsubst_template_arg (tree, tree, tsubst_flags_t, tree); static tree tsubst_template_args (tree, tree, tsubst_flags_t, tree); static tree tsubst_template_parms (tree, tree, tsubst_flags_t); @@ -11037,11 +11038,12 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain, /* For each argument in each argument pack, substitute into the pattern. */ result = make_tree_vec (len); + tree elem_args = copy_template_args (args); for (i = 0; i < len; ++i) { t = gen_elem_of_pack_expansion_instantiation (pattern, packs, i, - args, complain, + elem_args, complain, in_decl); TREE_VEC_ELT (result, i) = t; if (t == error_mark_node) @@ -11136,6 +11138,32 @@ make_argument_pack (tree vec) return pack; } +/* Return an exact copy of template args T that can be modified + independently. */ + +static tree +copy_template_args (tree t) +{ + if (t == error_mark_node) +return t; + + int len = TREE_VEC_LENGTH (t); + tree new_vec = make_tree_vec (len); + + for (int i = 0; i < len; ++i) +{ + tree elt = TREE_VEC_ELT (t, i); + if (elt && TREE_CODE (elt) == TREE_VEC) + elt = copy_template_args (elt); + TREE_VEC_ELT (new_vec, i) = elt; +} + + NON_DEFAULT_TEMPLATE_ARGS_COUNT (new_vec) += NON_DEFAULT_TEMPLATE_ARGS_COUNT (t); + + return new_vec; +} + /* Substitute ARGS into the vector or list of template arguments T. */ static tree diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic-tuple2.C b/gcc/testsuite/g++.dg/cpp0x/variadic-tuple2.C new file mode 100644 index 000..43c00e9 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/variadic-tuple2.C @@ -0,0 +1,29 @@ +// PR c++/67164 +// { dg-do compile { target c++11 } } + +#include + +namespace detail { +template +struct fast_and +: std::is_same > +{ }; +} + +template +struct tuple { +tuple() { } + +template ::value...>::value +>::type> +tuple(Yn&& ...yn) { } + +template ::value...>::value +>::type> +tuple(tuple const& other) { } +}; + +tuple > t{}; +tuple > copy = t;
Re: [AArch64] Emit square root using the Newton series
On 02/16/16 14:56, Evandro Menezes wrote: On 12/08/15 15:35, Evandro Menezes wrote: Emit square root using the Newton series 2015-12-03 Evandro Menezesgcc/ * config/aarch64/aarch64-protos.h (aarch64_emit_swsqrt): Declare new function. * config/aarch64/aarch64-simd.md (sqrt2): New expansion and insn definitions. * config/aarch64/aarch64-tuning-flags.def (AARCH64_EXTRA_TUNE_FAST_SQRT): New tuning macro. * config/aarch64/aarch64.c (aarch64_emit_swsqrt): Define new function. * config/aarch64/aarch64.md (sqrt2): New expansion and insn definitions. * config/aarch64/aarch64.opt (mlow-precision-recip-sqrt): Expand option description. * doc/invoke.texi (mlow-precision-recip-sqrt): Likewise. This patch extends the patch that added support for implementing x^-1/2 using the Newton series by adding support for x^1/2 as well. Is it OK at this point of stage 3? Thank you, James, As I was saying, this patch results in some validation errors in CPU2000 benchmarks using DF. Although proving the algorithm to be pretty solid with a vast set of random values, I'm confused why some benchmarks fail to validate with this implementation of the Newton series for square root too, when they pass with the Newton series for reciprocal square root. Since I had no problems with the same algorithm on x86-64, I wonder if the initial estimate on AArch64, which offers just 8 bits, whereas x86-64 offers 11 bits, has to do with it. Then again, the algorithm iterated 1 less time on x86-64 than on AArch64. Since it seems that the initial estimate is sufficient for CPU2000 to validate when using SF, I'm leaning towards restricting the Newton series for square root only for SF. Your thoughts on the matter are appreciated, Add choices for the reciprocal square root approximation Allow a target to prefer such operation depending on the FP precision. gcc/ * config/aarch64/aarch64-protos.h (AARCH64_EXTRA_TUNE_APPROX_RSQRT): New macro. * config/aarch64/aarch64-tuning-flags.def (AARCH64_EXTRA_TUNE_APPROX_RSQRT_DF): New mask. (AARCH64_EXTRA_TUNE_APPROX_RSQRT_SF): Likewise. * config/aarch64/aarch64.c (use_rsqrt_p): New argument for the mode. (aarch64_builtin_reciprocal): Devise mode from builtin. (aarch64_optab_supported_p): New argument for the mode. Feedback appreciated. Thank you, -- Evandro Menezes
Re: [PATCH] libffi testsuite: Use split to ensure valid tcl list
On Feb 25, 2016, at 12:15 PM, Thomas Schwingewrote: > On Thu, 25 Feb 2016 11:45:06 -0800, Mike Stump wrote: >> On Feb 25, 2016, at 11:10 AM, Thomas Schwinge >> wrote: >>> +set lines [libffi_target_compile $src /dev/null assembly “"] >> >> Does this work on a dos box, or windows or other random non-posix systems? > > I don't know, and can't easily test. However, I had seen the same > pattern be used elsewhere, for example: > >$ git grep dev/null -- libstdc++-v3/testsuite/lib/ >libstdc++-v3/testsuite/lib/libstdc++.exp: set lines > [v3_target_compile $src /dev/null executable ""] >[several more] Ah, I would not worry about it then.
Re: C++ PATCH to implement C++14 aggregate NSDMI (N3653)
This was missing a use of NSDMI when considering list-initialization via aggregate initialization in overload resolution. Tested x86_64-pc-linux-gnu, applying to trunk and 5. commit c2a038bbd3c2a82cc6f6679e5a70705f48571e07 Author: Jason MerrillDate: Wed Mar 2 17:24:27 2016 -0500 * call.c (build_aggr_conv): Use get_nsdmi. diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 8d5582a..3ad3bd5 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -897,6 +897,8 @@ build_aggr_conv (tree type, tree ctor, int flags, tsubst_flags_t complain) if (i < CONSTRUCTOR_NELTS (ctor)) val = CONSTRUCTOR_ELT (ctor, i)->value; + else if (DECL_INITIAL (field)) + val = get_nsdmi (field, /*ctor*/false); else if (TREE_CODE (ftype) == REFERENCE_TYPE) /* Value-initialization of reference is ill-formed. */ return NULL; diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr4.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr4.C new file mode 100644 index 000..71830cd --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr4.C @@ -0,0 +1,13 @@ +// { dg-do compile { target c++14 } } + +struct A +{ + A(int); +}; + +struct B +{ + A a{42}; +}; + +B f() { return {}; }
Re: C++ PATCH for c++/51406, 51161 (wrong-code with static cast to rvalue ref)
On 12/14/2011 12:14 AM, Jason Merrill wrote: The code for casting to rvalue ref was assuming that no base adjustment would be necessary. This patch delegates to the normal lvalue binding code, and then changes the result to be an rvalue reference. The test for DECL_P isn't sufficient to catch all cases where the result of cp_fold_convert is still an lvalue. Now that we're doing delayed folding, let's replace the cp_fold_convert with an unconditional NON_LVALUE_EXPR. Tested x86_64-pc-linux-gnu, applying to trunk. For GCC 5 I'm changing DECL_P to real_lvalue_p instead. commit 46ab061b39527bbdd427bdf887eb6954768d4b61 Author: Jason MerrillDate: Wed Mar 2 01:11:30 2016 -0500 PR c++/51406 * typeck.c (build_static_cast_1): Avoid folding back to lvalue. diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 5145879..20f0afc 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -6704,11 +6704,7 @@ build_static_cast_1 (tree type, tree expr, bool c_cast_p, tree lref = cp_build_reference_type (TREE_TYPE (type), false); result = (perform_direct_initialization_if_possible (lref, expr, c_cast_p, complain)); - result = cp_fold_convert (type, result); - /* Make sure we don't fold back down to a named rvalue reference, - because that would be an lvalue. */ - if (DECL_P (result)) - result = build1 (NON_LVALUE_EXPR, type, result); + result = build1 (NON_LVALUE_EXPR, type, result); return convert_from_reference (result); } else diff --git a/gcc/testsuite/g++.dg/cpp0x/rv-cast5.C b/gcc/testsuite/g++.dg/cpp0x/rv-cast5.C new file mode 100644 index 000..c2473e2 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/rv-cast5.C @@ -0,0 +1,12 @@ +// { dg-do compile { target c++11 } } + +template +struct hold { + T value; + constexpr T&& operator()() && { return static_cast (value); } +}; + +int main() +{ + hold {42}(); +}
Re: [PING] genattrab.c generate switch
On Thu, Mar 3, 2016 at 4:29 PM, Jesper Broge Jørgensenwrote: > > On 18/02/16 13:22, Bernd Schmidt wrote: >> >> On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote: >>> >>> Here is the reformatted patch: >> >> >> This will probably have to wait until stage1. >> >>> + const int code = GET_CODE (op2); >>> + if (code != IOR) >>> +{ >>> + if (code == EQ_ATTR) >> >> >> All the formatting still looks completely mangled. This was probably done >> by your mailer. Please try attaching the diff as text/plain. >> >> >> Bernd >> > Hi i send the patch back as an attatchment as requested about two weeks ago > (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i have not > received any response. > > If it has to wait for stage 1 are there anything else i can do for the patch > untill then? I still suggest to try making write_test_expr() avoid emitting redundant parentheses for chains of || or &&, which would fix the original issue all the same. Previously you claimed that such a change would not be simpler than your current patch, but I gave it a quick try and ended up with a much smaller patch: gcc/genattrtab.c | 26 +++--- 1 file changed, 19 insertions(+), 7 deletions(-)
Re: C++ PATCH for c++/67364 (constexpr vs. empty class)
On 02/25/2016 09:08 AM, Jason Merrill wrote: We don't bother evaluating a store to an empty class member, and we shouldn't complain about accesses either. This needs to use really_empty_class, since that's what expand_aggr_init_1 uses. Tested x86_64-pc-linux-gnu, applying to trunk and 5. commit 9a1d6b7c9fa018033ff444829ad3c597e61f5120 Author: Jason MerrillDate: Wed Mar 2 14:33:54 2016 -0500 PR c++/67364 * constexpr.c (cxx_eval_component_reference): Just return an empty CONSTRUCTOR for an empty class. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index bcb129f..5a81469 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1988,11 +1988,12 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t, } if (CONSTRUCTOR_NO_IMPLICIT_ZERO (whole) - && !is_empty_class (TREE_TYPE (part))) + && !is_really_empty_class (TREE_TYPE (t))) { /* 'whole' is part of the aggregate initializer we're currently building; if there's no initializer for this member yet, that's an - error. */ + error. But expand_aggr_init_1 doesn't bother to initialize really + empty classes, so ignore them here, too. */ if (!ctx->quiet) error ("accessing uninitialized member %qD", part); *non_constant_p = true; diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-empty11.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty11.C new file mode 100644 index 000..7437367 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty11.C @@ -0,0 +1,17 @@ +// PR c++/67364 +// { dg-do compile { target c++11 } } + +template +struct element : Xn { + constexpr element() : Xn() { } +}; + +template +struct closure { + element member; + constexpr closure() { } +}; + +struct empty { struct {} s; }; +constexpr closure tup{}; +constexpr empty first = tup.member;
Re: [PING] genattrab.c generate switch
On 18/02/16 13:22, Bernd Schmidt wrote: On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote: Here is the reformatted patch: This will probably have to wait until stage1. + const int code = GET_CODE (op2); + if (code != IOR) +{ + if (code == EQ_ATTR) All the formatting still looks completely mangled. This was probably done by your mailer. Please try attaching the diff as text/plain. Bernd Hi i send the patch back as an attatchment as requested about two weeks ago (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i have not received any response. If it has to wait for stage 1 are there anything else i can do for the patch untill then?
Re: Reinstate generic stack checking warning with LRA
> This works though, ok for trunk? > > 2016-03-03 Jakub Jelinek> > PR ada/70017 > * gcc.dg/pr70017.c (foo): Store 0 to first element of each array. Sure, thanks. -- Eric Botcazou
Re: [Patch, fortran] PR69834 - Collision in derived type hashes
On 03/03/2016 07:59 AM, Paul Richard Thomas wrote: > Dear All, > > What started out as a provisional kludge, when first working on OOP, > has come back to bite us after 7 years. A collision in derived type > has values has been reported on clf. In principle, as pointed out in > the clf thread, this could mean that existing code might be quietly > confusing dynamic types. Fortunately, this is unlikely because the > error in SELECT TYPE that flagged up this problem might appear or > incorrect fields might be accessed, giving rise to runtime errors. > > The fix uses a new vtable field, '_name' that is loaded with the > value, "typename_scopename", which is used for the cases in SELECT > TYPE and for comparison in SAME_TYPE_AS. I have retained the '_hash' > field for compatibility with existing libraries. It could easily be > removed, if that is preferred, but would require a publicity campaign > to ensure that users recompile their code. > > The changes are sufficiently well described in the ChangeLogs and the > comments in the patch to not warrant further comment. > > I have to confess to not knowing quite what to propose here. My gut > feeling is that we should bite the bullet and the patch should be > applied to trunk and 5-branch. However, I am open, on the grounds > above, to wait until 7.0.0. It does bootstrap and regtest on trunk > with FC23/x86_64. > > Thanks to Dominique for testing an early version of the test and to > Thomas for picking up on the clf thread. > In my very humble opinion, I think you should commit the patch now before release. As I have said before, people know major releases are bleeding edge, bugs if any will be flushed out and can be fixed at 6.2 or 6.3. It is the open nature of our software and the user feedback that makes this all work. (also we know Fortran is not release critical) I tested with my own OOP code which is an adaptation of Metcalf's linked anylist and it works fine. Thats the best I can do and it is fairly complex code. I can send it to you if you would like to have it in your test pile. Regards, Jerry
Re: [PATCH] Fix vec_set_hi* patterns (PR target/70059)
On Thu, Mar 03, 2016 at 01:08:41PM +0100, Jakub Jelinek wrote: > Fixed thusly, unfortunately I don't have access to avx512f (and not even to > avx512dq) hw, so while I will bootstrap/regtest it on Haswell-E, can't test > the tests if they now work at runtime (they link and the assembly of the foo > routine has changed and looks good to me). Can somebody test this please > on real hw or emulator? > Ok for trunk if it passes? FYI, my bootstrap/regtest on Haswell-E (but without trying to run any AVX512-* code, just link it at most) passed on both x86_64-linux and i686-linux. > 2016-03-03 Jakub Jelinek> > PR target/70059 > * config/i386/sse.md (vec_set_lo_, > _vinsert_mask): Formatting > fixes. > (vec_set_hi_): Likewise. Swap VEC_CONCAT operands. > > * gcc.target/i386/avx512f-pr70059.c: New test. > * gcc.target/i386/avx512dq-pr70059.c: New test. Jakub
[PATCH] Another fix for decide_alg (PR target/70062)
Hi! Before my recent decide_alg change, *dynamic_check == -1 was indeed guaranteed, because any_alg_usable_p doesn't depend on the arguments of decide_alg that might change during recursive call, so we'd only recurse if it wouldn't set *dynamic_check. But, if we give up because we'd otherwise recurse infinitely, we can set *dynamic_check to 128. Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2016-03-03 Jakub JelinekPR target/70062 * config/i386/i386.c (decide_alg): If TARGET_INLINE_STRINGOPS_DYNAMICALLY, allow *dynamic_check to be also 128 from the recursive call. * gcc.target/i386/pr70062.c: New test. --- gcc/config/i386/i386.c.jj 2016-03-02 14:08:00.0 +0100 +++ gcc/config/i386/i386.c 2016-03-03 17:48:18.587450348 +0100 @@ -26170,11 +26170,15 @@ decide_alg (HOST_WIDE_INT count, HOST_WI } alg = decide_alg (count, new_expected_size, min_size, max_size, memset, zero_memset, have_as, dynamic_check, noalign); - gcc_assert (*dynamic_check == -1); if (TARGET_INLINE_STRINGOPS_DYNAMICALLY) - *dynamic_check = max; + { + /* *dynamic_check could be set 128 above because we avoided +infinite recursion. */ + gcc_assert (*dynamic_check == -1 || *dynamic_check == 128); + *dynamic_check = max; + } else - gcc_assert (alg != libcall); + gcc_assert (alg != libcall && *dynamic_check == -1); return alg; } return (alg_usable_p (algs->unknown_size, memset, have_as) --- gcc/testsuite/gcc.target/i386/pr70062.c.jj 2016-03-03 17:54:13.167642050 +0100 +++ gcc/testsuite/gcc.target/i386/pr70062.c 2016-03-03 17:54:58.753023808 +0100 @@ -0,0 +1,11 @@ +/* PR target/70062 */ +/* { dg-options "-minline-all-stringops -minline-stringops-dynamically -mmemcpy-strategy=libcall:-1:noalign -Wno-psabi" } */ +/* { dg-additional-options "-mtune=k6-2" { target ia32 } } */ + +typedef int V __attribute__ ((vector_size (32))); + +V +foo (V x) +{ + return (V) { x[0] }; +} Jakub
Re: [PATCH] Fix PR69951
On 2 March 2016 at 10:49, Christophe Lyonwrote: > On 2 March 2016 at 10:16, James Greenhalgh wrote: >> On Tue, Mar 01, 2016 at 05:56:30PM +0100, Christophe Lyon wrote: >>> On 1 March 2016 at 10:51, James Greenhalgh wrote: >>> > On Tue, Mar 01, 2016 at 10:21:27AM +0100, Richard Biener wrote: >>> >> On Mon, 29 Feb 2016, James Greenhalgh wrote: >>> >> >>> >> > On Fri, Feb 26, 2016 at 09:32:53AM +0100, Richard Biener wrote: >>> >> > > >>> >> > > The following fixes PR69951, hopefully the last case of decl alias >>> >> > > issues with alias analysis. This time it's points-to and the >>> >> > > DECL_UIDs >>> >> > > used in points-to sets not being canonicalized. >>> >> > > >>> >> > > The simplest (and cheapest) fix is to make aliases refer to the >>> >> > > ultimate alias target via their DECL_PT_UID which we conveniently >>> >> > > have available. >>> >> > > >>> >> > > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to >>> >> > > trunk. >>> >> > > >>> >> > > Richard. >>> >> > > >>> >> > > 2016-02-26 Richard Biener >>> >> > > >>> >> > > PR tree-optimization/69551 >>> >> > > * tree-ssa-structalias.c (get_constraint_for_ssa_var): When >>> >> > > looking through aliases adjust DECL_PT_UID to refer to the >>> >> > > ultimate alias target. >>> >> > > >>> >> > > * gcc.dg/torture/pr69951.c: New testcase. >>> >> > >>> >> > I see this new testcase failing on an ARM target as so: >>> >> > >>> >> > /tmp/ccChjoFc.s: Assembler messages: >>> >> > /tmp/ccChjoFc.s:21: Warning: [-mwarn-syms]: Assignment makes a >>> >> > symbol match an ARM instruction: b >>> >> > >>> >> > FAIL: gcc.dg/torture/pr69951.c -O0 (test for excess errors) >>> >> > >>> >> > But I haven't managed to reproduce it outside of the test environment. >>> >> > >>> >> > The fix looks trivial, rename b to anything else you fancy (well... >>> >> > stay >>> >> > clear of add and ldr). I'll put a fix in myself if I can manage to get >>> >> > this to reproduce - though if anyone else wants to do it I won't be >>> >> > offended :-). >>> >> >>> >> Huh, I wonder what's the use of such warning. After all 'ldr' is a valid >>> >> C symbol name, too. In fact my cross arm as doesn't report this >>> >> warning (binutils 2.25.0) >>> >> >>> >> > arm-suse-linux-gnueabi-as t.s -mwarn-syms >>> >> Assembler messages: >>> >> Error: unrecognized option -mwarn-syms >>> > >>> > Right, I've figured out the set of conditions... You need to be targeting >>> > an arm-*-linux-* system to make sure that the ASM_OUTPUT_DEF definition >>> > from config/arm/linux-elf.h is pulled in. This causes us to emit: >>> > >>> > b = a >>> > >>> > Rather than >>> > >>> > .setb,a >>> > >>> > Writing it as "b = a" causes the warning added to resolve binutils >>> > PR18347 [1] to kick in, so you need binutils > 2.26 or to have backported >>> > that patch). >>> > >>> James, >>> >>> What happens for you on arm-none-eabi configurations? >>> I'm using binutils-2.25, so I can't see this warning whatever the target. >>> However, I'm using qemu-arm and this test fails on arm-none-eabi, >>> because argc is set to 0 during the startup-code. >>> >>> As I understand it, qemu-arm considers the code page as readonly, >>> and thus the GetCmdLine semi hosting call cannot write argc/argv >>> back to memory in the same code page (I'm using libgloss' crt0). >>> >>> I tried to play with qemu-system-arm, but then libgloss' crt0 does not >>> set the reset vector and the simulation does random things, starting at >>> address 0. >>> >>> Am I missing some newlib/libgloss configuration bits, or should I >>> send a newlib patch to address this? >> >> Hi Christophe, >> >> I didn't get this running under arm-none-eabi. The testcase does have >> undefined behaviour (too few arguments to main), but I'd be surprised if >> that was the issue... I'm sure the testcase could be rewritten to avoid >> the dependence on argc if this proves an issue for other bare-metal >> testers running under an emulator. >> > > Indeed, I'm wondering why the testcase depends on argc beeing non-zero? > To avoid modifying the testcase too much, I propose to replace if (argc) by if (argc >= 0) as in the attached patch. This does make the trick on arm-non-eabi. OK? Christophe. >> Thanks, >> James >> >>> > Resolving it by hacking the testcase would be one fix, but I wonder why >>> > the >>> > ARM port prefers to emit "b = a" in a linux environment if .set does the >>> > same thing and always avoids the warning? Maybe Ramana/Richard/Kyrill/Nick >>> > remember? (AArch64 does the same thing, but the AArch64 gas port doesn't >>> > have the PR18347 fix). >>> > >>> > Cheers, >>> > James >>> > >>> > --- >>> > >>> > [1]: https://sourceware.org/bugzilla/show_bug.cgi?id=18347 >>> > >>> >> >>> >> Richard. >>> >> >>> 2016-03-04 Christophe Lyon * gcc.dg/torture/pr69951.c:
Re: [Patch X86_64] : Fix type attribute for sseimul reservations in znver1.md
On Thu, Mar 3, 2016 at 7:05 PM, Kumar, Venkataramananwrote: > Hi Maintainers, > > The below patch corrects the type attribute for "sseimul" type reservations > in znver1.md. > > (snip) > diff --git a/gcc/config/i386/znver1.md b/gcc/config/i386/znver1.md > index 3db3bed..feeccd7 100644 > --- a/gcc/config/i386/znver1.md > +++ b/gcc/config/i386/znver1.md > @@ -913,28 +913,28 @@ > (define_insn_reservation "znver1_sseimul" 3 > (and (eq_attr "cpu" "znver1") > (and (eq_attr "mode" "TI") > - (and (eq_attr "type" "ssemul") > + (and (eq_attr "type" "sseimul") > (eq_attr "memory" "none" > "znver1-direct,znver1-fp0*3") > > (define_insn_reservation "znver1_sseimul_avx256" 4 > (and (eq_attr "cpu" "znver1") > (and (eq_attr "mode" "OI") > - (and (eq_attr "type" "ssemul") > + (and (eq_attr "type" "sseimul") > (eq_attr "memory" "none" > "znver1-double,znver1-fp0*4") > > (define_insn_reservation "znver1_sseimul_load" 7 > (and (eq_attr "cpu" "znver1") > (and (eq_attr "mode" "TI") > - (and (eq_attr "type" "ssemul") > + (and (eq_attr "type" "sseimul") > (eq_attr "memory" "load" > "znver1-direct,znver1-load,znver1-fp0*3") > > (define_insn_reservation "znver1_sseimul_avx256_load" 8 > (and (eq_attr "cpu" "znver1") > (and (eq_attr "mode" "OI") > - (and (eq_attr "type" "ssemul") > + (and (eq_attr "type" "sseimul") > (eq_attr "memory" "load" > "znver1-double,znver1-load,znver1-fp0*4") > > @@ -942,13 +942,13 @@ > (and (eq_attr "cpu" "znver1") > (and (eq_attr "mode" "DI") >(and (eq_attr "memory" "none") > - (eq_attr "type" "ssemul" > + (eq_attr "type" "sseimul" > "znver1-direct,znver1-fp0*4") > (Snip) > > Changelog > > 2016-03-03 Venkataramanan Kumar > >Fix sseimul type attribute. >* config/i386/znver1.md >(znver1_sseimul, znver1_sseimul_avx256, znver1_sseimul_load, >znver1_sseimul_avx256_load, znver1_sseimul_di, >znver1_sseimul_load_di) : Fix the type attribute. > > Ok for trunk if bootstrap and testing passes? OK. BTW: This patch is trivial, according to [1]. The criteria is simply: Obvious fixes can be committed without prior approval. Just check in the fix and copy it to gcc-patches. A good test to determine whether a fix is obvious: will the person who objects to my work the most be able to find a fault with my fix? If the fix is later found to be faulty, it can always be rolled back. We don't want to get overly restrictive about checkin policies. [1] https://gcc.gnu.org/svnwrite.html Thanks, Uros.
Re: Reinstate generic stack checking warning with LRA
On Thu, Mar 03, 2016 at 12:31:52PM +0100, Eric Botcazou wrote: > > Anyway, looking at pro_and_epilogue dumps, with additional > > -fstack-protector-strong we decrease sp only by 4176, while without it by > > 8224 (on x86_64; the testcase fails on all targets I've tried so far > > ({x86_64,i686,powerpc64{,le},s390{,x},aarch64,armv7hl}-linux). > > Yeah, the threshold is around 4KB, feel free to add a few more HUNDREDs. I've mislooked, with -fstack-protector-strong it just has 48 bytes, and adding many more HUNDREDs doesn't change anything. This works though, ok for trunk? 2016-03-03 Jakub JelinekPR ada/70017 * gcc.dg/pr70017.c (foo): Store 0 to first element of each array. --- gcc/testsuite/gcc.dg/pr70017.c.jj 2016-03-02 07:39:10.0 +0100 +++ gcc/testsuite/gcc.dg/pr70017.c 2016-03-03 19:22:02.098801850 +0100 @@ -13,4 +13,8 @@ void foo(void) { HUNDRED(a) HUNDRED(b) +#undef ONE +#define ONE(s) a##s[0] = 0; + HUNDRED(a) + HUNDRED(b) } /* { dg-warning "frame size too large for reliable stack checking" } */ Jakub
Re: [PATCH, ARM] Fix gcc.c-torture/execute/loop-2b.c execution failure on cortex-m0
On Thursday 03 March 2016 15:32:27 Thomas Preudhomme wrote: > On Thursday 03 March 2016 09:44:31 Ramana Radhakrishnan wrote: > > On Thu, Mar 3, 2016 at 9:40 AM, Thomas Preudhomme > > > >wrote: > > > On Friday 15 January 2016 12:45:04 Ramana Radhakrishnan wrote: > > >> On Wed, Dec 16, 2015 at 9:11 AM, Thomas Preud'homme > > >> > > >> wrote: > > >> > During reorg pass, thumb1_reorg () is tasked with rewriting mov rd, > > >> > rn > > >> > to > > >> > subs rd, rn, 0 to avoid a comparison against 0 instruction before > > >> > doing > > >> > a > > >> > conditional branch based on it. The actual avoiding of cmp is done in > > >> > cbranchsi4_insn instruction C output template. When the condition is > > >> > met, > > >> > the source register (rn) is also propagated into the comparison in > > >> > place > > >> > the destination register (rd). > > >> > > > >> > However, right now thumb1_reorg () only look for a mov followed by a > > >> > cbranchsi but does not check whether the comparison in cbranchsi is > > >> > against the constant 0. This is not safe because a non clobbering > > >> > instruction could exist between the mov and the comparison that > > >> > modifies > > >> > the source register. This is what happens here with a post increment > > >> > of > > >> > the source register after the mov, which skip the [i] == [1] > > >> > comparison for iteration i == 1. > > >> > > > >> > This patch fixes the issue by checking that the comparison is against > > >> > constant 0. > > >> > > > >> > ChangeLog entry is as follow: > > >> > > > >> > > > >> > *** gcc/ChangeLog *** > > >> > > > >> > 2015-12-07 Thomas Preud'homme > > >> > > > >> > * config/arm/arm.c (thumb1_reorg): Check that the comparison > > >> > is > > >> > against the constant 0. > > >> > > >> OK. > > >> > > >> Ramana > > >> > > >> > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > > >> > index 42bf272..49c0a06 100644 > > >> > --- a/gcc/config/arm/arm.c > > >> > +++ b/gcc/config/arm/arm.c > > >> > @@ -17195,7 +17195,7 @@ thumb1_reorg (void) > > >> > > > >> >FOR_EACH_BB_FN (bb, cfun) > > >> > > > >> > { > > >> > > > >> >rtx dest, src; > > >> > > > >> > - rtx pat, op0, set = NULL; > > >> > + rtx cmp, op0, op1, set = NULL; > > >> > > > >> >rtx_insn *prev, *insn = BB_END (bb); > > >> >bool insn_clobbered = false; > > >> > > > >> > @@ -17208,8 +17208,13 @@ thumb1_reorg (void) > > >> > > > >> > continue; > > >> > > > >> >/* Get the register with which we are comparing. */ > > >> > > > >> > - pat = PATTERN (insn); > > >> > - op0 = XEXP (XEXP (SET_SRC (pat), 0), 0); > > >> > + cmp = XEXP (SET_SRC (PATTERN (insn)), 0); > > >> > + op0 = XEXP (cmp, 0); > > >> > + op1 = XEXP (cmp, 1); > > >> > + > > >> > + /* Check that comparison is against ZERO. */ > > >> > + if (!CONST_INT_P (op1) || INTVAL (op1) != 0) > > >> > + continue; > > >> > > > >> >/* Find the first flag setting insn before INSN in basic block > > >> >BB. > > >> >*/ > > >> >gcc_assert (insn != BB_HEAD (bb)); > > >> > > > >> > @@ -17249,7 +17254,7 @@ thumb1_reorg (void) > > >> > > > >> > PATTERN (prev) = gen_rtx_SET (dest, src); > > >> > INSN_CODE (prev) = -1; > > >> > /* Set test register in INSN to dest. */ > > >> > > > >> > - XEXP (XEXP (SET_SRC (pat), 0), 0) = copy_rtx (dest); > > >> > + XEXP (cmp, 0) = copy_rtx (dest); > > >> > > > >> > INSN_CODE (insn) = -1; > > >> > > > >> > } > > >> > > > >> > } > > >> > > > >> > Testsuite shows no regression when run for arm-none-eabi with > > >> > -mcpu=cortex-m0 -mthumb > > > > > > The patch applies cleanly on gcc-5-branch and also show no regression > > > when > > > run for arm-none-eabi with -mcpu=cortex-m0 -mthumb. Is it ok to > > > backport? > > > > This deserves a testcase. > > The original patch don't have one initially because it fixes a fail of an > existing testcase (loop-2b.c). However, the test pass on gcc 5 due to > difference in code generation. I'm currently trying to come up with a > testcase and will get back at you. Sadly I did not manage to come up with a testcase that works on GCC 5. One need to reproduce a sequence of the form: (set B A) (insn clobbering A that is not a set, ie store with post increment) (conditional branch between A and something else) In that case, thumb1_reorg changes the set into (set B (minus A 0)) which is safe but also replace A by B in the conditional insn which is unsafe in the above situation. The problem I am having is to make GCC generate a move instruction because it's always optimized away. Using local register variable is not an option because the move should be between regular registers. Any idea to construct a
[Patch X86_64] : Fix type attribute for sseimul reservations in znver1.md
Hi Maintainers, The below patch corrects the type attribute for "sseimul" type reservations in znver1.md. (snip) diff --git a/gcc/config/i386/znver1.md b/gcc/config/i386/znver1.md index 3db3bed..feeccd7 100644 --- a/gcc/config/i386/znver1.md +++ b/gcc/config/i386/znver1.md @@ -913,28 +913,28 @@ (define_insn_reservation "znver1_sseimul" 3 (and (eq_attr "cpu" "znver1") (and (eq_attr "mode" "TI") - (and (eq_attr "type" "ssemul") + (and (eq_attr "type" "sseimul") (eq_attr "memory" "none" "znver1-direct,znver1-fp0*3") (define_insn_reservation "znver1_sseimul_avx256" 4 (and (eq_attr "cpu" "znver1") (and (eq_attr "mode" "OI") - (and (eq_attr "type" "ssemul") + (and (eq_attr "type" "sseimul") (eq_attr "memory" "none" "znver1-double,znver1-fp0*4") (define_insn_reservation "znver1_sseimul_load" 7 (and (eq_attr "cpu" "znver1") (and (eq_attr "mode" "TI") - (and (eq_attr "type" "ssemul") + (and (eq_attr "type" "sseimul") (eq_attr "memory" "load" "znver1-direct,znver1-load,znver1-fp0*3") (define_insn_reservation "znver1_sseimul_avx256_load" 8 (and (eq_attr "cpu" "znver1") (and (eq_attr "mode" "OI") - (and (eq_attr "type" "ssemul") + (and (eq_attr "type" "sseimul") (eq_attr "memory" "load" "znver1-double,znver1-load,znver1-fp0*4") @@ -942,13 +942,13 @@ (and (eq_attr "cpu" "znver1") (and (eq_attr "mode" "DI") (and (eq_attr "memory" "none") - (eq_attr "type" "ssemul" + (eq_attr "type" "sseimul" "znver1-direct,znver1-fp0*4") (Snip) Changelog 2016-03-03 Venkataramanan KumarFix sseimul type attribute. * config/i386/znver1.md (znver1_sseimul, znver1_sseimul_avx256, znver1_sseimul_load, znver1_sseimul_avx256_load, znver1_sseimul_di, znver1_sseimul_load_di) : Fix the type attribute. Ok for trunk if bootstrap and testing passes? Regards, Venkat.
Re: [PATCH]Replace -shared with -r -nostdlib in gcc.dg/lto/pr61526 pr54709 pr64415 test cases.
Hi Richard, On 03/03/16 12:47, Richard Biener wrote: On Thu, Mar 3, 2016 at 1:07 PM, Renlin Liwrote: Hi Richard, On 03/03/16 10:13, Richard Biener wrote: On Wed, Mar 2, 2016 at 5:12 PM, Renlin Li wrote: Hi Richard, On 02/03/16 13:35, Richard Biener wrote: On Tue, Mar 1, 2016 at 4:56 PM, Renlin Li wrote: Hi Richard, On 01/03/16 09:16, Richard Biener wrote: On Mon, Feb 29, 2016 at 5:13 PM, Renlin Li wrote: Hi all, The gcc.dg/lto/pr54709, pr61526, pr64415 linking testcases keep failing on arm/aarch64 bare-metal target. It's because statically built newlib library is used to link with shared object. And the linker complains about relocations which cannot be used in shared object. For example, the following errors are produced: crtbegin.o: relocation R_ARM_MOVW_ABS_NC against `a local symbol' can not be used when making a shared object; recompile with -fPIC crtbegin.o: relocation R_ARM_THM_MOVW_ABS_NC against `a local symbol' can not be used when making a shared object; recompile with -fPIC librdimon.a(rdimon-syscalls.o): relocation R_AARCH64_ADR_PREL_PG_HI21 against external symbol `_impure_ptr' can not be used when making a shared object; recompile with -fPIC Presumably, bare-metal toolchain for other architecture have those test case failures as well? In this patch, -shared option is replace by -r -nostdlib. So that the standard system startup files or libraries are not used when linking. Note that -shared is not equivalent to -r -nostdlib so please verify that the original issue can be still reproduced with its fix reverted but -r -nostdlib used with the new -r -nostdlib handling on trunk. pr54709_0.c: Cannot be reproduced with even -shared. The error message is the same as shown above. pr64415_0.c: Reproduced with "-r -nostdlib". pr61526_0.c: Reproduced with "-r -nostdlib". By the way, those linking test cases all pass for linux toolchain. Only fail for aarch64/arm baremetal toolchain. Andrew, I saw you have done similar things in r153555 https://gcc.gnu.org/viewcvs/gcc?view=revision=153555 Do you have any thoughts? And also here, the last comments in this ticket suggests to add check_effective_target_shared to the exp file to limit it to linux targets only: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61526 As said LTO testcases tend to be somewhat fragile so limiting them to targets known to work might be the best option. Richard. Forgot the mention that, by purely adding "-nostdlib" option (instead of replacing -shared) fixes the failures as well. I test those test cases again with fix reverted, keep "-shared" option, add "-nostdlib" option. Ok, so I discovered we have a "shared" target which means if a target doesn't support shared libs we can guard against it with using /* { dg-require-effective-target shared } */ does adding that to the three testcases fix the issue for you? By adding this target check /* { dg-require-effective-target shared } */ Those test cases aredeemed to be unsupported, and thus skipped for aarch64-none-elf target. However, it's a little bit tricky for arm bare-metal target. The shared option check actually successes for arm-none-eabi toolchain. This is because the default cpu for arm-none-eabi toolchain is arm7tdmi. And the start file crtbegin.o doesn't contains any modifications not allowed in shared object. arm-none-eabi is built with multilib. When running this testcase, it's compiled with "-march=armv7-a". The crtbegin.o for this architecture version contains relocations which cannot be used in shared object. That's why they fails to linking test. For -shared it should provide a crtbeginS.o then. Why not fix it properly? Richard. That's the case for linux toolchain. Multiple versions of startfile are generated. crtbegin.o, crtbeginS.o, crtbeginT.o etc. If I understand it correctly, this is not applicable to bare-metal tool-chain? Because, normally bare-metal toolchain will not be used to create shared object. I have double checked, almost all bare-metal toolchain only requires crtbegin.o. The targets define STARTFILE_SPEC in a simple way. The failures here are complaining creating shared object including statically generated object. The code in start files is not used or interact with the test cases. So I think it's reasonable to use "-nostdlib" to exclude standard startup file or libraries when testing the linking. Certainly, we can skip the test cases for bare-metal toolchain. However, as explained above, it seems this support checker is not fully capable to do this. /* { dg-require-effective-target shared } */ Regards, Renlin Will adding "-nostdlib" (instead of replace -shared) option be an reasonable fix given my previous check? Regards, Renlin Thanks, Richard. pr54709_0.c: Cannot be reproduced even with test case unmodified. The error message is the same as shown above. with "-nostdlib", no
Re: IRA costs tweaks, PR 56069
On 03/02/2016 10:53 PM, Vladimir Makarov wrote: 2. update_costs_from_allocno records a cost update not just for the initial allocno, but for each of the visited ones. I can sort of see an argument for doing that (let's say if you assign an allocno in the middle of a copy chain you'd want the tail end of the chain to be reset), but in practice I don't think the present algorithm can work at all. In the case of an allocno in the middle of a copy chain the restore would progress in both directions, and in any case it looks like this approach can end up double-counting things when restoring costs. It is just a heuristic. Richard Sandiford proposed this update approach. Before it we had only updates of allocnos directly connected to allocno in question. Richard's approach helped to improve code in some cases. If something works better we should use. The bechmarking is the best criterium. Ccing Richard in case he has comments. The patch is ok for me. But I'd wait for GCC7. People are sensitive to their code performance degradation. Even in most cases the patch improves performance, in some case it can worsen code and people might fill new PRs after such change. Bernd, thanks for working on it and providing a fresh view of the code. For me especially valuable when people benchmark their patches. Sometimes I have to do it by myself. Ok, I'll wait. I did not actually run any benchmarks either, but I looked at generated code for a large number of examples. Based on that I suspect the effect would be too small to detect with benchmark runs. Bernd
Re: Proposed Patch for Bug 69687
On 03/03/2016 04:18 PM, Mike Stump wrote: On Mar 3, 2016, at 6:55 AM, Marcel Böhmewrote: I have revised the patch and removed the limits. I looked at the patch, I can find no more unreasonable limits! Wonderful. Hope someone will finish off the review and approve. Marcel, if you haven't contributed before, we'll need a copyright assignment from you before we can accept the patch. Do you already have one on file? Bernd
Re: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)
On Thu, Mar 3, 2016 at 10:21 AM, David Malcolmwrote: > Comment #1 of PR c/68187 identified another overzealous warning > from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly > indented code: > > 115if (locked) > 116i = CRYPTO_add(>struct_ref, -1, CRYPTO_LOCK_ENGINE); > 117else > 118i = --e->struct_ref; > 119engine_ref_debug(e, 0, -1) > 120if (i > 0) > 121return 1; > > eng_lib.c:120:9: warning: statement is indented as if it were guarded by... > [-Wmisleading-indentation] > if (i > 0) > ^~ > eng_lib.c:117:5: note: ...this 'else' clause, but it is not > else > ^~~~ > > Line 120 is poorly indented, but the warning is arguably unjustified. > > Root cause is that "engine_ref_debug" is actually a debugging macro > that was empty in the given configuration, so the code effectively > was: > > 117else // GUARD > 118i = --e->struct_ref; // BODY > 119 > 120if (i > 0)// NEXT > > hence the warning. > > But the code as seen by a human is clearly *not* misleading, and > hence arguably we shouldn't warn for this case. > > The following patch fixes this by ruling that if there is non-whitespace > in a line between the BODY and the NEXT statements, and that this > non-whitespace is effectively an "unindent" or "outdent" (it's not clear > to me which of these terms is better), then to not issue a warning. Cool, this also fixes the false-positives seen in bdwgc, whose coding style suggests indenting things inside an #ifdef as if it were an if(), e.g.: if (a) foo (); # ifndef A bar (); # endif ...
Re: [committed] Fix libffi/70024
On 03/03/2016 01:55 AM, Richard Biener wrote: Thanks for doing this. I suppose this is also upstream now? I'm re-syncing with upstream now. I did just find that upstream's soname is already 6.4.0, so I may come back and bump both sonames to 7 instead of 5. r~
Re: [committed] Fix libffi/70024
On 03/03/2016 05:35 AM, Rainer Orth wrote: Hi Richard, Unfortunately, even with this fixed, all Solaris/x86 tests now fail to link: FAIL: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 (test for excess errors) Excess errors: Undefined first referenced symbol in file ffi_closure_alloc /var/tmp//ccIpq3qc.o ffi_type_float /var/tmp//ccIpq3qc.o ffi_type_uint64 /var/tmp//ccIpq3qc.o ffi_type_sint32 /var/tmp//ccIpq3qc.o ffi_type_sint16 /var/tmp//ccIpq3qc.o ffi_type_double /var/tmp//ccIpq3qc.o ffi_prep_cif/var/tmp//ccIpq3qc.o ld: fatal: symbol referencing errors turned out to be easy, too: make_sunver.pl got passed a list of non-existant object files: .libs/src/prep_cif.o .libs/src/types.o .libs/src/raw_api.o .libs/src/java_raw_api.o .libs/src/closures.o Treating $(libffi_la_OBJECTS) and $(libffi_la_LIBADD) the same fixed this so all the symbols above got included in libffi.map-sun and all tests pass. Tested on i386-pc-solaris2.12, installed on mainline. Thanks. r~
Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)
On Thu, Mar 3, 2016 at 10:56 AM, David Malcolmwrote: > On Thu, 2016-03-03 at 10:24 -0500, Patrick Palka wrote: >> On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm >> wrote: >> > PR c/68187 covers two cases involving poor indentation where >> > the indentation is arguably not misleading, but for which >> > -Wmisleading-indentation emits a warning. >> > >> > The two cases appear to be different in nature; one in comment #0 >> > and the other in comment #1. Richi marked the bug as a whole as >> > a P1 regression; it's not clear to me if he meant one or both of >> > these cases, so the following two patches fix both. >> > >> > The rest of this post addresses the case in comment #0 of the PR; >> > the followup post addresses the other case, in comment #1 of the >> > PR. >> > >> > Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64 >> > led to this diagnostic from -Wmisleading-indentation: >> > >> > ../stdlib/strtol_l.c: In function 'strtoul_l_internal': >> > ../stdlib/strtol_l.c:356:9: error: statement is indented as if it >> > were guarded by... [-Werror=misleading-indentation] >> > cnt < thousands_len; }) >> > ^ >> > ../stdlib/strtol_l.c:353:9: note: ...this 'for' clause, but it is >> > not >> >&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) >> > ^ >> > >> > The code is question looks like this: >> > >> >348for (c = *end; c != L_('\0'); c = *++end) >> >349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) >> > c > L_('9')) >> >350 # ifdef USE_WIDE_CHAR >> >351 && (wchar_t) c != thousands >> >352 # else >> >353 && ({ for (cnt = 0; cnt < thousands_len; >> > ++cnt) >> >354if (thousands[cnt] != end[cnt]) >> >355 break; >> >356cnt < thousands_len; }) >> >357 # endif >> >358 && (!ISALPHA (c) >> >359 || (int) (TOUPPER (c) - L_('A') + 10) >> > >= base)) >> >360break; >> > >> > Lines 354 and 355 are poorly indented, leading to the warning from >> > -Wmisleading-indentation at line 356. >> > >> > The wording of the warning is clearly wrong: line 356 isn't >> > indented as if >> > guarded by line 353, it's more that lines 354 and 355 *aren't* >> > indented. >> > >> > What's happening is that should_warn_for_misleading_indentation has >> > a >> > heuristic for handling "} else", such as: >> > >> > if (p) >> >foo (1); >> > } else // GUARD >> >foo (2); // BODY >> >foo (3); // NEXT >> > >> > and this heuristic uses the first non-whitespace character in the >> > line >> > containing GUARD as the column of interest: the "}" character. >> > >> > In this case we have: >> > >> >353&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) // >> > GUARD >> >354 if (thousands[cnt] != end[cnt])// >> > BODY >> >355break; >> >356 cnt < thousands_len; })// >> > NEXT >> > >> > and so it uses the column of the "&&", and treats it as if it were >> > indented thus: >> > >> >353for (cnt = 0; cnt < thousands_len; ++cnt)// >> > GUARD >> >354 if (thousands[cnt] != end[cnt])// >> > BODY >> >355break; >> >356 cnt < thousands_len; })// >> > NEXT >> > >> > and thus issues a warning. >> > >> > As far as I can tell the heuristic in question only makes sense for >> > "else" clauses, so the following patch updates it to only use the >> > special column when handling "else" clauses, eliminating the >> > overzealous warning. >> >> Wouldn't this change have the undesirable effect of no longer warning >> about: >> >> if (p) >> foo (1); >> } else if (q) >> foo (2); >> foo (3); > > No, because the rejection based on indentation is done relative to > guard_line_first_nws, rather than guard_vis_column (I tried doing it > via the latter in one version of the patch, and that broke some of the > existing cases, so I didn't make that change). I see. That clears things up for me, thanks. > > See the attached test file (which doesn't have dg-directives yet); the > example you give is test1_d, with an open-brace added to the "if (p)". > > Trunk emits warnings for: > * test1_c > * test1_d > * test1_e > * test1_f (two warnings, one for the "if", one for the "else") > * test1_g > * test2_c > * test2_d > * test2_e > > With the patches, it emits warnings for: > * test1_c > * test1_d > * test1_e > * test1_f (just one warnings, for the "if") > * test1_g > * test2_c > * test2_d > * test2_e > > so the only change is the removal of the erroneous double warning for > the "else" in test1_f. Nice!
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On Thu, Mar 03, 2016 at 09:24:36AM -0700, Jeff Law wrote: > >2016-03-03 Marek Polacek> > > > PR c/69798 > > * c-parser.c (c_parser_postfix_expression): Call > > c_parser_cast_expression instead of c_parser_postfix_expression. > > > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > > * gcc.dg/cilk-plus/pr69798-2.c: New test. > I'd wait for gcc-7. There's actually further Cilk+ fixes queued up from > Ryan. I wanted to get those into gcc-6, but just flat ran out of time. > Perhaps ask for an exception to address the queued up Cilk+ stuff in a minor > release? Well, this one looks fairly safe to me even for gcc-6, and has the additional benefit that it affects solely Cilk+ and nothing else. Furthermore, I believe the difference between cast and postfix expression is just in what is invalid for Cilk+, so it shouldn't affect valid Cilk+ code, only invalid. Jakub
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On Thu, Mar 03, 2016 at 09:24:36AM -0700, Jeff Law wrote: > I'd wait for gcc-7. There's actually further Cilk+ fixes queued up from > Ryan. I wanted to get those into gcc-6, but just flat ran out of time. > Perhaps ask for an exception to address the queued up Cilk+ stuff in a minor > release? Works for me. Marek
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On 03/03/2016 09:15 AM, Marek Polacek wrote: On Thu, Mar 03, 2016 at 03:28:01PM +0100, Jakub Jelinek wrote: On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword is parsed using recursive call in c_parser_postfix_expression (case RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a typename, so it thinks we're inside a compound literal, which means it expects '{', but that isn't there, so we crash on the assert in c_parser_braced_init: gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); But as the comment in c_parser_postfix_expression says, the code for parsing a compound literal here is likely dead. I made an experiment and added gcc_unreachable () in that block, ran regtest, and there were no failures. Thus it should be safe to just remove the code, which also fixes this ICE; with the patch we just give a proper error and don't crash anymore. Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly nervous about the change, so maybe better table until gcc7? This reminds me of PR67495. Perhaps the answer here is also during the _Cilk* stuff parsing don't call c_parser_postfix_expression, but call c_parser_cast_expression instead and then verify what it got? Alternatively this one works as well. I don't know if any verification of the result should be done (in the second call, the first one is invalid anyway). 2016-03-03 Marek PolacekPR c/69798 * c-parser.c (c_parser_postfix_expression): Call c_parser_cast_expression instead of c_parser_postfix_expression. * gcc.dg/cilk-plus/pr69798-1.c: New test. * gcc.dg/cilk-plus/pr69798-2.c: New test. I'd wait for gcc-7. There's actually further Cilk+ fixes queued up from Ryan. I wanted to get those into gcc-6, but just flat ran out of time. Perhaps ask for an exception to address the queued up Cilk+ stuff in a minor release? jeff
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On Thu, Mar 03, 2016 at 03:28:01PM +0100, Jakub Jelinek wrote: > On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: > > This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so > > e.g. > > _Cilk_spawn (void) is invalid. The function call after the cilk_spawn > > keyword > > is parsed using recursive call in c_parser_postfix_expression (case > > RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a > > typename, so it thinks we're inside a compound literal, which means it > > expects > > '{', but that isn't there, so we crash on the assert in > > c_parser_braced_init: > > gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); > > But as the comment in c_parser_postfix_expression says, the code for parsing > > a compound literal here is likely dead. I made an experiment and added > > gcc_unreachable () in that block, ran regtest, and there were no failures. > > Thus it should be safe to just remove the code, which also fixes this ICE; > > with > > the patch we just give a proper error and don't crash anymore. > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly > > nervous about the change, so maybe better table until gcc7? > > This reminds me of PR67495. Perhaps the answer here is also during the > _Cilk* stuff parsing don't call c_parser_postfix_expression, but call > c_parser_cast_expression instead and then verify what it got? Alternatively this one works as well. I don't know if any verification of the result should be done (in the second call, the first one is invalid anyway). 2016-03-03 Marek PolacekPR c/69798 * c-parser.c (c_parser_postfix_expression): Call c_parser_cast_expression instead of c_parser_postfix_expression. * gcc.dg/cilk-plus/pr69798-1.c: New test. * gcc.dg/cilk-plus/pr69798-2.c: New test. diff --git gcc/c/c-parser.c gcc/c/c-parser.c index bb508b7..ce00457 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -8024,8 +8024,8 @@ c_parser_postfix_expression (c_parser *parser) { error_at (loc, "-fcilkplus must be enabled to use " "%<_Cilk_spawn%>"); - expr = c_parser_postfix_expression (parser); - expr.value = error_mark_node; + expr = c_parser_cast_expression (parser, NULL); + expr.value = error_mark_node; } else if (c_parser_peek_token (parser)->keyword == RID_CILK_SPAWN) { @@ -8038,7 +8038,7 @@ c_parser_postfix_expression (c_parser *parser) } else { - expr = c_parser_postfix_expression (parser); + expr = c_parser_cast_expression (parser, NULL); expr.value = build_cilk_spawn (loc, expr.value); } break; diff --git gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c index e69de29..1120193 100644 --- gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c +++ gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c @@ -0,0 +1,12 @@ +/* PR c/69798 */ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ + +int +main () +{ + _Cilk_spawn (void); /* { dg-error "expected expression" } */ + _Cilk_spawn (char []); /* { dg-error "expected expression" } */ + _Cilk_spawn (int *); /* { dg-error "expected expression" } */ + _Cilk_spawn ({}); /* { dg-error "only function calls can be spawned" } */ +} diff --git gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c index e69de29..66bcdc8 100644 --- gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c +++ gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c @@ -0,0 +1,11 @@ +/* PR c/69798 */ +/* { dg-do compile } */ + +int +main () +{ + _Cilk_spawn (void); /* { dg-error "expected expression" } */ + _Cilk_spawn (char []); /* { dg-error "expected expression" } */ + _Cilk_spawn (int *); /* { dg-error "expected expression" } */ + _Cilk_spawn ({}); /* { dg-error "only function calls can be spawned" } */ +} Marek
Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level
On 3 March 2016 at 15:39, Patrick Palkawrote: > On Thu, Mar 3, 2016 at 10:22 AM, Manuel López-Ibáñez > wrote: >> It would be an overall improvement if it was neither a TREE_LIST, nor a >> TREE_VECTOR: https://gcc.gnu.org/wiki/Speedup_areas#Trees >> >> Those kinds of cleanups are always welcome even if they do not improve >> performance noticeably at first glance. The speed-up will show up once >> TREE_LIST is removed completely. > > Ah yeah, I meant if cp_binding_level::names were a vec > since it would have to be resizable. Sure, what I meant is that such a change is an improvement even if you cannot measure any speed-up at all right now. Go for it! Cheers, Manuel.
[Patch, fortran] PR69834 - Collision in derived type hashes
Dear All, What started out as a provisional kludge, when first working on OOP, has come back to bite us after 7 years. A collision in derived type has values has been reported on clf. In principle, as pointed out in the clf thread, this could mean that existing code might be quietly confusing dynamic types. Fortunately, this is unlikely because the error in SELECT TYPE that flagged up this problem might appear or incorrect fields might be accessed, giving rise to runtime errors. The fix uses a new vtable field, '_name' that is loaded with the value, "typename_scopename", which is used for the cases in SELECT TYPE and for comparison in SAME_TYPE_AS. I have retained the '_hash' field for compatibility with existing libraries. It could easily be removed, if that is preferred, but would require a publicity campaign to ensure that users recompile their code. The changes are sufficiently well described in the ChangeLogs and the comments in the patch to not warrant further comment. I have to confess to not knowing quite what to propose here. My gut feeling is that we should bite the bullet and the patch should be applied to trunk and 5-branch. However, I am open, on the grounds above, to wait until 7.0.0. It does bootstrap and regtest on trunk with FC23/x86_64. Thanks to Dominique for testing an early version of the test and to Thomas for picking up on the clf thread. Regards Paul 2016-03-03 Paul ThomasPR fortran/69834 * class.c (gfc_select_type_name): New function. (gfc_find_derived_vtab, find_intrinsic_vtab): Add a new field to the vtable '_name'. Initialize using gfc_select_type_name. * expr.c : Clean up some trailing white space. * gfortran.h : Define 'gfc_add_name_component' and provide prototype for 'gfc_select_type_name'. * module.c (mio_component): Deal with the initializer for the '_name' field. * resolve.c (resolve_select_type): Use the name generated by 'gfc_select_type_name' instead of the hash for the case labels. * trans-expr.c : Generate the access functions for the vtable '_name' field. * trans-intrinsic.c (gfc_conv_same_type_as): Rework to use the vtable '_name' field or, for derived types, the name produced by 'gfc_select_type_name' for comparison, instead of the hash. 2016-03-03 Paul Thomas PR fortran/69834 * gfortran.dg/finalize_21.f90 : Remove the right brace in the test for the tree dump to allow for the new field. * gfortran.dg/select_type_35.f90 : New test. -- The difference between genius and stupidity is; genius has its limits. Albert Einstein Index: gcc/fortran/class.c === *** gcc/fortran/class.c (revision 233626) --- gcc/fortran/class.c (working copy) *** gfc_intrinsic_hash_value (gfc_typespec * *** 552,557 --- 552,589 return (hash % 1); } + /* Provide a full name for any arbitrary type that can be used in +SELECT TYPE and the SAME_TYPE_AS intrinsic. This is loaded into the +vtable '_name' field and is used for the case label in SELECT TYPE +and for derived types in SAME_TYPE_AS. Unlike get_unique_type_string +the derived type name is put before the scope name on the grounds +that this will, most of the time, make distinguishing the names more +efficient. */ + void + gfc_select_type_name (char *name, gfc_typespec *ts, gfc_symbol *type) + { + if (ts != NULL && (ts->type == BT_DERIVED || ts->type == BT_CLASS)) + type = ts->u.derived; + else if (!type) + { + sprintf (name, "%s_%d", gfc_basic_typename (ts->type), ts->kind); + return; + } + gcc_assert (type); + + if (type->attr.unlimited_polymorphic) + { + sprintf (name, "STAR"); + return; + } + + if (type->module) + sprintf (name, "%s_%s", type->name, type->module); + else if (type->ns->proc_name) + sprintf (name, "%s_%s", type->name, type->ns->proc_name->name); + else + sprintf (name, "%s", type->name); + } /* Get the _len component from a class/derived object storing a string. For unlimited polymorphic entities a ref to the _data component is available *** gfc_find_derived_vtab (gfc_symbol *deriv *** 2203,2208 --- 2235,2241 if (ns) { char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1]; + char *cname; get_unique_hashed_string (tname, derived); sprintf (name, "__vtab_%s", tname); *** gfc_find_derived_vtab (gfc_symbol *deriv *** 2405,2410 --- 2438,2458 c->tb->ppc = 1; generate_finalization_wrapper (derived, ns, tname, c); + if (!gfc_add_component (vtype, "_name", )) + goto cleanup; + c->ts.type = BT_CHARACTER; + c->ts.kind = gfc_default_character_kind; + c->attr.access = ACCESS_PRIVATE; +
Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)
On Thu, 2016-03-03 at 10:24 -0500, Patrick Palka wrote: > On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm> wrote: > > PR c/68187 covers two cases involving poor indentation where > > the indentation is arguably not misleading, but for which > > -Wmisleading-indentation emits a warning. > > > > The two cases appear to be different in nature; one in comment #0 > > and the other in comment #1. Richi marked the bug as a whole as > > a P1 regression; it's not clear to me if he meant one or both of > > these cases, so the following two patches fix both. > > > > The rest of this post addresses the case in comment #0 of the PR; > > the followup post addresses the other case, in comment #1 of the > > PR. > > > > Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64 > > led to this diagnostic from -Wmisleading-indentation: > > > > ../stdlib/strtol_l.c: In function 'strtoul_l_internal': > > ../stdlib/strtol_l.c:356:9: error: statement is indented as if it > > were guarded by... [-Werror=misleading-indentation] > > cnt < thousands_len; }) > > ^ > > ../stdlib/strtol_l.c:353:9: note: ...this 'for' clause, but it is > > not > >&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) > > ^ > > > > The code is question looks like this: > > > >348for (c = *end; c != L_('\0'); c = *++end) > >349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) > > c > L_('9')) > >350 # ifdef USE_WIDE_CHAR > >351 && (wchar_t) c != thousands > >352 # else > >353 && ({ for (cnt = 0; cnt < thousands_len; > > ++cnt) > >354if (thousands[cnt] != end[cnt]) > >355 break; > >356cnt < thousands_len; }) > >357 # endif > >358 && (!ISALPHA (c) > >359 || (int) (TOUPPER (c) - L_('A') + 10) > > >= base)) > >360break; > > > > Lines 354 and 355 are poorly indented, leading to the warning from > > -Wmisleading-indentation at line 356. > > > > The wording of the warning is clearly wrong: line 356 isn't > > indented as if > > guarded by line 353, it's more that lines 354 and 355 *aren't* > > indented. > > > > What's happening is that should_warn_for_misleading_indentation has > > a > > heuristic for handling "} else", such as: > > > > if (p) > >foo (1); > > } else // GUARD > >foo (2); // BODY > >foo (3); // NEXT > > > > and this heuristic uses the first non-whitespace character in the > > line > > containing GUARD as the column of interest: the "}" character. > > > > In this case we have: > > > >353&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) // > > GUARD > >354 if (thousands[cnt] != end[cnt])// > > BODY > >355break; > >356 cnt < thousands_len; })// > > NEXT > > > > and so it uses the column of the "&&", and treats it as if it were > > indented thus: > > > >353for (cnt = 0; cnt < thousands_len; ++cnt)// > > GUARD > >354 if (thousands[cnt] != end[cnt])// > > BODY > >355break; > >356 cnt < thousands_len; })// > > NEXT > > > > and thus issues a warning. > > > > As far as I can tell the heuristic in question only makes sense for > > "else" clauses, so the following patch updates it to only use the > > special column when handling "else" clauses, eliminating the > > overzealous warning. > > Wouldn't this change have the undesirable effect of no longer warning > about: > > if (p) > foo (1); > } else if (q) > foo (2); > foo (3); No, because the rejection based on indentation is done relative to guard_line_first_nws, rather than guard_vis_column (I tried doing it via the latter in one version of the patch, and that broke some of the existing cases, so I didn't make that change). See the attached test file (which doesn't have dg-directives yet); the example you give is test1_d, with an open-brace added to the "if (p)". Trunk emits warnings for: * test1_c * test1_d * test1_e * test1_f (two warnings, one for the "if", one for the "else") * test1_g * test2_c * test2_d * test2_e With the patches, it emits warnings for: * test1_c * test1_d * test1_e * test1_f (just one warnings, for the "if") * test1_g * test2_c * test2_d * test2_e so the only change is the removal of the erroneous double warning for the "else" in test1_f. I can add dg-directives and add the attachment to Wmisleading -indentation.c as part of the patch (or keep it as a separate new test file, the former is getting large)./* PR c/68187. */ /* { dg-options "-Wmisleading-indentation" } */ /* { dg-do compile } */ extern int foo (int); extern int bar (int, int); extern int
Re: Proposed Patch for Bug 69687
On 03/03/16 14:21, Bernd Schmidt wrote: On 03/02/2016 06:22 PM, Mike Stump wrote: So, check for overflow, or better use unsigned values that are large enough to never overflow. With no possibility for overflow, you can then retest the bug and see if there are any other failure modes and fix those. What C standard can we assume for libiberty? I was looking@patching this and discovered that SIZE_MAX is defined only for C99, so I'm leaning towards retaining the ints and using INT_MAX. Retaining INT_MAX should be ok in this case, since that should allow pretty large mangled strings. As far as I know, the only users of libiberty are GDB and GCC, and GDB only because they have not completely moved to gnulib yet. GCC is C++, GDB assumes C90 but it is moving to C++ anyway, so it could be bumped to SIZE_MAX later. However, it would be much better to add to libiberty something like gnulib's x2realloc and x2nrealloc and use that because: * It is more concise. * Avoid duplication. * libiberty should be replaced by gnulib eventually * error-handling is shared with xrealloc, which gives both more consistency and more flexibility. Of course, there is an even better fix: Add to the GCC repository enough gnulib modules to use directly the x2realloc from gnulib, make the demangler use that. GDB is already using some gnulib modules, so it should not be a problem for them. It is a bit more work in the short term, but re-implementing function by function a lower quality implementation of the whole gnulib seems much worse in the long run. Cheers, Manuel.
Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level
On Thu, Mar 3, 2016 at 10:22 AM, Manuel López-Ibáñezwrote: > On 03/03/16 14:49, Patrick Palka wrote: >> >> I think the slowness of this function is mostly due to the pointer >> chasing performed in the function store_bindings, where we iterate >> over all the names in each non-global scope to figure out whether to >> preserve them. It would probably improve performance if >> cp_binding_level::names were a vector of trees instead of a linked >> list of trees. > > > It would be an overall improvement if it was neither a TREE_LIST, nor a > TREE_VECTOR: https://gcc.gnu.org/wiki/Speedup_areas#Trees > > Those kinds of cleanups are always welcome even if they do not improve > performance noticeably at first glance. The speed-up will show up once > TREE_LIST is removed completely. Ah yeah, I meant if cp_binding_level::names were a vec since it would have to be resizable. > > Cheers, > > Manuel. > >
Re: [patch] libstdc++/69945 Add __gnu_cxx::__freeres hook
On Wed, 2016-02-24 at 18:35 +, Jonathan Wakely wrote: > This adds a new function to libsupc++ which will free the memory still > in use by the pool used for allocating exceptions when malloc fails. > > This is similar to glibc's __libc_freeres, which valgrind (and other > tools?) use to tell glibc to deallocate everything before exiting. > > I initially called it __gnu_cxx::__free_eh_pool() but I figured we > might have other memory in use at some later date, and we wouldn't > want valgrind to have to start calling a second function, nor make a > function called __free_eh_pool() actually free other things. I tested this on x86_64-pc-linux-gnu with Ivo's valgrind patch from https://bugs.kde.org/show_bug.cgi?id=345307 and it works pretty nicely. No more spurious still reachable memory issues with memcheck. Is there any possibility to get this backported for 5.4? Thanks, Mark
Re: [PATCH, ARM] Fix gcc.c-torture/execute/loop-2b.c execution failure on cortex-m0
On Thursday 03 March 2016 09:44:31 Ramana Radhakrishnan wrote: > On Thu, Mar 3, 2016 at 9:40 AM, Thomas Preudhomme > >wrote: > > On Friday 15 January 2016 12:45:04 Ramana Radhakrishnan wrote: > >> On Wed, Dec 16, 2015 at 9:11 AM, Thomas Preud'homme > >> > >> wrote: > >> > During reorg pass, thumb1_reorg () is tasked with rewriting mov rd, rn > >> > to > >> > subs rd, rn, 0 to avoid a comparison against 0 instruction before doing > >> > a > >> > conditional branch based on it. The actual avoiding of cmp is done in > >> > cbranchsi4_insn instruction C output template. When the condition is > >> > met, > >> > the source register (rn) is also propagated into the comparison in > >> > place > >> > the destination register (rd). > >> > > >> > However, right now thumb1_reorg () only look for a mov followed by a > >> > cbranchsi but does not check whether the comparison in cbranchsi is > >> > against the constant 0. This is not safe because a non clobbering > >> > instruction could exist between the mov and the comparison that > >> > modifies > >> > the source register. This is what happens here with a post increment of > >> > the source register after the mov, which skip the [i] == [1] > >> > comparison for iteration i == 1. > >> > > >> > This patch fixes the issue by checking that the comparison is against > >> > constant 0. > >> > > >> > ChangeLog entry is as follow: > >> > > >> > > >> > *** gcc/ChangeLog *** > >> > > >> > 2015-12-07 Thomas Preud'homme > >> > > >> > * config/arm/arm.c (thumb1_reorg): Check that the comparison is > >> > against the constant 0. > >> > >> OK. > >> > >> Ramana > >> > >> > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > >> > index 42bf272..49c0a06 100644 > >> > --- a/gcc/config/arm/arm.c > >> > +++ b/gcc/config/arm/arm.c > >> > @@ -17195,7 +17195,7 @@ thumb1_reorg (void) > >> > > >> >FOR_EACH_BB_FN (bb, cfun) > >> > > >> > { > >> > > >> >rtx dest, src; > >> > > >> > - rtx pat, op0, set = NULL; > >> > + rtx cmp, op0, op1, set = NULL; > >> > > >> >rtx_insn *prev, *insn = BB_END (bb); > >> >bool insn_clobbered = false; > >> > > >> > @@ -17208,8 +17208,13 @@ thumb1_reorg (void) > >> > > >> > continue; > >> > > >> >/* Get the register with which we are comparing. */ > >> > > >> > - pat = PATTERN (insn); > >> > - op0 = XEXP (XEXP (SET_SRC (pat), 0), 0); > >> > + cmp = XEXP (SET_SRC (PATTERN (insn)), 0); > >> > + op0 = XEXP (cmp, 0); > >> > + op1 = XEXP (cmp, 1); > >> > + > >> > + /* Check that comparison is against ZERO. */ > >> > + if (!CONST_INT_P (op1) || INTVAL (op1) != 0) > >> > + continue; > >> > > >> >/* Find the first flag setting insn before INSN in basic block > >> >BB. > >> >*/ > >> >gcc_assert (insn != BB_HEAD (bb)); > >> > > >> > @@ -17249,7 +17254,7 @@ thumb1_reorg (void) > >> > > >> > PATTERN (prev) = gen_rtx_SET (dest, src); > >> > INSN_CODE (prev) = -1; > >> > /* Set test register in INSN to dest. */ > >> > > >> > - XEXP (XEXP (SET_SRC (pat), 0), 0) = copy_rtx (dest); > >> > + XEXP (cmp, 0) = copy_rtx (dest); > >> > > >> > INSN_CODE (insn) = -1; > >> > > >> > } > >> > > >> > } > >> > > >> > Testsuite shows no regression when run for arm-none-eabi with > >> > -mcpu=cortex-m0 -mthumb > > > > The patch applies cleanly on gcc-5-branch and also show no regression when > > run for arm-none-eabi with -mcpu=cortex-m0 -mthumb. Is it ok to backport? > This deserves a testcase. The original patch don't have one initially because it fixes a fail of an existing testcase (loop-2b.c). However, the test pass on gcc 5 due to difference in code generation. I'm currently trying to come up with a testcase and will get back at you. Best regards, Thomas
Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)
On Thu, Mar 3, 2016 at 10:21 AM, David Malcolmwrote: > PR c/68187 covers two cases involving poor indentation where > the indentation is arguably not misleading, but for which > -Wmisleading-indentation emits a warning. > > The two cases appear to be different in nature; one in comment #0 > and the other in comment #1. Richi marked the bug as a whole as > a P1 regression; it's not clear to me if he meant one or both of > these cases, so the following two patches fix both. > > The rest of this post addresses the case in comment #0 of the PR; > the followup post addresses the other case, in comment #1 of the PR. > > Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64 > led to this diagnostic from -Wmisleading-indentation: > > ../stdlib/strtol_l.c: In function 'strtoul_l_internal': > ../stdlib/strtol_l.c:356:9: error: statement is indented as if it were > guarded by... [-Werror=misleading-indentation] > cnt < thousands_len; }) > ^ > ../stdlib/strtol_l.c:353:9: note: ...this 'for' clause, but it is not >&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) > ^ > > The code is question looks like this: > >348for (c = *end; c != L_('\0'); c = *++end) >349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) c > > L_('9')) >350 # ifdef USE_WIDE_CHAR >351 && (wchar_t) c != thousands >352 # else >353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) >354if (thousands[cnt] != end[cnt]) >355 break; >356cnt < thousands_len; }) >357 # endif >358 && (!ISALPHA (c) >359 || (int) (TOUPPER (c) - L_('A') + 10) >= base)) >360break; > > Lines 354 and 355 are poorly indented, leading to the warning from > -Wmisleading-indentation at line 356. > > The wording of the warning is clearly wrong: line 356 isn't indented as if > guarded by line 353, it's more that lines 354 and 355 *aren't* indented. > > What's happening is that should_warn_for_misleading_indentation has a > heuristic for handling "} else", such as: > > if (p) >foo (1); > } else // GUARD >foo (2); // BODY >foo (3); // NEXT > > and this heuristic uses the first non-whitespace character in the line > containing GUARD as the column of interest: the "}" character. > > In this case we have: > >353&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD >354 if (thousands[cnt] != end[cnt])// BODY >355break; >356 cnt < thousands_len; })// NEXT > > and so it uses the column of the "&&", and treats it as if it were > indented thus: > >353for (cnt = 0; cnt < thousands_len; ++cnt)// GUARD >354 if (thousands[cnt] != end[cnt])// BODY >355break; >356 cnt < thousands_len; })// NEXT > > and thus issues a warning. > > As far as I can tell the heuristic in question only makes sense for > "else" clauses, so the following patch updates it to only use the > special column when handling "else" clauses, eliminating the > overzealous warning. Wouldn't this change have the undesirable effect of no longer warning about: if (p) foo (1); } else if (q) foo (2); foo (3);
Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level
On 03/03/16 14:49, Patrick Palka wrote: I think the slowness of this function is mostly due to the pointer chasing performed in the function store_bindings, where we iterate over all the names in each non-global scope to figure out whether to preserve them. It would probably improve performance if cp_binding_level::names were a vector of trees instead of a linked list of trees. It would be an overall improvement if it was neither a TREE_LIST, nor a TREE_VECTOR: https://gcc.gnu.org/wiki/Speedup_areas#Trees Those kinds of cleanups are always welcome even if they do not improve performance noticeably at first glance. The speed-up will show up once TREE_LIST is removed completely. Cheers, Manuel.
Re: Proposed Patch for Bug 69687
On Mar 3, 2016, at 6:55 AM, Marcel Böhmewrote: > I have revised the patch and removed the limits. I looked at the patch, I can find no more unreasonable limits! Wonderful. Hope someone will finish off the review and approve.
Re: [PATCH] Fix vec_set_hi* patterns (PR target/70059)
Hi Jakub, On 03 Mar 13:08, Jakub Jelinek wrote: > routine has changed and looks good to me). Can somebody test this please > on real hw or emulator? I'll run testing on the simulator. -- Thanks, K
Re: Proposed Patch for Bug 69687
On Mar 3, 2016, at 6:21 AM, Bernd Schmidtwrote: > What C standard can we assume for libiberty? I was looking at patching this > and discovered that SIZE_MAX is defined only for C99, so I'm leaning towards > retaining the ints and using INT_MAX. As long as you don’t need a constant… you can also do something like: #ifndef SIZE_MAX #define SIZE_MAX (sizeof (size_t) == sizeof (int) ? INT_MAX : sizeof (size_t) == sizeof (long) ? LONG_MAX : (abort (), 0)) #endif but, you need to consider the signedness of it. A size bounded by int might be annoying if an int was 16 bits, but, we don’t care about such platforms hosting gcc, so, not a problem in reality. Once we get to 32-biit (or more), we’re good. No one better have a symbol >2 billion bytes. And if they do, they can submit that patch to fix it in about 1000 years. :-) I think an INT_MAX only version is fine.
[PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)
PR c/68187 covers two cases involving poor indentation where the indentation is arguably not misleading, but for which -Wmisleading-indentation emits a warning. The two cases appear to be different in nature; one in comment #0 and the other in comment #1. Richi marked the bug as a whole as a P1 regression; it's not clear to me if he meant one or both of these cases, so the following two patches fix both. The rest of this post addresses the case in comment #0 of the PR; the followup post addresses the other case, in comment #1 of the PR. Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64 led to this diagnostic from -Wmisleading-indentation: ../stdlib/strtol_l.c: In function 'strtoul_l_internal': ../stdlib/strtol_l.c:356:9: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation] cnt < thousands_len; }) ^ ../stdlib/strtol_l.c:353:9: note: ...this 'for' clause, but it is not && ({ for (cnt = 0; cnt < thousands_len; ++cnt) ^ The code is question looks like this: 348for (c = *end; c != L_('\0'); c = *++end) 349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) c > L_('9')) 350 # ifdef USE_WIDE_CHAR 351 && (wchar_t) c != thousands 352 # else 353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) 354if (thousands[cnt] != end[cnt]) 355 break; 356cnt < thousands_len; }) 357 # endif 358 && (!ISALPHA (c) 359 || (int) (TOUPPER (c) - L_('A') + 10) >= base)) 360break; Lines 354 and 355 are poorly indented, leading to the warning from -Wmisleading-indentation at line 356. The wording of the warning is clearly wrong: line 356 isn't indented as if guarded by line 353, it's more that lines 354 and 355 *aren't* indented. What's happening is that should_warn_for_misleading_indentation has a heuristic for handling "} else", such as: if (p) foo (1); } else // GUARD foo (2); // BODY foo (3); // NEXT and this heuristic uses the first non-whitespace character in the line containing GUARD as the column of interest: the "}" character. In this case we have: 353&& ({ for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD 354 if (thousands[cnt] != end[cnt])// BODY 355break; 356 cnt < thousands_len; })// NEXT and so it uses the column of the "&&", and treats it as if it were indented thus: 353for (cnt = 0; cnt < thousands_len; ++cnt)// GUARD 354 if (thousands[cnt] != end[cnt])// BODY 355break; 356 cnt < thousands_len; })// NEXT and thus issues a warning. As far as I can tell the heuristic in question only makes sense for "else" clauses, so the following patch updates it to only use the special column when handling "else" clauses, eliminating the overzealous warning. Doing so led to a nonsensical warning for libstdc++-v3/src/c++11/random.cc:random_device::_M_init: random.cc: In member function ‘void std::random_device::_M_init(const string&)’: random.cc:102:10: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] else if (token != "/dev/urandom" && token != "/dev/random") ^~ random.cc:107:5: note: ...this statement, but the latter is indented as if it does _M_file = static_cast(std::fopen(fname, "rb")); ^~~ so the patch addresses this by tweaking the heuristic that rejects aligned BODY and NEXT so that it doesn't require them to be aligned with the first non-whitespace of the GUARD, simply that they not be indented relative to it. Successfully bootstrapped on x86_64-pc-linux-gnu in combination with the following patch; standalone bootstrap is in progress. OK for trunk if the latter is successful? gcc/c-family/ChangeLog: PR c/68187 * c-indentation.c (should_warn_for_misleading_indentation): When suppressing warnings about cases where the guard and body are on the same column, only use the first non-whitespace column in place of the guard token column when dealing with "else" clauses. When rejecting aligned BODY and NEXT, loosen the requirement from equality with the first non-whitespace of guard to simply that they not be indented relative to it. gcc/testsuite/ChangeLog: PR c/68187 * c-c++-common/Wmisleading-indentation.c (fn_40_a): New test function. (fn_40_b): Likewise. (fn_41_a): Likewise. (fn_41_b): Likewise. --- gcc/c-family/c-indentation.c | 16 +++-- .../c-c++-common/Wmisleading-indentation.c | 79 ++ 2 files changed, 89 insertions(+), 6 deletions(-) diff
[PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)
Comment #1 of PR c/68187 identified another overzealous warning from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly indented code: 115if (locked) 116i = CRYPTO_add(>struct_ref, -1, CRYPTO_LOCK_ENGINE); 117else 118i = --e->struct_ref; 119engine_ref_debug(e, 0, -1) 120if (i > 0) 121return 1; eng_lib.c:120:9: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation] if (i > 0) ^~ eng_lib.c:117:5: note: ...this 'else' clause, but it is not else ^~~~ Line 120 is poorly indented, but the warning is arguably unjustified. Root cause is that "engine_ref_debug" is actually a debugging macro that was empty in the given configuration, so the code effectively was: 117else // GUARD 118i = --e->struct_ref; // BODY 119 120if (i > 0)// NEXT hence the warning. But the code as seen by a human is clearly *not* misleading, and hence arguably we shouldn't warn for this case. The following patch fixes this by ruling that if there is non-whitespace in a line between the BODY and the NEXT statements, and that this non-whitespace is effectively an "unindent" or "outdent" (it's not clear to me which of these terms is better), then to not issue a warning. In doing so I eliminated one of the existing heuristics: we already had code to ignore preprocessor directives between BODY and NEXT for cases like this: if (flagA) // GUARD foo (0); // BODY #if SOME_CONDITION_THAT_DOES_NOT_HOLD if (flagB) #endif foo (1); // NEXT This is handled by the new heuristic, so the new heuristic simply replaces the old one. Sadly the replacement of two old functions with two new functions leads to a rather messy diff within c-indentation.c; I can split it up into a removal/addition pair of patches if that's easier to review. Successfully bootstrapped on x86_64-pc-linux-gnu (in combination with the previous patch). OK for trunk? Note: one of the new test cases adds a dg-warning/dg-message pair, and so would require updating if/when the wording change posted here: https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00068.html ("[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation") is applied. gcc/c-family/ChangeLog: PR c/68187 * c-indentation.c (get_visual_column): Move code to determine next tab stop to... (next_tab_stop): ...this new function. (line_contains_hash_if): Delete function. (detect_preprocessor_logic): Delete function. (get_first_nws_vis_column): New function. (detect_intervening_unindent): New function. (should_warn_for_misleading_indentation): Replace call to detect_preprocessor_logic with a call to detect_intervening_unindent. gcc/testsuite/ChangeLog: PR c/68187 * c-c++-common/Wmisleading-indentation.c (fn_42_a): New test function. (fn_42_b): Likewise. (fn_42_c): Likewise. --- gcc/c-family/c-indentation.c | 141 - .../c-c++-common/Wmisleading-indentation.c | 72 +++ 2 files changed, 152 insertions(+), 61 deletions(-) diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c index c72192d..b84fbf4 100644 --- a/gcc/c-family/c-indentation.c +++ b/gcc/c-family/c-indentation.c @@ -26,6 +26,16 @@ along with GCC; see the file COPYING3. If not see extern cpp_options *cpp_opts; +/* Round up VIS_COLUMN to nearest tab stop. */ + +static unsigned int +next_tab_stop (unsigned int vis_column) +{ + const unsigned int tab_width = cpp_opts->tabstop; + vis_column = ((vis_column + tab_width) / tab_width) * tab_width; + return vis_column; +} + /* Convert libcpp's notion of a column (a 1-based char count) to the "visual column" (0-based column, respecting tabs), by reading the relevant line. @@ -77,11 +87,7 @@ get_visual_column (expanded_location exploc, location_t loc, } if (ch == '\t') - { -/* Round up to nearest tab stop. */ -const unsigned int tab_width = cpp_opts->tabstop; -vis_column = ((vis_column + tab_width) / tab_width) * tab_width; - } + vis_column = next_tab_stop (vis_column); else vis_column++; } @@ -93,54 +99,49 @@ get_visual_column (expanded_location exploc, location_t loc, return true; } -/* Does the given source line appear to contain a #if directive? - (or #ifdef/#ifndef). Ignore the possibility of it being inside a - comment, for simplicity. - Helper function for detect_preprocessor_logic. */ +/* Attempt to determine the first non-whitespace character in line LINE_NUM + of source line FILE. + + If this is possible, return true and write its "visual column" to + *FIRST_NWS. + Otherwise, return false, leaving *FIRST_NWS untouched. */ static bool -line_contains_hash_if (const char *file, int line_num)
Re: Proposed Patch for Bug 69687
Thanks Mike. I have revised the patch and removed the limits. While perhaps less security critical, without the limits on the loop count (r) the test cases will still consume all your memory and effectively freeze GDB. * Before any realloc, check for overflow. * string_need now returns 1 if the allocation was successful. * all clients of string_need refrain from extending the string anything if string_need was unsuccessful. > better use unsigned values that are large enough to never overflow. Throughout cplus-dem.c, the length of a string is measured as pointer difference. So, technically length is of type ptrdiff_t which is signed. — a/libiberty/cplus-dem.c +++ b/libiberty/cplus-dem.c @@ -55,6 +55,7 @@ void * malloc (); void * realloc (); #endif +#include #include #undef CURRENT_DEMANGLING_STYLE @@ -379,7 +380,7 @@ static int arm_special (const char **, string *); -static void string_need (string *, int); +static int string_need (string *, int); static void string_delete (string *); @@ -4254,7 +4255,9 @@ } else { - work -> typevec_size *= 2; + if (work -> typevec_size > INT_MAX / 2) +return; + work -> typevec_size *= 2; work -> typevec = XRESIZEVEC (char *, work->typevec, work->typevec_size); } @@ -4281,7 +4284,9 @@ } else { - work -> ksize *= 2; + if (work -> ksize > INT_MAX / 2) +return; + work -> ksize *= 2; work -> ktypevec = XRESIZEVEC (char *, work->ktypevec, work->ksize); } @@ -4294,7 +4299,8 @@ /* Register a B code, and get an index for it. B codes are registered as they are seen, rather than as they are completed, so map- registers map as B0, and temp as B1 */ + registers map as B0, and temp as B1. Returns -1 + if registration was unsuccessful. */ static int register_Btype (struct work_stuff *work) @@ -4310,7 +4316,9 @@ } else { - work -> bsize *= 2; + if (work -> bsize > INT_MAX / 2) +return -1; + work -> bsize *= 2; work -> btypevec = XRESIZEVEC (char *, work->btypevec, work->bsize); } @@ -4328,6 +4336,8 @@ { char *tem; + if (index < 0) +return; tem = XNEWVEC (char, len + 1); memcpy (tem, start, len); tem[len] = '\0'; @@ -4591,7 +4601,8 @@ const char *tem; string_appendn (declp, (*mangled), scan - (*mangled)); - string_need (declp, 1); + if (! string_need (declp, 1)) + return 0; *(declp -> p) = '\0'; /* Consume the function name, including the "__" separating the name @@ -4747,7 +4758,7 @@ /* a mini string-handling package */ -static void +static int string_need (string *s, int n) { int tem; @@ -4765,11 +4776,14 @@ { tem = s->p - s->b; n += tem; + if ( n > INT_MAX / 2) +return 0; n *= 2; s->b = XRESIZEVEC (char, s->b, n); s->p = s->b + tem; s->e = s->b + n; } +return 1; } static void @@ -4811,7 +4825,8 @@ if (s == NULL || *s == '\0') return; n = strlen (s); - string_need (p, n); + if (! string_need (p, n)) +return; memcpy (p->p, s, n); p->p += n; } @@ -4824,7 +4839,8 @@ if (s->b != s->p) { n = s->p - s->b; - string_need (p, n); + if (! string_need (p, n)) +return; memcpy (p->p, s->b, n); p->p += n; } @@ -4835,7 +4851,8 @@ { if (n != 0) { - string_need (p, n); + if (! string_need (p, n)) +return; memcpy (p->p, s, n); p->p += n; } @@ -4866,7 +4883,8 @@ if (n != 0) { - string_need (p, n); + if (! string_need (p, n)) +return; for (q = p->p - 1; q >= p->b; q--) { q[n] = q[0];
Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level
On Thu, Mar 3, 2016 at 9:22 AM, Markus Trippelsdorfwrote: > On 2016.03.03 at 09:16 -0500, Patrick Palka wrote: >> push_to_top_level gets called fairly frequently in template-heavy code >> that performs a lot of instantiations, and we currently "leak" a lot of >> GC memory when compiling such code since [push|pop]_to_top_level() do >> not bother reusing or even freeing each saved_scope structure it >> allocates. >> >> This patch makes push_to_top_level() reuse the saved_scope structures it >> allocates. This is similar to how begin_scope() reuses the >> cp_binding_level structures it allocates. >> >> This patch reduces the maximum memory usage of the compiler by 4.5%, >> from 525MB to 500MB, when compiling the Boost::Fusion test file >> libs/fusion/test/compile_time/transform.cpp from the Boost 1.60 testsuite. >> >> Bootstrapped and tested on x86_64-pc-linux-gnu, OK for >> trunk or for GCC 7? > > Great. push_to_top_level also shows up very high in profiles when > building Chromium for example. > > There is an old bug for this issue: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64500 > > -- > Markus I forgot what exactly I was benchmarking but I also saw push_to_top_level high on the list which is what made me interested in this function in the first place. I think the slowness of this function is mostly due to the pointer chasing performed in the function store_bindings, where we iterate over all the names in each non-global scope to figure out whether to preserve them. It would probably improve performance if cp_binding_level::names were a vector of trees instead of a linked list of trees.
Patch ping
Hi! I'd like to ping fix for P1 PR69947: https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01743.html Jakub
Re: [PATCH] Specify that new ports should use LRA
On 2 March 2016 at 21:47, H.J. Luwrote: > On Wed, Mar 2, 2016 at 12:18 PM, Manuel López-Ibáñez > wrote: >> Pre-approved by Jeff here: >> https://gcc.gnu.org/ml/gcc-help/2016-03/msg6.html >> >> Committed as revision 233914. > > I checked in this missing patch. Thanks H.J.!
Re: [C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: > This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so > e.g. > _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword > is parsed using recursive call in c_parser_postfix_expression (case > RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a > typename, so it thinks we're inside a compound literal, which means it expects > '{', but that isn't there, so we crash on the assert in c_parser_braced_init: > gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); > But as the comment in c_parser_postfix_expression says, the code for parsing > a compound literal here is likely dead. I made an experiment and added > gcc_unreachable () in that block, ran regtest, and there were no failures. > Thus it should be safe to just remove the code, which also fixes this ICE; > with > the patch we just give a proper error and don't crash anymore. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly > nervous about the change, so maybe better table until gcc7? This reminds me of PR67495. Perhaps the answer here is also during the _Cilk* stuff parsing don't call c_parser_postfix_expression, but call c_parser_cast_expression instead and then verify what it got? > 2016-03-03 Marek Polacek> > PR c/69798 > * c-parser.c (c_parser_postfix_expression): Remove code dealing with > compound literals. > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > * gcc.dg/cilk-plus/pr69798-2.c: New test. Jakub
Re: [PATCH] Reuse the saved_scope structures allocated by push_to_top_level
On 2016.03.03 at 09:16 -0500, Patrick Palka wrote: > push_to_top_level gets called fairly frequently in template-heavy code > that performs a lot of instantiations, and we currently "leak" a lot of > GC memory when compiling such code since [push|pop]_to_top_level() do > not bother reusing or even freeing each saved_scope structure it > allocates. > > This patch makes push_to_top_level() reuse the saved_scope structures it > allocates. This is similar to how begin_scope() reuses the > cp_binding_level structures it allocates. > > This patch reduces the maximum memory usage of the compiler by 4.5%, > from 525MB to 500MB, when compiling the Boost::Fusion test file > libs/fusion/test/compile_time/transform.cpp from the Boost 1.60 testsuite. > > Bootstrapped and tested on x86_64-pc-linux-gnu, OK for > trunk or for GCC 7? Great. push_to_top_level also shows up very high in profiles when building Chromium for example. There is an old bug for this issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64500 -- Markus
Re: Proposed Patch for Bug 69687
On 03/02/2016 06:22 PM, Mike Stump wrote: So, check for overflow, or better use unsigned values that are large enough to never overflow. With no possibility for overflow, you can then retest the bug and see if there are any other failure modes and fix those. What C standard can we assume for libiberty? I was looking at patching this and discovered that SIZE_MAX is defined only for C99, so I'm leaning towards retaining the ints and using INT_MAX. Bernd
[PATCH] Reuse the saved_scope structures allocated by push_to_top_level
push_to_top_level gets called fairly frequently in template-heavy code that performs a lot of instantiations, and we currently "leak" a lot of GC memory when compiling such code since [push|pop]_to_top_level() do not bother reusing or even freeing each saved_scope structure it allocates. This patch makes push_to_top_level() reuse the saved_scope structures it allocates. This is similar to how begin_scope() reuses the cp_binding_level structures it allocates. This patch reduces the maximum memory usage of the compiler by 4.5%, from 525MB to 500MB, when compiling the Boost::Fusion test file libs/fusion/test/compile_time/transform.cpp from the Boost 1.60 testsuite. Bootstrapped and tested on x86_64-pc-linux-gnu, OK for trunk or for GCC 7? gcc/cp/ChangeLog: * name-lookup.c (free_saved_scope): New free list of saved_scope structures. (push_to_top_level): Attempt to reuse a saved_scope struct from free_saved_scope instead of allocating a new one each time. (pop_from_top_level_1): Chain the now-unused saved_scope structure onto free_saved_scope. --- gcc/cp/name-lookup.c | 25 - 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 89d84d7..3478b6a 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -6134,6 +6134,10 @@ store_class_bindings (vec*names, timevar_cond_stop (TV_NAME_LOOKUP, subtime); } +/* A chain of saved_scope structures awaiting reuse. */ + +static GTY((deletable)) struct saved_scope *free_saved_scope; + void push_to_top_level (void) { @@ -6144,7 +6148,21 @@ push_to_top_level (void) bool need_pop; bool subtime = timevar_cond_start (TV_NAME_LOOKUP); - s = ggc_cleared_alloc (); + + /* Reuse or create a new structure for this saved scope. */ + if (free_saved_scope != NULL) +{ + s = free_saved_scope; + free_saved_scope = s->prev; + + vec *old_bindings = s->old_bindings; + memset (s, 0, sizeof (*s)); + /* Also reuse the structure's old_bindings vector. */ + vec_safe_truncate (old_bindings, 0); + s->old_bindings = old_bindings; +} + else +s = ggc_cleared_alloc (); b = scope_chain ? current_binding_level : 0; @@ -6237,6 +6255,11 @@ pop_from_top_level_1 (void) current_function_decl = s->function_decl; cp_unevaluated_operand = s->unevaluated_operand; c_inhibit_evaluation_warnings = s->inhibit_evaluation_warnings; + + /* Make this saved_scope structure available for reuse by + push_to_top_level. */ + s->prev = free_saved_scope; + free_saved_scope = s; } /* Wrapper for pop_from_top_level_1. */ -- 2.8.0.rc0.11.g9bfbc33
[C PATCH] Fix ICE on invalid Cilk+ code (PR c/69798)
This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword is parsed using recursive call in c_parser_postfix_expression (case RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a typename, so it thinks we're inside a compound literal, which means it expects '{', but that isn't there, so we crash on the assert in c_parser_braced_init: gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); But as the comment in c_parser_postfix_expression says, the code for parsing a compound literal here is likely dead. I made an experiment and added gcc_unreachable () in that block, ran regtest, and there were no failures. Thus it should be safe to just remove the code, which also fixes this ICE; with the patch we just give a proper error and don't crash anymore. Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly nervous about the change, so maybe better table until gcc7? 2016-03-03 Marek PolacekPR c/69798 * c-parser.c (c_parser_postfix_expression): Remove code dealing with compound literals. * gcc.dg/cilk-plus/pr69798-1.c: New test. * gcc.dg/cilk-plus/pr69798-2.c: New test. diff --git gcc/c/c-parser.c gcc/c/c-parser.c index bb508b7..9e8ac1b 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -7512,28 +7512,6 @@ c_parser_postfix_expression (c_parser *parser) set_c_expr_source_range (, loc, close_loc); mark_exp_read (expr.value); } - else if (c_token_starts_typename (c_parser_peek_2nd_token (parser))) - { - /* A compound literal. ??? Can we actually get here rather -than going directly to -c_parser_postfix_expression_after_paren_type from -elsewhere? */ - location_t loc; - struct c_type_name *type_name; - c_parser_consume_token (parser); - loc = c_parser_peek_token (parser)->location; - type_name = c_parser_type_name (parser); - c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, -"expected %<)%>"); - if (type_name == NULL) - { - expr.value = error_mark_node; - } - else - expr = c_parser_postfix_expression_after_paren_type (parser, -type_name, -loc); - } else { /* A parenthesized expression. */ diff --git gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c index e69de29..1120193 100644 --- gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c +++ gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c @@ -0,0 +1,12 @@ +/* PR c/69798 */ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ + +int +main () +{ + _Cilk_spawn (void); /* { dg-error "expected expression" } */ + _Cilk_spawn (char []); /* { dg-error "expected expression" } */ + _Cilk_spawn (int *); /* { dg-error "expected expression" } */ + _Cilk_spawn ({}); /* { dg-error "only function calls can be spawned" } */ +} diff --git gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c index e69de29..66bcdc8 100644 --- gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c +++ gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c @@ -0,0 +1,11 @@ +/* PR c/69798 */ +/* { dg-do compile } */ + +int +main () +{ + _Cilk_spawn (void); /* { dg-error "expected expression" } */ + _Cilk_spawn (char []); /* { dg-error "expected expression" } */ + _Cilk_spawn (int *); /* { dg-error "expected expression" } */ + _Cilk_spawn ({}); /* { dg-error "only function calls can be spawned" } */ +} Marek
Re: [Ping^2][PATCH][GCC-5] Fix "#pragma GCC pop_options" warning.
On 03/03/16 12:11, Bernd Schmidt wrote: > On 03/03/2016 11:45 AM, Andre Vieira (lists) wrote: >> On 29/02/16 10:47, Andre Vieira (lists) wrote: >>> On 15/02/16 10:33, Andre Vieira (lists) wrote: On 18/01/16 11:04, Andre Vieira (lists) wrote: > Hi there, > > Can we have the "#pragma GCC pop_options" fix backported to GCC-5? > > Patch found in > https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01261.html > and was committed in r228794. > > The same patch applies cleanly to gcc-5, which would otherwise not be > able to use this pragma even though the support is there. > >> I understood it was a good idea to CC the appropriate maintainer on >> this, so adding Bernd Schmidt to the CC. > > Yeah, I think I remember this one. Ok. > > > Bernd > Thomas committed on my behalf at revision r233939. 2016-03-03 Andre VieiraBackport from mainline 2015-10-14 Dominik Vogt Fix "#pragma GCC pop_options" gcc/ChangeLog * targhooks.c (default_target_option_pragma_parse): Do not warn if called on behalf of "#pragma GCC pop_options". gcc/testsuite/ChangeLog * gcc.dg/pragma-pop_options-1.c: New test. Thank you Thomas and Bernd. Cheers, Andre
Re: [PATCH][ARM] PR target/70008
On 03/03/16 07:23, Michael Collison wrote: > I have attached a new patch which hopefully address Richard's concerns. > I modified the predicate on operand 1 to to "arm_rhs_operand" to be > consistent with the constraints. I retained the split into two patterns; > one for arm and another for thumb2. I thought this was cleaner. > > Okay for trunk? > > 2016-02-28 Michael Collison> > PR target/70008 > * config/arm/arm.md (*subsi3_carryin): Change predicate to > arm_rhs_operand to be consistent with constraints. > Only allow pattern for TARGET_ARM. > * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern. > I don't think we need two patterns. We could share this if we had a new predicate that was called something like reg_or_arm_rhs_operand, with a rule that's something like: register_operand (op) || (TARGET_ARM && arm_immediate_operand (op)) R. > On 02/29/2016 08:29 AM, Richard Earnshaw (lists) wrote: >> On 29/02/16 11:21, Michael Collison wrote: >>> >>> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote: Hi Michael, On 29/02/16 04:47, Michael Collison wrote: > This patches address PR 70008, where a reverse subtract with carry > instruction can be generated in thumb2 mode. It was tested with no > regressions in arm and thumb modes on the following targets: > > arm-none-linux-gnueabi > arm-none-linux-gnuabihf > armeb-none-linux-gnuabihf > arm-none-eabi > > Okay for trunk? > > 2016-02-28 Michael Collison > > PR target/70008 > * config/arm/arm.md (*subsi3_carryin): Only match pattern if > TARGET_ARM due to 'rsc' instruction alternative. > * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern. > > The *subsi3_carrying pattern has the arch attribute: (set_attr "arch" "*,a") That means that the second alternative that generates the RSC instruction is only enabled for ARM mode. Do you have a testcase where this doesn't happen and this pattern generates the second alternative for Thumb2? >>> No I don't have a test case; i noticed the pattern when working on the >>> overflow project. I did not realize >>> that an attribute could affect the matching of an alternative. I will >>> close the bug. >>> Thanks, Kyrill >> This is all true, but there is a potential performance issue with this >> pattern though, that could lead to sub-optimal code. >> >> The predicate accepts reg-or-int, but in ARM state only simple >> 'const-ok-for-arm' immediates are permitted by the predicates, and in >> thumb code, no immediates are permitted at all. This could potentially >> result in sub-optimal code due to late splitting of the pattern. It >> would be better if the predicate understood these limitations and >> restricted immediates accordingly. >> >> R. >> > > > bugzilla-70008-upstream-v2.patch > > > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md > index e67239d..e6bcd7f 100644 > --- a/gcc/config/arm/arm.md > +++ b/gcc/config/arm/arm.md > @@ -867,15 +867,14 @@ > > (define_insn "*subsi3_carryin" >[(set (match_operand:SI 0 "s_register_operand" "=r,r") > -(minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand" "r,I") > +(minus:SI (minus:SI (match_operand:SI 1 "arm_rhs_operand" "r,I") > (match_operand:SI 2 "s_register_operand" "r,r")) >(ltu:SI (reg:CC_C CC_REGNUM) (const_int 0] > - "TARGET_32BIT" > + "TARGET_ARM" >"@ > sbc%?\\t%0, %1, %2 > rsc%?\\t%0, %2, %1" >[(set_attr "conds" "use") > - (set_attr "arch" "*,a") > (set_attr "predicable" "yes") > (set_attr "predicable_short_it" "no") > (set_attr "type" "adc_reg,adc_imm")] > diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md > index 9925365..79305c5 100644 > --- a/gcc/config/arm/thumb2.md > +++ b/gcc/config/arm/thumb2.md > @@ -848,6 +848,20 @@ > (set_attr "type" "multiple")] > ) > > +(define_insn "*thumb2_subsi3_carryin" > + [(set (match_operand:SI 0 "s_register_operand" "=r") > +(minus:SI (minus:SI (match_operand:SI 1 "s_register_operand" "r") > +(match_operand:SI 2 "s_register_operand" "r")) > + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0] > + "TARGET_THUMB2" > + "@ > + sbc%?\\t%0, %1, %2" > + [(set_attr "conds" "use") > + (set_attr "predicable" "yes") > + (set_attr "predicable_short_it" "no") > + (set_attr "type" "adc_reg")] > +) > + > (define_insn "*thumb2_cond_sub" >[(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts") > (minus:SI (match_operand:SI 1 "s_register_operand" "0,?Ts") >
Re: [committed] Fix libffi/70024
Hi Richard, > Unfortunately, even with this fixed, all Solaris/x86 tests now fail to > link: > > FAIL: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 (test for excess > errors) > Excess errors: > Undefined first referenced > symbol in file > ffi_closure_alloc /var/tmp//ccIpq3qc.o > ffi_type_float /var/tmp//ccIpq3qc.o > ffi_type_uint64 /var/tmp//ccIpq3qc.o > ffi_type_sint32 /var/tmp//ccIpq3qc.o > ffi_type_sint16 /var/tmp//ccIpq3qc.o > ffi_type_double /var/tmp//ccIpq3qc.o > ffi_prep_cif/var/tmp//ccIpq3qc.o > ld: fatal: symbol referencing errors turned out to be easy, too: make_sunver.pl got passed a list of non-existant object files: .libs/src/prep_cif.o .libs/src/types.o .libs/src/raw_api.o .libs/src/java_raw_api.o .libs/src/closures.o Treating $(libffi_la_OBJECTS) and $(libffi_la_LIBADD) the same fixed this so all the symbols above got included in libffi.map-sun and all tests pass. Tested on i386-pc-solaris2.12, installed on mainline. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University 2016-03-03 Rainer Orth* Makefile.am (libffi.map-sun): Properly convert $(libffi_la_OBJECTS) to object names. * Makefile.in: Regenerate. # HG changeset patch # Parent ce4aa09ea32c03f4d0ad44415ce066a861bda6c3 Fix passing object names to make_sunver.pl diff --git a/libffi/Makefile.am b/libffi/Makefile.am --- a/libffi/Makefile.am +++ b/libffi/Makefile.am @@ -215,8 +215,7 @@ libffi_version_dep = libffi.map-sun libffi.map-sun : libffi.map $(top_srcdir)/../contrib/make_sunver.pl \ $(libffi_la_OBJECTS) $(libffi_la_LIBADD) perl $(top_srcdir)/../contrib/make_sunver.pl libffi.map \ - $(libffi_la_OBJECTS:%.lo=.libs/%.o) \ - `echo $(libffi_la_LIBADD) | \ + `echo $(libffi_la_OBJECTS) $(libffi_la_LIBADD) | \ sed 's,\([^/]*\)\.l\([ao]\),.libs/\1.\2,g'` \ > $@ || (rm -f $@ ; exit 1) endif
Re: [PATCH] Fix up vect pattern handling (PR target/70021)
On Wed, 2 Mar 2016, Jakub Jelinek wrote: > Hi! > > This patch fixes two issues: > 1) reverts part of https://gcc.gnu.org/ml/gcc-patches/2011-06/msg02183.html >changes, my understanding is that we don't emit or support what has been >mentioned as rationale for that, now that we can stick multiple pattern >stmts into a sequence; without this reversion, we mark both the pattern >and normal stmt relevant and then when vectorizing use in one spot >the pattern stmt and in another one the original stmt, while we clearly >want to use the pattern stmt always; also, without the reversion we >consider the original stmt as needed for VF computation and thus think >the loop needs QImode elements in addition to SImode and DImode. > 2) if the shift count is coming from stmt with corresponding pattern stmt, >we shouldn't look through it, because we might again refer to the middle >of a pattern stmt (this is similar to the recently committed patch); we >don't need to punt completely in that case though, the code can just >add a cast into the pattern sequence as it does in many other cases. > > Bootstrapped/regtested on {x86_64,i686,powerpc64,powerpc64le}-linux, > bootstrap/regtest is still pending on {s390,s390x,aarch64,armv7hl}-linux, > ok for trunk if it passes even there? Ok. I've been trying to understand this code multiple times myself and I'm happy to see it go ;) Even though in all cases I remember the issue was elsewhere... Thanks, Richard. > 2016-03-02 Jakub Jelinek> > PR target/70021 > * tree-vect-stmts.c (vect_mark_relevant): Remove USED_IN_PATTERN > argument, if STMT_VINFO_IN_PATTERN_P (stmt_info), always mark > the pattern no matter if it is used just by non-pattern, pattern > or mix thereof. > (process_use, vect_mark_stmts_to_be_vectorized): Adjust callers. > * tree-vect-patterns.c (vect_recog_vector_vector_shift_pattern): If > oprnd1 def_stmt is in pattern, don't look through it. > > * gcc.dg/vect/pr70021.c: New test. > * gcc.target/i386/pr70021.c: New test. > > --- gcc/tree-vect-stmts.c.jj 2016-03-01 19:23:51.0 +0100 > +++ gcc/tree-vect-stmts.c 2016-03-02 15:40:53.777231771 +0100 > @@ -181,8 +181,7 @@ create_array_ref (tree type, tree ptr, s > > static void > vect_mark_relevant (vec *worklist, gimple *stmt, > - enum vect_relevant relevant, bool live_p, > - bool used_in_pattern) > + enum vect_relevant relevant, bool live_p) > { >stmt_vec_info stmt_info = vinfo_for_stmt (stmt); >enum vect_relevant save_relevant = STMT_VINFO_RELEVANT (stmt_info); > @@ -202,62 +201,22 @@ vect_mark_relevant (vec *workl > stmt itself should be marked. */ >if (STMT_VINFO_IN_PATTERN_P (stmt_info)) > { > - bool found = false; > - if (!used_in_pattern) > -{ > - imm_use_iterator imm_iter; > - use_operand_p use_p; > - gimple *use_stmt; > - tree lhs; > - loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info); > - struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo); > - > - if (is_gimple_assign (stmt)) > -lhs = gimple_assign_lhs (stmt); > - else > -lhs = gimple_call_lhs (stmt); > - > - /* This use is out of pattern use, if LHS has other uses that are > - pattern uses, we should mark the stmt itself, and not the > pattern > - stmt. */ > - if (lhs && TREE_CODE (lhs) == SSA_NAME) > - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, lhs) > - { > - if (is_gimple_debug (USE_STMT (use_p))) > - continue; > - use_stmt = USE_STMT (use_p); > - > - if (!flow_bb_inside_loop_p (loop, gimple_bb (use_stmt))) > - continue; > - > - if (vinfo_for_stmt (use_stmt) > - && STMT_VINFO_IN_PATTERN_P (vinfo_for_stmt (use_stmt))) > - { > - found = true; > - break; > - } > - } > -} > + /* This is the last stmt in a sequence that was detected as a > + pattern that can potentially be vectorized. Don't mark the stmt > + as relevant/live because it's not going to be vectorized. > + Instead mark the pattern-stmt that replaces it. */ > > - if (!found) > -{ > - /* This is the last stmt in a sequence that was detected as a > - pattern that can potentially be vectorized. Don't mark the stmt > - as relevant/live because it's not going to be vectorized. > - Instead mark the pattern-stmt that replaces it. */ > - > - pattern_stmt = STMT_VINFO_RELATED_STMT (stmt_info); > - > - if (dump_enabled_p ()) > -dump_printf_loc (MSG_NOTE, vect_location, > - "last stmt in pattern. don't mark" > -
Re: [PATCH] Fix ICE with vector types in X % -Y pattern (PR middle-end/70050)
On Thu, Mar 03, 2016 at 01:56:05PM +0100, Marc Glisse wrote: > On Thu, 3 Mar 2016, Marek Polacek wrote: > > >We crashed on the attached testcase because we were trying to apply the > >X % -Y -> X % Y transformation even on vectors. That doesn't go well with > >the > >check for !TYPE_UNSIGNED. So I think let's limit the pattern to only work on > >integral types. > > I certainly hope the problem is not with TYPE_UNSIGNED, I think there are > many places where we use it on vectors. I would expect the issue to be with > TYPE_MIN_VALUE a few lines below. Not that it changes anything to the fix... > > (we can revisit that if vectors ever get VRP support) And expr_not_equal_to would need to be tought to use that... Jakub
Re: [PATCH] Fix ICE with vector types in X % -Y pattern (PR middle-end/70050)
On Thu, Mar 03, 2016 at 01:56:05PM +0100, Marc Glisse wrote: > On Thu, 3 Mar 2016, Marek Polacek wrote: > > >We crashed on the attached testcase because we were trying to apply the > >X % -Y -> X % Y transformation even on vectors. That doesn't go well with > >the > >check for !TYPE_UNSIGNED. So I think let's limit the pattern to only work on > >integral types. > > I certainly hope the problem is not with TYPE_UNSIGNED, I think there are > many places where we use it on vectors. I would expect the issue to be with > TYPE_MIN_VALUE a few lines below. Not that it changes anything to the fix... Yes, as Jakub already pointed out. My bad, sorry about that. Marek
Re: [PATCH] Fix ICE with vector types in X % -Y pattern (PR middle-end/70050)
On Thu, 3 Mar 2016, Marek Polacek wrote: We crashed on the attached testcase because we were trying to apply the X % -Y -> X % Y transformation even on vectors. That doesn't go well with the check for !TYPE_UNSIGNED. So I think let's limit the pattern to only work on integral types. I certainly hope the problem is not with TYPE_UNSIGNED, I think there are many places where we use it on vectors. I would expect the issue to be with TYPE_MIN_VALUE a few lines below. Not that it changes anything to the fix... (we can revisit that if vectors ever get VRP support) Bootstrapped/regtested on x86_64-linux, ok for trunk? 2016-03-03 Marek PolacekPR middle-end/70050 * match.pd (X % -Y): Add INTEGRAL_TYPE_P check. * gcc.dg/pr70050.c: New test. diff --git gcc/match.pd gcc/match.pd index 5903782..112deb3 100644 --- gcc/match.pd +++ gcc/match.pd @@ -293,7 +293,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) /* X % -Y is the same as X % Y. */ (simplify (trunc_mod @0 (convert? (negate @1))) - (if (!TYPE_UNSIGNED (type) + (if (INTEGRAL_TYPE_P (type) + && !TYPE_UNSIGNED (type) && !TYPE_OVERFLOW_TRAPS (type) && tree_nop_conversion_p (type, TREE_TYPE (@1)) /* Avoid this transformation if X might be INT_MIN or diff --git gcc/testsuite/gcc.dg/pr70050.c gcc/testsuite/gcc.dg/pr70050.c index e69de29..610456f 100644 --- gcc/testsuite/gcc.dg/pr70050.c +++ gcc/testsuite/gcc.dg/pr70050.c @@ -0,0 +1,11 @@ +/* PR middle-end/70025 */ +/* { dg-do compile } */ +/* { dg-options "-Wno-psabi" } */ + +typedef int v8si __attribute__ ((vector_size (32))); + +v8si +foo (v8si v) +{ + return v %= -v; +} -- Marc Glisse
Re: [PATCH]Replace -shared with -r -nostdlib in gcc.dg/lto/pr61526 pr54709 pr64415 test cases.
On Thu, Mar 3, 2016 at 1:07 PM, Renlin Liwrote: > Hi Richard, > > > On 03/03/16 10:13, Richard Biener wrote: >> >> On Wed, Mar 2, 2016 at 5:12 PM, Renlin Li wrote: >>> >>> Hi Richard, >>> >>> >>> On 02/03/16 13:35, Richard Biener wrote: On Tue, Mar 1, 2016 at 4:56 PM, Renlin Li wrote: > > Hi Richard, > > > On 01/03/16 09:16, Richard Biener wrote: >> >> On Mon, Feb 29, 2016 at 5:13 PM, Renlin Li >> wrote: >>> >>> Hi all, >>> >>> The gcc.dg/lto/pr54709, pr61526, pr64415 linking testcases keep >>> failing >>> on >>> arm/aarch64 bare-metal target. >>> >>> It's because statically built newlib library is used to link with >>> shared >>> object. >>> And the linker complains about relocations which cannot be used in >>> shared object. >>> >>> For example, the following errors are produced: >>> >>> crtbegin.o: relocation R_ARM_MOVW_ABS_NC against `a local symbol' can >>> not >>> be >>> used when making a shared object; recompile with -fPIC >>> >>> crtbegin.o: relocation R_ARM_THM_MOVW_ABS_NC against `a local symbol' >>> can >>> not >>> be used when making a shared object; recompile with -fPIC >>> >>> librdimon.a(rdimon-syscalls.o): relocation R_AARCH64_ADR_PREL_PG_HI21 >>> against >>> external symbol `_impure_ptr' can not be used when making a shared >>> object; >>> recompile with -fPIC >>> >>> Presumably, bare-metal toolchain for other architecture have those >>> test >>> case >>> failures as well? >>> >>> In this patch, -shared option is replace by -r -nostdlib. So that the >>> standard >>> system startup files or libraries are not used when linking. >> >> Note that -shared is not equivalent to -r -nostdlib so please verify >> that >> the >> original issue can be still reproduced with its fix reverted but -r >> -nostdlib >> used with the new -r -nostdlib handling on trunk. > > > pr54709_0.c: Cannot be reproduced with even -shared. The error message > is > the same as shown above. > pr64415_0.c: Reproduced with "-r -nostdlib". > pr61526_0.c: Reproduced with "-r -nostdlib". > > By the way, those linking test cases all pass for linux toolchain. Only > fail > for aarch64/arm baremetal toolchain. > > Andrew, I saw you have done similar things in r153555 > https://gcc.gnu.org/viewcvs/gcc?view=revision=153555 > > Do you have any thoughts? > > And also here, the last comments in this ticket suggests to add > check_effective_target_shared to the exp file to limit it to linux > targets > only: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61526 As said LTO testcases tend to be somewhat fragile so limiting them to targets known to work might be the best option. Richard. >>> >>> >>> Forgot the mention that, by purely adding "-nostdlib" option (instead of >>> replacing -shared) >>> fixes the failures as well. >>> >>> I test those test cases again with fix reverted, keep "-shared" option, >>> add >>> "-nostdlib" option. >> >> Ok, so I discovered we have a "shared" target which means if a target >> doesn't >> support shared libs we can guard against it with using >> >> /* { dg-require-effective-target shared } */ >> >> does adding that to the three testcases fix the issue for you? > > By adding this target check > /* { dg-require-effective-target shared } */ > > Those test cases aredeemed to be unsupported, and thus skipped for > aarch64-none-elf target. > > However, it's a little bit tricky for arm bare-metal target. > > The shared option check actually successes for arm-none-eabi toolchain. > This is because the default cpu for arm-none-eabi toolchain is arm7tdmi. And > the start file crtbegin.o doesn't contains any modifications not allowed in > shared object. > > arm-none-eabi is built with multilib. When running this testcase, it's > compiled with "-march=armv7-a". > The crtbegin.o for this architecture version contains relocations which > cannot be used in shared object. > That's why they fails to linking test. For -shared it should provide a crtbeginS.o then. Why not fix it properly? Richard. > Will adding "-nostdlib" (instead of replace -shared) option be an reasonable > fix given my previous check? > > Regards, > Renlin > > > >> >> Thanks, >> Richard. >> >>> pr54709_0.c: Cannot be reproduced even with test case unmodified. >>> The error message is the same as shown above. with "-nostdlib", no >>> failure. >>> >>> pr64415_0.c: Reproduced. >>> pr61526_0.c: Reproduced. >>> >>> Regards, >>> Renlin >>> >>> >>> > Regards, > Renlin > > >> Otherwise simply dg-skip for aarch64. >> >> Richard. >> >>> arm-none-eabi, aarch64-none-elf regression test
[PATCH] Fix PR70054
The following patch adjusts strict_aliasing_warning to use proper alias_set_subset_of instead of relying on alias_sets_conflict_p as after the PR66110 fix aggregates with a char[] member do not automatically behave like having alias-set zero. As a side-effect the test will be somewhat stricter as well accessing a 'long' with a struct { int i; long l; } * will now warn while it previously didn't: struct S { int i; long l; }; long x; struct S foo () { return *(struct S *) } now warns: t.c: In function ‘foo’: t.c:3:35: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] struct S foo () { return *(struct S *) } ^ Bootstrap and regtest running on x86_64-unknown-linux-gnu, ok for trunk? Thanks, Richard. 2016-03-03 Richard BienerPR c++/70054 * c-common.c (strict_aliasing_warning): Use alias_set_subset_of instead of alias_sets_conflict_p. * g++.dg/warn/Wstrict-aliasing-bogus-union-2.C: New testcase. * gcc.dg/Wstrict-aliasing-struct-member.c: New testcase. Index: gcc/c-family/c-common.c === *** gcc/c-family/c-common.c (revision 233902) --- gcc/c-family/c-common.c (working copy) *** strict_aliasing_warning (tree otype, tre *** 1568,1574 alias_set_type set2 = get_alias_set (TREE_TYPE (type)); if (set1 != set2 && set2 != 0 ! && (set1 == 0 || !alias_sets_conflict_p (set1, set2))) { warning (OPT_Wstrict_aliasing, "dereferencing type-punned " "pointer will break strict-aliasing rules"); --- 1568,1574 alias_set_type set2 = get_alias_set (TREE_TYPE (type)); if (set1 != set2 && set2 != 0 ! && (set1 == 0 || !alias_set_subset_of (set2, set1))) { warning (OPT_Wstrict_aliasing, "dereferencing type-punned " "pointer will break strict-aliasing rules"); Index: gcc/testsuite/g++.dg/warn/Wstrict-aliasing-bogus-union-2.C === *** gcc/testsuite/g++.dg/warn/Wstrict-aliasing-bogus-union-2.C (revision 0) --- gcc/testsuite/g++.dg/warn/Wstrict-aliasing-bogus-union-2.C (working copy) *** *** 0 --- 1,14 + /* { dg-do compile { target c++11 } } */ + /* { dg-options "-Wstrict-aliasing=2 -O2 -Wall" } */ + + #include + + struct foo + { + std::aligned_storage ::type raw; + + long& cooked() + { + return *static_cast (static_cast ()); /* { dg-bogus "strict-aliasing" } */ + } + }; Index: gcc/testsuite/gcc.dg/Wstrict-aliasing-struct-member.c === --- gcc/testsuite/gcc.dg/Wstrict-aliasing-struct-member.c (revision 0) +++ gcc/testsuite/gcc.dg/Wstrict-aliasing-struct-member.c (working copy) @@ -0,0 +1,6 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wall" } */ + +struct S { int i; long l; }; +long x; +struct S foo () { return *(struct S *) } /* { dg-warning "will break strict-aliasing" } */
Re: [Ping^2][PATCH][GCC-5] Fix "#pragma GCC pop_options" warning.
On 03/03/2016 11:45 AM, Andre Vieira (lists) wrote: On 29/02/16 10:47, Andre Vieira (lists) wrote: On 15/02/16 10:33, Andre Vieira (lists) wrote: On 18/01/16 11:04, Andre Vieira (lists) wrote: Hi there, Can we have the "#pragma GCC pop_options" fix backported to GCC-5? Patch found in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01261.html and was committed in r228794. The same patch applies cleanly to gcc-5, which would otherwise not be able to use this pragma even though the support is there. I understood it was a good idea to CC the appropriate maintainer on this, so adding Bernd Schmidt to the CC. Yeah, I think I remember this one. Ok. Bernd
[PATCH] Fix vec_set_hi* patterns (PR target/70059)
Hi! Unlike the older vec_set_hi 256-bit patterns, which are correctly using the VEC_SELECT as the first operand of VEC_CONCAT and match_operand as second, because that is what the instruction does, picks up low part from operand 1 and puts the operand 2 as the high part of the result, the 512-bit vec_set_hi patterns (both the avx512f ones and avx512dq ones) use the same order of VEC_CONCAT operands as vec_set_lo (and differ just by different second operand of VEC_SELECT). This leads to wrong-code on the following testcases, simplify-rtx.c during combine simply looks at the RTL patterns and simplifies stuff according to how the RTL patterns look like. Fixed thusly, unfortunately I don't have access to avx512f (and not even to avx512dq) hw, so while I will bootstrap/regtest it on Haswell-E, can't test the tests if they now work at runtime (they link and the assembly of the foo routine has changed and looks good to me). Can somebody test this please on real hw or emulator? Ok for trunk if it passes? 2016-03-03 Jakub JelinekPR target/70059 * config/i386/sse.md (vec_set_lo_, _vinsert_mask): Formatting fixes. (vec_set_hi_): Likewise. Swap VEC_CONCAT operands. * gcc.target/i386/avx512f-pr70059.c: New test. * gcc.target/i386/avx512dq-pr70059.c: New test. --- gcc/config/i386/sse.md.jj 2016-03-02 20:14:11.0 +0100 +++ gcc/config/i386/sse.md 2016-03-03 10:40:58.325680697 +0100 @@ -12426,13 +12426,13 @@ (define_expand "_vinsert { int mask = INTVAL (operands[3]); if (mask == 0) -emit_insn (gen_vec_set_lo__mask - (operands[0], operands[1], operands[2], - operands[4], operands[5])); +emit_insn (gen_vec_set_lo__mask (operands[0], operands[1], + operands[2], operands[4], + operands[5])); else -emit_insn (gen_vec_set_hi__mask - (operands[0], operands[1], operands[2], - operands[4], operands[5])); +emit_insn (gen_vec_set_hi__mask (operands[0], operands[1], + operands[2], operands[4], + operands[5])); DONE; }) @@ -12443,9 +12443,9 @@ (define_insn "vec_set_lo_ (match_operand:V16FI 1 "register_operand" "v") (parallel [(const_int 8) (const_int 9) - (const_int 10) (const_int 11) - (const_int 12) (const_int 13) - (const_int 14) (const_int 15)]] + (const_int 10) (const_int 11) + (const_int 12) (const_int 13) + (const_int 14) (const_int 15)]] "TARGET_AVX512DQ" "vinsert32x8\t{$0x0, %2, %1, %0|%0, %1, %2, $0x0}" [(set_attr "type" "sselog") @@ -12456,13 +12456,13 @@ (define_insn "vec_set_lo_" [(set (match_operand:V16FI 0 "register_operand" "=v") (vec_concat:V16FI - (match_operand: 2 "nonimmediate_operand" "vm") (vec_select: (match_operand:V16FI 1 "register_operand" "v") (parallel [(const_int 0) (const_int 1) - (const_int 2) (const_int 3) - (const_int 4) (const_int 5) - (const_int 6) (const_int 7)]] + (const_int 2) (const_int 3) + (const_int 4) (const_int 5) + (const_int 6) (const_int 7)])) + (match_operand: 2 "nonimmediate_operand" "vm")))] "TARGET_AVX512DQ" "vinsert32x8\t{$0x1, %2, %1, %0|%0, %1, %2, $0x1}" [(set_attr "type" "sselog") @@ -12477,7 +12477,7 @@ (define_insn "vec_set_lo_ (match_operand:V8FI 1 "register_operand" "v") (parallel [(const_int 4) (const_int 5) - (const_int 6) (const_int 7)]] + (const_int 6) (const_int 7)]] "TARGET_AVX512F" "vinsert64x4\t{$0x0, %2, %1, %0|%0, %1, %2, $0x0}" [(set_attr "type" "sselog") @@ -12488,11 +12488,11 @@ (define_insn "vec_set_lo_" [(set (match_operand:V8FI 0 "register_operand" "=v") (vec_concat:V8FI - (match_operand: 2 "nonimmediate_operand" "vm") (vec_select: (match_operand:V8FI 1 "register_operand" "v") (parallel [(const_int 0) (const_int 1) - (const_int 2) (const_int 3)]] + (const_int 2) (const_int 3)])) + (match_operand: 2 "nonimmediate_operand" "vm")))] "TARGET_AVX512F" "vinsert64x4\t{$0x1, %2, %1, %0|%0, %1, %2, $0x1}" [(set_attr "type" "sselog") --- gcc/testsuite/gcc.target/i386/avx512f-pr70059.c.jj 2016-03-03 11:06:00.949063626 +0100 +++ gcc/testsuite/gcc.target/i386/avx512f-pr70059.c 2016-03-03 11:10:42.772195215 +0100 @@ -0,0 +1,33 @@ +/* PR target/70059 */ +/* { dg-do run } */ +/* { dg-require-effective-target avx512f } */ +/* { dg-options "-O2 -mavx512f" } */ + +#include "avx512f-check.h" + +__attribute__((noinline, noclone)) __m512i +foo (__m256i a, __m256i b) +{ +
Re: [PATCH]Replace -shared with -r -nostdlib in gcc.dg/lto/pr61526 pr54709 pr64415 test cases.
Hi Richard, On 03/03/16 10:13, Richard Biener wrote: On Wed, Mar 2, 2016 at 5:12 PM, Renlin Liwrote: Hi Richard, On 02/03/16 13:35, Richard Biener wrote: On Tue, Mar 1, 2016 at 4:56 PM, Renlin Li wrote: Hi Richard, On 01/03/16 09:16, Richard Biener wrote: On Mon, Feb 29, 2016 at 5:13 PM, Renlin Li wrote: Hi all, The gcc.dg/lto/pr54709, pr61526, pr64415 linking testcases keep failing on arm/aarch64 bare-metal target. It's because statically built newlib library is used to link with shared object. And the linker complains about relocations which cannot be used in shared object. For example, the following errors are produced: crtbegin.o: relocation R_ARM_MOVW_ABS_NC against `a local symbol' can not be used when making a shared object; recompile with -fPIC crtbegin.o: relocation R_ARM_THM_MOVW_ABS_NC against `a local symbol' can not be used when making a shared object; recompile with -fPIC librdimon.a(rdimon-syscalls.o): relocation R_AARCH64_ADR_PREL_PG_HI21 against external symbol `_impure_ptr' can not be used when making a shared object; recompile with -fPIC Presumably, bare-metal toolchain for other architecture have those test case failures as well? In this patch, -shared option is replace by -r -nostdlib. So that the standard system startup files or libraries are not used when linking. Note that -shared is not equivalent to -r -nostdlib so please verify that the original issue can be still reproduced with its fix reverted but -r -nostdlib used with the new -r -nostdlib handling on trunk. pr54709_0.c: Cannot be reproduced with even -shared. The error message is the same as shown above. pr64415_0.c: Reproduced with "-r -nostdlib". pr61526_0.c: Reproduced with "-r -nostdlib". By the way, those linking test cases all pass for linux toolchain. Only fail for aarch64/arm baremetal toolchain. Andrew, I saw you have done similar things in r153555 https://gcc.gnu.org/viewcvs/gcc?view=revision=153555 Do you have any thoughts? And also here, the last comments in this ticket suggests to add check_effective_target_shared to the exp file to limit it to linux targets only: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61526 As said LTO testcases tend to be somewhat fragile so limiting them to targets known to work might be the best option. Richard. Forgot the mention that, by purely adding "-nostdlib" option (instead of replacing -shared) fixes the failures as well. I test those test cases again with fix reverted, keep "-shared" option, add "-nostdlib" option. Ok, so I discovered we have a "shared" target which means if a target doesn't support shared libs we can guard against it with using /* { dg-require-effective-target shared } */ does adding that to the three testcases fix the issue for you? By adding this target check /* { dg-require-effective-target shared } */ Those test cases aredeemed to be unsupported, and thus skipped for aarch64-none-elf target. However, it's a little bit tricky for arm bare-metal target. The shared option check actually successes for arm-none-eabi toolchain. This is because the default cpu for arm-none-eabi toolchain is arm7tdmi. And the start file crtbegin.o doesn't contains any modifications not allowed in shared object. arm-none-eabi is built with multilib. When running this testcase, it's compiled with "-march=armv7-a". The crtbegin.o for this architecture version contains relocations which cannot be used in shared object. That's why they fails to linking test. Will adding "-nostdlib" (instead of replace -shared) option be an reasonable fix given my previous check? Regards, Renlin Thanks, Richard. pr54709_0.c: Cannot be reproduced even with test case unmodified. The error message is the same as shown above. with "-nostdlib", no failure. pr64415_0.c: Reproduced. pr61526_0.c: Reproduced. Regards, Renlin Regards, Renlin Otherwise simply dg-skip for aarch64. Richard. arm-none-eabi, aarch64-none-elf regression test OK, OK for trunk? Regards, Renlin Li gcc/testsuite/ChangeLog: 2016-02-29 Renlin Li * gcc.dg/lto/pr54709_0.c: Replace -shard with -r -nostdlib. * gcc.dg/lto/pr61526_0.c: Ditto. * gcc.dg/lto/pr64415_0.c: Ditto.
Re: [PTX] port some cleanups from gomp4
Hello Nathan, On Wed, 9 Sep 2015, Nathan Sidwell wrote: > I've applied this patch to port some cleanups, mainly formatting and loop > idioms from the gomp4 branch. This patch that you committed to trunk in September 2015 forcefully disables generation of line number information, undoing a part of a patch by Bernd Schmidt that was committed to trunk by Thomas Schwinge in August 2015. I couldn't find a corresponding email for the gomp4 branch (apparently the same change was committed to the branch just a few hours prior to trunk), so I wonder if there was any technical reason to disable linenumber info, or just some kind of oversight? Would you like if I prepared a patch for trunk that backs out the override? It's useful not only for debugging, but also for instruction-level profiling (which is my main motivation at the moment). (relevant bit of the problematic patch quoted below) Thanks. Alexander > 2015-09-09 Nathan Sidwell> > * config/nvptx/nvptx.md (call_operation): Move bound out of loop. > (*cmp): Add assembler spacing. > (setcc_int, set_cc_float): Likewise. > * config/nvptx/nvptx.c (nvptx_option_override): Override debug > level. (the hunk for the above entry) > (write_func_decl_from_insn): Refactor argument loops & comma emission. > (nvptx_expand_call): Likewise. > (nvptx_output_call_insn): Likewise. > (nvptx_reorg_subreg): Add spacing. > > Index: src/gcc-mainline/gcc/config/nvptx/nvptx.c > === > --- src/gcc-mainline/gcc/config/nvptx/nvptx.c (revision 227587) > +++ src/gcc-mainline/gcc/config/nvptx/nvptx.c (working copy) > @@ -102,6 +102,8 @@ nvptx_option_override (void) >flag_toplevel_reorder = 1; >/* Assumes that it will see only hard registers. */ >flag_var_tracking = 0; > + write_symbols = NO_DEBUG; > + debug_info_level = DINFO_LEVEL_NONE; > >declared_fndecls_htab = hash_table::create_ggc (17); >needed_fndecls_htab = hash_table::create_ggc (17);
[PATCH][AArch64] PR target/70002: Make aarch64_set_current_function play nice with pragma resetting
Hi all, This patch fixes the ICE that was introduced by my earlier patch to aarch64_set_current_function: FAIL: gcc.dg/torture/pr52429.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) And it also fixes a bug that I was working on separately relating to popping pragmas. The patch rewrites the aarch64_set_current_function implementation to be the same as the one in the arm port that Christian wrote and which is simpler than the existing implementation and has been tested for some time without problems. I've thought this was the way to go but was hoping to do it for GCC 7 instead, but I think given the ICE we'd rather have consistent implementations of this hook between arm and aarch64 (and ideally this should be moved into the midend for all targets, I don't see much target-specific information in the implementation of this across the targets, but not at this stage). Similar to that implementation the setting and restoring of the target globals is factored into a separate function that is used in aarch64_set_current_function and the pragma handling function to tell the midend when to reinitialise its structures. This patch fixes the ICE, the testcase attached, and passes bootstrap and regression testing on aarch64-none-linux-gnu. Sorry for missing the ICE originally. Ok for trunk? Thanks, Kyrill P.S. The vector arguments re-layout hack that was in aarch64_set_current_function is removed because it has become unneeded since Christian fixed some midend bugs so that it's done automatically there. 2016-03-03 Kyrylo TkachovPR target/70002 * config/aarch64/aarch64-protos.h (aarch64_save_restore_target_globals): New prototype. * config/aarch64/aarch64-c.c (aarch64_pragma_target_parse): Call the above when popping pragma. * config/aarch64/aarch64.c (aarch64_save_restore_target_globals): New function. (aarch64_set_current_function): Rewrite using the above. 2016-03-03 Kyrylo Tkachov PR target/70002 PR target/69245 * gcc.target/aarch64/pr69245_2.c: New test. diff --git a/gcc/config/aarch64/aarch64-c.c b/gcc/config/aarch64/aarch64-c.c index 3590ae0daa5d80050b0f81cd6ab9a7779f463516..e057daaec24c0add673d0b2c776d4c4c43d1f0ea 100644 --- a/gcc/config/aarch64/aarch64-c.c +++ b/gcc/config/aarch64/aarch64-c.c @@ -178,6 +178,12 @@ aarch64_pragma_target_parse (tree args, tree pop_target) cpp_opts->warn_unused_macros = saved_warn_unused_macros; + /* If we're popping or reseting make sure to update the globals so that + the optab availability predicates get recomputed. */ + if (pop_target) +aarch64_save_restore_target_globals (pop_target); + + /* Initialize SIMD builtins if we haven't already. Set current_target_pragma to NULL for the duration so that the builtin initialization code doesn't try to tag the functions diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h index e4e49fc9ccc3d568c84b35c1a0c0733475017cca..c40d2b0c78494b50508c1b5135b8ee7676a61631 100644 --- a/gcc/config/aarch64/aarch64-protos.h +++ b/gcc/config/aarch64/aarch64-protos.h @@ -361,6 +361,7 @@ void aarch64_emit_call_insn (rtx); void aarch64_register_pragmas (void); void aarch64_relayout_simd_types (void); void aarch64_reset_previous_fndecl (void); +void aarch64_save_restore_target_globals (tree); void aarch64_emit_approx_rsqrt (rtx, rtx); /* Initialize builtins for SIMD intrinsics. */ diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 1e10d9798ddc5f5d2aac4255d3a8fe4ecaf1402a..a05160e08d0474ed9c1e2afa1d00375839417034 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -8570,6 +8570,21 @@ aarch64_reset_previous_fndecl (void) aarch64_previous_fndecl = NULL; } +/* Restore or save the TREE_TARGET_GLOBALS from or to NEW_TREE. + Used by aarch64_set_current_function and aarch64_pragma_target_parse to + make sure optab availability predicates are recomputed when necessary. */ + +void +aarch64_save_restore_target_globals (tree new_tree) +{ + if (TREE_TARGET_GLOBALS (new_tree)) +restore_target_globals (TREE_TARGET_GLOBALS (new_tree)); + else if (new_tree == target_option_default_node) +restore_target_globals (_target_globals); + else +TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts (); +} + /* Implement TARGET_SET_CURRENT_FUNCTION. Unpack the codegen decisions like tuning and ISA features from the DECL_FUNCTION_SPECIFIC_TARGET of the function, if such exists. This function may be called multiple @@ -8579,63 +8594,32 @@ aarch64_reset_previous_fndecl (void) static void aarch64_set_current_function (tree fndecl) { + if (!fndecl || fndecl == aarch64_previous_fndecl) +return; + tree old_tree = (aarch64_previous_fndecl ? DECL_FUNCTION_SPECIFIC_TARGET (aarch64_previous_fndecl) : NULL_TREE); - tree new_tree = (fndecl
Re: [PATCH] Fix ICE with vector types in X % -Y pattern (PR middle-end/70050)
On Thu, Mar 03, 2016 at 12:04:31PM +0100, Marek Polacek wrote: > We crashed on the attached testcase because we were trying to apply the > X % -Y -> X % Y transformation even on vectors. That doesn't go well with the > check for !TYPE_UNSIGNED. So I think let's limit the pattern to only work on > integral types. I think TYPE_UNSIGNED is fine, but TYPE_MIN_VALUE is not. > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2016-03-03 Marek Polacek> > PR middle-end/70050 > * match.pd (X % -Y): Add INTEGRAL_TYPE_P check. > > * gcc.dg/pr70050.c: New test. Ok, thanks. Jakub
Re: [PATCH 1/2][GCC][ARM] Add support for Cortex-R8
On 03/03/16 11:31, Kyrill Tkachov wrote: On 03/03/16 11:28, Kyrill Tkachov wrote: Hi Andre, On 02/03/16 12:20, Andre Vieira (lists) wrote: gcc/ChangeLog: 2016-03-02 Andre Vieira* config/arm/arm-cores.def (cortex-r8): New. * config/arm/arm-tables.opt (cortex-r8): New. * config/arm/arm-tune.md: Regenerate. * gcc/doc/invoke.texi: Add cortex-r8 to list of cpu values. One nit I just noticed. The arm-tables.opt entry should say "Renerate" as it's auto-generated from arm-cores.def. Of course, that should say "Regenerate." Sorry, fingers slipping :( Ok with that change to the ChangeLog Kyrill Ok. Thanks, Kyrill
Re: Reinstate generic stack checking warning with LRA
> Anyway, looking at pro_and_epilogue dumps, with additional > -fstack-protector-strong we decrease sp only by 4176, while without it by > 8224 (on x86_64; the testcase fails on all targets I've tried so far > ({x86_64,i686,powerpc64{,le},s390{,x},aarch64,armv7hl}-linux). Yeah, the threshold is around 4KB, feel free to add a few more HUNDREDs. -- Eric Botcazou
Re: [PATCH 1/2][GCC][ARM] Add support for Cortex-R8
On 03/03/16 11:28, Kyrill Tkachov wrote: Hi Andre, On 02/03/16 12:20, Andre Vieira (lists) wrote: gcc/ChangeLog: 2016-03-02 Andre Vieira* config/arm/arm-cores.def (cortex-r8): New. * config/arm/arm-tables.opt (cortex-r8): New. * config/arm/arm-tune.md: Regenerate. * gcc/doc/invoke.texi: Add cortex-r8 to list of cpu values. One nit I just noticed. The arm-tables.opt entry should say "Renerate" as it's auto-generated from arm-cores.def. Ok with that change to the ChangeLog Kyrill Ok. Thanks, Kyrill
Re: [PATCH 2/2][GCC][ARM] Fix testcases after introduction of Cortex-R8
Hi Andre, On 02/03/16 12:21, Andre Vieira (lists) wrote: Hi, Tests used to check for "r8" which will not work because cortex-r8 string is now included in the assembly. Fixed by checking for "[^\-]r8". Is this Ok? Cheers, Andre gcc/testsuite/ChangeLog: 2016-03-02 Andre Vieira* gcc.target/arm/pr45701-1.c: Change assembler scan to not trigger for cortex-r8, when scanning for register r8. * gcc.target/arm/pr45701-2.c: Likewise. Ok. Thanks, Kyrill
Re: [PATCH 1/2][GCC][ARM] Add support for Cortex-R8
Hi Andre, On 02/03/16 12:20, Andre Vieira (lists) wrote: gcc/ChangeLog: 2016-03-02 Andre Vieira* config/arm/arm-cores.def (cortex-r8): New. * config/arm/arm-tables.opt (cortex-r8): New. * config/arm/arm-tune.md: Regenerate. * gcc/doc/invoke.texi: Add cortex-r8 to list of cpu values. Ok. Thanks, Kyrill
Re: [PATCH][wwwdocs] Remove (Pending) tag from 5.3 notes, add 5.4 entry
On 03/03/16 11:14, Gerald Pfeifer wrote: On Thu, 3 Mar 2016, Kyrill Tkachov wrote: Ok to commit? Richi already approved, so this is only for future cases: Please do consider changes like this either as trivial (and go ahead, just posting the patch) or pre-approved by me (and go ahead, just posting the patch). As you prefer. ;-) Thanks Gerald, I'll keep it in mind. Kyrill Thanks, Gerald
Re: [PATCH][wwwdocs] Remove (Pending) tag from 5.3 notes, add 5.4 entry
On Thu, 3 Mar 2016, Kyrill Tkachov wrote: Ok to commit? Richi already approved, so this is only for future cases: Please do consider changes like this either as trivial (and go ahead, just posting the patch) or pre-approved by me (and go ahead, just posting the patch). As you prefer. ;-) Thanks, Gerald
[PATCH] Fix ICE with vector types in X % -Y pattern (PR middle-end/70050)
We crashed on the attached testcase because we were trying to apply the X % -Y -> X % Y transformation even on vectors. That doesn't go well with the check for !TYPE_UNSIGNED. So I think let's limit the pattern to only work on integral types. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2016-03-03 Marek PolacekPR middle-end/70050 * match.pd (X % -Y): Add INTEGRAL_TYPE_P check. * gcc.dg/pr70050.c: New test. diff --git gcc/match.pd gcc/match.pd index 5903782..112deb3 100644 --- gcc/match.pd +++ gcc/match.pd @@ -293,7 +293,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) /* X % -Y is the same as X % Y. */ (simplify (trunc_mod @0 (convert? (negate @1))) - (if (!TYPE_UNSIGNED (type) + (if (INTEGRAL_TYPE_P (type) + && !TYPE_UNSIGNED (type) && !TYPE_OVERFLOW_TRAPS (type) && tree_nop_conversion_p (type, TREE_TYPE (@1)) /* Avoid this transformation if X might be INT_MIN or diff --git gcc/testsuite/gcc.dg/pr70050.c gcc/testsuite/gcc.dg/pr70050.c index e69de29..610456f 100644 --- gcc/testsuite/gcc.dg/pr70050.c +++ gcc/testsuite/gcc.dg/pr70050.c @@ -0,0 +1,11 @@ +/* PR middle-end/70025 */ +/* { dg-do compile } */ +/* { dg-options "-Wno-psabi" } */ + +typedef int v8si __attribute__ ((vector_size (32))); + +v8si +foo (v8si v) +{ + return v %= -v; +} Marek
Re: [Ping^2][PATCH][GCC-5] Fix "#pragma GCC pop_options" warning.
On 29/02/16 10:47, Andre Vieira (lists) wrote: > On 15/02/16 10:33, Andre Vieira (lists) wrote: >> On 18/01/16 11:04, Andre Vieira (lists) wrote: >>> Hi there, >>> >>> Can we have the "#pragma GCC pop_options" fix backported to GCC-5? >>> >>> Patch found in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01261.html >>> and was committed in r228794. >>> >>> The same patch applies cleanly to gcc-5, which would otherwise not be >>> able to use this pragma even though the support is there. >>> >>> Cheers, >>> Andre >>> >> >> Ping. >> > Ping. > I understood it was a good idea to CC the appropriate maintainer on this, so adding Bernd Schmidt to the CC. Sorry for the noise. Cheers, Andre
Re: [committed] Fix libffi/70024
Hi Richard, > As discussed in the PR, let's take the opportunity while bumping the soname > to add symbol versioning. great idea: I'd already suggested this back in 2010 when doing the bulk of the Solaris symbol versioning work http://gcc.gnu.org/ml/gcc/2010-02/msg00339.html http://sourceware.org/ml/libffi-discuss/2010/msg00045.html but there never was a conclusion on the questions I'd raised. > Versioning is complicated by the fact that there are several pieces of API > that are "optional" based on the target. If these optional pieces are > later implemented by targets that currently do not, we can't have these > extra symbols suddenly appear in the base version -- that voids the promise > that symbol versioning makes. > > So the symbols are split among 4 different versions. Each version's set of > symbols is either entirely present[*] or entirely absent. Programs using > the library will only depend on the subset of versions that they use. > > Tested on {x86_64,ppc64,aarch64}-linux. Compile tested on mips64el-linux, > which doesn't implement go closures or complex types. Unfortunately, the patch broke Solaris bootstrap: make[2]: Entering directory '/var/gcc/regression/trunk/12-gcc/build/i386-pc-solaris2.12/libffi' Makefile:1906: *** missing separator (did you mean TAB instead of 8 spaces?). Stop. Fixed as obvious like this, will commit shortly: 2016-03-03 Rainer Orth* Makefile.am (libffi.map-sun): Tabify: * Makefile.in: Regenerate. # HG changeset patch # Parent 3195cd69f93aa48e3c342ea80e6f2660e30f33da Tabify libffi/Makefile.am diff --git a/libffi/Makefile.am b/libffi/Makefile.am --- a/libffi/Makefile.am +++ b/libffi/Makefile.am @@ -214,11 +214,11 @@ libffi_version_script = -Wl,-M,libffi.ma libffi_version_dep = libffi.map-sun libffi.map-sun : libffi.map $(top_srcdir)/../contrib/make_sunver.pl \ $(libffi_la_OBJECTS) $(libffi_la_LIBADD) -perl $(top_srcdir)/../contrib/make_sunver.pl libffi.map \ - $(libffi_la_OBJECTS:%.lo=.libs/%.o) \ - `echo $(libffi_la_LIBADD) | \ -sed 's,\([^/]*\)\.l\([ao]\),.libs/\1.\2,g'` \ - > $@ || (rm -f $@ ; exit 1) + perl $(top_srcdir)/../contrib/make_sunver.pl libffi.map \ + $(libffi_la_OBJECTS:%.lo=.libs/%.o) \ + `echo $(libffi_la_LIBADD) | \ + sed 's,\([^/]*\)\.l\([ao]\),.libs/\1.\2,g'` \ + > $@ || (rm -f $@ ; exit 1) endif else libffi_version_script = Unfortunately, even with this fixed, all Solaris/x86 tests now fail to link: FAIL: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 (test for excess errors) Excess errors: Undefined first referenced symbol in file ffi_closure_alloc /var/tmp//ccIpq3qc.o ffi_type_float /var/tmp//ccIpq3qc.o ffi_type_uint64 /var/tmp//ccIpq3qc.o ffi_type_sint32 /var/tmp//ccIpq3qc.o ffi_type_sint16 /var/tmp//ccIpq3qc.o ffi_type_double /var/tmp//ccIpq3qc.o ffi_prep_cif/var/tmp//ccIpq3qc.o ld: fatal: symbol referencing errors > For gcc7, we really should pull out these m4 macros to new config/ files. > I didn't really want to touch anything except libffi for now. Great idea: I'd meant to do this for a long time, but never got around to it. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] Add -funknown-commons to work around PR/69368 (and others) in SPEC2006
On 25/02/16 18:00, Alan Lawrence wrote: On 22/02/16 12:03, Jakub Jelinek wrote: (f) A global command-line option, which we check alongside DECL_COMMON and further tests (basically, we want only DECL_COMMON decls that either have ARRAY_TYPE, or some other aggregate type with flexible array member or some other trailing array in the struct), and use the resulting predicate in places where we optimize based on DECL_SIZE{,_UNIT} of the decl - if that predicate returns true, we assume the DECL_SIZE is "variable"/"unlimited"/whatever. The two spots known to me that would need to use this new predicate would be: tree.c (array_at_struct_end_p): [...] tree-dfa.c (get_ref_base_and_extent): [...] Close enough to what I meant by (a)/(b), but never mind that, and I hadn't looked at where the change for aggressive loop opts needed to be. However... Well you are essentially writing the patch for me at this point (so, thanks!), but here's a patch that puts that under a global -funknown-commons flag. Testcase as before. Bootstrapped (with flag overridden to true) on x86_64 and aarch64, check-gcc, check-fortran, and tested 416.gamess on AArch64, where this patch enables running *without* the -fno-aggressive-loop-opts previously required. In the gcc testsuite, these fail with the option turned on: gcc.dg/pr53265.c (test for warnings, line 137) gcc.dg/pr53265.c (test for warnings, line 138) gcc.dg/pr64277.c scan-tree-dump cunroll ... (*2) gcc.dg/tree-ssa/cunroll-{1,2,3,4,9,10,11}.c scan-tree-dump cunroll/cunrolli/ivcanon (various) gcc.dg/tree-ssa/loop-38.c scan-tree-dump cunrolli "Loop 1 iterates at most 11 times" ...which all appear harmless. Thomas Koenig suggests (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69368#c80) that this be combined with some flag fiddling and warnings in the Fortran front-end; this patch doesn't do that, as I'm not very familiar with the frontends, but that can follow in a separate patch. (Thomas?) OK for trunk? Cheers, Alan gcc/ChangeLog: DATE Alan LawrenceJakub Jelinek * common.opt (funknown-commons, flag_unknown_commons): New. * tree.c (array_at_struct_end_p): Do not limit to size of decl for DECL_COMMONS if flag_unknown_commons is set. * tree-dfa.c (get_ref_base_and_extent): Likewise. gcc/testsuite/ChangeLog: * gfortran.dg/unknown_commons.f: New. Ping. (Besides my original patch https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01356.html which we decided was too fragile, I believe this is the only patch proposed that actually fixes the miscompare in 416.gamess.) Thanks, Alan
Re: Fix/work around PR57676, LRA terminates prematurely
On Wed, Mar 2, 2016 at 7:39 PM, Bernd Schmidtwrote: > On 02/24/2016 11:01 PM, Jeff Law wrote: >> >> As Vlad noted, the test is definitely a pathological case. I think >> Bernd's patch is a very reasonable approach to address the current >> problem. Namely that LRA can be making progress on a pathological >> testcase, but it gets terminated by the anti-looping clamp. The clamp >> itself was put in place to catch bugs in LRA or ports that are in the >> process of converting to LRA. > > > Richard, are you upholding your objection? No. Richard. > > Bernd >
Re: [PATCH]Replace -shared with -r -nostdlib in gcc.dg/lto/pr61526 pr54709 pr64415 test cases.
On Wed, Mar 2, 2016 at 5:12 PM, Renlin Liwrote: > Hi Richard, > > > On 02/03/16 13:35, Richard Biener wrote: >> >> On Tue, Mar 1, 2016 at 4:56 PM, Renlin Li wrote: >>> >>> Hi Richard, >>> >>> >>> On 01/03/16 09:16, Richard Biener wrote: On Mon, Feb 29, 2016 at 5:13 PM, Renlin Li wrote: > > Hi all, > > The gcc.dg/lto/pr54709, pr61526, pr64415 linking testcases keep failing > on > arm/aarch64 bare-metal target. > > It's because statically built newlib library is used to link with > shared > object. > And the linker complains about relocations which cannot be used in > shared object. > > For example, the following errors are produced: > > crtbegin.o: relocation R_ARM_MOVW_ABS_NC against `a local symbol' can > not > be > used when making a shared object; recompile with -fPIC > > crtbegin.o: relocation R_ARM_THM_MOVW_ABS_NC against `a local symbol' > can > not > be used when making a shared object; recompile with -fPIC > > librdimon.a(rdimon-syscalls.o): relocation R_AARCH64_ADR_PREL_PG_HI21 > against > external symbol `_impure_ptr' can not be used when making a shared > object; > recompile with -fPIC > > Presumably, bare-metal toolchain for other architecture have those test > case > failures as well? > > In this patch, -shared option is replace by -r -nostdlib. So that the > standard > system startup files or libraries are not used when linking. Note that -shared is not equivalent to -r -nostdlib so please verify that the original issue can be still reproduced with its fix reverted but -r -nostdlib used with the new -r -nostdlib handling on trunk. >>> >>> >>> pr54709_0.c: Cannot be reproduced with even -shared. The error message is >>> the same as shown above. >>> pr64415_0.c: Reproduced with "-r -nostdlib". >>> pr61526_0.c: Reproduced with "-r -nostdlib". >>> >>> By the way, those linking test cases all pass for linux toolchain. Only >>> fail >>> for aarch64/arm baremetal toolchain. >>> >>> Andrew, I saw you have done similar things in r153555 >>> https://gcc.gnu.org/viewcvs/gcc?view=revision=153555 >>> >>> Do you have any thoughts? >>> >>> And also here, the last comments in this ticket suggests to add >>> check_effective_target_shared to the exp file to limit it to linux >>> targets >>> only: >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61526 >> >> As said LTO testcases tend to be somewhat fragile so limiting them to >> targets known to work might be the best option. >> >> Richard. > > > Forgot the mention that, by purely adding "-nostdlib" option (instead of > replacing -shared) > fixes the failures as well. > > I test those test cases again with fix reverted, keep "-shared" option, add > "-nostdlib" option. Ok, so I discovered we have a "shared" target which means if a target doesn't support shared libs we can guard against it with using /* { dg-require-effective-target shared } */ does adding that to the three testcases fix the issue for you? Thanks, Richard. > pr54709_0.c: Cannot be reproduced even with test case unmodified. > The error message is the same as shown above. with "-nostdlib", no failure. > > pr64415_0.c: Reproduced. > pr61526_0.c: Reproduced. > > Regards, > Renlin > > > >> >>> Regards, >>> Renlin >>> >>> Otherwise simply dg-skip for aarch64. Richard. > arm-none-eabi, aarch64-none-elf regression test OK, OK for trunk? > > Regards, > Renlin Li > > gcc/testsuite/ChangeLog: > > 2016-02-29 Renlin Li > > * gcc.dg/lto/pr54709_0.c: Replace -shard with -r -nostdlib. > * gcc.dg/lto/pr61526_0.c: Ditto. > * gcc.dg/lto/pr64415_0.c: Ditto. > >
Re: [PATCH][wwwdocs] Remove (Pending) tag from 5.3 notes, add 5.4 entry
On Thu, Mar 3, 2016 at 10:49 AM, Kyrill Tkachovwrote: > Hi all, > > I noticed that changes.html for GCC 5 has an entry for GCC 5.3 saying > (Pending) and linking to the fixed PRs. > 5.3 has already been released, so this patch removes it from there, and > instead adds a similar entry for the pending 5.4 release. > > Ok to commit? Ok. Richard. > Thanks, > Kyrill
Re: [testsuite] Invoke gdb with -batch to avoid prompts
Mike Stumpwrites: > On Mar 1, 2016, at 6:20 AM, Rainer Orth wrote: >> When switching from gdb 7.10 to the newly released gdb 7.11 on Solaris, >> all simulate-thread tests started to timeout > > Ok. If a domain expert prefers a different strategy, I’m fine with > anything better as well. Given that there were neither objections nor better suggestions, I've installed the patch everywhere. Thanks. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [Patch testsuite] Change xfail conditions for bb-slp-34.c
On Thu, 3 Mar 2016, James Greenhalgh wrote: > > Hi, > > ARM and AArch64 will still vectorize bb-slp-34.c - we're not operating > with a cost model so we vectorize to a 64-bit vector of two ints, and the > permutes are just element swaps. > > So, don't mark this test xfail for arm/aarch64. > > Checked on x86_64-none-linux-gnu, arm-none-eabi and aarch64-none-elf with > no issues. > > OK? Ok. Indeed with using V2SI vectors the vectorization is valid. Richard.
Re: [wwwdocs] Note for pr70024 for gcc-5
On Thu, Mar 3, 2016 at 3:09 AM, Richard Hendersonwrote: > Is there anything else we should say? > > I thought about recommending that distributions not install the libffi > shared library from gcc and instead use upstream source. But that doesn't > really help one way or the other, so I dropped that language. I wonder if it should go to the Caveats section but otherwise looks ok (noting the breakage is on aarch64 only). Thanks, Richard. > > r~
Re: [committed] Fix libffi/70024
On Thu, Mar 3, 2016 at 3:13 AM, Richard Hendersonwrote: > [ Ho hum. Re-send due to spam rejection. > Trying again with compressed patch. ] > > > As discussed in the PR, let's take the opportunity while bumping the soname > to add symbol versioning. > > Versioning is complicated by the fact that there are several pieces of API > that are "optional" based on the target. If these optional pieces are later > implemented by targets that currently do not, we can't have these extra > symbols suddenly appear in the base version -- that voids the promise that > symbol versioning makes. > > So the symbols are split among 4 different versions. Each version's set of > symbols is either entirely present[*] or entirely absent. Programs using > the library will only depend on the subset of versions that they use. > > Tested on {x86_64,ppc64,aarch64}-linux. Compile tested on mips64el-linux, > which doesn't implement go closures or complex types. > > For gcc7, we really should pull out these m4 macros to new config/ files. > I didn't really want to touch anything except libffi for now. Thanks for doing this. I suppose this is also upstream now? Richard. > > r~ > > > [*] Except for i386, where FFI_NATIVE_RAW_API modifies the API, suppressing > ffi_java_raw_call, ffi_prep_java_raw_closure, ffi_prep_java_raw_closure_loc. > > IMO it's a libffi bug that these symbols are suppressed; they could just as > easily have been aliases for other symbols within libffi. But it's too late > now. Instead, users of libffi (e.g. libjava) are already checking > FFI_NATIVE_RAW_API and adjusting the function called. > > Anyway, all is well so long as no target changes to define > FFI_NATIVE_RAW_API that hasn't previously. I'm going to assume that i386 is > unique and no other target will ever define FFI_NATIVE_RAW_API.