On Wed, Apr 29, 2026 at 10:47:28PM +0100, David Laight wrote: > On Wed, 29 Apr 2026 13:42:26 -0400 > Steven Rostedt <[email protected]> wrote: > > > On Thu, 30 Apr 2026 00:57:07 +0800 > > Qian-Yu Lin <[email protected]> wrote: > > > > > The macro trace_printk() uses a hardcoded identifier _______STR > > > within a statement expression, which can lead to variable name > > > shadowing if a caller happens to use the same name in its scope. > > > > Has this ever been a problem? > > > > > > > > Following the pattern in commit 24ba53017e18 ("rcu: Replace ________p1 > > > and _________p1 with __UNIQUE_ID(rcu)") and commit 589a9785ee3a > > > ("min/max: remove sparse warnings when they're nested"), replace the > > > hardcoded identifier with __UNIQUE_ID(STR). > > min and max do get nested - so it is important that the 'local' > variables have different names - otherwise you can get invalid expansions. > > No one is going to have: trace_printk(fmt1, trace_printk(ftm2, ...), ...) > it just doesn't make sense. > > There is a slight problem the ____________________________STR might > be used by an entirely different #define. > Just using _trace_printk_str[] would make this pretty unlikely. > It would also have the advantage of making the .i file a bit easier > to read, all the UNIQUE names in min/max output make it really hard > to see what the output actually means. >
Thank you for the suggestion. However, _trace_printk_str still carries a theoretical shadowing risk if a caller happens to use the same name. > > > > > > Since __UNIQUE_ID() must be expanded once to remain consistent across > > > declaration and sizeof() within the statement expression, introduce a > > > nested helper macro ___trace_printk. > > > > Hmm, so we are replacing one name with underscores with another name > > with underscores? > > > > > > > > Signed-off-by: Qian-Yu Lin <[email protected]> > > > --- > > > include/linux/trace_printk.h | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/linux/trace_printk.h b/include/linux/trace_printk.h > > > index 2670ec7f4262..060eccb40838 100644 > > > --- a/include/linux/trace_printk.h > > > +++ b/include/linux/trace_printk.h > > > @@ -2,6 +2,7 @@ > > > #ifndef _LINUX_TRACE_PRINTK_H > > > #define _LINUX_TRACE_PRINTK_H > > > > > > +#include <linux/compiler.h> > > > > People are already saying that trace_printk.h slows down the compile. > > Does this add any overhead to the compile? > > A little - nothing is free. > > David > Indeed. I measured compile time of kernel/trace/ring_buffer_benchmark.o after make clean on an x86_64 machine running Ubuntu 24.04 LTS: - Original _______STR: 49.8s - v1 with __UNIQUE_ID (compiler.h): 53.5s - compound literal (no extra include): 33.2s A compound literal eliminates the local variable entirely, removing the shadowing risk without any extra include or compile overhead: #define trace_printk(fmt, ...) \ do { \ if (sizeof((char[]) \ {__stringify((__VA_ARGS__))}) > 3) \ do_trace_printk(fmt, ##__VA_ARGS__); \ else \ trace_puts(fmt); \ } while (0) Qian-Yu > > > > -- Steve > > > > > > > #include <linux/compiler_attributes.h> > > > #include <linux/instruction_pointer.h> > > > #include <linux/stddef.h> > > > @@ -84,15 +85,18 @@ do { > > > \ > > > * let gcc optimize the rest. > > > */ > > > > > > -#define trace_printk(fmt, ...) \ > > > +#define ___trace_printk(fmt, str, ...) \ > > > do { \ > > > - char _______STR[] = __stringify((__VA_ARGS__)); \ > > > - if (sizeof(_______STR) > 3) \ > > > + char str[] = __stringify((__VA_ARGS__)); \ > > > + if (sizeof(str) > 3) \ > > > do_trace_printk(fmt, ##__VA_ARGS__); \ > > > else \ > > > trace_puts(fmt); \ > > > } while (0) > > > > > > +#define trace_printk(fmt, ...) \ > > > + ___trace_printk(fmt, __UNIQUE_ID(str), ##__VA_ARGS__) > > > + > > > #define do_trace_printk(fmt, args...) > > > \ > > > do { > > > \ > > > static const char *trace_printk_fmt __used \ > > > > >
