segmentation fault (probably concerning) 2D array

[dr_michali]

Hi,

Here's a part of the code:

#define S 1000   /* rows */
#define T 200     /* columns */

int main()
{
   int n, i, j;
   float **x;

   x=(float**) malloc (T*sizeof(float*));
   for(i=0; i<T; i++)
      x[i]=(float*) malloc (S*sizeof(float));

   memset(x, 0, sizeof(x[0][0])*S*T);

   return 0;
}

This was compiled well (gcc) and had no troubles running (a.out).
But when I added the following simple thing, just to make a small check:

printf("\n%g\n", x[0][0]);   /* or some other cells in this array */

Compilation was still ok, but I got a segmentation fault.

BTW -
I tried another almost identical version of the malloc, instead of what's above:

x=(float**) malloc (T*sizeof(float));
   for(i=0; i<T; i++)
      x[i]=(float*) malloc (S*sizeof(float));

(which is the correct version? both went fine anyway through compilation and run)

Any kind of help would be great :)
Many thanks!
Michal

[Salem]

> x=(float**) malloc (T*sizeof(float*));
1. Make sure you're compiling your code with a C compiler (not a C++ compiler). If you're getting warnings about "void* to T*", then you're compiling C++ code.
2. Remove the cast of the return result. It does nothing useful if this really is C code.
3. Make sure you're including stdlib.h. If you fail to include this header, AND you have the cast, then you're potentially hiding a very serious bug.

Use x=malloc (T*sizeof(float*)); or better, since it doesn't refer explicitly to a type x= malloc (T*sizeof(*x)); > memset(x, 0, sizeof(x[0][0])*S*T);
This is wrong for two reasons.
1. The memory you allocated was the result of multiple malloc calls. Thus, the memory is NOT contiguous.
2. http://c-faq.com/malloc/calloc.html
All bits zero does NOT imply a floating point constant of 0.0f
If you really want to initialise this memory properly, then you have to use the obvious nested for loops to initialise each element with 0.0

[dr_michali]

Thank you for your quick and most informative reply.
I still have some questions...

1. "cast of the return" - is that the "return 0" at the end of the main function?
2. The code is meant to be in C++ (I'll be using some FFT's and thus some complex variables), but right now it hasn't any C++ chracteristics, so I tried compiling it both through gcc and g++ and I got the same thing going on - that's also why I haven't specified the code.
(while trying to compile with gcc I just commented the few things that make it C++, such as the complex lib and "using namespace std").
I haven't got any warnings such as "void* to T*".
3. I added the stdlib anyway, right now there's still a segmentation fault.
4. "x= malloc (T*sizeof(*x)); " - should that repace all of the malloc 3 lines? or just the last 2?
5. About the initialization, is calloc too (like malloc) no good here? "use the obvious nested for loops" - is this simply placing 0.0 in each cell?

Many thanks once more,
Michal

[jephthah]

(1) means to not cast the return value of malloc. Simply write x=malloc (T*sizeof(float*)); instead of x=(float**) malloc (T*sizeof(float*)); . the cast is not required by the C standard and is redundant. this is at best "bad practice" and at worst can hide a serious bug if you didn't include the <stdlib.h> library.

(3) always include the <stdlib.h> library, otherwise malloc will assume the return of a type int regardless of your attempt to cast its return value. This can completely destroy the stack in some cases. It may not be the case for you, but is still in the "very bad practice" area.

(2) if you want to use the same code for C++ as for C, you invite more complexity that will require conditional compilation statements using #ifdef / #elsif / #endif methods.

Furthermore, the C language can certainly handle complex variables and perform FFT calculations, so this is not a reason for using C++. The <complex.h> library is standard to C99. You should be using this standard if you don't have a compelling reason not to. If you are forced to use C89, complex math can be handled using a third-party library.

(4) this is a style point, and diverges from the original problem. since you've already typed "x" as a type float**, Salem suggests you use sizeof(*x) instead of sizeof(float*) , and sizeof(**x) instead of sizeof(float) .

but a problem is you're not fully allocating your memory correctly. following this style, it would look like this:

for(i = 0; i < T; i++)
      x[i] = malloc (T * sizeof(*x));
      for (j=0; j < S; j++)
         x[i][j] = malloc (S * sizeof(**x));

Now finally ...

(5) This issue is another problem

Memset operates on a contiguous block of memory. multiple calls to malloc (which is correct) will not necessarily be contiguous, not only would this not zero your variables like you expect, it would zero memory that you had no intention of changing.

Calloc allocates the same as malloc but also "zeros" (clears) the memory that was just allocated. Malloc leaves the memory in the "unknown" state and assumes you will take care of it.

In this case you could use calloc to zero out your memory locations. I think the "preferred" method would be to initialize each floating point variable using nested for loops, because it is more clear (readable) and will be less likely to become a source of error (like it is right now). But again, we're in the arguable area of "best practices"

[jephthah]

oops, i had a brainfart at the last minute and edited the code wrong. disregard the code i gave in #4, above. you had the basic form right the first time.

using your style (the style traditionally seen in many texts) it would look like:

x = malloc (T * sizeof(float*));
      for (i=0; i < T; i++)
         x[i] = malloc (S * sizeof(float));

using Salem's style it would look like:

x = malloc (T * sizeof(*x));
      for (i=0; i < T; i++)
         x[i] = malloc (S * sizeof(**x));

.

[dr_michali]

Many thank you for your help as well :)
I tried what you suggested above, and both ways led to the following compilation errors:
- "invalid conversion from 'void*' to 'float**'
- "invalid conversion from 'void*' to 'float*'
This code must be written in C++, and thus I compile with "g++ -lm ... (file name)". I'm not familiar with the #ifdef / #elsif / #endif methods that you mentioned, but I've been writing in C++ for quite a while and never had troubles until using 2D arrays with pointers... (so I've never tried to compile differently)

But the problem was finally solved with the most simple initialization - placing a zero in each and every cell through a loop (I don't know if this is what you call "nested for loops").

There's another thing I'm having troubles with, I'm gonna open a new thread now for this. :)

[Salem]

If this is really C++ (check the forum you post in), then you should be using new[] and delete[] instead of malloc/free

[dr_michali]

Thanks, next time I'll be noticing the forum I post in.