views:

33

answers:

1

This code is for Microchip's PIC32MX microprocessor. Their compiler is essentially GCC 3.4.

I tend use GCC's __packed__ attribute to pack bitfields into a union, and later retrieve them as an unsigned char (ie. type-punning) for sending over SPI or I2C. This behaviour is all defined by my implementation, and works perfectly. I prefer this to a hundred or so lines of masking and shifting :)

My question is: are there __packed__ attributes in the code below that are redundant? At first glance, I would think that those on the top-level union members can be dispensed with, but I'm not so sure. Or can I leave out those in the nested struct?

// Remember that bitfields cannot straddle word boundaries!
typedef struct
{
    /// Some flag #1
    unsigned FlagOne            : 1 __attribute__((packed));
    /// Some flag #2
    unsigned FlagTwo            : 1 __attribute__((packed));
    /// A chunk of data
    unsigned SomeData           : 5 __attribute__((packed));

    // and so on, maybe up to 32 bits long depending on the destination

} BlobForSomeChip;

/// This kind of type-punning is implementation defined. Read Appendix A (A7, A12) of
/// the MPLAB C Compiler for PIC32 MCUs manual.
typedef union
{
    /// Access the members of this union to set flags, etc
    BlobForSomeChip blobdata __attribute__((packed));

    /// As a byte for sending via SPI, I2C etc
    unsigned char bytes[4] __attribute__((packed));

} BlobData;
+1  A: 

First of all, I recommend compiling with -Wall.

Now:

  1. The BlobForSomeChip struct has 7 bits declared. Normally, it would be 4 bytes long due to alignment, but with packed attributes it will be only 1 byte long.
  2. A unsigned char[4] can't be packed. It will always be 4 bytes long, no matter what.

In short:

  1. struct BlobForSomeChip = 1 byte
  2. unsigned char[4] = 4 bytes
  3. BlobData = 4 bytes (its largest member's size).

Concluding, the packed attributes on BlobData are not required. GCC will simply ignore them if used - see the output using -Wall.

jweyrich
For point 1 above - the `// and so on` comment was meant to imply that it could be anything up to 32 bits long. I would normally pad anything short of an 8 bit multiple with unnamed bitfields. So consider also the case that `BlobForSomeChip` is exactly 32 bits (but your explanation should apply then as well, I think).
detly
@detly: correct, the size in bytes is always ceil(total bits / 8). The packed attribute only removes padding/alignment. N explicit/declared bits will always be N bits.
jweyrich
`-Wall` does not seem to catch this on GCC 3.4 (at least in MC's compiler). I'll check against the Debian packaged version. But other than that, good explanation, thanks :)
detly
Oh, for the sake of completeness, if my bitfield struct **is** incomplete (ie. < 32 bits), is the way in which it is aligned relative to the other union member defined by the standard, by the implementation, or not at all?
detly
@detly: sorry, I didn't pay attention to the compiler version you mentioned. That may be correct, I guess only GCC>=4.1 checks for this, my bad. As for the last comment, there's no alignment between members of an union because all members start at the same address - the union's address. And since there will be no padding, its size always matches its largest member size.
jweyrich
Okay, that makes sense. Thanks :)
detly