memcpy() and volatile

[Simon]

If you made the variable static const and assigned value of 28000 to it, then the compiler is effectively treating it as if it was a constant.⁽¹⁾ So that would indeed make the issue go away. But I assume your goal was to have it as a variable, right? I quickly looked in the code you provided later and it seems you use it as a constant, though.

Well I was happy to leave it to the compiler to optimise it, really it should have been a constant.

[Simon]

In that regard: no. It retains all the complexity of C and adds its own.

well that was my assumption. So I have continued with C and put C++ on the back burner as i can probably work without it and have much more important things to get into like linux programming at which point i will learn the background that i am missing now.

[Siwastaja]

Here, I quickly found a serious bug which overruns other variables in .bss:

Code: [Select]

    static  int16_t i16_bit_var ;
        ...
switch(number_bytes)
{
case 0x82 :
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;

You are copying 130(!!!) bytes from something that is only 4 bytes long (can_data[4 to 7]), to something that is only 2 bytes long (int16), overrunning the target by 128 bytes.

Always with memcpy, be super careful with the length, and prefer the pattern of using sizeof target as the final parameter, instead of some int n:

mymemcpy(&i16_bit_var, &can_data[4], sizeof i16_bit_var);

Homework: find the rest of such bugs and fix them.

Now you see how changing compiler settings or adding random keywords causes the compiler/linker to place variables in different order, so that something else gets corrupted and the bug seems to move elsewhere. Therefore by doing such changes, you are wasting time.

oh shit thanks, yes that should have a number_bytes &= 0x0F ; at the start of every case.

Why are the xxx_bit_vars static, anyway? You use them as temporary variables on two adjacent lines of code:

Code: [Select]

   static  int16_t i16_bit_var ;
   ...
   mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
   server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;

should simply become

Code: [Select]

   {
      int16_t i16_bit_var ;
      mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
      server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;
    } break;   // braces added to create local namespace for i16_bit_var.

and now you would save your precious RAM because compiler doesn't need to reserve global space from .bss for these variables. Only one CASE is entered per function execution, so local variable only uses one variable worth of stack. Compiler could even completely optimize out both the memcpy, and the variable! (Especially if you get rid of mymemcpy and just use standard memcpy, which the compiler can recognize).

And what's best, code becomes much more readable as the variable is declared just before use so you get visual context from reading the code.

What does "number_bytes" mean anyway, clearly it isn't actually number of bytes, but some MSB flag, too? Name it properly.

Then why do both of these CASEs do the exact same thing:

Code: [Select]


                                                case 0x84 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;

case 0x04 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;

In other cases, the highest bit seems to signify the signedness, but not for the 4-byte object. Is this intentional or a copy-pasta mistake? If latter, try to refactor to get rid of copypasta.



I'm not particularly fast/good at finding bugs in other's code. The fact I found these so quickly indicates there probably are many more.

[Andy Watson]

I am not familiar with this compiler or processor - so this comment may not be applicable - but:

When dealing with structures, especially of mixed variable type, like say this example (picked at random)

Code: [Select]

typedef struct
{
uint16_t index ;
uint8_t  sub_index ;
uint8_t  type ; // 0x80 for signed 0x40 for read only 0x20 for disable initialize in node dictionary
int32_t  value[N_CANOPEN_NODES] ;
uint32_t last_received[N_CANOPEN_NODES] ;
uint32_t last_sent[N_CANOPEN_NODES] ;

}   nodes_objects_def_t ;

there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise. This will really screw with your data if you carelessy use byte-copying functions (like memcpy).

If you are copying bytes in or out of a mixed variable structure it would be safer to force the compiler to "pack" the structure such that there are no gaps. In gcc this would mean defining the structure with the attribute __packed__

[Simon]

Here, I quickly found a serious bug which overruns other variables in .bss:

Code: [Select]

    static  int16_t i16_bit_var ;
        ...
switch(number_bytes)
{
case 0x82 :
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;

You are copying 130(!!!) bytes from something that is only 4 bytes long (can_data[4 to 7]), to something that is only 2 bytes long (int16), overrunning the target by 128 bytes.

Always with memcpy, be super careful with the length, and prefer the pattern of using sizeof target as the final parameter, instead of some int n:

mymemcpy(&i16_bit_var, &can_data[4], sizeof i16_bit_var);

Homework: find the rest of such bugs and fix them.

Now you see how changing compiler settings or adding random keywords causes the compiler/linker to place variables in different order, so that something else gets corrupted and the bug seems to move elsewhere. Therefore by doing such changes, you are wasting time.

oh shit thanks, yes that should have a number_bytes &= 0x0F ; at the start of every case.

Why are the xxx_bit_vars static, anyway? You use them as temporary variables on two adjacent lines of code:

Code: [Select]

   static  int16_t i16_bit_var ;
   ...
   mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
   server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;

should simply become

Code: [Select]

   {
      int16_t i16_bit_var ;
      mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
      server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;
    } break;   // braces added to create local namespace for i16_bit_var.

and now you would save your precious RAM because compiler doesn't need to reserve global space from .bss for these variables. Only one CASE is entered per function execution, so local variable only uses one variable worth of stack. Compiler could even completely optimize out both the memcpy, and the variable! (Especially if you get rid of mymemcpy and just use standard memcpy, which the compiler can recognize).

And what's best, code becomes much more readable as the variable is declared just before use so you get visual context from reading the code.

What does "number_bytes" mean anyway, clearly it isn't actually number of bytes, but some MSB flag, too? Name it properly.

Then why do both of these CASEs do the exact same thing:

Code: [Select]


                                                case 0x84 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;

case 0x04 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;

In other cases, the highest bit seems to signify the signedness, but not for the 4-byte object. Is this intentional or a copy-pasta mistake? If latter, try to refactor to get rid of copypasta.



I'm not particularly fast/good at finding bugs in other's code. The fact I found these so quickly indicates there probably are many more.

Well I set up static variables as these are needed often so I thought it will be a waste of time to create them every time. but then given the time it takes to find the correct array element I am probably saving no time. I am up to 63% of my 16kB RAM so I'm not too precious about it but yes I may as well have those created on the fly, the same applies to another function.

number_bytes is broadly the number of byes but 3 MSb are used as flags. As you correctly guessed bit 7 is to indicate a signed variable.

So if you look in canopen_dictionary.h you will find a structure typdef, this is the building block of the dictionary, there is an array of these structures. The structure for each dictionary element holds the index and subindex of the can open system. The type variable specifies the number of bytes, whether it is signed, if it is a read only variable and if it is to be sent as part of the initialization of the can open device(s).

This little setup allows the data to be handled in multiple ways. Yes the int32 for both 0x04 and 0x84 is correct. Somewhere there is a thread I started where I presented my reasoning.

Basically it makes sense to have the data in one array, but the data can be any of the 6 types. Initially I experimented with an array of pointers to the various types of variables but this was not the best either and in any case I wound still need to use 32 bits of storage for the pointers. So I reasoned that each variable shall be stored as an int32 as this will store anything the other 5 types store if the uint32 does not exceed 2^31 for positive numbers. I am yet to find a number in the CAN open devices I am using that exceeds this. In fact interestingly one of the objects I use is stated in the manual to be a uint32 with a maximum number of 2^31, I think the person who programmed these devices also followed my logic.

[Simon]

I am not familiar with this compiler or processor - so this comment may not be applicable - but:

When dealing with structures, especially of mixed variable type, like say this example (picked at random)

Code: [Select]

typedef struct
{
uint16_t index ;
uint8_t  sub_index ;
uint8_t  type ; // 0x80 for signed 0x40 for read only 0x20 for disable initialize in node dictionary
int32_t  value[N_CANOPEN_NODES] ;
uint32_t last_received[N_CANOPEN_NODES] ;
uint32_t last_sent[N_CANOPEN_NODES] ;

}   nodes_objects_def_t ;

there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise. This will really screw with your data if you carelessy use byte-copying functions (like memcpy).

If you are copying bytes in or out of a mixed variable structure it would be safer to force the compiler to "pack" the structure such that there are no gaps. In gcc this would mean defining the structure with the attribute __packed__

This data is not being messed about with by pointers. I use pointers to the array element but I don't tend to point into the structure.

As a general rule if the structure is actually packed out with data that lines up to 4 bytes it will be stored as written. In fact it is my understanding that the pack directive will actually meddle with the structure layout to optimize it as instructed so you cannot then use pointers to copy data to a structure variable as you don't know how the bytes are ordered. Same problem with data being sent over a serial bus. I had to have a lot of arguments with someone about that as we were unable to communicate as he wanted to compact the data as much as possible and totally missed that now we did not know where in many bytes any bit of data was so that it could be recovered at the other end.

[Siwastaja]

there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise. This will really screw with your data if you carelessy use byte-copying functions (like memcpy).

More accurately - aligns each member to its own natural alignment requirement (that would imply word alignment only for word-wide members). In any case, padding should not affect memcpy when used in sane way: always use sizeof struct. But indeed, if one goes through extra hassle of adding up member sizes manually and using that as n for memcpy, things fail because of padding.

Utilize sizeof, instead of magic numbers, makes your life much easier.

[Simon]

I always pad structures out to align manually, habit of that first time I used a structure to hold can bus data and had to know where the data was so manual alignment was the only option. The person that wanted to use the pack thing just made it into a bit soup as we no longer knew where any of the data was which does not work when you communicate between devices.

[ejeffrey]

So a new problem cropped up, same thing, variable being ignored at random. Making it "static" solves this. The variable in only used in this one C file. So why look at code anywhere else to determine it's usefulness ?

Oh my god stop this.  Randomly permuting the code to see what makes it "work" according to your expectations is a recipe for disaster. 

Instead, do this (this advice is specific for you, not an approach that should be taken 100% by everyone):

First: assume the compiler is correct.  It's definitely true that compilers have bugs, but the vast, vast majority of problems are user errors, not compiler bugs.  So get that out of your head, and also get out that the compiler is "not doing what you tell it" -- this is basically equivalent to calling it a compiler bug.  The compiler always does what you tell it, often people just don't know what they are telling it.  With your mindset fixed, follow this procedure:

Comment out all casts and all uses of volatile in the entire code base.  Compile with -Wall -Werror.   Maybe -Wextra for good measure. Keep *fixing* bugs until your code compiles -- not working around, but actually fixing.  If you absolutely have to add back in a cast, write a comment about why it is correct, and ask an expert if there is a way around it.  If they say yes, even if it includes changing a bunch of code, do it.  volatile should *only ever* be used for hardware registers and global variables accessed by interrupt handlers.  All other users should not be allowed under any circumstances.  Then if you can possibly run your code with valgrind, ubsan or any other kind of memory testing.  This is usually hard on embedded systems, but if you can run in an emulator or cross compile for a PC, even for parts of the code do so, and keep *fixing* bugs until those run without complaint.

Now you have code that is reasonably likely to have well defined behavior.  It might not be correct, but it is code worth debugging.  Code that changes behavior when you make meaningless changes is not worth debugging.  My recommendation then is to now skip directly to -O2.  When you find a bug, then switch back to -Og or -O0 to debug it.  The reason to do this is mostly that it will be easier to understand what is going on in an interactive debugger.  It doesn't matter what debugging tool you use: gdb, printf, toggling GPIO pins, or likely a combination of all three and more. What is good is that all of the work you did above will make it much less likely that random permutations of your code during debugging will cause the issue to go away.  This is good: ultimately predictable systems are easy to debug and unpredictable ones difficult.  What you have gained by following these steps is both made your code more predictable (in that it avoids undefined or many types of surprising behavior) and you have improved your ability to predict the C language.  Starting with -O2 helps you because you never get to a situation of having a program that works with a debug build and you are afraid to turn on optimizations because it might break.  In addition, if the behavior changes when you turn off optimizations, then you have already determined that either the problem is undefined behavior or a timing error.  That means you are already well on your way to diagnosing and fixing it.

But the #1 piece of advice I would distill from this is: use volatile only for hardware registers and global variables used to communicate between interrupt handlers (or async signal handlers) and the main thread or other normal threads.  Any other use is guaranteed incorrect.

[langwadt]

Well I set up static variables as these are needed often so I thought it will be a waste of time to create them every time.

setting them as static likely just wastes memory and the cycles to restore and store them in memory, when it isn't needed and the compiler would likely just use a register instead if it wasn't static

[IanB]

Well I set up static variables as these are needed often so I thought it will be a waste of time to create them every time.

It may be that the opposite is true. For the compiler, an automatic variable is "normal" and a static variable is "special". The compiler has to do extra work to allocate, preserve and access a static variable. An automatic variable may take no work at all. Either the compiler will just place it in a spare register, or if not it will just increment the stack pointer to make a slot for it on the stack. The cost of incrementing and decrementing the stack pointer is trivial.

[SiliconWizard]

A *local* variable that is qualified static is basically just like a static global variable, except that it's only visible locally (so, only the visibility differs).

[ejeffrey]

As a general rule if the structure is actually packed out with data that lines up to 4 bytes it will be stored as written. In fact it is my understanding that the pack directive will actually meddle with the structure layout to optimize it as instructed so you cannot then use pointers to copy data to a structure variable as you don't know how the bytes are ordered. Same problem with data being sent over a serial bus. I had to have a lot of arguments with someone about that as we were unable to communicate as he wanted to compact the data as much as possible and totally missed that now we did not know where in many bytes any bit of data was so that it could be recovered at the other end.

If you use pack you know that the bytes will be packed in with no padding.  You still need to know the endianness and size, but once you do then you know the bytewise layout.  You can use memcpy on the entire struct as usual to copy it in or out of a buffer to send over the wire, but you can't use normal pointers to individual members because they might be misaligned.  So the easiest way to get the data into a format that you can use more flexibly is to make a non-packed version of the structure and do element-by-element assignment from one to the other.  The compiler should warn you about misusing packed structures unless you silence those with an explicit cast, so don't do that.  I'm not sure exactly what you want to do with "cannot use pointers to copy data to a structure variable" -- that might or might not be OK depending on what you mean. 

[ejeffrey]

there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise.

No, it will not align based on "what it is trying to optimize." it will align them based on the ABI specification for the platform which on modern systems almost always requires natural alignment (alignment of primitive types == their size) for the standard primitive types.  Of course, if a particular structure instance is only used as a local variable and proven to not be accessed outside the compilers visibility it is free to do optimizations that include removing the struct entirely while operating under the aegis of the "as-if" rule.  But the externally-visible memory layout for programs that have defined behavior is fixed by the ABI and not subject to being changed by the compiler without warning.

[SiliconWizard]

Also, the only way you can ensure a specific layout of global variables in memory is to write an appropriate linker script, if said variables are distinct.
But the very simple way not requiring that would be to define those "variables" that need specific ordering in a global struct, which guarantees the ordering of its members in C. And then, you can either rely on the alignment ensured by the ABI, or specify one with alignas directives on the struct members (or a 'packed' attribute for the structure, which is not standard C, but supported both by GCC and Clang.) alignas, OTOH, is standard C11. But while alignas guarantees a certain alignment, the compiler is free to use a larger alignment than what's specified (which would still comply), so not a failsafe way of forcing specific 'offsets' for struct members.

The "best" way of forcing a specific layout in memory, as long as you use GCC or Clang as a compiler, is to define a struct, give it the packed attribute, and 'pad' members as appropriate to yield the exact layout you want.
(Note that even if you use a different C compiler that doesn't support those attributes, the venerable "#pragma pack(1)" has been available on almost all C compilers I've run into for as long as I can remember (mid-nineties).)

[Simon]

If you use pack you know that the bytes will be packed in with no padding.  You still need to know the endianness and size, but once you do then you know the bytewise layout.  You can use memcpy on the entire struct as usual to copy it in or out of a buffer to send over the wire, but you can't use normal pointers to individual members because they might be misaligned. 

That is itself a contradiction. If I can use memcpy on the structure then pointers are OK. I think you are however referring to copying the entire structure. I am talking about a structure that spans multiple messages so you must know where the data is by putting it there yourself.

The idea is that no matter how many messages are needed to send the data you will always have a single structure for all data sent and one for all data received. I am not taking about can open type data here but limited information that is exchanged between two devices for example the speed demand from a HMI and the actual speed sent to the HMI. We ended up with up to 24 bytes of this sort of stuff and to avoid struct1. struct2. struct3. the simplest way is to lay the data out manually and know where it is. Then you know exactly what you send in each message (J1939 style) and the program is easier to manage.

[DiTBho]

The "best" way of forcing a specific layout in memory, as long as you use GCC or Clang as a compiler, is to define a struct, give it the packed attribute, and 'pad' members as appropriate to yield the exact layout you want.
(Note that even if you use a different C compiler that doesn't support those attributes, the venerable "#pragma pack(1)" has been available on almost all C compilers I've run into for as long as I can remember (mid-nineties).)

That's the case for { Sierra-C, Avocet-C, Panasonic-SH-C, ... }
(talking about DOS-v5, Windows-3.*, Linux-v2.* era, so old tools, but still in use ...)

They don't support gcc-attributes, and they don't even support #pragma pack.
so, my solution was/is:
define structs in assembly, as "module" (.S -> .o) to yield the exact layout you want
define symbols in a header file (extern)
make good use, every C-compiler will adapt

[gf]

If you use pack you know that the bytes will be packed in with no padding.  You still need to know the endianness and size, but once you do then you know the bytewise layout.  You can use memcpy on the entire struct as usual to copy it in or out of a buffer to send over the wire, but you can't use normal pointers to individual members because they might be misaligned. 

That is itself a contradiction. If I can use memcpy on the structure then pointers are OK. I think you are however referring to copying the entire structure. I am talking about a structure that spans multiple messages so you must know where the data is by putting it there yourself.

You missed the point: Assigning the address of a member of type T inside a packed structure object to a T* pointer is problematic. The pointer value may not be properly aligned for a subsequent access to the member if alignof(T) > 1.

[DiTBho]

there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise.

No, it will not align based on "what it is trying to optimize." it will align them based on the ABI specification for the platform which on modern systems almost always requires natural alignment (alignment of primitive types == their size) for the standard primitive types.  Of course, if a particular structure instance is only used as a local variable and proven to not be accessed outside the compilers visibility it is free to do optimizations that include removing the struct entirely while operating under the aegis of the "as-if" rule.  But the externally-visible memory layout for programs that have defined behavior is fixed by the ABI and not subject to being changed by the compiler without warning.

Yup, ABI-spec rules everything.

[Simon]

so if I turn pedantic warnings on to try and weed stuff out I get errors likes:

Warning      ISO C forbids conversion of function pointer to object pointer type [-Wpedantic]   SAMC21J starter project

But this is in the micro controller header files so very much not an issue or at least not one I can deal with.

what version of C should I be using? it seems to be set to: -std=gnu99

[newbrain]

Good point too. On a machine with bytes larger than 8 bits this will only copy the lower 8 bits of each byte, leaving 0s in the rest.

I know I'm late, and the thread has moved on, but my inner pedant was itching since I saw this yesterday...

In C99 through C23, the exact width integer types (intN_t, uintN_t) are defined to have no padding bits.
So, if the byte (i.e. char, the minimum amount of memory) is larger than 8 bits, int8_t and uint8_t types cannot exist.

Conversely they must exist if the machine can provide them - still, with no padding, and since C11 with 2's complement representation.

Ref: 7.[18,20,22].1.1 Exact-width integer types, respectively in C99, C11 and C23 (draft).
From C11:

The typedef name intN_t designates a signed integer type with width N , no padding
bits, and a two’s complement representation. Thus, int8_t denotes such a signed
integer type with a width of exactly 8 bits.
The typedef name uintN_t designates an unsigned integer type with width N and no
padding bits. Thus, uint24_t denotes such an unsigned integer type with a width of
exactly 24 bits.
These types are optional. However, if an implementation provides integer types with
widths of 8, 16, 32, or 64 bits, no padding bits, and (for the signed types) that have a
two’s complement representation, it shall define the corresponding typedef names.

[Siwastaja]

structure then pointers are OK.

Be careful here:
* Pointers to the struct, packed or not, are always OK
* Pointers to the MEMBERS of the struct are always OK for a normal, non-packed struct
* Pointers to the MEMBERS of the packed struct might or might not be OK (depending on struct, object attributes, and even platform), and as such require special care and pretty deep understanding. Avoid.

Really, just compile with -Wall -Werror. Taking pointer of packet struct which might produce unaligned access generates compiler warning. At least usually.

[IanB]

structure then pointers are OK.

Be careful here:
* Pointers to the struct, packed or not, are always OK
* Pointers to the MEMBERS of the struct are always OK for a normal, non-packed struct
* Pointers to the MEMBERS of the packed struct might or might not be OK (depending on struct, object attributes, and even platform), and as such require special care and pretty deep understanding. Avoid.

For understanding and clarity, a standard idiom in C is to reference structure members through a base pointer, like this:

Code: [Select]

var = structPtr->member;My expectation is that this idiom would not be invalidated by having a packed structure. Is that right?

[Siwastaja]

For understanding and clarity, a standard idiom in C is to reference structure members through a base pointer, like this:

Code: [Select]

var = structPtr->member;My expectation is that this idiom would not be invalidated by having a packed structure. Is that right?

Correct, that works fine, because compiler sees type of structPtr, and as such, knows it's packed and generates the correct instructions to access unaligned data.

Safe:

Code: [Select]

typedef struct __attribute__((packed))
{
   uint8_t a;
   uint32_t b;
} s_t;

s_t s;
...
s_t* p_s = &s;
...
p_s->b = 420;

Not safe:

Code: [Select]

typedef struct __attribute__((packed))
{
   uint8_t a;
   uint32_t b;
} s_t;

s_t s;

uint32_t * p_b = &s.b;
*p_b = 420;


[Simon]

   mpm_to_rpm   0x20002768   const int32_t(static storage at address 0x0.)

This is from the watch window regarding a variable called "mpm_to_rpm" do i read correctly that it is located at address 0 ? I know enough to know that that is bad!