Messages in this thread | | | Date | Tue, 16 Mar 2021 14:28:12 +0000 | From | Chris Down <> | Subject | Re: [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.
| |