problem with a code segment.

[Fei Liu]

struct dimension{
int dimlen;
nc_type xtype; // NC_DOUBLE or NC_FLOAT
void * dimdata;
double * bound_data;
double * edges_data;
};

void compute_bound_d ata(dimension * dim){

assert(dim->bound_data == 0 && dim->edges_data == 0);
assert(dim->bound_data = (double *)malloc(dim->dimlen));
assert(dim->edges_data = (double *)malloc(dim->dimlen+1));

double dimdata[dim->dimlen];
if(type_size(di m->xtype) == sizeof(float))
for(int i = 0; i < (int)dim->dimlen; i ++) dimdata[i] = *(((float
*)dim->dimdata)+i);
else
for(int i = 0; i < (int)dim->dimlen; i ++) dimdata[i] =
*(((double*)dim->dimdata)+i);

dim->edges_data[0] = dimdata[0] - (dimdata[1]-dimdata[0])/2;
for(int i = 1; i < (int)dim->dimlen; i ++) dim->edges_data[i] =
(dimdata[i-1] + dimdata[i])/2.;
dim->edges_data[dim->dimlen] = dimdata[dim->dimlen-1] +
(dimdata[dim->dimlen-1]-dimdata[dim->dimlen-2])/2.;

for(int i = 0; i < (int)dim->dimlen; i ++)
dim->bound_data[i] = (dim->edges_data)[i+1] - (dim->edges_data)[i];
}

For some wierd reason, when i = 6, the last line in compute_bound_d ata
starts to overwrite edges_data when everything about edges_data were on
LHS of the assignment...I can't fathom why it's happening...but it does
in the debugger. Any clue why it could have happened from this code
segment? The entire software package is too big to copy/paste.

[Fei Liu]

Re: problem with a code segment.


