diff mbox

[RFC] Fix PR48124

Message ID alpine.LNX.2.00.1202021420130.4999@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Feb. 2, 2012, 1:20 p.m. UTC 
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)
diff mbox

Patch

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;
+           }
        }