On 24.08.24 23:23, Trevor Gross wrote:
> On Sat, Aug 24, 2024 at 8:16 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>>> We shouldn't enable `const_mut_refs`. It is indeed close to
>>> stabilization, but it is still kind of churny right now and we don't
>>> want to enable the sharp edges everywhere.
>>>
>>> If the change from `static mut` to `UnsafeCell` that I mentioned above
>>> happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the
>>> place of `addr_of_mut!` and doesn't require this feature (and also
>>> isn't unsafe).
>>
>> I think this is a good idea. There might only be a problem with not
>> being `Sync` though... So probably need to use `SyncUnsafeCell` instead.
>
> Ah whoops, yeah that is what I meant.
>
>>> If you prefer not to make that change, I think
>>> `addr_of!(...).cast_mut()` might be the best solution.
>>
>> Won't that be resulting in the wrong provenance? I.e. the pointer won't
>> be allowed to write to that location?
>>
>> I just checked with miri, it doesn't complain (even with
>> `strict-provenance`), so I guess it's fine? It feels rather wrong to me
>> to allow writing through a pointer obtained via `addr_of!`.
>
> I think that `static mut` gets the interior mutability rules that
> `UnsafeCell` has, that *const and *mut become interchangeable. Quick
> demo for the `UnsafeCell` at [1]. We would probably have to ask opsem
> to clarify.
I see. I suggested to Andreas to change it to the current version, seems
like I was wrong on this... I feel like the `SyncUnsafeCell` version is
still better, since we can avoid a `static mut`.
> Coincidentally I had been talking to Ralf about this very pattern
> before seeing this, at [2].
>
>>> Other thought: would a wrapper type make more sense here? Something like this:
>>>
>>> ```
>>> /* implementation */
>>> struct ModParam<T>(UnsafeCell<T>);
>>> [...]
>>> module! {
>>> // ...
>>> // instantiated as:
>>> // `static MY_PARAM: ModParam<i64> = ModParam::new(1);`
>>
>> We used to do this, but it lead to problems: normally the parameter has
>> a lower case name, since that's the convention in the kernel. [...]
>
> To me it seemed logical to keep the uppercase names here since it
> matches the convention for what they are (statics), and the macro
> could lowercase it for the bits exposed to the kernel. But I
> absolutely get the consistency argument here.
That would also be an option, but it might be confusing for people, they
enter a SCREAMING_CASE_SNAKE name, but when they start the kernel it
doesn't work.
>> [...] But then
>> pattern matching prioritises the static instead of introducing it as a
>> local parameter:
>>
>> let MY_PARAM = ...;
>>
>> would fail, since you can't match MY_PARAM.
>>
>> This is also the reason why they live in their own module.
>
> I'm not sure I follow the example here. It looks like it is shadowing
> a static's name as a local, why would you want that? Or if you meant
> the other way around `let SomePat(...) = MY_PARAM`, wouldn't it just
> be `let SomePat(...) = MY_PARAM.get()`? (Sorry if I missed some
> context here).
Yeah I expressed this poorly, the problem before was that you would
write:
module! {
/* ... */
params: {
foo: i64 {
default: 1,
description: "foo",
}
}
}
pub struct MyDriver {
foo: i64,
}
impl Module for MyDriver {
fn init(_: &'static ThisModule) -> Result<Self> {
let foo = foo.read();
// ^^^ cannot be named the same as a static
Ok(Self { foo })
}
}
>> But you can still do the modification of creating `ModParam` and using
>> that as the type of the static.
>
> Do you mean an expansion like this?
>
> // module_parameters is kind of a long name
> mod mod_params {
> #[allow(non_upper_case_globals)]
> static my_param: ModParam<i32> = ModParam::new(...);
> }
Yes that's what I meant, although `my_param` should be `pub(crate)`.
> I don't mind that, even if the name is a bit weird by rust conventions.
Yeah, but I think since they are in their own module, it's fine.
One thing that we need to decide would be if we want
mod_params::my_param::read()
or
mod_params::my_param.read()
I slightly prefer the first one, which would mean that the expansion
would have to be:
mod mod_params {
pub(crate) enum my_param {}
static my_param_value: ModParam<i32> = ModParam::new(...);
impl my_param {
pub(crate) fn read() {
my_param_value.read()
}
/* ... */
}
}
> (For what it's worth, I used this wrapper type pattern for a plugin
> project that does shared variables in a similar way. I have been quite
> happy with it.)
Good to know!
---
Cheers,
Benno
>
> - Trevor
>
> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=43664620f50384b7a3d5bf74ce7c3e39
> [2]: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/More.20accidental.20stabilization
