diff mbox series

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

Message ID 20241009114446.14873-1-przemyslaw.kitszel@intel.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] cleanup: adjust scoped_guard() to avoid potential warning | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Przemek Kitszel Oct. 9, 2024, 11:44 a.m. UTC 
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).

Big thanks to Andy Shevchenko for help on this patch, both internal and
public, ranging from whitespace/formatting, through commit message
clarifications, general improvements, ending with presenting alternative
approaches - all despite not even liking the idea.

Big thanks to Dmitry Torokhov for the idea of compile-time check for
scoped_cond_guard(), and general improvements for the patch.

CC: Dan Carpenter <dan.carpenter@linaro.org>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Andy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
PATCH v2:
drop Andy's NACK,
 (the reasons for NACK were in RFC v1; Peter backed up my idea for this
 patch in PATCH v1 discussion, and Andy withdrawn the NACK);
whitespace/formatting/style issues - Andy;
additional code comments - Dmitry.

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()
https://lore.kernel.org/netdev/20241003113906.750116-1-przemyslaw.kitszel@intel.com

RFC v2:
https://lore.kernel.org/netdev/20241001145718.8962-1-przemyslaw.kitszel@intel.com
 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 | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)


base-commit: 44badc908f2c85711cb18e45e13119c10ad3a05f

Comments

Andy Shevchenko Oct. 9, 2024, 1:21 p.m. UTC | #1 
On Wed, Oct 09, 2024 at 01:44:17PM +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).
> 
> Big thanks to Andy Shevchenko for help on this patch, both internal and
> public, ranging from whitespace/formatting, through commit message
> clarifications, general improvements, ending with presenting alternative
> approaches - all despite not even liking the idea.
> 
> Big thanks to Dmitry Torokhov for the idea of compile-time check for
> scoped_cond_guard(), and general improvements for the patch.

...

> @@ -149,14 +149,21 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>   *      similar to scoped_guard(), except it does fail when the lock
>   *      acquire fails.
>   *
> + *	Only for conditional locks.

> + *

Slipped redundant blank line.

>   */

...

> +/* helper for the scoped_guard() macro

 /*
  * This is wrong style of the comment block, it's not network
  * related code where it's acceptable. Also, respect English,
  * i.e. capitalisation and punctuation in the sentences.
  */

> + *
> + * Note that the "!__is_cond_ptr(_name)" part of the condition ensures
> + * that compiler would be sure that for unconditional locks the body of
> + * the loop could not be skipped; it is needed because the other
> + * part - "__guard_ptr(_name)(&scope)" - is too hard to deduce (even if
> + * could be proven true for unconditional locks).
> + */
David Lechner Oct. 10, 2024, 8:13 p.m. UTC | #2 
On 10/9/24 6:44 AM, 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]

I was hoping that this would allow us to do the same with
scoped_cond_guard() so that we could remove a bunch of
unreachable(); that we had to add in the IIO subsystem. But
with this patch we still get compile errors if we remove them.

Is it possible to apply the same if/goto magic to scoped_cond_guard()
to make it better too?

---

Here is a test case if that helps...

For example, I made this change:

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index e8bddfb0d07d..f1c85690af1e 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -577,7 +577,6 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 		else
 			return regmap_write(st->regmap, reg, writeval);
 	}
-	unreachable();
 }
 
 /*
@@ -824,7 +823,6 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 			return ad7380_read_direct(st, chan->scan_index,
 						  scan_type, val);
 		}
-		unreachable();
 	case IIO_CHAN_INFO_SCALE:
 		/*
 		 * According to the datasheet, the LSB size is:
@@ -933,7 +931,6 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 					FIELD_PREP(AD7380_CONFIG2_RESET,
 						   AD7380_CONFIG2_RESET_SOFT));
 		}
-		unreachable();
 	default:
 		return -EINVAL;
 	}

And then I get the following compiler errors/warnings:

/home/david/work/linux/drivers/iio/adc/ad7380.c: In function ‘ad7380_debugfs_reg_access’:
/home/david/work/linux/drivers/iio/adc/ad7380.c:580:1: error: control reaches end of non-void function [-Werror=return-type]
  580 | }
      | ^
In file included from /home/david/work/linux/include/linux/irqflags.h:17,
                 from /home/david/work/linux/arch/arm/include/asm/bitops.h:28,
                 from /home/david/work/linux/include/linux/bitops.h:68,
                 from /home/david/work/linux/drivers/iio/adc/ad7380.c:20:
/home/david/work/linux/drivers/iio/adc/ad7380.c: In function ‘ad7380_write_raw’:
/home/david/work/linux/include/linux/cleanup.h:337:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
  337 |         for (CLASS(_name, scope)(args), \
      |         ^~~
/home/david/work/linux/include/linux/iio/iio.h:689:9: note: in expansion of macro ‘scoped_cond_guard’
  689 |         scoped_cond_guard(iio_claim_direct_try, fail, iio_dev)
      |         ^~~~~~~~~~~~~~~~~
/home/david/work/linux/drivers/iio/adc/ad7380.c:910:17: note: in expansion of macro ‘iio_device_claim_direct_scoped’
  910 |                 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/david/work/linux/drivers/iio/adc/ad7380.c:934:9: note: here
  934 |         default:
      |         ^~~~~~~
/home/david/work/linux/drivers/iio/adc/ad7380.c: In function ‘ad7380_read_raw’:
/home/david/work/linux/include/linux/cleanup.h:337:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
  337 |         for (CLASS(_name, scope)(args), \
      |         ^~~
/home/david/work/linux/include/linux/iio/iio.h:689:9: note: in expansion of macro ‘scoped_cond_guard’
  689 |         scoped_cond_guard(iio_claim_direct_try, fail, iio_dev)
      |         ^~~~~~~~~~~~~~~~~
/home/david/work/linux/drivers/iio/adc/ad7380.c:822:17: note: in expansion of macro ‘iio_device_claim_direct_scoped’
  822 |                 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/david/work/linux/drivers/iio/adc/ad7380.c:826:9: note: here
  826 |         case IIO_CHAN_INFO_SCALE:
      |         ^~~~

Using compiler:

$ arm-linux-gnueabi-gcc --version
arm-linux-gnueabi-gcc (Ubuntu 13.2.0-23ubuntu4) 13.2.0
Przemek Kitszel Oct. 11, 2024, 7:25 a.m. UTC | #3 
On 10/10/24 22:13, David Lechner wrote:
> On 10/9/24 6:44 AM, 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]
> 
> I was hoping that this would allow us to do the same with
> scoped_cond_guard() so that we could remove a bunch of
> unreachable(); that we had to add in the IIO subsystem. But
> with this patch we still get compile errors if we remove them.
> 
> Is it possible to apply the same if/goto magic to scoped_cond_guard()
> to make it better too?
sure, will do

I will also combine both macros __helper at the same time to reduce
duplication
diff mbox series

Patch

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index a3d3e888cf1f..7cb733bc981e 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -149,14 +149,21 @@  static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *      similar to scoped_guard(), except it does fail when the lock
  *      acquire fails.
  *
+ *	Only for conditional locks.
+ *
  */
 
+#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 +174,33 @@  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
+
+/* helper for the scoped_guard() macro
+ *
+ * Note that the "!__is_cond_ptr(_name)" part of the condition ensures
+ * that compiler would be sure that for unconditional locks the body of
+ * the loop could not be skipped; it is needed because the other
+ * part - "__guard_ptr(_name)(&scope)" - is too hard to deduce (even if
+ * could be proven true for unconditional locks).
+ */
+#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 +259,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;	\