| Message ID | 20201208164821.2686082-1-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros | expand |
On 12/8/20 8:48 AM, Paul Cercueil wrote: > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> Hi Paul, Why not just add these 2 new macros to <linux/kconfig.h> ? Maybe you don't want to add the other 2 headers there also? > --- > 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 */ > thanks.
Hi Randy, Le mar. 8 déc. 2020 à 9:39, Randy Dunlap <rdunlap@infradead.org> a écrit : > On 12/8/20 8:48 AM, Paul Cercueil wrote: >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > Hi Paul, > > Why not just add these 2 new macros to <linux/kconfig.h> ? > > Maybe you don't want to add the other 2 headers there also? That means including <linux/compiler.h> in <linux/kconfig.h>, which causes build failures: LD vmlinux SORTTAB vmlinux SYSMAP System.map AS arch/mips/boot/compressed/head.o CC arch/mips/boot/compressed/decompress.o CC arch/mips/boot/compressed/string.o CC arch/mips/boot/compressed/dummy.o OBJCOPY arch/mips/boot/compressed/vmlinux.bin HOSTCC arch/mips/boot/compressed/calc_vmlinuz_load_addr GZIP arch/mips/boot/compressed/vmlinux.bin.z In file included from ./include/linux/kcsan-checks.h:7, from ./include/asm-generic/rwonce.h:27, from ./arch/mips/include/generated/asm/rwonce.h:1, from ./include/linux/compiler.h:246, from ././include/linux/kconfig.h:8, from <command-line>:32: /include/linux/compiler_attributes.h:64: warning: "__always_inline" redefined 64 | #define __always_inline inline __attribute__((__always_inline__)) | In file included from ./include/linux/stddef.h:5, from ./include/uapi/linux/posix_types.h:5, from ./include/uapi/linux/types.h:14, from ./include/linux/types.h:6, from ./include/linux/kasan-checks.h:5, from ./include/asm-generic/rwonce.h:26, from ./arch/mips/include/generated/asm/rwonce.h:1, from ./include/linux/compiler.h:246, from ././include/linux/kconfig.h:8, from <command-line>:32: /include/uapi/linux/stddef.h:5: note: this is the location of the previous definition 5 | #define __always_inline inline | In file included from ./arch/mips/include/generated/asm/rwonce.h:1, from ./include/linux/compiler.h:246, from ././include/linux/kconfig.h:8, from <command-line>:32: /include/asm-generic/rwonce.h:64:31: error: expected ‘;’ before ‘unsigned’ 64 | static __no_sanitize_or_inline | ^ | ; 65 | unsigned long __read_once_word_nocheck(const void *addr) | ~~~~~~~~ /include/asm-generic/rwonce.h:65:15: warning: no previous prototype for ‘__read_once_word_nocheck’ [-Wmissing-prototypes] 65 | unsigned long __read_once_word_nocheck(const void *addr) | ^~~~~~~~~~~~~~~~~~~~~~~~ /include/asm-generic/rwonce.h:82:28: error: expected ‘;’ before ‘unsigned’ 82 | static __no_kasan_or_inline | ^ | ; 83 | unsigned long read_word_at_a_time(const void *addr) | ~~~~~~~~ /include/asm-generic/rwonce.h:83:15: warning: no previous prototype for ‘read_word_at_a_time’ [-Wmissing-prototypes] 83 | unsigned long read_word_at_a_time(const void *addr) | ^~~~~~~~~~~~~~~~~~~ That's why I opted for a new header. 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 */ >> > > > thanks. > -- > ~Randy >
On Tue, Dec 8, 2020 at 5:48 PM Paul Cercueil <paul@crapouillou.net> wrote: > Introduce a new header <linux/if_enabled.h>, that brings two new macros: > IF_ENABLED_OR_ELSE() and IF_ENABLED(). I understand what the patch is trying to do, but when we already have IS_ENABLED() in <linux/kconfig.h> this syntax becomes a big cognitive confusion for the mind. At least the commit needs to explain why it doesn't work to use IS_ENABLED() instead so that this is needed. Certainly the build failures must be possible to solve so that this can live with the sibling IS_ENABLED() inside <linux/kconfig.h>, it can't be too hard. Yours, Linus Walleij
On Tue, 8 Dec 2020 at 17:49, Paul Cercueil <paul@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 <paul@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. > --- > 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-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Ard, Le mer. 9 déc. 2020 à 10:38, Ard Biesheuvel <ardb@kernel.org> a écrit : > On Tue, 8 Dec 2020 at 17:49, Paul Cercueil <paul@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 <paul@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-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Linus, Le mer. 9 déc. 2020 à 9:59, Linus Walleij <linus.walleij@linaro.org> a écrit : > On Tue, Dec 8, 2020 at 5:48 PM Paul Cercueil <paul@crapouillou.net> > wrote: > >> Introduce a new header <linux/if_enabled.h>, that brings two new >> macros: >> IF_ENABLED_OR_ELSE() and IF_ENABLED(). > > I understand what the patch is trying to do, but when we already have > IS_ENABLED() in <linux/kconfig.h> this syntax becomes a big cognitive > confusion for the mind. > > At least the commit needs to explain why it doesn't work to use > IS_ENABLED() instead so that this is needed. You can use IS_ENABLED(). Then you'd write: field = IS_ENABLED(CONFIG_FOO) ? &my_ptr : NULL, the IF_ENABLED() macro makes it a bit cleaner by allowing you to write: field = IF_ENABLED(CONFIG_FOO, &my_ptr), Cheers, -Paul > Certainly the build failures must be possible to solve so that this > can live with the sibling IS_ENABLED() inside <linux/kconfig.h>, > it can't be too hard. > > Yours, > Linus Walleij
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 */
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 <paul@crapouillou.net> --- include/linux/if_enabled.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 include/linux/if_enabled.h