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

Add XArray abstraction #1019

Draft
wants to merge 37 commits into
base: rust-dev
Choose a base branch
from

Conversation

matthewtgilbride
Copy link

@matthewtgilbride matthewtgilbride commented Jun 29, 2023

  • updates documentation for installing bindgen (now bindgen-cli)
  • adds the XArray abstraction

fbq and others added 2 commits June 25, 2023 16:49
Currently the KernelAllocator simply passes the size of the type Layout
to krealloc(), and in theory the alignment requirement from the type
Layout may be larger than the guarantee provided by SLAB, which means
the allocated object is mis-aligned.

Fixes this by adjusting the allocation size to the nearest power of two,
which SLAB always guarantees a size-aligned allocation. And because Rust
guarantees that original size must be a multiple of alignment and the
alignment must be a power of two, then the alignment requirement is
satisfied.

Suggested-by: Vlastimil Babka <[email protected]>
Co-developed-by: Andreas Hindborg (Samsung) <[email protected]>
Signed-off-by: Andreas Hindborg (Samsung) <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
Cc: [email protected] # v6.1+
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
When combining `UnsafeCell` with `MaybeUninit`, it is idiomatic to use
`UnsafeCell` as the outer type. Intuitively, this is because a
`MaybeUninit<T>` might not contain a `T`, but we always want the effect
of the `UnsafeCell`, even if the inner value is uninitialized.

Now, strictly speaking, this doesn't really make a difference. The
compiler will always apply the `UnsafeCell` effect even if the inner
value is uninitialized. But I think we should follow the convention
here.

Signed-off-by: Alice Ryhl <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
@@ -98,7 +98,7 @@ the ``bindgen`` tool. A particular version is required.

Install it via (note that this will download and build the tool from source)::

cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen
cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen-cli
Copy link
Member

Choose a reason for hiding this comment

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

I also discovered this yesterday. Could you send this patch directly to the mailing list?

@ojeda probably should take this as a fix.

Copy link
Member

@ojeda ojeda Jun 29, 2023

Choose a reason for hiding this comment

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

This is https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/Build.20errors/near/365685550 (applied at the rebased ojeda@698129c), i.e. I was going to fix it directly when we take the upgrade to 0.65.1.

@@ -45,6 +45,7 @@ pub mod str;
pub mod sync;
pub mod task;
pub mod types;
pub mod xarray;
Copy link
Member

Choose a reason for hiding this comment

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

not urgent, but should we introduce a collections module and make xarray a submod of it?

}

/// Wrapper for a value owned by the `XArray` which holds the `XArray` lock until dropped.
pub struct ValueGuard<'a, T: ForeignOwnable>(NonNull<T>, Pin<&'a XArray<T>>);
Copy link
Member

Choose a reason for hiding this comment

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

Given it's a guard for (a value in) XArrary, maybe call it XGuard? (Sorry couldn't resist ;-))

Copy link
Member

@fbq fbq Jun 29, 2023

Choose a reason for hiding this comment

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

I don't think NonNull<T> is the correct type here, consider you have a XArray<Box<i32>>, then you have a NonNull<Box<i32>> here, that's a pointer to a Box (i.e. second-level pointer).

It should be NonNull<core::ffi::c_void> I think, of course the type here doesn't matter much, so another idea is to use NonNull<u32>, since xarray requires pointers to be 4-byte aligned

Copy link
Member

Choose a reason for hiding this comment

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

We probably need a new type to capture the 4-byte alignment invariant.

Comment on lines 80 to 134
// SAFETY: We have just created `xa`. This data structure does not require
// pinning.
Copy link
Member

Choose a reason for hiding this comment

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

Why do all of the functions take Pin<&Self> then? wouldnt &self be ok if this statement is true?

Also if I read this correctly, then there is a spinlock inside of the xarray, that might requiring pinning.

Copy link
Author

Choose a reason for hiding this comment

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

¯_(ツ)_/¯

