Bug 48124

While on this exact testcase problems started with
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159321
after slightly modifying the testcase:

/* { dg-do run } */
/* { dg-options "-Os -fno-toplevel-reorder" } */

extern void abort (void);

struct S
{
  signed a : 26;
  signed b : 16;
  signed c : 10;
  volatile signed d : 14;
};

static struct S e = { 0, 0, 0, 1 };
static int f = 1;

void __attribute__((noinline))
foo (void)
{
  e.d = 0;
  f = 2;
}

int
main ()
{
  if (e.a || e.b || e.c || e.d != 1 || f != 1)
    abort ();
  foo ();
  if (e.a || e.b || e.c || e.d || f != 2)
    abort ();
  return 0;
}

it keeps failing at -Os -fno-toplevel-reorder already in 4.2 and with just -Os
in 4.3.

On the adjusted testcase it started with http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=115421
Before that the volatile bitfield was accessed just through 32-bit access (and in 4.1 even with 16-bit access).

Hm, e has alignment of 8 bytes and f of 4 but the assembler doesn't pad
e's size to 8 byte multiples appearantly

        .data
        .align 8
        .type   e, @object
        .size   e, 12
e:
        .byte   0
        .byte   0
        .byte   0
        .byte   0
        .value  0
        .byte   0
        .byte   0
        .byte   1
        .byte   0
        .zero   2
        .align 4
        .type   f, @object
        .size   f, 4
f:
        .long   1

this is probably a more general problem also with the vectorizer which
happily increases alignment of global variables independed on if their
size is a multiple of that alignment.

If the asm output machinery does not try to handle padding to multiples
of alignment size then that's the bug I guess.

I think that is not the problem, it is fine if some variables are aligned more than they need to be for performance reasons.  TYPE_ALIGN is still 32 bits, while just DECL_ALIGN is 64 bits.

The bug is in expand_assignment/store_bit_field{,_1} I think.
First expand_assignment calls set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
which changes
(mem/s/c:BLK (symbol_ref:DI ("e") [flags 0x2] <var_decl 0x7ffff1f4b000 e>) [2 e+0 S12 A64])
into:
(mem/s/v/j/c:BLK (symbol_ref:DI ("e") [flags 0x2] <var_decl 0x7ffff1f4b000 e>) [2 e+0 S2 A64])
so the information that e is 12 bytes long is harder to find (of course, one can still look at MEM_EXPR's size).
Then store_bit_field_1 sees i386 has HAVE_insv and op_mode for -m64 insv is DImode.  The first HAVE_insv if doesn't apply, as i386 (like also at least half of other targets that HAVE_insv) doesn't allow memory on op0.
But then the if (HAVE_insv && MEM_P (op0)) triggers and it calls get_best_mode, which for volatile (this case) and !targetm.narrow_bit_fields () decides to enlarge the mode as much as possible and this "possible" takes into account just
MEM_ALIGN (which comes from DECL_ALIGN and thus is large here) and the passed in largest_mode (for insv it comes from insv insn mode).  It doesn't consider
the size of the decl (but see above, expand_assignment made it impossible to use MEM_SIZE here).  Now, if this if (HAVE_insv && MEM_P (op0)) case is disabled, it still generates wrong code, as store_fixed_bit_field does essentially the same, just instead of insv's mode uses word_mode as largest_mode passed to get_best_mode.

Now, the question is what we want to use as largest_mode in these cases.
Obviously it can't be too large to go beyond end of the object's size here (the case in this testcase, the reason why sched2 reorders the two stores is because
it sees the first store using SYMBOL_REF ("e") as base and the second store using SYMBOL_REF ("f") and assumes that those can't alias).  For OpenMP or C++0x
memory model that isn't enough I guess, even if you have:
struct S
{
  signed a : 26;
  signed b : 16;
  signed c : 10;
  volatile signed d : 14;
  int e;
} s;
I think you can't just modify s.e when writing s.d (I think it is fine to modify
adjacent bitfields though, Aldy?).  So for C++0x memory model we probably want
to find the next non-bitfield field and use that or something similar.

> struct S
> {
>    signed a : 26;
>    signed b : 16;
>    signed c : 10;
>    volatile signed d : 14;
>    int e;
> } s;
> I think you can't just modify s.e when writing s.d (I think it is fine to
> modify
> adjacent bitfields though, Aldy?).

