memory mgmt problem

R

richard

I wrote this code to create and store some structs. This code occasionally blows
up in FreeItem, and I can't figure out why. I only see problems with very large
amounts of data, and then only rarely, making it harder to debug. Do you see
any flaws or have any suggestions for improving the routines?

I declare a data structure:

typedef struct {
char this[256];
int that;
} Item; // this is just an example, usually item is more complex

and another struct to hold these structs:

typedef struct {
int entries;
Item **item;
} List;

before first use:

List list;
list.entries = 0;
list.item = NULL;

We add items to the list with:

void AddItem(List *d, Item *item) {

if( (d->item = (Item **)realloc(d->item, (d->entries+1)*sizeof(*d->item))) ==
NULL) {
printf("AddItem: couldn't reallocate memory for entry %d\n", d->entries);
exit(1);
}

if( (d->item[d->entries] = (Item *)malloc(sizeof(*item))) == NULL) {
printf("AddItem: couldn't allocate memory for entry %d\n", d->entries);
exit(1);
}

memcpy(d->item[d->entries++], item, sizeof(*item));
} // AddItem

And when done, call this to free

void FreeList(List *d) {
int i;

for(i = 0; i < d->entries; i++)
free(d->item);
free(d); // this line is sometimes the source of memory errors
}
 
I

Irrwahn Grausewitz

<[email protected]>:

if( (d->item = (Item **)realloc(d->item, (d->entries+1)*sizeof(*d->item))) ==
Click to expand...
^^^^^^^^^
unnecessary cast
if( (d->item[d->entries] = (Item *)malloc(sizeof(*item))) == NULL) {
Click to expand...
^^^^^^^^
unnecessary cast
void FreeList(List *d) {
int i;

for(i = 0; i < d->entries; i++)
free(d->item);
free(d); // this line is sometimes the source of memory errors
Click to expand...

^^^^^^^
I *think* you would like to free( d->item ), as this is still pointing
to allocated memory. Freeing d will fail fatally when you pass &list to
FreeList()!!! (Which I assume you would have done if your sample code
has been accompanied by a main() to show what you're actually doing with
your functions).

Finally, you want to set d->item to NULL in order to give subsequent
attempts to regenerate the list a chance to work properly (as calling
realloc( wild-pointer, ... ) isn't a good idea)!

BTW: why don't you just use a simple linear linked list for a problem
like this? It'll save you many calls to realloc(), which may eventually
fail if you try to realloc huge amounts of contigous memory.

Irrwahn
 
R

richard

<[email protected]>:
void FreeList(List *d) {
int i;

for(i = 0; i < d->entries; i++)
free(d->item);
free(d); // this line is sometimes the source of memory errors
Click to expand...

^^^^^^^
I *think* you would like to free( d->item ), as this is still pointing
to allocated memory. Freeing d will fail fatally when you pass &list to
FreeList()!!! (Which I assume you would have done if your sample code
has been accompanied by a main() to show what you're actually doing with
your functions).
Click to expand...


I believe you're right.
I'm adding items, then accessing them, then freeing the space. free(&d) almost
never fails, which I guess is just luck.
BTW: why don't you just use a simple linear linked list for a problem
like this? It'll save you many calls to realloc(), which may eventually
fail if you try to realloc huge amounts of contigous memory.
Click to expand...

I frequently access the Nth item in the list. I can easily write:
xyz = list->item[n]->this

For a simple linear list, wouldn't I have to iterate through the list to find
the Nth item?
 
I

Irrwahn Grausewitz

richard said:
I believe you're right.
I'm adding items, then accessing them, then freeing the space. free(&d) almost
never fails, which I guess is just luck.
Click to expand...
No, it was bad luck. If it crashed in the first place you would have
known that there's a severe bug. :)
I frequently access the Nth item in the list. I can easily write:
xyz = list->item[n]->this

For a simple linear list, wouldn't I have to iterate through the list to find
the Nth item?
Click to expand...
You're definitely right on that, but as I said, I didn't know what you
want to achieve with your code...

Nevertheless, one proposal for improvement: you could allocate memory
in bigger chunks to avoid at least some of the calls to realloc.

Irrwahn
 
R

richard

<[email protected]>:
Click to expand...
Nevertheless, one proposal for improvement: you could allocate memory
in bigger chunks to avoid at least some of the calls to realloc.
Click to expand...

I've always thought it better to leave this to the compiler and runtime, on the
theory that the people who wrote the malloc, realloc, free code are better at
memory management than I am.

On the other hand, I know more about the specifics of my program than they do.


Going back to an earlier post, there doesn't seem any harm to an unnecessary
cast in malloc or realloc statement
xyz = (xyz **)realloc(

thanks for your help on this. any other suggestions would be appreciated.
 
I

Irrwahn Grausewitz

I've always thought it better to leave this to the compiler and runtime, on the
theory that the people who wrote the malloc, realloc, free code are better at
memory management than I am.
Click to expand...
It's not a question of how good these people are at memory managment.
It's a question of how often you will call [re]alloc() as this will
have an impact on your programs performance.
On the other hand, I know more about the specifics of my program than they do.
Click to expand...
Use this knowledge well, and you will be fine. :))))
Going back to an earlier post, there doesn't seem any harm to an unnecessary
cast in malloc or realloc statement
xyz = (xyz **)realloc(
Click to expand...
It's not harmful, but it may hide errors. Please refer to FAQ 7.6 and
7.7 for an explanation.
thanks for your help on this. any other suggestions would be appreciated.
Click to expand...
No problem.

Irrwahn
 
R

richard

I've always thought it better to leave this to the compiler and runtime, on the
theory that the people who wrote the malloc, realloc, free code are better at
memory management than I am.
Click to expand...
It's not a question of how good these people are at memory managment.
It's a question of how often you will call [re]alloc() as this will
have an impact on your programs performance.
On the other hand, I know more about the specifics of my program than they do.
Click to expand...
Use this knowledge well, and you will be fine. :))))
Click to expand...

