What is wrong with this C code

Question

I have a piece of code where I am trying to return the square of the value pointed to by *ptr.

int square(volatile int *ptr)
{
  int a,b;
  a = *ptr;
  b = *ptr;
  return a * b;
}

  main()
  {
    int a=8,t;
    t=square(&a);
    printf("%d",t);
  }

Its working fine for me but author of this code said it might not work because of following reason:
Because it's possible for the value of *ptr to change unexpectedly, it is possible for a and b to be different. Consequently, this code could return a number that is not a square!. The correct way to do is

long square(volatile int *ptr)
{
  int a;
  a = *ptr;
  return a * a;
}

I really wanted to know why he said like that?

If `*ptr` changes between the assignment to a and the assignment to b, the result is not a square. I have no idea what will cause `*ptr` to change. — Sjoerd, Jan 16 '12 at 12:42
Only thing that I can think of that in a multi thread environment the content of *ptr could change in another thread. In this case a might have a different value than b. — Totonga, Jan 16 '12 at 12:44
Your second version has a misleading signature; `a * a` is an `int`, and the implicit conversion to `long` comes too late for it to increase the precision of the return value. To fix that, you should declare `a` as `long`. (Of course, on many systems `long` and `int` are synonymous anyway.) — ruakh, Jan 16 '12 at 14:25

score 10 · Accepted Answer · edited Jan 16 '12 at 13:17

The idea of the volatile keyword is exactly to indicate to the compiler that a variable marked as such can change in unexpected ways during the program execution.

However, that does not make it a source of "random numbers" - it just advises the compiler - what is responsible for actually changing the variable contents should be another process, thread, some hardware interrupt - anything that would write to the process memory but not inlined in the function where the volatile declaration finds itself. In "older times" (compilers with less magic) everything it did was preventing the compiler from caching the variable value in one of the CPU registers. I have no idea on the optimisations/de-optimistions strategies triggered by it by modern compilers - but it will at least do that.

In the absense of any such external factor, a "volatile" variable is just like any other. Actually - it is just like any other variable - as variables not marked as volatile can also be changed by the same external causes (but the compiled C code would not be prepared for that in this case, which might lead to incorrect values being used).

Hold short with the threads. Using `volatile` variable for thread synchronization is a **bug** that will come to get you one day. See [N2016](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2016.html) for discussion why `volatile` didn't acquire semantics for thread-synchronization in C++11. — Jan Hudec, Jan 16 '12 at 18:14
`volatile` can be a part of a thread-safe solution. Even an important part. But it takes real expertise to know exactly how, when and where. The guys who write synchronization primitives have this expertise, and we common programmers should just use their work. — ugoren, Jan 16 '12 at 18:44

Sergey Kalinichenko · Answer 2 · 2012-01-16T13:18:20.520

Since the question has an accepted and correct answer, I will be brief: here is a short program that you can run to see the incorrect behavior happening for yourself.

#include <pthread.h>
#include <math.h>
#include <stdio.h>

int square(volatile int *p) {
    int a = *p;
    int b = *p;
    return a*b;
}

volatile int done;

void* call_square(void* ptr) {
    int *p = (int*)ptr;
    int i = 0;
    while (++i != 2000000000) {
        int res = square(p);
        int root = sqrt(res);
        if (root*root != res) {
            printf("square() returned %d after %d successful calls\n", res, i);
            break;
        }
    }
    done = 1;
}

int main() {
    pthread_t thread;
    int num = 0, i = 0;
    done = 0;
    int ret = pthread_create(&thread, NULL, call_square, (void*)&num);
    while (!done) {
        num = i++;
        i %= 100;
    }
    return 0;
}

The main() function spawns a thread, and modifies the data being squared in a loop concurrently with another loop calling the square with a volatile pointer. Relatively speaking, it does not fail often, but it does so very reliably in less than a second:

square() returned 1353 after 5705 successful calls <<== 1353 = 33*41
square() returned 340 after 314 successful calls   <<== 340 = 17*20
square() returned 1023 after 5566 successful calls <<== 1023 = 31*33

