Dynamic Allocation and Reallocation of Memory

Question

This is a code snippet of a larger program. I tried to simplify it as much as possible.

This code identifies certain particles and stores them in a list which corresponds to a certain processor.
Besides that I count the number of particles per processor that are already in the list.
After the work is done I want to use the message-passing system MPI to send the particles in the list to other processors (not shown here).

I would like to know:

• Is the process of allocation and deallocation of memory correct?
• Is the code robust?
• Can this be simplified?

Code:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#define DIM 2
typedef struct{
 double m; 
 double x[DIM];
 double v[DIM];
 int id;
} Particle;
typedef struct{
 double **list;
 int *counter;
} ParticleList;
void generateParticles(Particle *p, int N);
void buildList(Particle *p, ParticleList *plist, int numprocs, int N);
int main() {
 time_t t;
 srand((unsigned)time(&t));
 // Generate data, print data
 int N = 50;
 Particle *p = (Particle*)malloc(N*sizeof(*p));
 generateParticles(p, N);
 for(int i=0; i<N; i++){
 printf("id: %d m: %lf x: %lf %lf v: %lf %lf\n", p[i].id, p[i].m, p[i].x[0], p[i].x[1], p[i].v[0], p[i].v[1]);
 }
 // Fill lists of every processor
 int numprocs = 4;
 ParticleList plist;
 plist.list = malloc(sizeof(double*)*numprocs);
 for(int k=0; k<numprocs; k++)
 plist.list[k] = malloc(sizeof(double)*1);
 plist.counter = calloc(numprocs, sizeof(int));
 buildList(p, &plist, numprocs, N);
 for(int k=0; k<numprocs; k++){
 printf("Processor %d Particles in list %d\n", k, plist.counter[k]);
 for(int c=0; c<(DIM*plist.counter[k]); c++){
 printf("%lf ", plist.list[k][c]);
 }
 printf("\n");
 }
 // deallocate memory
 for(int k=0; k<numprocs; k++){
 free(plist.list[k]);
 }
 free(plist.list);
 free(plist.counter);
 free(p);
 return 0;
}
void buildList(Particle *p, ParticleList *plist, int numprocs, int N){
 for(int k=0; k<numprocs; k++){
 for(int i=0; i<N; i++){
 printf("k %d, i %d\n", k, i);
 if(rand()%10 < 5){
 plist->counter[k] = plist->counter[k] + 1;
 plist->list[k] = realloc(plist->list[k], (DIM*plist->counter[k])*sizeof(plist->list[k][0]));
 int d=0;
 for(int j=(plist->counter[k] - 1)*DIM; j < (plist->counter[k] * DIM); j++){
 plist->list[k][j] = p[i].x[d];
 d++;
 }
 }
 }
 }
}
double frand(){
 return (double)rand() / (double)RAND_MAX; 
}
void generateParticles(Particle *p, int N){
 for(int i=0; i<N; i++){
 for(int d=0; d<DIM; d++){
 p[i].x[d] = frand();
 p[i].v[d] = frand();
 }
 p[i].m = frand();
 p[i].id = i;
 }
}

Answer 1

Code would have benefited with allocateList(), allocateParticles(), freeList(), freeParticles() functions, rather than trying coding it all up in main().

allocation and deallocation of memory correct?

Allocations lack error checking. This is the big issue as it also impacts proper de-allocation code.

plist->list[k] = realloc(plist->list[k], .... leak memory should the allocation fail.

DIM*plist->counter[k])*sizeof(plist->list[k][0]) could overflow int math, yet work well with size_t math. See re-ordering below.

Robust-ness

Use size_t instead of int for indexing. As "code snippet of a larger program", the array sizes may be very large. The best type for array indexing is size_t, an unsigned type.

// for(int d=0; d<DIM; d++){
for(size_t d=0; d<DIM; d++){
 p[i].x[d] = frand();
 p[i].v[d] = frand();
 }

Simplification

