Link to home
Start Free TrialLog in
Avatar of phoffric
phoffric

asked on

Wrong answer when running a member function in another thread

Constraint: Cannot change from C++03 or C-88.

After some online search, I wrote the following program to see how to run a class non-static member function in a pthread via a static member helper. But I got the wrong answer. According to breakpoints, the DoSomething constructor is called once; yet on the 2nd call to doSomething, the initial values of val1 and val2 are set to 100, 200.0, respectively. Granted, I set the members of an internal struct to those values, but not to the class data members.

So, how did that happen; how do I get the args.retval return value from the pthread to end in 5 + 16 = 21 + a multiple of 100.
// header
class DoSomething
{
public:
	DoSomething(int v1, float v2)
		: val1(v1), val2(v2) {
	retval = val2 - val1;
	}

	float* doSomething(int v1, float v2) 
	{
		val1 += v1;
		val2 += v2;
		retval = val1 + val2;
		return &retval;
	}

	struct v1v2_s
	{
		int v1;
		float v2;
		float retval;
	};

	static
    void* DS_helper(void* args)
	{
		int v1 = ((v1v2_s*)args)->v1;
		float v2 = ((v1v2_s*)args)->v2;
		((v1v2_s*)args)->retval = *((DoSomething*)args)->doSomething(v1, v2);
		return args;
	}

private:
	int val1;
	float val2;
	float retval;
};

Open in new window

#include "DoSomething.h"

int main()
{
	DoSomething DS(2, 9);

	DS.doSomething(3, 7);

	DoSomething::v1v2_s args = {100, 200.0};

	pthread_t t;
	pthread_create(&t, NULL, DS.DS_helper, &args);
	pthread_join(t, (void**)&args);
	float res = args.retval;
	return 0;
}

Open in new window


If you could correct the existing code w/o trying to revamp the whole approach, I would appreciate that. (After this is done, then, sure, revamp the whole approach.)
Thanks in advance.
ASKER CERTIFIED SOLUTION
Avatar of Zoppo
Zoppo
Flag of Germany image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
you're passing a pointer to args as parameter to the thread function, but then, in the thread function, you cast the same pointer to a DoSomething.

as the class DoSomething has the same members as struct DoSomething::v1v2_s it is legal to cast a pointer of the one to a pointer of the other.

because of that,

*((DoSomething*)args)->doSomething(v1, v2);

is a legal expression where (DoSomething*)args == { 100, 200.0 } and the doSomething function has arguments (100, 200.0) as well.

hence the function would perform the following statements

        val1 += v1;    // 100 += 100 ==> args->val1 = 200
        val2 += v2;     // 200. += 200. ==> args->val2 = 400.
	retval = val1 + val2;  // args->retval = 200 + 400. = 600.

Open in new window


so in your main function you should see with the debugger

args = { 200, 400.0, 600.0 }

however, since the values are changed asynchronously in a thread, there might happen two things which are not intended:

1. the debugger steps to next statement

       float res = args.retval;
 
    in main but the thread statements are not yet performed.

2. the compiler didn't add statements to retrieve the new values of args since you didn't define the members with the volatile keyword.

in both cases the args would show as { 100, 200., 0. } in the debugger.

how do I get the args.retval return value from the pthread to end in 5 + 16 = 21 + a multiple of 100.

actually i don't know what the purpose of the v1v2_s is. if you would pass &DS to the thread as an argument, then the thread would get

   { 5, 16., 21. } and could add 100, 200. two times:

volatile DoSomething DS(2, 9);
DS.doSomething(3, 7);
pthread_t t;
pthread_create(&t, NULL, DS.DS_helper, &DS);
...

static
void* DS_helper(void* data)
{
	DoSomething * args = (DoSomething *)data;
        args->doSomething(100, 200.);
        args->doSomething(100, 200.);
	return args;
}

Open in new window


alternatively you may create another structure which contains two pointers to a class and/or a struct and pass this as arguments to the pthread.

Sara
It is strongly recommended to use a pointer of the class 'DoSomething' rather than an irrelavent stack variable pointer as the parameter of pthread.

Try it and you will get why your code is not going to work.
Avatar of phoffric
phoffric

ASKER

@Zoppo, Thanks. The code and explanation is clear. One thing that is interesting...
	pthread_join(t, (void**)&args);
	float res = args.retval;

