[PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Przemek Kitszel]

Change scoped_guard() to make reasoning about it easier for static
analysis tools (smatch, compiler diagnostics), especially to enable them
to tell if the given scoped_guard() is conditional (interruptible-locks,
try-locks) or not (like simple mutex_lock()).

Add compile-time error if scoped_cond_guard() is used for non-conditional
lock class.

Beyond easier tooling and a little shrink reported by bloat-o-meter:
add/remove: 3/2 grow/shrink: 45/55 up/down: 1573/-2069 (-496)
this patch enables developer to write code like:

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]

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 non-const 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 the
"for (...; goto label) if (0) label: break;" idiom, so it's not packed
for reuse, what makes actual macros code cleaner.

There was also a need to introduce const true/false 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).

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Dan Carpenter <dan.carpenter@linaro.org>
CC: Peter Zijlstra <peterz@infradead.org>
NAKed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
Andy believes that this change enables completely wrong C, the reasons
(that I disagree with of course, are in v1, below the commit message).

PATCH v1:
changes thanks to Dmitry Torokhov:
 better writeup in commit msg;
 "__" prefix added to internal macros;
 reorder "if (0)-else" and "for" to avoid goto jumping back;
 compile-time check for scoped_cond_guard()

RFC v2:
https://lore.kernel.org/netdev/35b3305f-d2c6-4d2b-9ac5-8bbb93873b92@stanley.mountain
 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.

RFC v1:
https://lore.kernel.org/netdev/20240926134347.19371-1-przemyslaw.kitszel@intel.com
---
 include/linux/cleanup.h | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index a3d3e888cf1f..eb97d0520202 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -151,12 +151,17 @@ 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, false); \
 	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, true); \
 	EXTEND_CLASS(_name, _ext, \
 		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
 		     class_##_name##_t _T) \
@@ -167,14 +172,25 @@ 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_labeled(_label, _name, args...)			\
+	for (CLASS(_name, scope)(args);					\
+	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
+		     ({ goto _label; }))				\
+		if (0)							\
+		_label:							\
+			break;						\
+		else
+
+#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_cond_guard(_name, _fail, args...) \
 	for (CLASS(_name, scope)(args), \
-	     *done = NULL; !done; done = (void *)1) \
+	     *done = NULL; !done; done = (void *)1 +  	\
+	     BUILD_BUG_ON_ZERO(!__is_cond_ptr(_name)))	\
 		if (!__guard_ptr(_name)(&scope)) _fail; \
 		else
 
@@ -233,14 +249,17 @@ static inline class_##_name##_t class_##_name##_constructor(void)	\
 }
 
 #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...)		\
+__DEFINE_CLASS_IS_CONDITIONAL(_name, false);				\
 __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, false);				\
 __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, true);		\
 	EXTEND_CLASS(_name, _ext,					\
 		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
 		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\

base-commit: 62a0e2fa40c5c06742b8b4997ba5095a3ec28503
-- 
2.39.3

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Markus Elfring]

…
> PATCH v1:
> changes thanks to Dmitry Torokhov:
>  better writeup in commit msg;
>  "__" prefix added to internal macros;
…

Would you get into the mood to reconsider the usage of leading underscores
any more for selected identifiers?
https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

Regards,
Markus

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Andy Shevchenko]

On Thu, Oct 03, 2024 at 02:34:57PM +0200, Markus Elfring wrote:

…

> >  "__" prefix added to internal macros;

…

> Would you get into the mood to reconsider the usage of leading underscores
> any more for selected identifiers?
> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

The mentioned URL doesn't cover the special cases like the kernels of the
operating systems or other quite low level code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Markus Elfring]

> …
>
>>>  "__" prefix added to internal macros;
>
> …
>
>> Would you get into the mood to reconsider the usage of leading underscores
>> any more for selected identifiers?
>> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
>
> The mentioned URL doesn't cover the special cases like the kernels of the
> operating systems or other quite low level code.
Can such a view be clarified further according to available information?

