memory mgmt problem

Discussion in 'C Programming' started by richard, Sep 1, 2003.

  1. richard

    richard Guest

    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
    }
     
    richard, Sep 1, 2003
    #1
    1. Advertising

  2. richard <> wrote in
    <>:

    <SNIP>
    > if( (d->item = (Item **)realloc(d->item, (d->entries+1)*sizeof(*d->item))) ==

    ^^^^^^^^^
    unnecessary cast
    <SNIP>
    > if( (d->item[d->entries] = (Item *)malloc(sizeof(*item))) == NULL) {

    ^^^^^^^^
    unnecessary cast
    <SNIP>
    >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 *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

    --
    I wish life had a scroll-back buffer.
     
    Irrwahn Grausewitz, Sep 1, 2003
    #2
    1. Advertising

  3. richard

    richard Guest

    On Mon, 01 Sep 2003 15:21:51 +0200, Irrwahn Grausewitz <>
    wrote:

    >richard <> wrote in
    ><>:

    <snip>
    >>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 *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).


    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.


    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?
     
    richard, Sep 1, 2003
    #3
  4. richard <> wrote in
    <>:
    >
    >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.

    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?

    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

    --
    It is not necessary to understand things
    in order to argue about them.
     
    Irrwahn Grausewitz, Sep 1, 2003
    #4
  5. richard

    richard Guest

    On Mon, 01 Sep 2003 16:57:06 +0200, Irrwahn Grausewitz <>
    wrote:

    >richard <> wrote in
    ><>:

    <snip>

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


    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.
     
    richard, Sep 1, 2003
    #5
  6. richard <> wrote in
    <>:

    >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.

    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.

    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(

    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.

    No problem.

    Irrwahn

    --
    It is not necessary to understand things
    in order to argue about them.
     
    Irrwahn Grausewitz, Sep 1, 2003
    #6
  7. richard

    richard Guest

    On Mon, 01 Sep 2003 17:26:19 +0200, Irrwahn Grausewitz <>
    wrote:

    >richard <> wrote in
    ><>:
    >
    >>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.

    >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.

    >Use this knowledge well, and you will be fine. :))))


    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));
    }
     
    richard, Sep 1, 2003
    #7
  8. richard <> wrote in
    <>:
    >
    >What would be a good rewrite for the realloc statement?

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

    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

    --
    My opinions are not those of my ex-employer.
     
    Irrwahn Grausewitz, Sep 1, 2003
    #8
  9. Irrwahn Grausewitz <> wrote in
    <>:
    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;
    }
    /****************************************/

    --
    It is not necessary to understand things
    in order to argue about them.
     
    Irrwahn Grausewitz, Sep 1, 2003
    #9
  10. richard

    Al Bowers Guest

    richard wrote:
    > 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;


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

    > } List;
    >
    > before first use:
    >
    > List list;
    > list.entries = 0;
    > list.item = NULL;


    You could intialize in the declaration.
    List list = {0, NULL};

    >
    > We add items to the list with:
    >
    > void AddItem(List *d, Item *item) {


    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

    d->item = NULL;
    d->entries = 0;

    > }
    >


    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;
    }

    --
    Al Bowers
    Tampa, Fl USA
    mailto: (remove the x)
    http://www.geocities.com/abowers822/
     
    Al Bowers, Sep 2, 2003
    #10
  11. richard

    richard Guest

    On Mon, 01 Sep 2003 20:19:29 -0400, Al Bowers <> wrote:
    >richard wrote:

    [snip]
    >> typedef struct {
    >> int entries;
    >> Item **item;

    >
    >This is fine but you could also make thie *item.


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

    [snip]
    >> list.entries = 0;
    >> list.item = NULL;

    >
    >You could intialize in the declaration.
    >List list = {0, NULL};


    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]
    >> void AddItem(List *d, Item *item) {

    >
    >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.


    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);


    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;
    >}


    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
     
    richard, Sep 2, 2003
    #11
  12. richard

    Al Bowers Guest

    richard wrote:
    > On Mon, 01 Sep 2003 20:19:29 -0400, Al Bowers <> wrote:
    >
    >>richard wrote:

    >
    > [snip]
    >
    >>>typedef struct {
    >>> int entries;
    >>> Item **item;

    >>
    >>This is fine but you could also make thie *item.

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


    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]
    >
    >>>list.entries = 0;
    >>>list.item = NULL;

    >>
    >>You could intialize in the declaration.
    >>List list = {0, NULL};

    >
    >
    > 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.
    >


    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);

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


    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;
    >>}

    >
    >
    > 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.
    >


    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.

    --
    Al Bowers
    Tampa, Fl USA
    mailto: (remove the x)
    http://www.geocities.com/abowers822/
     
    Al Bowers, Sep 2, 2003
    #12
  13. richard

    richard Guest

    On Tue, 02 Sep 2003 12:26:16 -0400, Al Bowers <> wrote:

    [snip]
    >> 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.
    >>

    >
    >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.


    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
     
    richard, Sep 2, 2003
    #13
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Krishnan

    ASP .Net pages - instance mgmt

    Krishnan, Apr 5, 2004, in forum: ASP .Net
    Replies:
    0
    Views:
    304
    Krishnan
    Apr 5, 2004
  2. =?Utf-8?B?QVZM?=

    state mgmt issue

    =?Utf-8?B?QVZM?=, Feb 13, 2006, in forum: ASP .Net
    Replies:
    1
    Views:
    340
    Steve C. Orr [MVP, MCSD]
    Feb 13, 2006
  3. Fastau B

    session mgmt - redirection

    Fastau B, Mar 19, 2006, in forum: ASP .Net
    Replies:
    2
    Views:
    428
    Fastau B
    Mar 20, 2006
  4. Karthik D
    Replies:
    3
    Views:
    731
    Karthik D
    Jul 2, 2003
  5. Mark Shelor

    Memory Mgmt Using C and Perl

    Mark Shelor, Jan 21, 2004, in forum: Perl Misc
    Replies:
    7
    Views:
    175
    Sisyphus
    Jan 22, 2004
Loading...

Share This Page