[PATCH 0/1] clean up the code related to ASSERT()

[PATCH 0/1] clean up the code related to ASSERT()

[Chunguang Xu]

The kernel has not yet defined ASSERT(). Indeed, BUG() and WARN() are very
clear and can cover most application scenarios. However, some applications
require more debugging information and similar behavior to assert(), which
cannot be directly provided by BUG() and WARN().

Therefore, many modules independently implement ASSERT(), and most of them
are similar, but slightly different, such as:

 #define ASSERT(expr) \
         if(!(expr)) { \
                 printk( "\n" __FILE__ ":%d: Assertion " #expr " failed!\n",__LINE__); \
                 panic(#expr); \
         }

 #define ASSERT(x)                                                       \
 do {                                                                    \
         if (!(x)) {                                                     \
                 printk(KERN_EMERG "assertion failed %s: %d: %s\n",      \
                        __FILE__, __LINE__, #x);                         \
                 BUG();                                                  \
         }                                                               \
 } while (0)

Some implementations are not optimal for instruction prediction, such as
missing unlikely():

 #define assert(expr) \
         if(!(expr)) { \
         printk( "Assertion failed! %s,%s,%s,line=%d\n",\
         #expr,__FILE__,__func__,__LINE__); \
         BUG(); \
         }

Some implementations have too little log content information, such as:

 #define ASSERT(X)                                               \
 do {                                                            \
        if (unlikely(!(X))) {                                   \
                printk(KERN_ERR "\n");                          \
                printk(KERN_ERR "XXX: Assertion failed\n");     \
                BUG();                                          \
        }                                                       \
 } while(0)

As we have seen, This makes the code redundant and inconvenient to
troubleshoot the system. Therefore, perhaps we need to define two
wrappers for BUG() and WARN_ON(), provide the implementation of ASSERT(),
simplifyy the code and facilitate problem analysis .

Maybe I missed some information, but I think there is a need to clean
up the code, maybe in other ways, and more discussion is needed here.
If this approach is reasonable, I will clean up these codes later and
issue related patches.
-- 
1.8.3.1

[PATCH 1/1] include/asm-generic/bug.h: add ASSERT_FAIL() and ASSERT_WARN() wrapper

[Chunguang Xu]

The kernel has not yet defined ASSERT(). Indeed, BUG() and WARN() are very
clear and can cover most application scenarios. However, some applications
require more debugging information and similar behavior to assert(), which
cannot be directly provided by BUG() and WARN().

Therefore, many modules independently implement ASSERT(), and most of them
are similar, but slightly different. This makes the code redundant and
inconvenient to troubleshoot the system. Therefore, perhaps we need to
define two wrappers for BUG() and WARN(), provide the implementation of
ASSERT(), simplify the code and facilitate problem analysis.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 include/asm-generic/bug.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 18b0f4e..28f8c27 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -174,6 +174,31 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 	unlikely(__ret_warn_once);				\
 })
 
+/*
+ * ASSERT_FAIL() and ASSERT_WARN() can be used to check whether some
+ * conditions have failed. We generally use ASSERT_FAIL() to check
+ * critical conditions, and other use ASSERT_WARN().
+ */
+#ifndef ASSERT_FAIL
+#define ASSERT_FAIL(condition) do {					\
+	if (unlikely(!(condition))) {					\
+		pr_emerg("Assertion failed: %s, file: %s, line: %d\n",	\
+			  #condition, __FILE__, __LINE__);		\
+		BUG();							\
+	}								\
+} while (0)
+#endif
+
+#ifndef ASSERT_WARN
+#define ASSERT_WARN(condition) do {					\
+	if (unlikely(!(condition))) {					\
+		pr_warn("Assertion failed: %s, file: %s, line: %d\n",	\
+			 #condition, __FILE__, __LINE__);		\
+		WARN_ON(1);						\
+	}								\
+} while (0)
+#endif
+
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
 #define BUG() do {} while (1)
@@ -203,6 +228,14 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 #define WARN_TAINT(condition, taint, format...) WARN(condition, format)
 #define WARN_TAINT_ONCE(condition, taint, format...) WARN(condition, format)
 
+#ifndef ASSERT_FAIL
+#define ASSERT_FAIL(condition) do { } while (0)
+#endif
+
+#ifndef ASSERT_WARN
+#define ASSERT_WARN(condition) do { } while (0)
+#endif
+
 #endif
 
 /*
-- 
1.8.3.1

Re: [PATCH 1/1] include/asm-generic/bug.h: add ASSERT_FAIL() and ASSERT_WARN() wrapper

[Mike Rapoport]

On Thu, Aug 27, 2020 at 07:04:53AM +0800, Chunguang Xu wrote:
> The kernel has not yet defined ASSERT(). Indeed, BUG() and WARN() are very
> clear and can cover most application scenarios. However, some applications
> require more debugging information and similar behavior to assert(), which
> cannot be directly provided by BUG() and WARN().
> 
> Therefore, many modules independently implement ASSERT(), and most of them
> are similar, but slightly different. This makes the code redundant and
> inconvenient to troubleshoot the system. Therefore, perhaps we need to
> define two wrappers for BUG() and WARN(), provide the implementation of
> ASSERT(), simplify the code and facilitate problem analysis.
> 
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>
> ---
>  include/asm-generic/bug.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

It would be useful to convert at least one occurrence of the existing
ASSERT()s to the new implementation in the same patch series.

> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 18b0f4e..28f8c27 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -174,6 +174,31 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
>  	unlikely(__ret_warn_once);				\
>  })
>  
> +/*
> + * ASSERT_FAIL() and ASSERT_WARN() can be used to check whether some
> + * conditions have failed. We generally use ASSERT_FAIL() to check
> + * critical conditions, and other use ASSERT_WARN().
> + */
> +#ifndef ASSERT_FAIL
> +#define ASSERT_FAIL(condition) do {					\
> +	if (unlikely(!(condition))) {					\
> +		pr_emerg("Assertion failed: %s, file: %s, line: %d\n",	\
> +			  #condition, __FILE__, __LINE__);		\
> +		BUG();							\
> +	}								\
> +} while (0)
> +#endif
> +
> +#ifndef ASSERT_WARN
> +#define ASSERT_WARN(condition) do {					\
> +	if (unlikely(!(condition))) {					\
> +		pr_warn("Assertion failed: %s, file: %s, line: %d\n",	\
> +			 #condition, __FILE__, __LINE__);		\
> +		WARN_ON(1);						\
> +	}								\
> +} while (0)
> +#endif
> +
>  #else /* !CONFIG_BUG */
>  #ifndef HAVE_ARCH_BUG
>  #define BUG() do {} while (1)
> @@ -203,6 +228,14 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
>  #define WARN_TAINT(condition, taint, format...) WARN(condition, format)
>  #define WARN_TAINT_ONCE(condition, taint, format...) WARN(condition, format)
>  
> +#ifndef ASSERT_FAIL
> +#define ASSERT_FAIL(condition) do { } while (0)
> +#endif
> +
> +#ifndef ASSERT_WARN
> +#define ASSERT_WARN(condition) do { } while (0)
> +#endif
> +
>  #endif
>  
>  /*
> -- 
> 1.8.3.1
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 0/1] clean up the code related to ASSERT()

[Arnd Bergmann]

On Thu, Aug 27, 2020 at 1:04 AM Chunguang Xu <brookxu.cn@gmail.com> wrote:
>
> The kernel has not yet defined ASSERT(). Indeed, BUG() and WARN() are very
> clear and can cover most application scenarios. However, some applications
> require more debugging information and similar behavior to assert(), which
> cannot be directly provided by BUG() and WARN().
>
> Therefore, many modules independently implement ASSERT(), and most of them
> are similar, but slightly different, such as:
>
>  #define ASSERT(expr) \
>          if(!(expr)) { \
>                  printk( "\n" __FILE__ ":%d: Assertion " #expr " failed!\n",__LINE__); \
>                  panic(#expr); \
>          }
>
>  #define ASSERT(x)                                                       \
>  do {                                                                    \
>          if (!(x)) {                                                     \
>                  printk(KERN_EMERG "assertion failed %s: %d: %s\n",      \
>                         __FILE__, __LINE__, #x);                         \
>                  BUG();                                                  \


Generally, I don't think the extra printk() here helps much, as BUG() and
panic() both provide the source location already.

> Some implementations are not optimal for instruction prediction, such as
> missing unlikely():
>
>  #define assert(expr) \
>          if(!(expr)) { \
>          printk( "Assertion failed! %s,%s,%s,line=%d\n",\
>          #expr,__FILE__,__func__,__LINE__); \
>          BUG(); \
>          }
>
> Some implementations have too little log content information, such as:
>
>  #define ASSERT(X)                                               \
>  do {                                                            \
>         if (unlikely(!(X))) {                                   \
>                 printk(KERN_ERR "\n");                          \
>                 printk(KERN_ERR "XXX: Assertion failed\n");     \
>                 BUG();                                          \
>         }                                                       \
>  } while(0)
>
> As we have seen, This makes the code redundant and inconvenient to
> troubleshoot the system. Therefore, perhaps we need to define two
> wrappers for BUG() and WARN_ON(), provide the implementation of ASSERT(),
> simplifyy the code and facilitate problem analysis .
>
> Maybe I missed some information, but I think there is a need to clean
> up the code, maybe in other ways, and more discussion is needed here.
> If this approach is reasonable, I will clean up these codes later and
> issue related patches.

In general, I'd argue that any direct usage of macros like these should
just use BUG(). However it seems that the ones you are replacing
tend to make it conditional on either the 'DEBUG' macro, on a Kconfig
symbol or on some other preprocessor conditional. My feeling is that
if there is some cleanup, maybe we should instead be adding a variant
of BUG_ON() that takes both a preprocessor token (like IS_ENABLED()
does) and a condition.

    Arnd