What would be a good rewrite for the realloc statement?

void AddItem(List *d, Item *item) {

if( (d->item = realloc(d->item, (d->entries+1)*sizeof(*d->item))) == NULL) {
printf("AddItem: couldn't reallocate memory for entry %d\n", d->entries);
exit(1);
}

if( (d->item[d->entries] = malloc(sizeof(*item))) == NULL) {
printf("AddItem: couldn't allocate memory for entry %d\n", d->entries);
exit(1);
}

memcpy(d->item[d->entries++], item, sizeof(*item));
} // AddItem

Assume a few thousand calls, if that matters. Any suggestions to improve the
following

#define BLKSIZE 5000

void AddItem(List *d, Item *item) {
static int avail = 0, used = 0;

if(avail == 0) {
avail = BLKSIZE;
d->item = malloc(sizeof(*d->item) * avail);
}
else if(used > avail) {
avail += BLKSIZE;
d->item = realloc(d->item, sizeof(*d->item) * avail);
}
if(d->item == NULL) {
if( (d->item = realloc(d->item, (d->entries+1)*sizeof(*d->item))) == NULL) {
printf("AddItem: couldn't allocate enough memory\");
exit(1);
}
avail -= BLKSIZE
}
++used;

if( (d->item[d->entries] = malloc(sizeof(*item))) == NULL) {
printf("AddItem: couldn't allocate memory for entry %d\n", d->entries);
exit(1);
}

memcpy(d->item[d->entries++], item, sizeof(*item));
}
 
I

Irrwahn Grausewitz

richard said:
What would be a good rewrite for the realloc statement?
Click to expand...
Assume a few thousand calls, if that matters. Any suggestions to improve the
following
Click to expand...
Based on your code I come up with the following (I included and enhanced
the struct declarations from your OP):

#define BLKSIZE 5000 /* or 1000, or whatever suits your needs most */

typedef struct Item_t
{
char this[ 256 ];
int that;
} Item_t;

typedef struct List_t
{
int entries;
int avail;
Item **item;
} List_t;

void AddItem( List_t *d, Item_t *item )
{
Item_t **tmp;

if ( ( d->avail == 0 ) || ( d->item == NULL ) )
{
tmp = realloc( d->item, (d->entries + BLKSIZE) * sizeof *d->item );
if( tmp == NULL)
{ /* try to alloacte at least one more: */
tmp = realloc( d->item, ( d->entries + 1 ) * sizeof *d->item );
if ( tmp == NULL )
{
printf("AddItem: couldn't allocate enough memory!");
exit EXIT_FAILURE;
}
else
d->avail++;
}
else
d->avail += BLKSIZE;
}
d->item = tmp;
d->item[ d->entries ] = malloc( sizeof *item );
if ( d->item[ d->entries ] == NULL)
{
printf("AddItem: couldn't allocate memory for entry %d!\n",
d->entries );
exit EXIT_FAILURE;
}
d->avail--;
memcpy( d->item[ d->entries++ ], item, sizeof *item );
}

I didn't test it yet, but it should work. Always make sure your
List_t objects are initialized correctly before the first call to
AddItem().

Exercises: (I always wanted to do that, it's not to annoy you!:))
----------
1. Why wouldn't your last proposal work when called subsequently
with distinct objects of type List_t as the first parameter?

2. Why had I to use the variable tmp in my above code?

3. Think about making BLKSIZE a variable, so you can adjust it
at runtime (maybe useful or not, depends on your programs
goal and runtime behaviour).

Did it help?

Irrwahn
 
I

Irrwahn Grausewitz

<[email protected]>:
Thats what you get when blindly typing code while talking on the phone.
Loads of mistakes and typos. :-(

Here's the corrected code, completed by a small test-main():

/****************************************/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define BLKSIZE 5000

typedef struct Item_t
{
char this[ 256 ];
int that;
} Item_t;

typedef struct List_t
{
int entries;
int avail;
Item_t **item;
} List_t;

void AddItem( List_t *d, Item_t *item )
{
Item_t **tmp;

if ( ( d->avail == 0 ) || ( d->item == NULL ) )
{
tmp = realloc( d->item, (d->entries + BLKSIZE) * sizeof *d->item );
if( tmp == NULL)
{ /* try to alloacte at least one more: */
tmp = realloc( d->item, ( d->entries + 1 ) * sizeof *d->item );
if ( tmp == NULL )
{
printf("AddItem: couldn't allocate enough memory!");
exit( EXIT_FAILURE );
}
else
d->avail++;
}
else
d->avail += BLKSIZE;

d->item = tmp;
}

d->item[ d->entries ] = malloc( sizeof *item );
if ( d->item[ d->entries ] == NULL )
{
printf("AddItem: couldn't allocate memory for entry %d!\n",
d->entries );
exit( EXIT_FAILURE );
}
memcpy( d->item[ d->entries ], item, sizeof *item );
d->entries++;
d->avail--;
}

int main(void)
{
List_t list = { 0, 0, NULL };
Item_t item;
int i;

for ( i = 0; i < 10000; i++ )
AddItem( &list, &item );

return EXIT_SUCCESS;
}
/****************************************/
 
A

Al Bowers

richard said:
I wrote this code to create and store some structs. This code occasionally blows
up in FreeItem, and I can't figure out why. I only see problems with very large
amounts of data, and then only rarely, making it harder to debug. Do you see
any flaws or have any suggestions for improving the routines?

I declare a data structure:

typedef struct {
char this[256];
int that;
} Item; // this is just an example, usually item is more complex

and another struct to hold these structs:

typedef struct {
int entries;
Item **item;
Click to expand...

This is fine but you could also make thie *item.
} List;

before first use:

List list;
list.entries = 0;
list.item = NULL;
Click to expand...

You could intialize in the declaration.
List list = {0, NULL};
We add items to the list with:

void AddItem(List *d, Item *item) {
Click to expand...

It would be good here to return a value which would indicate
an allocation failure and remove the exit function. This gives
a change for the calling function to take different actions if
it choses.

Something like:
Item *AddList(List *d, const char *s, int i);
if( (d->item = (Item **)realloc(d->item, (d->entries+1)*sizeof(*d->item))) ==
NULL) {
printf("AddItem: couldn't reallocate memory for entry %d\n", d->entries);
exit(1);
}

if( (d->item[d->entries] = (Item *)malloc(sizeof(*item))) == NULL) {
printf("AddItem: couldn't allocate memory for entry %d\n", d->entries);
exit(1);
}

memcpy(d->item[d->entries++], item, sizeof(*item));
} // AddItem

And when done, call this to free

void FreeList(List *d) {
int i;

for(i = 0; i < d->entries; i++)
free(d->item);
free(d); // this line is sometimes the source of memory errors
Click to expand...

d->item = NULL;
d->entries = 0;
}
Click to expand...

Example 1: Using Item *item

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

typedef struct {
char this[256];
int that;
} Item; // this is just an example, usually item is more complex

typedef struct {
unsigned entries;
Item *item;
} List;

Item *AddList(List *d, const char *s, int i);
void FreeList(List *d);

int main(void)
{
List data = { 0, NULL};
unsigned i;

AddList(&data, "Hello World!", 15);
AddList(&data, "Goodnight my dear",32);
if(data.item)
for( i = 0; i < data.entries ; i++)
printf("data.item[%u].this = \"%s\"\n"
"data.item[%u].that = %d\n\n",
i,data.item.this,i, data.item.that);
FreeList(&data);
return 0;
}

Item *AddList(List *d, const char *s, int i)
{
Item *tmp;
unsigned u = d->entries,size;

tmp = realloc(d->item,(u+1)*sizeof(*d->item));
if(tmp == NULL) return NULL;

d->item = tmp;
d->item.that = i;
size = sizeof(d->item.this);
strncpy(d->item.this,s,size);
d->item.this[size-1] = '\0';
d->entries++;
return &d->item;
}

void FreeList(List *d)
{
free(d->item);
d->item = NULL;
d->entries = 0;
}
/**********************End****************/

Example 2: Usint Item **item

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

typedef struct {
char this[256];
int that;
} Item; // this is just an example, usually item is more complex

typedef struct {
unsigned entries;
Item **item;
} List;

Item *AddList(List *d, const char *s, int i);
void FreeList(List *d);

int main(void)
{
List data = { 0, NULL};
unsigned i;

AddList(&data, "Hello World!", 15);
AddList(&data, "Goodnight my dear",32);
if(data.item)
for( i = 0; i < data.entries ; i++)
printf("data.item[%u]->this = \"%s\"\n"
"data.item[%u]->that = %d\n\n",
i,data.item->this,i, data.item->that);
FreeList(&data);
return 0;
}

Item *AddList(List *d, const char *s, int i)
{
Item **tmp;
unsigned u = d->entries,size;

if((tmp = realloc(d->item,(u+1)*sizeof(*d->item))) == NULL)
return NULL;
d->item = tmp;
if((d->item = malloc(sizeof *d->item)) == NULL) return NULL;

d->item->that = i;
size = sizeof(d->item->this);
strncpy(d->item->this,s,size);
d->item->this[size-1] = '\0';
d->entries++;
return d->item;
}

void FreeList(List *d)
{
unsigned i;

for(i = 0; i < d->entries;i++)
free(d->item);
free(d->item);
d->item = NULL;
d->entries = 0;
}
 
R

richard

richard wrote: [snip]
typedef struct {
int entries;
Item **item;
Click to expand...

This is fine but you could also make thie *item.
Click to expand...

**item means the storage space for each item is allocated and freed through this
struct, which seems cleaner than the alternative.

[snip]
You could intialize in the declaration.
List list = {0, NULL};
Click to expand...

I go back and forth on that. My current worry is that it I add a new element to
the struct and forget to change the initializers.

[snip]
It would be good here to return a value which would indicate
an allocation failure and remove the exit function. This gives
a change for the calling function to take different actions if
it choses.
Click to expand...

Agreed. In this particular application, exiting is the best alternative, but
for more general use I should let the calling program rather than the sub make
the choice.
Something like:
Item *AddList(List *d, const char *s, int i);
Click to expand...

Why return a pointer to the item rather than an integer return code?

[snip]
void FreeList(List *d)
{
free(d->item);
d->item = NULL;
d->entries = 0;
}
Click to expand...

Going back to *item v. **item, I'd like to free each underlying item, and having
AddItem allocate the storage (rather than just pointing to storage that may be
allocated by the calling function) seems better, especially if you don't know
whether the call will point to static storage (such as "Hello World!") or
something malloc'd.

[snip]

thanks
 
A

Al Bowers

richard said:
richard wrote:
[snip]
typedef struct {
int entries;
Item **item;
Click to expand...

This is fine but you could also make thie *item.
Click to expand...


**item means the storage space for each item is allocated and freed through this
struct, which seems cleaner than the alternative.
Click to expand...

Why does it seem clearer? IMO, if you are allocating a one-dimensional
array, which I interpeted was the attempt of the OP, it would
seem clearer to use *item. On the other hand, for two-dimensional
array you would use **item.
[snip]
You could intialize in the declaration.
List list = {0, NULL};
Click to expand...


I go back and forth on that. My current worry is that it I add a new element to
the struct and forget to change the initializers.
Click to expand...

Why worry? Concerns will still exist that one forgets to add the
additional assignment statement(s).

[snip]
Something like:
Item *AddList(List *d, const char *s, int i);
Click to expand...


Why return a pointer to the item rather than an integer return code?
Click to expand...

In this trivial example, a bool return value would be ok.
However a Item * return value can be used not just for determining
success of failure of the allocation. This appears to be
a matter of preference.

Item *tmp;
if((tmp = AddList(&data,string, number)) != NULL)
printf("Gee, space was successfully allocated for\n"
"the string \"%s\" and the int %d\n",tmp->this,tmp->that);
[snip]
void FreeList(List *d)
{
free(d->item);
d->item = NULL;
d->entries = 0;
}
Click to expand...


Going back to *item v. **item, I'd like to free each underlying item, and having
AddItem allocate the storage (rather than just pointing to storage that may be
allocated by the calling function) seems better, especially if you don't know
whether the call will point to static storage (such as "Hello World!") or
something malloc'd.
Click to expand...

I am lost in understanding this statement. Whether *item or **item all
allocations are assumed to be be "malloc'd", ie. allocated by a call
to realloc, malloc, or calloc. And the function free will free those
dynamic allocations.
 
R

richard

[snip]
I am lost in understanding this statement. Whether *item or **item all
allocations are assumed to be be "malloc'd", ie. allocated by a call
to realloc, malloc, or calloc. And the function free will free those
dynamic allocations.
Click to expand...

I fear I'm being unclear. The issue is storage space for the underlying data
objects pointed to by our item list and how to free that space. In the prior
examples, the underlying data was a char[] and an int.

List contains either *item or **item

*item is a list of pointers to objects that are stored outside List

**item is a list of pointers to objects that are stored in List. AddItem
allocates space for the objects and copies the data into that space.

After creating List and doing whatever we want with it, we'd like to free all
associated storage space, both pointers to data and the underlying data.

In the *item approach, we don't know if the data objects are still being used,
so we can't free that space.

In the **item approach, we know that the List structure owns the space for the
underlying data, so we can feel comfortable freeing it.

hope this makes a bit more sense
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

L-value problem 5
C program: memory leak/ segmentation fault/ memory limit exceeded 0
How do I make this craftinfsystem Work 1
Command Line Arguments 0
Fibonacci 0
Adding adressing of IPv6 to program 1
problem with memory allocation and structs 5
URGENT 1

Members online

No members online now.
Total: 37 (members: 0, guests: 37)
Robots: 403

Forum statistics

Threads
473,755
Messages
2,569,536
Members
45,013
Latest member
KatriceSwa

Latest Threads

Top