CODEWITHSUNDEEP
cstructmemcpy
Aune
Aune

Reputation: 253

using memcpy for structs

I have a problem when using memcpy on a struct.

Consider the following struct

struct HEADER
{
    unsigned int preamble;
    unsigned char length;
    unsigned char control;
    unsigned int destination;
    unsigned int source;
    unsigned int crc;
}

If I use memcpy to copy data from a receive buffer to this struct the copy is OK, but if i redeclare the struct to the following :

struct HEADER
{
    unsigned int preamble;
    unsigned char length;
    struct CONTROL control;
    unsigned int destination;
    unsigned int source;
    unsigned int crc;
}

struct CONTROL
{
    unsigned dir : 1;
    unsigned prm : 1;
    unsigned fcb : 1;
    unsigned fcb : 1;
    unsigned function_code : 4;
}

Now if I use the same memcpy code as before, the first two variables ( preamble and length ) are copied OK. The control is totally messed up, and last three variables are shifted one up, aka crc = 0, source = crc, destination = source...

ANyone got any good suggestions for me ?

Upvotes: 1

Views: 5510

Answers (5)

wildplasser
wildplasser

Reputation: 44240

The clean way would be to use a union, like in.:

struct HEADER
{
    unsigned int preamble;
    unsigned char length;
    union {
      unsigned char all;
      struct CONTROL control;
      } uni;
    unsigned int destination;
    unsigned int source;
    unsigned int crc;
};

The user of the struct can then choose the way he wants to access the thing.

struct HEADER thing = {... };

if (thing.uni.control.dir) { ...}

or

#if ( !FULL_MOON ) /* Update: stacking of bits within a word appears to depend on the phase of the moon */
if (thing.uni.all & 1) { ... }
#else
if (thing.uni.all & 0x80) { ... }
#endif

Note: this construct does not solve endianness issues, that will need implicit conversions.

Note2: and you'll have to check the bit-endianness of your compiler, too.

Also note that bitfields are not very useful, especially if the data goes over the wire, and the code is expected to run on different platforms, with different alignment and / or endianness. Plain unsigned char or uint8_t plus some bitmasking yields much cleaner code. For example, check the IP stack in the BSD or linux kernels.

Upvotes: 0

unwind
unwind

Reputation: 399803

Do you know that the format in the receive buffer is correct, when you add the control in the middle?

Anyway, your problem is that bitfields are the wrong tool here: you can't depend on the layout in memory being anything in particular, least of all the exact same one you've chosen for the serialized form.

It's almost never a good idea to try to directly copy structures to/from external storage; you need proper serialization. The compiler can add padding and alignment between the fields of a structure, and using bitfields makes it even worse. Don't do this.

Implement proper serialization/deserialization functions:

unsigned char * header_serialize(unsigned char *put, const struct HEADER *h);
unsigned char * header_deserialize(unsigned char *get, struct HEADER *h);

That go through the structure and read/write as many bytes as you feel are needed (possibly for each field):

static unsigned char * uint32_serialize(unsigned char *put, uint32_t x)
{
    *put++ = (x >> 24) & 255;
    *put++ = (x >> 16) & 255;
    *put++ = (x >> 8) & 255;
    *put++ = x & 255;
    return put;
}

unsigned char * header_serialize(unsigned char *put, const struct HEADER *h)
{
    const uint8_t ctrl_serialized = (h->control.dir << 7) |
                                    (h->control.prm << 6) |
                                    (h->control.fcb << 5) |
                                    (h->control.function_code);

    put = uint32_serialize(put, h->preamble);
    *put++ = h->length;
    *put++ = ctrl_serialized;
    put = uint32_serialize(put, h->destination);
    put = uint32_serialize(put, h->source);
    put = uint32_serialize(put, h->crc);

    return put;
}

Note how this needs to be explicit about the endianness of the serialized data, which is something you always should care about (I used big-endian). It also explicitly builds a single uint8_t version of the control fields, assuming the struct version was used.

Also note that there's a typo in your CONTROL declaration; fcb occurs twice.

Upvotes: 2

Siddhartha Ghosh
Siddhartha Ghosh

Reputation: 3188

Check sizeof(struct CONTROL) -- I think it would be 2 or 4 depending on the machine. Since you are using unsigned bitfields (and unsigned is shorthand of unsigned int), the whole structure (struct CONTROL) would take at least the size of unsigned int -- i.e. 2 or 4 bytes.

And, using unsigned char control takes 1 byte for this field. So, definitely there should be mismatch staring with the control variable.

Try rewriting the struct control as below:-

struct CONTROL
{
    unsigned char dir : 1;
    unsigned char prm : 1;
    unsigned char fcb : 1;
    unsigned char fcb : 1;
    unsigned char function_code : 4;
}

Upvotes: 0

Sadique
Sadique

Reputation: 22823

Memcpy copies the values of bytes from the location pointed by source directly to the memory block pointed by destination.

The underlying type of the objects pointed by both the source and destination pointers are irrelevant for this function; The result is a binary copy of the data. So if there is any structure padding then you will have messed up results.

Upvotes: 0

Ingo Leonhardt
Ingo Leonhardt

Reputation: 9894

Using struct CONTROL control; instead of unsigned char control; leads to a different alignment inside the struct and so filling it with memcpy() produces a different result.

Upvotes: 0

Related Questions