On Tue, Jun 21, 2022 at 09:04:38AM +0200, Rasmus Villemoes wrote:
> On 20/06/2022 02.42, Kent Overstreet wrote:
> > +Note that a pretty-printer may not sleep, if called from printk(). If called
> > +from pr_buf() or sprintf() there are no such restrictions.
>
> I know what you're trying to say, but if the sprintf() call itself is
> from a non-sleepable context this is obviously not true. So please just
> make the rule "A pretty-printer must not sleep.". That's much simpler
> and less error-prone. Otherwise I guarantee you that somebody is going
> to add a sleeping pretty-printer for their own need, use it in a couple
> of safe places, and then somebody wants to add a printk() in that driver
> and sees "hey, I can get all this state dumped very easily with this
> pretty-printer".
Kernel programmers are used to having to consider the context they're in and
what the functions they're calling might do, a pretty-printer being called
indirectly via sprintf() is absolutely no different.
> > struct printf_spec {
> > @@ -2520,7 +2521,16 @@ int format_decode(const char *fmt, struct printf_spec *spec)
> > return ++fmt - start;
> >
> > case 'p':
> > - spec->type = FORMAT_TYPE_PTR;
> > + fmt++;
> > + if (fmt[0] == 'f' &&
> > + fmt[1] == '(') {
> > + fmt += 2;
> > + spec->type = FORMAT_TYPE_FN;
> > + } else
> > + spec->type = FORMAT_TYPE_PTR;
> > + return fmt - start;
> > + case '(':
> > + spec->type = FORMAT_TYPE_FN;
> > return ++fmt - start;
>
> NAK. Don't implement something that will never be tested nor used.
> There's not a snowball's chance in hell that we'll ever build the kernel
> without -Wformat.
We're not stopping here. Matthew is taking this to WG14 and I'll be working on
adding this functionality to glibc next, and %() is the syntax we intend to take
to the working group.
But the working group is naturally going to want to see that a working
implementation of it exists.
> Sorry, but this is way too ugly, and the prospect of at some point in
> the future invoking libffi to do something even naster... eww. We do not
> need more functions with completely generic prototypes with no
> typechecking and making it extremely hard to teach one of our static
> analyzers (smatch has some %pX checking) to do that typechecking.
>
> There are at least two ways you can achieve this passing of a variable
> number of arguments with proper types.
>
> (1) Each pretty-printer comes with a struct wrapping up its real
> arguments and a macro for creating a compound literal passing those
> arguments.
>
> struct foo_pp {
> void (*func)(struct printbuf *pb, void *ctx); /* always first */
> int x;
> long y;
> };
> void foo_pp(struct printbuf *pb, void *ctx)
> {
> struct foo_pp *f = ctx;
> pr_printf(pb, "%d %ld", f->x, f->y);
> }
>
> #define FOO_PP(_x, _y) (struct foo_pp){.func = foo_pp, .x = (_x), .y = (_y)}
>
> printk("bla bla %pf\n", &FOO_PP(aa, bb));
Hellllllllll no.
All that's missing right now is gcc checking that the function signature matches
the args specified within the (). That will come. No way in hell am I going to
implement some half baked hacked up macro crap - I intend to do this right.
> (2) Let the pretty-printer itself extract the varargs it expects. To
> portably pass around a va_list by reference it needs to be wrapped, so
> this would be
>
> struct wva { va_list ap; };
>
> void foo_pp(struct printbuf *pb, struct wva *w)
> {
> int x = va_arg(w->ap, int);
> long y = va_arg(w->ap, long);
> pr_printf(pb, "%d %ld", x, y);
> }
>
> printk("bla bla %pf(%d, %ld)\n", foo_pp, aa, bb)
>
> with the core printf implementation internally using such a wrapped
> va_list, and after a %pf( relying on the pretty-printer having consumed
> the arguments up until the closing ). It would probably be a good idea
> to give the pretty-printer a pointer to that opening '(' or one-past-it
> as well so it could do a sanity check.
Also no. Varargs is terrible in most situations, and should only be used for
functions that actually want to take variable numbers of arguments - that
doesn't apply to pretty printers.
