-1

I want to program a graph but I face issues when I want to add vertices. When I want to reallocate the memory, the program stops and the console just says "aborted (core dumped)".

Here is my code

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

struct Graph
{
    int VertexCounter;
    struct Vertices *vertex;
    struct Edge **adjMat;
}MyGraph;

struct Vertices
{
    int id;
    char name[15];
    float xPos;
    float yPos;
};

struct Edge
{
    int id;
    struct Vertices *start;
    struct Vertices *end;
};


//Initializing a graph with memory for one Vertex and one 1x1 adjecency Matrix but setting the number of Vertices to zero
void initGraph(struct Graph *graph)
{
    graph = (struct Graph *) calloc(1, sizeof(struct Graph));
    graph->vertex = (struct Vertices *) calloc(1, sizeof(struct Vertices));
    graph->adjMat = (struct Edge **) calloc(1, sizeof(struct Edge *));
    *(graph->adjMat) = (struct Edge *) calloc(1, sizeof(struct Edge));

    graph->VertexCounter = 0;
    printf("Number of Vertices: %d\n", graph->VertexCounter);

}

//printing the number of Vertices
void test(struct Graph *graph)
{
    printf("Number of Vertices: %d\n", (*graph).VertexCounter);
}


//Reallocating the memory for an additional Vertex. 
//I multiply the VertexCounter - 1 because the struct Graph contains space for one pointer of the type (struct Vertices *)
void addVertex(struct Graph *graph)
{
    graph->VertexCounter++;
    graph = (struct Graph *) realloc(graph, sizeof(struct Graph) + 
                            (graph->VertexCounter - 1) * sizeof(struct Vertices));

}


int main()
{
    struct Graph *graphPointer;
    graphPointer = &MyGraph;
    initGraph(graphPointer);
    test(graphPointer);
    addVertex(graphPointer);
    test(graphPointer);
    return 0;
}

Output:

Number of Vertices: 0

Number of Vertices: 0

Aborted (core dumped)

So the function addVertex(struct Graph *) doesn't work.

I did not include the reallocation for new Edges yet.

How can I solve this?

Update:

void addVertex(struct Graph **graph)
{
    (*graph)->VertexCounter++;
    *graph = realloc(*graph, sizeof(struct Graph) + 
                ((*graph)->VertexCounter - 1) * sizeof(struct Vertices));

}


int main()
{
    struct Graph *graphPointer;
    graphPointer = &MyGraph;
    initGraph(graphPointer);
    test(graphPointer);
    addVertex(&graphPointer);
    test(graphPointer);
    return 0;
}

This will return the same output as before

Update 2:

void initGraph(struct Graph **graph)
{
    (*graph) = (struct Graph *) calloc(1, sizeof(struct Graph *));
    (*graph)->vertex = (struct Vertices *) calloc(1, sizeof(struct Vertices));
    (*graph)->adjMat = (struct Edge **) calloc(1, sizeof(struct Edge *));
    *((*graph)->adjMat) = (struct Edge *) calloc(1, sizeof(struct Edge));

    (*graph)->VertexCounter = 0;
    printf("Number of Vertices: %d\n", (*graph)->VertexCounter);

}

//printing the number of Vertices
void test(struct Graph *graph)
{
    printf("Number of Vertices: %d\n", (*graph).VertexCounter);
}


//Reallocating the memory for an additional Vertex. 
//I multiply the VertexCounter - 1 because the struct Graph contains space for one pointer of the type (struct Vertices *)
void addVertex(struct Graph **graph)
{

    (*graph)->VertexCounter++;

    void *temp = realloc(temp, sizeof(struct Graph *));
    *graph = temp;

    if(graph == NULL)
        printf("Realloc failed");
}


int main()
{
    struct Graph *graphPointer;
    graphPointer = &MyGraph;
    initGraph(&graphPointer);
    test(graphPointer);
    addVertex(&graphPointer);
    test(graphPointer);
    return 0;
}

I changed initGraph and addVertex, but the output won't change.