I will say it makes things a bit awkward if you have a &mut ref as the caller. You can't freely call the methods that take &self without converting over to a Pin<&mut Self>, which I think works in normal rust...correct?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what you mean. At the moment the new function just returns a plain XArray. If it needs to be pinned to be used, you should just use pin-init. If it does not need to be pinned, then the functions should take &self instead of self: Pin<&Self>.

Copy link
Author

Choose a reason for hiding this comment

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

As discussed - will update new to return Pin<Self> (or whatever the right thing should be return for pin-init)

Copy link
Member

Choose a reason for hiding this comment

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

It should be impl PinInit<Self>

)
};

let ret = unsafe { bindings::xa_err(old) };
Copy link
Member

Choose a reason for hiding this comment

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

Missing SAFETY comment.

T::from_foreign(new);
});

// SAFETY: `self.xa` is always valid by the type invariant, and we are storing
Copy link
Member

Choose a reason for hiding this comment

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

This invariant should be documented.

impl<'a, T: ForeignOwnable> Drop for ValueGuard<'a, T> {
fn drop(&mut self) {
// SAFETY: The XArray we have a reference to owns the C xarray object.
unsafe { bindings::xa_unlock(self.1.xa.get()) };
Copy link
Member

Choose a reason for hiding this comment

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

this should be a type invariant.


/// Removes and returns an entry, returning it if it existed.
pub fn remove(self: Pin<&Self>, index: usize) -> Option<T> {
let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) };
Copy link
Member

Choose a reason for hiding this comment

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

safety comment

///
/// assert!(xarr.get(0).is_none());
///
/// xarr.set(0, Box::try_new(0)?);
Copy link
Member

Choose a reason for hiding this comment

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

missing ? at the end.

Comment on lines 100 to 101
/// // `find` returns the index/value pair matching the index, the next larger
/// // index/value pair if it doesn't exist, or `None` of no larger index exists.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would also be good to know if the index wraps around, so if this is true:

let xarr: Box<XArray<Box<usize>>> = Box::try_new(XArray::new(LOCK_IRQ))?;
let xarr: Pin<Box<XArray<Box<usize>>>> = Box::into_pin(xarr);
let xarr: Pin<&XArray<Box<usize>>> = xarr.as_ref();
xarr.set(0, Box::try_new(42)?)?;
let (i, v) = xarr.find(1).unwrap();
assert_eq!(i, 0);
assert_eq!(v.borrow(), &42);

Comment on lines +71 to +50
/// In this example, we create a new `XArray` and demonstrate
/// rudimentary read/write operations.
Copy link
Member

Choose a reason for hiding this comment

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

I think this example is very helpful! Additionally it might be a good idea to take each example and put it on the relevant functions as well, especially the find function.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't realize that worked! I think at one point in time I could only get the kunit stuff working if the tests were on the top of the struct.

I see now that it does...what do we consider better, a long doctest at the top with interactions between different functions, or individual tests for each function? (Or both...with some repetition?)

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have a examples on functions. Big examples only make sense if you want to demonstrate how things are used together. I would recommend this when the API is excessively complicated. I dont think that is the case here.

Comment on lines 268 to 269
/// This guard blocks all other actions on the `XArray`. Callers are expected to drop the
/// `ValueGuardMut` eagerly to avoid blocking other users, such as by taking a clone of the value.
Copy link
Member

Choose a reason for hiding this comment

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

The receiver type is Pin<&mut Self>, so we have unique access. therefore other users need to wait for some other lock until they get access... I would just remove this.

///
/// Returns a `Reservation` object, which can then be used to store a value at this index or
/// otherwise free it for reuse.
pub fn reserve_limits(self: Pin<&Self>, min: u32, max: u32) -> Result<Reservation<'_, T>> {
Copy link
Member

Choose a reason for hiding this comment

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

just unimportant bikeshedding: I would call this reserve_between and the other functions adjusted similarly. What do the others think?

Benno Lossin and others added 24 commits June 30, 2023 13:24
Adds a `PhantomPinned` field to `Opaque<T>`. This removes the last Rust
guarantee: the assumption that the type `T` can be freely moved. This is
not the case for many types from the C side (e.g. if they contain a
`struct list_head`). This change removes the need to add a
`PhantomPinned` field manually to Rust structs that contain C structs
which must not be moved.

Signed-off-by: Benno Lossin <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
The -v option is passed when this script is invoked from Makefile,
but not when invoked from Kconfig.

As you can see in scripts/Kconfig.include, the 'success' macro suppresses
stdout and stderr anyway, so this script does not need to be quiet.

Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Miguel Ojeda <[email protected]>
Tested-by: Miguel Ojeda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[ Reworded prefix to match the others in the patch series. ]
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
…uments

rust_is_available.sh uses cc-version.sh to identify which C compiler is
in use, as scripts/Kconfig.include does.  cc-version.sh isn't designed to
be able to handle multiple arguments in one variable, i.e. "ccache clang".
Its invocation in rust_is_available.sh quotes "$CC", which makes
$1 == "ccache clang" instead of the intended $1 == ccache & $2 == clang.

cc-version.sh could also be changed to handle having "ccache clang" as one
argument, but it only has the one consumer upstream, making it simpler to
fix the caller here.

Signed-off-by: Russell Currey <[email protected]>
Fixes: 78521f3 ("scripts: add `rust_is_available.sh`")
Link: Rust-for-Linux#873
[ Reworded title prefix and reflow line to 75 columns. ]
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Sometimes users need to tweak the finding process of `libclang`
for `bindgen` via the `clang-sys`-provided environment variables.

Thus add a paragraph to the setting up guide, including a reference
to `clang-sys`'s relevant documentation.

Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
Reviewed-by: Nick Desaulniers <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
People trying out the Rust support in the kernel may get
warnings and errors from `scripts/rust_is_available.sh`
from the `rustavailable` target or the build step.

Some of those users may be following the Quick Start guide,
but others may not (likely those getting warnings from
the build step instead of the target).

While the messages are fairly clear on what the problem is,
it may not be clear how to solve the particular issue,
especially for those not aware of the documentation.

We could add all sorts of details on the script for each one,
but it is better to point users to the documentation instead,
where it is easily readable in different formats. It also
avoids duplication.

Thus add a reference to the documentation whenever the script
fails or there is at least a warning.

Reviewed-by: Finn Behrens <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
`scripts/rust_is_available.sh` calls `bindgen` with a special
header in order to check whether the `libclang` version in use
is suitable.

However, the invocation itself may fail if, for instance, `bindgen`
cannot locate `libclang`. This is fine for Kconfig (since the
script will still fail and therefore disable Rust as it should),
but it is pretty confusing for users of the `rustavailable` target
given the error will be unrelated:

