Call a function multiple times with the same input but get different return value

Question

I'm using GNU C in Ubuntu 14.0.4. I wrote a CRC_XOR() function and call it with the same input argument multiple times. But It's wierd, each call may sometimes get different result. What's going wrong? Sample code is here:

#include <stdio.h>
#include <stdlib.h>

char CRC_XOR(char *as_Pkt,int ai_len);
void DO_Get_CRC(void);
int main(void)
{
    DO_Get_CRC(); //get 01 00 02 03
    DO_Get_CRC(); //get 01 00 02 03
    DO_Get_CRC(); //get 01 00 02 00  (strange?)  
    DO_Get_CRC(); //get 01 00 02 03
    DO_Get_CRC(); //get 01 00 02 00  (strange?) 
    DO_Get_CRC(); //get 01 00 02 03
    DO_Get_CRC(); //get 01 00 02 00  (strange?) 
    exit(0);
}
/*
    use same input to invoke CRC_XOR()
*/
void DO_Get_CRC(void)
{
    char  ls_pkt[20];
    int   li_idx;
    short li_max = 512;
    int   li_len = 0;

    ls_pkt[li_len++] = 0x01;  //station ID
    ls_pkt[li_len++] = 0x00;  //length-low byte
    ls_pkt[li_len++] = 0x02;  //length-high byte
    ls_pkt[li_len++] = CRC_XOR(ls_pkt,li_len);
    ls_pkt[li_len]   = 0;
    for (li_idx=0; li_idx<li_len;li_idx++) {
         printf("%02X ", ls_pkt[li_idx]); //display in hexdigits
    }

    printf("\n");
}
/*
    return 1 byte of CRC by XOR byte array
*/
char CRC_XOR(char *as_Pkt,int ai_len)
{
    int  li_idx;
    char lc_CRC = 0x00;

    for (li_idx=0; li_idx < ai_len; li_idx++){
        lc_CRC ^= as_Pkt[li_idx]; //XOR each byte
    }
    return (char)(lc_CRC & 0x000000FF); //return CRC byte
}

The program you should should not build. What is, for example, `li_len`? Where is it defined? What is it initialized to? — Some programmer dude, Aug 10 '17 at 05:57
As for your problem, please take some time to read [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) by Eric Lippert. And learn how to use a debugger, which is the right tool to solve problems like this. — Some programmer dude, Aug 10 '17 at 05:59
Have you removed anything else that could potentially be of substance? I can't reproduce your behavior. — colavitam, Aug 10 '17 at 06:02
On a totally unrelated note: The `main` function should be declared to return an `int`. It is required by the C specification. On a more related note: Do you get any warnings when building? What if you enable more warnings (something you really should do anyway, I recommend at least adding `-Wall` when compiling)? If you get warnings, what do they say? Could they be related to your problem? — Some programmer dude, Aug 10 '17 at 06:05
Sorry, this is my first time to ask question here. I've copied the source program. As you can see, the bug may occur in this line: ls_pkt[li_len++] = CRC_XOR(ls_pkt,li_len); if I changed li_len++ to li_len. The bug disappeared. However thank you so much for your kindness — Victor Huang, Aug 10 '17 at 06:22
@VictorHuang wait a second, let's make this even better. With `li_len++` in place can you compile it into assembly, with `gcc -S` and then copy the contents of the produced `.s` file, it would be nice to see the compiler output! — Antti Haapala -- Слава Україні, Aug 10 '17 at 07:05
@VictorHuang btw your code has a portability bug. `"%02x"` should be `%02hhx` because you're printing characters, not ints... — Antti Haapala -- Слава Україні, Aug 10 '17 at 07:19
Why are you programming this in such a complicated way? Just *initialize* your `ls_pkt` array properly and then call `CRC_XOR` for element `3`. — Jens Gustedt, Aug 10 '17 at 08:05

score 8 · Answer 1 · answered Aug 10 '17 at 06:30

8

You have undefined behavior.

The reason is because there is no sequence point with assignment.

For

ls_pkt[li_len++] = CRC_XOR(ls_pkt,li_len);

you don't know if li_len++ happens before or after the evaluation of li_len in the function call. That in turns might mean that li_len might be equal to 4 which means your CRC_XOR function will use the as of yet uninitialized ls_pkt[4].

Since ls_pkt[4] is uninitialized before the assignment, its value will be indeterminate, and might be seemingly random.

The simple solution is to increase li_len after the assignment:

ls_pkt[li_len] = CRC_XOR(ls_pkt,li_len);
++li_len;

answered Aug 10 '17 at 06:30

Some programmer dude

400,186
35
402
621

Generally, programming with side effects is a bad idea. It buys you nothing but trouble. – Jens Gustedt Aug 10 '17 at 08:00
I think this comes under the clause that all reads between the sequence points should directly be required for the write. – Ajay Brahmakshatriya Aug 10 '17 at 09:43

score 1 · Answer 2 · answered Aug 10 '17 at 07:56

I couldn't originally reproduce in 64-bit mode in my x86, but it seems to be easy to reproduce on my computer in 32-bit mode; perhaps the different stack width is the reason.

% gcc crc.c -m32
% ./a.out  
01 00 02 03 
01 00 02 00 
01 00 02 03 
01 00 02 00 
01 00 02 03 
01 00 02 00 
01 00 02 03

I've numbered the lines here so one can easily see how they correspond to the .loc lines from the assembler output:

20  void DO_Get_CRC(void)
21  {
22      char  ls_pkt[20];
23      int   li_idx;
24      short li_max = 512;
25      int   li_len = 0;
26
27      ls_pkt[li_len++] = 0x01;  //station ID
28      ls_pkt[li_len++] = 0x00;  //length-low byte
29      ls_pkt[li_len++] = 0x02;  //length-high byte
30      ls_pkt[li_len++] = CRC_XOR(ls_pkt,li_len);
31      ls_pkt[li_len]   = 0;
32      for (li_idx=0; li_idx<li_len;li_idx++) {
33           printf("%02X ", ls_pkt[li_idx]); //display in hexdigits
34      }
35
36      printf("\n");
37  }

