From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32692 invoked by alias); 25 Nov 2015 14:45:14 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 32677 invoked by uid 89); 25 Nov 2015 14:45:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Wed, 25 Nov 2015 14:45:12 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 7ECB8AAC7; Wed, 25 Nov 2015 14:43:29 +0000 (UTC) Date: Wed, 25 Nov 2015 14:45:00 -0000 From: Richard Biener To: Marek Polacek cc: GCC Patches , Joseph Myers Subject: Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513) In-Reply-To: <20151125143509.GU21807@redhat.com> Message-ID: References: <20151125143509.GU21807@redhat.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-11/txt/msg03078.txt.bz2 On Wed, 25 Nov 2015, Marek Polacek wrote: > This is a bit tangled and I don't know how to best fix this so let me explain > in more detail. > > The gist is that a C_MAYBE_CONST_EXPR leaks into the gimplifier. > > In the testcase, we have > > (short) ((i ? *e : 0) & ~u | i & u); > > where 'e' is a pointer to volatile. During c_parser_conditional_expression, we > wrap 'i' and '*e' in C_MAYBE_CONST_EXPR. Later on, we get to build_c_cast, and > here we're trying to cast > > (c_maybe_const_expr != 0 ? (unsigned int) c_maybe_const_expr <*e > : 0) > & ~u | (unsigned int) i & u > > to "short", so we call convert()->convert_to_integer() etc. As a part of this > conversion, we try to fold the expr. Now, we match this pattern: > > (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x > > Look how 'x' is duplicated in the result of the pattern, and since (because of > the volatile 'e') it has TREE_SIDE_EFFECTS, we need to wrap it in a SAVE_EXPR, > which means the convert() produces > > (short int) (((SAVE_EXPR != 0 ? (unsigned short) > c_maybe_const_expr <*e > : 0>) > ^ (unsigned short) i) & (unsigned short) u > ^ (SAVE_EXPR != 0 ? (unsigned short) > c_maybe_const_expr <*e > : 0>)) > > What's important is that we end up with a C_MAYBE_CONST_EXPR in a SAVE_EXPR. > > Down the line, we get into c_process_expr_stmt, where there's > > expr = c_fully_fold (expr, false, NULL); > > so we try to fully-fold the whole expression. However, c_fully_fold just > gives up when it sees a SAVE_EXPR, so it doesn't scrub out C_MAYBE_CONST_EXPR. > It then leaks into the gimplifier and crashes. > > This pattern is probably the only one that is problematical in this regard. > Looking more at this pattern, it looks dubious, because imagine the 'x' in > the pattern is something complex, e.g. a huge COND_EXPR or somesuch -- in > that case duplicating the 'x' does more harm than good. But the whole point of the SAVE_EXPR is that it does _not_ "duplicate" it, it just creates another use of the same value. > So after all, I > think this transformation should be restricted a bit, and only kick in when > 'x' is a constant, an SSA_NAME, or a certain DECL_P. Seems simple_operand_p > in fold-const.c was what I was after, so I've just used that. > > With this patch, we won't create those SAVE_EXPRs so c_fully_fold is able to > get rid of the C_MAYBE_CONST_EXPRs and the gimplifier is happy. > > Thoughts? No. If c_fully_fold can't handle SAVE_EXPRs then maybe c_gimplify_expr can simply strip them. Richard. > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-11-25 Marek Polacek > > PR c/68513 > * fold-const.c (simple_operand_p): Export. > * fold-const.h (simple_operand_p): Declare. > * match.pd ((x & ~m) | (y & m)): Guard with simple_operand_p. > > * gcc.dg/torture/pr68513.c: New test. > > diff --git gcc/fold-const.c gcc/fold-const.c > index 698062e..9bb3a53 100644 > --- gcc/fold-const.c > +++ gcc/fold-const.c > @@ -122,7 +122,6 @@ static tree decode_field_reference (location_t, tree, HOST_WIDE_INT *, > HOST_WIDE_INT *, > machine_mode *, int *, int *, int *, > tree *, tree *); > -static int simple_operand_p (const_tree); > static bool simple_operand_p_2 (tree); > static tree range_binop (enum tree_code, tree, tree, int, tree, int); > static tree range_predecessor (tree); > @@ -4059,10 +4058,10 @@ sign_bit_p (tree exp, const_tree val) > return NULL_TREE; > } > > -/* Subroutine for fold_truth_andor_1: determine if an operand is simple enough > - to be evaluated unconditionally. */ > +/* Determine if an operand is simple enough to be evaluated > + unconditionally. */ > > -static int > +bool > simple_operand_p (const_tree exp) > { > /* Strip any conversions that don't change the machine mode. */ > diff --git gcc/fold-const.h gcc/fold-const.h > index 7741802..717c840 100644 > --- gcc/fold-const.h > +++ gcc/fold-const.h > @@ -181,6 +181,7 @@ extern tree const_unop (enum tree_code, tree, tree); > extern tree const_binop (enum tree_code, tree, tree, tree); > extern bool negate_mathfn_p (combined_fn); > extern const char *c_getstr (tree); > +extern bool simple_operand_p (const_tree); > > /* Return OFF converted to a pointer offset type suitable as offset for > POINTER_PLUS_EXPR. Use location LOC for this conversion. */ > diff --git gcc/match.pd gcc/match.pd > index e86cc8b..4aee4a6 100644 > --- gcc/match.pd > +++ gcc/match.pd > @@ -877,7 +877,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > /* (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x */ > (simplify > (bit_ior:c (bit_and:cs @0 (bit_not @2)) (bit_and:cs @1 @2)) > - (bit_xor (bit_and (bit_xor @0 @1) @2) @0)) > + (if (simple_operand_p (@0)) > + (bit_xor (bit_and (bit_xor @0 @1) @2) @0))) > > /* Fold A - (A & B) into ~B & A. */ > (simplify > diff --git gcc/testsuite/gcc.dg/torture/pr68513.c gcc/testsuite/gcc.dg/torture/pr68513.c > index e69de29..4e08b29 100644 > --- gcc/testsuite/gcc.dg/torture/pr68513.c > +++ gcc/testsuite/gcc.dg/torture/pr68513.c > @@ -0,0 +1,13 @@ > +/* PR c/68513 */ > +/* { dg-do compile } */ > + > +int i; > +unsigned u; > +volatile unsigned int *e; > + > +void > +fn1 (void) > +{ > + (short) ((i ? *e : 0) & ~u | i & u); > + (short) (((0, 0) ? *e : 0) & ~u | i & u); > +} > > Marek > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)