No, you can't modify s.e when writing to s.d.  However, you can modify 
adjacent bitfields.  All contiguous bitfields can be considered a single 
memory location for the purpose of introducing data races.  The only 
exception is when bitfields are separated by a zero-length bitfield, or 
when they happen to be contiguous but occur in different 
structures/unions.  Those conditions force alignments on those fields.

Created attachment 23837 [details]
gcc46-pr48124.patch

WIP patch, completely untested, that attempts to pass down to get_best_mode exact offset and type.  The patch just attempts to avoid larger modes crossing
into following objects, but for C++0x model it probably should do more than that.

*** Bug 49071 has been marked as a duplicate of this bug. ***

4.3 branch is being closed, moving to 4.4.7 target.

Simpler patch I am going to test.  Let's hope the wreckage adjust_address
does to the to_rtx MEM (apart from setting its mode) is harmless.

Index: expr.c
===================================================================
--- expr.c      (revision 183791)
+++ expr.c      (working copy)
@@ -4705,6 +4705,21 @@ 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 the alignment of tem is larger than its size and we
+            are performing a bitfield access limit the mode we use
+            for the access to make sure we do not access the decl
+            beyond its end.  See PR48124.  */
+         else if (GET_MODE (to_rtx) == BLKmode
+                  && mode1 == VOIDmode
+                  && DECL_P (tem)
+                  && TREE_CODE (DECL_SIZE (tem)) == INTEGER_CST
+                  && TREE_INT_CST_LOW (DECL_SIZE (tem)) % DECL_ALIGN (tem))
+           {
+             mode1 = mode_for_size (TREE_INT_CST_LOW (DECL_SIZE (tem))
+                                    % DECL_ALIGN (tem),
+                                    MODE_INT, 0);
+             to_rtx = adjust_address (to_rtx, mode1, 0);
+           }
        }
  
       if (offset != 0)

(In reply to comment #9)
> Simpler patch I am going to test.  Let's hope the wreckage adjust_address
> does to the to_rtx MEM (apart from setting its mode) is harmless.
> 
> Index: expr.c
> ===================================================================
> --- expr.c      (revision 183791)
> +++ expr.c      (working copy)
> @@ -4705,6 +4705,21 @@ 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 the alignment of tem is larger than its size and we
> +            are performing a bitfield access limit the mode we use
> +            for the access to make sure we do not access the decl
> +            beyond its end.  See PR48124.  */
> +         else if (GET_MODE (to_rtx) == BLKmode
> +                  && mode1 == VOIDmode
> +                  && DECL_P (tem)
> +                  && TREE_CODE (DECL_SIZE (tem)) == INTEGER_CST
> +                  && TREE_INT_CST_LOW (DECL_SIZE (tem)) % DECL_ALIGN (tem))
> +           {
> +             mode1 = mode_for_size (TREE_INT_CST_LOW (DECL_SIZE (tem))
> +                                    % DECL_ALIGN (tem),
> +                                    MODE_INT, 0);
> +             to_rtx = adjust_address (to_rtx, mode1, 0);
> +           }
>         }
> 
>        if (offset != 0)

Or rather

Index: gcc/expr.c
===================================================================
--- gcc/expr.c  (revision 183791)
+++ gcc/expr.c  (working copy)
@@ -4705,6 +4705,22 @@ 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 the alignment of tem is larger than its size and we
+            are performing a bitfield access limit the mode we use
+            for the access to make sure we do not access the decl
+            beyond its end.  See PR48124.  */
+         else if (GET_MODE (to_rtx) == BLKmode
+                  && mode1 == VOIDmode
+                  && DECL_P (tem)
+                  && TREE_CODE (DECL_SIZE (tem)) == INTEGER_CST
+                  && (TREE_INT_CST_LOW (DECL_SIZE (tem))
+                      & (DECL_ALIGN (tem) - 1)) != 0)
+           {
+             unsigned HOST_WIDE_INT mis;
+             mis = TREE_INT_CST_LOW (DECL_SIZE (tem)) & (DECL_ALIGN (tem) - 1);
+             mode1 = mode_for_size (mis & -mis, MODE_INT, 0);
+             to_rtx = adjust_address (to_rtx, mode1, 0);
+           }
        }
  
       if (offset != 0)

