Re: [PATCH 04/13] Kbuild: Rust support
On Mon, Apr 19, 2021 at 10:18 PM Matthew Wilcox wrote: > > Yes, I agree, we need a better story for name mangling. > My proposal is that we store a pretty name which matches the source > (eg rust_binder::range_alloc) and a sha1 of the mangled symbol > (40 bytes of uninteresting hex). Symbol resolution is performed against > the sha1. Printing is of the pretty name. It should be obvious from > the stack trace which variant of a function is being called, no? If the pretty name is only `rust_binder::range_alloc`, that would not be enough, since (in this case) that is a module name (i.e. the namespace of the `DescriptorState` type). The function being called here is `fmt` (the one outside the `<>`), which is a method of the `Debug` trait. We could perhaps reduce this down to: rust_binder::range_alloc::DescriptorState::fmt without much ambiguity (in most cases). Cheers, Miguel
Re: [PATCH 04/13] Kbuild: Rust support
Hi David, On Mon, Apr 19, 2021 at 10:01 PM David Sterba wrote: > > for simple functions it's barely parseable but the following is hardly > readable > > translates to > >core[8787f43e282added]::fmt::Debug>::fmt Some details can be omitted without much problem, e.g. try the `-i` option of `c++filt`: ::fmt Cheers, Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Mon, Apr 19, 2021 at 09:58:51PM +0200, David Sterba wrote: > On Fri, Apr 16, 2021 at 07:34:51PM +0200, Miguel Ojeda wrote: > > something like: > > > > [0.903456] rust_begin_unwind+0x9/0x10 > > [0.903456] ? _RNvNtCsbDqzXfLQacH_4core9panicking9panic_fmt+0x29/0x30 > > [0.903456] ? _RNvNtCsbDqzXfLQacH_4core9panicking5panic+0x44/0x50 > > [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1h+0x1c/0x20 > > [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1g+0x9/0x10 > > [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1f+0x9/0x10 > > [0.903456] ? > > _RNvXCsbDqzXfLQacH_12rust_minimalNtB2_11RustMinimalNtCsbDqzXfLQacH_6kernel12KernelModule4init+0x73/0x80 > > [0.903456] ? > > _RNvXsa_NtCsbDqzXfLQacH_4core3fmtbNtB5_5Debug3fmt+0x30/0x30 > > [0.903456] ? __rust_minimal_init+0x11/0x20 > > Are there plans to unmangle the symbols when printing stacks? c++filt > says: > > rust_begin_unwind+0x9/0x10 > ? core[8787f43e282added]::panicking::panic_fmt+0x29/0x30 > ? core[8787f43e282added]::panicking::panic+0x44/0x50 > ? rust_minimal[8787f43e282added]::h+0x1c/0x20 > ? rust_minimal[8787f43e282added]::g+0x9/0x10 > ? rust_minimal[8787f43e282added]::f+0x9/0x10 > ? kernel[8787f43e282added]::KernelModule>::init+0x73/0x80 > ? ::fmt+0x30/0x30 > ? __rust_minimal_init+0x11/0x20 > > for simple functions it's barely parseable but the following is hardly > readable > > > _RNvXs5_NtCsbDqzXfLQacH_11rust_binder11range_allocNtB5_15DescriptorStateNtNtCsbDqzXfLQacH_4core3fmt5Debug3fmt+0x60/0x60 > > translates to > >core[8787f43e282added]::fmt::Debug>::fmt Yes, I agree, we need a better story for name mangling. My proposal is that we store a pretty name which matches the source (eg rust_binder::range_alloc) and a sha1 of the mangled symbol (40 bytes of uninteresting hex). Symbol resolution is performed against the sha1. Printing is of the pretty name. It should be obvious from the stack trace which variant of a function is being called, no?
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 07:34:51PM +0200, Miguel Ojeda wrote: > On Fri, Apr 16, 2021 at 3:38 PM Peter Zijlstra wrote: > > > > So if I read all this right, rust compiles to .o and, like any other .o > > file is then fed into objtool (for x86_64). Did you have any problems > > with objtool? Does it generate correct ORC unwind information? > > I opened an issue a while ago to take a closer look at the ORC > unwinder etc., so it is in my radar (thanks for raising it up, > nevertheless!). > > Currently, causing a panic in a nested non-inlined function (f -> g -> > h) in one of the samples with the ORC unwinder enabled gives me > something like: > > [0.903456] rust_begin_unwind+0x9/0x10 > [0.903456] ? _RNvNtCsbDqzXfLQacH_4core9panicking9panic_fmt+0x29/0x30 > [0.903456] ? _RNvNtCsbDqzXfLQacH_4core9panicking5panic+0x44/0x50 > [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1h+0x1c/0x20 > [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1g+0x9/0x10 > [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1f+0x9/0x10 > [0.903456] ? > _RNvXCsbDqzXfLQacH_12rust_minimalNtB2_11RustMinimalNtCsbDqzXfLQacH_6kernel12KernelModule4init+0x73/0x80 > [0.903456] ? _RNvXsa_NtCsbDqzXfLQacH_4core3fmtbNtB5_5Debug3fmt+0x30/0x30 > [0.903456] ? __rust_minimal_init+0x11/0x20 Are there plans to unmangle the symbols when printing stacks? c++filt says: rust_begin_unwind+0x9/0x10 ? core[8787f43e282added]::panicking::panic_fmt+0x29/0x30 ? core[8787f43e282added]::panicking::panic+0x44/0x50 ? rust_minimal[8787f43e282added]::h+0x1c/0x20 ? rust_minimal[8787f43e282added]::g+0x9/0x10 ? rust_minimal[8787f43e282added]::f+0x9/0x10 ? ::init+0x73/0x80 ? ::fmt+0x30/0x30 ? __rust_minimal_init+0x11/0x20 for simple functions it's barely parseable but the following is hardly readable > _RNvXs5_NtCsbDqzXfLQacH_11rust_binder11range_allocNtB5_15DescriptorStateNtNtCsbDqzXfLQacH_4core3fmt5Debug3fmt+0x60/0x60 translates to ::fmt
Re: [PATCH 04/13] Kbuild: Rust support
On Thu, Apr 15, 2021 at 9:43 AM Miguel Ojeda
wrote:
>
> On Thu, Apr 15, 2021 at 1:19 AM Nick Desaulniers
> wrote:
> >
> > Rather than check the origin (yikes, are we intentionally avoiding env
> > vars?), can this simply be
> > ifneq ($(CLIPPY),)
> > KBUILD_CLIPPY := $(CLIPPY)
> > endif
> >
> > Then you can specify whatever value you want, support command line or
> > env vars, etc.?
>
> I was following the other existing cases like `V`. Masahiro can
> probably answer why they are done like this.
You are asking about this code:
ifeq ("$(origin V)", "command line")
KBUILD_VERBOSE = $(V)
endif
You can pass V=1 from the Make command line,
but not from the environment.
KBUILD_VERBOSE is intended as an environment variable,
but you can use it from the Make command line.
Work:
- make V=1
- make KBUILD_VERBOSE=1
- KBUILD_VERBOSE=1 make
Not work:
- V=1 make
The behavior is like that before I became the maintainer.
In my best guess, the reason is,
V=1 is a useful shorthand of KBUILD_VERBOSE=1,
but it is too short. It should not accidentally
pick up an unintended environment variable.
--
Best Regards
Masahiro Yamada
Re: [PATCH 04/13] Kbuild: Rust support
On Sat, Apr 17, 2021 at 6:24 AM Willy Tarreau wrote: > > My concern was to know what field to look at to reliably detect an error > from the C side after a sequence doing C -> Rust -> C when the inner C > code uses NULL to mark an error and the upper C code uses NULL as a valid > value and needs to look at an error code instead to rebuild a result. But I see, thanks for clarifying. I don't think we want to change anything on either of the C sides (at least for the foreseeable future). So the Rust code in-between must respect whatever conventions both C sides already use, even if they happen to be different on each side. Thus the C side will not know there was a `Result` inside the Rust side and so it does not need to worry about which field to look at. Cheers, Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Sat, Apr 17, 2021 at 01:46:35AM +0200, Miguel Ojeda wrote:
> On Sat, Apr 17, 2021 at 12:04 AM Willy Tarreau wrote:
> >
> > But my point remains that the point of extreme care is at the interface
> > with the rest of the kernel because there is a change of semantics
> > there.
> >
> > Sure but as I said most often (due to API or ABI inheritance), both
> > are already exclusive and stored as ranges. Returning 1..4095 for
> > errno or a pointer including NULL for a success doesn't shock me at
> > all.
>
> At the point of the interface we definitely need to take care of
> converting properly, but for Rust-to-Rust code (i.e. the ones using
> `Result` etc.), that would not be a concern.
Sure.
> Just to ensure I understood your concern, for instance, in this case
> you mentioned:
>
>result.status = foo_alloc();
>if (!result.status) {
>result.error = -ENOMEM;
>return result;
>}
Yes I mentioned this when it was my understanding that the composite
result returned was made both of a pointer and an error code, but Connor
explained that it was in fact more of a selector and a union.
> Is your concern is that the caller would mix up the `status` with the
> `error`, basically bubbling up the `status` as an `int` and forgetting
> about the `error`, and then someone else later understanding that
> `int` as a non-error because it is non-negative?
My concern was to know what field to look at to reliably detect an error
from the C side after a sequence doing C -> Rust -> C when the inner C
code uses NULL to mark an error and the upper C code uses NULL as a valid
value and needs to look at an error code instead to rebuild a result. But
if it's more:
if (result.ok)
return result.pointer;
else
return (void *)-result.error;
then it shouldn't be an issue.
Willy
Re: [PATCH 04/13] Kbuild: Rust support
On Sat, Apr 17, 2021 at 12:04 AM Willy Tarreau wrote:
>
> But my point remains that the point of extreme care is at the interface
> with the rest of the kernel because there is a change of semantics
> there.
>
> Sure but as I said most often (due to API or ABI inheritance), both
> are already exclusive and stored as ranges. Returning 1..4095 for
> errno or a pointer including NULL for a success doesn't shock me at
> all.
At the point of the interface we definitely need to take care of
converting properly, but for Rust-to-Rust code (i.e. the ones using
`Result` etc.), that would not be a concern.
Just to ensure I understood your concern, for instance, in this case
you mentioned:
result.status = foo_alloc();
if (!result.status) {
result.error = -ENOMEM;
return result;
}
Is your concern is that the caller would mix up the `status` with the
`error`, basically bubbling up the `status` as an `int` and forgetting
about the `error`, and then someone else later understanding that
`int` as a non-error because it is non-negative?
If yes: in C, yes, that could be a concern (if done with raw `int`s).
In Rust, if you get an `Err(ENOMEM)` from somebody, you cannot easily
conflate it with another type and return it by mistake because it is
more strictly typed than C.
Cheers,
Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Sat, Apr 17, 2021 at 12:04:16AM +0200, Willy Tarreau wrote: > Yep but I kept it just to have comparable output code since in C > you'd simply use "goto leave" and not have this function call to > do the cleanup. ... or use any number of other technics; the real question was how much of cleanups would be skipped by that syntax sugar. IME anything that interacts with flow control should be as explicit and unambiguous as possible. _Especially_ concerning how large a scope are we leaving. There's a bunch of disciplines that make use of that kind of tools and do it more or less safely, but they need to be well-specified and very well understood. And some tools (C++-style exceptions, for example) simply need to be taken out and shot, but that's a separate story^Wflamewar...
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 11:39:00PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 16, 2021 at 10:58 PM Willy Tarreau wrote:
> >
> > No, two:
> > - ok in %rax (seems like it's "!ok" technically speaking since it
> > returns 1 on !ok and 0 on ok)
> > - foo_or_err in %rdx
>
> Yes, but that is the implementation -- conceptually you only have one
> or the other, and Rust won't allow you to use the wrong one.
OK so for unions you always pass two values along the whole chain, a
selector and the value itself.
But my point remains that the point of extreme care is at the interface
with the rest of the kernel because there is a change of semantics
there.
> > However then I'm bothered because Miguel's example showed that regardless
> > of OK, EINVAL was always returned in foo_or_err, so maybe it's just
> > because his example was not well chosen but it wasn't very visible from
> > the source:
>
> That is the optimizer being fancy since the error can be put
> unconditionally in `rdx`.
Yes that's what I understood as well. I just didn't know that it had
to be seen as a union.
On Fri, Apr 16, 2021 at 11:19:18PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 16, 2021 at 10:22 PM Willy Tarreau wrote:
> >
> > So it simply does the equivalent of:
> >
> > struct result {
> > int status;
> > int error;
> > };
>
> Not exactly, it is more like a tagged union, as Connor mentioned.
>
> However, and this is the critical bit: it is a compile-time error to
> access the inactive variants (in safe code). In C, it is on you to
> keep track which one is the current one.
Sure but as I said most often (due to API or ABI inheritance), both
are already exclusive and stored as ranges. Returning 1..4095 for
errno or a pointer including NULL for a success doesn't shock me at
all.
Along thes lines I hardly see how you'd tag pointers by manipulating
their lower unused bits. That's something important both for memory
usage and performance (supports atomic opts).
> > kill_foo(); // only for rust, C doesn't need it
>
> Please note that `kill_foo()` is not needed in Rust -- it was an
> example of possible cleanup (since Al mentioned resources/cleanup)
> using RAII.
Yep but I kept it just to have comparable output code since in C
you'd simply use "goto leave" and not have this function call to
do the cleanup.
Willy
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 10:58 PM Willy Tarreau wrote:
>
> No, two:
> - ok in %rax (seems like it's "!ok" technically speaking since it
> returns 1 on !ok and 0 on ok)
> - foo_or_err in %rdx
Yes, but that is the implementation -- conceptually you only have one
or the other, and Rust won't allow you to use the wrong one.
> However then I'm bothered because Miguel's example showed that regardless
> of OK, EINVAL was always returned in foo_or_err, so maybe it's just
> because his example was not well chosen but it wasn't very visible from
> the source:
That is the optimizer being fancy since the error can be put
unconditionally in `rdx`.
If you compile:
pub fn it_is_ok() -> KernelResult {
Ok(Bar)
}
you will see it just clears `rax`.
Cheers,
Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 10:22 PM Willy Tarreau wrote:
>
> So it simply does the equivalent of:
>
> struct result {
> int status;
> int error;
> };
Not exactly, it is more like a tagged union, as Connor mentioned.
However, and this is the critical bit: it is a compile-time error to
access the inactive variants (in safe code). In C, it is on you to
keep track which one is the current one.
> kill_foo(); // only for rust, C doesn't need it
Please note that `kill_foo()` is not needed in Rust -- it was an
example of possible cleanup (since Al mentioned resources/cleanup)
using RAII.
Cheers,
Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 03:34:50PM -0500, Connor Kuehl wrote:
> On 4/16/21 3:22 PM, Willy Tarreau wrote:
> > So it simply does the equivalent of:
> >
> > #define EINVAL -1234
> >
> > struct result {
> > int status;
> > int error;
> > };
>
> Result and Option types are more like a union with a tag that
> describes which variant it is.
>
> struct foo_result {
> /* if ok, then access foo_or_err.successful_foo
> *else, access foo_or_err.error
> */
> bool ok;
> union {
> struct foo successful_foo;
> int error;
> } foo_or_err;
> };
OK.
> > [..]
> >
> > So it simply returns a pair of values instead of a single one, which
>
> It will only return 1 value.
No, two:
- ok in %rax (seems like it's "!ok" technically speaking since it
returns 1 on !ok and 0 on ok)
- foo_or_err in %rdx
However then I'm bothered because Miguel's example showed that regardless
of OK, EINVAL was always returned in foo_or_err, so maybe it's just
because his example was not well chosen but it wasn't very visible from
the source:
bar:
pushrbx
mov ebx, 1
callqword ptr [rip + black_box@GOTPCREL]
testal, al
jne .LBB2_2
callqword ptr [rip + kill_foo@GOTPCREL]
xor ebx, ebx
.LBB2_2:
mov eax, ebx
mov edx, -1234
pop rbx
ret
Willy
Re: [PATCH 04/13] Kbuild: Rust support
On 4/16/21 3:22 PM, Willy Tarreau wrote:
> So it simply does the equivalent of:
>
> #define EINVAL -1234
>
> struct result {
> int status;
> int error;
> };
Result and Option types are more like a union with a tag that
describes which variant it is.
struct foo_result {
/* if ok, then access foo_or_err.successful_foo
*else, access foo_or_err.error
*/
bool ok;
union {
struct foo successful_foo;
int error;
} foo_or_err;
};
> [..]
>
> So it simply returns a pair of values instead of a single one, which
It will only return 1 value.
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 08:57:07PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 16, 2021 at 8:10 PM Al Viro wrote:
> >
> > How well would ? operator fit that pattern? _If_ it's just a syntax sugar
> > along the lines of "if argument matches Err(_), return Err(_)", the types
> > shouldn't be an issue, but that might need some fun with releasing
> > resources,
> > etc. If it's something more elaborate... details, please.
>
> Yes, it is just syntax sugar -- it doesn't introduce any power to the
> language.
>
> It was introduced because it is a very common pattern when using the
> `Result` and `Option` enums. In fact, before it existed, it was just a
> simple macro that you could also implement yourself.
>
> For instance, given `Foo` and `Bar` types that need RAII cleanup of
> some kind (let's say `kill_foo()` and `kill_bar()`):
>
> fn foo() -> KernelResult {
> if black_box() {
> return Err(EINVAL);
> }
>
> // something that gets you a `Foo`
> let foo = ...;
>
> Ok(foo)
> }
>
> fn bar() -> KernelResult {
> let p = foo()?;
>
> // something that gets you a `Bar`, possibly using the `p`
> let bar = ...;
>
> Ok(bar)
> }
>
> This reduces to (full example at https://godbolt.org/z/hjTxd3oP1):
>
> bar:
> pushrbx
> mov ebx, 1
> callqword ptr [rip + black_box@GOTPCREL]
> testal, al
> jne .LBB2_2
> callqword ptr [rip + kill_foo@GOTPCREL]
> xor ebx, ebx
> .LBB2_2:
> mov eax, ebx
> mov edx, -1234
> pop rbx
> ret
>
> You can see `bar()` calls `black_box()`. If it failed, it returns the
> EINVAL. Otherwise, it cleans up the `foo` automatically and returns
> the successful `bar`.
So it simply does the equivalent of:
#define EINVAL -1234
struct result {
int status;
int error;
};
extern bool black_box();
extern void kill_foo();
struct result bar()
{
return (struct error){ !!black_box(), EINVAL };
}
struct result foo()
{
struct result res = bar();
if (res.status)
goto leave;
/* ... */
kill_foo(); // only for rust, C doesn't need it
leave:
return res;
}
So it simply returns a pair of values instead of a single one, which
is nothing special but not much conventional in the kernel either given
that ultimately syscalls will usually return a single value anyway. At
some point some code will have to remerge them to provide a composite
result and even take care of ambigous special cases like { true, 0 }
which may or may not indicate an error, and avoiding to consider the
unused error code on success.
For example some code called from mmap() might have to deal with this
this way:
if (result.status == (void *)-1)
return -result.error;
else
return result.status;
But possibly a lot of code feeding this result struct would be tempted
to do something like this to deal with NULL returns:
result.status = foo_alloc();
if (!result.status) {
result.error = -ENOMEM;
return result;
}
And suddenly the error is lost, as a NULL is returned to the upper layers
which does not correspond to an failure status. Conversely, with a unique
result you'd do something like this:
result = foo_alloc();
if (!result)
return -ENOMEM;
So it might possibly be safer to stick to the usually expected return
values instead of introducing composite ones.
I tend to agree that composite results can be better from new projects
started from scratch when all the API follows this principle, but here
there's quite a massive code base that was not designed along these
lines and I easily see how this can become a source of trouble over
time.
Willy
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 8:10 PM Al Viro wrote:
>
> How well would ? operator fit that pattern? _If_ it's just a syntax sugar
> along the lines of "if argument matches Err(_), return Err(_)", the types
> shouldn't be an issue, but that might need some fun with releasing resources,
> etc. If it's something more elaborate... details, please.
Yes, it is just syntax sugar -- it doesn't introduce any power to the language.
It was introduced because it is a very common pattern when using the
`Result` and `Option` enums. In fact, before it existed, it was just a
simple macro that you could also implement yourself.
For instance, given `Foo` and `Bar` types that need RAII cleanup of
some kind (let's say `kill_foo()` and `kill_bar()`):
fn foo() -> KernelResult {
if black_box() {
return Err(EINVAL);
}
// something that gets you a `Foo`
let foo = ...;
Ok(foo)
}
fn bar() -> KernelResult {
let p = foo()?;
// something that gets you a `Bar`, possibly using the `p`
let bar = ...;
Ok(bar)
}
This reduces to (full example at https://godbolt.org/z/hjTxd3oP1):
bar:
pushrbx
mov ebx, 1
callqword ptr [rip + black_box@GOTPCREL]
testal, al
jne .LBB2_2
callqword ptr [rip + kill_foo@GOTPCREL]
xor ebx, ebx
.LBB2_2:
mov eax, ebx
mov edx, -1234
pop rbx
ret
You can see `bar()` calls `black_box()`. If it failed, it returns the
EINVAL. Otherwise, it cleans up the `foo` automatically and returns
the successful `bar`.
Cheers,
Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 07:47:32PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 16, 2021 at 7:05 PM Linus Torvalds
> wrote:
> >
> > Typical Rust error handling should match the regular kernel
> > IS_ERR/ERR_PTR/PTR_ERR model fairly well, although the syntax is
> > fairly different (and it's not limited to pointers).
>
> Yeah, exactly. We already have a `KernelResult` type which is a
> `Result`, where `Error` is a wrapper for the usual kernel
> int errors.
>
> So, for instance, a function that can either fail or return `Data`
> would have a declaration like:
>
> pub fn foo() -> KernelResult
>
> A caller that needs to handle the error can use pattern matching or
> one of the methods in `Result`. And if they only need to bubble the
> error up, they can use the ? operator:
>
> pub fn bar() -> KernelResult {
> let data = foo()?;
>
> // `data` is already a `Data` here, not a `KernelResult`
> }
Umm... A fairly common situation is
foo() returns a pointer to struct foo instance or ERR_PTR()
bar() returns a pointer to struct bar instance of ERR_PTR()
bar()
{
struct foo *p;
struct bar *res;
// do some work, grab a mutex, etc.
p = foo();
if (IS_ERR(p))
res = ERR_CAST(p); // (void *)p, internally; conceptually it's
// ERR_PTR(PTR_ERR(p));
else
res = blah();
// matching cleanup
return res;
}
How well would ? operator fit that pattern? _If_ it's just a syntax sugar
along the lines of "if argument matches Err(_), return Err(_)", the types
shouldn't be an issue, but that might need some fun with releasing resources,
etc. If it's something more elaborate... details, please.
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 7:05 PM Linus Torvalds
wrote:
>
> Typical Rust error handling should match the regular kernel
> IS_ERR/ERR_PTR/PTR_ERR model fairly well, although the syntax is
> fairly different (and it's not limited to pointers).
Yeah, exactly. We already have a `KernelResult` type which is a
`Result`, where `Error` is a wrapper for the usual kernel
int errors.
So, for instance, a function that can either fail or return `Data`
would have a declaration like:
pub fn foo() -> KernelResult
A caller that needs to handle the error can use pattern matching or
one of the methods in `Result`. And if they only need to bubble the
error up, they can use the ? operator:
pub fn bar() -> KernelResult {
let data = foo()?;
// `data` is already a `Data` here, not a `KernelResult`
}
Cheers,
Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 3:38 PM Peter Zijlstra wrote: > > So if I read all this right, rust compiles to .o and, like any other .o > file is then fed into objtool (for x86_64). Did you have any problems > with objtool? Does it generate correct ORC unwind information? I opened an issue a while ago to take a closer look at the ORC unwinder etc., so it is in my radar (thanks for raising it up, nevertheless!). Currently, causing a panic in a nested non-inlined function (f -> g -> h) in one of the samples with the ORC unwinder enabled gives me something like: [0.903456] rust_begin_unwind+0x9/0x10 [0.903456] ? _RNvNtCsbDqzXfLQacH_4core9panicking9panic_fmt+0x29/0x30 [0.903456] ? _RNvNtCsbDqzXfLQacH_4core9panicking5panic+0x44/0x50 [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1h+0x1c/0x20 [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1g+0x9/0x10 [0.903456] ? _RNvCsbDqzXfLQacH_12rust_minimal1f+0x9/0x10 [0.903456] ? _RNvXCsbDqzXfLQacH_12rust_minimalNtB2_11RustMinimalNtCsbDqzXfLQacH_6kernel12KernelModule4init+0x73/0x80 [0.903456] ? _RNvXsa_NtCsbDqzXfLQacH_4core3fmtbNtB5_5Debug3fmt+0x30/0x30 [0.903456] ? __rust_minimal_init+0x11/0x20 But it also shows this one below: [0.903456] ? _RNvXs5_NtCsbDqzXfLQacH_11rust_binder11range_allocNtB5_15DescriptorStateNtNtCsbDqzXfLQacH_4core3fmt5Debug3fmt+0x60/0x60 So something does not look correct. > AFAICT rust has try/throw/catch exception handling (like > C++/Java/others) which is typically implemented with stack unwinding of > its own. Rust has optional unwinding for panics, yeah, but we don't enable it. Instead, we tell the compiler to use its "abort" panic strategy. In the handler currently we just call `BUG()`, but we will need to do something less aggressive (e.g. kill the current thread). But please note that it is not our plan to rely on panics for normal error conditions. For instance, the allocations currently panic due to our re-use of `alloc`, but will be fallible eventually (`Result` etc.). Cheers, Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Fri, Apr 16, 2021 at 6:38 AM Peter Zijlstra wrote: > > AFAICT rust has try/throw/catch exception handling (like > C++/Java/others) which is typically implemented with stack unwinding of > its own. I was assuming that the kernel side would never do that. There's some kind of "catch_unwind()" thing that catches a Rust "panic!" thing, but I think it's basically useless for the kernel. Typical Rust error handling should match the regular kernel IS_ERR/ERR_PTR/PTR_ERR model fairly well, although the syntax is fairly different (and it's not limited to pointers). Linus
Re: [PATCH 04/13] Kbuild: Rust support
On Wed, Apr 14, 2021 at 08:45:55PM +0200, oj...@kernel.org wrote: > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 1b6094a13034..3665c49c4dcf 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -26,6 +26,7 @@ EXTRA_CPPFLAGS := > EXTRA_LDFLAGS := > asflags-y := > ccflags-y := > +rustcflags-y := > cppflags-y := > ldflags-y := > > @@ -287,6 +288,24 @@ quiet_cmd_cc_lst_c = MKLST $@ > $(obj)/%.lst: $(src)/%.c FORCE > $(call if_changed_dep,cc_lst_c) > > +# Compile Rust sources (.rs) > +# --- > + > +rustc_cross_flags := --target=$(srctree)/arch/$(SRCARCH)/rust/target.json > + > +quiet_cmd_rustc_o_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@ > + cmd_rustc_o_rs = \ > + RUST_MODFILE=$(modfile) \ > + $(RUSTC_OR_CLIPPY) $(rustc_flags) $(rustc_cross_flags) \ > + --extern alloc --extern kernel \ > + --crate-type rlib --out-dir $(obj) -L $(objtree)/rust/ \ > + --crate-name $(patsubst %.o,%,$(notdir $@)) $<; \ > + mv $(obj)/$(subst .o,,$(notdir $@)).d $(depfile); \ > + sed -i '/^\#/d' $(depfile) > + > +$(obj)/%.o: $(src)/%.rs FORCE > + $(call if_changed_dep,rustc_o_rs) > + > # Compile assembler sources (.S) > # --- > So if I read all this right, rust compiles to .o and, like any other .o file is then fed into objtool (for x86_64). Did you have any problems with objtool? Does it generate correct ORC unwind information? AFAICT rust has try/throw/catch exception handling (like C++/Java/others) which is typically implemented with stack unwinding of its own. Does all that interact sanely?
Re: [PATCH 04/13] Kbuild: Rust support
On Thu, Apr 15, 2021 at 8:03 PM Nick Desaulniers wrote: > > Until then, I don't see why we need to permit developers to express > such flexibility for just the Rust code, or have it differ from the > intent of the C code. Does it make sense to set RUST_OPT_LEVEL_3 and > CC_OPTIMIZE_FOR_SIZE? I doubt it. That doesn't seem like a development > feature, but a mistake. YAGNI. Instead developers should clarify > what they care about in terms of high level intent; if someone wants > to micromanage optimization level flags in their forks they don't need > a Kconfig to do it (they're either going to hack KBUILD_CFLAGS, > CFLAGS_*.o, or KCFLAGS), and there's probably better mechanisms for > fine-tooth precision of optimizing actually hot code or not via PGO > and AutoFDO. I completely agree when we are talking about higher level optimization levels. From a user perspective, it does not make much sense to want slightly different optimizations levels or different size/performance trade-offs between C and Rust. However, I am thinking from the debugging side, i.e. mostly low or no optimization; rather than about micromanaging optimizations for performance. For instance, last year I used `RUST_OPT_LEVEL_0/1` to quickly rule out optimizer/codegen/etc. bugs on the Rust side when we had some memory corruption over Rust data (https://github.com/Rust-for-Linux/linux/pull/28), which is important when dealing with compiler nightly versions. It was also nice to be able to easily follow along when stepping, too. Having said that, I agree that in those cases one can simply tweak the flags manually -- so that's why I said it is fine dropping the the `Kconfig` options. There might be some advantages of having them, such as making developers aware that those builds should work, to keep them tested/working, etc.; but we can do that manually too in the CI/docs too. Cheers, Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Wed, Apr 14, 2021 at 5:43 PM Miguel Ojeda wrote: > > On Thu, Apr 15, 2021 at 1:19 AM Nick Desaulniers > wrote: > > > > -Oz in clang typically generates larger kernel code than -Os; LLVM > > seems to aggressively emit libcalls even when the setup for a call > > would be larger than the inlined call itself. Is z smaller than s for > > the existing rust examples? > > I will check if the `s`/`z` flags have the exact same semantics as > they do in Clang, but as a quick test (quite late here, sorry!), yes, > it seems `z` is smaller: > > text data bssdec hex filename > > 1265688 104 126680 1eed8 drivers/android/rust_binder.o [s] > 1229238 104 123035 1e09b drivers/android/rust_binder.o [z] > > 2123510 0 212351 33d7f rust/core.o [s] > 2076530 0 207653 32b25 rust/core.o [z] cool, thanks for verifying. LGTM > > This is a mess; who thought it would be a good idea to support > > compiling the rust code at a different optimization level than the > > rest of the C code in the kernel? Do we really need that flexibility > > for Rust kernel code, or can we drop this feature? > > I did :P > > The idea is that, since it seemed to work out of the box when I tried, > it could be nice to keep for debugging and for having another degree > of freedom when testing the compiler/nightlies etc. > > Also, it is not intended for users, which is why I put it in the > "hacking" menu -- users should still only modify the usual global > option. > > However, it is indeed strange for the kernel and I don't mind dropping > it if people want to see it out (one could still do it manually if > needed...). > > (Related: from what I have been told, the kernel does not support > lower levels in C just due to old problems with compilers; but those > may be gone now). IIRC the kernel (or at least x86_64 defconfig) cannot be built at -O0, which is too bad if developers were myopically focused on build times. It would have been nice to have something like CONFIG_CC_OPTIMIZE_FOR_COMPILE_TIME to join CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE and CONFIG_CC_OPTIMIZE_FOR_SIZE, but maybe it's still possible to support one day. (¿Por qué no los tres? Perhaps a false-trichotomy? Sorry, but those 3 are somewhat at odds for compilation). Until then, I don't see why we need to permit developers to express such flexibility for just the Rust code, or have it differ from the intent of the C code. Does it make sense to set RUST_OPT_LEVEL_3 and CC_OPTIMIZE_FOR_SIZE? I doubt it. That doesn't seem like a development feature, but a mistake. YAGNI. Instead developers should clarify what they care about in terms of high level intent; if someone wants to micromanage optimization level flags in their forks they don't need a Kconfig to do it (they're either going to hack KBUILD_CFLAGS, CFLAGS_*.o, or KCFLAGS), and there's probably better mechanisms for fine-tooth precision of optimizing actually hot code or not via PGO and AutoFDO. https://lore.kernel.org/lkml/20210407211704.367039-1-mo...@google.com/ -- Thanks, ~Nick Desaulniers
Re: [PATCH 04/13] Kbuild: Rust support
On Thu, Apr 15, 2021 at 1:19 AM Nick Desaulniers wrote: > > Rather than check the origin (yikes, are we intentionally avoiding env > vars?), can this simply be > ifneq ($(CLIPPY),) > KBUILD_CLIPPY := $(CLIPPY) > endif > > Then you can specify whatever value you want, support command line or > env vars, etc.? I was following the other existing cases like `V`. Masahiro can probably answer why they are done like this. > -Oz in clang typically generates larger kernel code than -Os; LLVM > seems to aggressively emit libcalls even when the setup for a call > would be larger than the inlined call itself. Is z smaller than s for > the existing rust examples? I will check if the `s`/`z` flags have the exact same semantics as they do in Clang, but as a quick test (quite late here, sorry!), yes, it seems `z` is smaller: text data bssdec hex filename 1265688 104 126680 1eed8 drivers/android/rust_binder.o [s] 1229238 104 123035 1e09b drivers/android/rust_binder.o [z] 2123510 0 212351 33d7f rust/core.o [s] 2076530 0 207653 32b25 rust/core.o [z] > This is a mess; who thought it would be a good idea to support > compiling the rust code at a different optimization level than the > rest of the C code in the kernel? Do we really need that flexibility > for Rust kernel code, or can we drop this feature? I did :P The idea is that, since it seemed to work out of the box when I tried, it could be nice to keep for debugging and for having another degree of freedom when testing the compiler/nightlies etc. Also, it is not intended for users, which is why I put it in the "hacking" menu -- users should still only modify the usual global option. However, it is indeed strange for the kernel and I don't mind dropping it if people want to see it out (one could still do it manually if needed...). (Related: from what I have been told, the kernel does not support lower levels in C just due to old problems with compilers; but those may be gone now). > Don't the target.json files all set `"eliminate-frame-pointer": > false,`? Is this necessary then? Also, which targets retain frame > pointers at which optimization levels is quite messy (in C compilers), > as well as your choice of unwinder, which is configurable for certain > architectures. For this (and other questions regarding the target definition files you have below): the situation is quite messy, indeed. Some of these options can be configured via flags too. Also, the target definition files are actually unstable in `rustc` because they are too tied to LLVM. So AFAIK if a command-line flag exists, we should use that. But I am not sure if the target definition file is supposed to support removing keys etc. Anyway, the plan here short-term is to generate the target definition file on the fly taking into account the options, and keep it working w.r.t. `rustc` nightlies (this is also why we don't have have big endian for ppc or 32-bit x86). Longer-term, hopefully `rustc` adds enough command-line flags to tweak as needed, or stabilizes the target files somehow, or stabilizes all the target combinations we need (i.e. as built-in targets in the compiler). In fact, I will add this to the "unstable features" list. > Seems like a good way for drive by commits where someone reformatted > the whole kernel. We enforce the formatting for all the code at the moment in the CI and AFAIK `rustfmt` tries to keep formatting stable (as long as one does not use unstable features), so code should always be formatted. Having said that, I'm not sure 100% how stable it actually is in practice over long periods of time -- I guess we will find out soon enough. > Might be nice to invoke this somehow from checkpatch.pl somehow for > changes to rust source files. Not necessary in the RFC, but perhaps > one day. We do it in the CI (see above). > Yuck. This should be default on and not configurable. See above for the opt-levels. Cheers, Miguel
Re: [PATCH 04/13] Kbuild: Rust support
On Wed, Apr 14, 2021 at 11:48 AM wrote: > > From: Miguel Ojeda > > This commit includes also the `Kconfig` entries related to Rust, > the Rust configuration printer, the target definition files, > the version detection script and a few other bits. > > In the future, we will likely want to generate the target files > on the fly via a script. > > With this in place, we will be adding the rest of the bits piece > by piece and enabling their builds. > > Co-developed-by: Alex Gaynor > Signed-off-by: Alex Gaynor > Co-developed-by: Geoffrey Thomas > Signed-off-by: Geoffrey Thomas > Co-developed-by: Finn Behrens > Signed-off-by: Finn Behrens > Co-developed-by: Adam Bratschi-Kaye > Signed-off-by: Adam Bratschi-Kaye > Co-developed-by: Wedson Almeida Filho > Signed-off-by: Wedson Almeida Filho > Co-developed-by: Michael Ellerman > Signed-off-by: Michael Ellerman > Signed-off-by: Miguel Ojeda > --- > .gitignore| 2 + > .rustfmt.toml | 12 +++ > Documentation/kbuild/kbuild.rst | 4 + > Documentation/process/changes.rst | 9 ++ > Makefile | 120 +++-- > arch/arm64/rust/target.json | 40 + > arch/powerpc/rust/target.json | 30 +++ > arch/x86/rust/target.json | 42 + > init/Kconfig | 27 ++ > lib/Kconfig.debug | 100 + > rust/.gitignore | 5 ++ > rust/Makefile | 141 ++ > scripts/Makefile.build| 19 > scripts/Makefile.lib | 12 +++ > scripts/kconfig/confdata.c| 67 +- > scripts/rust-version.sh | 31 +++ > 16 files changed, 654 insertions(+), 7 deletions(-) > create mode 100644 .rustfmt.toml > create mode 100644 arch/arm64/rust/target.json > create mode 100644 arch/powerpc/rust/target.json > create mode 100644 arch/x86/rust/target.json > create mode 100644 rust/.gitignore > create mode 100644 rust/Makefile > create mode 100755 scripts/rust-version.sh > > diff --git a/.gitignore b/.gitignore > index 3af66272d6f1..6ba4f516f46c 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -37,6 +37,7 @@ > *.o > *.o.* > *.patch > +*.rmeta > *.s > *.so > *.so.dbg > @@ -96,6 +97,7 @@ modules.order > !.gitattributes > !.gitignore > !.mailmap > +!.rustfmt.toml > > # > # Generated include files > diff --git a/.rustfmt.toml b/.rustfmt.toml > new file mode 100644 > index ..5893c0e3cbde > --- /dev/null > +++ b/.rustfmt.toml > @@ -0,0 +1,12 @@ > +edition = "2018" > +format_code_in_doc_comments = true > +newline_style = "Unix" > + > +# Unstable options that help catching some mistakes in formatting and that > we may want to enable > +# when they become stable. > +# > +# They are kept here since they are useful to run from time to time. > +#reorder_impl_items = true > +#comment_width = 100 > +#wrap_comments = true > +#normalize_comments = true > diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst > index 2d1fc03d346e..1109d18d9377 100644 > --- a/Documentation/kbuild/kbuild.rst > +++ b/Documentation/kbuild/kbuild.rst > @@ -57,6 +57,10 @@ CFLAGS_MODULE > - > Additional module specific options to use for $(CC). > > +KRUSTCFLAGS > +--- > +Additional options to the Rust compiler (for built-in and modules). > + > LDFLAGS_MODULE > -- > Additional options used for $(LD) when linking modules. > diff --git a/Documentation/process/changes.rst > b/Documentation/process/changes.rst > index dac17711dc11..4b6ba5458706 100644 > --- a/Documentation/process/changes.rst > +++ b/Documentation/process/changes.rst > @@ -31,6 +31,8 @@ you probably needn't concern yourself with pcmciautils. > == === > > GNU C 4.9 gcc --version > Clang/LLVM (optional) 10.0.1 clang --version > +rustc (optional) nightly rustc --version > +bindgen (optional) 0.56.0 bindgen --version > GNU make 3.81 make --version > binutils 2.23 ld -v > flex 2.5.35 flex --version > @@ -56,6 +58,7 @@ iptables 1.4.2iptables -V > openssl & libcrypto1.0.0openssl version > bc 1.06.95 bc --version > Sphinx\ [#f1]_1.3 sphinx-build --version > +rustdoc (optional) nightly rustdoc --version > == === > > > .. [#f1] Sphinx is needed only to build the Kernel documentation > @@ -330,6 +333,12 @@ Sphinx > Please see :ref:`sphinx_install` in :ref:`Documentation/doc-guide/sphinx.rst > ` > for details about Sphinx requirements. > > +rustdoc > +--- > + > +``rustdoc`` is
