Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 28 Jun 2022 13:23:21 -0400
From:   Kent Overstreet <kent.overstreet@gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH v5whatever, now with typechecking] vsprintf: %pf(%p)
Message-ID: <20220628172321.gbgfif7zgz5ny3mo@moria.home.lan>
References: <20220628032439.3314451-1-kent.overstreet@gmail.com>
 <CAHk-=wiGMNvKaVuSDD7y2JeK+NsNyXtqZEusOLmEw9uE+0ZySg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wiGMNvKaVuSDD7y2JeK+NsNyXtqZEusOLmEw9uE+0ZySg@mail.gmail.com>
Precedence: bulk

On Mon, Jun 27, 2022 at 09:46:46PM -0700, Linus Torvalds wrote:
>     // type warning *and* link-time failure
>     extern int not_a_pointer;
> 
>     extern int pretty_print_int(int *);
>     extern int pretty_print_long_long(long long *);
> 
>     #define __PP_FN(ptr) _Generic(*(ptr),               \
>         int:            pretty_print_int,       \
>         long long:      pretty_print_long_long, \
>         default:        not_a_pointer )
> 
>     #define __PP_CHECK(fn,ptr)  \
>         __PP_MAGIC+(__typeof__ ((fn)(ptr)))0, fn, ptr
> 
>    #define pretty_fn(fn,ptr) &(struct pretty_print_struct) \
>        { __PP_CHECK(__PP_FN(ptr), ptr) }
> 
>     #define pretty_print(ptr) pretty_fn(__PP_FN(ptr), ptr)
> 
>     void test_me(int val)
>     {
>         printf("%pf\n", pretty_print(&val));
>     }

Your version is nicer for the simpler cases, but it's not general enough for
what I'm looking at.

 - We need to be able to specify the pretty-printer function, and not just have
   one per type

From the commit message for my patch converting %p[iI] pretty printers to
printbuf style pretty printers:

     %piS     -> prt_sockaddr_zeropad
     %pIS     -> prt_sockaddr
     %pISc    -> prt_sockaddr_compressed
     %pISfc   -> prt_sockaddr_flow_compressed
     %pISp    -> prt_sockaddr_port
     %pISpc   -> prt_sockaddr_port_compressed
     %pISpsc  -> prt_sockaddr_port_scope_compressed
     %pISpfc  -> prt_sockaddr_port_flow_compressed
     %pISpfsc -> prt_sockaddr_port_scope_flow_compressed

That's just sockaddrs, and I limited it to creating functions for the printk
formats that are actually currently in use.

What we _really_ want is just to have a separate flags argument to prt_sockaddr,
but in the current code only pointer arguments are supported.

 - We need pretty-printer functions that take variable numbers of arguments, and
   not just for a flags argument

In my OOM report patch series, I'm doing

printk("%pf()", CALL_PP(slabs_to_text));
printk("%pf()", CALL_PP(shrinkers_to_text));

We want slabs_to_text and shrinkers_to_text to be pretty-printers instead of
just calling printk directly so that procfs/sysfs/whatever the current fashion
is can also dump the same report.

And in bcachefs, I've got a bunch of pretty-printers that need a pointer to the
filesystem object in addition to a pointer to the object they're printing so
that they can decode all kinds of things. Pretty sure this is going to come up
elsewhere (other filesystems, for sure).

Also:

I want the pretty-printer function to be specified explicitly, so that when
reading the code we can cscope to it. When we're doing a function call, even
indirectly, _I want the name of that function in front of me_ and I think that
is one of the major advantages of this approach vs. our current %p extensions.

BUT

Maybe we can get rid of specifying the arguments in the format string, and have
it be just %pf instead of %pf(...).

The struct call_pp needs to handle a variable number of arguments, not just one
as in your example, and I really do want it to handle integer arguments, and not
just pointer arguments.

So here's the $20 million question: Can we, safely, on all archictectures, treat
all arguments as ulongs as long as the actual function arguments are either
pointers or integers not bigger than a ulong? Or are there crazy architectures
out there where passing pointers is different from passing integers in the ABI?

We'd also really like to be able to pass u64s too - prt_u64() is now a standard
pretty-printer, along with prt_u64_human_readable()!

Because yeah fundamentally the reason for the %pf(%p,%llu) thing is that we know
how to pass those types through varargs to sprintf, and libffi is a thing so we
know that the future we'll be able to do constructed function calls including
with u64 arguments, so this seems solvable.

But maybe serializing them before the call in struct call_pp isn't actually any
harder, and actually it gets around the fact that C promotes all ints smaller
than ints to ints in varargs, so that's nice.

I need to think about this some more.