On Wed, Jul 2, 2014 at 7:10 AM, DJ Delorie <d...@redhat.com> wrote: > > Revisiting an old thread, as I still want to get this feature in... > > https://gcc.gnu.org/ml/gcc/2012-10/msg00099.html > >> >> Why do you need to change varasm.c at all? The hunks seem to be >> >> completely separate of the attribute. >> > >> > Because static constructors have fields in the original order, not the >> > reversed order. Otherwise code like this is miscompiled: >> >> Err - the struct also has fields in the original order - only the bit >> positions >> of the fields are different because of the layouting option. > > The order of the field decls in the type (stor-layout.c) is not > changed, only the bit position information. The order here *can't* be > changed, because the C language assumes that parameters, initializers, > etc are presented in the same order as the original declaration, > regardless of the target-specific layout. > > When the program includes an initializer: > >> > struct foo a = { 1, 2, 3 }; > > The order of 1, 2, and 3 need to correspond to the order of the > bitfields in 'a', so we can change neither the order of the bitfields > in 'a' nor the order of constructor fields. > > However, when we stream the initializer out to the .S file, we need to > pack the bitfields in the right sequence to generate the right bit > patterns in the final output image. The code in varasm.c exists to > make sure that the initializers for bitfields are written/packed in > the correct order, to correspond to the bitfield positions. I.e. the > 1,2,3 initializer needs to be written to the .S file as either 0x0123 > or 0x3210 depending on the bit positions.
Ok, but as we are dealing exclusively with bitfields there is already output_constructor_bitfield which uses an intermediate state to "pack" bits into units that are then emitted. It shouldn't be hard to change that to make it pack into the appropriate bits instead. > In neither case do we change the order of the fields in the type > itself, i.e. the array/chain order. > >> And you expect no other code looks at fields of a structure and its >> initializer? It's bad to keep this not in-sync. Thus I don't think it's >> viable to re-order fields just because bit allocation is reversed. > > The fields are in sync. The varasm.c change sorts the elements as > they're being output into the byte stream in the .S, it doesn't sort > the field definitions themselves. > >> > + /* If the bitfield-order attribute has been used on this >> > + structure, the fields might not be in bit-order. In that >> > + case, we need a separate representative for each >> > + field. */ >> > The typical use-case for this feature is memory-mapped hardware, where >> > pessimum access is preferred anyway. >> >> I doubt that, looking at constraints for strict volatile bitfields. > > The code that handles representatives requires (via an assert, IIRC) > that the bit offsets within a representative be in ascending order. Well, because that's supposed to be an invariant in all record field-decls ... (which you break - for example fold_ctor_reference and friends might be unhappy about this as well). Note that code expects that representatives are byte-aligned so better would be to not assign representatives or make the code work with the swapped layout (I see no reason why that shouldn't work - maybe it works doing before swapping the layout)? > I.e. gcc ICEs if I don't bypass this. In the case of volatile > bitfields, which would be the typical use case for a reversed > bitfield, the access mode is going to match the type size regardless, > so performance is not changed by this patch. representatives are not about performance but about correctness. I'm still not happy about the idea in general (why is this a bitfield exclusive thing? If a piece of HW is big/little-endian then even regular fields would have that property. Your patch comes with no testcase - testcases should cover all attribute variants, multiple bitfield (group) sizes and mixed initializations / reads / writes and be best execute testcases. Richard.