Regards,
Markus

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Przemek Kitszel]

On 10/3/24 18:00, Markus Elfring wrote:
>> …
>>
>>>>   "__" prefix added to internal macros;
>>
>> …
>>
>>> Would you get into the mood to reconsider the usage of leading underscores
>>> any more for selected identifiers?
>>> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
>>
>> The mentioned URL doesn't cover the special cases like the kernels of the
>> operating systems or other quite low level code.
> Can such a view be clarified further according to available information?
> 
> Regards,
> Markus

could you please update CMU SEI wiki to be relevant for kernel drivers
development, so future generations of C bureaucrats would not be fooled?

Re: cleanup: adjust scoped_guard() to avoid potential warning

[Markus Elfring]

>>>> Would you get into the mood to reconsider the usage of leading underscores
>>>> any more for selected identifiers?
>>>> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
>>>
>>> The mentioned URL doesn't cover the special cases like the kernels of the
>>> operating systems or other quite low level code.
>> Can such a view be clarified further according to available information?
…
> could you please update CMU SEI wiki to be relevant for kernel drivers
> development, so future generations of C bureaucrats would not be fooled?

How do you disagree to mentioned technical aspects?

Regards,
Markus

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Andy Shevchenko]

On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:
> Change scoped_guard() to make reasoning about it easier for static
> analysis tools (smatch, compiler diagnostics), especially to enable them
> to tell if the given scoped_guard() is conditional (interruptible-locks,
> try-locks) or not (like simple mutex_lock()).
> 
> Add compile-time error if scoped_cond_guard() is used for non-conditional
> lock class.
> 
> Beyond easier tooling and a little shrink reported by bloat-o-meter:
> add/remove: 3/2 grow/shrink: 45/55 up/down: 1573/-2069 (-496)
> this patch enables developer to write code like:
> 
> 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]
> 
> 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 non-const 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 the
> "for (...; goto label) if (0) label: break;" idiom, so it's not packed
> for reuse, what makes actual macros code cleaner.
> 
> There was also a need to introduce const true/false 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).

...

> +#define __scoped_guard_labeled(_label, _name, args...)			\
> +	for (CLASS(_name, scope)(args);					\
> +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> +		     ({ goto _label; }))				\
> +		if (0)							\
> +		_label:							\
> +			break;						\
> +		else

I believe the following will folow more the style we use in the kernel:

#define __scoped_guard_labeled(_label, _name, args...)			\
	for (CLASS(_name, scope)(args);					\
	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
		     ({ goto _label; }))				\
		if (0) {						\
_label:									\
			break;						\
		} else

...

> -	     *done = NULL; !done; done = (void *)1) \
> +	     *done = NULL; !done; done = (void *)1 +  	\

You have TABs/spaces mix in this line now.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Andy Shevchenko]

On Thu, Oct 03, 2024 at 03:43:17PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:

...

> > +#define __scoped_guard_labeled(_label, _name, args...)			\
> > +	for (CLASS(_name, scope)(args);					\
> > +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > +		     ({ goto _label; }))				\
> > +		if (0)							\
> > +		_label:							\
> > +			break;						\
> > +		else
> 
> I believe the following will folow more the style we use in the kernel:
> 
> #define __scoped_guard_labeled(_label, _name, args...)			\
> 	for (CLASS(_name, scope)(args);					\
> 	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> 		     ({ goto _label; }))				\
> 		if (0) {						\
> _label:									\
> 			break;						\
> 		} else
> 
> ...
> 
> > -	     *done = NULL; !done; done = (void *)1) \
> > +	     *done = NULL; !done; done = (void *)1 +  	\
> 
> You have TABs/spaces mix in this line now.

And FWIW:
1) still NAKed;
2) interestingly you haven't mentioned that meanwhile I also helped you to
improve this version of the patch. Is it because I NAKed it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Przemek Kitszel]

