Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240413 > unrolled thread

[PATCH 1/2] x86: dumpstack, use pr_cont

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] x86: dumpstack, use pr_cont Jiri Slaby <jslaby@suse.cz> - 2015-10-06 14:30 +0200
    Re: [PATCH 1/2] x86: dumpstack, use pr_cont Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 18:10 +0200
      Re: [PATCH 1/2] x86: dumpstack, use pr_cont Jiri Slaby <jslaby@suse.cz> - 2015-10-06 18:20 +0200
        [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 23:10 +0200
          [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 23:10 +0200
            Re: [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Ingo Molnar <mingo@kernel.org> - 2015-10-07 09:10 +0200
          Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Ingo Molnar <mingo@kernel.org> - 2015-10-07 09:00 +0200
          Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Michal Marek <mmarek@suse.cz> - 2015-10-07 10:20 +0200
            Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-07 23:40 +0200
              Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Michal Marek <mmarek@suse.cz> - 2015-10-08 13:50 +0200

#1240413 — [PATCH 1/2] x86: dumpstack, use pr_cont

When dumping flags with which the kernel was built, we print them one
by one in separate printks. Let's use pr_cont as they are
continuation prints.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/dumpstack.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 9c30acfadae2..3850c992f767 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -260,18 +260,18 @@ int __die(const char *str, struct pt_regs *regs, long err)
 	printk(KERN_DEFAULT
 	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
 #ifdef CONFIG_PREEMPT
-	printk("PREEMPT ");
+	pr_cont("PREEMPT ");
 #endif
 #ifdef CONFIG_SMP
-	printk("SMP ");
+	pr_cont("SMP ");
 #endif
 #ifdef CONFIG_DEBUG_PAGEALLOC
-	printk("DEBUG_PAGEALLOC ");
+	pr_cont("DEBUG_PAGEALLOC");
 #endif
 #ifdef CONFIG_KASAN
-	printk("KASAN");
+	pr_cont("KASAN");
 #endif
-	printk("\n");
+	pr_cont("\n");
 	if (notify_die(DIE_OOPS, str, regs, err,
 			current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP)
 		return 1;
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1240625

On Tue, Oct 06 2015, Jiri Slaby <jslaby@suse.cz> wrote:

> When dumping flags with which the kernel was built, we print them one
> by one in separate printks. Let's use pr_cont as they are
> continuation prints.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> ---
>  arch/x86/kernel/dumpstack.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 9c30acfadae2..3850c992f767 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -260,18 +260,18 @@ int __die(const char *str, struct pt_regs *regs, long err)
>  	printk(KERN_DEFAULT
>  	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
>  #ifdef CONFIG_PREEMPT
> -	printk("PREEMPT ");
> +	pr_cont("PREEMPT ");
>  #endif
>  #ifdef CONFIG_SMP
> -	printk("SMP ");
> +	pr_cont("SMP ");
>  #endif
>  #ifdef CONFIG_DEBUG_PAGEALLOC
> -	printk("DEBUG_PAGEALLOC ");
> +	pr_cont("DEBUG_PAGEALLOC");

cosmetic: this lost a space.

May I suggest 

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 9c30acfadae2..b473a47d1851 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -257,21 +257,23 @@ int __die(const char *str, struct pt_regs *regs, long err)
 	unsigned short ss;
 	unsigned long sp;
 #endif
-	printk(KERN_DEFAULT
-	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
+	static const char build_flags[] = ""
 #ifdef CONFIG_PREEMPT
-	printk("PREEMPT ");
+	" PREEMPT"
 #endif
 #ifdef CONFIG_SMP
-	printk("SMP ");
+	" SMP"
 #endif
 #ifdef CONFIG_DEBUG_PAGEALLOC
-	printk("DEBUG_PAGEALLOC ");
+	" DEBUG_PAGEALLOC"
 #endif
 #ifdef CONFIG_KASAN
-	printk("KASAN");
+	" KASAN"
 #endif
-	printk("\n");
+		;
+	printk(KERN_DEFAULT
+	       "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter,
+		build_flags);
 	if (notify_die(DIE_OOPS, str, regs, err,
 			current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP)
 		return 1;

instead, so that there's only one printk call and the pr_cont issue goes
away?

Rasmus

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240656

On 10/06/2015, 06:00 PM, Rasmus Villemoes wrote:
>> --- a/arch/x86/kernel/dumpstack.c
>> +++ b/arch/x86/kernel/dumpstack.c
>> @@ -260,18 +260,18 @@ int __die(const char *str, struct pt_regs *regs, long err)
>>  	printk(KERN_DEFAULT
>>  	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
>>  #ifdef CONFIG_PREEMPT
>> -	printk("PREEMPT ");
>> +	pr_cont("PREEMPT ");
>>  #endif
>>  #ifdef CONFIG_SMP
>> -	printk("SMP ");
>> +	pr_cont("SMP ");
>>  #endif
>>  #ifdef CONFIG_DEBUG_PAGEALLOC
>> -	printk("DEBUG_PAGEALLOC ");
>> +	pr_cont("DEBUG_PAGEALLOC");
> 
> cosmetic: this lost a space.

Oh, good catch. This was left from the times when DEBUG_PAGEALLOC was
last. Now KASAN is, but I added a space to both of them locally for the
time being. Anyway:

> May I suggest 
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 9c30acfadae2..b473a47d1851 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -257,21 +257,23 @@ int __die(const char *str, struct pt_regs *regs, long err)
>  	unsigned short ss;
>  	unsigned long sp;
>  #endif
> -	printk(KERN_DEFAULT
> -	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
> +	static const char build_flags[] = ""
>  #ifdef CONFIG_PREEMPT
> -	printk("PREEMPT ");
> +	" PREEMPT"
>  #endif
>  #ifdef CONFIG_SMP
> -	printk("SMP ");
> +	" SMP"
>  #endif
>  #ifdef CONFIG_DEBUG_PAGEALLOC
> -	printk("DEBUG_PAGEALLOC ");
> +	" DEBUG_PAGEALLOC"
>  #endif
>  #ifdef CONFIG_KASAN
> -	printk("KASAN");
> +	" KASAN"
>  #endif
> -	printk("\n");
> +		;
> +	printk(KERN_DEFAULT
> +	       "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter,
> +		build_flags);
>  	if (notify_die(DIE_OOPS, str, regs, err,
>  			current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP)
>  		return 1;
> 
> instead, so that there's only one printk call and the pr_cont issue goes
> away?

I like this. So if you resend as a proper patch, I will definitely ack
it. But it's up to the maintainers which one they prefer.

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240998 — [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic

It's not hard to generalize the macro magic used to build the
IS_ENABLED macro and friends to produce a few other potentially useful
macros:

CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to
expr, otherwise expands to nothing.

CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set,
expands to expr1, otherwise expands to expr2.

While the latter is roughly the same as
__builtin_choose_expr(IS_ENABLED(CONFIG_FOO), expr1, expr2), the macro
version has the advantage that expr1 and expr2 may be string literals,
and they would preserve their ability to be concatenated with other
string literals. For example, this little snippet

#ifdef CONFIG_X86_64
        "     x86-tsc:   TSC cycle counter\n"
#endif

from kernel/trace/trace.c (which is surrounded by other string
literals) could be written as

  CHOOSE_EXPR(CONFIG_X86_64, "     x86-tsc:   TSC cycle counter\n")

We're also not really restricted to expressions in the C sense; the
only limitation I can see is that they cannot contain unparenthesized
commas. (Obviously, if one starts getting too creative, readability
will suffer rather than increase.)

Similarly, we can define helpers for conditional struct members and
their associated initializers. It would probably take some time to get
used to reading, to pick another random example,

struct task_struct {
   ...
   COND_DECLARATION(CONFIG_KASAN, unsigned int kasan_depth)
   ...
}

#define INIT_KASAN(tsk) COND_INITIALIZER(CONFIG_KASAN, .kasan_depth = 1)

[and I'm certainly not proposing any mass conversion], but I think it
might be nice to avoid lots of short #ifdef/#else/#endif sections. The
above would replace 3 and 5 lines, respectively. Also, git grep'ing
for CONFIG_KASAN currently just reveals that _something_ in sched.h
and init_task.h depends on it; with the above, one could at least
deduce that it's guarding a certain member of some struct.

Namewise, I think CHOOSE_EXPR is appropriate because of its similarity
to __builtin_choose_expr, but I'm not sure about the COND_* ones. Feel
free to suggest better names, and/or to flame this idea to death.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/kconfig.h | 75 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index b33c7797eb57..ac209814b111 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -51,4 +51,79 @@
 #define IS_ENABLED(option) \
 	(IS_BUILTIN(option) || IS_MODULE(option))
 
+/*
+ * CHOOSE_EXPR, COND_DECLARATION and COND_INITIALIZER only work for
+ * boolean config options.
+ *
+ * CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to
+ * expr, otherwise expands to nothing.
+ *
+ * CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set,
+ * expands to expr1, otherwise expands to expr2.
+ *
+ * COND_DECLARATION(CONFIG_FOO, decl): if CONFIG_FOO is set, expands to
+ *
+ *   decl;
+ *
+ * (a semicolon should not be part of decl), otherwise expands to
+ * nothing.
+ *
+ * COND_INITIALIZER(CONFIG_FOO, init): if CONFIG_FOO is set, expands to
+ *
+ *   init,
+ *
+ * otherwise expands to nothing.
+ *
+ * CHOOSE_EXPR(CONFIG_FOO, expr1, expr2) is roughly equivalent to
+ * __builtin_choose_expr(IS_ENABLED(CONFIG_FOO), expr1,
+ * expr2). However, since the expansion is done by the preprocessor,
+ * expr1 and expr2 can be string literals which can then participate
+ * in string concatenation. Also, we're not really limited to
+ * expressions, and can choose to expand to nothing (this is also used
+ * internally by the COND_* macros). The only limitation is that expr1
+ * and expr2 cannot contain unparenthesized commas.
+ *
+ * COND_DECLARATION can, for example, be used inside a struct
+ * declaration to eliminate a #ifdef/#endif pair. This would look
+ * something like
+ *
+ * struct foo {
+ *     int a;
+ *     COND_DECLARATION(CONFIG_FOO_DEBUG, int b)
+ *     int c;
+ * };
+ *
+ * COND_INITIALIZER is the companion for initializing such
+ * conditionally defined members, again for eliminating the bracketing
+ * #ifdef/#endif pair.
+ *
+ * struct foo f = {
+ *     .a = 1,
+ *     COND_INITIALIZER(CONFIG_FOO_DEBUG, .b = 2)
+ *     .c = 3
+ * };
+ *
+ * This is mostly useful when only a single or a few members would be
+ * protected by the #ifdef/#endif. One advantage of the COND_* macros
+ * is that git grep'ing for CONFIG_FOO_DEBUG reveals more information
+ * (above, we would see that it protects the "b" member of some
+ * struct).
+ */
+
+#define _COMMA ,
+#define _COND_PUNCTUATION_0(p)
+#define _COND_PUNCTUATION_1(p) p
+
+#define CHOOSE_EXPR(cfg, expr, ...) _CHOOSE_EXPR(cfg, expr, ##__VA_ARGS__, /* empty defalt arg */)
+#define _CHOOSE_EXPR(cfg, expr, def, ...) __CHOOSE_EXPR(__ARG_PLACEHOLDER_##cfg, expr, def)
+#define __CHOOSE_EXPR(arg1_or_junk, expr, def) ___CHOOSE_EXPR(arg1_or_junk expr, def)
+#define ___CHOOSE_EXPR(__ignored, expr, ...) expr
+
+#define COND_DECLARATION(cfg, decl) _COND_DECLARATION(cfg, decl, CHOOSE_EXPR(cfg, 1, 0))
+#define _COND_DECLARATION(cfg, decl, sfx) __COND_DECLARATION(cfg, decl, sfx)
+#define __COND_DECLARATION(cfg, decl, sfx) CHOOSE_EXPR(cfg, decl) _COND_PUNCTUATION_##sfx(;)
+#define COND_INITIALIZER(cfg, init) _COND_INITIALIZER(cfg, init, CHOOSE_EXPR(cfg, 1, 0))
+#define _COND_INITIALIZER(cfg, init, sfx) __COND_INITIALIZER(cfg, init, sfx)
+#define __COND_INITIALIZER(cfg, init, sfx) CHOOSE_EXPR(cfg, init) _COND_PUNCTUATION_##sfx(_COMMA)
+
 #endif /* __LINUX_KCONFIG_H */
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241004 — [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs

Jiri Slaby noted that these #ifdef protected printks should really be
pr_cont. However, we might as well get completely rid of both the
multiple printk calls and the tiny #ifdef sections by just building an
appropriate string and passing that to the first printk call.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Mostly an example of how 1/2 can be used.

 arch/x86/kernel/dumpstack.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 9c30acfadae2..6ae7e65e734e 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -257,21 +257,16 @@ int __die(const char *str, struct pt_regs *regs, long err)
 	unsigned short ss;
 	unsigned long sp;
 #endif
+	static const char build_flags[] = ""
+		CHOOSE_EXPR(CONFIG_PREEMPT, " PREEMPT")
+		CHOOSE_EXPR(CONFIG_SMP, " SMP")
+		CHOOSE_EXPR(CONFIG_DEBUG_PAGEALLOC, " DEBUG_PAGEALLOC")
+		CHOOSE_EXPR(CONFIG_KASAN, " KASAN");
+
 	printk(KERN_DEFAULT
-	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
-#ifdef CONFIG_PREEMPT
-	printk("PREEMPT ");
-#endif
-#ifdef CONFIG_SMP
-	printk("SMP ");
-#endif
-#ifdef CONFIG_DEBUG_PAGEALLOC
-	printk("DEBUG_PAGEALLOC ");
-#endif
-#ifdef CONFIG_KASAN
-	printk("KASAN");
-#endif
-	printk("\n");
+	       "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter,
+	       build_flags);
+
 	if (notify_die(DIE_OOPS, str, regs, err,
 			current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP)
 		return 1;
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241181 — Re: [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs

* Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> +	static const char build_flags[] = ""
> +		CHOOSE_EXPR(CONFIG_PREEMPT, " PREEMPT")
> +		CHOOSE_EXPR(CONFIG_SMP, " SMP")
> +		CHOOSE_EXPR(CONFIG_DEBUG_PAGEALLOC, " DEBUG_PAGEALLOC")
> +		CHOOSE_EXPR(CONFIG_KASAN, " KASAN");
> +
>  	printk(KERN_DEFAULT
> -	       "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter);
> -#ifdef CONFIG_PREEMPT
> -	printk("PREEMPT ");
> -#endif
> -#ifdef CONFIG_SMP
> -	printk("SMP ");
> -#endif
> -#ifdef CONFIG_DEBUG_PAGEALLOC
> -	printk("DEBUG_PAGEALLOC ");
> -#endif
> -#ifdef CONFIG_KASAN
> -	printk("KASAN");
> -#endif
> -	printk("\n");
> +	       "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter,
> +	       build_flags);
> +
>  	if (notify_die(DIE_OOPS, str, regs, err,
>  			current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP)
>  		return 1;

Looks cleaner than what we had before, but I have a naming nit: CHOOSE_EXPR() is 
not something I'd be able to remember, I'd have to look it up again all the time, 
because it does not have 'config' in its name, and because 'choose' is usually 
associated with different constructs.

So how about something more intuitive, like:

		COND_CONFIG(CONFIG_PREEMPT,		" PREEMPT")
		COND_CONFIG(CONFIG_SMP,			" SMP")
		COND_CONFIG(CONFIG_DEBUG_PAGEALLOC,	" DEBUG_PAGEALLOC")
		COND_CONFIG(CONFIG_KASAN,		" KASAN");

or:

		IF_CONFIG(CONFIG_PREEMPT,		" PREEMPT")
		IF_CONFIG(CONFIG_SMP,			" SMP")
		IF_CONFIG(CONFIG_DEBUG_PAGEALLOC,	" DEBUG_PAGEALLOC")
		IF_CONFIG(CONFIG_KASAN,			" KASAN");

?

Both names are still unused in the kernel repo.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241176 — Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic

* Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> +#define _COMMA ,
> +#define _COND_PUNCTUATION_0(p)
> +#define _COND_PUNCTUATION_1(p) p
> +
> +#define CHOOSE_EXPR(cfg, expr, ...) _CHOOSE_EXPR(cfg, expr, ##__VA_ARGS__, /* empty defalt arg */)
> +#define _CHOOSE_EXPR(cfg, expr, def, ...) __CHOOSE_EXPR(__ARG_PLACEHOLDER_##cfg, expr, def)
> +#define __CHOOSE_EXPR(arg1_or_junk, expr, def) ___CHOOSE_EXPR(arg1_or_junk expr, def)
> +#define ___CHOOSE_EXPR(__ignored, expr, ...) expr
> +
> +#define COND_DECLARATION(cfg, decl) _COND_DECLARATION(cfg, decl, CHOOSE_EXPR(cfg, 1, 0))
> +#define _COND_DECLARATION(cfg, decl, sfx) __COND_DECLARATION(cfg, decl, sfx)
> +#define __COND_DECLARATION(cfg, decl, sfx) CHOOSE_EXPR(cfg, decl) _COND_PUNCTUATION_##sfx(;)
> +#define COND_INITIALIZER(cfg, init) _COND_INITIALIZER(cfg, init, CHOOSE_EXPR(cfg, 1, 0))
> +#define _COND_INITIALIZER(cfg, init, sfx) __COND_INITIALIZER(cfg, init, sfx)
> +#define __COND_INITIALIZER(cfg, init, sfx) CHOOSE_EXPR(cfg, init) _COND_PUNCTUATION_##sfx(_COMMA)

Pet peeve, mind structuring this in a typographically more readable fashion, by 
adding some common-sense vertical structure to the definitions:

#define    CHOOSE_EXPR(cfg, expr, ...)		  _CHOOSE_EXPR(cfg, expr, ##__VA_ARGS__, /* empty defalt arg */)
#define   _CHOOSE_EXPR(cfg, expr, def, ...)	 __CHOOSE_EXPR(__ARG_PLACEHOLDER_##cfg, expr, def)
#define  __CHOOSE_EXPR(arg1_or_junk, expr, def)	___CHOOSE_EXPR(arg1_or_junk expr, def)
#define ___CHOOSE_EXPR(__ignored, expr, ...)	expr

#define   COND_DECLARATION(cfg, decl)	    _COND_DECLARATION(cfg, decl, CHOOSE_EXPR(cfg, 1, 0))
#define  _COND_DECLARATION(cfg, decl, sfx) __COND_DECLARATION(cfg, decl, sfx)
#define __COND_DECLARATION(cfg, decl, sfx) CHOOSE_EXPR(cfg, decl) _COND_PUNCTUATION_##sfx(;)

#define   COND_INITIALIZER(cfg, init)	    _COND_INITIALIZER(cfg, init, CHOOSE_EXPR(cfg, 1, 0))
#define  _COND_INITIALIZER(cfg, init, sfx) __COND_INITIALIZER(cfg, init, sfx)
#define __COND_INITIALIZER(cfg, init, sfx) CHOOSE_EXPR(cfg, init) _COND_PUNCTUATION_##sfx(_COMMA)

?

It's still a mouthful, but at least readable at a glance.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241221 — Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic

On 2015-10-06 23:05, Rasmus Villemoes wrote:
> It's not hard to generalize the macro magic used to build the
> IS_ENABLED macro and friends to produce a few other potentially useful
> macros:
> 
> CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to
> expr, otherwise expands to nothing.
> 
> CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set,
> expands to expr1, otherwise expands to expr2.

FWIW, I agree with Ingo that the CHOOSE_EXPR name is not really obvious.
IF_CONFIG is a better alternative IMO, since the average programmer
probably does not know __builtin_choose_expr() to see the analogy.


> Similarly, we can define helpers for conditional struct members and
> their associated initializers. It would probably take some time to get
> used to reading, to pick another random example,
> 
> struct task_struct {
>    ...
>    COND_DECLARATION(CONFIG_KASAN, unsigned int kasan_depth)
>    ...
> }

While the C standard syntax requires struct-declaration to actually
declare a member, the compiler will happily ignore the extra semicolon
if you write

truct task_struct {
   ...
   CHOOSE_EXPR(CONFIG_KASAN, unsigned int kasan_depth);
   ...
}

So I think that the COND_DECLARATION macro is not necessary.


> #define INIT_KASAN(tsk) COND_INITIALIZER(CONFIG_KASAN, .kasan_depth = 1)

COND_INITIALIZER on the other hand is useful (CHOOSE_EXPR(CONFIG_KASAN,
.kasan_depth = 1 _COMMA) does does not work, unfortunately).


> [and I'm certainly not proposing any mass conversion], but I think it
> might be nice to avoid lots of short #ifdef/#else/#endif sections.

It should be accompanied by a patch to scripts/tags.sh teaching
ctags/etags about the new macros.

Michal

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241815 — Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic

On Wed, Oct 07 2015, Michal Marek <mmarek@suse.cz> wrote:

> On 2015-10-06 23:05, Rasmus Villemoes wrote:
>> It's not hard to generalize the macro magic used to build the
>> IS_ENABLED macro and friends to produce a few other potentially useful
>> macros:
>> 
>> CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to
>> expr, otherwise expands to nothing.
>> 
>> CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set,
>> expands to expr1, otherwise expands to expr2.
>
> FWIW, I agree with Ingo that the CHOOSE_EXPR name is not really obvious.
> IF_CONFIG is a better alternative IMO, since the average programmer
> probably does not know __builtin_choose_expr() to see the analogy.

OK, CHOOSE_EXPR is out. But I think IF_CONFIG/COND_CONFIG might be a
little annoying or redundant, since "CONFIG" would also always be part
of the first argument.

Come to think of it, since this would be a primitive for conditional
compilation whose primary purpose is to eliminate the verbosity of
#ifdef/#endif, I'd prefer plain and simple COND, with COND_INITIALIZER
as a sidekick for that special purpose. Unfortunately, COND is already
used in a few places :( So I'll go with COND_CONFIG for now, but wait a
few days before sending v2, to see if anyone else has comments or naming
suggestions.

> While the C standard syntax requires struct-declaration to actually
> declare a member, the compiler will happily ignore the extra semicolon
> if you write
>
> truct task_struct {
>    ...
>    CHOOSE_EXPR(CONFIG_KASAN, unsigned int kasan_depth);
>    ...
> }
>
> So I think that the COND_DECLARATION macro is not necessary.

Thanks, I didn't know that. I see that both gcc and clang accept it
whether the extra semicolon is at the beginning or end of the struct,
and whether there's even a single actual member. OK, then
COND_DECLARATION is redundant (though it might still be useful as a
natural buddy to COND_INITIALIZER).

>> #define INIT_KASAN(tsk) COND_INITIALIZER(CONFIG_KASAN, .kasan_depth = 1)
>
> COND_INITIALIZER on the other hand is useful (CHOOSE_EXPR(CONFIG_KASAN,
> .kasan_depth = 1 _COMMA) does does not work, unfortunately).

Yeah, since we need to do the multiple expansion thing there's no way of
preventing _COMMA from expanding too early, so I'm pretty sure one would
need some specialized version of CHOOSE_EXPR (or whatever the name ends
up being). Also, I wouldn't really want users to have to supply the
_COMMA. One could also consider making COND_ARGUMENT an alias for it -
that could be useful for some seq_printf calls generating /proc files
(where the format string would be built with COND_CONFIG).

>> [and I'm certainly not proposing any mass conversion], but I think it
>> might be nice to avoid lots of short #ifdef/#else/#endif sections.
>
> It should be accompanied by a patch to scripts/tags.sh teaching
> ctags/etags about the new macros.

Do you mean that something like

	--regex-c='/COND_CONFIG\([^,]*,([^,]*)\)/\1/'

should be added so ctags would pick up the text in the true branch? I'm
not very familiar with ctags.

Thanks for the feedback, Michal and Ingo.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242261 — Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic

On 2015-10-07 23:33, Rasmus Villemoes wrote:
> On Wed, Oct 07 2015, Michal Marek <mmarek@suse.cz> wrote:
>> It should be accompanied by a patch to scripts/tags.sh teaching
>> ctags/etags about the new macros.
> 
> Do you mean that something like
> 
> 	--regex-c='/COND_CONFIG\([^,]*,([^,]*)\)/\1/'
> 
> should be added so ctags would pick up the text in the true branch? I'm
> not very familiar with ctags.

Something like this, yes. This particular rule does not work for me,
though and I don't see an obvious reason why.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web