[RFC,v2] Simply enable one to write code like:

Commit Message

Przemek Kitszel Oct. 1, 2024, 2:57 p.m. UTC

int foo(struct my_drv *adapter)
{
	scoped_guard(spinlock, &adapter->some_spinlock)
		return adapter->spinlock_protected_var;
}

Current scoped_guard() implementation does not support that,
due to compiler complaining:
error: control reaches end of non-void function [-Werror=return-type]

One could argue that for such use case it would be better to use
guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
that coding with my proposed locking style is also very pleasant, as I'm
doing so for a few weeks already.

Technical stuff about the change:
scoped_guard() macro uses common idiom of using "for" statement to declare
a scoped variable. Unfortunately, current logic is too hard for compiler
diagnostics to be sure that there is exactly one loop step; fix that.

To make any loop so trivial that there is no above warning, it must not
depend on any variable to tell if there are more steps. There is no
obvious solution for that in C, but one could use the compound statement
expression with "goto" jumping past the "loop", effectively leaving only
the subscope part of the loop semantics.

More impl details:
one more level of macro indirection is now needed to avoid duplicating
label names;
I didn't spot any other place that is using
	if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop}))
idiom, so it's not packed for reuse what makes actual macros code cleaner.

There was also a need to introduce const 0/1 variable per lock class, it
is used to aid compiler diagnostics reasoning about "exactly 1 step" loops
(note that converting that to function would undo the whole benefit).

NAKed-by: Andy Shevchenko <[e-mail]>
Signed-off-by: Przemek Kitszel <[e-mail]>
---
Andy believes that this change is completely wrong C, the reasons
(that I disagree with of course, are in v1, below the commit message).

v2:
 remove ", 1" condition, as scoped_guard() could be used also for
 conditional locks (try-lock, irq-lock, etc) - this was pointed out by
 Dmitry Torokhov and Dan Carpenter;
 reorder macros to have them defined prior to use - Markus Elfring.

v1:
https://lore.kernel.org/netdev/[e-mail]
---
 include/linux/cleanup.h | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)


base-commit: c824deb1a89755f70156b5cdaf569fca80698719

Comments

Andy Shevchenko Oct. 1, 2024, 3:09 p.m. UTC | #1

On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:

...

> NAKed-by: Andy Shevchenko <[e-mail]>

And still NAKed.

> Signed-off-by: Przemek Kitszel <[e-mail]>
> ---
> Andy believes that this change is completely wrong C, the reasons
> (that I disagree with of course, are in v1, below the commit message).

Have you retested the macro expansion for this version?

...

>   */
>  
> +

Too many blank lines.

> +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
> +static __maybe_unused const bool class_##_name##_is_conditional = _is_cond

...

> +	DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \

Here and everywhere else, boolean has values true and false.

Dmitry Torokhov Oct. 1, 2024, 3:21 p.m. UTC | #2

Hi Przemek,

On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:
> int foo(struct my_drv *adapter)
> {
> 	scoped_guard(spinlock, &adapter->some_spinlock)
> 		return adapter->spinlock_protected_var;
> }

Could you change the subject to say something like:

"Adjust cond_guard() implementation to avoid potential warnings"

And then give detailed explanation in the body?

> 
> Current scoped_guard() implementation does not support that,
> due to compiler complaining:
> error: control reaches end of non-void function [-Werror=return-type]
> 
> One could argue that for such use case it would be better to use
> guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
> that coding with my proposed locking style is also very pleasant, as I'm
> doing so for a few weeks already.

I'd drop this paragraph from the patch description (and moved past "---"
if you prefer to keep it for additional context.

