Re: [PATCH v5] printk: Userspace format enumeration support

[Chris Down]

Rasmus Villemoes writes:
> I think it's pointless renaming the symbol to _printk, with all the
> churn and reduced readability that involves (especially when reading
> assembly "why are we calling _printk and not printk here?"). There's
> nothing wrong with providing a macro wrapper by the same name
>
> #define printk(bla bla) ({ do_stuff; printk(bla bla); })
>
> Only two places would need to be updated to surround the word printk in
> parentheses to suppress macro expansion: The declaration and the
> definition of printk. I.e.
>
> int (printk)(const char *s, ...)

Hmm, I'm indifferent to either. Personally I don't like the ambiguity of having 
both a macro and function share the same name and having to think "what's the 
preprocessor context here?".

> > +extern struct pi_object __start_printk_index[];
> > +extern struct pi_object __stop_printk_index[];
>
> Do you need these declarations to be visible to the whole kernel? Can't
> they live in printk/index.c?

Yeah, this is a leftover: already noted for rescoping in v6. :-)

> > +
> > +#define pi_sec_elf_embed(_p_func, _fmt, ...)				       \
> > +	({								       \
> > +		int _p_ret;						       \
> > +									       \
> > +		if (__builtin_constant_p(_fmt)) {			       \
> > +			/*
> > +			 * The compiler may not be able to eliminate this, so
> > +			 * we need to make sure that it doesn't see any
> > +			 * hypothetical assignment for non-constants even
> > +			 * though this is already inside the
> > +			 * __builtin_constant_p guard.
> > +			 */						       \
> > +			static struct pi_object _pi			       \
>
> static const struct pi_object?
>
> > +			__section(".printk_index") = {			       \
> > +				.fmt = __builtin_constant_p(_fmt) ? (_fmt) : NULL, \
> > +				.func = __func__,			       \
> > +				.file = __FILE__,			       \
> > +				.line = __LINE__,			       \
> > +			};						       \
> > +			_p_ret = _p_func(_pi.fmt, ##__VA_ARGS__);	       \
>
> Is the use of _pi.fmt here a trick to prevent gcc from eliding the _pi
> object, so it is seen as "used"? That seems a bit fragile, especially if
> the compiler ends up generating the same code in .text - that means gcc
> does not load the format string from the _pi object (which it
> shouldn't), but then I don't see why it (or the next version of gcc)
> couldn't realize that _pi is indeed unused.
>
> There's the __used attribute precisely for this kind of thing. Then you
> could also eliminate
>
> > +		} else							       \
> > +			_p_ret = _p_func(_fmt, ##__VA_ARGS__);		       \
> > +									       \
>
> this and the _p_ret variable
>
> > +		_p_ret;							       \
>
> and just end the ({}) with _p_func(_fmt, ##__VA_ARGS__);

Oh, this is a leftover from the early days of the patch when we used to 
explicitly store the formats in ._printk_fmts in order to avoid duplication. 
Now that we just store a pointer instead of storing the format itself, it 
probably should be fine to move to using _fmt directly and __used. Thanks, I'll 
investigate this for v6.