0

I have always learned that it is good to be generous with error checks in your code, and to provide an error message in case certain conditions are met. Consider a program of the following structure:

int main(const int argc, const char *argv[]) {

// read two integers M and N from input file 1 provided in argv[]
// read two integers K and L from input file 2 provided in argv[]

if (M*N < 1000000) {
    allocate array A
} else {
    printf("With your input values, the matrix will be too large!");
    return 1;
}

if (K*L < 1000000) {
    allocate array B
} else {
    printf("With your input values, the matrix will be too large!");
    return 1;
}

// multiply arrays elementwise
// free memory
return 0;
}

Ignore for the moment that this code could be reorganised, such that the validity of the input arguments is checked before allocations take place (which would be the straightforward solution in this simple example). My actual problem will be elaborated on down below for those who are interested.

If in the pseudocode above A is successfully allocated, but B is not because K*L exceeds the limit, the memory free statements at the end of the program are not reached, and a memory leak is associated with A. What would be the best way to avoid this issue if the code cannot be reorganised as described above? Removing the return 1; might cause other errors further down the line. The other two options I could think of are using the notorious goto statement (I did not dare), or indeed including all free calls inside the conditional as;

if (K*L < 1000000) {
    allocate array B
} else {
    printf("With your input values, the matrix will be too large!");
    free(A);
    return 1;
}

, but that would mean a lot of code repetition when one deals with a large number of dynamically allocated arrays. So: what program structure is recommended for a combination of intermediate error-checking and memory handling?

My actual case: I am working with CUDA, which (for those that are unfamiliar with it) allows one to write code to be executed on a GPU. All (or most) code associated with this API returns an error flag, and which it is recommended that it is checked before the program proceeds. In my case, at some point in my program I am allocating memory on the GPU like such (should be readable for those familiar with C):

double *dev_I;
cudaError_t err;
err = cudaMalloc(&dev_I, N*M, sizeof(*dev_I)); // note: cudaMalloc takes a double pointer
if (err != cudaSuccess) { printf("Error allocating dev_I.\n"); return 1; }

Here comes the important part: the next step is to copy memory from the host to a place where the GPU can access it. This looks somewhat like this:

err = cudaMemcpy(dev_I, host_I, M*N * sizeof(*host_I), cudaMemcpyHostToDevice);
if (err != cudaSuccess) { printf("Error copying dev_I"); return 1; }

And this is the part I am concerned about. Should the allocation be successful but the memory copy fail, the program will (should) exit, and I will be left with a memory leak. Hence my question how this is best dealt with.

Yann Droneaud
  • 5,277
  • 1
  • 23
  • 39
