memcpy() and volatile

[Simon]

My program is totally failing to work in parts and I suspect it is due to some variables being optimized out or in fact some code just being ignored. While the variables can be observed to have values in some places they do not.

Yes they are altered in interrupt routines. If I make the variables volatile every use of memcpy() throws a warning. What do I do?

[langwadt]

if they are changed in interrupt they have to be volatile, so don't use memcpy

https://stackoverflow.com/a/36729638

[Simon]

unfortunately as I am extracting data from CAN bus stream and putting in into the variable of the. type that it is I have little choice but to use memcpy().

The whole point of using volatile is not really for many of the reasons it is used but simply to make the damn compiler do as it is told. I want this calculation doing so do it and don't assume that value is 0 because you decided.

[Jeroen3]

No the compiler doesn't try to know better then you. You just didn't tell him everything. 

The compiler can only see what happens in the main thread. So, all functions called from main().
If it then tries to follow the logic and never sees any change to the `int number = 0` you've put there, it will think it's 0. No point in keeping in memory then.
If anything else changes `int number`, such as an interrupt handler, other thread or hardware. You must tell the compiler to assume this value is different every read. And for good measure, ensure it is written every write. In the exact order you wrote. With the volatile keyword.

CAN registers will be marked volatile by the vendor header file. You cannot use memcpy on hardware registers because,
- memcpy cannot guarantee data access width and alignment. (void*!)
- memcpy cannot guarantee data access order. (this can be implementation specific)
- memcpy cannot guarantee atomicity of the read. (data could change during copy)
^{(it may not even be threadsafe?)}
Thus memcpy takes `void*`, which should be fine to copy within standard application memory.

That the compiler throws you a discard volatile warning is your lucky day saving you from way more possible bugs.
memcpy is not the function for your are looking for.

[Simon]

I also get warnings where I use pointers, again unavoidable.

[Jeroen3]

Yes, you will get warnings anywhere you discard volatile keywords, you must use explicit casts if you are sure it's safe to do.

Can you share a minimal sample of your problematic code?

[SiliconWizard]

But it isn't, because memcpy calls may get optimized out in such contexts.
langwadt gave a link detailing why. Write your own copy function(s) in this case.

[Simon]

No the compiler doesn't try to know better then you. You just didn't tell him everything. 

The compiler can only see what happens in the main thread. So, all functions called from main().
If it then tries to follow the logic and never sees any change to the `int number = 0` you've put there, it will think it's 0. No point in keeping in memory then.
If anything else changes `int number`, such as an interrupt handler, other thread or hardware. You must tell the compiler to assume this value is different every read. And for good measure, ensure it is written every write. In the exact order you wrote. With the volatile keyword.

CAN registers will be marked volatile by the vendor header file. You cannot use memcpy on hardware registers because,
- memcpy cannot guarantee data access width and alignment. (void*!)
- memcpy cannot guarantee data access order. (this can be implementation specific)
- memcpy cannot guarantee atomicity of the read. (data could change during copy)
^{(it may not even be threadsafe?)}
Thus memcpy takes `void*`, which should be fine to copy within standard application memory.

That the compiler throws you a discard volatile warning is your lucky day saving you from way more possible bugs.
memcpy is not the function for your are looking for.

This is a micro controller, so single thread. As I said the problem is that I need the compiler to not assume variables have not changed, basically to not try to optimize that, to do that I have to rewrite everything from what you say. Sorry, but the compiler has caused the compilers own problem because even with no optimization turned on it will cause an issue. Now it is easy to simply disable the interrupt during the memcpy access but the compiler will not understand this either will it? I have no way of helping the compiler.

[Simon]

But it isn't, because memcpy calls may get optimized out in such contexts.
langwadt gave a link detailing why. Write your own copy function(s) in this case.

That is what I am thinking, sure I have been here before. Seen a lot of online discussions. I've found this for the sake of being too lazy to do my own from scratch:

void mymemcpy(void *dest, void *src, size_t n)
{
   // Typecast src and dest addresses to (char *)
   uint8_t *csrc = (uint8_t *)src;
   uint8_t *cdest = (uint8_t *)dest;
   
   // Copy contents of src[] to dest[]
   for (uint8_t i=0; i<n; i++)
   cdest = csrc;
}

Ok they used char variables, so I replaced with the more modern uint8_t

[Simon]

volatile also will not work with pointers...... arrrrrg

I think the problem may have also been missing header file inclusions. I'll retry.

[SiliconWizard]

Note that in C you don't need to cast from and to void *, so just:
uint8_t *csrc = src;
uint8_t *cdest = dest;
is fine.

