From patchwork Tue May 28 13:27:10 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 246863 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 1F8092C00A7 for ; Tue, 28 May 2013 23:27:31 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=fppPQUKg9V8R19i9K x4xFaxt7TJH3cQnjvUHwUAHu5Z7JDvflzBmphqfzQshxJbm/vjHX7I6R773fuCU8 +O928z42PzvpC+AFNf48t5sxXqbc8QdbUd0Juo1kgN1LGR76XmNr+HUb4Ud1lVi7 i+tWE/Bw8FTAdNjUp/l9SCo7nk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=cDgXOseOLcbF5P3pnb9aUAQ jNd4=; b=sGzR/j6UJZM8ixcvCipedQY7JgfSGktCwGW7SDlFY13VEBTlNK3VDwm H6XbiCGqnB0QsULl5GXk6LUuBmtqKWrPkdZAk3uaehT8Jxg/rrebWc2Dxl3yW59J v5iYq7irGJz5G0FFFNYwgaZyPRQRTQMX6ZjnafOw7XqpqxyPUd3w= Received: (qmail 457 invoked by alias); 28 May 2013 13:27:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 392 invoked by uid 89); 28 May 2013 13:27:16 -0000 X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, RCVD_IN_HOSTKARMA_W, RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 28 May 2013 13:27:13 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id EC9C7A5359; Tue, 28 May 2013 15:27:10 +0200 (CEST) Date: Tue, 28 May 2013 15:27:10 +0200 From: Martin Jambor To: Richard Biener Cc: Eric Botcazou , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Do not allow non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs Message-ID: <20130528132709.GE27165@virgil.suse> Mail-Followup-To: Richard Biener , Eric Botcazou , gcc-patches@gcc.gnu.org References: <20130522151105.GF23266@virgil.suse> <4311044.6dSfYhQ6mB@polaris> <20130524132634.GA27165@virgil.suse> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Hi, On Mon, May 27, 2013 at 10:02:19AM +0200, Richard Biener wrote: > On Fri, 24 May 2013, Martin Jambor wrote: > > > Hi, > > > > On Thu, May 23, 2013 at 11:38:10AM +0200, Richard Biener wrote: > > > On Thu, 23 May 2013, Eric Botcazou wrote: > > > > > > > > earlier this week I asked on IRC whether we could have non-top-level > > > > > BIT_FIELD_REFs and Richi said that we could. However, when I later > > > > > looked at SRA code, quite apparently it is not designed to handle > > > > > non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs. So in > > > > > order to test whether that assumption is OK, I added the following > > > > > into the gimple verifier and ran bootstrap and testsuite of all > > > > > languages including Ada and ObjC++ on x86_64. It survived, which > > > > > makes me wondering whether we do not want it in trunk. > > > > > > > > This looks plausible to me, but I think that you ought to verify the real > > > > assumption instead, which is that the type of the 3 nodes is always scalar. > > > > The non-toplevelness of the nodes is merely a consequence of this property. > > > > > > Yeah. But please put the verification into tree-cfg.c:verify_expr > > > instead. > > > > > > > Like this? Also bootstrapped and tested on x86_64-linux. > > > > Thanks, > > > > Martin > > > > > > 2013-05-23 Martin Jambor > > > > * tree-cfg.c (verify_expr): Verify that BIT_FIELD_REFs, IMAGPART_EXPRs > > and REALPART_EXPRs have scalar type. > > > > Index: src/gcc/tree-cfg.c > > =================================================================== > > --- src.orig/gcc/tree-cfg.c > > +++ src/gcc/tree-cfg.c > > @@ -2669,10 +2669,17 @@ verify_expr (tree *tp, int *walk_subtree > > > > case REALPART_EXPR: > > case IMAGPART_EXPR: > > + case BIT_FIELD_REF: > > + if (!is_gimple_reg_type (TREE_TYPE (t))) > > + { > > + error ("non-scalar BIT_FIELD_REF, IMAGPART_EXPR or REALPART_EXPR"); > > + return t; > > + } > > + /* Fall-through. */ > > case COMPONENT_REF: > > case ARRAY_REF: > > case ARRAY_RANGE_REF: > > - case BIT_FIELD_REF: > > case VIEW_CONVERT_EXPR: > > /* We have a nest of references. Verify that each of the operands > > that determine where to reference is either a constant or a variable, > > Yes, that looks good to me. Note that this still does not verify > that REALPART_EXPR, IMAGPART_EXPR and BIT_FIELD_REF are only > outermost handled-component refs. It merely verifies that if they > are outermost then they are not aggregate. > > Thus a followup would be to move the BIT_FIELD_REF handling in the > loop below to the above case sub-set and disallow BIT_FIELD_REF, > REALPART_EXPR and IMAGPART_EXPR inside that loop. > > Though I'm pretty sure that evetually this will fail ... > > The patch is ok, it's an improvement over the current state. I've committed it s revision 199379, thanks. As far as the non-top-levelness is concerned, the following (on top of the previous patch) also survives bootstrap and testsuite on x86_64 (all languages including Ada and Obj-C++). Do you think it would be acceptable as well? Thanks, Martin 2013-05-27 Martin Jambor * tree-cfg.c (verify_expr): Verify that BIT_FIELD_REF, REALPART_EXPR and IMAGPART_EXPR do not occur within other handled_components. Index: src/gcc/tree-cfg.c =================================================================== --- src.orig/gcc/tree-cfg.c +++ src/gcc/tree-cfg.c @@ -2675,6 +2675,33 @@ verify_expr (tree *tp, int *walk_subtree return t; } + if (TREE_CODE (t) == BIT_FIELD_REF) + { + if (!host_integerp (TREE_OPERAND (t, 1), 1) + || !host_integerp (TREE_OPERAND (t, 2), 1)) + { + error ("invalid position or size operand to BIT_FIELD_REF"); + return t; + } + if (INTEGRAL_TYPE_P (TREE_TYPE (t)) + && (TYPE_PRECISION (TREE_TYPE (t)) + != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) + { + error ("integral result type precision does not match " + "field size of BIT_FIELD_REF"); + return t; + } + else if (!INTEGRAL_TYPE_P (TREE_TYPE (t)) + && TYPE_MODE (TREE_TYPE (t)) != BLKmode + && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t))) + != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) + { + error ("mode precision of non-integral result does not " + "match field size of BIT_FIELD_REF"); + return t; + } + } + /* Fall-through. */ case COMPONENT_REF: case ARRAY_REF: @@ -2697,35 +2724,16 @@ verify_expr (tree *tp, int *walk_subtree if (TREE_OPERAND (t, 3)) CHECK_OP (3, "invalid array stride"); } - else if (TREE_CODE (t) == BIT_FIELD_REF) - { - if (!host_integerp (TREE_OPERAND (t, 1), 1) - || !host_integerp (TREE_OPERAND (t, 2), 1)) - { - error ("invalid position or size operand to BIT_FIELD_REF"); - return t; - } - if (INTEGRAL_TYPE_P (TREE_TYPE (t)) - && (TYPE_PRECISION (TREE_TYPE (t)) - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) - { - error ("integral result type precision does not match " - "field size of BIT_FIELD_REF"); - return t; - } - else if (!INTEGRAL_TYPE_P (TREE_TYPE (t)) - && !AGGREGATE_TYPE_P (TREE_TYPE (t)) - && TYPE_MODE (TREE_TYPE (t)) != BLKmode - && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t))) - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1)))) - { - error ("mode precision of non-integral result does not " - "match field size of BIT_FIELD_REF"); - return t; - } - } t = TREE_OPERAND (t, 0); + if (TREE_CODE (t) == BIT_FIELD_REF + || TREE_CODE (t) == REALPART_EXPR + || TREE_CODE (t) == IMAGPART_EXPR) + { + error ("non-top-level BIT_FIELD_REF, IMAGPART_EXPR or " + "REALPART_EXPR"); + return t; + } } if (!is_gimple_min_invariant (t) && !is_gimple_lvalue (t))