Support Rust `core::fmt::Argument` in vsprintf #280

nbdd0121 · 2021-05-19T20:03:39Z

This PR adds to format %pfr format to vsprintf which formats a pointer as a core::fmt::Arguments. Doing so allows us to directly format to the internal buffer of printk, so we don't have to assemble the message on a temporary buffer on the stack.

This does however require changes on the C-side, so I made this PR a RFC.

ojeda · 2021-05-19T20:48:51Z

Why fr in particular?

nbdd0121 · 2021-05-19T20:51:05Z

Both r and R are occupied, so I use f for fmt and r for rust.

ojeda · 2021-05-19T21:01:18Z

Hmm... A new one would allow us to have a little less changes on the C side (at least slightly simpler ones). Even a rare symbol could be used instead of a letter, since users in the C side should never use it on their own. Something that needs escaping like " would really stand out, or even something that needs an octal/hexadecimal escape sequence.

ojeda · 2021-05-19T21:03:10Z

I think the C changes are small enough to not affect our decision.

If we do this, we should also add a check in checkpatch.pl so that people do not use it from C -- but perhaps we can leave that in a "good first issue" instead; and then someone that likes Perl can do it ;-)

nbdd0121 · 2021-05-19T21:13:55Z

Even a rare symbol could be used instead of a letter, since users in the C side should never use it on their own. Something that needs escaping like " would really stand out.

People may use symbol after a regular %p. rg '%p\\"' gives me 2 matches. checkpatch.pl uses (\w)(\w)+ to match extensions, so it suggests that extension has to be alphanumerical characters.

If we do this, we should also add a check in checkpatch.pl so that people do not use it from C.

If we want to prevent people from using it from C, I can just remove the change I made to checkpatch.pl. It'll then complain about invalid specifier.

rust/kernel/print.rs

ojeda · 2021-05-19T21:17:37Z

People may use symbol after a regular %p. rg '%p\\"' gives me 2 matches. checkpatch.pl uses (\w)(\w)+ to match extensions, so it suggests that extension has to be alphanumerical characters.

What about the octal/hex escape sequence idea? (I added it to the comment after posting, sorry)

If we want to prevent people from using it from C, I can just remove the change I made to checkpatch.pl. It'll then complain about invalid specifier.

+1 I would do that, yeah -- nevertheless, having a more specific error (e.g. "the format specifier exists, but should not be used from C") would be best (but please do not do that part, that way we have one more "good first issue " :)

nbdd0121 · 2021-05-19T21:25:33Z

What about the octal/hex escape sequence idea? (I added it to the comment after posting, sorry)

It might be seen as unorthodox 😂. How about just an 'A' for core::fmt::Arguments?

ojeda · 2021-05-19T21:49:09Z

It is not unorthodox if it is not seen ;D

Having an 0xFF byte or similar is fine -- after all, the kernel does a similar thing with e.g. KERN_SOH.

But perhaps a new entry point would be best, that way we do not "pollute" vsprintf.

Anyway, it is not a big deal -- the important bit is avoiding the stack!

nbdd0121 · 2021-05-19T22:49:21Z

If we don't want the C code to use it, do we need to respect the precision and field width specified in printf specifier? If not, then the C side could be even simpler.

ojeda · 2021-05-20T18:51:42Z

Yeah, I do not think we need. Perhaps we could have a check nevertheless that the value is the expected default one, or something like that.

nbdd0121 · 2021-05-20T18:55:28Z

Checking printf specs for default value requires as much change as supporting them. If we can just ignore the values, then we only need to declare and call the function prototypes in lib/vsprintf.c and the C-side change would truly be minimal.

lib/vsprintf.c

ojeda · 2021-05-20T18:59:32Z

rust/Makefile

-		| grep -E ' (T|R|D) ' | cut -d ' ' -f 3 | grep -E '^(__rust_|_R)' \
+		| grep -E ' (T|R|D) ' | cut -d ' ' -f 3 \


Why do we need to remove the filtering? Which symbols are needed now?

(Also, please put the reason in the commit message.)

This is to expose the no_mangle-ed symbol so functions can be called from C-side. I actually don't understand why there is the filtering in the first place, since nm is only showing defined symbols anyway.

The ones that would appear would be:

__rdl_oom __rg_oom __rg_alloc __rg_alloc_zeroed __rg_dealloc __rg_realloc rust_oom

Sorry I don't quite get it, why shouldn't they appear?

I was simply stating the ones for information. :)

I do not remember if there were other symbols being included when I wrote it originally. Perhaps I was trying to ensure we did not expose anything else, or trying to use the smaller set I could.

Should those 7 be exposed, i.e. do we expect rustc to emit calls to those? (even from e.g. generics?)

I mean emitting calls to those from leaf modules or external third-party modules; and by "from generics" I mean whether we could have a call in one of the generics in e.g. kernel and then a leaf module instantiating that generic, generating the call etc.

If it is not something that can be easily answered, or not guaranteed, or something like that, we can remove the filtering, it is not a big deal.

I checked the disassembly, and it seems they are never called directly, but always through __rust_alloc, __rust_alloc_error_handler and friends. Anyway even if we want some filter it should be a denylist rather than by symbol prefixes?