Fei Liu wrote:[color=blue]
> struct dimension{
> int dimlen;
> nc_type xtype; // NC_DOUBLE or NC_FLOAT
> void * dimdata;
> double * bound_data;
> double * edges_data;
> };
>
> void compute_bound_d ata(dimension * dim){
>
> assert(dim->bound_data == 0 && dim->edges_data == 0);
> assert(dim->bound_data = (double *)malloc(dim->dimlen));
> assert(dim->edges_data = (double *)malloc(dim->dimlen+1));
>[/color]
damn, these should be[color=blue]
> assert(dim->bound_data = (double *)malloc(dim->dimlen * type_size(dim->xtype)));
> assert(dim->edges_data = (double *)malloc(dim->dimlen+1 * type_size(dim->xtype)));[/color]

I ended up examing the assembly to find out this bug...

[mlimber]

Re: problem with a code segment.

Fei Liu wrote:[color=blue]
> struct dimension{
> int dimlen;
> nc_type xtype; // NC_DOUBLE or NC_FLOAT
> void * dimdata;
> double * bound_data;
> double * edges_data;
> };
>
> void compute_bound_d ata(dimension * dim){
>
> assert(dim->bound_data == 0 && dim->edges_data == 0);
> assert(dim->bound_data = (double *)malloc(dim->dimlen));
> assert(dim->edges_data = (double *)malloc(dim->dimlen+1));[/color]

First, it's great that you're using asserts to document your
assumptions. However, you failed to use an assert to validate dim,
dim->dimdata, or dim->dimlen. Also, since asserts disappear when you go
to release mode, you will lose your mallocs. Better to do explicit
checking for them (or perhaps use new/delete or better, std::vector --
see http://www.parashift.com/c++-faq-lit...html#faq-34.1).

Second, note that your dim->dimlen apparently represents the number of
elements (be they float or double) pointed to by your dim->dimdata void
pointer. Yet, your malloc only allocates 1 byte for each. Unless
sizeof(double)= =sizeof(char) on your system (very doubtful), that's a
bug. You probably meant to say malloc( sizeof(double)* dim->dimlen ),
although that allocates twice the memory (on a typical system) required
for float computations.
[color=blue]
>
> double dimdata[dim->dimlen];
> if(type_size(di m->xtype) == sizeof(float))
> for(int i = 0; i < (int)dim->dimlen; i ++) dimdata[i] = *(((float
> *)dim->dimdata)+i);
> else
> for(int i = 0; i < (int)dim->dimlen; i ++) dimdata[i] =
> *(((double*)dim->dimdata)+i);
>
> dim->edges_data[0] = dimdata[0] - (dimdata[1]-dimdata[0])/2;
> for(int i = 1; i < (int)dim->dimlen; i ++) dim->edges_data[i] =
> (dimdata[i-1] + dimdata[i])/2.;
> dim->edges_data[dim->dimlen] = dimdata[dim->dimlen-1] +
> (dimdata[dim->dimlen-1]-dimdata[dim->dimlen-2])/2.;
>
> for(int i = 0; i < (int)dim->dimlen; i ++)
> dim->bound_data[i] = (dim->edges_data)[i+1] - (dim->edges_data)[i];
> }
>
> For some wierd reason, when i = 6, the last line in compute_bound_d ata
> starts to overwrite edges_data when everything about edges_data were on
> LHS of the assignment...I can't fathom why it's happening...but it does
> in the debugger. Any clue why it could have happened from this code
> segment? The entire software package is too big to copy/paste.[/color]

It happens because your memory is not properly allocated. Prefer
containers and smart pointers to handle all dynamic allocations.

Cheers! --M

[Phlip]

Re: problem with a code segment.

Fei Liu wrote:
[color=blue]
> damn, these should be[color=green]
>> assert(dim->bound_data = (double *)malloc(dim->dimlen *
>> type_size(dim->xtype))); assert(dim->edges_data = (double
>> *)malloc(dim->dimlen+1 * type_size(dim->xtype)));[/color]
>
> I ended up examing the assembly to find out this bug...[/color]

Next time don't write so complex code. Don't do important things inside
assert(), because that goes away in Release mode. Don't write so much on
one line. Don't use malloc() in C++ code. (Alternately, declare this to be
C code and compile it as such, so news:comp.lang. c can help instead.)

Now write unit tests for this code, and run them after every few edits. Only
make so few edits that you can run the tests and predict they still pass.
Once the code is safe to change, refactor it to use fewer lines with
simpler expressions.

--
Phlip
http://www.greencheese.org/ZeekLand <-- NOT a blog!!!

[mlimber]

Re: problem with a code segment.

Fei Liu wrote:[color=blue]
> Fei Liu wrote:[color=green]
> > struct dimension{
> > int dimlen;
> > nc_type xtype; // NC_DOUBLE or NC_FLOAT
> > void * dimdata;
> > double * bound_data;
> > double * edges_data;
> > };
> >
> > void compute_bound_d ata(dimension * dim){
> >
> > assert(dim->bound_data == 0 && dim->edges_data == 0);
> > assert(dim->bound_data = (double *)malloc(dim->dimlen));
> > assert(dim->edges_data = (double *)malloc(dim->dimlen+1));
> >[/color]
> damn, these should be[color=green]
> > assert(dim->bound_data = (double *)malloc(dim->dimlen * type_size(dim->xtype)));
> > assert(dim->edges_data = (double *)malloc(dim->dimlen+1 * type_size(dim->xtype)));[/color]
>
> I ended up examing the assembly to find out this bug...[/color]

Easier would be to use the tools C++ (but not C) provides for you to
*automatically* handle memory management. I'll refer you again to the
excellent FAQ:

http://www.parashift.com/c++-faq-lite/containers.html#faq-34.1

Cheers! --M

[Fei Liu]

Re: problem with a code segment.

[color=blue]
> First, it's great that you're using asserts to document your
> assumptions. However, you failed to use an assert to validate dim,
> dim->dimdata, or dim->dimlen. Also, since asserts disappear when you go
> to release mode, you will lose your mallocs. Better to do explicit
> checking for them (or perhaps use new/delete or better, std::vector --
> see http://www.parashift.com/c++-faq-lit...html#faq-34.1).
>
> Second, note that your dim->dimlen apparently represents the number of
> elements (be they float or double) pointed to by your dim->dimdata void
> pointer. Yet, your malloc only allocates 1 byte for each. Unless
> sizeof(double)= =sizeof(char) on your system (very doubtful), that's a
> bug. You probably meant to say malloc( sizeof(double)* dim->dimlen ),
> although that allocates twice the memory (on a typical system) required
> for float computations.
>[color=green]
> >
> > double dimdata[dim->dimlen];
> > if(type_size(di m->xtype) == sizeof(float))
> > for(int i = 0; i < (int)dim->dimlen; i ++) dimdata[i] = *(((float
> > *)dim->dimdata)+i);
> > else
> > for(int i = 0; i < (int)dim->dimlen; i ++) dimdata[i] =
> > *(((double*)dim->dimdata)+i);
> >
> > dim->edges_data[0] = dimdata[0] - (dimdata[1]-dimdata[0])/2;
> > for(int i = 1; i < (int)dim->dimlen; i ++) dim->edges_data[i] =
> > (dimdata[i-1] + dimdata[i])/2.;
> > dim->edges_data[dim->dimlen] = dimdata[dim->dimlen-1] +
> > (dimdata[dim->dimlen-1]-dimdata[dim->dimlen-2])/2.;
> >
> > for(int i = 0; i < (int)dim->dimlen; i ++)
> > dim->bound_data[i] = (dim->edges_data)[i+1] - (dim->edges_data)[i];
> > }
> >
> > For some wierd reason, when i = 6, the last line in compute_bound_d ata
> > starts to overwrite edges_data when everything about edges_data were on
> > LHS of the assignment...I can't fathom why it's happening...but it does
> > in the debugger. Any clue why it could have happened from this code
> > segment? The entire software package is too big to copy/paste.[/color]
>
> It happens because your memory is not properly allocated. Prefer
> containers and smart pointers to handle all dynamic allocations.
>
> Cheers! --M[/color]

Thank you, kind Sir. I had to work on this C->C++ code and it's too
Cish to start with. Once I got time (yes...) I plan to rewrite the
whole thing in C++. The thing is for high performance computing, so I
am trying to make the code as fast as possible.