Improvement to Error by fbq · Pull Request #267 · Rust-for-Linux/linux

fbq · 2021-05-13T12:13:22Z

Allow Result::Expect to work
Allow converting a pointer to Error

So that `Result::expect` can be used. Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

fbq · 2021-05-13T12:15:10Z

+/// Kernel pointers can be used to store error values (see
+/// [`Error::from_kernel_ptr`]), therefore this function returns `Err` if `ptr
+/// indicates an error, otherwise returns `Ok(ptr)`.
+#[allow(dead_code)]


Note: I have to make this as #[allow(dead_code)] because there is no real user since I separate this from my thread PR #109, this can be removed once we finish our discussion and have real user merged

TheSven73

Thank you for preparing this !

TheSven73 · 2021-05-13T12:38:26Z

+ ///
+ /// - [`core::usize::MAX - bindings::MAX_ERRNO`,..,`core::usize::MAX] is the
+ /// range for error values stored in pointer.
+ pub fn from_kernel_ptr(ptr: *mut c_types::c_void) -> Option<Error> {


The kernel pointer we want to convert from isn't necessarily a *mut c_void, right? It's always a pointer to a bindings:: type, or c_void. Casts are not idiomatic, so can we avoid the cast by using a template here? E.g. from_kernel_ptr<T>(ptr: *mut T).
Also, why have a pub function that returns Option<Error>? Where would this be used outside of error.rs? Would it be simpler to have a single pub ptr_to_result() function that returns Result<*mut T>?

The kernel pointer we want to convert from isn't necessarily a *mut c_void, right? It's always a pointer to a bindings:: type, or c_void. Casts are not idiomatic, so can we avoid the cast by using a template here? E.g. from_kernel_ptr<T>(ptr: *mut T).
Try to avoid template functions when possible. Would unnecessary template functions increase the code size? For example, if we have from_kernel_ptr<i32> and from_kernel_ptr<i8>, there will be two functions instead of one.
Also, why have a pub function that returns Option<Error>? Where would this be used outside of error.rs? Would it be simpler to have a single pub ptr_to_result() function that returns Result<*mut T>?
Oh, I just thought this might be useful, some one may use it in the future. I'm OK if we remove it.

Try to avoid template functions when possible. Would unnecessary template functions increase the code size? For example, if we have from_kernel_ptr<i32> and from_kernel_ptr<i8>, there will be two functions instead of one.
Perhaps the template instantiation would just get optimized away by LLVM? Maybe from_kernel_ptr<i32> and from_kernel_ptr<i8> will both result in the same function (inlined or not) which just calls IS_ERR() and ERR_PTR() ? Or perhaps one or two asm instructions if the kernel helpers are not used.
Maybe that's worth verifying? Premature optimization and roots of all evil etc :)

I took a quick look at the asm generated for this:
pubfninit_mmio_platform_resource(pdev:&mutPlatformDevice,index:u32,cfg:&RegmapConfig,) -> Result<Self>{let iomem = Self::devm_platform_ioremap_resource(pdev, index)?;Self::devm_regmap_init_mmio(pdev, iomem, cfg)}
where
fndevm_platform_ioremap_resource(pdev:&mutPlatformDevice,index:u32,) -> Result<*mut c_types::c_void>{// SAFETY: FFI call.unsafe{ptr_err_check(bindings::devm_platform_ioremap_resource( pdev.to_ptr(), index,))}}pub(crate)unsafefnptr_err_check<T>(ptr:*mutT) -> Result<*mutT>{ifrust_helper_is_err(ptr as_){returnErr(Error::from_kernel_errno(rust_helper_ptr_err(ptr as_)asi32));}Ok(ptr)}
and it does look like the ptr_err_check<T>() template function is simply optimized away (together with the Self:: helper):
0001407c <_RNvMs0_NtCsbDqzXfLQacH_6kernel6regmapNtB5_6Regmap27init_mmio_platform_resource>: 1407c: e92d4bf0 push {r4, r5, r6, r7,r8,r9, fp, lr}14080: e24dd0a8 subsp,sp, #168 ; 0xa814084: e1a04000 mov r4, r014088: e5900000 ldr r0,[r0] 1408c: e1a06002 mov r6, r214090: ebfffffe bl0 <devm_platform_ioremap_resource>14094: e1a05000 mov r5, r014098: ebfffffe bl0 <rust_helper_is_err> 1409c: e3500000 cmp r0, #0 140a0: 1a000029 bne 1414c <_RNvMs0_NtCsbDqzXfLQacH_6kernel6regmapNtB5_6Regmap27init_mmio_platform_resource+0xd0>

Note that I agree we need a template ptr_err_check or ptr_to_result, because the return type will use the template parameter, and that will empower us with some strong type checking. I also made ptr_to_result as a template function.
And if we think that from_kernel_ptr is worth discussing, I don't think the template parameter will buy us anything, it only appears at the input parameter, and the type information gets lost afterwards. So it's really unnecessary ;-(
Anyway thanks for looking into the generated code to prove I'm wrong ;-)

Anyway thanks for looking into the generated code to prove I'm wrong ;-)
I didn't want to imply that you were wrong - you were asking a question that I didn't know the answer to either (Would unnecessary template functions increase the code size?) and it sounded like an interesting question.
I apologize if it came across that way.

Anyway thanks for looking into the generated code to prove I'm wrong ;-)
I didn't want to imply that you were wrong - you were asking a question that I didn't know the answer to either (Would unnecessary template functions increase the code size?) and it sounded like an interesting question.
I apologize if it came across that way.
No need to apologize ;-) Actually I'm glad you looked into it.
Note that I actually kinda answered my question myself with "For example, if we have from_kernel_ptr and from_kernel_ptr, there will be two functions instead of one.", so literally, I was wrong and you simply showed the answer is opposite.

TheSven73 · 2021-05-13T12:44:08Z

+ let value = ptr as usize;
+
+ if value >= core::usize::MAX - bindings::MAX_ERRNO as usize {
+ Some(Error::from_kernel_errno(value as c_types::c_int))


Is casting pointers to c_int idiomatic? And aren't some pointers wider than c_int? Could we perhaps use the C function provided by the kernel, i.e. PTR_ERR()? We know it'll always work, even if the kernel encodes the errnos differently. And it would save us from having to carefully document/prove that these casts are safe.
(edited: added safety comment)

We have done the check on value, so it's guaranteed that it's safe to convert to c_int. As for using PTR_ERR(), as I said in another PR, we would have extra cost on function calls.

TheSven73 · 2021-05-13T12:45:34Z

+ pub fn from_kernel_ptr(ptr: *mut c_types::c_void) -> Option<Error> {
+ let value = ptr as usize;
+
+ if value >= core::usize::MAX - bindings::MAX_ERRNO as usize {


Does this expose a kernel implementation detail (i.e. the way errors are encoded inside pointers)? Would it be more safe to use the kernel function IS_ERR() here? We know it'll always work, even if the kernel encodes the errnos differently. And it would save us from having to carefully document/prove that the as usize casts are safe (i.e. won't overflow).
(edited: added safety comment)

I tend to agree (and mistakes are easy to make, e.g. see the missing + 1).
The as usize should both be fine (the first is from a pointer and the second could be put into a const variable, which I would do anyway).

ojeda · 2021-05-13T12:26:35Z

+/// Converts a kernel pointer to a [`Result`].
+///
+/// Kernel pointers can be used to store error values (see
+/// [`Error::from_kernel_ptr`]), therefore this function returns `Err` if `ptr


Suggested change
/// [`Error::from_kernel_ptr`]), therefore this function returns `Err` if `ptr
/// [`Error::from_kernel_ptr`]), therefore this function returns `Err` if `ptr`

ojeda · 2021-05-13T12:53:39Z

+ /// - [0, .., `core::usize::MAX - bindings::MAX_ERRNO`) is the range for
+ /// normal values of pointer.
+ ///
+ /// - [`core::usize::MAX - bindings::MAX_ERRNO`,..,`core::usize::MAX] is the


Suggested change
/// - [`core::usize::MAX - bindings::MAX_ERRNO`,..,`core::usize::MAX] is the
/// - [`core::usize::MAX - bindings::MAX_ERRNO`,..,`core::usize::MAX`] is the

ojeda · 2021-05-13T12:53:49Z

+ /// # Pointer value range
+ ///
+ /// According to `include/linux/err.h`:
+ /// - [0, .., `core::usize::MAX - bindings::MAX_ERRNO`) is the range for


Suggested change
/// - [0, .., `core::usize::MAX - bindings::MAX_ERRNO`) is the range for
/// - [`0`, .., `core::usize::MAX - bindings::MAX_ERRNO`) is the range for

ojeda · 2021-05-13T13:28:48Z

+ pub fn from_kernel_ptr(ptr: *mut c_types::c_void) -> Option<Error> {
+ let value = ptr as usize;
+
+ if value >= core::usize::MAX - bindings::MAX_ERRNO as usize {


Is this correct? core::usize::MAX - bindings::MAX_ERRNO as usize is -4096, rather than -4095.
Suggested change
if value >= core::usize::MAX - bindings::MAX_ERRNOasusize{
if value >= core::usize::MAX - bindings::MAX_ERRNOasusize+ 1{

I think you're right. This is indeed an embarrassing bug, and prove the point ;-)

ojeda · 2021-05-13T13:42:04Z

From #254:

In general, I think we want to go for re-using the C functions/macros where possible. Performance work should be based on actual numbers, which we can focus on later. And, anyway, we should be able to have cross-language LTO at some point, which will most likely inline those.

fbq · 2021-05-13T13:50:37Z

From #254:
In general, I think we want to go for re-using the C functions/macros where possible. Performance work should be based on actual numbers, which we can focus on later. And, anyway, we should be able to have cross-language LTO at some point, which will most likely inline those.

Yep, I also hope that cross-language LTO could work, I'd like also to try some experiments myself. In the meanwhile, @TheSven73 feel free to submit your version as a PR, I will close this one once your version has been fully reviewed.

TheSven73 · 2021-05-13T14:02:20Z

In the meanwhile, @TheSven73 feel free to submit your version as a PR, I will close this one once your version has been fully reviewed.

When PRs are as closely reviewed as we're doing here, I feel that they are always co-owned. So I wouldn't like to call it "my version". A new PR just means we try again, from a different starting point.

fbq · 2021-05-13T14:10:53Z

In the meanwhile, @TheSven73 feel free to submit your version as a PR, I will close this one once your version has been fully reviewed.
When PRs are as closely reviewed as we're doing here, I feel that they are always co-owned. So I wouldn't like to call it "my version". A new PR just means we try again, from a different starting point.

"your version" was simply a reference ;-) I could say "the implementation which reuses the Linux functions/macros" but I was lazy typing. Hope you don't mind ;-)

TheSven73 · 2021-05-13T15:29:12Z

Don't mind at all :)

Alternative suggestion in #268

fbq · 2021-05-17T04:56:29Z

Drop the "ptr-to-err" commit since @TheSven73 already uploaded #268, only keep the commit that makes Error as Debug so that Result::expect can work.

fbq · 2021-05-17T16:10:33Z

@ojeda could you approve the CI, now I drop the ptr-to-err commit, so only remaining commit is the one that make Result::expect work. ;-) Thanks!

ojeda · 2021-05-17T16:35:32Z

Approved!

TheSven73 · 2021-05-17T17:57:13Z

A silly question maybe, but do we want expect() to work inside the kernel? AFAIK, that function shows an error string and then proceeds to blow up the kernel. Instead of erroring out gracefully.

I could be wrong though - maybe there are use cases that I don't know of?

ojeda · 2021-05-17T21:58:32Z

A silly question maybe, but do we want expect() to work inside the kernel? AFAIK, that function shows an error string and then proceeds to blow up the kernel. Instead of erroring out gracefully.
I could be wrong though - maybe there are use cases that I don't know of?

Given C's panic(), WARN(), etc. support a message, we could use it. It also helps the reader of the code. On the other hand, BUG() does not ask for one, and we also have PANIC/NOPANIC comments (although those are not for the user, but...).

We could even think about disallowing unwrap() so that everyone provides a message...

In any case, please also note that it is likely that we will need to switch Rust panics to not-actual-kernel-panics, instead killing the current thread or something like that.

ksquirrel · 2021-05-19T11:57:20Z

Review of 40a271acef46:

✔️ Commit 40a271a: Looks fine!

fbq · 2021-05-21T11:18:28Z

@ojeda Is this PR (with only one commit) OK to merge? I'm thinking about rebasing the thread PR, which depends on this. If there is anything I need to improve for this, please let me know.

ojeda · 2021-05-21T22:47:02Z

Sure, LGTM.

nbdd0121 · 2021-05-22T05:29:29Z

It would make more sense to use errname to print out the name instead.

nbdd0121 · 2021-05-22T05:56:59Z

Filed in #287

rust: error: Derive Error as Debug
40a271a
So that `Result::expect` can be used. Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

fbq commented May 13, 2021
View reviewed changes

fbq mentioned this pull request May 13, 2021
WIP: Simple Rust driver that touches real hardware #254
Closed

TheSven73 reviewed May 13, 2021
View reviewed changes

ojeda requested changes May 13, 2021
View reviewed changes

fbq force-pushed the dev/rust-err branch from 0966891 to 40a271aCompare May 17, 2021 04:53

ojeda merged commit 45d79fc into Rust-for-Linux:rustMay 21, 2021

fbq deleted the dev/rust-err branch October 11, 2021 10:56

	/// [`Error::from_kernel_ptr`]), therefore this function returns `Err` if `ptr
	/// [`Error::from_kernel_ptr`]), therefore this function returns `Err` if `ptr`

	/// - [`core::usize::MAX - bindings::MAX_ERRNO`,..,`core::usize::MAX] is the
	/// - [`core::usize::MAX - bindings::MAX_ERRNO`,..,`core::usize::MAX`] is the

	/// - [0, .., `core::usize::MAX - bindings::MAX_ERRNO`) is the range for
	/// - [`0`, .., `core::usize::MAX - bindings::MAX_ERRNO`) is the range for

	if value >= core::usize::MAX - bindings::MAX_ERRNOasusize{
	if value >= core::usize::MAX - bindings::MAX_ERRNOasusize+ 1{

Uh oh!

Conversation

fbq commented May 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSven73 left a comment

Choose a reason for hiding this comment

Uh oh!

TheSven73May 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSven73May 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSven73May 13, 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!

fbqMay 13, 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!

TheSven73May 13, 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!

ojedaMay 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ojeda commented May 13, 2021

Uh oh!

fbq commented May 13, 2021

Uh oh!

TheSven73 commented May 13, 2021

Uh oh!

fbq commented May 13, 2021

Uh oh!

TheSven73 commented May 13, 2021

Uh oh!

fbq commented May 17, 2021

Uh oh!

fbq commented May 17, 2021

Uh oh!

ojeda commented May 17, 2021

Uh oh!

TheSven73 commented May 17, 2021

Uh oh!

ojeda commented May 17, 2021

Uh oh!

ksquirrel commented May 19, 2021

Uh oh!

fbq commented May 21, 2021

Uh oh!

ojeda commented May 21, 2021

Uh oh!

nbdd0121 commented May 22, 2021

TheSven73May 13, 2021•
edited
Loading

TheSven73May 13, 2021•
edited
Loading

TheSven73May 13, 2021•
edited
Loading

fbqMay 13, 2021•
edited
Loading

TheSven73May 13, 2021•
edited
Loading

ojedaMay 13, 2021•
edited
Loading