-2

I am trying to implement a function that multiplies two dynamic matrices (AxB) and returns a pointer to the dynamically allocated product (C). The arguments of the function are:

  1. pointer to matrix A
  2. a_rows, number of rows in A
  3. a_cols number of columns in A
  4. pointer to matrix B
  5. b_cols, number of columns in B

Since the product will have the same number of rows as A and the same number of columns as B, a_rows and b_cols are used to determine the size of the product to be allocated.

The function is as shown:

double** MatrixMultiply(
    double** a,
    const uint32_t a_rows,
    const uint32_t a_cols,
    double** b,
    const uint32_t b_cols) {

    double** c;
    c = (double**) calloc(a_rows, sizeof(double**));  //product matrix C has 
    //matrix A rows and Matrix B cols

    for(int32_t i = 0; i < a_rows; i++) {
        c[i] = (double*) calloc(b_cols, sizeof(double*));
    }

    for(uint32_t i = 0; i < a_rows; i++) {
        for(uint32_t j = 0; j < b_cols; j++) {
            for(uint32_t k = 0; k < a_cols; k++) {
                c[i][j] += a[i][k] * b[k][j];
            }
        }
    }
    return c;
}

The problem is that whenever I run my program it gets crashed by the function because of what I assume is some sort of segmentation fault. However, whenever I run the debugger and step through each line, it never shows that any errors have occurred.

What is strange to me is that when I change "a_rows" and "b_cols" to a constant (e.g., 20) the function runs correctly. What could be causing this problem? And how can it be fixed? Any help would be greatly appreciated.

trincot
  • 317,000
  • 35
  • 244
  • 286
David D.
  • 59
  • 5

2 Answers2

4

You are allocating the wrong amount of memory.

c[i] = (double*) calloc(b_cols, sizeof(double*));

This allocates memory for b_cols of double-pointers, not b_cols of doubles.

This is the typical error when using typedef(<type>), one make mistakes very easy, because you add a * where is not needed or you forget to add * when it's needed.

It's better practice to use sizeof *var than sizeof(<type>):

int *arr = malloc(size * sizeof *arr);

The sizeof *arr returns the exact amount of bytes regardless of the type. It also has the benefit that if you later happen to change the type of the variable, you don't have to worry about changing the expression inside sizeof.

The correct way is:

c = calloc(a_rows, sizeof *c);

if(c == NULL)
{
    // error handling
}

for(int32_t i = 0; i < a_rows; i++) {
    c[i] = calloc(b_cols, sizeof *c[i]);
    if(c[i] == NULL)
    {
        // error handling
    }
}

Also don't cast malloc and always check that the result is not NULL. If you don't check and the result is NULL, then you will try to access a NULL pointer which is undefined behaviour and will lead to a segfault. Also don't forget to free the memory afterwards.

Pablo
  • 13,271
  • 4
  • 39
  • 59
3

Your code is wrong. calloc (and malloc) can fail, and you always should handle that. You don't need to cast it, and the second argument is the size of each cell (not of a pointer to it).

A minima you should have

double** c=  calloc(a_rows, sizeof(double*));
if (c==NULL) { perror("calloc c"); exit(EXIT_FAILURE); }

and likewise

c[i] = calloc(b_cols, sizeof(double));
if (c[i]==NULL) { perror("calloc c row"); exit(EXIT_FAILURE); };

Notice that for each calloc the second argument is the size of one cell (not of the pointer to it). The first argument is the number of cells (are you sure you want b_cols, not a_cols? only you can know).

For the allocation of c as a whole, in practice all pointers have the same size, so sizeof(double**) is generally the same value (8 bytes on my Linux/x86-64) than sizeof(double*).

You should compile with all warnings and debug info: gcc -Wall -Wextra -g. Some compilers (recent versions of GCC) could have warned you. And you could use valgrind to run (with checks) your program (that could have detected the buffer overflow). Of course you also should use the gdb debugger.

BTW, a better way of having matrixes is to make them some abstract data type like here.

However, whenever I run the debugger and step through each line, it never shows that any errors have occurred.

On Linux (or MacOSX), you could set up your system so that a core dump occurs (be sure to have a large enough limit for your core files, perhaps with the ulimit builtin of bash), and analyze post mortem that core dump with gdb; see core(5), gdb(1), signal(7). And gdb is able to run your program till it crashes.

Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547