On 10/3/24 14:46, Andy Shevchenko wrote:
> On Thu, Oct 03, 2024 at 03:43:17PM +0300, Andy Shevchenko wrote:
>> On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:
> 
> ...
> 
>>> +#define __scoped_guard_labeled(_label, _name, args...)			\
>>> +	for (CLASS(_name, scope)(args);					\
>>> +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
>>> +		     ({ goto _label; }))				\
>>> +		if (0)							\
>>> +		_label:							\
>>> +			break;						\
>>> +		else
>>
>> I believe the following will folow more the style we use in the kernel:
>>
>> #define __scoped_guard_labeled(_label, _name, args...)			\
>> 	for (CLASS(_name, scope)(args);					\
>> 	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
>> 		     ({ goto _label; }))				\
>> 		if (0) {						\
>> _label:									\
>> 			break;						\
>> 		} else
>>
>> ...
>>
>>> -	     *done = NULL; !done; done = (void *)1) \
>>> +	     *done = NULL; !done; done = (void *)1 +  	\
>>
>> You have TABs/spaces mix in this line now.
> 
> And FWIW:
> 1) still NAKed;

I guess you are now opposed to just part of the patch, should I add:
# for enabling "scoped_guard(...) return ...;" shortcut
or keep it unqualified?

> 2) interestingly you haven't mentioned that meanwhile I also helped you to
> improve this version of the patch. Is it because I NAKed it?
> 

0/1 vs false/true and whitespaces, especially for RFC, are not big deal

anyway, I will reword v2 to give you credits for your valuable
contribution during internal review :)

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Andy Shevchenko]

On Thu, Oct 03, 2024 at 03:38:45PM +0200, Przemek Kitszel wrote:
> On 10/3/24 14:46, Andy Shevchenko wrote:
> > On Thu, Oct 03, 2024 at 03:43:17PM +0300, Andy Shevchenko wrote:
> > > On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:

...

> > > > +#define __scoped_guard_labeled(_label, _name, args...)			\
> > > > +	for (CLASS(_name, scope)(args);					\
> > > > +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > > > +		     ({ goto _label; }))				\
> > > > +		if (0)							\
> > > > +		_label:							\
> > > > +			break;						\
> > > > +		else
> > > 
> > > I believe the following will folow more the style we use in the kernel:
> > > 
> > > #define __scoped_guard_labeled(_label, _name, args...)			\
> > > 	for (CLASS(_name, scope)(args);					\
> > > 	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > > 		     ({ goto _label; }))				\
> > > 		if (0) {						\
> > > _label:									\
> > > 			break;						\
> > > 		} else
> > > 
> > > ...
> > > 
> > > > -	     *done = NULL; !done; done = (void *)1) \
> > > > +	     *done = NULL; !done; done = (void *)1 +  	\
> > > 
> > > You have TABs/spaces mix in this line now.
> > 
> > And FWIW:
> > 1) still NAKed;
> 
> I guess you are now opposed to just part of the patch, should I add:
> # for enabling "scoped_guard(...) return ...;" shortcut
> or keep it unqualified?

As you put a reference to the whole list the detailed elaboration
is not needed.

> > 2) interestingly you haven't mentioned that meanwhile I also helped you to
> > improve this version of the patch. Is it because I NAKed it?
> 
> 0/1 vs false/true and whitespaces, especially for RFC, are not big deal

+ the above now.

I assume every contribution should be credited, no?
Otherwise it sounds like a bit of disrespect.

> anyway, I will reword v2 to give you credits for your valuable
> contribution during internal review :)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Peter Zijlstra]

