From patchwork Thu Feb 2 13:20:51 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 139135 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]) by ozlabs.org (Postfix) with SMTP id 92CD2104785 for ; Fri, 3 Feb 2012 00:21:16 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1328793677; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Date: From:To:Cc:Subject:In-Reply-To:Message-ID:References:User-Agent: MIME-Version:Content-Type:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=wT/KCBxrKRAh/ZGPomhBsXdXjaM=; b=Vzp77WlejMJgqP1 oWwlz0LUxPEpeVLPNf/tBbETh7c1aPUc6UdDlpVTNFfaE1EiOEFHUM2vtkWIhIku XkKr9I+AUQ5uoTvvgyWyIC8YpWgCPNSBWfA3P21nZ8/Lbl3tgtoDrI7FOF0cBQ38 rLpI6oKObzPAmLjkCsWRwEvGJIkw= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Date:From:To:Cc:Subject:In-Reply-To:Message-ID:References:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=EqNZLAyOvAT666hwXK/KqzmuxfKULQQ/bJlKYDEzYHC54T2/lQBfj0rve+9/P4 o0rfUAqAwnPmYZnx8W3fbfG6xHWZIu6rREoFaPb4eevKBpp3YkXaplHrLzVYh+l7 gURG35kIhn9k6biqGndPV6aPcXmgdM29TzG8qczve7f+w=; Received: (qmail 23081 invoked by alias); 2 Feb 2012 13:21:10 -0000 Received: (qmail 23063 invoked by uid 22791); 2 Feb 2012 13:21:07 -0000 X-SWARE-Spam-Status: No, hits=-5.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 02 Feb 2012 13:20:53 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 215098FA97; Thu, 2 Feb 2012 14:20:52 +0100 (CET) Date: Thu, 2 Feb 2012 14:20:51 +0100 (CET) From: Richard Guenther To: gcc-patches@gcc.gnu.org Cc: ebotcazou@adacore.com Subject: Re: [PATCH][RFC] Fix PR48124 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 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 On Thu, 2 Feb 2012, Richard Guenther wrote: > On Wed, 1 Feb 2012, Richard Guenther wrote: > > > > > This fixes PR48124 where a bitfield store leaks into adjacent > > decls if the decl we store to has bigger alignment than what > > its type requires (thus there is another decl in that "padding"). > > [...] > > > Bootstraped on x86_64-unknown-linux-gnu, testing in progress. > > So testing didn't go too well (which makes me suspicious about > the adjust_address the strict-volatile-bitfield path does ...). > > The following patch instead tries to make us honor mode1 as > maximum sized mode to be used for accesses to the bitfield > (mode1 as that returned from get_inner_reference, so in theory > that would cover the strict-volatile-bitfield cases already). > > It does so by passing down that mode to store_fixed_bit_field > and use it as max-mode argument to get_best_mode there. The > patch also checks that the HAVE_insv paths would not use > bigger stores than that mode (hopefully I've covered all cases > where we would do that). > > Currently all bitfields (that are not volatile) get VOIDmode > from get_inner_reference and the patch tries hard to not > change things in that case. The expr.c hunk contains one > possible fix for 48124 by computing mode1 based on MEM_SIZE > (probably not the best way - any good ideas welcome). > > The patch should also open up the way for fixing PR52080 > (that bitfield-store-clobbers-adjacent-fields bug ...) > by simply making get_inner_reference go the > strict-volatile-bitfield path for all bitfield accesses > (and possibly even the !DECL_BIT_FIELD pieces of it). > Thus, do what people^WLinus would expect for "sane" C > (thus non-mixed base-type bitfields). > > I'm looking for comments on both pieces of the patch > (is the strict-volatile-bitfields approach using > adjust-address really correct? is passing down mode1 > as forced maximum-size mode correct, or will it pessimize > code too much?) > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > I don't see that we can fix 52080 in full for 4.7 but it > would be nice to at least fix 48124 with something not > too invasive (suggestions for some narrower cases to > adjust mode1 welcome). Other than MEM_SIZE we could > simply use TYPE_ALIGN if that is less than MEM_ALIGN > and compute a maximum size mode based on that. The following variant also bootstrapped and tested ok. Richard. if (offset != 0) Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 183833) +++ gcc/expr.c (working copy) @@ -4705,6 +4705,23 @@ expand_assignment (tree to, tree from, b to_rtx = adjust_address (to_rtx, mode1, 0); else if (GET_MODE (to_rtx) == VOIDmode) to_rtx = adjust_address (to_rtx, BLKmode, 0); + /* If we have a bitfield access and the alignment of the + accessed object is larger than what its type would require + restrict the mode we use for accesses to avoid touching + the tail alignment-padding. See PR48124. */ + else if (mode1 == VOIDmode + && TREE_CODE (to) == COMPONENT_REF + && TYPE_ALIGN (TREE_TYPE (tem)) < MEM_ALIGN (to_rtx)) + { + mode1 = mode_for_size (TYPE_ALIGN (TREE_TYPE (tem)), MODE_INT, 1); + if (mode1 == BLKmode + /* Not larger than word_mode. */ + || GET_MODE_SIZE (mode1) > GET_MODE_SIZE (word_mode) + /* Nor smaller than the fields mode. */ + || (GET_MODE_SIZE (mode1) + < GET_MODE_SIZE (DECL_MODE (TREE_OPERAND (to, 1))))) + mode1 = VOIDmode; + } }