[C++0x] contiguous bitfields race implementation

[Richard Guenther]

On Fri, Aug 5, 2011 at 7:25 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Alright, I'm back and bearing patches.  Firmly ready for the crucifixion you
> will likely submit me to. :)
>
> I've pretty much rewritten everything, taking into account all your
> suggestions, and adding a handful of tests for corner cases we will now
> handle correctly.
>
> It seems the minimum needed is to calculate the byte offset of the start of
> the bit region, and the length of the bit region.  (Notice I say BYTE
> offset, as the start of any bit region will happily coincide with a byte
> boundary).  These will of course be adjusted as various parts of the
> bitfield infrastructure adjust offsets and memory addresses throughout.
>
> First, it's not as easy as calling get_inner_reference() only once as you've
> suggested.  The only way to determine the padding at the end of a field is
> getting the bit position of the field following the field in question (or
> the size of the direct parent structure in the case where the field in
> question is the last field in the structure).  So we need two calls to
> get_inner_reference for the general case.  Which is at least better than my
> original call to get_inner_reference() for every field.
>
> I have clarified the comments and made it clear what the offsets are
> relative to.
>
> I am now handling large offsets that may appear as a tree OFFSET from
> get_inner_reference, and have added a test for one such corner case,
> including nested structures with head padding as you suggested.  I am still
> unsure that a variable length offset can happen before a bit field region.
>  So currently we assert that the final offset is host integer representable.
>  If you have a testcase that invalidates my assumption, I will gladly add a
> test and fix the code.
>
> Honestly, the code isn't pretty, but neither is the rest of the bit field
> machinery.  I tried to make due, but I'll gladly take suggestions that are
> not in the form of "the entire bit field code needs to be rewritten" :-).
>
> To aid in reviewing, the crux of everything is in the rewritten
> get_bit_range() and the first block of store_bit_field().  Everything else
> is mostly noise.  I have attached all of get_bit_range() as a separate
> attachment to aid in reviewing, since that's the main engine, and it has
> been largely rewritten.
>
> This pacth handles all the testcases I could come up with, mostly inspired
> by your suggestions.  Eventually I would like to replace these target
> specific tests with target-agnostic tests using the gdb simulated thread
> test harness in the cxx-mem-model branch.
>
> Finally, you had mentioned possible problems with tail padding in C++, and
> suggested I use DECL_SIZE instead of calculating the padding using the size
> of direct parent structure.  DECL_SIZE doesn't include padding, so I'm open
> to suggestions.
>
> Fire away, but please be kind :).

Just reading and commenting top-down on the new get_bit_range function.

      /* If we have a bit-field with a bitsize > 0... */
      if (DECL_BIT_FIELD_TYPE (fld)
	  && (!host_integerp (DECL_SIZE (fld), 1)
	      || tree_low_cst (DECL_SIZE (fld), 1) > 0))

DECL_SIZE should always be host_integerp for bitfields.

	      /* Save starting bitpos and offset.  */
	      get_inner_reference (build3 (COMPONENT_REF,
					   TREE_TYPE (exp),
					   TREE_OPERAND (exp, 0),
					   fld, NULL_TREE),
				   &bitsize, &start_bitpos, &start_offset,
				   &mode, &unsignedp, &volatilep, true);

ok, so now you do this only for the first field in a bitfield group.  But you
do it for _all_ bitfield groups in a struct, not only for the interesting one.

May I suggest to split the loop into two, first searching the first field
in the bitfield group that contains fld and then in a separate loop computing
the bitwidth?

Backing up, considering one of my earlier questions.  What is *offset
supposed to be relative to?  The docs say sth like "relative to INNERDECL",
but the code doesn't contain a reference to INNERDECL anymore.

I think if the offset is really supposed to be relative to INNERDECL then
you should return a split offset, similar to get_inner_reference itself.
Thus, return a byte tree offset plus a HWI bit offset and maxbits
(that HWI bit offset is the offset to the start of the bitfield group, right?
Not the offset of the field that is referenced?)

It really feels like you should do something like

  /* Get the offset to our parent structure.  */
  get_inner_reference (TREE_OPERAND (exp, 0), &offset, &bit_offset....);

  for (fld = TYPE_FIELDS (...) ...)
    /* Search for the starting field of the bitfield group of
TREE_OPERAND (exp, 1) */

  offset += DECL_FIELD_OFFSET (first_field_of_group);
  bit_offset += DECL_FIELD_BIT_OFFSET (first_field_of_group);
  (well, basically copy what get_inner_reference would do here)

  for (...)
    accumulate bit-offsets of the group (mind they'll eventually wrap
    when hitting DECL_OFFSET_ALIGN) to compute maxbits
    (that also always will fit in a HWI)

Now we come to that padding thing.  What's the C++ memory model
semantic for re-used tail padding?  Consider

  struct A
  {
     int i;
     bool a:1;
  }
  struct B : public A
  {
     bool b:1;
  }

The tail-padding of A is 3 bytes that may be used by b.  Now, is
accessing a allowed to race with accessing b?  Then the group for
a may include the 3 bytes tail padding.  If not, then it may not
(in which case using DECL_SIZE would be appropriate).

There is too much get_inner_reference and tree folding stuff in this
patch (which makes it expensive given that the algorithm is still
inherently quadratic).  You can rely on the bitfield group advancing
by integer-cst bits (but the start offset may be non-constant, so
may the size of the underlying record).

Now seeing all this - and considering that this is purely C++ frontend
semantics.  Why can't the C++ frontend itself constrain accesses
according to the required semantics?  It could simply create
BIT_FIELD_REF <MEM_REF <&containing_record,
byte-offset-to-start-of-group>, bit-size, bit-offset> for all bitfield
references (with a proper
type for the MEM_REF, specifying the size of the group).  That would
also avoid issues during tree optimization and would at least allow
optimizing the bitfield accesses according to the desired C++ semantics.

Richard.