Yeah, the prefixes approach was intended to pick the "Rust-y" symbols. If we go for a list, perhaps the opposite: only allowing the ones we really need (that way we cover future ones that may appear etc.). We can put the adding the list on an issue.

I'll just leave it as is then. We can open an issue to investigate what's best approach for symbol exports.

rust/kernel/print.rs

nbdd0121 · 2021-05-20T20:50:15Z

Changed %pfr to %pA, and removed support for printf specifiers (and thus the width limit).

ojeda

Usual nits.

rust/kernel/print.rs

lib/vsprintf.c

ojeda · 2021-05-20T21:52:30Z

Looks much simpler now, which is always good, and it is always a pleasure to remove code ;)

Please add some explanation in the commit message. I know I am annoying :) but I insist on this because it is how every change will be required to be in the kernel, so I prefer that everybody gets accustomed before we get there :) In our case it is even more important, because we are using GitHub and our discussions are not officially archived in lore etc. as other patches/discussions are.

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 don't 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. Signed-off-by: Gary Guo <[email protected]>

ksquirrel · 2021-05-21T16:57:25Z

Review of 9e8bd679ecf2:

✔️ Commit 9e8bd67: Looks fine!

nbdd0121 · 2021-05-21T16:57:53Z

Changed Arguments to Arguments<'_>

nbdd0121 · 2021-05-26T02:46:17Z

Any blocker?

ojeda · 2021-05-26T03:07:34Z

Changed Arguments to Arguments<'_>

In which cases it is needed?

ojeda · 2021-05-26T03:07:43Z

LGTM

nbdd0121 · 2021-05-26T03:11:58Z

Changed Arguments to Arguments<'_>

In which cases it is needed?

It's part of Rust 2018 edition idioms. https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#elided-lifetimes-in-paths. It's recommended to spell out lifetime arguments.

I'll enable #[warn(rust-2018-idioms)] in another PR.

ojeda · 2021-05-26T03:14:33Z

Ah, thanks for the pointer! I did not know about that.

This comment has been minimized.

Sign in to view

soruh reviewed May 19, 2021

View reviewed changes

rust/kernel/print.rs Outdated Show resolved Hide resolved

rust/kernel/print.rs Outdated Show resolved Hide resolved

ojeda reviewed May 20, 2021

View reviewed changes

nbdd0121 changed the title ~~[RFC] Support Rust core::fmt::Argument in vsprintf~~ Support Rust core::fmt::Argument in vsprintf May 20, 2021

nbdd0121 force-pushed the print branch from c961c4f to f4dafe9 Compare May 20, 2021 20:48

This comment has been minimized.

Sign in to view

ojeda reviewed May 20, 2021

View reviewed changes

rust/kernel/print.rs Outdated Show resolved Hide resolved

rust/kernel/print.rs Outdated Show resolved Hide resolved

lib/vsprintf.c Outdated Show resolved Hide resolved

nbdd0121 force-pushed the print branch from f4dafe9 to 2659ce4 Compare May 20, 2021 22:02

This comment has been minimized.

Sign in to view

ojeda approved these changes May 20, 2021

View reviewed changes

nbdd0121 force-pushed the print branch from 2659ce4 to 9e8bd67 Compare May 21, 2021 16:57

ojeda merged commit bcd1464 into Rust-for-Linux:rust May 26, 2021

nbdd0121 deleted the print branch May 26, 2021 03:15

		\| grep -E ' (T\|R\|D) ' \| cut -d ' ' -f 3 \| grep -E '^(__rust_\|_R)' \
		\| grep -E ' (T\|R\|D) ' \| cut -d ' ' -f 3 \

Support Rust core::fmt::Argument in vsprintf #280

Support Rust core::fmt::Argument in vsprintf #280

Uh oh!

Conversation

nbdd0121 commented May 19, 2021

Uh oh!

This comment has been minimized.

ojeda commented May 19, 2021

Uh oh!

nbdd0121 commented May 19, 2021

Uh oh!

ojeda commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ojeda commented May 19, 2021

Uh oh!

nbdd0121 commented May 19, 2021

Uh oh!

Uh oh!

Uh oh!

ojeda commented May 19, 2021

Uh oh!

nbdd0121 commented May 19, 2021

Uh oh!

ojeda commented May 19, 2021

Uh oh!

nbdd0121 commented May 19, 2021

Uh oh!

ojeda commented May 20, 2021

Uh oh!

nbdd0121 commented May 20, 2021

Uh oh!

Uh oh!

ojeda May 20, 2021

Choose a reason for hiding this comment

Uh oh!

nbdd0121 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

ojeda May 20, 2021

Choose a reason for hiding this comment

Uh oh!

nbdd0121 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

ojeda May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ojeda May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ojeda May 20, 2021

Choose a reason for hiding this comment

Uh oh!

nbdd0121 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

ojeda May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbdd0121 May 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

nbdd0121 commented May 20, 2021

Uh oh!

ojeda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ojeda commented May 20, 2021

Uh oh!

This comment has been minimized.

Support Rust `core::fmt::Argument` in vsprintf #280

Support Rust `core::fmt::Argument` in vsprintf #280

ojeda commented May 19, 2021 •

edited

Loading

ojeda May 20, 2021 •

edited

Loading

ojeda May 20, 2021 •

edited

Loading

ojeda May 20, 2021 •

edited

Loading