2

My C++ program (handling some calculation with matrices, see files below) crashes with the following messages:

*** glibc detected *** ./matrix: munmap_chunk(): invalid pointer: 0x08bfd068 ***

followed by the backtrace and memory map. It happens when I call the set-Method in the Matrix-class for the first time - yet I don't know what I'm doing wrong ... Every help (and general hints for improvement of my code) very appreciated!

Array.cpp

#include <iostream>
#include <stdlib.h>
#include <string.h>
#include "Array.h"

using namespace std;

Array::Array(){
    Array(10);
}

Array::Array(int size){
    data = new int[size];
    memset(data, 0, sizeof(data));
    length = size;
}

Array::~Array(){
    delete [] data;
}

void Array::set(int pos, int value){
    if(pos < 0) pos = 0;
    if(pos >= length) pos = length-1;
    *(data + pos) = value;
}

int Array::get(int pos){
    if(pos < 0) pos = 0;
    if(pos >= length) pos = length-1;
    return *(data + pos);
}

void Array::print(){
    for(int i = 0; i < length; i++){
        cout << *(data + i) << "\t";
    }
    cout << endl;
    return;
}

/*works only for arrays of length 9*/
int Array::find_max(int data[]){
    int max = data[0];

    for(int i = 1; i < 9; i++){
        if(data[i] > max) max = data[i];
    }

    return max;
}

Matrix.h

#ifndef MATRIX_H
#define MATRIX_H
#include "Array.h"

class Matrix{

    private:
        Array * data;
        int height;
        int width;

    public:
        Matrix();
        ...
};

#endif

Matrix.cpp

#include <iostream>
#include <stdlib.h>
#include <string.h>
#include "Matrix.h"

using namespace std;

Matrix::Matrix(){
    Matrix(10, 10);
}

Matrix::Matrix(int h, int w){
    height = h;
    width = w;

    data = new Array(height);

    for(int i = 0; i < height; i++){
        *(data + i) = *(new Array(width));
    }

}

Matrix::~Matrix(){
    for(int i = 0; i < height; i++){
        Array * row = (data + i);
        delete row;
    }

    delete data;
}

void Matrix::set(int h, int w, int value){
    Array row = *(data + h);
    row.set(w, value);
}

...

main.cpp

#include <iostream>
#include <stdlib.h>
#include <string.h>
#include "Array.h"
#include "Matrix.h"

using namespace std;

int main(int argc, char** argv){
    if(argc != 3){
        cout << "usage: " << argv[0] << " <m> x <n>" << endl;
        exit(-1);
    }

    int m = atoi(argv[1]);
    int n = atoi(argv[2]);

    Matrix * myMatrix = new Matrix(m, n);

    /*fill matrix randomly*/
    int guess, minus;
    srand(time(NULL));

    for(int r = 0; r < m; r++){
        for(int c = 0; c < n; c++){
            guess = rand() % 1001;
            minus = rand() % 2;

            if(minus == 0) guess *= -1;
            std::cout << " set " << c << ", " << r << " " << guess << std::endl;
            myMatrix->set(r, c, guess);

        }
    }

    ...

    delete myMatrix;
    ...

    return 0;
}
kafman
  • 2,862
  • 1
  • 29
  • 51
  • 1
    Please use a debugger and/or valgrind to narrow down your issue. That's way too much code to go through. – Mat May 05 '13 at 20:20
  • @Mat I did use a debugger, and narrowed it down to the method Matrix::set, but I included all the code in case it was necessary - but you're right of course - I'll clean that up to the important parths – kafman May 05 '13 at 20:27
  • The problem is that you use pointers and dynamic allocation. It's essentially impossible to write correct code with pointers and dynamic allocation. Use `std::vector` instead. – Kerrek SB May 05 '13 at 20:30
  • @KerrekSB Unfortunately it's a classroom assignment and I have to work with "native" arrays ... – kafman May 05 '13 at 20:44

1 Answers1

3
Matrix::Matrix(){
    Matrix(10, 10);
}

This is not doing what you think it is. It is just creating a temporary matrix and discarding it. That means the height, width and data of the Matrix being constructed are not initialized there.

I think your intention was to do the same thing as was done in Matrix::Matrix(int h, int w). However you can not do it like that.


If you have C++11 support you can use delegated construction to do the same thing.

Matrix::Matrix() :  // <- Notice the colon
    Matrix(10, 10)
{
}

If you don't have C++11 support You have to do it manually

Matrix::Matrix(){
    height = 10;
    width = 10;

    data = new Array(height);

    for(int i = 0; i < height; i++){
        *(data + i) = *(new Array(width));
    }

}

or you can create another member function which does the common operations and call that from both constructors.


Another issue

Array row = *(data + h);
row.set(w, value);

You are copying the array row in to a new Array called row. You don't have a copy constructor. So the value of the pointers is copied directly. When the function returns row is destructed that means the memory pointed by row.data is deleted too and that means the data in the matrix is also affected because it is pointing to the same thing. See The rule of three.

Should be

Array* row = (data + h);
row->set(w, value);
Community
  • 1
  • 1
stardust
  • 5,918
  • 1
  • 18
  • 20
  • Ok, thanks! I fixed this, but it is not the problem, as I always indicate the size of the matrix when creating one ... – kafman May 05 '13 at 20:33
  • @StringerBell See the edit. And also you have a similar problem to the first one in `Array::Array()`; – stardust May 05 '13 at 20:42