Printing a matrix causes a segmentation fault

Question

I tried to print a matrix based on this code but somehow it causes a segmentation fault. I want to transfer the given arrays to the struct member data and then print them out by referecing to data. I scanned through it but I cannot find my mistake. Maybe I messed up when allocating memory for the structure.

#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) {
    Matrix* mat = xcalloc(n_rows,sizeof(double*));
    mat->rows = n_rows;
    mat->cols = n_cols;
    return mat;
}

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

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

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);

    double a2[] = { 
        1, 2, 3, 
        4, 5, 6 };
    Matrix* m2 = copy_matrix(a2, 2, 3);
    print_matrix(m2);

    double a3[] = { 
        1, 2, 
        3, 4, 
        5, 6 };
    Matrix* m3 = copy_matrix(a3, 3, 2);
    print_matrix(m3);
}

Answer 1

In this particular example, there is no obvious reason why you should allocate the struct itself dynamically, since it's just 3 items. Because this is plain wrong:

Matrix* mat = xcalloc(n_rows,sizeof(double*));

Either you have to allocate room for the struct and its pointer array separately, or you can just skip dynamic allocation of the struct and then only allocate room for the data member.

Furthermore xcalloc(n_rows,sizeof(double*)); only allocates room for an array of pointers, not what those pointers point at. So you need to allocate room for each data item too, in a separate loop.

Furthermore, using a pointer array for a matrix is almost certainly needlessly obscure and inefficient practice. Instead you should use a 2D array as shown here: Correctly allocating multi-dimensional arrays

You should also free() all memory that you allocated.

---

Here is a cleaned up example where I replaced the pointer to pointer with a so-called "flexible array member". This has the advantage of allocating the struct together with the data for faster access. Also all the problems with fragmented allocation will be gone.

The down side is that a flexible array member has to to be regarded as a flat 1D array, so we have to cast to a pointer to 2D array type before using it with array[i][j] syntax.

#include <stdio.h>
#include <stdlib.h>

typedef struct {
    size_t rows;
    size_t cols;
    double data[];
} Matrix;

Matrix* make_matrix (size_t n_rows, size_t n_cols) {
    Matrix* mat = malloc(sizeof(Matrix) + sizeof(double[n_rows][n_cols]));
    if(mat == NULL)
    {
        return NULL;
    }

    mat->rows = n_rows;
    mat->cols = n_cols;
    return mat;
}

Matrix* copy_matrix (const double* data, size_t n_rows, size_t n_cols) {
    Matrix* mat = make_matrix(n_rows, n_cols);
    double (*new_data)[mat->cols];
    new_data = (double(*)[mat->cols]) mat->data;

    for(size_t i = 0; i < mat->rows; i++)
    {
        for(size_t j = 0; j < mat->cols; j++)
        {
            new_data[i][j] = data[i] + j;
        }
    }
    return mat;
}

void print_matrix (const Matrix* mat)
{
    double (*data)[mat->cols];
    data = (double(*)[mat->cols]) mat->data;

    for(size_t i = 0; i < mat->rows; i++)
    {
        for(size_t j = 0; j < mat->cols; j++)
        {
            printf("%g ", data[i][j]);
        }
        printf("\n");
    }
}

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

    double a2[] = { 
        1, 2, 3, 
        4, 5, 6 };
    Matrix* m2 = copy_matrix(a2, 2, 3);
    print_matrix(m2);
    puts("");

    double a3[] = { 
        1, 2, 
        3, 4, 
        5, 6 };
    Matrix* m3 = copy_matrix(a3, 3, 2);
    print_matrix(m3);

    free(m1);
    free(m2);
    free(m3);
}