Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warning fixes and 6.13 backport #25

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

bonzini
Copy link
Contributor

@bonzini bonzini commented Jan 18, 2025

The 6.13 changes focused on fixing and enabling warnings, so do the same and make the standalone pinned-init warning-free. Similar changes are in #20.

ojeda and others added 16 commits January 18, 2025 14:47
In Rust 1.71.0, `rustdoc` added the `unescaped_backticks` lint, which
detects what are typically typos in Markdown formatting regarding inline
code [1], e.g. from the Rust standard library:

    /// ... to `deref`/`deref_mut`` must ...

    /// ... use [`from_mut`]`. Specifically, ...

It does not seem to have almost any false positives, from the experience
of enabling it in the Rust standard library [2], which will be checked
starting with Rust 1.82.0. The maintainers also confirmed it is ready
to be used.

Thus enable it.

Link: https://doc.rust-lang.org/rustdoc/lints.html#unescaped_backticks [1]
Link: rust-lang/rust#128307 [2]
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-9-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
(cherry picked from commit bef83245f5ed434932aaf07f890142b576dc5d85)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Avoid using "pub use", which results in warnings about unused imports.  It is
easy enough to include all that is needed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
When examples are compiled independently, they need the enable the
allocator_api feature.  When they are brought in as modules from
other examples, the parent crate does it for them.  In the latter
case, the compiler complains that the module cannot enable the
feature and therefore the attribute is unused.  Follow the example
already present in examples/mutex.rs, and shut up the compiler
about cases that are expected.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
…s` lint

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
…ents

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Elide the functions completely when miri or NO_UI_TESTS are enabled.

Extracted from a patch by Benno Lossin.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
The kernel is usually compiled with -Dwarnings, so use deny for these
as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: Rust-for-Linux/linux#351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-5-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
(cherry picked from commit db4f72c904cb116e2bf56afdd67fc5167a607a7b)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
In Rust 1.76.0, Clippy added the `check-private-items` lint configuration
option. When turned on (the default is off), it makes several lints
check private items as well.

In our case, it affects two lints we have enabled [1]:
`missing_safety_doc` and `unnecessary_safety_doc`.

It also seems to affect the new `too_long_first_doc_paragraph` lint [2],
even though the documentation does not mention it.

Thus allow the few instances remaining we currently hit and enable
the lint.

Link: https://doc.rust-lang.org/nightly/clippy/lint_configuration.html#check-private-items [1]
Link: https://rust-lang.github.io/rust-clippy/master/index.html#/too_long_first_doc_paragraph [2]
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-16-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
(cherry picked from commit 624063b9ac97f40cadca32a896aafeb28b1220fd)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
These few cases, unlike others in the same file, did not need the `allow`.

Thus clean them up.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-10-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
(cherry picked from commit d5cc7ab0a0a99496de1bd933dac242699a417809)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Rust 1.82.0's Clippy is introducing [1][2] a new warn-by-default lint,
`too_long_first_doc_paragraph` [3], which is intended to catch titles
of code documentation items that are too long (likely because no title
was provided and the item documentation starts with a paragraph).

This lint does not currently trigger anywhere, but it does detect a couple
cases if checking for private items gets enabled (which we will do in
the next commit):

    error: first doc comment paragraph is too long
      --> src/__internal.rs:18:1
       |
    18 | / /// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this
    19 | | /// type, since the closure needs to fulfill the same safety requirement as the
    20 | | /// `__pinned_init`/`__init` functions.
       | |_
       |
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
       = note: `-D clippy::too-long-first-doc-paragraph` implied by `-D warnings`
       = help: to override `-D warnings` add `#[allow(clippy::too_long_first_doc_paragraph)]`

    error: first doc comment paragraph is too long
     --> rust/kernel/sync/arc/std_vendor.rs:3:1
      |
    3 | / //! The contents of this file come from the Rust standard library, hosted in
    4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under
    5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
    6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
      | |_
      |
      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph

Thus clean those two instances.

In addition, since we have a second `std_vendor.rs` file with a similar
header, do the same there too (even if that one does not trigger the lint,
because it is `doc(hidden)`).

Link: rust-lang/rust#129531 [1]
Link: rust-lang/rust-clippy#12993 [2]
Link: https://rust-lang.github.io/rust-clippy/master/index.html#/too_long_first_doc_paragraph [3]
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-15-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
(cherry picked from commit 2f390cc589433dfcfedc307a141e103929a6fd4d)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
In Rust, it is possible to `allow` particular warnings (diagnostics,
lints) locally, making the compiler ignore instances of a given warning
within a given function, module, block, etc.

It is similar to `#pragma GCC diagnostic push` + `ignored` + `pop` in C:

    #pragma GCC diagnostic push
    #pragma GCC diagnostic ignored "-Wunused-function"
    static void f(void) {}
    #pragma GCC diagnostic pop

But way less verbose:

    #[allow(dead_code)]
    fn f() {}

