Flow controlling macros with 'goto'

Question

Yes, two hated constructs combined. Is it as bad as it sounds or can it be seen as a good way to control usage of goto and also provide a reasonable cleanup strategy?

At work we had a discussion about whether or not to allow goto in our coding standard. In general nobody wanted to allow free usage of goto but some were positive about using it for cleanup jumps. As in this code:

void func()
{
   char* p1 = malloc(16);
   if( !p1 )
      goto cleanup;

   char* p2 = malloc(16);
   if( !p2 )
      goto cleanup;

 goto norm_cleanup;

 err_cleanup:

   if( p1 )
      free(p1);

   if( p2 )
      free(p2);

 norm_cleanup:
}

The abovious benefit of such use is that you don't have to end up with this code:

void func()
{
   char* p1 = malloc(16);
   if( !p1 ){
      return;
   }

   char* p2 = malloc(16);
   if( !p2 ){
      free(p1);
      return;
   }

   char* p3 = malloc(16);
   if( !p3 ){
      free(p1);
      free(p2);
      return;
   }
}

Especially in constructor-like functions with many allocations this can sometimes grow very bad, not the least when someone has to insert something in the middle.

So, in order to be able to use goto, but still clearly isolate it from being used freely, a set of flow controlling macros was created for handling the task. Looks something like this (simplified):

#define FAIL_SECTION_BEGIN int exit_code[GUID] = 0;
#define FAIL_SECTION_DO_EXIT_IF( cond, exitcode ) if(cond){exit_code[GUID] = exitcode; goto exit_label[GUID];}
#define FAIL_SECTION_ERROR_EXIT(code) exit_label[GUID]: if(exit_code[GUID]) int code = exit_code[GUID];else goto end_label[GUID]
#define FAIL_SECTION_END end_label[GUID]:

We can use this as follows:

int func()
{
   char* p1 = NULL;
   char* p2 = NULL;
   char* p3 = NULL;

   FAIL_SECTION_BEGIN
   {
      p1 = malloc(16);
      FAIL_SECTION_DO_EXIT_IF( !p1, -1 );

      p2 = malloc(16);
      FAIL_SECTION_DO_EXIT_IF( !p2, -1 );

      p3 = malloc(16);
      FAIL_SECTION_DO_EXIT_IF( !p3, -1 );
   }
   FAIL_SECTION_ERROR_EXIT( code )
   {
      if( p3 ) 
         free(p3);

      if( p2 ) 
         free(p2);

      if( p1 ) 
         free(p1);

      return code;
    }
    FAIL_SECTION_END

  return 0;

It looks nice, and comes with many benefits, BUT, are there any drawbacks we should be thinking about before rolling this out into development? It is after all very flow controlling and goto:ish. Both are discouraged. What are the arguments for discouraging them in this case?

Thanks.

Answer 1

Using goto to go to a common error handler/cleanup/exit sequence is absolutely fine.

Answer 2

This code:

void func()
{
   char* p1 = malloc(16);
   if( !p1 )
      goto cleanup;

   char* p2 = malloc(16);
   if( !p2 )
      goto cleanup;

 cleanup:

   if( p1 )
      free(p1);

   if( p2 )
      free(p2);
}

can be legally written as:

void func()
{
   char* p1 = malloc(16);
   char* p2 = malloc(16);

    free(p1);
    free(p2);
}

whether or not the memory allocations succeed.

This works because free() does nothing if passed a NULL pointer. You can use the same idiom when designing your own APIs to allocate and free other resources:

// return handle to new Foo resource, or 0 if allocation failed
FOO_HANDLE AllocFoo();

// release Foo indicated by handle, - do nothing if handle is 0
void ReleaseFoo( FOO_HANDLE h );

Designing APIs like this can considerably simplify resource management.

Answer 3

The first example looks much more readable to me than the macroised version. And mouviciel said it much better than I did

Answer 4

Cleanup with goto is a common C idiom and is used in Linux kernel*.

*Perhaps Linus' opinion is not the best example of a good argument, but it does show goto being used in a relatively large scale project.

Answer 5

Error handling is one of the rare situations when goto is not so bad.

But if I had to maintain that code I would be very upset that goto are hidden by macros.

So in this case goto is OK for me but not macros.

Answer 6

If the first malloc fails you then cleanup both p1 and p2. Due to the goto, p2 is not initialised and may point to anything. I ran this quickly with gcc to check and attempting to free(p2) would indeed cause a seg fault.

In your last example the variables are scoped inside the braces (i.e. they only exist in the FAIL_SECTION_BEGIN block).

Assuming the code works without the braces you'd still have to initialise all the pointers to NULL before FAIL_SECTION_BEGIN to avoid seg faulting.

I have nothing against goto and macros but I prefer Neil Butterworth's idea..

void func(void)
{
    void *p1 = malloc(16);
    void *p2 = malloc(16);
    void *p3 = malloc(16);

    if (!p1 || !p2 || !p3) goto cleanup;

    /* ... */

cleanup:
    if (p1) free(p1);
    if (p2) free(p2);
    if (p3) free(p3);
}

Or if it's more appropriate..

void func(void)
{
    void *p1 = NULL;
    void *p2 = NULL;
    void *p3 = NULL;

    p1 = malloc(16);
    if (!p1) goto cleanup;

    p2 = malloc(16);
    if (!p2) goto cleanup;

    p3 = malloc(16);
    if (!p3) goto cleanup;

    /* ... */

cleanup:
    if (p1) free(p1);
    if (p2) free(p2);
    if (p3) free(p3);
}

Answer 7

The original code would benefit from using multiple return statements - there is no need to hop around the error return clean up code. Plus, you normally need the allocated space released on an ordinary return too - otherwise you are leaking memory. And you can rewrite the example without goto if you are careful. This is a case where you can usefully declare variables before otherwise necessary:

void func()
{
    char *p1 = 0;
    char *p2 = 0;
    char *p3 = 0;

    if ((p1 = malloc(16)) != 0 &&
        (p2 = malloc(16)) != 0 &&
        (p3 = malloc(16)) != 0)
    {
        // Use p1, p2, p3 ...
    }
    free(p1);
    free(p2);
    free(p3);
}

When there are non-trivial amounts of work after each allocation operation, then you can use a label before the first of the free() operations, and a goto is OK - error handling is the main reason for using goto these days, and anything much else is somewhat dubious.

I look after some code which does have macros with embedded goto statements. It is confusing on first encounter to see a label that is 'unreferenced' by the visible code, yet that cannot be removed. I prefer to avoid such practices. Macros are OK when I don't need to know what they do - they just do it. Macros are not so OK when you have to know what they expand to to use them accurately. If they don't hide information from me, they are more of a nuisance than a help.

Illustration - names disguised to protect the guilty:

#define rerrcheck if (currval != &localval && globvar->currtub &&          \
                    globvar->currtub->te_flags & TE_ABORT)                 \
                    { if (globvar->currtub->te_state)                      \
                         globvar->currtub->te_state->ts_flags |= TS_FAILED;\
                      else                                                 \
                         delete_tub_name(globvar->currtub->te_name);       \
                      goto failure;                                        \
                    }


