using memcpy for structs

Question

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 ?

Are you sure, that after change in HEADER, received buffer has exact same changes? — zoska, Oct 09 '13 at 11:25

unwind · Answer 1 · 2013-10-09T11:09:42.533

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.

Could you please explain a bit more what the serialization function does ? Never used anything like that before... — Aune, Oct 09 '13 at 10:52
When looking at the pointer memory ( the pointer to the struct ), the data is correctly located there, but when looking at the watch the data is scambled... — Aune, Oct 09 '13 at 10:57
@user1244472 In serialization function you should convert each struct member to bytes. — user694733, Oct 09 '13 at 11:09

score 0 · Answer 2 · answered Oct 09 '13 at 10:01

0

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.

answered Oct 09 '13 at 10:01

Ingo Leonhardt

9,435
2
24
33

score 0 · Answer 3 · answered Oct 09 '13 at 10:02

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.

score 0 · Answer 4 · answered Oct 09 '13 at 10:57

0

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;
}

answered Oct 09 '13 at 10:57

Siddhartha Ghosh

2,988
5
18
25

using unsigned char x : 1; is illegal – Aune Oct 09 '13 at 11:13
@user1244472: no it is not. Read about bitfields. – wildplasser Oct 09 '13 at 11:17
@user1244472: On gcc, it is compiling. Which compiler are you using? – Siddhartha Ghosh Oct 09 '13 at 11:55
crossWorks, I tried unsigned char a : 1; in AVR studio just now and experienced the same thing, it compiled there... Writing the same code in AVR studio now to see if it works there.. – Aune Oct 09 '13 at 12:07
I tried to search a bit. Refer this link:- [http://stackoverflow.com/questions/3971085/how-does-a-bit-field-work-with-character-types](http://stackoverflow.com/questions/3971085/how-does-a-bit-field-work-with-character-types). From the discussions, it seems handling char bit field are somewhat implementation defined. – Siddhartha Ghosh Oct 09 '13 at 12:20
Just tried the same thing in AVR studio now and it worked perfectly, both with and without the type declaration in the control struct.... Going to email the crossworks compilator guy to see if there is any compilator settings that might cause this behaviour – Aune Oct 09 '13 at 12:23
Got the reply for Crossworks guy and he said that the type definition in the bitfield is non-standard and just an extension in the GCC compiler and therefore not supported by crossworks. :( So no luch there.... Guess I'm back to re-writing the code :( – Aune Oct 10 '13 at 05:58
The official C only supports `[signed/unsigned] int` and `Bool_` bitfield types. Using `unsigned char` for bitfield might be allowed by a specific implementation or it might be prohibited. – AnT stands with Russia Oct 10 '13 at 18:39

wildplasser · Answer 5 · 2013-10-10T18:31:01.103

0

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.

edited Oct 10 '13 at 18:31

answered Oct 09 '13 at 11:07

wildplasser

43,142
8
66
109

Union alone won't work, because `all` and `control` -members have different sizes. – user694733 Oct 09 '13 at 11:08
Could you please explain in more detail how to use this ? – Aune Oct 09 '13 at 11:09
The struct Control is 8 bits – Aune Oct 09 '13 at 11:09
@user1244472 `struct Control` is larger than 8 bits, because you use `unsigned` type inside it. – user694733 Oct 09 '13 at 11:13
The size is not important, the union members are located at the same offset. If you want to constrain the thing to strictly 8 bits space you'll have to instruct your compiler not to pad the struct CONTROL inside the union. (and the resulting union) – wildplasser Oct 09 '13 at 11:16
@wildplasser Size matters because *"The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined."* (from C standard). It means that first bit-field member bit is not necessarily same as `uni.all & 1`. – user694733 Oct 10 '13 at 11:37
That's another _bit-endianness_ issue to consider. I'll update. – wildplasser Oct 10 '13 at 18:26

using memcpy for structs

5 Answers5