    ./scripts/rust_is_available.sh: 21: arithmetic expression: expecting primary: "100000 *  + 100 *  + "
    make: *** [Makefile:1816: rustavailable] Error 2

Instead, run the `bindgen` invocation independently in a previous
step, saving its output and return code. If it fails, then show
the user a proper error message. Otherwise, continue as usual
with the saved output.

Since the previous patch we show a reference to the docs, and
the docs now explain how `bindgen` looks for `libclang`,
thus the error message can leverage the documentation, avoiding
duplication here (and making users aware of the setup guide in
the documentation).

Reported-by: Nick Desaulniers <[email protected]>
Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
Reported-by: François Valenduc <[email protected]>
Closes: Rust-for-Linux#934
Reported-by: Alexandru Radovici <[email protected]>
Closes: Rust-for-Linux#921
Reported-by: Matthew Leach <[email protected]>
Closes: https://lore.kernel.org/rust-for-linux/[email protected]/
Fixes: 78521f3 ("scripts: add `rust_is_available.sh`")
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Sometimes [1] users may attempt to setup the Rust support by
checking what Kbuild does and they end up finding out about
`scripts/rust_is_available.sh`. Inevitably, they run the script
directly, but unless they setup the required variables,
the result of the script is not meaningful.

We could add some defaults to the variables, but that could be
confusing for those that may override the defaults (compared
to their kernel builds), and `$CC` would not be a simple default
in any case.

Therefore, instead, explicitly check whether the expected variables
are set (`$RUSTC`, `$BINDGEN` and `$CC`). If not, print an explanation
about the fact that the script is meant to be called from Kbuild,
since that is the most likely cause for the variables not being set.

Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/ [1]
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
…e path

`bindgen`'s output for `libclang`'s version check contains paths, which
in turn may contain strings that look like version numbers [1][2]:

