I'm working on a fairly straightforward priority scheduling program which takes a text file with lines in the format:
[N/S/n/s] number number
And I'm attempting to convert the numbers into a double format. I'm trying to do this with a stringstream (this is a class project which must run on a version of Linux without stod), using the example here as a reference: https://www.geeksforgeeks.org/converting-strings-numbers-cc/
The problem is, when I try and implement what I'd think should be a fairly simple few lines of code to do this, I'm met with "Segmentation Fault (Core Dumped)" which seems directly related to my attempt to actually send the stringstream to the double variable I've created. I've included my code so far (still obviously quite far from done) and have also indicated the last line I'm able to execute by outputting "made it to here." I'm very confused about this issue, and would appreciate any help available. Note that although I've posted my entire code for completion's sake, only a small part near the bottom is relevant to the question, which I've clearly indicated.
Code:
#include <iostream>
#include <stdio.h>
#include<string.h>
#include <stdlib.h>
#include<cstring>
#include<sstream>
#include<cstdlib>
#include <unistd.h>
#include<pthread.h>
#include<ctype.h>
#include <vector>
#include <sys/wait.h>
#include <fstream>
#include<ctype.h>
using namespace std;
struct Train{
public:
char trainDirection;
double trainPriority;
double trainTimeToLoad;
double trainTimeToCross;
};
void *trainFunction (void* t){cout << "placeholder function for "<< t <<endl;}
vector<string> split(string str, char c = ' '){
vector<string> result;
int start = 0;
int end = 3;
int loadCounter = 1;
int crossCounter = 1;
result.push_back(str.substr(start, 1));
start = 2;
while (str.at(end) != ' '){
end++;
loadCounter++;
}
result.push_back(str.substr(start, loadCounter));
start = end + 1;
end = start +1;
while(end < str.size()){
end++;
crossCounter++;
}
result.push_back(str.substr(start, crossCounter));
for(int i = 0; i < result.size(); i++){
cout << result[i] <<"|";
}
cout<<endl;
return result;
}
int main(int argc, char **argv){
//READ THE FILE
const char* file = argv[1];
cout << file <<endl;
ifstream fileInput (file);
string line;
char* tokenPointer;
int threadCount = 0;
int indexOfThread = 0;
while(getline(fileInput, line)){
threadCount++;
}
fileInput.clear();
fileInput.seekg(0, ios::beg);
//CREATE THREADS
pthread_t thread[threadCount];
while(getline(fileInput, line)){
vector<string> splitLine = split(line);
//create thread
struct Train *trainInstance;
stringstream directionStringStream(splitLine[0]);
char directionChar = 'x';
directionStringStream >> directionChar;
trainInstance->trainDirection = directionChar;
if(splitLine[0] == "N" || splitLine[0] == "S"){
trainInstance->trainPriority = 1;
}
else{
trainInstance->trainPriority = 0;
}
stringstream loadingTimeStringStream(splitLine[1]);
double doubleLT = 0;
cout << "made it to here" <<endl;
loadingTimeStringStream >> doubleLT; //THIS IS THE PROBLEM LINE
trainInstance->trainTimeToLoad = doubleLT;
stringstream crossingTimeStringStream(splitLine[2]);
double doubleCT = 0;
crossingTimeStringStream >> doubleCT;
trainInstance->trainTimeToCross = doubleCT;
pthread_create(&thread[indexOfThread], NULL, trainFunction,(void *) trainInstance);
indexOfThread++;
}
}
There are some mistakes in your code that lead to undefined behavior and that's the cause of your segmentation fault. Namely:
You don't check the number of arguments before using them
You do not return a value in trainFunction
You do not create a valid object for trainInstance to point to
The first two are somewhat obvious to solve, so I'll talk about the last one. Memory management in C++ is nuanced and the proper solution depends on your use case. Because Train objects are small it would be optimal to have them allocated as local variables. The tricky part here is to ensure they are not destroyed prematurely.
Simply changing the declaration to struct Train trainInstance; won't work because this struct will be destroyed at the end of the current loop iteration while the thread will still be potentially alive and try to access the struct.
To ensure that the Train objects are destroyed after the threads are done we must declare them before the thread array and make sure to join the threads before they go out of scope.
Train trainInstances[threadCount];
pthread_t thread[threadCount];
while(...) {
...
pthread_create(&thread[indexOfThread], nullptr, trainFunction,static_cast<void *>(&trainInstances[indexOfThread]));
}
// Join threads eventually
// Use trainInstances safely after all threads have joined
// trainInstances will be destroyed at the end of this scope
This is clean and works, but it isn't optimal because you might want the threads to outlive the trainInstances for some reason. Keeping them alive until the threads are destroyed is a waste of memory in that case. Depending on the number of objects it might not even be worth it to waste time trying to optimize the time of their destruction but you can do something like the following.
pthread_t thread[threadCount];
{
Train trainInstances[threadCount];
while(...) {
...
pthread_create(&thread[indexOfThread], nullptr, trainFunction,static_cast<void *>(&trainInstances[indexOfThread]));
}
// Have threads use some signalling mechanism to signify they are done
// and will never attempt to use their Train instance again
// Use trainInstances
} // trainInstances destroyed
// threads still alive
It is best to avoid pointers when dealing with threads that don't provide a C++ interface because it is a pain to deal with dynamic memory management when you can't simply pass smart pointers around by value. If you use a new statement, execution must always reach exactly one corresponding delete statement on the returned pointer. While this may sound trivial in some cases it is complicated because of potential exceptions and early return statements.
Finally, note the change of the pthread_create call to the following.
pthread_create(&thread[indexOfThread], nullptr, trainFunction,static_cast<void *>(&trainInstances[indexOfThread]));
There are two significant changes to the safety of this line.
Use of nullptr : NULL has an integral type and can silently be passed to non-pointer argument. Without named arguments, this is a problem because it is hard to spot the error without looking up the function signature and verifying the arguments one-by-one. nullptr is type-safe and will cause a compiler error whenever it is assigned to a non-pointer type without an explicit cast.
Use of static_cast : C-style casts are dangerous business. They will try a bunch of different casts and go with the first one that works, which might not be what you intended. Take a look at the following code.
// Has the generic interface required by pthreads
void* pthreadFunc(void*);
int main() {
int i;
pthreadFunc((void*)i);
}
Oops! It should have been (void*)(&i) to cast the address of i to void*. But the compiler won't throw an error because it can implicitly convert an integral value to a void* so it will simply cast the value of i to void* and pass that to the function with potentially disastrous effects. Using a static_cast will catch that error. static_cast<void*>(i) simply fails to compile, so we notice our mistake and change it to static_cast<void*>(&i)