*** Bug 49758 has been marked as a duplicate of this bug. ***

*** Bug 51176 has been marked as a duplicate of this bug. ***

Doesn't work.

MEM_SIZE also seems to be somewhat random, because we
re-initialize it via set_mem_attributes_minus_bitpos.  So we can't
fixup the caller to get_best_mode easily without passing down extra
knowledge.

We could pass down fieldmode from store_bit_field_1 to store_fixed_bit_field
and give it some more meaningful mode in the ultimate caller
(expand_assignment).

But then there is the HAVE_insv path which uses mode_for_extraction
which would need to be disabled if that mode is bigger than fieldmode
(but we don't do that at the moment, not sure why).

As usual, bitfield expansion seems to be quite a messy area.

I remember (bug number?) we have the same issue with multi-word accesses
where the last part uses word_mode (all other pieces as well, of course)
and that accesses the object out-of-bounds.

*** Bug 50235 has been marked as a duplicate of this bug. ***

Created attachment 26546 [details]
another patch

This patch passes bootstrap on x86_64-unknown-linux-gnu but ICEs in struct-layout tests.

Mine, at least for 4.8, see PR52080 for the patch.

4.4 branch is being closed, moving to 4.5.4 target.

Author: rguenth
Date: Wed Mar 14 10:55:09 2012
New Revision: 185379

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185379
Log:
2012-03-14  Richard Guenther  <rguenther@suse.de>

	* tree.h (DECL_BIT_FIELD_REPRESENTATIVE): New define.
	* stor-layout.c (start_bitfield_representative): New function.
	(finish_bitfield_representative): Likewise.
	(finish_bitfield_layout): Likewise.
	(finish_record_layout): Call finish_bitfield_layout.
	* tree.c (free_lang_data_in_decl): Only free DECL_QUALIFIER
	for QUAL_UNION_TYPE fields.
	* tree-streamer-in.c (lto_input_ts_field_decl_tree_pointers):
	Stream DECL_BIT_FIELD_REPRESENTATIVE.
	* tree-streamer-out.c (write_ts_field_decl_tree_pointers): Likewise.

	PR middle-end/52080
	PR middle-end/52097
	PR middle-end/48124
	* expr.c (get_bit_range): Unconditionally extract bitrange
	from DECL_BIT_FIELD_REPRESENTATIVE.
	(expand_assignment): Adjust call to get_bit_range.

	* gcc.dg/torture/pr48124-1.c: New testcase.
	* gcc.dg/torture/pr48124-2.c: Likewise.
	* gcc.dg/torture/pr48124-3.c: Likewise.
	* gcc.dg/torture/pr48124-4.c: Likewise.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr48124-1.c
    trunk/gcc/testsuite/gcc.dg/torture/pr48124-2.c
    trunk/gcc/testsuite/gcc.dg/torture/pr48124-3.c
    trunk/gcc/testsuite/gcc.dg/torture/pr48124-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expr.c
    trunk/gcc/stor-layout.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-streamer-in.c
    trunk/gcc/tree-streamer-out.c
    trunk/gcc/tree.c
    trunk/gcc/tree.h

Fixed for 4.8.  Don't hold your breath for a backport of this patch (it should
apply cleanly to 4.7 though, but not to anything earlier).

Created attachment 27525 [details]
backport of patch and followups from trunk

Bootstrapped on x86_64-unknown-linux-gnu.

> Bootstrapped on x86_64-unknown-linux-gnu.

This also needs to be tested on 32-bit and strict-alignment platforms.

On Wed, 30 May 2012, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48124
> 
> --- Comment #22 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-30 15:28:14 UTC ---
> > Bootstrapped on x86_64-unknown-linux-gnu.
> 
> This also needs to be tested on 32-bit and strict-alignment platforms.

I'll test it on i?85-linux as well.  I don't have access to a 
strict-alignment platform - but the patch is essentially the same
as on trunk.  Can you give it a shot on sparc or do you forsee any
issues and thus would rather not backport this kind of change?  The
idea was that 4.7.1 would still be ok but later this kind of change
is obviously too intrusive.

Thanks,
Richard.

