Strange problem caused by optimisation?

Go To Last Post
10 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi all. I've a question.
I've this code:

char _tx[57];
//some code that initialize _tx
//method 1
strlcat(_tx, (char *)global_checksum(_tx), sizeof(_tx));

//method 2
char *chk = global_checksum(_tx);
char _chk[3] = "\0\0\0";
_chk[0] = *chk++;
_chk[1] = *chk++;
_chk[2] = *chk;
strlcat(_tx, _chk, sizeof(_tx));


//The function
char *global_checksum(char *msg)
{
unsigned char t = 0;
volatile char ret[3] = "\0\0\0";
int i = 0;
do
{
	t -= msg[i];
}while(msg[i++] != 0);
ret[1] = t;
ret[1] &= 0x0F;
ret[0] = t;
ret[0] &= 0xF0;
ret[0] = (ret[0] >> 4);
for(int i=0; i<2; i++)
{
	switch (ret[i])
	{
	case 0x0A:
	case 0x0B:
	case 0x0C:
	case 0x0D:
	case 0x0E:
	case 0x0F:
		ret[i] += 0x40 - 0x09;
		break;
	default:
		ret[i] += 0x30;
		break;
	}
}
char *__ret = (char *)ret;
return __ret;
}

Ok, if I use the method 1 with optimization -O0 all works fine, but if I use any other optimizations the strlcat don't work and I can't find the right string at the end of my variable _tx.

If I use the method 2 all works fine with any optimizations.
The question is.. Why???
Thanks to everyone.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The compiler probably recognizes that _tx is not subsequently used so realizes it would be a waste of time to generate code to access it.

Try adding some code beyond this that outputs the contents of _tx to a volatile destination or make _tx itself 'volatile'

Cliff

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You have two auto variables ret[] and __ret that will both vanish into the stratosphere at the end of your global_checksum() function. Make ret[] static if you want to return it.

Your whole system seems a little iffy anyway with or without any optimisation. Conventionally one would keep a long integer checksum rather than a string. You can always output a string from the integer.

char _tx[57];
//some code that initialize _tx
//method 1
strlcat(_tx, (char *) global_checksum(_tx), sizeof(_tx));

//method 2
char *chk = global_checksum(_tx);
char _chk[3] = "\0\0\0";
_chk[0] = *chk++;
_chk[1] = *chk++;
_chk[2] = *chk;
strlcat(_tx, _chk, sizeof(_tx));


//The function
char *global_checksum(char *msg)
{
    unsigned char t = 0;
    volatile char ret[3] = "\0\0\0";
    int i = 0;
    do {
        t -= msg[i];
    } while (msg[i++] != 0);
    ret[1] = t;
    ret[1] &= 0x0F;
    ret[0] = t;
    ret[0] &= 0xF0;
    ret[0] = (ret[0] >> 4);
    for (int i = 0; i < 2; i++) {
        switch (ret[i]) {
        case 0x0A:
        case 0x0B:
        case 0x0C:
        case 0x0D:
        case 0x0E:
        case 0x0F:
            ret[i] += 0x40 - 0x09;
            break;
        default:
            ret[i] += 0x30;
            break;
        }
    }
    char *__ret = (char *) ret;
    return __ret;
}

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
The compiler probably recognizes that _tx is not subsequently used so realizes it would be a waste of time to generate code to access it.

Try adding some code beyond this that outputs the contents of _tx to a volatile destination or make _tx itself 'volatile'

Cliff

This will help in optimization like O1,2,3. But in Os it does not work.
I can not understand why..

In your opinion, what is the optimization that creates fewer problems of this type?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
You have two auto variables ret[] and __ret that will both vanish into the stratosphere at the end of your global_checksum() function. Make ret[] static if you want to return it.

Thanks David, makes the ret[] static resolve my problem with all the optimizations level.
But do you know why it will happen?

david.prentice wrote:

Your whole system seems a little iffy anyway with or without any optimisation. Conventionally one would keep a long integer checksum rather than a string. You can always output a string from the integer.

I've to transform "int" into his "bcd" translation because I don't use printf.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Maybe I've understand why I've to declare the variable static.
If I use

char *__ret = (char *) ret;
return __ret;

The compiler thinks that is the value of __ret that I want, not the content of ret.
Now, if an interrupt occurs, and I've used the Os optimization, the compiler assign the location of ret to another variable and this will corrupt my ret string.
Makes ret static and return pointer to this resolv the problem!
My reasoning is right?
However thanks to everyone for the help.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You haven't shown enough context to be able to help much - if you present a complete test program that shows the issue you are having it may be possible to provide advice. Personally I'd always use -Os. The usual "problem" with optimisation is when folks don't write "real" programs with real inputs and real outputs but just a "test" of some feature which involves the whole or part of the thing being discarded as it seems to serve no purpose. This is why we need to see you routines in context to know if they are being used for something real or not.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
But do you know why it will happen?

Yes. The clue is in the name of the storage class. "auto" or "static". "volatile" has nothing to do with it.

You can still do the BCD transformation on an integer. You may choose to do it on individual 4 bit nibbles of the integer.

A C function can return a value that you can use in an assignment. e.g. long checksum = calc_check(char *string);
It can also return a pointer as you do. The pointer has a value. It points to an "auto" area of memory. The contents were valid during the function. What happens outside of the function is anyone's guess. So the contents may well be turned into garbage.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
You haven't shown enough context to be able to help much - if you present a complete test program that shows the issue you are having it may be possible to provide advice. Personally I'd always use -Os. The usual "problem" with optimisation is when folks don't write "real" programs with real inputs and real outputs but just a "test" of some feature which involves the whole or part of the thing being discarded as it seems to serve no purpose. This is why we need to see you routines in context to know if they are being used for something real or not.

you are right, sorry, next time I will post more code!

david.prentice wrote:
Quote:
But do you know why it will happen?

Yes. The clue is in the name of the storage class. "auto" or "static". "volatile" has nothing to do with it.

You can still do the BCD transformation on an integer. You may choose to do it on individual 4 bit nibbles of the integer.

A C function can return a value that you can use in an assignment. e.g. long checksum = calc_check(char *string);
It can also return a pointer as you do. The pointer has a value. It points to an "auto" area of memory. The contents were valid during the function. What happens outside of the function is anyone's guess. So the contents may well be turned into garbage.

David.

Ok, I've understand my error. Now I will rewrite my function in a better way following your advices.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You're returning a pointer to storage that is allocated on the stack. This storage gets overwritten by the next function that is called as the program runs. You might want to read up on the difference between "auto" and "static" storage.