Floris
  • 508
  • 2
  • 9
  • 3
    Classic case for `goto`... add variable for the exit status, initialize all pointers to `0` (so a `free()` doesn't hurt) and use something like `goto cleanup;` on errors. –  May 22 '18 at 14:54
  • 2
    `goto` gets a bad rap in C. Many of today's attitudes about it are more along the line of religion than informed understanding. There are superior alternatives for *most* situations, but sometimes `goto` is the tool that yields the cleanest, clearest code. Jumping to distant error-handling code is often one of those cases. – John Bollinger May 22 '18 at 15:20
  • It's a matter of opinion whether it's mandatory to free all memory before exit. The fact that it can be tricky to -- as this question attests -- is one of the strong arguments in favor of *not* worrying about freeing all memory before exit. See [this question](https://stackoverflow.com/questions/36584062/) for discussion on this point. – Steve Summit May 22 '18 at 16:36
  • 1
    no, you won't be left with a memory leak. All GPU memory allocations are automatically freed by the CUDA runtime on the GPU that belong to a process that terminates. IMO use of `goto` is extremely annoying from a compilation perspective as you will soon discover. – Robert Crovella May 23 '18 at 10:12
  • Thanks for your reply, Robert. By that last sentence, do you mean that it slows down compilation considerably? And how do you go about this then, do you not use cudaFree() at all? Or do you use one of the ways I mentioned in the first post? – Floris May 24 '18 at 07:46
  • @FlorisSA he means that you do not need to worry, in CUDA, as if the process exits, the memory is automatically freed. – Ander Biguri Jun 07 '18 at 13:59

2 Answers2

2

General structured programming in C requires goto, as follows:

int f() {
  void *p = alloc_resource();
  if (p == NULL) goto error_1;

  void* q = alloc_resource();
  if (q == NULL) goto error_2;

  return g(p, q);  // g takes ownership

error_2:
  dealloc_resource(p);
error_1:
  return -1;  // some error code
}

You can see how this pattern generalizes to arbitrary numbers or ownership acquisitions: you basically have to keep a list of cleanup operations around and jump to the appropriate one when your control flow hits an error and all state that you've build up so far needs to be unwound.

Usually, one would try to avoid excessively complex functions by keeping each function small so that the cleanup can be done in a more ad-hoc way without gotos, but sometimes you need something structured like this (e.g. when you are composing multiple bits of ownership).

(Other languages offer language-level support for multiple exits, e.g. in C++ this logic is provided by so-called "destructors". They pretty much effect the same thing only in a more composable and reusable way with all the jumps generated by the compiler.)

Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
  • 1
    `requires` considered harmful ;) – 500 - Internal Server Error May 22 '18 at 14:57
  • @500-InternalServerError: Most over-repeated and least understoond-in-context? :-) It may sound suprising, but it's really the only way you can handle *arbitrary* complexity in a systematic fashion. But I think most people prefer to not have arbitrary complexity, and instead stick with very small functions (which is a good thing!), or perhaps choose to not handle ownership completely and accept "benign leaks". (I think what should be "considered harmful" is rampant spaghettification of code, not structured exits.) – Kerrek SB May 22 '18 at 21:23
  • Thanks for this sample, Kerrek. I suppose this combines well with Felix Palmen's suggestion to initialise all pointers to null at the start of the program, which could reduce the number of `goto` statements to a single one before all `free()` calls (and possibly others for file closes, etc). – Floris May 23 '18 at 08:46
  • @FlorisSA: I'm not sure how that relates. Unless you're programming in 40-year old C89, you can declare variables at the point of initialization: `void *p = f(); CHECK; void* q = g(); CHECK;` etc. Note that the earlier errors jump to later labels, so the error handling would never have to even consider variables that haven't yet become relevant. And even if you need to declare variables at the beginning, the same reasoning shows you that your cleanup code will never touch uninitialized variables. – Kerrek SB May 23 '18 at 20:47
0

In addition to @Kerrek SB good answer, another approach is to assign/initialize all objects to some valid state, test the validity of all resources, use if ok, then free.

The key idea is at declaration of the object (e.g. bar *A = NULL), the object A is immediately given a valid value - some valid value.

int foo(void) {
  bar *A = NULL;
  bar *B = NULL;
  int error = 0;

  if (M*N < 1000000) {
    A = allocate(M, N);
  } else {
    printf("With your input values, the matrix will be too large!");
    error = 1;
  }

  if (K*L < 1000000) {
    B = allocate(K, L);
  } else {
    printf("With your input values, the matrix will be too large!");
    error = 1;
  }

  // Was resource allocation successful?
  if (A && B) {
   // multiply arrays
   mul(A,B);
  }

  // free resources
  free(A);
  free(B);  
  return error;
}

A pseudo-code outline

Get resources for A
Get resources for B
Get resources for C
If (OK(A) && OK(B) && OK(C)) {
  Perform the operation(A,B,C)
}
Return resources(C)
Return resources(B)
Return resources(A)

I find this approach easier to test.

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • This approach works in the special case where `free` is allowed on both the success and the failure state of `A` and `B`. But more general resources may have stricter preconditions (e.g.`fclose`), and you would need to distinguish success cleanup from failure cleanup. – Kerrek SB May 22 '18 at 21:32