C does not require the cast nor () with sizeof object

// Particle *p = (Particle*)malloc(N*sizeof(*p));
Particle *p = malloc(N * sizeof *p);
// I prefer `sizeof` first, especially for math that may benefit with `size_t` math.
// v---------------v all size_t math 
Particle *p = malloc(sizeof *p * N * N);
// vs v---v may overflow `int` math
Particle *p = malloc(N * N * sizeof *p);

Simplify coding, review and maintenance. Is plist.list a double *? Why bother attempting to co-coordinate the type of member list with the malloc() call which may be in another file?

// plist.list = malloc(sizeof(double*)*numprocs);
plist.list = malloc(sizeof *(plist.list) * numprocs); 
// plist.counter = calloc(numprocs, sizeof(int));
plist.counter = calloc(numprocs, sizeof *(plist.counter));
// or 
plist.counter = calloc(numprocs, sizeof plist.counter[0]);

The *1 is curious and lacks benefit

// plist.list[k] = malloc(sizeof(double)*1);
plist.list[k] = malloc(sizeof *(plist.list[k]));

---

buildList() would benefit with re-write - IMO, is is not simple to follow. As code lacks guiding comments, I am left to guess how to simplify it without losing meaning. Sample idea with some style improvements.

// Style issue: Avoid wrapping presentation code and horizontal scroll bars
void buildList(Particle *p, size_t n, ParticleList *plist, size_t numprocs) {
 for (size_t np=0; np<numprocs; np++) {
 for (size_t i=0; i<n; i++){
 printf("np %zu, i %zu\n", np, i);
 if (rand()%10 < 5) {
 size_t new_count = plist->counter[k] + 1;
 void *new_ptr = realloc(plist->list[np],sizeof plist->list[np][0] * DIM*new_count);
 if (new_ptr == NULL) {
 TBD_ErrorHandler();
 // ...
 }
 plist->list[k] = new_ptr;
 plist->counter[k] = new_count;
 // I think the following is the same - I had trouble following original code.
 for(size_t dm = 0; dm < DIM; dm++) {
 plist->list[pn][new_count + j] = p[i].x[dm];
 }
 } // endif
 }
 }
}

"l" in "%lf" not required - more of a style issue

// printf("id: %d m: %lf x: %lf %lf v: %lf %lf\n", 
printf("id: %d m: %f x: %f %f v: %f %f\n", 
 p[i].id, p[i].m, p[i].x[0], p[i].x[1], p[i].v[0], p[i].v[1]);

---

Sample ParticleList_free()

void ParticleList_free(ParticleList *pl) {
 if (pl) {
 if (pl->list) {
 for(int k=0; k<numprocs; k++) {
 free(pl->list[k]);
 pl->list[k] = NULL; // ***
 }
 free(pl->list);
 pl->list = NULL; // ***
 }
 free(pl>counter);
 pl>counter = NULL; // ***
 free(pl);
 }
 // Sample usage
 ParticleList *pl = ParticleList_alloc();
 buildList(p, pl, numprocs, N);
 ....
 ParticleList_free(pl);
 pl = NULL; // ***

*** Useful for debugging, but not needed for production code. By setting to NULL, errant uses of pointers are typically more likely to be found. YMMV.

• Gilfoyle: is the part where the deallocation takes place correct? for(int k=0; k<numprocs;k++)free(plist.list[k]);}free(plist.list);free(plist.counter);free(p); So far i don't get an error. But maybe I miss something. Thank you!
• chux: Looks OK, yet I see weaknesses as the free'ing does the loop and free(plist.list);free(pl‌​ist.counter); without first checking if plist is not NULL.
• Gilfoyle: Ok, so I just do in the loop if(plist.list[k] != NULL) and outside if(plist.counter != NULL)?
• chux: @Samuel Sample code posted.
• Gilfoyle: I think in your loop where you free the memory an i is missing. And should it not be pl->list[i] and pl->counter instead of p->list[i] and p->counter?