4 ms·

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 tear

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 11y ago
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 11y ago
  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 11y ago
    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 11y ago
    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 11y ago
      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).
- haberman 11y ago
  > 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 11y ago
    out_error: dtor(self);
- nkurz 11y ago
  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 11y ago
    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)

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 11y ago

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 11y ago
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 11y ago
  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 11y ago
  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 11y ago
    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).
haberman 11y ago
> 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 11y ago
  out_error: dtor(self);
nkurz 11y ago
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 11y ago
  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)