    .../6.1.0-dev/.../rust_is_available_bindgen_libclang.h:2:9: warning: clang version 11.1.0  [-W#pragma-messages], err: false

which the script will pick up as the version instead of the latter.

It is also the case that versions may appear after the actual version
(e.g. distribution's version text), which was the reason behind `head` [3]:

    .../rust-is-available-bindgen-libclang.h:2:9: warning: clang version 13.0.0 (Fedora 13.0.0-3.fc35) [-W#pragma-messages], err: false

Thus instead ask for a match after the `clang version` string.

Reported-by: Jordan Isaacs <[email protected]>
Closes: Rust-for-Linux#942 [1]
Reported-by: Ethan D. Twardy <[email protected]>
Closes: https://lore.kernel.org/rust-for-linux/[email protected]/ [2]
Reported-by: Tiago Lam <[email protected]>
Closes: Rust-for-Linux#789 [3]
Fixes: 78521f3 ("scripts: add `rust_is_available.sh`")
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-By: Ethan Twardy <[email protected]>
Tested-By: Ethan Twardy <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
In order to match the version string, `sed` is used in a couple
cases, and `grep` and `head` in a couple others.

Make the script more consistent and easier to understand by
using the same method, `sed`, for all of them.

This makes the version matching also a bit more strict for
the changed cases, since the strings `rustc ` and `bindgen `
will now be required, which should be fine since `rustc`
complains if one attempts to call it with another program
name, and `bindgen` uses a hardcoded string.

In addition, clarify why one of the existing `sed` commands
does not provide an address like the others.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
The script already checks if `$RUSTC` and `$BINDGEN` exists via
`command`, but the environment variables may point to a
non-executable file, or the programs may fail for some other reason.
While the script successfully exits with a failure as it should,
the error given can be quite confusing depending on the shell and
the behavior of its `command`. For instance, with `dash`:

    $ RUSTC=./mm BINDGEN=bindgen CC=clang scripts/rust_is_available.sh
    scripts/rust_is_available.sh: 19: arithmetic expression: expecting primary: "100000 *  + 100 *  + "

Thus detect failure exit codes when calling `$RUSTC` and `$BINDGEN` and
print a better message, in a similar way to what we do when extracting
the `libclang` version found by `bindgen`.

Link: https://lore.kernel.org/rust-for-linux/CAK7LNAQYk6s11MASRHW6oxtkqF00EJVqhHOP=5rynWt-QDUsXw@mail.gmail.com/
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
The script already checks for `$RUSTC` and `$BINDGEN` existing
and exiting without failure. However, one may still pass an
unexpected binary that does not output what the later parsing
expects. The script still successfully reports a failure as
expected, but the error is confusing. For instance:

    $ RUSTC=true BINDGEN=bindgen CC=clang scripts/rust_is_available.sh
    scripts/rust_is_available.sh: 19: arithmetic expression: expecting primary: "100000 *  + 100 *  + "
    ***
    *** Please see Documentation/rust/quick-start.rst for details
    *** on how to set up the Rust support.
    ***

Thus add an explicit check and a proper message for unexpected
output from the called command.

Similarly, do so for the `libclang` version parsing, too.

Link: https://lore.kernel.org/rust-for-linux/CAK7LNAQYk6s11MASRHW6oxtkqF00EJVqhHOP=5rynWt-QDUsXw@mail.gmail.com/
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
The `rust_is_available.sh` script runs for everybody compiling the
kernel, even if not using Rust. Therefore, it is important to ensure
that the script is correct to avoid breaking people's compilation.

In addition, the script needs to be able to handle a set of subtle
cases, including parsing version strings of different tools.

Therefore, maintenance of this script can be greatly eased with
a set of tests.

Thus add a test suite to cover hopefully most of the setups that
the script may encounter in the wild. Extra setups can be easily
added later on if missing.

The script currently covers all the branches of the shell script,
including several ways in which they may be entered.

Python is used for this script, since the script under test
does not depend on Rust, thus hopefully making it easier for others
to use if the need arises.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
While there are default impls for these methods, using the respective C
api's is faster. Currently neither the existing nor these new
GlobalAlloc method implementations are actually called. Instead the
__rust_* function defined below the GlobalAlloc impl are used. With
rustc 1.71 these functions will be gone and all allocation calls will go
through the GlobalAlloc implementation.

Link: Rust-for-Linux#68
Signed-off-by: Björn Roy Baron <[email protected]>
[boqun: add size adjustment for alignment requirement]
Signed-off-by: Boqun Feng <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
* Rationale:

Upgrades bindgen to code-generation for anonymous unions, structs, and enums [7]
for LLVM-16 based toolchains.

The following upgrade also incorporates `noreturn` support from bindgen
allowing us to remove useless `loop` calls which was placed as a
workaround.

* Truncated build logs on using bindgen `v0.56.0` prior to LLVM-16 toolchain:

```
$ make rustdoc LLVM=1 CLIPPY=1 -j$(nproc)
  RUSTC L rust/core.o
  BINDGEN rust/bindings/bindings_generated.rs
  BINDGEN rust/bindings/bindings_helpers_generated.rs
  BINDGEN rust/uapi/uapi_generated.rs
thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at__/_/include/linux/compiler_types_h_146_2)" is not a valid Ident', .../proc-macro2-1.0.24/src/fallback.rs:693:9
...
thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at__/_/include/linux/compiler_types_h_146_2)" is not a valid Ident', .../proc-macro2-1.0.24/src/fallback.rs:693:9
...
```

* LLVM-16 Changes:

API changes [1] were introduced such that libclang would emit names like
"(unnamed union at compiler_types.h:146:2)" for unnamed unions, structs, and
enums whereas it previously returned an empty string.

* Bindgen Changes:

Bindgen `v0.56.0` on LLVM-16 based toolchains hence was unable to process the
anonymous union in `include/linux/compiler_types` `struct ftrace_branch_data`
and caused subsequent panics as the new `libclang` API emitted name was not
being handled. The following issue was fixed in Bindgen `v0.62.0` [2].

Bindgen `v0.58.0` changed the flags `--whitelist-*` and `--blacklist-*`
to `--allowlist-*` and `--blocklist-*` respectively [3].

Bindgen `v0.61.0` added support for `_Noreturn`, `[[noreturn]]`, `__attribute__((noreturn))` [4],
hence the empty `loop`s used to circumvent bindgen returning `!` in place of `()`
for noreturn attributes have been removed completely.

Bindgen `v0.61.0` also changed default functionality to bind `size_t` to `usize` [5] and
added the `--no-size_t-is-usize` [5] flag to not bind `size_t` as `usize`.

Bindgen `v0.65.0` removed `--size_t-is-usize` flag [6].

Link: llvm/llvm-project@19e984e [1]
Link: rust-lang/rust-bindgen#2319 [2]
Link: rust-lang/rust-bindgen#1990 [3]
Link: rust-lang/rust-bindgen#2094 [4]
Link: rust-lang/rust-bindgen@cc78b6f [5]
Link: rust-lang/rust-bindgen#2408 [6]
Closes: Rust-for-Linux#1013 [7]
Signed-off-by: Aakash Sen Sharma <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Tested-by: Ariel Miculas <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[boqun: resolve conflicts because of Rust version bump]
Signed-off-by: Boqun Feng <[email protected]>
Rust documentation tests are going to be build/run-tested
with the KUnit integration added in a future patch, thus
update them to make them compilable/testable so that we
may start enforcing it.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
Reviewed-by: David Gow <[email protected]>
Reviewed-by: Vincenzo Palazzo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Rust documentation tests are going to be build/run-tested
with the KUnit integration added in a future patch, thus
update them to make them compilable/testable so that we
may start enforcing it.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
Reviewed-by: David Gow <[email protected]>
Reviewed-by: Vincenzo Palazzo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Rust documentation tests are going to be build/run-tested
with the KUnit integration added in a future patch, thus
update them to make them compilable/testable so that we
may start enforcing it.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
Reviewed-by: David Gow <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Rust documentation tests are going to be build/run-tested
with the KUnit integration added in a future patch, thus
update them to make them compilable/testable so that we
may start enforcing it.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
Reviewed-by: David Gow <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Rust has documentation tests: these are typically examples of
usage of any item (e.g. function, struct, module...).

They are very convenient because they are just written
alongside the documentation. For instance:

    /// Sums two numbers.
    ///
    /// ```
    /// assert_eq!(mymod::f(10, 20), 30);
    /// ```
    pub fn f(a: i32, b: i32) -> i32 {
        a + b
    }

In userspace, the tests are collected and run via `rustdoc`.
Using the tool as-is would be useful already, since it allows
to compile-test most tests (thus enforcing they are kept
in sync with the code they document) and run those that do not
depend on in-kernel APIs.

However, by transforming the tests into a KUnit test suite,
they can also be run inside the kernel. Moreover, the tests
get to be compiled as other Rust kernel objects instead of
targeting userspace.

On top of that, the integration with KUnit means the Rust
support gets to reuse the existing testing facilities. For
instance, the kernel log would look like:

    KTAP version 1
    1..1
        KTAP version 1
        # Subtest: rust_doctests_kernel
        1..59
        # Doctest from line 13
        ok 1 rust_doctest_kernel_build_assert_rs_0
        # Doctest from line 56
        ok 2 rust_doctest_kernel_build_assert_rs_1
        # Doctest from line 122
        ok 3 rust_doctest_kernel_init_rs_0
        ...
        # Doctest from line 150
        ok 59 rust_doctest_kernel_types_rs_2
    # rust_doctests_kernel: pass:59 fail:0 skip:0 total:59
    # Totals: pass:59 fail:0 skip:0 total:59
    ok 1 rust_doctests_kernel

Therefore, add support for running Rust documentation tests
in KUnit. Some other notes about the current implementation
and support follow.

The transformation is performed by a couple scripts written
as Rust hostprogs.

Tests using the `?` operator are also supported as usual, e.g.:

    /// ```
    /// # use kernel::{spawn_work_item, workqueue};
    /// spawn_work_item!(workqueue::system(), || pr_info!("x"))?;
    /// # Ok::<(), Error>(())
    /// ```

The tests are also compiled with Clippy under `CLIPPY=1`, just like
normal code, thus also benefitting from extra linting.

The names of the tests are currently automatically generated.
This allows to reduce the burden for documentation writers,
while keeping them fairly stable for bisection. This is an
improvement over the `rustdoc`-generated names, which include
the line number; but ideally we would like to get `rustdoc` to
provide the Rust item path and a number (for multiple examples
in a single documented Rust item).

In order for developers to easily see from which original line
a failed doctests came from, a KTAP diagnostic line is printed
to the log. In the future, we may be able to use a proper KUnit
facility to append this sort of information instead.

A notable difference from KUnit C tests is that the Rust tests
appear to assert using the usual `assert!` and `assert_eq!`
macros from the Rust standard library (`core`). We provide
a custom version that forwards the call to KUnit instead.
Importantly, these macros do not require passing context,
unlike the KUnit C ones (i.e. `struct kunit *`). This makes
them easier to use, and readers of the documentation do not need
to care about which testing framework is used. In addition, it
may allow us to test third-party code more easily in the future.

However, a current limitation is that KUnit does not support
assertions in other tasks. Thus we presently simply print an
error to the kernel log if an assertion actually failed. This
should be revisited to properly fail the test, perhaps saving
the context somewhere else, or letting KUnit handle it.

Signed-off-by: Miguel Ojeda <[email protected]>
Tested-by: Sergio González Collado <[email protected]>
Reviewed-by: David Gow <[email protected]>
Reviewed-by: Vincenzo Palazzo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Tested-by: Matt Gilbride <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[boqun: Add #include <linux/stddef.h>]
The KUnit maintainers would like to maintain these files on their
side too (thanks!), so add them to their entry.

With this in place, `scripts/get_maintainer.pl` prints both sets
of maintainers/reviewers (i.e. KUnit and Rust) for those files,
which is the behavior we are looking for.

Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: David Gow <[email protected]>
Reviewed-by: Vincenzo Palazzo <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
If the trait has same function name, the `vtable` macro
will redefine its `gen_const_name`, e.g.:
```rust
    #[vtable]
    pub trait Foo {
        #[cfg(CONFIG_X)]
        fn bar();

        #[cfg(not(CONFIG_X))]
        fn bar(x: usize);
    }
```
Use `HashSet` to avoid this.

Signed-off-by: Qingsong Chen <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
This macro provides a flexible way to concatenated identifiers together
and it allows the resulting identifier to be used to declare new items,
which `concat_idents!` does not allow. It also allows identifiers to be
transformed before concatenated.

The `concat_idents!` example

    let x_1 = 42;
    let x_2 = concat_idents!(x, _1);
    assert!(x_1 == x_2);

can be written with `paste!` macro like this:

    let x_1 = 42;
    let x_2 = paste!([<x _1>]);
    assert!(x_1 == x_2);

However `paste!` macro is more flexible because it can be used to create
a new variable:

    let x_1 = 42;
    paste!(let [<x _2>] = [<x _1>];);
    assert!(x_1 == x_2);

While this is not possible with `concat_idents!`.

This macro is similar to the `paste!` crate [1], but this is a fresh
implementation to avoid vendoring large amount of code directly. Also, I
have augmented it to provide a way to specify span of the resulting
token, allowing precise control.

For example, this code is broken because the variable is declared inside
the macro, so Rust macro hygiene rules prevents access from the outside:

    macro_rules! m {
        ($id: ident) => {
            // The resulting token has hygiene of the macro.
            paste!(let [<$id>] = 1;)
        }
    }

    m!(a);
    let _ = a;

In this versionn of `paste!` macro I added a `span` modifier to allow
this:

    macro_rules! m {
        ($id: ident) => {
            // The resulting token has hygiene of `$id`.
            paste!(let [<$id:span>] = 1;)
        }
    }

    m!(a);
    let _ = a;

Link: http://docs.rs/paste/ [1]
Signed-off-by: Gary Guo <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
Reviewed-by: Benno Lossin <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
This commit provides the build flags for Rust for AArch64. The core Rust
support already in the kernel does the rest.

When disabling the neon and fp target features to avoid fp & simd
registers. The use of fp-armv8 will cause a warning from rustc about
an unknown feature that is specified. The target feature is still
passed through to LLVM, this behaviour is documented as part of the
warning. This will be fixed in a future version of the rustc
toolchain.

The Rust samples have been tested with this commit.

Signed-off-by: Jamie Cunliffe <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[boqun: resolve the conflicts with kunit test enablement]
Enable the PAC ret and BTI options in the Rust build flags to match
the options that are used when building C.

Signed-off-by: Jamie Cunliffe <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
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.