On Thu, Oct 03, 2024 at 03:46:24PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 03, 2024 at 03:43:17PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:
> 
> ...
> 
> > > +#define __scoped_guard_labeled(_label, _name, args...)			\
> > > +	for (CLASS(_name, scope)(args);					\
> > > +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > > +		     ({ goto _label; }))				\
> > > +		if (0)							\
> > > +		_label:							\
> > > +			break;						\
> > > +		else
> > 
> > I believe the following will folow more the style we use in the kernel:
> > 
> > #define __scoped_guard_labeled(_label, _name, args...)			\
> > 	for (CLASS(_name, scope)(args);					\
> > 	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > 		     ({ goto _label; }))				\
> > 		if (0) {						\
> > _label:									\
> > 			break;						\
> > 		} else
> > 

Yeah, needs braces like that. I'm not super opposed to this, however, 

> And FWIW:
> 1) still NAKed;

I would really like to understand why you don't like this; care to
elaborate Andy?

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Andy Shevchenko]

On Thu, Oct 03, 2024 at 04:12:21PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 03, 2024 at 03:46:24PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 03, 2024 at 03:43:17PM +0300, Andy Shevchenko wrote:
> > > On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:

...

> > > > +#define __scoped_guard_labeled(_label, _name, args...)			\
> > > > +	for (CLASS(_name, scope)(args);					\
> > > > +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > > > +		     ({ goto _label; }))				\
> > > > +		if (0)							\
> > > > +		_label:							\
> > > > +			break;						\
> > > > +		else
> > > 
> > > I believe the following will folow more the style we use in the kernel:
> > > 
> > > #define __scoped_guard_labeled(_label, _name, args...)			\
> > > 	for (CLASS(_name, scope)(args);					\
> > > 	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> > > 		     ({ goto _label; }))				\
> > > 		if (0) {						\
> > > _label:									\
> > > 			break;						\
> > > 		} else
> > > 
> 
> Yeah, needs braces like that. I'm not super opposed to this, however, 
> 
> > And FWIW:
> > 1) still NAKed;
> 
> I would really like to understand why you don't like this; care to
> elaborate Andy?

To me the idea of

int my_foo(...)
{
	NOT_my_foo_macro(...)
		return X;
}

is counter intuitive from C programming. Without knowing the magic behind the
scenes of NOT_my_foo_macro() I would eager to ask for adding a dead code like

int my_foo(...)
{
	NOT_my_foo_macro(...)
		return X;
	return 0;
}

What I would agree on is

int my_foo(...)
{
	return NOT_my_foo_macro(..., X);
}

Or just using guard()().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Peter Zijlstra]

On Thu, Oct 03, 2024 at 08:51:46PM +0300, Andy Shevchenko wrote:

> > I would really like to understand why you don't like this; care to
> > elaborate Andy?
> 
> To me the idea of
> 
> int my_foo(...)
> {
> 	NOT_my_foo_macro(...)
> 		return X;
> }
> 
> is counter intuitive from C programming. Without knowing the magic behind the
> scenes of NOT_my_foo_macro() I would eager to ask for adding a dead code like
> 
> int my_foo(...)
> {
> 	NOT_my_foo_macro(...)
> 		return X;
> 	return 0;
> }

Well, this is kernel coding, we don't really do (std) C anymore, and
using *anything* without knowing the magic behind it is asking for fail.

Also, something like:

int my_foo()
{
	for (;;)
		return X;
}

or

int my_foo()
{
	do {
		return X;
	} while (0);
}

is perfectly valid C that no compiler should be complaining about. Yes
its a wee bit daft, but if you want to write it, that's fine.

The point being that the compiler can determine there is no path not
hitting that return.

Apparently the current for loop is defeating the compiler, I see no
reason not to change it in such a way that the compiler is able to
determine wtf happens -- that can only help.

> What I would agree on is
> 
> int my_foo(...)
> {
> 	return NOT_my_foo_macro(..., X);
> }

That just really won't work with things as they are ofcourse.

> Or just using guard()().

That's always an option. You don't *have* to use the -- to you -- weird
form.

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Andy Shevchenko]

