Turning on g++ optimization leads to segmentation fault

Question

My program gives segmentation fault when I compile using:

g++ -std=c++11 iForest.cpp -o iForest -O2

I have read this thread --Turning on g++ optimization causes segfault - I don't get it, but I don't think I made the same problem there. I also checked my code. I don't really see where may exist a problem. Please provide some help. Here is my code:

    #include <bits/stdc++.h>
using namespace std;

class iTree{
public:
    iTree(): root(NULL) {}
    iTree(const vector<double>& _items): items(_items){}
    iTree(const string &fname){ readData(fname); }
    ~iTree(){delete root;}

    void print(){
        for(int i = 0; i < np; ++i){
            for(int j = 0; j < nd; ++j){
                cout << items[i*nd + j] << " ";
            }
            cout << endl;
        }
    }

private:
    int height, np, nd; //np: # of points, nd: # of dimensions of a point
    vector<double> items;   // items.size() = np*nd;
    struct Node{
        double val;
        Node *left, *right;
        int attri;  // index of the attribute we pick

        Node(): val(0.0), left(NULL), right(NULL), attri(-1) {}
        ~Node(){ delete left; delete right;}
    } *root;

    void readData(const string &fname){
        ifstream ifs(fname);
        ifs >> np >> nd;

        items.resize(np*nd, 0.0);
        for(int i = 0; i < np*nd; ++i)  ifs >> items[i];
        ifs.close();
    }
};

int main(){
    iTree forest("data.dat");
    forest.print();
    return 0;
}

There is no segfault generated when I compile with g++ -std=c++11 iForest.cpp -o iForest. There is also no segfault generated if I don't print. But I don't think there is any error in my print() function.

Here is the "data.dat" I used if you want to run it:

10 5
509304 9 0 2 1.0
509305 9 0 2 0.0
509306 9 0 2 0.0
509307 9 0 2 0.0
509308 9 0 2 0.0
509309 9 0 2 0.0
509310 9 0 2 0.0
509311 9 0 2 0.0
509312 9 0 2 0.0
509313 9 0 2 0.0

Answer 1

Here is the corrected program, reduced to one file (data is now an embedded string, to make the demo easier).

I have embedded explanatory notes. Note the separation of concerns, use of unique_ptr and as a result, no need for destructors. This is turn gives me free move semantics.

// note 8: include the proper headers
#include <vector>
#include <sstream>
#include <iostream>
#include <memory>

class iTree{
public:
    iTree(): root(nullptr) {}
    iTree(const std::vector<double>& _items): items(_items){}

    // note 5: separate concerns - a tree needs to know nothing about files - only about istreams.
    iTree(std::istream & is){ readData(is); }

    // note 3: destructor now un-necessary

    void print(){
        for(int i = 0; i < np; ++i){
            for(int j = 0; j < nd; ++j){
                std::cout << items[i*nd + j] << " ";
            }
            std::cout << std::endl;
        }
    }

private:
    int height, np, nd; //np: # of points, nd: # of dimensions of a point
    std::vector<double> items;   // items.size() = np*nd;
    struct Node{
        double val;
        // note 1: unique_ptr instead of raw pointer
        std::unique_ptr<Node> left, right;
        int attri;  // index of the attribute we pick

        Node(): val(0.0), left(nullptr), right(nullptr), attri(-1) {}
        // note 4: destructor now un-necessary
    };
    // note 2: unique_ptr instead of raw pointer
    std::unique_ptr<Node> root;

    void readData(std::istream &is){
        is >> np >> nd;

        items.resize(np*nd, 0.0);
        for(int i = 0; i < np*nd; ++i)  is >> items[i];
    }
};

int main(){

    static constexpr auto data =
R"data(10 5
509304 9 0 2 1.0
509305 9 0 2 0.0
509306 9 0 2 0.0
509307 9 0 2 0.0
509308 9 0 2 0.0
509309 9 0 2 0.0
509310 9 0 2 0.0
509311 9 0 2 0.0
509312 9 0 2 0.0
509313 9 0 2 0.0
)data";

    // note 6: now I can express the entire example in one file - no need for a data file.
    std::istringstream is(data);

    // note 7: because I didnt define destuctors I now have move semantics for free
    //         which will be useful for performance and give me more expressive code.
    auto forest = iTree(is);
    forest.print();
    return 0;
}

expected output:

509304 9 0 2 1
509305 9 0 2 0
509306 9 0 2 0
509307 9 0 2 0
509308 9 0 2 0
509309 9 0 2 0
509310 9 0 2 0
509311 9 0 2 0
509312 9 0 2 0
509313 9 0 2 0

Answer 2

As DeiDei wrote in his comment, you delete some pointers you didn't allocated. I'll go further and say that you should use smart pointers and RAII idiom to avoir such problems. C-pointers often lead to a full mountain of problems. Also, use nullptr instead of NULL macro.

EDIT : To add precisions, use std::unique_ptr which is not copyable and only movable, as M.M pointed out in his post.

Answer 3

Your class contains uninitialized variable root, when you enter via the constructor iTree(const string &fname){ readData(fname); } (which you do in your example). Then your destructor does delete root;

To fix this you could initialize root, the easiest way to do this is to put {} before the ; in its declaration.

To avoid bugs later on it would be good to declare both Node and iTree as non-copyable and non-movable, until you are ready to implement copy operations for them. The default copy/move behaviour will lead to memory errors due to rule of three violation.