> 
> Technical stuff about the change:
> scoped_guard() macro uses common idiom of using "for" statement to declare
> a scoped variable. Unfortunately, current logic is too hard for compiler
> diagnostics to be sure that there is exactly one loop step; fix that.
> 
> To make any loop so trivial that there is no above warning, it must not
> depend on any variable to tell if there are more steps. There is no
> obvious solution for that in C, but one could use the compound statement
> expression with "goto" jumping past the "loop", effectively leaving only
> the subscope part of the loop semantics.
> 
> More impl details:
> one more level of macro indirection is now needed to avoid duplicating
> label names;
> I didn't spot any other place that is using
> 	if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop}))
> idiom, so it's not packed for reuse what makes actual macros code cleaner.
> 
> There was also a need to introduce const 0/1 variable per lock class, it
> is used to aid compiler diagnostics reasoning about "exactly 1 step" loops
> (note that converting that to function would undo the whole benefit).
> 
> NAKed-by: Andy Shevchenko <[e-mail]>
> Signed-off-by: Przemek Kitszel <[e-mail]>
> ---
> Andy believes that this change is completely wrong C, the reasons
> (that I disagree with of course, are in v1, below the commit message).
> 
> v2:
>  remove ", 1" condition, as scoped_guard() could be used also for
>  conditional locks (try-lock, irq-lock, etc) - this was pointed out by
>  Dmitry Torokhov and Dan Carpenter;
>  reorder macros to have them defined prior to use - Markus Elfring.
> 
> v1:
> https://lore.kernel.org/netdev/[e-mail]
> ---
>  include/linux/cleanup.h | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index a3d3e888cf1f..72dcfeb3ec13 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -151,12 +151,18 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>   *
>   */
>  
> +
> +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\