Open in new window

At line 2, args.v1 has a value that is the same address as (&args). Inside of DS_helper, args at return args is correct. It could be that my implementation of pthreads is clobbering it, since it is a special version. Or, it could be that there is something fundamentally wrong with the code. (Maybe this weekend, I'll have time to install Ubuntu and try the code with bonafide POSIX pthreads.

@sara, Thanks. I see why the initial values of val1 and val2 are set to 100, 200.0, respectively.
So, from the first two posts, both my questions are answered.


@博旭 张, Could you please clarify the new point and new benefits that you are making.
>> you will get why your code is not going to work.
I got why the code doesn't work from the first two posts. It seems like you are proposing changes other than what Zoppo suggested. If so, please identify all the code changes you are suggesting in a manner similar to what Zoppo did, and I will evaluate the benefits. Thanks.
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Thanks a lot. Remind me not to write code after midnight and don't give water or food to the Mogwai after midnight.
Some further comments...

Zoppo's suggestion, https:#a42368623, seemed to help the args->v1 being clobbered issue, where
At line 2, args.v1 has a value that is the same address as (&args).

>> yes, it is a fact that the address of a structure object always is the same as the address of the first member.
That appears to be true; however, args.v1 is an int, and I was referring to the value in v1 which got clobbered.

>> the compiler didn't add statements to retrieve the new values of args since you didn't define the members with the volatile keyword.
I think you are suggesting volatile to be a form of synchronization. I am still not using volatile; and applying Zoppo's last suggestion, the program behaves better. Not to say that volatile isn't needed; however the community at large doesn't think so in the case of multithreading usage. Here are some articles related to this topic:
https://software.intel.com/en-us/blogs/2007/11/30/volatile-almost-useless-for-multi-threaded-programming
http://www.drdobbs.com/parallel/volatile-vs-volatile/212701484
I think you are suggesting volatile to be a form of synchronization
no. volatile prevents the compiler from optimizing code such that read access to a variable was not made again if the current code doesn't make any changes. but in case of a shared variable in a multi-threading environment, another thread might have changed the variable.

//thread 1:

bool terminated = false;
pthread_create(..., thread_func);
bool * pend = &terminated;
pthread_join(.., &pend);
while (terminated == false)
{
    ...
    sleep(1);
}


// thread 2:

void * thread_func(void ** args)
{
     bool * pend = (bool *)args;
     doSomeLengthyThing();
     *pend = true;
     return NULL;
} 

Open in new window


the while loop in thread 1 will last infinite since the compiler wouldn't reread the contents of the bool variable as it was not declared as volatile. some compilers might give a warning and the debugger might change the wrong behavior, but in my opinion the volatile is required if you define shared variables.

note, the first article regarding volatile you posted a link to, you might read the comments at the end ...


however, args.v1 is an int, and I was referring to the value in v1 which got clobbered.
if 'clobbered' means that the pointer looked to be corrupted than the corruption most likeley happened because of &args was passed to pthread_join and so the args was corrupted when pthread_join returns a different address.


Sara
I think there is a chance that your first use of volatile to solve the OP multi-threading issue may be incorrect. If I/we find this to be true, then I would like to remove all subsequent volatile discussion from this question. You previous post seems to be similar to this, but may not have enough commonality with the OP.

Question for you. Are you saying that generally, when multiple threads access shared data with combinations of reads and writes, then we ought to make those shared data as volatile? Like as in the following:
volatile bool flag = false;

void protectCS() {
  while (!flag) {
    Sleep(1000); // sleeps for 1000 milliseconds
  }
}

void WakeMe() { // one thread that allows decrement() to proceed
  flag = true;
}

void decrement(int val){ // another thread
   protectCS();
   accumulation -= val; // critical section
}

Open in new window

yes, it is not only an opinion but experience. we already discussed the design to have a main (gui) thread running in an infinite message loop. if you want to terminate a running worker thread for example because of a user request, the most simple thing is to set a commonly shared bool variable to true. however, the worker thread would not be aware of any change to this variable while running in a loop beside it was defined as volatile.

you should be able to verify this but you shouldn't mix it up with mutex locking. if using a mutex you would use atomic (thread-safe) services from os for synchronisation. setting a shared flag is not thread-safe in that way that if both threads would write to that flag that the result is predictable. it could be that thread 2 reads the flag as false although thread 1 has already assigned true. if using a flag the point is that only one thread may write and all others will only read. then, it doesn't matter whether the change of the flag was recognized within the current loop cycle or in the next one. the volatile only is a means to tell the compiler that it has to read every time again and do not an optimization where the flag wasn't read at all beside of the first time.

I think there is a chance that your first use of volatile to solve the OP multi-threading issue may be incorrect.
what do you mean by this? my first code where i used the volatile keyword was to define a variable of DoSomething which was passed as argument to pthread_join. if you omit the volatile keyword, any change to the variable done in the thread might not be recognizable in subsequent statements of the main thread. this could be different between compilers. it also could be different if running in the debugger because the code would run without standard optimizations.

Sara
There is a lot of negative literature about using volatile for multithreading programming. For primitives such as bool, it may be useful, but also may need a mutex. In one project a few years ago, we use very heavy multithreading with shared variables and we did not use volatile . Instead we use the text and condition variables . My understanding is that the new text , condition variables and also some atomic operations was what was needed to ensure cash consistency. Mr. anonymous wrote the link that needs volatile in high-performance computing. On the other hand volatile slows down processing.
There is a lot of negative literature about using volatile
actually i don't have noidea why. volatile is only a compiler directive. and the only purpose is to flag shared variables such they could be changed in aqnother thread. if someone says shared (global) variables are bad and that is why they don't use it and because of that they don't need volatile, it is ok. but you could define shared variables as static class members and there is nothing bad to do so.

On the other hand volatile slows down processing
i bet that you can't measure it. that is one of the myth people invented to strongen their weak argumentation. you need the volatile to check a shared variable periodically in a polling loop. inside of that loop there is at least one wait or sleep or deferred operation if you don't want the cpu (core) to be busy 100 percent. while this costs microseconds or even milliseconds, it costs nanoseconds to read from a shared variable once in that same loop.

sorry to have extended the volatile discussion in that thread.

My understanding is that the new text , condition variables and also some atomic operations was what was needed to ensure cash consistency.

absolutely.

Sara
>> sorry to have extended the volatile discussion in that thread.

I am not concerned about this if, and a big if, the volatile was necessary in my OP.

Usage of volatile in high performance computing seems to defeat the goals of HPC. That is why we didn't use volatile, in general. Maybe it was used in some places that I didn't look at. So this discussion has been useful in that I see that it is sometimes necessary in multi-threading environments where shared variables are involved. The question is whether it is always necessary; and from my experience and readings, I think that it may not always be necessary and therefore volatile would be a detriment to high performance computing.

If it isn't necessary in the be OP , then I wouldn't want paq readers to get the impression that it is necessary. I found a link that I will share as soon as I get home that may shed more light on this.

By the way, the commenters on the Intel link that I posted who disagreed with the Intel expert, who happened to be a TBB writer for Intel, all happened to have the same name, namely anonymous. Anonymous writers are not necessarily experts and maybe they're just giving their opinions and we don't really know the justifications for those opinions.
Here is the link that I said I would put up for you to review.
https://wiki.sei.cmu.edu/confluence/display/cplusplus/CON01-CPP.+Do+not+use+volatile+as+a+synchronization+primitive

I have skimmed it, and will have to read it more carefully next year when multi-threading becomes a higher priority.

The point I am trying to make is that the use of volatile is debatable with you taking the pro side and others online taking the con side. In your first use of volatile, I am not convinced that it should be used; and it could even be a detriment to multi-threading.

In that case, we should agree to remove volatile from this discussion, because if we do not, then that says we are endorsing the usage of it. I accepted your "volatile" post by mistake, and do not want to endorse it as of now unless I can be convinced that it is absolutely necessary for correctness.

We may have to create another blog or question or article in order to bring in other experts into this discussion.
Thanks again for your help. The discussion on volatile is debatable in my opinion. It may be useful in some situations, but generally, given that volatile keyword slows down multi-core programming, I do not want to endorse the use of volatile for multi-threading, multi-core programs. I have been on HPC programs and as a guideline, we were not to use volatile to handle the shared memory threading issues.

From my readings, there is a place for volatile in such environments, but apparently, to make it a general rule for shared memory may be misleading; and as experts, I don't want to endorse it unless I am totally convinced otherwise.