std::thread leads to Abort

[User Name:]

I'm trying to code a program for dealing with prime numbers. The idea is to have the prime list readable by all threads, but writeable by 1.

#include "gmpxx.h"

#include <iostream>
#include <thread>
#include <vector>

// primes must be initialised with at least the first two prime numbers.
static std::vector<mpz_class> primes = {2, 3};

// integer sqrt
static mpz_class sqrt(mpz_class n)
{
    mpf_class x, y = n;
    
    do{
        x = y;
        y = (x + n / x) / 2;
    }while(abs(y - x) >= 1);
    
    return y;
}

// extends primes to the lowest prime greater than or equal to n
static void extend_primes(mpz_class n)
{
    // Unfortunately, the primes can't efficiently, if at all, be extended concurrently...
    static std::mutex m;
    std::lock_guard<std::mutex> lock(m);
    
    for(mpz_class i = primes.back() + 2;i <= n;i += 2)
    {
        bool prime = true; // Assume i is prime.
        
        for(auto j = primes.begin();prime && *j <= sqrt(i);j++)
            if(i % *j == 0)
                prime = false;
        
        if(prime)
            primes.push_back(i);
    }
}

int main()
{
    extend_primes(7);
    std::thread t(extend_primes, 19); // This (or a resulting call) must be the cause. When commented out, everything works fine.
    
    for(auto i = primes.begin();i < primes.end();i++)
        std::cout << i->get_str() << " ";
    
    std::cout << std::endl;
    return 0;
}

The error:

$ gcc -ggdb -std=c++0x -lstdc++ -lgmp -o prime prime.cpp
$ ./prime 
terminate called after throwing an instance of 'std::system_error'
  what():  
Aborted

[iMalc]

You start a thread to calculate the primes up to 19, but then immediately go and try outputting the list, whilst the other thread is generating them, and without any locking. Yes I can see why it causes problems, epecially when the vector resizes and all the iterators are invalidated. I assume you understand that the point of a thread is that it doesn't block execution of the thread that created it.
The thing that reads from the list needs to lock also.

Other problems...
You hold the lock whilst generating the entire list of primes. Surely you want a finer grained lock, i.e. only locking as you actually do a push_back? However, once you do that, you wont be able to iterate through the vector using iterators at the same time. I suggest either using a std::list instead, or simply iterating by index. Then you can simultaneously generate primes in one thread and stream them out from other threads.
You're calculating the square root every time through the loop.
I would expect that using mpz_sqrt would at least be faster than hand-rolling a basic iteration method, so why not just use that?

I would like to commend you on your nice use of modern C++

[User Name:]

You start a thread to calculate the primes up to 19, but then immediately go and try outputting the list, whilst the other thread is generating them, and without any locking. Yes I can see why it causes problems, epecially when the vector resizes and all the iterators are invalidated. I assume you understand that the point of a thread is that it doesn't block execution of the thread that created it.
The thing that reads from the list needs to lock also.

Okay, so I need a write lock that will lock all write and read locks. I will work on that, but I think I'm still not understanding why it's aborting with the following modification:

static std::mutex m;

static void extend_primes(mpz_class n)
{
	// Unfortunately, the primes can't effeciently, if at all, be extended concurrently...
	std::lock_guard<std::mutex> lock(m);
	
	for(mpz_class i = primes.back() + 2;i <= n;i += 2)
	{
		bool prime = true; // Assume i is prime.
		
		for(auto j = primes.begin();prime && *j <= sqrt(i);j++)
			if(i % *j == 0)
				prime = false;
		
		if(prime)
			primes.push_back(i);
	}
}

int main()
{
	std::thread t(extend_primes, 19);
	
	std::lock_guard<std::mutex> lock(m); // Wait to print
	
	for(auto i = primes.begin();i < primes.end();i++)
		std::cout << i->get_str() << " ";
	
	std::cout << std::endl;
	return 0;
}

You hold the lock whilst generating the entire list of primes. Surely you want a finer grained lock, i.e. only locking as you actually do a push_back? However, once you do that, you wont be able to iterate through the vector using iterators at the same time. I suggest either using a std::list instead, or simply iterating by index. Then you can simultaneously generate primes in one thread and stream them out from other threads.

But, if I do that, I can't make the assumption that primes contains all primes from 2 to primes.back(), and from that, it follows that I cannot be certain I have the primes necessary to check if primes.back() + 2 is prime, from which it follows that I cannot generate new primes without resorting to brute force checking. I need to be certain of these properties to be able to use my previously generated primes to do an optimal primality check. I need the lock to make sure I insert, in order, all primes from primes.back() to n. I chose not to use a std::set (for its ordered-ness) because it uses extra complexity that I can avoid by inserting them in order. I get what you mean, it's just that there is a good reason for the coarse grained control I'm using.

