linux-kernel - Re: [PATCH 0/6] KUnit integration for Rust doctests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSk8bK-xzp-681t0E4j3+tjuraCysD+uUaLaC5otPZzRCQ@mail.gmail.com>
Date:   Fri, 16 Jun 2023 12:44:39 +0800
From:   David Gow <davidgow@...gle.com>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Benno Lossin <benno.lossin@...ton.me>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Andreas Hindborg <nmi@...aspace.dk>,
        Philip Li <philip.li@...el.com>, kunit-dev@...glegroups.com,
        linux-kselftest@...r.kernel.org, rust-for-linux@...r.kernel.org,
        linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH 0/6] KUnit integration for Rust doctests

On Thu, 15 Jun 2023 at 16:20, Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
>
> On Thu, Jun 15, 2023 at 3:44 AM Boqun Feng <boqun.feng@...il.com> wrote:
> >
> > Great work! I've played this for a while, and it's really useful ;-)
>
> Thanks!
>
> > 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?

I like it.

A part of me would like to keep the
kernel::kunit::info(format_args!("    # Doctest from line {line}\n"));

If only so we can preserve the line information when the tests
actually pass. This is something that'd probably ultimately fit as a
"test attribute":
https://lore.kernel.org/linux-kselftest/20230610005149.1145665-1-rmoar@google.com/

For C tests we're not bothering outputting line numbers now (though
again, are considering if we can do it with an attribute), but those
tests have much more searchable names, so I think it still makes sense
for the Rust ones.

How about printing something like:
    # source_line: {}

kunit.py will hide this when the test passes unless the user
explicitly passes --raw_output.

>
> Sounds good to me. However, David/Philip, is this OK or do you really
> need/use the actual/compiled source file there? If you don't need it,
> does it need to exist / be a real file at least? If the latter answer
> is a "yes", which I guess it may be likely, then:

I don't think there's anything automated using the file:line in
assertion messages, it's just for human consumption.
This may change in the future (and there are probably some text
editors around which will turn a path like that into, e.g., a
clickable link now), but we're not currently doing anything which
would actually open the file.

>
> > +        let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/"));
>
> This would not work for files with a `_` in their name, like
> `locked_by.rs`. I guess we could still find the real filename based on
> that information walking the dir, which is another hack I recall
> considering at some point.
>
> Otherwise, if "fake" filenames in the line above are OK for
> David/Philip (I suspect they may want to open them for reporting?),
> then I guess the `file` one may be good enough and eventually we
> should get `rustdoc` to give us the proper metadata anyway.

Personally, I'd think a "fake filename" is okay (especially if it's
temporary until we can get the right one), though I'd prefer there to
be some indication that it's "fake": maybe leaving the _ separator in,
or wrapping it in brackets, or something? Unless the whole
walk-the-filesystem technique ends up being worth doing, so we don't
have a significant chance of the filename being dud.

-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)