Compiling this into assembly, we get the following assembler output:

DO_Get_CRC:
.LFB3:
        .loc 1 21 0
        .cfi_startproc
        pushl   %ebp
        .cfi_def_cfa_offset 8
        .cfi_offset 5, -8
        movl    %esp, %ebp
        .cfi_def_cfa_register 5
        pushl   %esi
        pushl   %ebx
        subl    $48, %esp
        .cfi_offset 6, -12
        .cfi_offset 3, -16
        call    __x86.get_pc_thunk.bx
        addl    $_GLOBAL_OFFSET_TABLE_, %ebx
        .loc 1 21 0
        movl    %gs:20, %eax
        movl    %eax, -12(%ebp)
        xorl    %eax, %eax
        .loc 1 24 0
        movw    $512, -42(%ebp)
        .loc 1 25 0
        movl    $0, -36(%ebp)      // int li_len = 0
        .loc 1 27 0
        movl    -36(%ebp), %eax    // eax = li_len
        leal    1(%eax), %edx      // edx = eax + 1
        movl    %edx, -36(%ebp)    // li_len = edx
        movb    $1, -32(%ebp,%eax) // ls_pkt[eax] = 1
        .loc 1 28 0
        movl    -36(%ebp), %eax    // eax = li_len
        leal    1(%eax), %edx      // edx = eax + 1
        movl    %edx, -36(%ebp)    // li_len = edx
        movb    $0, -32(%ebp,%eax) // ls_pkt[eax] = 0
        .loc 1 29 0
        movl    -36(%ebp), %eax    // eax = li_len
        leal    1(%eax), %edx      // edx = eax + 1
        movl    %edx, -36(%ebp)    // li_len = edx
        movb    $2, -32(%ebp,%eax) // ls_pkt[eax] = 2
        .loc 1 30 0
        movl    -36(%ebp), %esi    // esi = li_len
        leal    1(%esi), %eax      // eax = esi + 1
        movl    %eax, -36(%ebp)    // li_len = eax --li_len is now **4**
        subl    $8, %esp
        pushl   -36(%ebp)          // push the value of li_len (2nd arg)
        leal    -32(%ebp), %eax    // load address of ls_pkt
        pushl   %eax               // and push ls_pkt (1śt arg)
        call    CRC_XOR            // call CRC_XOR

The post-increments are encoded as the "load effective address" instruction, it is a common trick in X86 assembler to use the address calculation hardware for constant arithmetic as the instructions are smaller. Anyway, there are a total of 4 leal 1(%XXX), %YYY followed by movl %YYY, -36(%ebp), where -36(%ebp) is the location of the variable li_len, meaning that li_len was incremented 4 times before its value is pushed onto the stack as the argument for CRC_XOR. However if the code is mutated elsewhere it easily happens so that the same compiler produces code that increments li_len only 3 times before calling the function, proving that they're indeed indeterminately sequenced.

ntshetty · Answer 3 · 2017-08-10T07:17:04.057

0

Remove ; in this line

void DO_Get_CRC(void); --> void DO_Get_CRC(void)

Modify this line, because you wanted to pass li_len but you passing li_len+1 because of ++ while assigning

ls_pkt[li_len++] = CRC_XOR(ls_pkt,li_len); -->  ls_pkt[li_len++] = CRC_XOR(ls_pkt,li_len-1);

Its works fine without any issue main.c:31:27: note: each undeclared identifier is reported only once for each function it appears in
sh-4.2$ gcc -o main *.c
sh-4.2$ main
01 00 02 03
01 00 02 03
01 00 02 03
01 00 02 03
01 00 02 03
01 00 02 03
01 00 02 03

edited Aug 10 '17 at 07:17

answered Aug 10 '17 at 06:05

ntshetty

1,293
9
20

1

That misplaced semicolon should lead to build-errors, not unexpected results. – Some programmer dude Aug 10 '17 at 06:06
@Someprogrammerdude thanks for pointing, because i missed to paste important code line change – ntshetty Aug 10 '17 at 07:18
no, that fix is *absolutely incorrect*. Please read and understand @Someprogrammerdude's answer. – Antti Haapala -- Слава Україні Aug 10 '17 at 07:20
@AnttiHaapala don't know for what you referring. But as per logic, CRC_XOR function should receive 3 but it receives 4 because of increment. – ntshetty Aug 10 '17 at 07:30
@ThiruShetty your "fix" repeats the same problem. – Antti Haapala -- Слава Україні Aug 10 '17 at 07:31
@AnttiHaapala i have put in loop and run for 1000 times but the result is same under ubuntu 16.04, I don't know about other platforms – ntshetty Aug 10 '17 at 07:36
@ThiruShetty exactly, that is the problem. You "fixed" the problem., The corpse is under the carpet. – Antti Haapala -- Слава Україні Aug 10 '17 at 07:38
The thing is, it is never specified **when** `++` increments the pointer. It just needs to do so before *calling the function*, not before preparing the arguments for the function call. [More explanation here](https://stackoverflow.com/questions/4865599/post-increment-with-respect-to-sequence-points). Thus you cannot fix this by assuming that it was incremented *before* you calculate `i + 1`. There is no such guarantee. – Antti Haapala -- Слава Україні Aug 10 '17 at 07:41
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/151581/discussion-between-thiru-shetty-and-antti-haapala). – ntshetty Aug 10 '17 at 09:13

Call a function multiple times with the same input but get different return value

3 Answers3