Message-ID: <c6c1e84a-40f3-41a5-a732-f1cf06521691@proton.me>
Date: Wed, 21 Aug 2024 11:31:25 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Matthew Maurer <mmaurer@...gle.com>, Sami Tolvanen <samitolvanen@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Petr Pavlu <petr.pavlu@...e.com>, Neal Gompa <neal@...pa.dev>, Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 16/19] gendwarfksyms: Add support for reserved structure fields

On 20.08.24 22:03, Matthew Maurer wrote:
>>> The way `KAbiReserved` is implemented is via a `union` (maybe a bit
>>> ironic, considering what I said in my other replies, but in this case,
>>> we would provide a safe abstraction over this `union`, thus avoiding
>>> exposing users of this type to `unsafe`):
>>>
>>>     #[repr(C)]
>>>     pub union KAbiReserved<T, R> {
>>>         value: T,
>>>         _reserved: R,
>>>     }
>>
>> I like this approach even better, assuming any remaining issues with
>> ownership etc. can be sorted out. This would also look identical to
>> the C version in DWARF if you rename _reserved in the union to
>> __kabi_reserved. Of course, we can always change gendwarfksyms to
>> support a different scheme for Rust code if a better solution comes
>> along later.

Yeah sure, that should also then work directly with this patch, right?

>> Sami
> 
> Agreement here - this seems like a good approach to representing
> reserved in Rust code. A few minor adjustments we discussed off-list
> which aren't required for gendwarfksyms to know about:
> 1. Types being added to reserved fields have to be `Copy`, e.g. they
> must be `!Drop`.
> 2. Types being added to reserved fields must be legal to be
> represented by all zeroes.
> 3. Reserved fields need to be initialized to zero before having their
> union set to the provided value when constructing them.
> 4. It may be helpful to have delegating trait implementations to avoid
> the couple places where autoderef won't handle the conversion.
> 
> While I think this is the right solution, esp. since it can share a
> representation with C, I wanted to call out one minor shortfall - a
> reserved field can only be replaced by one type. We could still
> indicate a replacement by two fields the same as in C, by using a
> tuple which will look like an anonymous struct. The limitation will be
> that if two or more new fields were introduced, we'd need to edit the
> patches accessing them to do foo.x.y and foo.x.z for their accesses
> instead of simply foo.y and foo.z - the autoref trick only works for a
> single type.

We will have to see how often multiple fields are added to a struct. If
they are infrequent and it's fine for those patches to then touch the
field accesses, then I think we can just stick with this approach.
If there are problems with that, we can also try the following:
all fields of kABI structs must be private and must only be accessed
through setters/getters. We can then modify the body the setters/getters
to handle the additional indirection.

If that also sounds too much work compared to the C side, then we can
get in touch with the Rust language folks and see if they can provide us
with a better solution.

---
Cheers,
Benno

Powered by blists - more mailing lists