Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iad51458e:Fastmail
Date:   Wed, 14 Jun 2023 18:44:19 -0700
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Miguel Ojeda <ojeda@kernel.org>
Cc:     David Gow <davidgow@google.com>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Gary Guo <gary@garyguo.net>,
        =?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        Benno Lossin <benno.lossin@proton.me>,
        Alice Ryhl <aliceryhl@google.com>,
        Andreas Hindborg <nmi@metaspace.dk>,
        Philip Li <philip.li@intel.com>, kunit-dev@googlegroups.com,
        linux-kselftest@vger.kernel.org, rust-for-linux@vger.kernel.org,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev
Subject: Re: [PATCH 0/6] KUnit integration for Rust doctests
Message-ID: <ZIps86MbJF/iGIzd@boqun-archlinux>
References: <20230614180837.630180-1-ojeda@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230614180837.630180-1-ojeda@kernel.org>
Precedence: bulk

On Wed, Jun 14, 2023 at 08:08:24PM +0200, Miguel Ojeda wrote:
> This is the initial KUnit integration for running Rust documentation
> tests within the kernel.
> 
> Thank you to the KUnit team for all the input and feedback on this
> over the months, as well as the Intel LKP 0-Day team!
> 
> This may be merged through either the KUnit or the Rust trees. If
> the KUnit team wants to merge it, then that would be great.
> 
> Please see the message in the main commit for the details.
> 

Great work! I've played this for a while, and it's really useful ;-)

One thing though, maybe we can provide more clues for users to locate
the corresponding Doctests? For example, I did the following to trigger
an assertion:

	diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
	index 91eb2c9e9123..9ead152e2c7e 100644
	--- a/rust/kernel/sync/lock/spinlock.rs
	+++ b/rust/kernel/sync/lock/spinlock.rs
	@@ -58,7 +58,7 @@ macro_rules! new_spinlock {
	 ///
	 /// // Allocate a boxed `Example`.
	 /// let e = Box::pin_init(Example::new())?;
	-/// assert_eq!(e.c, 10);
	+/// assert_eq!(e.c, 11);
	 /// assert_eq!(e.d.lock().a, 20);
	 /// assert_eq!(e.d.lock().b, 30);
	 /// # Ok::<(), Error>(())

Originally I got:

	[..] # Doctest from line 35
	[..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/doctests_kernel_generated.rs:2437
	[..] Expected e.c == 11 to be true, but is false
	[..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0

The assertion warning only says line 35 but which file? Yes, the
".._sync_lock_spinlock_rs" name does provide the lead, however since we
generate the test code, so we actually know the line # for each real
test body, so I come up a way to give us the following:

	[..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61
	[..] Expected e.c == 11 to be true, but is false
	[..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0

Thoughts?

Regards,
Boqun

----------------->8
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 3c94efcd7f76..807fe3633567 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -49,15 +49,15 @@ pub fn info(args: fmt::Arguments<'_>) {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! kunit_assert {
-    ($name:literal, $condition:expr $(,)?) => {
+    ($name:literal, $diff:expr, $file:expr, $condition:expr $(,)?) => {
         'out: {
             // Do nothing if the condition is `true`.
             if $condition {
                 break 'out;
             }
 
-            static LINE: i32 = core::line!() as i32;
-            static FILE: &'static $crate::str::CStr = $crate::c_str!(core::file!());
+            static LINE: i32 = core::line!() as i32 - $diff;
+            static FILE: &'static $crate::str::CStr = $crate::c_str!($file);
             static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition));
 
             // SAFETY: FFI call without safety requirements.
@@ -148,9 +148,9 @@ unsafe impl Sync for UnaryAssert {}
 #[doc(hidden)]
 #[macro_export]
 macro_rules! kunit_assert_eq {
-    ($name:literal, $left:expr, $right:expr $(,)?) => {{
+    ($name:literal, $diff:expr, $file:expr, $left:expr, $right:expr $(,)?) => {{
         // For the moment, we just forward to the expression assert because, for binary asserts,
         // KUnit supports only a few types (e.g. integers).
-        $crate::kunit_assert!($name, $left == $right);
+        $crate::kunit_assert!($name, $diff, $file, $left == $right);
     }};
 }
diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs
index 793885c32c0d..4786a2ef0dc6 100644
--- a/scripts/rustdoc_test_gen.rs
+++ b/scripts/rustdoc_test_gen.rs
@@ -75,6 +75,11 @@ fn main() {
 
         let line = line.parse::<core::ffi::c_int>().unwrap();
 
+        let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/"));
+
+        // Calculate how many lines before `main` function (including the `main` function line).
+        let body_offset = body.lines().take_while(|l| !l.contains("fn main() {")).count() + 1;
+
         use std::fmt::Write;
         write!(
             rust_tests,
@@ -85,7 +90,7 @@ pub extern "C" fn {kunit_name}(__kunit_test: *mut kernel::bindings::kunit) {{
     #[allow(unused)]
     macro_rules! assert {{
         ($cond:expr $(,)?) => {{{{
-            kernel::kunit_assert!("{kunit_name}", $cond);
+            kernel::kunit_assert!("{kunit_name}", anchor - {line}, "{src_file}", $cond);
         }}}}
     }}
 
@@ -93,7 +98,7 @@ macro_rules! assert {{
     #[allow(unused)]
     macro_rules! assert_eq {{
         ($left:expr, $right:expr $(,)?) => {{{{
-            kernel::kunit_assert_eq!("{kunit_name}", $left, $right);
+            kernel::kunit_assert_eq!("{kunit_name}", anchor - {line}, "{src_file}", $left, $right);
         }}}}
     }}
 
@@ -101,9 +106,8 @@ macro_rules! assert_eq {{
     #[allow(unused)]
     use kernel::prelude::*;
 
-    // Display line number so that developers can map the test easily to the source code.
-    kernel::kunit::info(format_args!("    # Doctest from line {line}\n"));
-
+    // The anchor where the test code body starts.
+    static anchor: i32 = core::line!() as i32 + {body_offset} + 1;
     {{
         {body}
         main();