| Message ID | 1476616078-32252-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Accepted |
| Commit | c0a0aba8e478229b2f0956918542152fbad3f794 |
| Headers | show |
Hi Peter. 2016-10-17 3:44 GMT+09:00 <hpa@zytor.com>: > > How is __is_defined() different from defined()? defined() can be used only in pre-processor's #if context, like #if defined(FOO) ... #endif __is_defined() can be used more flexibly. -- Best Regards Masahiro Yamada
Masahiro, A few comments regarding, I guess, future work. On Sun, 2016-10-16 at 20:07 +0900, Masahiro Yamada wrote: > The use of config_enabled() is ambiguous. For config options, > IS_ENABLED(), IS_REACHABLE(), etc. will make intention clearer. > Sometimes config_enabled() has been used for non-config options > because it is useful to check whether the given symbol is defined > or not. > > I have been tackling on deprecating config_enabled(), and now is the > time to finish this work. > > Some new users have appeared for v4.9-rc1, but it is trivial to > replace them: > > - arch/x86/mm/kaslr.c > replace config_enabled() with IS_ENABLED() because > CONFIG_X86_ESPFIX64 and CONFIG_EFI are boolean. > > - include/asm-generic/export.h > replace config_enabled() with __is_defined(). > > Then, config_enabled() can be removed now. > > Going forward, please use IS_ENABLED(), IS_REACHABLE(), etc. for > config options, and __is_defined() for non-config symbols. There are about a dozen instances of IS_ENABLED() that target something other than a kconfig macros. Are you planning to convert those to __is_defined() too? > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- a/include/linux/kconfig.h > +++ b/include/linux/kconfig.h > @@ -31,7 +31,6 @@ > * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when > * the last step cherry picks the 2nd arg, we get a zero. > */ > -#define config_enabled(cfg) ___is_defined(cfg) Is there a reason to keep using the double underscore prefix? > #define __is_defined(x) ___is_defined(x) > #define ___is_defined(val) ____is_defined(__ARG_PLACEHOLDER_##val) > #define ____is_defined(arg1_or_junk) __take_second_arg(arg1_or_junk 1, 0) __is_defined() is now meant to be used generally, and not just on kconfig macros. Can it be moved into another header? Paul Bolle
Hi Paul, 2016-10-20 19:04 GMT+09:00 Paul Bolle <pebolle@tiscali.nl>: > Masahiro, > > A few comments regarding, I guess, future work. > > On Sun, 2016-10-16 at 20:07 +0900, Masahiro Yamada wrote: >> The use of config_enabled() is ambiguous. For config options, >> IS_ENABLED(), IS_REACHABLE(), etc. will make intention clearer. >> Sometimes config_enabled() has been used for non-config options >> because it is useful to check whether the given symbol is defined >> or not. >> >> I have been tackling on deprecating config_enabled(), and now is the >> time to finish this work. >> >> Some new users have appeared for v4.9-rc1, but it is trivial to >> replace them: >> >> - arch/x86/mm/kaslr.c >> replace config_enabled() with IS_ENABLED() because >> CONFIG_X86_ESPFIX64 and CONFIG_EFI are boolean. >> >> - include/asm-generic/export.h >> replace config_enabled() with __is_defined(). >> >> Then, config_enabled() can be removed now. >> >> Going forward, please use IS_ENABLED(), IS_REACHABLE(), etc. for >> config options, and __is_defined() for non-config symbols. > > There are about a dozen instances of IS_ENABLED() that target something > other than a kconfig macros. Are you planning to convert those to > __is_defined() too? I did not notice that, but looks like there are some to be checked. $ git grep '[^A-Za-z0-9_]IS_ENABLED([^C]' arch/arc/include/asm/setup.h:#define IS_USED_CFG(cfg) IS_USED_RUN(IS_ENABLED(cfg)) arch/arm64/include/asm/alternative.h: __ALTERNATIVE_CFG(oldinstr, newinstr, feature, IS_ENABLED(cfg)) arch/arm64/include/asm/alternative.h: alternative_insn insn1, insn2, cap, IS_ENABLED(cfg) drivers/crypto/sahara.c: if (!IS_ENABLED(DEBUG)) drivers/crypto/sahara.c: if (!IS_ENABLED(DEBUG)) drivers/crypto/sahara.c: if (!IS_ENABLED(DEBUG)) drivers/gpu/drm/exynos/exynos_drm_drv.c:#define DRV_PTR(drv, cond) (IS_ENABLED(cond) ? &drv : NULL) include/asm-generic/vmlinux.lds.h:#define OF_TABLE(cfg, name) __OF_TABLE(IS_ENABLED(cfg), name) include/linux/init.h: int var = IS_ENABLED(config); \ include/linux/kconfig.h: * This is similar to IS_ENABLED(), but returns false when invoked from include/linux/kconfig.h:#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option)) kernel/locking/locktorture.c:int torture_runnable = IS_ENABLED(MODULE); kernel/rcu/rcuperf.c:static int perf_runnable = IS_ENABLED(MODULE); kernel/rcu/rcutorture.c:static int torture_runnable = IS_ENABLED(MODULE); scripts/checkpatch.pl: "Prefer IS_ENABLED(<FOO>) to CONFIG_<FOO> || CONFIG_<FOO>_MODULE\n" scripts/checkpatch.pl: $fixed[$fixlinenr] = "\+#if IS_ENABLED($config)"; >> --- a/include/linux/kconfig.h >> +++ b/include/linux/kconfig.h >> @@ -31,7 +31,6 @@ >> * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when >> * the last step cherry picks the 2nd arg, we get a zero. >> */ >> -#define config_enabled(cfg) ___is_defined(cfg) > > Is there a reason to keep using the double underscore prefix? I followed the suggestion in the following message: https://lkml.org/lkml/2016/6/6/944 >> #define __is_defined(x) ___is_defined(x) >> #define ___is_defined(val) ____is_defined(__ARG_PLACEHOLDER_##val) >> #define ____is_defined(arg1_or_junk) __take_second_arg(arg1_or_junk 1, 0) > > __is_defined() is now meant to be used generally, and not just on > kconfig macros. Can it be moved into another header? Currently, __is_defined() is only used in two places: include/linux/export.h include/asm-generic/export.h Even if we fix something like IS_ENABLED(DEBUG), we do not have many for now, but perhaps will it be used more widely in the future? If so, do we need to add IS_DEFINED() or is_defined()? in include/linux/kconfig.h ? or include/linux/kernel.h ? -- Best Regards Masahiro Yamada
[Added Nicolas.] On Thu, 2016-10-20 at 21:06 +0900, Masahiro Yamada wrote: > 2016-10-20 19:04 GMT+09:00 Paul Bolle <pebolle@tiscali.nl>: > > On Sun, 2016-10-16 at 20:07 +0900, Masahiro Yamada wrote: > > There are about a dozen instances of IS_ENABLED() that target something > > other than a kconfig macros. Are you planning to convert those to > > __is_defined() too? > > I did not notice that, but looks like there are some to be checked. Are you willing to do that or should I give it a try (after this patch has landed, of course)? > > > --- a/include/linux/kconfig.h > > > +++ b/include/linux/kconfig.h > > > @@ -31,7 +31,6 @@ > > > * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when > > > * the last step cherry picks the 2nd arg, we get a zero. > > > */ > > > -#define config_enabled(cfg) ___is_defined(cfg) > > > > Is there a reason to keep using the double underscore prefix? > > I followed the suggestion in the following message: > > https://lkml.org/lkml/2016/6/6/944 Nicolas: was there any specific reason to suggest __is_defined() and not, say, is_defined()? > > > #define __is_defined(x) ___is_defined(x) > > > #define > > > ___is_defined(val) ____is_defined(__ARG_PLACEHOLDER_##v > > > al) > > > #define ____is_defined(arg1_or_junk) > > > __take_second_arg(arg1_or_junk 1, 0) > > > > __is_defined() is now meant to be used generally, and not just on > > kconfig macros. Can it be moved into another header? > > Currently, __is_defined() is only used in two places: > > include/linux/export.h > include/asm-generic/export.h > > Even if we fix something like IS_ENABLED(DEBUG), > we do not have many for now, > but perhaps will it be used more widely in the future? > > If so, do we need to add IS_DEFINED() or is_defined()? Either is fine with me. I'll gladly defer to anyone with good taste in naming things. > in include/linux/kconfig.h ? or include/linux/kernel.h ? kernel.h seems to be included just about anywhere and it contains various preprocessor macros of general utility. So that looks like a fine candidate. Would it be a problem to put it there? Thanks, Paul Bolle
On Thu, 20 Oct 2016, Paul Bolle wrote: > [Added Nicolas.] > > On Thu, 2016-10-20 at 21:06 +0900, Masahiro Yamada wrote: > > 2016-10-20 19:04 GMT+09:00 Paul Bolle <pebolle@tiscali.nl>: > > > Is there a reason to keep using the double underscore prefix? > > > > I followed the suggestion in the following message: > > > > https://lkml.org/lkml/2016/6/6/944 > > Nicolas: was there any specific reason to suggest __is_defined() and > not, say, is_defined()? Not really, except maybe to keep more distance from the defined() preprocessor macro. But I don't have a strong opinion either ways. Nicolas
Hi Paul, 2016-10-20 21:44 GMT+09:00 Paul Bolle <pebolle@tiscali.nl>: > [Added Nicolas.] > > On Thu, 2016-10-20 at 21:06 +0900, Masahiro Yamada wrote: >> 2016-10-20 19:04 GMT+09:00 Paul Bolle <pebolle@tiscali.nl>: >> > On Sun, 2016-10-16 at 20:07 +0900, Masahiro Yamada wrote: >> > There are about a dozen instances of IS_ENABLED() that target something >> > other than a kconfig macros. Are you planning to convert those to >> > __is_defined() too? >> >> I did not notice that, but looks like there are some to be checked. > > Are you willing to do that or should I give it a try (after this patch > has landed, of course)? If you volunteer to do it, that'll be appreciated. If you CC me, I am happy to review the patches. -- Best Regards Masahiro Yamada
diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c index ddd2661..887e571 100644 --- a/arch/x86/mm/kaslr.c +++ b/arch/x86/mm/kaslr.c @@ -104,10 +104,10 @@ void __init kernel_randomize_memory(void) * consistent with the vaddr_start/vaddr_end variables. */ BUILD_BUG_ON(vaddr_start >= vaddr_end); - BUILD_BUG_ON(config_enabled(CONFIG_X86_ESPFIX64) && + BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) && vaddr_end >= EFI_VA_START); - BUILD_BUG_ON((config_enabled(CONFIG_X86_ESPFIX64) || - config_enabled(CONFIG_EFI)) && + BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) || + IS_ENABLED(CONFIG_EFI)) && vaddr_end >= __START_KERNEL_map); BUILD_BUG_ON(vaddr_end > __START_KERNEL_map); diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h index 43199a0..63554e9 100644 --- a/include/asm-generic/export.h +++ b/include/asm-generic/export.h @@ -70,7 +70,7 @@ #include <generated/autoksyms.h> #define __EXPORT_SYMBOL(sym, val, sec) \ - __cond_export_sym(sym, val, sec, config_enabled(__KSYM_##sym)) + __cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym)) #define __cond_export_sym(sym, val, sec, conf) \ ___cond_export_sym(sym, val, sec, conf) #define ___cond_export_sym(sym, val, sec, enabled) \ diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index 15ec117..8f2e059 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -31,7 +31,6 @@ * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when * the last step cherry picks the 2nd arg, we get a zero. */ -#define config_enabled(cfg) ___is_defined(cfg) #define __is_defined(x) ___is_defined(x) #define ___is_defined(val) ____is_defined(__ARG_PLACEHOLDER_##val) #define ____is_defined(arg1_or_junk) __take_second_arg(arg1_or_junk 1, 0) @@ -41,13 +40,13 @@ * otherwise. For boolean options, this is equivalent to * IS_ENABLED(CONFIG_FOO). */ -#define IS_BUILTIN(option) config_enabled(option) +#define IS_BUILTIN(option) __is_defined(option) /* * IS_MODULE(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'm', 0 * otherwise. */ -#define IS_MODULE(option) config_enabled(option##_MODULE) +#define IS_MODULE(option) __is_defined(option##_MODULE) /* * IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled
The use of config_enabled() is ambiguous. For config options, IS_ENABLED(), IS_REACHABLE(), etc. will make intention clearer. Sometimes config_enabled() has been used for non-config options because it is useful to check whether the given symbol is defined or not. I have been tackling on deprecating config_enabled(), and now is the time to finish this work. Some new users have appeared for v4.9-rc1, but it is trivial to replace them: - arch/x86/mm/kaslr.c replace config_enabled() with IS_ENABLED() because CONFIG_X86_ESPFIX64 and CONFIG_EFI are boolean. - include/asm-generic/export.h replace config_enabled() with __is_defined(). Then, config_enabled() can be removed now. Going forward, please use IS_ENABLED(), IS_REACHABLE(), etc. for config options, and __is_defined() for non-config symbols. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- arch/x86/mm/kaslr.c | 6 +++--- include/asm-generic/export.h | 2 +- include/linux/kconfig.h | 5 ++--- 3 files changed, 6 insertions(+), 7 deletions(-) -- 1.9.1