You're calculating the square root every time through the loop.
I would expect that using mpz_sqrt would at least be faster than hand-rolling a basic iteration method, so why not just use that?

I didn't know they had an integer square root. Thanks. I was also assuming GCC would optimise away unnecessary sqrt() calls. I will profile it when I'm done, with -03 enabled, and if it dosen't get rid of them, I'll create a temporary and so on.

I would like to commend you on your nice use of modern C++

Thanks. I'm so excited about all the new stuff. I've started like 5 new projects in the last few weeks (since the final draft) just to play with regex, thread, etc.

[Elysia]

I'm only going to say say thing now...
What you are doing is wrong, absolutely wrong.
A lock is meant to cause exclusive access to certain resources. This is clearly not what you are trying to do.
What you need is an event. The thread should notify the main function that it has completed, or the main thread can wait until the thread is complete.
What you are doing ends up in a race condition. That is, the code may have two outcomes depending on which thread acquires the lock first.

I'd suggest the later of the two since it's portable. There should be a join member that will block until the thread is finished. Use it.
Unfortunately, to my disappointment, I have not found an easy and portable way of using events. It's a shame, really. Windows supports it gracefully, yet boost neglects to add it.

[User Name:]

Sorry I failed to mention this earlier, but join() doesn't work either. It was actually my first try. [In fact, that is why std::mutex m was originally a static local of extend_primes(); it was intended as a mutually exclusive entrance guard for extend_primes(). My later modification was rushed, and admittedly ill conceived.]

int main()
{
	std::thread t(extend_primes, 19);
	t.join();
	
	for(auto i = primes.begin();i < primes.end();i++)
		std::cout << i->get_str() << " ";
	
	std::cout << std::endl;
	return 0;
}

Same error.

[Elysia]

How does your mpz_class look like?

[User Name:]

Um... I'm confused. I didn't code it; it's GMP's.

[Elysia]

I see... let's just cut away that part and go with ints then...

[User Name:]

#include "gmpxx.h"

#include <cmath>
#include <iostream>
#include <set>
#include <thread>
#include <vector>

// primes must be initialized with at least the first two prime numbers.
static std::vector<int> primes = {2, 3};

static void extend_primes(int n)
{
	// Unfortunately, the primes can't effeciently, if at all, be extended concurrently...
	static std::mutex m;
	std::lock_guard<std::mutex> lock(m);
	
	for(int i = primes.back() + 2;i <= n;i += 2)
	{
		bool prime = true; // Assume i is prime.
		
		for(auto j = primes.begin();prime && *j <= sqrt(i);j++)
			if(i % *j == 0)
				prime = false;
		
		if(prime)
			primes.push_back(i);
	}
}

int main()
{
	std::thread t(extend_primes, 19);
	t.join();
	
	for(auto i = primes.begin();i < primes.end();i++)
		std::cout << *i << " ";
	
	std::cout << std::endl;
	return 0;
}

I also cut out my sqrt, since, with ints, it is unnecessary.

Still same error, though.

[Elysia]

I still can't reproduce your problem.
My VS10 compliant code if anyone else finds they need it:

#include <cmath>
#include <iostream>
#include <set>
#include <boost/thread.hpp>
#include <vector>
#include <cmath>

// primes must be initialized with at least the first two prime numbers.
static std::vector<int> primes;

static void extend_primes(int n)
{
	// Unfortunately, the primes can't effeciently, if at all, be extended concurrently...
	static boost::mutex m;
	boost::lock_guard<boost::mutex> lock(m);
	
	for(int i = primes.back() + 2;i <= n;i += 2)
	{
		bool prime = true; // Assume i is prime.
		
		for(auto j = primes.begin();prime && *j <= (int)std::sqrt((double)i);j++)
			if(i % *j == 0)
				prime = false;
		
		if(prime)
			primes.push_back(i);
	}
}

int main()
{
	primes.push_back(2);
	primes.push_back(3);

	boost::thread t(extend_primes, 19);
	t.join();
	
	for(auto i = primes.begin();i < primes.end();i++)
		std::cout << *i << " ";
	
	std::cout << std::endl;
	return 0;
}

You need to bring out your debugger.

[User Name:]

It's GCC 4.5.2's thread implementation, I think.

I made a SMALL test case, and get the same error.

#include <thread>

using namespace std;

void test() {}

int main()
{
	thread t(test);
}

Lesson learned: Don't assume standard libraries work like they're supposed to.

[Elysia]

That's a pretty big error... is this some alpha release or something?

[User Name:]

No, far from it, it ships default with Ubuntu.

[Elysia]

I find it very difficult to believe they would just throw in something that is half-finished or doesn't work properly into something such as a distribution...
Well, I dunno. You might try boost's threading facilities until the "bugs" are fixed.

[cyberfish]

I can't believe they STILL haven't fixed this.

You need to add "-pthread".

std::thread segfault

2 years ago!!

A more descriptive error message would be nice...