Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros

[Paul Cercueil]

Hi Ard,

Le mer. 9 déc. 2020 à 10:38, Ard Biesheuvel <a...@kernel.org> a 
écrit :
On Tue, 8 Dec 2020 at 17:49, Paul Cercueil <p...@crapouillou.net> 
wrote:

 Introduce a new header <linux/if_enabled.h>, that brings two new 
macros:
 IF_ENABLED_OR_ELSE() and IF_ENABLED().

 IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO 
is set
 to 'y' or 'm', (b) otherwise. It is used internally to define the
 IF_ENABLED() macro. The (a) and (b) arguments must be of the same 
type.

 IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set 
to 'y'
 or 'm', NULL otherwise. The (ptr) argument must be a pointer.

 The IF_ENABLED() macro can be very useful to help GCC drop dead 
code.

 For instance, consider the following:

     #ifdef CONFIG_FOO_SUSPEND
     static int foo_suspend(struct device *dev)
     {
        ...
     }
     #endif

     static struct pm_ops foo_ops = {
         #ifdef CONFIG_FOO_SUSPEND
         .suspend = foo_suspend,
         #endif
     };

 While this works, the foo_suspend() macro is compiled conditionally,
 only when CONFIG_FOO_SUSPEND is set. This is problematic, as there 
could
 be a build bug in this function, we wouldn't have a way to know 
unless
 the config option is set.

 An alternative is to declare foo_suspend() always, but mark it as 
maybe
 unused:

     static int __maybe_unused foo_suspend(struct device *dev)
     {
        ...
     }

     static struct pm_ops foo_ops = {
         #ifdef CONFIG_FOO_SUSPEND
         .suspend = foo_suspend,
         #endif
     };

 Again, this works, but the __maybe_unused attribute is required to
 instruct the compiler that the function may not be referenced 
anywhere,
 and is safe to remove without making a fuss about it. This makes the
 programmer responsible for tagging the functions that can be
 garbage-collected.

 With this patch, it is now possible to write the following:

     static int foo_suspend(struct device *dev)
     {
        ...
     }

     static struct pm_ops foo_ops = {
         .suspend = IF_ENABLED(CONFIG_FOO_SUSPEND, foo_suspend),
     };

 The foo_suspend() function will now be automatically dropped by the
 compiler, and it does not require any specific attribute.

 Signed-off-by: Paul Cercueil <p...@crapouillou.net>

Hi Paul,

I understand the issue you are trying to address here, but please note
that there are many cases where the struct member in question will not
even be declared if the associated CONFIG option is not set, in which
case this will cause a compile error.

Of course. This is for the case where the field is always present. For 
instance, (struct device_driver *)->pm is always present, independently 
of CONFIG_PM.

Cheers,
-Paul


 ---
  include/linux/if_enabled.h | 22 ++++++++++++++++++++++
  1 file changed, 22 insertions(+)
  create mode 100644 include/linux/if_enabled.h

 diff --git a/include/linux/if_enabled.h b/include/linux/if_enabled.h
 new file mode 100644
 index 000000000000..8189d1402340
 --- /dev/null
 +++ b/include/linux/if_enabled.h
 @@ -0,0 +1,22 @@
 +/* SPDX-License-Identifier: GPL-2.0 */
 +#ifndef __LINUX_IF_ENABLED_H
 +#define __LINUX_IF_ENABLED_H
 +
 +#include <linux/build_bug.h>
 +#include <linux/compiler_types.h>
 +#include <linux/kconfig.h>
 +
 +/*
 + * IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if 
CONFIG_FOO is set
 + * to 'y' or 'm', (b) otherwise.
 + */
 +#define IF_ENABLED_OR_ELSE(option, a, b) \
 +       (BUILD_BUG_ON_ZERO(__same_type((a), (b))) || 
IS_ENABLED(option) ? (a) : (b))
 +
 +/*
 + * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is 
set to 'y'
 + * or 'm', NULL otherwise.
 + */
 +#define IF_ENABLED(option, ptr) IF_ENABLED_OR_ELSE(option, ptr, 
NULL)
 +
 +#endif /* __LINUX_IF_ENABLED_H */
 --
 2.29.2


 _______________________________________________
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel