| Message ID | 20211206140313.5653-12-ojeda@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust support | expand |
On Mon, Dec 06, 2021 at 03:03:05PM +0100, Miguel Ojeda wrote: > From: Gary Guo <gary@garyguo.net> > > This patch adds a format specifier `%pA` to `vsprintf` which formats > a pointer as `core::fmt::Arguments`. Doing so allows us to directly > format to the internal buffer of `printf`, so we do not have to use > a temporary buffer on the stack to pre-assemble the message on > the Rust side. > > This specifier is intended only to be used from Rust and not for C, so > `checkpatch.pl` is intentionally unchanged to catch any misuse. > > Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com> > Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com> > Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com> > Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com> > Signed-off-by: Gary Guo <gary@garyguo.net> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > lib/vsprintf.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 58d5e567f836..bc9c05427d9a 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -2233,6 +2233,10 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode, > return widen_string(buf, buf - buf_start, end, spec); > } > > +#ifdef CONFIG_RUST > +char *rust_fmt_argument(char* buf, char* end, void *ptr); > +#endif That should be in a .h file somewhere. Remember, don't put #ifdef in .c files please. > + > /* Disable pointer hashing if requested */ > bool no_hash_pointers __ro_after_init; > EXPORT_SYMBOL_GPL(no_hash_pointers); > @@ -2388,6 +2392,10 @@ early_param("no_hash_pointers", no_hash_pointers_enable); > * > * Note: The default behaviour (unadorned %p) is to hash the address, > * rendering it useful as a unique identifier. > + * > + * There is also a '%pA' format specifier, but it is only intended to be used > + * from Rust code to format core::fmt::Arguments. Do *not* use it from C. > + * See rust/kernel/print.rs for details. > */ > static noinline_for_stack > char *pointer(const char *fmt, char *buf, char *end, void *ptr, > @@ -2460,6 +2468,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > return device_node_string(buf, end, ptr, spec, fmt + 1); > case 'f': > return fwnode_string(buf, end, ptr, spec, fmt + 1); > +#ifdef CONFIG_RUST > + case 'A': > + return rust_fmt_argument(buf, end, ptr); > +#endif Same here, this should not be needed if you put it in a .h file correctly. thanks, greg k-h
On Mon, Dec 6, 2021 at 4:46 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > That should be in a .h file somewhere. Remember, don't put #ifdef in .c > files please. Will do, thanks for reviewing! > Same here, this should not be needed if you put it in a .h file > correctly. This one is mimicking the `CONFIG_BLOCK` one (`case 'g'` a bit above) -- but we can change it, of course. Cheers, Miguel
On Mon, Dec 06, 2021 at 04:56:32PM +0100, Miguel Ojeda wrote: > On Mon, Dec 6, 2021 at 4:46 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > That should be in a .h file somewhere. Remember, don't put #ifdef in .c > > files please. > > Will do, thanks for reviewing! > > > Same here, this should not be needed if you put it in a .h file > > correctly. > > This one is mimicking the `CONFIG_BLOCK` one (`case 'g'` a bit above) > -- but we can change it, of course. That should be changed as well :)
On Mon, Dec 6, 2021 at 8:14 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Dec 06, 2021 at 04:56:32PM +0100, Miguel Ojeda wrote: > > On Mon, Dec 6, 2021 at 4:46 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > That should be in a .h file somewhere. Remember, don't put #ifdef in .c > > > files please. Why not put #ifdef in .c files? > > Will do, thanks for reviewing! > > > > > Same here, this should not be needed if you put it in a .h file > > > correctly. I guess IS_ENABLED could be used in the .c code, but I don't see how they could move the dispatch to rust_fmt_argument to a header without moving the definition of pointer() to a header, which they probably _cant_ do because it's noinline_for_stack. > > > > This one is mimicking the `CONFIG_BLOCK` one (`case 'g'` a bit above) > > -- but we can change it, of course. > > That should be changed as well :) Isn't the point to minimize code that's unused for certain configurations?
On Mon, Dec 06, 2021 at 11:52:09AM -0800, Nick Desaulniers wrote: > On Mon, Dec 6, 2021 at 8:14 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Dec 06, 2021 at 04:56:32PM +0100, Miguel Ojeda wrote: > > > On Mon, Dec 6, 2021 at 4:46 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > That should be in a .h file somewhere. Remember, don't put #ifdef in .c > > > > files please. > > Why not put #ifdef in .c files? > > > > Will do, thanks for reviewing! > > > > > > > Same here, this should not be needed if you put it in a .h file > > > > correctly. > > I guess IS_ENABLED could be used in the .c code, but I don't see how > they could move the dispatch to rust_fmt_argument to a header without > moving the definition of pointer() to a header, which they probably > _cant_ do because it's noinline_for_stack. In the header file, you put: #ifdef CONFIG_FOO int foo(void); #else static inline int foo(void) { } #endif and then in your .c file, you call foo() unconditionally, and everything works beautifully.
On Mon, Dec 6, 2021 at 11:55 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Dec 06, 2021 at 11:52:09AM -0800, Nick Desaulniers wrote: > > On Mon, Dec 6, 2021 at 8:14 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Dec 06, 2021 at 04:56:32PM +0100, Miguel Ojeda wrote: > > > > On Mon, Dec 6, 2021 at 4:46 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > That should be in a .h file somewhere. Remember, don't put #ifdef in .c > > > > > files please. > > > > Why not put #ifdef in .c files? > > > > > > Will do, thanks for reviewing! > > > > > > > > > Same here, this should not be needed if you put it in a .h file > > > > > correctly. > > > > I guess IS_ENABLED could be used in the .c code, but I don't see how > > they could move the dispatch to rust_fmt_argument to a header without > > moving the definition of pointer() to a header, which they probably > > _cant_ do because it's noinline_for_stack. > > In the header file, you put: > > #ifdef CONFIG_FOO > int foo(void); > #else > static inline int foo(void) { } > #endif > > and then in your .c file, you call foo() unconditionally, and everything > works beautifully. > Ah, that is nice, thanks!
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 58d5e567f836..bc9c05427d9a 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2233,6 +2233,10 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode, return widen_string(buf, buf - buf_start, end, spec); } +#ifdef CONFIG_RUST +char *rust_fmt_argument(char* buf, char* end, void *ptr); +#endif + /* Disable pointer hashing if requested */ bool no_hash_pointers __ro_after_init; EXPORT_SYMBOL_GPL(no_hash_pointers); @@ -2388,6 +2392,10 @@ early_param("no_hash_pointers", no_hash_pointers_enable); * * Note: The default behaviour (unadorned %p) is to hash the address, * rendering it useful as a unique identifier. + * + * There is also a '%pA' format specifier, but it is only intended to be used + * from Rust code to format core::fmt::Arguments. Do *not* use it from C. + * See rust/kernel/print.rs for details. */ static noinline_for_stack char *pointer(const char *fmt, char *buf, char *end, void *ptr, @@ -2460,6 +2468,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return device_node_string(buf, end, ptr, spec, fmt + 1); case 'f': return fwnode_string(buf, end, ptr, spec, fmt + 1); +#ifdef CONFIG_RUST + case 'A': + return rust_fmt_argument(buf, end, ptr); +#endif case 'x': return pointer_string(buf, end, ptr, spec); case 'e':