C matrix struct

Question

I am learning C and having a hard time identifying what I am doing wrong since I am getting a segmentation fault. I am trying to initialize a Matrix structure which contains a pointer to a 2D array with the actual data. Then fill it with data from an array and print it.

#include "base.h" 

struct Matrix {
    int rows; // number of rows
    int cols; // number of columns
    double** data; // a pointer to an array of n_rows pointers to rows
};
typedef struct Matrix Matrix;

Matrix* make_matrix(int n_rows, int n_cols) {
    struct Matrix matrix;
    matrix.rows = n_rows;
    matrix.cols = n_cols;
    matrix.data = (double**)malloc(sizeof(double*) * n_rows);
    for(int x = 0; x < n_rows; x++){
        matrix.data[x] = (double*)calloc(n_cols, sizeof(double));
    }
    struct Matrix *m;
    m = &matrix;
    return m;
}

Matrix* copy_matrix(double* data, int n_rows, int n_cols) {
    struct Matrix *matrix = make_matrix(n_rows, n_cols);
    for(int x = 0; x < n_rows; x++) {
        for(int y = 0; y < n_cols; y++) {
            matrix->data[x][y] = data[x+y];
        }
    }
    return matrix;
}

void print_matrix(Matrix* m) {
    for(int x = 0; x < m->rows; x++) {
        for(int y = 0; y < m->cols; y++) {
            printf("%f", m->data[x][y]);
        }
    }
}

void matrix_test(void) {

    double a[] = { 
        1, 2, 3, 
        4, 5, 6, 
        7, 8, 9 };
    Matrix* m1 = copy_matrix(a, 3, 3);
    print_matrix(m1);
}

int main(void) {
    base_init();
    base_set_memory_check(true);
    matrix_test();
    return 0;
}

Also, what is there to change to the better besides the segmentation fault triggering error?

Solution 1

The following:

Matrix* make_matrix(int n_rows, int n_cols) {
    struct Matrix matrix;

declares matrix as a local variable within function make_matrix(). Near the end of that function you take the address of that local variable, you for some reason store it in a pointer, and then you return that pointer. So, you are returning a pointer which points to a location within the stack of the function. But the function has just returned, so that location is now invalid.

Instead of struct Matrix matrix; you need to do Matrix* m = malloc( sizeof( Matrix ) ); (and if your compiler does not allow that, then try Matrix* m; m = malloc( sizeof( Matrix ) );) Then proceed to fill your struct as follows: m->rows = n_rows; etc.

Solution 2

Welcome to C. Two big issues here:

1) You can't return pointer to local variable of function. (relates to bug in make_matrix())

2) There is no obvious way to define 'multidimensional' array in C where you can conveniently access an element like data[x][y], unless your row size is known and fixed at compile time. (And your matrix dimension is not known at compile time.)

Let's deal with them separately.

To solve 1), what you want to do in make_matrix() is actually:

Matrix* make_matrix(int n_rows, int n_cols) {
    struct Matrix* pmatrix = malloc(sizeof(struct Matrix));
    pmatrix->rows = n_rows;
    pmatrix->cols = n_cols;
    ...
    ...
    return pmatrix;
}

But this alone can't fix the bug. The data need to be stored in one big array of cols*rows elements and you need to specify how to locate each item instead of data[x][y].

So to solve 2), your Matrix struct definition should be

struct Matrix {
    int rows; // number of rows
    int cols; // number of columns
    double* data; // <- note that this is a pointer to one dim array
};

, and inside make_matrix()

pmatrix->data = malloc(sizeof(double)*n_rows*n_cols);

is all you need now.

To duplicate a matrix of same dimension and format,

Matrix* copy_matrix(double* data, int n_rows, int n_cols) {
    struct Matrix *matrix = make_matrix(n_rows, n_cols);
    for(int i = 0; i < n_rows*n_cols; i++)
        matrix->data[i] = data[i];
    return matrix;
}

(I would name this function as dup_matrix() instead of copy_matrix() because it actually creates a new instance)

And finally if you want to access an element at [x][y], the position should be explicitly calculated as data[x*cols + y], so the printf() line becomes

printf("%f ", m->data[x*(m->cols) + y]);

Of course you need to properly check return from malloc() for error and properly clean up.

Solution 3

Thank you for your responses, I have learned where I failed. For the sake of completeness, the working code looks like this:

#include "base.h"

struct Matrix {
    int rows; // number of rows
    int cols; // number of columns
    double** data; // a pointer to an array of n_rows pointers to rows; a row is an array of n_cols doubles 
};
typedef struct Matrix Matrix;

Matrix* make_matrix(int n_rows, int n_cols) {
    struct Matrix* matrix = malloc(sizeof(Matrix));
    matrix->rows = n_rows;
    matrix->cols = n_cols;
    double** data = malloc(sizeof(double*) * n_rows); 
    for(int x = 0; x < n_rows; x++){
        data[x] = calloc(n_cols, sizeof(double));
    }
    matrix->data = data;
    return matrix;
}

Matrix* copy_matrix(double* data, int n_rows, int n_cols) {
    struct Matrix *matrix = make_matrix(n_rows, n_cols);
    for(int x = 0; x < n_rows; x++) {
        for(int y = 0; y < n_cols; y++) {
            matrix->data[x][y] = data[n_cols*x+y];
        }
    }
    return matrix;    
}

void print_matrix(Matrix* m) {
    for(int x = 0; x < m->rows; x++) {
        printf("%s", "\n");
        for(int y = 0; y < m->cols; y++) {
            printf("%f\t", m->data[x][y]);
        }
    }
}

void matrix_test(void) {   

    double a[] = { 
        1, 2, 3, 
        4, 5, 6, 
        7, 8, 9,
        10,11,12
        };
    Matrix* m1 = copy_matrix(a, 4, 3);
    print_matrix(m1);
}

int main(void) {
    base_init();
    base_set_memory_check(true);
    matrix_test();
    return 0;
}

If there are more improvements to make, or hurt conventions coming to your minds, please tell me.

Comments

[user2357112]

You're returning a pointer to the local variable matrix.

[too honest for this site]

Your code does not contain a pointer to a 2D array! double * is not a 2D array and cannot represent one, nor is double ** a pointer to a 2D array. And don't cast the result of malloc & friends or void * in general.

[toster-cx]

Or use a static variable.

[Jonathan Leffler]

The alternative is to return the Matrix by value: code as now except Matrix make_matrix(…) { …; return matrix; } (losing Matrix *m = &matrix;). The structure is small enough that it isn't going to be a problem returning a copy of the entire structure.

[Jonathan Leffler]

@toster-cx: No! Then you can only have one Matrix at a time until you make a copy.

[toster-cx]

@Jonathan Leffler I'm aware of that, that was the idea. Probably not the best practice.

[toster-cx]

Also matrix->data[x][y] = data[x+y]; -> matrix->data[x][y] = data[n_rows*x+y];

[Jonathan Leffler]

@toster-cx: Maybe I shouldn't be rising to the bait, but that makes your proposed solution (pointlessly?) thread-unsafe, when a thread-safe solution is available at essentially zero cost. It doesn't seem like a good idea to limit the functionality of your code like that, especially when the alternative is as simple, if not simpler.

[Bob__]

@toster-cx well, in that function, it could also be matrix->data[x][y] = *(data++);, but you're right, OP's code is wrong.

[toster-cx]

2) - wouldn't the way he had set up an array of pointers to arrays work as well?