Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere

[Richard Guenther <rguenther at suse dot de>]

On Thu, 15 Mar 2012, Richard Guenther wrote:

> On Thu, 15 Mar 2012, Eric Botcazou wrote:
> 
> > > Ok, I applied a fix for PR52134 and am preparing a fix for PR52578.
> > > It seems we might not be able to rely on
> > >
> > >       tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)),
> > >                                   DECL_FIELD_OFFSET (repr));
> > >       gcc_assert (host_integerp (maxsize, 1));
> > >
> > > but at least until we get a testcase that shows so I won't add
> > > (unexercised) code that handles it.  Eventually we'd need to treat
> > > tail-padding specially for some languages anyway, via a new langhook.
> > 
> > This caused 3 classes of problems in Ada:
> > 
> >   1. failure of the above assertion (pack7.ads)
> >   2. ICE in tree_low_cst (pack16.adb, pack16_pkg.ads)
> >   3. miscompilation (to be dealt with later).
> > 
> > 1. and 2. appear to come from variable-sized fields (and 3. from record types 
> > with variant part).  Testcases attached, they can be installed as:
> > 
> >   gnat.dg/pack16.adb
> >   gnat.dg/pack16_pkg.ads
> >   gnat.dg/specs/pack7.ads
> > 
> > in the testsuite.
> 
> I'll address these and add the testcases.

1. is easy, see patch below.  2. is much harder - we need to
compute the bit-offset relative to the bitfield group start,
thus in get_bit_range we do

  /* Compute the adjustment to bitpos from the offset of the field
     relative to the representative.  */ 
  offset = size_diffop (DECL_FIELD_OFFSET (field),
                        DECL_FIELD_OFFSET (repr));
  bitoffset = (tree_low_cst (offset, 1) * BITS_PER_UNIT
               + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
               - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));

and we cannot generally assume offset is zero (well, maybe we could
arrange that though, at least we could assert that for all fields
in a bitfield group DECL_FIELD_OFFSET is the same?).  Now, first
of we lack gimplification of the DECL_BIT_FIELD_REPRESENTATIVE
DECL_FIELD_OFFSET (not that we really need it - nothing could
later introduce a COMPONENT_REF with that reliably as nothing
holds on to the generated expressions).  But then even for
originally equal DECL_FIELD_OFFSETs we gimplify different
stmts and thus have a different decl in DECL_FIELD_OFFSET so
the folding above does not return a constant either (this makes
me think that we can avoid gimplifying most of the sizepos - we
only need to gimplify those that actually appear in COMPONENT_REFs).

Any suggestion?  Apart from trying to make sure that offset will
be zero by construction?  Or by simply not handling bitfields
properly that start at a variable offset?

The finish_bitfield_representative hunk implements the fix for 1.,
the rest the proposed zero-by-construction solution for 2.

Thanks,
Richard.

Index: gcc/stor-layout.c
===================================================================
*** gcc/stor-layout.c	(revision 185426)
--- gcc/stor-layout.c	(working copy)
*************** finish_bitfield_representative (tree rep
*** 1765,1770 ****
--- 1765,1773 ----
  	     - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)
  	     + tree_low_cst (DECL_SIZE (field), 1));
  
+   /* Round up bitsize to multiples of BITS_PER_UNIT.  */
+   bitsize = (bitsize + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1);
+ 
    /* Now nothing tells us how to pad out bitsize ...  */
    nextf = DECL_CHAIN (field);
    while (nextf && TREE_CODE (nextf) != FIELD_DECL)
*************** finish_bitfield_representative (tree rep
*** 1787,1798 ****
      {
        /* ???  If you consider that tail-padding of this struct might be
           re-used when deriving from it we cannot really do the following
! 	 and thus need to set maxsize to bitsize?  */
        tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)),
  				  DECL_FIELD_OFFSET (repr));
!       gcc_assert (host_integerp (maxsize, 1));
!       maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
! 		    - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
      }
  
    /* Only if we don't artificially break up the representative in
--- 1790,1805 ----
      {
        /* ???  If you consider that tail-padding of this struct might be
           re-used when deriving from it we cannot really do the following
! 	 and thus need to set maxsize to bitsize?  Also we cannot
! 	 generally rely on maxsize to fold to an integer constant, so
! 	 use bitsize as fallback for this case.  */
        tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)),
  				  DECL_FIELD_OFFSET (repr));
!       if (host_integerp (maxsize, 1))
! 	maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
! 		      - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
!       else
! 	maxbitsize = bitsize;
      }
  
    /* Only if we don't artificially break up the representative in
*************** finish_bitfield_representative (tree rep
*** 1801,1809 ****
       at byte offset.  */
    gcc_assert (maxbitsize % BITS_PER_UNIT == 0);
  
-   /* Round up bitsize to multiples of BITS_PER_UNIT.  */
-   bitsize = (bitsize + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1);
- 
    /* Find the smallest nice mode to use.  */
    for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
         mode = GET_MODE_WIDER_MODE (mode))
--- 1808,1813 ----
*************** finish_bitfield_layout (record_layout_in
*** 1884,1889 ****
--- 1888,1896 ----
  	}
        else if (DECL_BIT_FIELD_TYPE (field))
  	{
+ 	  gcc_assert (repr != NULL_TREE
+ 		      && operand_equal_p (DECL_FIELD_OFFSET (repr),
+ 					  DECL_FIELD_OFFSET (field), 0));
  	  /* Zero-size bitfields finish off a representative and
  	     do not have a representative themselves.  This is
  	     required by the C++ memory model.  */
Index: gcc/expr.c
===================================================================
*** gcc/expr.c	(revision 185426)
--- gcc/expr.c	(working copy)
*************** get_bit_range (unsigned HOST_WIDE_INT *b
*** 4452,4458 ****
  	       HOST_WIDE_INT bitpos)
  {
    unsigned HOST_WIDE_INT bitoffset;
!   tree field, repr, offset;
  
    gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
  
--- 4452,4458 ----
  	       HOST_WIDE_INT bitpos)
  {
    unsigned HOST_WIDE_INT bitoffset;
!   tree field, repr;
  
    gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
  
*************** get_bit_range (unsigned HOST_WIDE_INT *b
*** 4467,4477 ****
      }
  
    /* Compute the adjustment to bitpos from the offset of the field
!      relative to the representative.  */
!   offset = size_diffop (DECL_FIELD_OFFSET (field),
! 			DECL_FIELD_OFFSET (repr));
!   bitoffset = (tree_low_cst (offset, 1) * BITS_PER_UNIT
! 	       + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
  	       - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
  
    *bitstart = bitpos - bitoffset;
--- 4467,4475 ----
      }
  
    /* Compute the adjustment to bitpos from the offset of the field
!      relative to the representative.  DECL_FIELD_OFFSET of field and
!      repr are the same by construction.  */
!   bitoffset = (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
  	       - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
  
    *bitstart = bitpos - bitoffset;