This is not supposed to be used outside of cleanup.h so probably
__DEFINE_CLASS_IS_CONDITIONAL()?
> +static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
> +
>  #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
> +	DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \
>  	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
>  	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
>  	{ return *_T; }
>  
>  #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
> +	DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1); \
>  	EXTEND_CLASS(_name, _ext, \
>  		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
>  		     class_##_name##_t _T) \
> @@ -167,10 +173,18 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  	CLASS(_name, __UNIQUE_ID(guard))
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
> +#define __is_cond_ptr(_name) class_##_name##_is_conditional
> +
> +#define scoped_guard(_name, args...)	\
> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>  
> -#define scoped_guard(_name, args...)					\
> -	for (CLASS(_name, scope)(args),					\
> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> +#define __scoped_guard_labeled(_label, _name, args...)	\
> +	if (0)						\
> +		_label: ;				\
> +	else						\
> +		for (CLASS(_name, scope)(args);		\
> +		     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
> +		     ({goto _label;}))

The "jump back" throws me a little, do you think if can be rewritten as:

	if (true)
		for (...)
	else
		_label: /* dummy */ ;

>  
>  #define scoped_cond_guard(_name, _fail, args...) \
>  	for (CLASS(_name, scope)(args), \

With your __is_cond_ptr() can this be made to warn or error if
scoped_cond_guard() is used with a non-conditional lock/class? As that
would make no sense.

> @@ -233,14 +247,17 @@ static inline class_##_name##_t class_##_name##_constructor(void)	\
>  }
>  
>  #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...)		\
> +DEFINE_CLASS_IS_CONDITIONAL(_name, 0);					\
>  __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__)		\
>  __DEFINE_LOCK_GUARD_1(_name, _type, _lock)
>  
>  #define DEFINE_LOCK_GUARD_0(_name, _lock, _unlock, ...)			\
> +DEFINE_CLASS_IS_CONDITIONAL(_name, 0);					\
>  __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)		\
>  __DEFINE_LOCK_GUARD_0(_name, _lock)
>  
>  #define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock)		\
> +	DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1);			\
>  	EXTEND_CLASS(_name, _ext,					\
>  		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
>  		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\
> 
> base-commit: c824deb1a89755f70156b5cdaf569fca80698719
> -- 
> 2.39.3
> 

Thanks.

Dan Carpenter Oct. 1, 2024, 3:29 p.m. UTC | #3

Nice.  This would make it easier for Smatch to parse scope_guard() macros.

regards,
dan carpenter

Przemek Kitszel Oct. 2, 2024, 8:07 a.m. UTC | #4

On 10/1/24 17:09, Andy Shevchenko wrote:
> On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:
> 
> ...
> 
>> NAKed-by: Andy Shevchenko <[e-mail]>
> 
> And still NAKed.
> 
>> Signed-off-by: Przemek Kitszel <[e-mail]>
>> ---
>> Andy believes that this change is completely wrong C, the reasons
>> (that I disagree with of course, are in v1, below the commit message).
> 
> Have you retested the macro expansion for this version?

yeah, I basically work with the expanded version, still linear
expansion, just ~dozen bytes more than v1

> 
> ...
> 
>>    */
>>   
>> +
> 
> Too many blank lines.

thanks!

> 
>> +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
>> +static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
> 
> ...
> 
>> +	DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \
> 
> Here and everywhere else, boolean has values true and false.

sure, thanks

Przemek Kitszel Oct. 2, 2024, 9:55 a.m. UTC | #5

On 10/1/24 17:21, Dmitry Torokhov wrote:
> Hi Przemek,
> 
> On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:
>> int foo(struct my_drv *adapter)
>> {
>> 	scoped_guard(spinlock, &adapter->some_spinlock)
>> 		return adapter->spinlock_protected_var;
>> }
> 
> Could you change the subject to say something like:
> 
> "Adjust cond_guard() implementation to avoid potential warnings"
> 
> And then give detailed explanation in the body?

thanks, sure
(and apologies that I forgot to add any subject :F (this was just my
very first non-subject paragraph))

> 
>>
>> Current scoped_guard() implementation does not support that,
>> due to compiler complaining:
>> error: control reaches end of non-void function [-Werror=return-type]
>>
>> One could argue that for such use case it would be better to use
>> guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
>> that coding with my proposed locking style is also very pleasant, as I'm
>> doing so for a few weeks already.
> 
> I'd drop this paragraph from the patch description (and moved past "---"
> if you prefer to keep it for additional context.

I will think about that, especially given that since v2 this patch is
not only fixing "my case", but just it's regular hardening for static
analysis needs.


>> +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
> 
> This is not supposed to be used outside of cleanup.h so probably
> __DEFINE_CLASS_IS_CONDITIONAL()?

indeed

>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>> +	if (0)						\
>> +		_label: ;				\
>> +	else						\
>> +		for (CLASS(_name, scope)(args);		\
>> +		     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
>> +		     ({goto _label;}))
> 
> The "jump back" throws me a little, do you think if can be rewritten as:
> 
> 	if (true)
> 		for (...)
> 	else
> 		_label: /* dummy */ ;

user code must be glued at the end, so there must be "if (0) label:"
however I figured that you could reorder for and else:

	for (
		CLASS(...);
		__guard_ptr(...) || __is_cond_ptr(...);
		({ goto label; })
	)
		if (0)
			label:
				break;
		else
			// actual user code glued here

and this jumps forward

> 
>>   
>>   #define scoped_cond_guard(_name, _fail, args...) \
>>   	for (CLASS(_name, scope)(args), \
> 
> With your __is_cond_ptr() can this be made to warn or error if
> scoped_cond_guard() is used with a non-conditional lock/class? As that
> would make no sense.

good idea, thanks

Patch

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index a3d3e888cf1f..72dcfeb3ec13 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -151,12 +151,18 @@  static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *
  */
 
+
+#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
+static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
+
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
+	DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \
 	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
 	{ return *_T; }
 
 #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
+	DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1); \
 	EXTEND_CLASS(_name, _ext, \
 		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
 		     class_##_name##_t _T) \
@@ -167,10 +173,18 @@  static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 	CLASS(_name, __UNIQUE_ID(guard))
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
+#define __is_cond_ptr(_name) class_##_name##_is_conditional
+
+#define scoped_guard(_name, args...)	\
+	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
 
-#define scoped_guard(_name, args...)					\
-	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
+#define __scoped_guard_labeled(_label, _name, args...)	\
+	if (0)						\
+		_label: ;				\
+	else						\
+		for (CLASS(_name, scope)(args);		\
+		     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
+		     ({goto _label;}))
 
 #define scoped_cond_guard(_name, _fail, args...) \
 	for (CLASS(_name, scope)(args), \
@@ -233,14 +247,17 @@  static inline class_##_name##_t class_##_name##_constructor(void)	\
 }
 
 #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...)		\
+DEFINE_CLASS_IS_CONDITIONAL(_name, 0);					\
 __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__)		\
 __DEFINE_LOCK_GUARD_1(_name, _type, _lock)
 
 #define DEFINE_LOCK_GUARD_0(_name, _lock, _unlock, ...)			\
+DEFINE_CLASS_IS_CONDITIONAL(_name, 0);					\
 __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)		\
 __DEFINE_LOCK_GUARD_0(_name, _lock)
 
 #define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock)		\
+	DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1);			\
 	EXTEND_CLASS(_name, _ext,					\
 		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
 		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\