By that virtue, it makes it possible to comfortably enable more
diagnostics by default (i.e. outside `W=` levels) that may have some
false positives but that are otherwise quite useful to keep enabled to
catch potential mistakes.

The `#[expect(...)]` attribute [1] takes this further, and makes the
compiler warn if the diagnostic was _not_ produced. For instance, the
following will ensure that, when `f()` is called somewhere, we will have
to remove the attribute:

    #[expect(dead_code)]
    fn f() {}

If we do not, we get a warning from the compiler:

    warning: this lint expectation is unfulfilled
     --> x.rs:3:10
      |
    3 | #[expect(dead_code)]
      |          ^^^^^^^^^
      |
      = note: `#[warn(unfulfilled_lint_expectations)]` on by default

This means that `expect`s do not get forgotten when they are not needed.

See the next commit for more details, nuances on its usage and
documentation on the feature.

The attribute requires the `lint_reasons` [2] unstable feature, but it
is becoming stable in 1.81.0 (to be released on 2024-09-05) and it has
already been useful to clean things up in this patch series, finding
cases where the `allow`s should not have been there.

Thus, enable `lint_reasons` and convert some of our `allow`s to `expect`s
where possible.

This feature was also an example of the ongoing collaboration between
Rust and the kernel -- we tested it in the kernel early on and found an
issue that was quickly resolved [3].

Cc: Fridtjof Stoldt <xfrednet@gmail.com>
Cc: Urgau <urgau@numericable.fr>
Link: https://rust-lang.github.io/rfcs/2383-lint-reasons.html#expect-lint-attribute [1]
Link: rust-lang/rust#54503 [2]
Link: rust-lang/rust#114557 [3]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-18-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
(cherry picked from commit 1f9ed172545687e5c04c77490a45896be6d2e459)
[pinned_init: sync ui tests with new expanded code. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Avoid future regressions in compiler warnings.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Extracted from a patch by Benno Lossin.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Copy link
Member

@y86-dev y86-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR! I have one small comment/question, the rest looks good!

@@ -19,7 +19,8 @@ impl Foo {
pin_init!(&this in Self {
marks: {
let ptr = this.as_ptr();
let ptr = unsafe { addr_of_mut!((*ptr).buf)}.cast::<u8>();
// SAFETY: project from the NonNull<Foo> to the buf field
let ptr = unsafe { addr_of_mut!((*ptr).buf[0]) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you changed the .cast to the indexing?
I checked with miri to see that this is allowed, but in my mind, this goes through the IndexMut trait, which would result in a dereference of the pointer... I am a bit confused, maybe that is wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check the MIR that's produced—I don't have the code at hand so I am not sure what is an intrinsic and what isn't. Certainly I can change it back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I tried this sample code

use std::ptr::addr_of_mut;

struct Buf {
    buf: [u8; 64],
}

fn foo(ptr: *mut Buf) -> *mut u8 {
    unsafe { addr_of_mut!((*ptr).buf).cast::<u8>() }
}

fn bar(ptr: *mut Buf) -> *mut u8 {
    unsafe { addr_of_mut!((*ptr).buf[0]) }
}

Unoptimized MIR:

fn foo(_1: *mut Buf) -> *mut u8 {
    debug ptr => _1;
    let mut _0: *mut u8;
    let mut _2: *mut [u8; 64];

    bb0: {
        _2 = &raw mut ((*_1).0: [u8; 64]);
        _0 = std::ptr::mut_ptr::<impl *mut [u8; 64]>::cast::<u8>(move _2) -> [return: bb1, unwind continue];
    }

    bb1: {
        return;
    }
}

fn bar(_1: *mut Buf) -> *mut u8 {
    debug ptr => _1;
    let mut _0: *mut u8;
    let _2: usize;
    let mut _3: usize;
    let mut _4: bool;

    bb0: {
        _2 = const 0_usize;
        _3 = const 64_usize;
        _4 = Lt(copy _2, copy _3);
        assert(move _4, "index out of bounds: the length is {} but the index is {}", move _3, copy _2) -> [success: bb1, unwind continue];
    }

    bb1: {
        _0 = &raw mut ((*_1).0: [u8; 64])[_2];
        return;
    }
}

Optimized MIR:

fn foo(_1: *mut Buf) -> *mut u8 {
    debug ptr => _1;
    let mut _0: *mut u8;
    let mut _2: *mut [u8; 64];

    bb0: {
        StorageLive(_2);
        _2 = &raw mut ((*_1).0: [u8; 64]);
        _0 = std::ptr::mut_ptr::<impl *mut [u8; 64]>::cast::<u8>(move _2) -> [return: bb1, unwind continue];
    }

    bb1: {
        StorageDead(_2);
        return;
    }
}

fn bar(_1: *mut Buf) -> *mut u8 {
    debug ptr => _1;
    let mut _0: *mut u8;

    bb0: {
        _0 = &raw mut ((*_1).0: [u8; 64])[0 of 1];
        return;
    }
}

I don't know if this makes a difference though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants