2

I am using the cleanup variable attribute in order to free a mutex using this macro:

#define LOCK()                                                          \
    pthread_mutex_lock(&mutex);                                         \
    int32_t tmpv_ __attribute__((cleanup(mutexUnlock))) = 0;            \
    (void)tmpv_

The mutexUnlock is a custom function that simply releases the mutex. 

void mutexUnlock(int32_t* dummy)
{
    (void)dummy;
    pthread_mutex_unlock(&mutex);
}

After learning about compound literal's, I wanted to get rid of the tmpv_ variable, since its not needed in this context:

#define LOCK()                                                          \
    pthread_mutex_lock(&mutex);                                         \
    (void)(int32_t __attribute__((cleanup(mutexUnlock)))){0}

But the compiler gives me a warning for each place the macro is used:

warning: 'cleanup' attribute does not apply to types [-Wattributes]

I am not sure how to interpret this. Also the macro doesnt work as supposed to (mutexUnlock is not called). What is the problem with this code?

Łukasz Przeniosło
  • 2,725
  • 5
  • 38
  • 74
  • Did you honor this requirement: `This attribute can only be applied to auto function scope variables; ; it may not be applied to parameters or variables with static storage duration.`? https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html – Fiddling Bits Jan 16 '20 at 20:57
  • Yes, in this case the scope is auto. The variable will live till the end of the block in which it is created. – Łukasz Przeniosło Jan 16 '20 at 21:02
  • 2
    I'm guessing that the whole point of that attribute is that the cleanup function is called when the associated variable goes out of scope. Hence, there needs to be a variable. – user3386109 Jan 16 '20 at 21:03

1 Answers1

2

I am using the cleanup variable attribute in order to free a mutex using this macro [...]

I can't say I think very highly of that idea. It makes your code mysterious to anyone reading it (maybe even future you), and it risks producing undefined behavior in the event that the proxy variable goes out of scope in a different manner than you expect, so that perhaps the mutex is already (or still) unlocked or maybe even uninitialized.

Not to mention that using language extensions, especially such idiosyncratic ones as GCC attributes, ties you very tightly to a particular C implementation. Maybe you don't care about that, but if it's me, I at least want to get something very valuable in return, and I don't see that here.

After learning about compound literal's, I wanted to get rid of the tmpv_ variable, since its not needed in this context.

But it is needed, or at least something is needed to hang the attribute on. I guess you are trying to hang the attribute on a compound literal, but GCC does not document any support for that:

The keyword __attribute__ allows you to specify special properties of variables, function parameters, or structure, union, and, in C++, class members. [...] Other attributes are available for functions (see Function Attributes), labels (see Label Attributes), enumerators (see Enumerator Attributes), statements (see Statement Attributes), and for types (see Type Attributes).

(GCC online documentation)

A compound literal is not any of the items documented to support GCC attributes.

You continue,

#define LOCK()                                                          \
    pthread_mutex_lock(&mutex);                                         \
    (void)(int32_t __attribute__((cleanup(mutexUnlock)))){0}

But the compiler gives me a warning for each place the macro is used:

warning: 'cleanup' attribute does not apply to types [-Wattributes]

I am not sure how to interpret this.

The only syntactic construct in that code to which the attribute could conceivably be attached is the type name int32_t. As GCC warns you, the cleanup attribute does not apply to types (though there are other attributes that do apply to types). The upshot is that that use of the attribute will have no effect, as indeed you observed (another reason to avoid trying to be cute / magical). GCC chooses to warn you about this issue instead of either failing compilation altogether or silently ignoring the attribute.

What is the problem with this code?

If you could attach attributes to compound literals, I would expect GCC to require them to precede or follow the literal, not appear inside it. But again, as far as I can tell, GCC does not support attributes for compound literals.

