CODEWITHSUNDEEP
c++segmentation-fault
JMzance
JMzance

Reputation: 1746

Code segfaulting when I move a set of commands into a seperate function

I'm working with the CLHEP library and I have run into some difficulties. Before these changes I had been generating a set of particle momenta as a vector of CLHEP objects (std::vector) and then converting these into vectors of normal arrays - still containing the same momenta - for another program (MadGraph) to use. I wanted to put this conversion routine into a new function and so I made this:

std::vector<double*> MadGraphConvert(vector<CLHEP::HepLorentzVector> p) {

double ptemp[6][4];

for (int i = 0; i < 6; i++) {
    ptemp[i][0] = p.at(i).e();
    ptemp[i][1] = p.at(i).x();
    ptemp[i][2] = p.at(i).y();
    ptemp[i][3] = p.at(i).z();
}

// Give particles to MG in a 'vector of arrays' format
std::vector<double*> p_MG;

p_MG.push_back(ptemp[0]);
p_MG.push_back(ptemp[1]);
p_MG.push_back(ptemp[2]);
p_MG.push_back(ptemp[3]);
p_MG.push_back(ptemp[4]);
p_MG.push_back(ptemp[5]);

return p_MG;
}

Now when I run my code something in this other code throws a segfault but I think I am passing what I had before? My old set of conversions looked like this:

    p[0][0] = pa.e();
    p[0][1] = pa.x();
    p[0][2] = pa.y();
    p[0][3] = pa.z();

    p[1][0] = pb.e();
    p[1][1] = pb.x();
    p[1][2] = pb.y();
    p[1][3] = pb.z();
.
'
'
    std::vector<double*> p_MG;

    p_MG.push_back(p[0]);
    p_MG.push_back(p[1]);
    p_MG.push_back(p[2]);
    p_MG.push_back(p[3]);
    p_MG.push_back(p[4]);
    p_MG.push_back(p[5]);

If anyone can spot where the difference in these two approaches is I would be very grateful! Cheers Jack

Upvotes: 0

Views: 73

Answers (1)

hmjd
hmjd

Reputation: 121971

The vector contains dangling pointers as it being populating with addresses from a local variable, the array ptemp, which is out of scope when the function MadGraphConvert() returns. Dereferencing a dangling pointer is undefined behaviour, in this case it causes a segmentation fault.

Use a std::vector<std::vector<double>> instead:

std::vector<std::vector<double>> ptemp(6, std::vector<double>(4));

The for loop will work as is (as this constructor creates the vector with six elements with each element being another vector of four doubles ) and just return ptemp (remembering to change the return value of MadGraphConvert()).

If, as commented to this answer, the type change is forbidden then dynamically allocate the double[4] array using new. The vector now owns the arrays and must delete[] them when no longer required:

// In function.
std::vector<double*> ptemp(6); // By default, created with six null pointers.
try
{
    for (size_t i(0), count(ptemp.size()); i < count; i++)
    {
        ptemp[i] = new double[4];
        // Remainder of loop as before.
    }
}
catch (std::exception const&) // std::out_of_range, std::bad_alloc
{
    // Exception safety.
    // delete[] whatever was allocated, remember that
    // delete[] on a null pointer is a no-op so no
    // prior check required.
    for (auto i : ptemp) delete[] i;
    throw;
}

and just return ptemp. Later, when vector is no longer required:

// c++11
for (auto i : ptemp) delete[] i;

// c++03
for (std::vector<double*>::iterator i(ptemp.begin());
     i != ptemp.end();
     i++)
{
    delete[] (*i);
}

Upvotes: 4

Related Questions