On: C – Preliminary C2x Charter

[JoshTriplett]

Agreed. __attribute__((cleanup)) provides a way to do block-scoped cleanup of each variable and eliminate the "tear down in reverse order in each error path" or "tear down in reverse order with goto labels" pattern.

[haberman → JoshTriplett]

I have never seen this before but was just reading about it. It appears to run unconditionally when the scope is exited though: how do you prevent it from tearing down even in the success case?

[rwmj → haberman]

Tearing down in the success case is a feature, not a bug! If you don't want cleanup along the success path (eg if you're returning an allocated string), don't use cleanup on the returned variable.

However that does point to the one problem with attribute((cleanup)). If you consider code like this (using the systemd macros):

    int f (void)
    {
      _cleanup_free char *s1;
      _cleanup_free char *s2;
      s1 = strdup ("foo");
      if (!s1) return -1; /* crash */
      s2 = strdup ("bar");
      if (!s2) return -1;
      // ...
      return 0;
    }

If strdup fails, it'll crash at the place I marked because it will try to free the uninitialized pointer s2.

To get around that you have to initialize everything to NULL (since free(NULL) is legal).

The problem then becomes that you end up "over-initializing" and "over-freeing". It would be nice to have a version of attribute((cleanup)) that would eliminate the call to the cleanup function if the pointer is NULL (or uninitialized?). But then you're relying on the compiler to do some kind of dataflow analysis, which is difficult in a standard (excludes simple compiler implementations), and may not even be possible because of the halting problem.

This is basically the reason why you wouldn't want to use cleanups in kernel code, because performance is paramount there and unnecessary cleanup calls wouldn't be welcome.

[JoshTriplett → rwmj]

You could potentially write the "free" function like this:

    static inline void free(void *ptr)
    {
        if (!ptr)
            return;
        _free(ptr);
    }

Then the compiler can easily inline this, omit the NULL check if it knows the pointer can't be NULL, or omit the whole thing if it knows the pointer is NULL.

[the_why_of_y → JoshTriplett]

GCC 5 already does that for you (and more), since malloc/free are C standard library functions:

  #include <stdlib.h>

  int main()
  {
      free(NULL);
      char *foo = malloc(42);
      free(foo);
  }

gcc -O1 compiles it to:

  00000000004004f6 <main>:
    4004f6:	b8 00 00 00 00       	mov    $0x0,%eax
    4004fb:	c3                   	retq   
    4004fc:	0f 1f 40 00          	nopl   0x0(%rax)

[PeCaN → JoshTriplett]

free(NULL) is already perfectly legal (specced to be a no-op) as noted by the parent. The problem is that `char * s;` is probably _not_ NULL, so you'd try to free some random address on the stack. Of course it is easily fixed with `_cleanup_free char * s = NULL;`, but it's a somewhat difficult error to spot sometimes.

EDIT: I super misread that, my bad.

[adrusi → PeCaN]

The point is that this informs the compiler of that behavior of free so that it can optimize it away. Basically it gives you some of the advantages you'd get if free were a compiler intrinsic (which it might be, idk).

[nkurz → rwmj]

I'm not familiar with using attributes for cleanup, but isn't the problem caused by the predeclaration of the variables? That is, wouldn't this work (and be shorter and simpler)?

    int f (void) {
      _cleanup_free char *s1 = strdup ("foo");
      if (!s1) return -1;
      _cleanup_free char *s2 = strdup ("bar");
      if (!s2) return -1;
      // ...
      return 0;
    }

[nkurz → nkurz]

OK, I tested it. Yes, this approach worked fine for me in GCC, Clang, and ICC.

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

  #define autofree __attribute((cleanup(autofree_func)))
  #define PASS 0
  #define FAIL 1

  void autofree_func(void *ptr_ptr) {
    void *ptr = * (void **) ptr_ptr;
    printf("%s(%p)\n", __func__, ptr);
    free(ptr);
  }

  int test(int fail1, int fail2) {
    printf("\n%s(%s, %s):\n", __func__,
           fail1 ? "FAIL" : "PASS",
           fail2 ? "FAIL" : "PASS");
    autofree char *s1 = strdup("foo");
    if (fail1) return -1;
    printf("s1: '%s' (%p)\n", s1, s1);
    autofree char *s2 = strdup("bar");
    if (fail2) return -1;
    printf("s2: '%s' (%p)\n", s2, s2);
    return 0;
  }

  int main(/* int argc, char **argv */) {
    test(PASS, PASS);
    test(PASS, FAIL);
    test(FAIL, FAIL);
    return 0;
  }

nate@skylake$ cc -Wall -Wextra -Wconversion -O3 scoping.c -o scoping

nate@skylake$ ./scoping

  test(PASS, PASS):
  s1: 'foo' (0x2056010)
  s2: 'bar' (0x2056030)
  autofree_func(0x2056030)
  autofree_func(0x2056010)

  test(PASS, FAIL):
  s1: 'foo' (0x2056010)
  autofree_func(0x2056030)
  autofree_func(0x2056010)

  test(FAIL, FAIL):
  autofree_func(0x2056010)

[haberman → rwmj]

> Tearing down in the success case is a feature, not a bug!

I guess it is if you are thinking of this as C++ RAII destructors, which is indeed one very useful case.

It's not useful in some other cases. Like imagine that you are writing the C equivalent of a constructor. If you initialize half your members and then run out of memory, you want to uninitialize only the members you managed to initialize already. attribute((cleanup)) doesn't help with this case.

[loeg → haberman]

    out_error:
      dtor(self);

[loeg → JoshTriplett]

@haberman, you don't use it for constructing an object you want to return.