Re: cleanup: Make no_free_ptr() __must_check

[Rasmus Villemoes]

On 16/08/2023 10.00, Linus Torvalds wrote:
> On Wed, 16 Aug 2023 at 07:23, Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:
>>
>> Ah, ok, I thought the purpose was to ensure the p expression gets
>> evaluated. Well, we can still do without the temp var and weird comma or
>> statement expressions:
> 
> Well, the statement expression version with that types temporary
> pointer was much better than yours, which just randomly returns 'void
> *' and then lets the user assign it to any random pointer with no
> warning.
> 
> But I think you can add just a 'typeof(p)' cast to it and that should
> work out ok. 

Yes, that was an omission, and probably any version will need to have
that cast.

> But all of those macros seem to be fundamentally buggy because 'p'
> gets evaluated twice. It could have side effects, even when just
> having its address taken.

True. I don't know how I convinced myself that & behaved like sizeof(),
probably because I ran with my first "this seems to just be to ensure p
gets evaluated" interpretation. For my own education, '&(foo[bar])' is
by definition equivalent to 'foo + bar', and both foo and bar can be
mostly arbitrary expressions (subject to one being a pointer and the
other an integer). Similarly for &(*foo).

> At that point the whole "comma or statement expression" discussion is
> entirely immaterial.
> 
> So I think it needs to be something like
> 
>   #define __get_and_free_ptr(p) \
>       ({ __auto_type __ptr = &(p); \
>          __auto_type __val = *__ptr; \
>          *__ptr = NULL;  __val; })
> 
> to deal with the "assign NULL and return old value without double evaluation".
> 
> And then you can have a wrapper macro to do the __must_check part,
> something like
> 
>   static inline __must_check
>   const volatile void * __must_check_fn(const volatile void *val)
>   { return val; }
> 
>     #define no_free_ptr(p) \
>         ((typeof(p)) __must_check_fn(__get_and_free_ptr(p)))
> 
> the above is entirely untested. Of course.

I think it should work, the final cast both gives the right type and
removes any unwanted const or volatile qualifiers. It's the same thing
we ended up doing in overflow.h.

While I'm embarrassing myself in public: since p is really only supposed
to be a local auto variable with __cleanup attribute, one could throw in
some ## games to try to prevent no_free_ptr() from being used on other
kinds of lvalues, something like adding "extern int bla_ ## p ## _bla;"
inside __get_and_free_ptr().

Rasmus