The 'magic' disappears if even the optimization `-O1` is enabled in gcc. The `volatile` keyword should be used to prevent optimation of 'critical' variables. There are two such variables in the above example: `int *p` in `square` to force dereferencing `p` two times and also pointed by `p` `int num` in `main()` must be `volatile`. — Orest Hera, Sep 27 '15 at 22:10

score 5 · Answer 3 · edited May 23 '17 at 12:15

5

First understand what's volatile: Why is volatile needed in C?

and then, try to find answer by yourself.

It's a game of volatile and hardware world. :-)

Read answer given by Chris Jester-Young:

volatile tells the compiler that your variable may be changed by other means, than the code that is accessing it. e.g., it may be a I/O-mapped memory location. If this is not specified in such cases, some variable accesses can be optimised, e.g., its contents can be held in a register, and the memory location not read back in again.

edited May 23 '17 at 12:15

Community

1
1

answered Jan 16 '12 at 12:43

Azodious

13,752
1
36
71

Indeed "reason for downvote?" (/me upvoted) - this is one of the few answers here that actually addresses the point - the use of the "volatile" keyword. – jsbueno Jan 16 '12 at 12:50

score 2 · Answer 4 · answered Jan 16 '12 at 12:42

2

If there is more than one thread, the value the pointer points to might change inbetween statement "a = *ptr" and statement "b = *ptr". Also: you want the square of a value, why put it into two variables?

answered Jan 16 '12 at 12:42

jgroenen

1,332
1
8
13

score 2 · Answer 5 · answered Jan 16 '12 at 12:43

In the code you present then there is no way for the variable a that is defined in your main to be modified whilst square is running.

However, consider a multi-threaded program. Suppose that another thread modified the value to your your pointer refers. And suppose that this modification took place after you had assigned a, but before you had assigned b, in the function sqaure.

int square(volatile int *ptr)
{
  int a,b;
  a = *ptr;
  //the other thread writes to *ptr now
  b = *ptr;
  return a * b;
}

In this scenario, a and b would have different values.

score 1 · Answer 6 · answered Jan 16 '12 at 12:44

1

The author is correct (if *ptr will be changed by other threads)

int square(volatile int *ptr)
{
  int a,b;
  a = *ptr; 
  //between this two assignments *ptr can change. So it is dangerous to do so. His way is safer
  b = *ptr;
  return a * b;
}

answered Jan 16 '12 at 12:44

aayoubi

11,285
3
22
20

score 0 · Answer 7 · edited Sep 27 '15 at 21:36

I see some answers with *ptr can be changed by other threads. But this cannot happen since *ptr is not a static data variable. Its a parameter variable and local and parameter variables being hold inside stack. Each thread has its own stack section and if *ptr has been changed by another thread, it should not effect the current thread's.

One reason why the result might not give the square can be an HW interrupt might happen before assigning b = *ptr; operation as indicated below:

int square(volatile int *ptr) {
    int a,b;
    a = *ptr; //assuming a is being kept inside CPU registers.

    //an HW interrupt might occur here and change the value inside the register which keeps the value of integer "a"

    b = *ptr;
    return a * b;
}

Wrong. The variable `ptr` is local. However it points to some address. That address may point to some variable on stack of other thread or even to a static variable. The value `ptr` cannot be changed, however variable pointed by `ptr` is not local and it can be changed. So, `*ptr` can be chaged. — Orest Hera, Sep 27 '15 at 21:27

score 0 · Answer 8 · answered Jan 16 '12 at 12:43

0

Because the value of the pointer *ptr might change between the first affection and the second one.

answered Jan 16 '12 at 12:43

mlemay

1,622
2
32
53

score 0 · Answer 9 · answered Jan 16 '12 at 20:02

I don't think the value of *ptr can change in this code barring an extremely unusual (and non-standards-compliant) runtime environment.

We're looking at the entirety of main() here and it's not starting up other threads. The variable a, whose address we are taking, is a local in main(), and main() doesn't inform any other function of that variable's address.

If you added the line mysterious_external_function(&a); before the t=square(&a) line, then yes, mysterious_external_function could start a thread and diddle the a variable asynchronously. But there's no such line, so as written square() always returns a square.

(Was the OP a troll post, by the way?)

What is wrong with this C code

9 Answers9