> I'll test it on i?85-linux as well.  I don't have access to a 
> strict-alignment platform - but the patch is essentially the same
> as on trunk.  Can you give it a shot on sparc or do you forsee any
> issues and thus would rather not backport this kind of change?  The
> idea was that 4.7.1 would still be ok but later this kind of change
> is obviously too intrusive.

Yes, I think it's appropriate for 4.7.1 if all the follow-ups are backported as well.  I'll give it a whirl on SPARC.

On Thu, 31 May 2012, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48124
> 
> --- Comment #24 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-31 10:36:10 UTC ---
> > I'll test it on i?85-linux as well.  I don't have access to a 
> > strict-alignment platform - but the patch is essentially the same
> > as on trunk.  Can you give it a shot on sparc or do you forsee any
> > issues and thus would rather not backport this kind of change?  The
> > idea was that 4.7.1 would still be ok but later this kind of change
> > is obviously too intrusive.
> 
> Yes, I think it's appropriate for 4.7.1 if all the follow-ups are backported as
> well.  I'll give it a whirl on SPARC.

I think I have included them all in the patch I attached.  Meanwhile
multilib testing has finished on x86_64, a pure i?86 bootstrap still
pending (I'm also testing on arm and ppc/ppc64 now, but that may
take a while)

(In reply to comment #25)
> On Thu, 31 May 2012, ebotcazou at gcc dot gnu.org wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48124
> > 
> > --- Comment #24 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-31 10:36:10 UTC ---
> > > I'll test it on i?85-linux as well.  I don't have access to a 
> > > strict-alignment platform - but the patch is essentially the same
> > > as on trunk.  Can you give it a shot on sparc or do you forsee any
> > > issues and thus would rather not backport this kind of change?  The
> > > idea was that 4.7.1 would still be ok but later this kind of change
> > > is obviously too intrusive.
> > 
> > Yes, I think it's appropriate for 4.7.1 if all the follow-ups are backported as
> > well.  I'll give it a whirl on SPARC.
> 
> I think I have included them all in the patch I attached.  Meanwhile
> multilib testing has finished on x86_64, a pure i?86 bootstrap still
> pending (I'm also testing on arm and ppc/ppc64 now, but that may
> take a while)

http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186819
is kind of a follow-up too.

On Thu, 31 May 2012, jakub at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48124
> 
> --- Comment #26 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-05-31 10:42:10 UTC ---
> (In reply to comment #25)
> > On Thu, 31 May 2012, ebotcazou at gcc dot gnu.org wrote:
> > 
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48124
> > > 
> > > --- Comment #24 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-31 10:36:10 UTC ---
> > > > I'll test it on i?85-linux as well.  I don't have access to a 
> > > > strict-alignment platform - but the patch is essentially the same
> > > > as on trunk.  Can you give it a shot on sparc or do you forsee any
> > > > issues and thus would rather not backport this kind of change?  The
> > > > idea was that 4.7.1 would still be ok but later this kind of change
> > > > is obviously too intrusive.
> > > 
> > > Yes, I think it's appropriate for 4.7.1 if all the follow-ups are backported as
> > > well.  I'll give it a whirl on SPARC.
> > 
> > I think I have included them all in the patch I attached.  Meanwhile
> > multilib testing has finished on x86_64, a pure i?86 bootstrap still
> > pending (I'm also testing on arm and ppc/ppc64 now, but that may
> > take a while)
> 
> http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186819
> is kind of a follow-up too.

Yes, that's what motivated me to look again at a possible backport
of the DECL_BIT_FIELD_REPRESENTATIVE patch.  I'd backport that separately
though.

On Wed, 30 May 2012, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48124
> 
> --- Comment #22 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-05-30 15:28:14 UTC ---
> > Bootstrapped on x86_64-unknown-linux-gnu.
> 
> This also needs to be tested on 32-bit and strict-alignment platforms.

I have now successfully tested it on i586-suse-linux-gnu,
powerpc-suse-linux-gnu and powerpc64-suse-linux-gnu.

> I have now successfully tested it on i586-suse-linux-gnu,
> powerpc-suse-linux-gnu and powerpc64-suse-linux-gnu.

It looks OK on x86/x86-64/PowerPC/SPARC/SPARC64 on my side.

Author: rguenth
Date: Mon Jun  4 08:43:23 2012
New Revision: 188167

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188167
Log:
2012-06-04  Richard Guenther  <rguenther@suse.de>
	Eric Botcazou  <ebotcazou@adacore.com>

	Backport from mainline
	2012-04-03  Eric Botcazou  <ebotcazou@adacore.com>

        * expr.c (get_bit_range): Add OFFSET parameter and adjust BITPOS.
        Change type of BITOFFSET to signed.  Make sure the lower bound of
        the computed range is non-negative by adjusting OFFSET and BITPOS.
        (expand_assignment): Adjust call to get_bit_range.

	2012-03-27  Eric Botcazou  <ebotcazou@adacore.com>

        * expr.c (get_bit_range): Return the null range if the enclosing record
        is part of a larger bit field.

	2012-03-20  Richard Guenther  <rguenther@suse.de>

        * stor-layout.c (finish_bitfield_representative): Fallback
        to conservative maximum size if the padding up to the next
        field cannot be computed as a constant.
        (finish_bitfield_layout): If we cannot compute the distance
        between the start of the bitfield representative and the
        bitfield member start a new representative.
        * expr.c (get_bit_range): The distance between the start of
        the bitfield representative and the bitfield member is zero
        if the field offsets are not constants.

	2012-03-16  Richard Guenther  <rguenther@suse.de>

        * stor-layout.c (finish_bitfield_representative): Fall back
        to the conservative maximum size if we cannot compute the
        size of the tail padding.

	2012-03-14  Richard Guenther  <rguenther@suse.de>

	* tree.h (DECL_BIT_FIELD_REPRESENTATIVE): New define.
	* stor-layout.c (start_bitfield_representative): New function.
	(finish_bitfield_representative): Likewise.
	(finish_bitfield_layout): Likewise.
	(finish_record_layout): Call finish_bitfield_layout.
	* tree.c (free_lang_data_in_decl): Only free DECL_QUALIFIER
	for QUAL_UNION_TYPE fields.
	* tree-streamer-in.c (lto_input_ts_field_decl_tree_pointers):
	Stream DECL_BIT_FIELD_REPRESENTATIVE.
	* tree-streamer-out.c (write_ts_field_decl_tree_pointers): Likewise.

	PR middle-end/52080
	PR middle-end/52097
	PR middle-end/48124
	* expr.c (get_bit_range): Unconditionally extract bitrange
	from DECL_BIT_FIELD_REPRESENTATIVE.
	(expand_assignment): Adjust call to get_bit_range.

	* gcc.dg/torture/pr48124-1.c: New testcase.
	* gcc.dg/torture/pr48124-2.c: Likewise.
	* gcc.dg/torture/pr48124-3.c: Likewise.
	* gcc.dg/torture/pr48124-4.c: Likewise.
	* gnat.dg/pack16.adb: Likewise.
	* gnat.dg/pack16_pkg.ads: Likewise.
	* gnat.dg/pack17.adb: Likewise.
	* gnat.dg/specs/pack7.ads: Likewise.
	* gnat.dg/specs/pack8.ads: Likewise.
	* gnat.dg/specs/pack8_pkg.ads: Likewise.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr48124-1.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr48124-2.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr48124-3.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr48124-4.c
    branches/gcc-4_7-branch/gcc/testsuite/gnat.dg/pack16.adb
    branches/gcc-4_7-branch/gcc/testsuite/gnat.dg/pack16_pkg.ads
    branches/gcc-4_7-branch/gcc/testsuite/gnat.dg/pack17.adb
    branches/gcc-4_7-branch/gcc/testsuite/gnat.dg/specs/pack7.ads
    branches/gcc-4_7-branch/gcc/testsuite/gnat.dg/specs/pack8.ads
    branches/gcc-4_7-branch/gcc/testsuite/gnat.dg/specs/pack8_pkg.ads
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/expr.c
    branches/gcc-4_7-branch/gcc/stor-layout.c
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_7-branch/gcc/tree-streamer-in.c
    branches/gcc-4_7-branch/gcc/tree-streamer-out.c
    branches/gcc-4_7-branch/gcc/tree.c
    branches/gcc-4_7-branch/gcc/tree.h

Fixed on the 4.7 branch, too.  Not applicable as is to older branches.

The 4.5 branch is being closed, adjusting target milestone.

The 4.6 branch has been closed, fixed in GCC 4.7.1.