lkml.org 
[lkml]   [2021]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5] printk: Userspace format enumeration support
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.

\
 
 \ /
  Last update: 2021-03-16 15:29    [W:0.161 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site