MantisBT - CMake
On 06/15/2010 05:07 PM, Kevin Fitch wrote:
> I have been converting an existing make based build system over to cmake,
> and overall I am loving it so far.
>
> I happened to run across CheckForPthreads.c, and was a little surprised to
> see that it was basically a classic example of racy code.
>
> The two spawned threads both increment res, which is then used as the return
> value of the whole program. So to prove to myself that this was a real
> issue, not just theoretical raciness, I compile the program, and ran it a
> couple million times on my linux box, and 7 of those two million runs
> returned 1 instead of 2. So the chance of screwup is quite small, but
> definitely non-zero.
Hi Kevin,
after having taken a look at CheckForPthreads.c, I would say you are
absolutely right: If C's postfix "++" operator isn't atomic - and AFAIK,
it isn't guaranteed to be - there's a certain probability for one thread
to intercept the other right at "res++" which may result in each thread
storing "1" to "res", so joining both threads isn't sufficient to ensure
a return value of "2". Actually, I can confirm that the program returns
a value of "1" sometimes. In connection with FindThreads.cmake relying
on "2" to indicate a working "pthread" option of the compiler, this is
definitive a bug, IMO - feel encouraged to file an appropriate report.
> Looking over the source I have a few questions:
> Should it protect the increment with a mutex?
The straightest measure to ensure proper functioning.
> Or, should it really just spawn 1 thread? Is there a point to the spawning
> two?
As far as I can see, one thread would be enough to prove the "pthread"
option, but this would also necessitate a change in FindThreads.cmake.
> Is there a purpose to all the prints?
Probably not since FindThreads.cmake doesn't catch them. Maybe, they
serve as a visual verification when the program is launched manually.
> What is the line "if(ac > 1000){return *av[0];}" there for? If it has a
> purpose that definitely deserves a comment.
A subtle joke we don't understand? ;)
> What about the "#if defined(__BEOS__)"... lines? The usleep will only be
> there on BEOS, but the comment on the usleep line mentions sun.
Perhaps, this means BEOS-on-SUN, but I don't know for sure.
Regards,
Michael
Uh, BeOS has never run on Sun hardware (more properly known as SPARC) AFAIK. So I am going to say, either we should drop the usleep altogether, or fix the conditional compile lines to compile in on whatever 'sun' the comment references.
I have attached a patch for what I think CheckForPthreads.c should probably look like.
I have run my modified version for over 1000000 iterations and it has not failed, the original had a failure rate of about 1/285000 on the same box.
checkforpthreads.patch (1,007) 2010-06-17 13:30https://public.kitware.com/Bug/file/3200/checkforpthreads.patch