Raman
  • 1
  • 3
  • 2
    `void addVertex(struct Graph **graph)` google how to update pointer inside function in C. [example](https://stackoverflow.com/questions/29658614/updating-pointers-in-a-function) – rafix07 Jan 17 '20 at 21:52
  • I will try this and post a comment of my results. Thanks @rafix07 – Raman Jan 17 '20 at 21:54
  • valgrind shows an invalid free in the realloc() call in addVertex(). – jmq Jan 17 '20 at 21:54
  • I posted an update on your suggestion, but this also didn't work. @rafix07 – Raman Jan 17 '20 at 22:00
  • All you are doing with `void initGraph(struct Graph *graph)`, etc.. is creating memory leaks. You must pass the address of the pointer in order to modify the original pointer, otherwise the parameter passed to the function is only a ***copy of*** the original pointer. Since you don't return the new address the copy goes out of scope on the function return and the allocations are never seen back in the caller. There is no need to cast the return of `malloc`, it is unnecessary. See: [Do I cast the result of malloc?](http://stackoverflow.com/q/605845/995714) – David C. Rankin Jan 17 '20 at 22:09
  • You have to fix all functions which take `Graph*` and modyfing it, `initGraph` also must be fixed. – rafix07 Jan 17 '20 at 22:10
  • ALWAYS `realloc` to a temporary pointer, see [How to make a function that modifies a struct](https://stackoverflow.com/questions/55755111/how-to-make-a-function-that-modifies-a-struct/55755175?r=SearchResults&s=1|73.3852#55755175) When `realloc` fails, it returns `NULL` and if you fail to use a temporary pointer, you have just overwritten the pointer to your existing block of memory with `NULL` creating another memory leak. – David C. Rankin Jan 17 '20 at 22:17
  • Thanks to all your replies! @DavidC.Rankin I will later post and update. – Raman Jan 17 '20 at 22:19
  • Good deal, if you are still having problems, I suspect we can clear up any confusion and get your going. – David C. Rankin Jan 17 '20 at 22:21
  • @DavidC.Rankin Should I also update my initGraph to initGraph(struct Graph **) and call this in my main by initGraph(&graphPointer)? But I don't need to calloc a temp pointer, or do I? – Raman Jan 17 '20 at 22:31
  • Yes. For every pointer in the calling function, you pass the address of the pointer to your called function with `&pointer`, (resulting in `type **`) then in the called function you access the address held by the original pointer with `*pointer` (providing `type *`) which you can then update allocate/reallocate, etc.. and it will be seen back in the caller because you are operating on the pointer at the original address. – David C. Rankin Jan 17 '20 at 22:46
  • @DavidC.Rankin I posted a second Update. I tried to use your tips but I think I implemented it wrong... – Raman Jan 17 '20 at 22:47
  • Give me a few minutes and I'll see if I can drop an example. – David C. Rankin Jan 17 '20 at 22:48
  • Another Question: Is it naive from me to program a "dynamic" graph from scratch in c instead using some finished frameworks? Because for my project I would like to implement an extra type of struct nested into the edges. – Raman Jan 17 '20 at 22:57
  • Write your own, otherwise you just inherit somebody else's code. (though take that with a grain of salt -- there are good libraries out there, but the learning factor nosedives using something off-the-shelf) – David C. Rankin Jan 17 '20 at 23:08

2 Answers2

0

As pointed out in the comment above, you need to keep track of when you change, or MAY change graphPointer. For example, any time you call realloc, the system may be forced to move your structure, and returns a new pointer to the new allocation. Again, as pointed out, this new pointer is thrown away, for example, at the end of addVertex. If you are going to change a pointer in a function, just like any other "call by reference" parameter, pass a pointer to it. (If you are completely rigorous about types, and only typecast when you are sure you need to, the compiler helps with this.)

Another thing you really need to do is to check the return values from your calls. Sometimes you think a call is working, but it isn't, and is always returning NULL because your parameters are wrong. Either way, you don't want your program to crash sometime in the future without any indication of why. So eg if realloc fails, at a minimum, print that realloc failed and exit.

Mike Layton
  • 93
  • 1
  • 6
  • Thanks for your response. I will compare from now on if the realloc equals NULL and print the result – Raman Jan 17 '20 at 22:34
  • The first part is the critical part of the answer. If realloc makes your allocation bigger, it is quite likely it moves and copies it, and returns a new pointer. And frees the old allocation! The next time you use the old pointer is guaranteed to give you the crash. – Mike Layton Jan 17 '20 at 22:45
0

OK, I've boiled down what it appears you were attempting to do in your example. One logic issue you are having is in initGraph. There you want to allocate for a struct Graph, not a struct Vertices or struct Edge **adjMat;. Unless you are adding a value for either, then just set those pointers to NULL until such a time as you actually need to allocate for either.

First, let's reorder your struct definitions so any struct required as part of another is defined before it is required, e.g.

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

struct Vertices {
    int id;
    char name[15];
    float xPos;
    float yPos;
};

struct Edge {
    int id;
    struct Vertices *start;
    struct Vertices *end;
};

struct Graph {
    int VertexCounter;
    struct Vertices *vertex;
    struct Edge **adjMat;
};

(note: your instance of MyGraph has been removed -- it is unnecessary if you are allocating for graph)

Let's skip forward and look at how your pointer will be handled in main(), e.g.

Edit Changed Parameter to addVertex

Note in all code below, the parameter for addVertex was changed from struct Graph **graph to struct Graph *graph as there is no reallocation of graphpointer in addVertex.

int main (void) {

    struct Graph *graphPointer = NULL;

    initGraph (&graphPointer);
    test(graphPointer);

    addVertex (graphPointer);
    test(graphPointer);

    freeGraph (graphPointer);   /* don't forget to free what you allocate */
}

Above, graphpointer is the only pointer needed. You then pass the address of graphpointer to initGraph so that initGraph can operate on the original and the allocation within initGraph will be available back in main().

To make that happen, you can do the following in initGraph (note: the allocation for any other members in struct Graph are delayed until you actually have something to add). If you try and pre-allocate for something that will not occur until some unknown point later in your code -- you create a recipe for disaster. Allocate for what you need, when you need it. (that doesn't prevent you from allocating for a block of each and keeping a counter to reduce the number of realloc required)

Your initGraph then becomes:

void initGraph (struct Graph **graph)
{
    *graph = calloc (1, sizeof **graph);
    if (!*graph) {  /* validate EVERY allocation */
        perror ("calloc-*graph");
        exit (EXIT_FAILURE);
    }

    (*graph)->vertex = NULL;    /* set pointers NULL until you add_vertex */
    (*graph)->adjMat = NULL;    /* or you add_adjmat */

    (*graph)->VertexCounter = 0;    /* initialize counter */
}

Above you have initialized one struct Graph -- nothing more -- as it should be. While with initGraph you were required to pass the address of graphpointer so the allocation could be seen back in main(), with addVertex all you need to pass is graphpointer -- because the address of that pointer does NOT change in addVertex. All you are doing is allocating for a member of struct Graph in graphpointer which can simply be assigned to the vertex pointer. Now when you are ready to add a vertex, you can do:

void addVertex (struct Graph *graph)
{
    /* always realloc using a temporary pointer */
    void *tmp = realloc (graph->vertex, (graph->VertexCounter + 1) * 
                            sizeof *graph->vertex);
    if (!tmp) { /* validate EVERY reallocation */
        perror ("realloc-(*graph)->vertex");
        exit (EXIT_FAILURE);
    }
    graph->vertex = tmp;

    graph->VertexCounter++;
}

Also note in addVertex all you are operating on is the struct Vertices member of graphpointer -- also as it should be. If you have a vertex to add at this point, consider passing it in as a parameter (or the data in as parameters) so that the vertex data can be assigned/copied in addVertex to keep all add-vertex code in one place.

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed. A quick freeGraph function to free what has been allocated thus far could be:

void freeGraph (struct Graph *graph)
{
    free (graph->vertex);
    free (graph);
}

(when you add your allocation for adjMat, don't forget to add a corresponding free in freeGraph)

Now putting it altogether, you could do:

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

struct Vertices {
    int id;
    char name[15];
    float xPos;
    float yPos;
};

struct Edge {
    int id;
    struct Vertices *start;
    struct Vertices *end;
};

struct Graph {
    int VertexCounter;
    struct Vertices *vertex;
    struct Edge **adjMat;
};

void initGraph (struct Graph **graph)
{
    *graph = calloc (1, sizeof **graph);
    if (!*graph) {  /* validate EVERY allocation */
        perror ("calloc-*graph");
        exit (EXIT_FAILURE);
    }

    (*graph)->vertex = NULL;    /* set pointers NULL until you add_vertex */
    (*graph)->adjMat = NULL;    /* or you add_adjmat */

    (*graph)->VertexCounter = 0;    /* initialize counter */
}

void test (struct Graph *graph)
{
    printf("Number of Vertices: %d\n", graph->VertexCounter);
}

void addVertex (struct Graph *graph)
{
    /* always realloc using a temporary pointer */
    void *tmp = realloc (graph->vertex, (graph->VertexCounter + 1) * 
                            sizeof *graph->vertex);
    if (!tmp) { /* validate EVERY reallocation */
        perror ("realloc-(*graph)->vertex");
        exit (EXIT_FAILURE);
    }
    graph->vertex = tmp;

    graph->VertexCounter++;
}

void freeGraph (struct Graph *graph)
{
    free (graph->vertex);
    free (graph);
}

int main (void) {

    struct Graph *graphPointer = NULL;

    initGraph (&graphPointer);
    test(graphPointer);

    addVertex (graphPointer);
    test(graphPointer);

    freeGraph (graphPointer);   /* don't forget to free what you allocate */
}

/* move to add_adjmat functions:

    (*graph)->adjMat = calloc (1, sizeof *(*graph)->adjMat);
    if (!(*graph)->adjMat) {
        perror ("calloc-(*graph)->adjMat");
        exit (EXIT_FAILURE);
    }

    *(*graph)->adjMat = calloc (1, sizeof **(*graph)->adjMat);

*/

(note: the comments at the bottom to the allocations for adjMat to whatever function will deal with add_adjmat(), etc..)

Example Use/Output

$ ./bin/graphalloc
Number of Vertices: 0
Number of Vertices: 1

Memory Use/Error Check

It is imperative that you use a memory error checking program to ensure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind --leak-check=full ./bin/graphalloc
==19442== Memcheck, a memory error detector
==19442== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==19442== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==19442== Command: ./bin/graphalloc
==19442==
Number of Vertices: 0
Number of Vertices: 1
==19442==
==19442== HEAP SUMMARY:
==19442==     in use at exit: 0 bytes in 0 blocks
==19442==   total heap usage: 3 allocs, 3 frees, 1,076 bytes allocated
==19442==
==19442== All heap blocks were freed -- no leaks are possible
==19442==
==19442== For counts of detected and suppressed errors, rerun with: -v
==19442== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

This is the approach that will allow you to add what you need when you need. Note to minimize reallocations, you can add a VertexAllocated member and allocate an initial block of 8, 16, 32, etc.. and then when VertexCounter == VertexAllocated you can call realloc then and realloc 2 times VertexAllocated and keep going.

Look things over and let me know if you have questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • Wow, thank you for your reply! I really appreciate your time. I will follow these tips and try to understand pointers better! – Raman Jan 18 '20 at 12:07
  • It works!! Thank you so much!!! I want to upvote your answer but I don't have enough points... – Raman Jan 18 '20 at 12:25
  • Glad it helped. Dealing with dynamic memory is a lot like dealing with your checkbook. If you write a check, you have to have the money to cover it, and you have to make a deposit to bring your balance back up. When you allocate, you have to verify that there was sufficient memory so it succeeds, then you have to `free` what you use to prevent draining your account. Good luck with your coding! – David C. Rankin Jan 18 '20 at 21:09
  • Hello again. I am sorry if the question sounds a bit dumb. We used calloc once for the graph pointer in initGraph() but for the vertices we just started using realloc. But shouldn't I use for the adjacency Matrix just realloc aswell? I use some kind of 2 dimensional array for the matrix. Isn't there a need for a loop to realloc all the coulmns of the matrix? Thank you – Raman Jan 20 '20 at 10:57
  • @Raman - good question. `calloc` allocates a zeros all new memory (which can be helpful if you are filling random indexes (it ensures all memory is initialized). `realloc` and `malloc` do not initialize the memory allocated. In addition if the pointer used with `realloc` is `NULL`, the `realloc` functions like `malloc` and will provide the initial allocation (as well as any subsequent reallocation). So you can use `realloc` as you would with `malloc`, but you must initialize the pointer `NULL` if you want to use it for initial allocation (which is what we do) – David C. Rankin Jan 20 '20 at 21:51