Message ID | 20150513191803.GG27320@redhat.com |
---|---|
State | New |
Headers | show |
On Wed, 13 May 2015, Marek Polacek wrote: > > Rather, how about having an extra argument to c_fully_fold_internal (e.g. > > for_int_const) indicating that the folding is of an expression with > > integer constant operands (so this argument would be true in the new case > > of folding the contents of a C_MAYBE_CONST_EXPR with > > C_MAYBE_CONST_EXPR_INT_OPERANDS set)? In that special case, > > c_fully_fold_internal would only fold the expression itself if all > > evaluated operands folded to INTEGER_CSTs (so if something that doesn't > > get folded, such as division by 0, is encountered, then all evaluated > > expressions containing it also don't get folded). > > Did you mean something like the following? It is on top of the previous > c_fully_fold_internal patch; if it makes any sense, I'll squash these > into one. Yes. The two patches are OK together, though I think it would be better to add for_int_const checks also in the cases of unary operations, &&, || and ?: (the last three being cases where it's only the evaluated operands that need to fold to INTEGER_CSTs, not any unevaluated operands). It may well be the case that no folding at present can fold any of those cases to an INTEGER_CST when it shouldn't be one, but I don't think we should rely on that. > This still doesn't reject enum { A = 0 * (unsigned) (1 / 0) };, but note > that we don't reject such an enum at present. It's rejected with -pedantic-errors, but it should indeed be rejected unconditionally like the other cases. Casts can do some optimization prematurely, but I don't know if that has anything to do with the non-rejection here.
On May 14, 2015 12:46:06 AM GMT+02:00, Joseph Myers <joseph@codesourcery.com> wrote: >On Wed, 13 May 2015, Marek Polacek wrote: > >> > Rather, how about having an extra argument to c_fully_fold_internal >(e.g. >> > for_int_const) indicating that the folding is of an expression with > >> > integer constant operands (so this argument would be true in the >new case >> > of folding the contents of a C_MAYBE_CONST_EXPR with >> > C_MAYBE_CONST_EXPR_INT_OPERANDS set)? In that special case, >> > c_fully_fold_internal would only fold the expression itself if all >> > evaluated operands folded to INTEGER_CSTs (so if something that >doesn't >> > get folded, such as division by 0, is encountered, then all >evaluated >> > expressions containing it also don't get folded). >> >> Did you mean something like the following? It is on top of the >previous >> c_fully_fold_internal patch; if it makes any sense, I'll squash these >> into one. > >Yes. The two patches are OK together, though I think it would be >better >to add for_int_const checks also in the cases of unary operations, &&, >|| >and ?: (the last three being cases where it's only the evaluated >operands >that need to fold to INTEGER_CSTs, not any unevaluated operands). It >may >well be the case that no folding at present can fold any of those cases >to >an INTEGER_CST when it shouldn't be one, but I don't think we should >rely >on that. > >> This still doesn't reject enum { A = 0 * (unsigned) (1 / 0) };, but >note >> that we don't reject such an enum at present. > >It's rejected with -pedantic-errors, but it should indeed be rejected >unconditionally like the other cases. > >Casts can do some optimization prematurely, but I don't know if that >has >anything to do with the non-rejection here. Do the patches allow us to remove the restrictions from the folding patterns? Richard.
On Thu, May 14, 2015 at 09:59:49AM +0200, Richard Biener wrote:
> Do the patches allow us to remove the restrictions from the folding patterns?
With the c_fully_fold_internal patches there's no need to tweak any match.pd
patterns. So PR66127 + PR66066 are to be solved solely in the C FE. Is that
what you're asking about?
Marek
On May 14, 2015 10:10:35 AM GMT+02:00, Marek Polacek <polacek@redhat.com> wrote: >On Thu, May 14, 2015 at 09:59:49AM +0200, Richard Biener wrote: >> Do the patches allow us to remove the restrictions from the folding >patterns? > >With the c_fully_fold_internal patches there's no need to tweak any >match.pd >patterns. So PR66127 + PR66066 are to be solved solely in the C FE. >Is that >what you're asking about? No, I'm asking whether we can remove the existing guards in match.pd. Richard. > Marek
On Thu, May 14, 2015 at 11:48:55AM +0200, Richard Biener wrote: > On May 14, 2015 10:10:35 AM GMT+02:00, Marek Polacek <polacek@redhat.com> wrote: > >On Thu, May 14, 2015 at 09:59:49AM +0200, Richard Biener wrote: > >> Do the patches allow us to remove the restrictions from the folding > >patterns? > > > >With the c_fully_fold_internal patches there's no need to tweak any > >match.pd > >patterns. So PR66127 + PR66066 are to be solved solely in the C FE. > >Is that > >what you're asking about? > > No, I'm asking whether we can remove the existing guards in match.pd. There is also the C++ FE... Jakub
On Thu, May 14, 2015 at 11:48:55AM +0200, Richard Biener wrote: > On May 14, 2015 10:10:35 AM GMT+02:00, Marek Polacek <polacek@redhat.com> wrote: > >On Thu, May 14, 2015 at 09:59:49AM +0200, Richard Biener wrote: > >> Do the patches allow us to remove the restrictions from the folding > >patterns? > > > >With the c_fully_fold_internal patches there's no need to tweak any > >match.pd > >patterns. So PR66127 + PR66066 are to be solved solely in the C FE. > >Is that > >what you're asking about? > > No, I'm asking whether we can remove the existing guards in match.pd. Sorry, I don't know which guards exactly you mean (side effects?) so can't verify, but as Jakub points out, I doubt we can remove anything at this point because of C++ FE. (c_fully_fold{,_internal} are never used in the C++ FE.) Marek
Marek, This may be unrelated to your patches for PR 66127 and 66066 but I am having a new failure when building the latest glibc with the latest GCC. I haven't yet tracked down exactly which patch caused the problem. Included is a cutdown test case from libio/memstream.c in glibc that results in a strict-aliasing error. Is this is something you already know about or have seen? In the mean time I will try to figure out exactly which patch caused this error to trigger. Steve Ellcey sellcey@imgtec.com typedef unsigned int size_t; extern void *malloc (size_t __size) __attribute__ ((__nothrow__ )) __attribute__ ((__malloc__)) ; struct _IO_FILE_plus { void *vtable; }; void *_IO_mem_jumps; struct _IO_streambuf { }; typedef struct _IO_strfile_ { struct _IO_streambuf _sbf; } _IO_strfile; struct _IO_FILE_memstream { _IO_strfile _sf; }; void open_memstream (int bufloc, int sizeloc) { struct locked_FILE { struct _IO_FILE_memstream fp; } *new_f; new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); ((struct _IO_FILE_plus *) &new_f->fp._sf._sbf)->vtable = &_IO_mem_jumps; } x.c: In function 'open_memstream': x.c:28:12: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] ((struct _IO_FILE_plus *) &new_f->fp._sf._sbf)->vtable = &_IO_mem_jumps; ^ cc1: all warnings being treated as errors
On Thu, 2015-05-14 at 09:42 -0700, Steve Ellcey wrote: > Marek, > > This may be unrelated to your patches for PR 66127 and 66066 but I am > having a new failure when building the latest glibc with the latest GCC. > > I haven't yet tracked down exactly which patch caused the problem. Included > is a cutdown test case from libio/memstream.c in glibc that results in a > strict-aliasing error. Is this is something you already know about or have > seen? > > In the mean time I will try to figure out exactly which patch caused this error > to trigger. > > Steve Ellcey > sellcey@imgtec.com Sorry for the noise, it looks like this failure is not related to any recent changes in GCC. I still get the error in older GCC's so I think something must have changed in glibc. I will start digging there. Steve Ellcey sellcey@imgtec.com
On Thu, May 14, 2015 at 10:07:56AM -0700, Steve Ellcey wrote: > Sorry for the noise, it looks like this failure is not related to any > recent changes in GCC. I still get the error in older GCC's so I think > something must have changed in glibc. I will start digging there. Yeah -- strict aliasing should be unrelated to the changes I've recently done in the C FE. Marek
On Thu, 2015-05-14 at 19:22 +0200, Marek Polacek wrote: > On Thu, May 14, 2015 at 10:07:56AM -0700, Steve Ellcey wrote: > > Sorry for the noise, it looks like this failure is not related to any > > recent changes in GCC. I still get the error in older GCC's so I think > > something must have changed in glibc. I will start digging there. > > Yeah -- strict aliasing should be unrelated to the changes I've recently done > in the C FE. > > Marek FYI: I finally found the change that caused this failure, it is a GCC patch after all. It starts with Richard's fix to PR middle-end/66110. I sent some email to glibc but I am not sure (again) if we want to change GCC or glibc. https://sourceware.org/ml/libc-alpha/2015-05/msg00258.html Steve Ellcey sellcey@imgtec.com
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c index 24200f0..1dc181d 100644 --- gcc/c-family/c-common.c +++ gcc/c-family/c-common.c @@ -315,7 +315,7 @@ const struct fname_var_t fname_vars[] = /* Global visibility options. */ struct visibility_flags visibility_options; -static tree c_fully_fold_internal (tree expr, bool, bool *, bool *); +static tree c_fully_fold_internal (tree expr, bool, bool *, bool *, bool); static tree check_case_value (location_t, tree); static bool check_case_bounds (location_t, tree, tree, tree *, tree *); @@ -1148,7 +1148,7 @@ c_fully_fold (tree expr, bool in_init, bool *maybe_const) expr = TREE_OPERAND (expr, 0); } ret = c_fully_fold_internal (expr, in_init, maybe_const, - &maybe_const_itself); + &maybe_const_itself, false); if (eptype) ret = fold_convert_loc (loc, eptype, ret); *maybe_const &= maybe_const_itself; @@ -1161,11 +1161,13 @@ c_fully_fold (tree expr, bool in_init, bool *maybe_const) arithmetic overflow (for C90, *MAYBE_CONST_OPERANDS is carried from both evaluated and unevaluated subexpressions while *MAYBE_CONST_ITSELF is carried from only evaluated - subexpressions). */ + subexpressions). FOR_INT_CONST indicates if EXPR is an expression + with integer constant operands, and if any of the operands doesn't + get folded to an integer constant, don't fold the expression itself. */ static tree c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, - bool *maybe_const_itself) + bool *maybe_const_itself, bool for_int_const) { tree ret = expr; enum tree_code code = TREE_CODE (expr); @@ -1212,7 +1214,7 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, { *maybe_const_itself = false; inner = c_fully_fold_internal (inner, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, true); } if (pre && !in_init) ret = build2 (COMPOUND_EXPR, TREE_TYPE (expr), pre, inner); @@ -1263,7 +1265,7 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, op1 = TREE_OPERAND (expr, 1); op2 = TREE_OPERAND (expr, 2); op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, for_int_const); STRIP_TYPE_NOPS (op0); if (op0 != orig_op0) ret = build3 (COMPONENT_REF, TREE_TYPE (expr), op0, op1, op2); @@ -1280,10 +1282,10 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, op2 = TREE_OPERAND (expr, 2); op3 = TREE_OPERAND (expr, 3); op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, for_int_const); STRIP_TYPE_NOPS (op0); op1 = c_fully_fold_internal (op1, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, for_int_const); STRIP_TYPE_NOPS (op1); op1 = decl_constant_value_for_optimization (op1); if (op0 != orig_op0 || op1 != orig_op1) @@ -1340,7 +1342,7 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, orig_op0 = op0 = TREE_OPERAND (expr, 0); orig_op1 = op1 = TREE_OPERAND (expr, 1); op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, for_int_const); STRIP_TYPE_NOPS (op0); if (code != MODIFY_EXPR && code != PREDECREMENT_EXPR @@ -1352,9 +1354,14 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, expression for the sake of conversion warnings. */ if (code != MODIFY_EXPR) op1 = c_fully_fold_internal (op1, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, for_int_const); STRIP_TYPE_NOPS (op1); op1 = decl_constant_value_for_optimization (op1); + + if (for_int_const && (TREE_CODE (op0) != INTEGER_CST + || TREE_CODE (op1) != INTEGER_CST)) + goto out; + if (op0 != orig_op0 || op1 != orig_op1 || in_init) ret = in_init ? fold_build2_initializer_loc (loc, code, TREE_TYPE (expr), op0, op1) @@ -1424,7 +1431,7 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, /* Unary operations. */ orig_op0 = op0 = TREE_OPERAND (expr, 0); op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands, - maybe_const_itself); + maybe_const_itself, for_int_const); STRIP_TYPE_NOPS (op0); if (code != ADDR_EXPR && code != REALPART_EXPR && code != IMAGPART_EXPR) op0 = decl_constant_value_for_optimization (op0); @@ -1472,14 +1479,16 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, arguments. */ orig_op0 = op0 = TREE_OPERAND (expr, 0); orig_op1 = op1 = TREE_OPERAND (expr, 1); - op0 = c_fully_fold_internal (op0, in_init, &op0_const, &op0_const_self); + op0 = c_fully_fold_internal (op0, in_init, &op0_const, &op0_const_self, + for_int_const); STRIP_TYPE_NOPS (op0); unused_p = (op0 == (code == TRUTH_ANDIF_EXPR ? truthvalue_false_node : truthvalue_true_node)); c_disable_warnings (unused_p); - op1 = c_fully_fold_internal (op1, in_init, &op1_const, &op1_const_self); + op1 = c_fully_fold_internal (op1, in_init, &op1_const, &op1_const_self, + for_int_const); STRIP_TYPE_NOPS (op1); c_enable_warnings (unused_p); @@ -1510,16 +1519,19 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands, orig_op0 = op0 = TREE_OPERAND (expr, 0); orig_op1 = op1 = TREE_OPERAND (expr, 1); orig_op2 = op2 = TREE_OPERAND (expr, 2); - op0 = c_fully_fold_internal (op0, in_init, &op0_const, &op0_const_self); + op0 = c_fully_fold_internal (op0, in_init, &op0_const, &op0_const_self, + for_int_const); STRIP_TYPE_NOPS (op0); c_disable_warnings (op0 == truthvalue_false_node); - op1 = c_fully_fold_internal (op1, in_init, &op1_const, &op1_const_self); + op1 = c_fully_fold_internal (op1, in_init, &op1_const, &op1_const_self, + for_int_const); STRIP_TYPE_NOPS (op1); c_enable_warnings (op0 == truthvalue_false_node); c_disable_warnings (op0 == truthvalue_true_node); - op2 = c_fully_fold_internal (op2, in_init, &op2_const, &op2_const_self); + op2 = c_fully_fold_internal (op2, in_init, &op2_const, &op2_const_self, + for_int_const); STRIP_TYPE_NOPS (op2); c_enable_warnings (op0 == truthvalue_true_node);