On Fri, Aug 5, 2011 at 7:25 PM, Aldy Hernandez <al...@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.