More generally, the only remotely reasonable way to use the __cleanup__ attribute is to attach it to the object you want cleaned up -- mutex in this case. Attaching it to a proxy object so as to use the scope of that object to trigger random behavior involving a different object is a deplorable abuse of the feature. I would recommend discarding the whole idea and instead unlocking the mutex explicitly.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157
  • Thanks for answer to my question John. I hear your concerns about using cleanup, but I don't fully agree with you- am I using it in a abusive way? Yes, a bit, in regards to the global variable `mutex`. But is the whole concept wrong? Not at all. This way you can have RAII like features for memory deallocation or mutexes freeing without having to worry about it. Yes, this code will only work with GCC ports, so its bad for portability. – Łukasz Przeniosło Jan 16 '20 at 21:45
  • @Bremen, it is exactly for RAII-like behavior that attaching a `__cleanup__` attribute to `mutex` would be potentially reasonable. I still wouldn't do that myself, but I can see the argument for it. But I don't see anything RAII-like in your case: that's why you decided you don't really need the proxy variable, which is in fact the resource being acquired. You are of course welcome to your own opinion, but code such as you presented would never pass code review with me, and I don't think I'm out of the mainstream in that regard. – John Bollinger Jan 16 '20 at 21:52
  • This is a trade off- under the `pthread_mutex_t` is the type `typedef void *pthread_mutex_t;`. It is not possible to make this compatible with the cleanup interface, that is, to pass this mutex as parameter for the `mutexUnlock` function. – Łukasz Przeniosło Jan 16 '20 at 22:26
  • Unless, this syntax is more pleasing for you. It does the same thing, but maybe its more correct in the usage way: https://pastebin.com/ZGZMN6dZ . The `LOCK()` macro could also take the mutex as argument, but in this application this is dedicated to one global mutex only. – Łukasz Przeniosło Jan 16 '20 at 22:45
  • @Bremen, the claim that the cleanup interface is somehow inapplicable to `pthread_mutex_t` seems improbable, and experimentation shows it to be empirically untrue, at least with GCC 4.8.5. If you had said that the semantics of such an RAII approach were not what you needed then I would find that entirely plausible, but it wasn't me who brought up RAII. – John Bollinger Jan 16 '20 at 22:47
  • Let me rephrase- with the updated code from the pastebin link, would you say this time the cleanup is utilized correctly? – Łukasz Przeniosło Jan 17 '20 at 08:02
  • No, @Bremen, I wouldn't. That code still uses the cleanup hook to perform automagical *non-cleanup* actions involving *a different object* (the mutex) than the one going out of scope (a pointer that happens to be pointing to the statically-allocated mutex). It's a bit better than the original in that the proxy object is not wholly synthetic, but it's fundamentally the same as the original. – John Bollinger Jan 17 '20 at 13:46
  • I gues you are right, but I cant think of any better solution. Thanks. – Łukasz Przeniosło Jan 17 '20 at 15:34
  • @Bremen, the better solution is to lock and unlock your mutex explicitly. I don't have a problem with writing macros around those actions if you like, but the unlocks should appear explicitly in the code. It is not a net gain to make the unlocks automagical, because to use that correctly you have to pay exactly as much attention to the scope of your declarations as you otherwise would have to devote to placing explicit unlocks appropriately. You are likely also occasionally to run into circumstances where you *can't* set scope boundaries where you need them to make it work. – John Bollinger Jan 17 '20 at 15:56
  • Im not sure I agree. The whole point of this is to simute an unique_lock from cpp. Its not hard to follow the scope, since you lock in a function and auto unlock on leaving it. As long as you dont use goto (I assume noone does anymore) it will work as desired. The constraint of building the function body for one return only since the unlock is there, or worse, adding unlocks on each return is too much. – Łukasz Przeniosło Jan 17 '20 at 20:34
  • 1
    @Bremen, simulating C++ constructs and idioms in C is usually a fool's errand. It takes a lot of work, rarely turns out well, and typically results in non-idiomatic C. I infer that you would prefer to work in C++, but inasmuch as I guess you are constrained to use C instead, I urge you to strive to stick to typical C style and idiom for the duration. – John Bollinger Jan 17 '20 at 21:23