On Fri, Oct 04, 2024 at 11:33:08AM +0200, Peter Zijlstra wrote:
> On Thu, Oct 03, 2024 at 08:51:46PM +0300, Andy Shevchenko wrote:
> > > I would really like to understand why you don't like this; care to
> > > elaborate Andy?
> > 
> > To me the idea of
> > 
> > int my_foo(...)
> > {
> > 	NOT_my_foo_macro(...)
> > 		return X;
> > }
> > 
> > is counter intuitive from C programming. Without knowing the magic behind the
> > scenes of NOT_my_foo_macro() I would eager to ask for adding a dead code like
> > 
> > int my_foo(...)
> > {
> > 	NOT_my_foo_macro(...)
> > 		return X;
> > 	return 0;
> > }
> 
> Well, this is kernel coding, we don't really do (std) C anymore, and
> using *anything* without knowing the magic behind it is asking for fail.

True in many cases, mostly for macros themselves, but in the functions
I would prefer to stay away from magic as possible.

> Also, something like:
> 
> int my_foo()
> {
> 	for (;;)
> 		return X;
> }
> 
> or
> 
> int my_foo()
> {
> 	do {
> 		return X;
> 	} while (0);
> }
> 
> is perfectly valid C that no compiler should be complaining about. Yes
> its a wee bit daft, but if you want to write it, that's fine.

Yes, the difference is that it's not hidden from the reader.
The code behind the macro is magic for the reader by default.

> The point being that the compiler can determine there is no path not
> hitting that return.
> 
> Apparently the current for loop is defeating the compiler, I see no
> reason not to change it in such a way that the compiler is able to
> determine wtf happens -- that can only help.
> 
> > What I would agree on is
> > 
> > int my_foo(...)
> > {
> > 	return NOT_my_foo_macro(..., X);
> > }
> 
> That just really won't work with things as they are ofcourse.
> 
> > Or just using guard()().

Okay, thanks for sharing your view on this. Since you are the author
of the original code and seems fine with a change, I can't help myself
from withdrawing my NACK. OTOH, I am not going to give an Ack either.

> That's always an option. You don't *have* to use the -- to you -- weird
> form.

Yes, I am not convinced that using scoped_guard() (or any other macro)
in a described way is okay. Definitely, *I am* not going to do a such
until I understand the real benefit of it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Dmitry Torokhov]

Hi Przemek,

On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:
> @@ -167,14 +172,25 @@ 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_labeled(_label, _name, args...)			\
> +	for (CLASS(_name, scope)(args);					\
> +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\

It would be great if you added the comment that "!__is_cond_ptr(_name)"
condition ensures that the compiler does not believe that it is possible
to skip the loop body because it does not realize that
"__guard_ptr(_name)(&scope)" will never return 0 for unconditional
locks. You have the explanation in the patch description, but I think it
is worth to reiterate here as well.

> +		     ({ goto _label; }))				\
> +		if (0)							\
> +		_label:							\
> +			break;						\
> +		else
> +

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

[Przemek Kitszel]

On 10/3/24 15:00, Dmitry Torokhov wrote:
> Hi Przemek,
> 
> On Thu, Oct 03, 2024 at 01:39:06PM +0200, Przemek Kitszel wrote:
>> @@ -167,14 +172,25 @@ 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_labeled(_label, _name, args...)			\
>> +	for (CLASS(_name, scope)(args);					\
>> +	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
> 
> It would be great if you added the comment that "!__is_cond_ptr(_name)"
> condition ensures that the compiler does not believe that it is possible
> to skip the loop body because it does not realize that
> "__guard_ptr(_name)(&scope)" will never return 0 for unconditional
> locks. You have the explanation in the patch description, but I think it
> is worth to reiterate here as well.

thanks, I will add an in-code comment; sometimes it's easy to loose
outside perspective if you spend too much time on one piece

> 
>> +		     ({ goto _label; }))				\
>> +		if (0)							\
>> +		_label:							\
>> +			break;						\
>> +		else
>> +
> 
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Thanks.
>