#define rgetunsigned(b) {if (_iincnt>=2)  \
                           {_iinptr+=2;_iincnt-=2;b = ldunsigned(_iinptr-2);} \
                         else {b = _igetunsigned(); rerrcheck}}

There are several dozen variants on rgetunsigned() that are somewhat similar - different sizes and different loader functions.

One place where these are used contains this loop - in a larger block of code in a single case of a large switch with some small and some big blocks of code (not particularly well structured):

        for (i = 0 ; i < no_of_rows; i++)
            {
            row_t *tmprow = &val->v_coll.cl_typeinfo->clt_rows[i];

            rgetint(tmprow->seqno);
            rgetint(tmprow->level_no);
            rgetint(tmprow->parent_no);
            rgetint(tmprow->fieldnmlen);
            rgetpbuf(tmprow->fieldname, IDENTSIZE);
            rgetint(tmprow->field_no);
            rgetint(tmprow->type);
            rgetint(tmprow->length);
            rgetlong(tmprow->xid);
            rgetint(tmprow->flags);
            rgetint(tmprow->xtype_nm_len);
            rgetpbuf(tmprow->xtype_name, IDENTSIZE);
            rgetint(tmprow->xtype_owner_len);
            rgetpbuf(tmprow->xtype_owner_name, IDENTSIZE);
            rgetpbuf(tmprow->xtype_owner_name,
                     tmprow->xtype_owner_len);
            rgetint(tmprow->alignment);
            rgetlong(tmprow->sourcetype);
            }

It isn't obvious that the code there is laced with goto statements! And clearly, a full exegesis of the sins of the code it comes from would take all day - they are many and varied.

Answer 8

#define malloc_or_die(size) if(malloc(size) == NULL) exit(1)

It's not like you can really recover from failed malloc's unless you have software worth writing a transaction system for, if you do, add roll back code to malloc_or_die.

For a real example of good use of goto, check out parsing dispatch code that use computed goto.