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

[Chris Down <chris@chrisdown.name>]

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.