1

Allocating memory to matrix struct with some values of [row, column] pair for create_matrix function changes values of layer->values pointer which has no relation with matrix struct being allocated.

Some tested [row, column] values are:

[1, 2]
[2, 2]

gdb output for [2,2] i.e create_matrix(2, 2):

(gdb) print *prev_layer
$1 = {
  nodes = 2,
  weights = 0xb6438030,
  biases = 0xb6438060,
  values = 0xb6438080
}
(gdb) n
(before allocation): 0xb6438080
50          weights = create_matrix(2, 2);
(gdb) n
51          if (!weights)
(gdb) print *prev_layer
$2 = {
  nodes = 2,
  weights = 0xb6438030,
  biases = 0xb64380b0, <- this changes
  values = 0xb64380c0 <- this changes
}
(gdb)

From above it seems that it assigns last two pointer associated with memory allocation to last two members of the struct. Sometimes even NULL pointer

Program output for [2,2]:

Values of prev_layer->values
(before allocation): 0xb6438080
(after allocation): 0xb64380c0

Code used:

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


typedef struct matrix {
    int rows;
    int cols;
    double **m;
} matrix;

typedef struct layer {
    int nodes;
    matrix *weights;
    matrix *biases;
    matrix *values;
} layer;

matrix *create_matrix(int rows, int cols) {
    matrix *ret = malloc(sizeof(matrix));
    if (!ret)
        return NULL;
    double **m = malloc(rows * sizeof(double *));
    if (!m)
        return NULL;

    for (int c = 0; c < rows; c++) {
        m[c] = calloc(cols, sizeof(double));
        if (!m[c]) {
            return NULL;
        }
    }

    ret->rows = rows;
    ret->cols = cols;
    ret->m = m;

    return ret;
}

layer *create_layer(int nodes, const layer *prev_layer) {

    matrix *weights, *biases;
    /* Just after allocation it changes pointer of
     * prev_layer->bias and prev_layer->values
     * to last to matrix row allocations
     * bug works with values in ordered pair [row, col] => [1,2], [2,2],
     * doesn't with when used values like [5,3]
     * */
    if (prev_layer)
        printf("(before allocation): %p\n", prev_layer->values);

    weights = create_matrix(1,2);
    if (!weights)
        return NULL;

    if (prev_layer)
        printf("(after allocation): %p\n", prev_layer->values);

    biases = create_matrix(1, nodes);
    if (!biases)
        return NULL;

    matrix *values = create_matrix(1, nodes);
    if (!values)
        return NULL;

    layer *ret = malloc(sizeof(layer *));
    if (!ret)
        return NULL;

    ret->nodes = nodes;
    ret->weights = weights;
    ret->biases = biases;
    ret->values = values;
    return ret;
}

int main() {
    int nodes[] = {2, 2};
    layer *p1 = create_layer(2, NULL);
    layer *p2 = create_layer(2, p1);
    return 0;
}

Compiler: clang 9.0.0

5
  • for (int c = 0; c < cols; c++) That looks wrong. Need to iterate over the rows not the columns. So the result is undefined behaviour.
    – kaylum
    Commented Feb 13, 2020 at 6:22
  • 2
    layer *ret = malloc(sizeof(layer *)); is one major problem. I suggest you use the pattern layer *ret = malloc(sizeof *ret); instead. Commented Feb 13, 2020 at 6:23
  • @kaylum I have edited and tested it but doesn't fix the problem used with [2,2]
    – AVX-42
    Commented Feb 13, 2020 at 6:26
  • He has a keen eye... You were short on allocation by at minimum sizeof (2_pointers+1_int) -- not including padding. Commented Feb 13, 2020 at 6:40
  • @DavidC.Rankin yeah sizeof(layer *) is only size of one pointer 8 and 4 on 32-bit.
    – AVX-42
    Commented Feb 13, 2020 at 6:45

2 Answers 2

2

The type used to compute the allocation size is incorrect in:

layer *ret = malloc(sizeof(layer *));  // should be sizeof(layer)

You allocate the size of a pointer instead of the size of a structure.

To avoid such silly mistakes, you can use the destination pointer type directly:

layer *ret = malloc(sizeof(*ret));

Alternately, you could use allocation wrapper macros and rely on the compiler to detect mismatched types:

#define ALLOC(t)           ((t *)calloc(1, sizeof(t)))
#define ALLOC_ARRAY(t, n)  ((t *)calloc(n, sizeof(t)))

layer *ret = ALLOC(layer);
1

The line with ret's malloc needs to make enough space for a whole layer, however you have asked for enough space for a layer*.

So

layer *ret = malloc(sizeof(layer *));

should be

layer *ret = malloc(sizeof(layer));

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.