3

I am trying to write a function that returns the pointer of 2d array read from a binary file. Although I compile without error there is always a segmentation fault, when I try to print one of the elements of the array. Here my code:

double ** readArray(int rows, int cols)
{
   int i;
   double **myArray=malloc(rows*sizeof(double*));
   if (myArray){ 
      for (i=0; i < rows; i++)
      {
        myArray[i]=malloc(cols*sizeof(double));
      }
   }        

   FILE *data;
   data=fopen("matrix.bin", "rb");
   fread(myArray,sizeof(double),rows*cols,data);

   return myArray; 
}

int main ()
{
  int cols = 7;
  int rows = 15;
  double **myArray=readArray(rows, cols);
  printf("%f\n", myArray[1][0]);
  return 0; 
}
LAM
  • 31
  • 1
  • 2
  • You forgot to `free()` what you `malloc()` 'ed – Michi Feb 24 '16 at 10:27
  • 3
    `myArray` is not a contiguous area of memory, each allocation you make can return a pointer to just about anywhere in the free space. See e.g. [this old answer of mine](http://stackoverflow.com/questions/18440205/casting-void-to-2d-array-of-int-c/18440456#18440456) for the difference between an array of arrays, and a pointer to pointer "matrix". – Some programmer dude Feb 24 '16 at 10:27
  • 1
    Look at the [C FAQ section 6.16](http://c-faq.com/aryptr/dynmuldimary.html) and user the second method to allocate the memory contiguous. If you can ensure that the memory is contiguous you can read the whole file with one `fread()` as your code does. But also consider using `mmap()` – Øystein Schønning-Johansen Feb 24 '16 at 10:30

2 Answers2

2

The problem is that there is no 2D array in your code. The pointer-to-pointer look-up table thing is not a 2D array. It is [rows] number of segments scattered all over the heap, at random places. It is therefore also needlessly slow.

Also, you should keep memory allocation and algorithms separated.

Do something like this instead:

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

void* allocArray (int rows, int cols)
{
  return malloc( sizeof(double[rows][cols]) ); // allocate 1 2D-array
}

void readArray (int rows, int cols, double array[rows][cols])
{
   FILE *data;
   data=fopen("matrix.bin", "rb");
   fread(array, sizeof(double[rows][cols]), 1, data); // read 1 2D-array
}

int main ()
{
  int cols = 7;
  int rows = 15;
  double (*myArray)[cols] = allocArray(rows, cols);

  readArray(rows, cols, myArray);

  printf("%f\n", myArray[1][0]);

  free(myArray); // free 1 2D-array
  return 0; 
}

The reason for the peculiar declaration double (*myArray)[cols] instead of the more logical double (*myArray)[rows][cols], is that we want to avoid the inconvenient array pointer de-referencing syntax. (*myArray)[1][0] is not easy to read. So instead of declaring an array pointer to a 2D array, declare an array pointer to a 1D array, then use pointer indexing on that array pointer. For any pointer, any_pointer[n] gives pointed-at item number n. Array pointers are no difference, so you get 1D array number n.

Lundin
  • 195,001
  • 40
  • 254
  • 396
1

Your fread() call is overwriting all those pointers you painfully set up.

You need to read a single row at a time, and use the set-up pointer to store to:

for(size_t i = 0; i < rows; ++i)
  fread(myArray[i], cols * sizeof *myArray[i], data);

Also, when doing I/O and memory allocation you should check the return values too, of course.

unwind
  • 391,730
  • 64
  • 469
  • 606