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.
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;
+ }
}
if (offset != 0)
