Re: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility

[David Lechner]

On 12/6/24 3:19 AM, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
>> On Wed, 20 Nov 2024 at 09:57, David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>>>
>>>         cond_guard(mutex_intr, &st->lock, &ret);
>>>         if (ret)
>>>                 return ret;
>>
>> I'm not convinced that improves on anything.
>>
>> You just replace one disgusting syntax with another, and force people
>> to have a variable that they may not want to have (even if they have
>> an error return variable, it might commonly be an error pointer, for
>> example)
>>
>> I really think the basic issue is that "cond_guard" itself is a pretty
>> broken concept. It simply doesn't work very well in the C syntax.
>>
>> I wish people just gave up on it entirely rather than try to work
>> around that fundamental fact.
>>
>> Not that long ago, Mathieu wanted to introduce "inactive guards" for
>> some similar reasons - kind of "conditional guards, except the
>> conditional is outside the guard". And I pointed out that the fix was
>> to rewrite the disgusting code so that THEY WEREN'T NEEDED in the
>> place he wanted to use them. Rewriting things to "Just Don't Do That,
>> Then" actually just improved code entirely:
>>
>>    https://lore.kernel.org/all/CAHk-=wgRefOSUy88-rcackyb4Ss3yYjuqS_TJRJwY_p7E3r0SA@xxxxxxxxxxxxxx/
>>
>> and honestly, I suspect the same is often true of this whole
>> "if_not_guard()" thing. It's not *hugely* often needed, and I strongly
>> suspect that the explicitly scoped version would be a *lot* safer.
>>
>> The "if_not_guard()" model may be great for mindless conversions of
>> existing code. But I'm not convinced it's a great interface in itself,
>> or that "mindless conversions" of conditional locking is actually a
>> good thing.
> 
> Ok, agreed - and to progress with fixing the bug & the fragility you 
> noticed, let's remove if_cond_guard() as a first step via the patch 
> below.
> 
> Thanks,
> 
> 	Ingo
> 
> =================================>
> From: Ingo Molnar <mingo@xxxxxxxxxx>
> Date: Fri, 6 Dec 2024 10:13:32 +0100
> Subject: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility
> 
> Linus noticed that the new if_not_guard() definition is fragile:
> 
>    "This macro generates actively wrong code if it happens to be inside an
>     if-statement or a loop without a block.
> 
>     IOW, code like this:
> 
>       for (iterate-over-something)
>           if_not_guard(a)
>               return -BUSY;
> 
>     looks like will build fine, but will generate completely incorrect code."
> 
> The reason is that the __if_not_guard() macro is multi-statement, so
> while most kernel developers expect macros to be simple or at least
> compound statements - but for __if_not_guard() it is not so:
> 
>  #define __if_not_guard(_name, _id, args...)            \
>         BUILD_BUG_ON(!__is_cond_ptr(_name));            \
>         CLASS(_name, _id)(args);                        \
>         if (!__guard_ptr(_name)(&_id))
> 
> To add insult to injury, the placement of the BUILD_BUG_ON() line makes
> the macro appear to compile fine, but it will generate incorrect code
> as Linus reported, for example if used within iteration or conditional
> statements that will use the first statement of a macro as a loop body
> or conditional statement body.
> 
> It doesn't appear to be possible to turn this macro into a robust
> single or compound statement that could be used in single statements,
> due to the necessity to define an auto scope variable with an open
> scope and the necessity of it having to expand to a partial 'if'
> statement with no body.
> 
> Instead of trying to work around this fragility, just remove the
> construct before it gets used by code.
> 
> Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
I agree this is the right thing to do. Thanks for writing up the patch.

Acked-by: David Lechner <dlechner@xxxxxxxxxxxx>