Also, of course, you'll need to declare the parameters volatile:
void mymemcpy(volatile void *dest, volatile void *src, size_t n)
{
volatile uint8_t *csrc = src;
volatile uint8_t *cdest = dest;
...

That could probably be optimized a bit if there is some alignment guarantee (although in your case unaligned memory accesses may work fine and still more efficient than byte-by-byte copy, don't know.)

[Jeroen3]

A microcontroller with interrupts is at least two threads, and when you have multiple interrupt preemption levels, two or more.

Yes, if you want the compiler to assume values may change anytime you must use the volatile keyword.

A bit of code copying data isn't that complicated. Except your function there will still give you a discard volatile warning.
Make the arguments `volatile uint8_t *` and you eliminate all things in my earlier list except atomicity.

https://github.com/gcc-mirror/gcc/blob/master/libgcc/memcpy.c

example, untested

Code: [Select]

#include <stddef.h>

volatile unsigned char *
volatile_memcpy (volatile unsigned char *dest, volatile unsigned char *src, size_t len)
{
  volatile unsigned char *d = dest;
  volatile const unsigned char *s = src;
  while (len--)
    *d++ = *s++;
  return dest;
}
*return should also be volatile*

[Berni]

This is a micro controller, so single thread. As I said the problem is that I need the compiler to not assume variables have not changed, basically to not try to optimize that, to do that I have to rewrite everything from what you say. Sorry, but the compiler has caused the compilers own problem because even with no optimization turned on it will cause an issue. Now it is easy to simply disable the interrupt during the memcpy access but the compiler will not understand this either will it? I have no way of helping the compiler.

Microcontrollers can be just as multithreaded.

You can run a RTOS on a MCU and have many threads. Even without a RTOS you still have the equivalent of threads when interrupt service routines get called. The ISR could happen at any point in your main thread code, so things could change in between any two instructions. Heck these days MCUs even have CPU cache, so you can even run into cache coherency issues like you can in multicore CPUs (Except here it tends to happen between the CPU and DMA controller). All of this is just part of programing modern computers.

You can always just globally disable all optimizations, but this has a significant speed impact so you likely won't want to do that. So you have to explain to the C compiler what it can and can't assume.

The volatile keyword only sticks to the variable type, not the contents, hence why C will warn you when casting volatiles into other normal types, so after the cast the compiler can't know it is volatile. If you have issues with memcpy, you can always write your own mem copy function that takes volatile as parameters. Inside your custom mem copy you can optimize it as much as you like for what your application can handle, it can be a simple byte by byte FOR loop, or up to a fancy super fast assembler hand optimized function with SIMD instructions.

Just make sure you understand where the problem you are having is coming from. Reading weird values from RAM can be caused by compiler attempting to optimize multitreaded code, however it can also be caused by a buffer overrun somewhere tramping over your memory or a cache coherence issue holding onto old data.

[Simon]

Just make sure you understand where the problem you are having is coming from. Reading weird values from RAM can be caused by compiler attempting to optimize multitreaded code, however it can also be caused by a buffer overrun somewhere tramping over your memory or a cache coherence issue holding onto old data.

Yes you are also right there as the issues are beyond this I think and may be related to the code organization. I need to make sure that the headers with the declarations of the variables are included, but then why does the compiler not complain when a variable is not named? somehow it knows it exists, but as "this" C file does not officially know about it it optimizes.

[JPortici]

This is a micro controller, so single thread.

every interrupt service routine is another thread
if you use a RTOS, every thread is another thread

just cast the pointers, memcpy doesn't care if the data is volatile but the compiler has to warn you that you are passing volatile to something that doesn't expect volatile ---> cast away

To those concerned: I don't remember which part simon is using, but most recent can controllers use regular data ram for the FIFOs, you write the data to regular ram as you wish to the address the peripheral tells you, and then set the appropriate bit and the magic happens. To read, the peripheral tells you the FIFO has new data so you set the bit that you want to read the new entry and read the data from regular ram as you wish from the address the peripheral tells you.

[Simon]

Yes you are correct, this micro controller (SAMC21) uses the BOSCH M_CAN controller that uses RAM memory for all of the working memory.

[Jeroen3]

And then your intent is to copy it from this working memory to somewhere else in your application?

[Simon]

And then your intent is to copy it from this working memory to somewhere else in your application?

Yes, so RAM memory is used for the CAN TX and RX buffers, being just 8 dumb bytes so that anything can come and go the best way to extract the data or put the data in is to use memcpy().

[Psi]

You can turn off/on global interrupts in main() if its just for a short time to perform some steps that you don't want an interrupt happening in the middle of and messing things up. 

Any interrupt that happens while global interrupts are off will still cause that interrupt flag bit to get set, but the interrupt handler wont fire until you reenable global interrupts.  So it gets delayed, not lost. (Unless two of them happen, then yes, first one gets lost. But that is pretty unlikely most of the time.)

Just don't use any delay function while you have global interrupts off.  Figure out exactly where the critical instructions are and turn off global interrupts only for them.

[eutectique]

Yes, so RAM memory is used for the CAN TX and RX buffers, being just 8 dumb bytes so that anything can come and go the best way to extract the data or put the data in is to use memcpy().

If your buffers are word-aligned, threat them as two-word arrays. Use union and uint32_t to read/write.

[Simon]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

[DavidAlfa]

Disable interrupts before memcpy, then enable then again.

[tggzzz]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

That seem surprising. I wonder if that will still work if the compiler options change or when the compiler's optimisation algorithms are changed.

There is an awful/aweful amount of confusion about what "volatile" does and does not mean/guarantee.

Have you looked at the generated code to see what the compiler has instructed the processor to do?

[golden_labels]

My take on this is: don’t. No benefit of implementing your own memcpy over using a loop or, where logically identical operation is performed many times, wrapping that entire operation in a function.

memcpy is valuable, because the platform may provide a version better than a plain loop and do it transparently. Usually that’s a better copying algorithm. But also that the compiler knows memcpy semantics and may provide a better code⁽¹⁾. Modern implementations also completely ignore memcpy and replace it with memmove, avoiding potential errors from the programmer not being aware of memcpy limitations.

Your own code will not be able to offer that. In particular memmove can’t be reasonably implemented in pure C. So your gain is nothing more than one line less in code. The cost: having to write another function, less flexibility, sometimes making code bigger and slower.

⁽¹⁾ For example memcpy(*singlebyte, *singleOtherByte, 1) may easily be replaced with a single move instruction. In some cases even a zero-cost registers reassignment.

[eutectique]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

This sounds like a broken compiler. Can you show any code?