On 24.08.24 13:27, Trevor Gross wrote:
> On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@xxxxxxxxxxxx> wrote:
>> From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>> + write!(
>> + self.param_buffer,
>> + "
>> + static mut __{name}_{param_name}_value: {param_type} = {param_default};
>
> Ah.. we need to migrate from `static mut` to `UnsafeCell` wrappers at
> some point. Since `module!` already uses `static mut`, this may need
IIRC Alice wanted to do something about that.
> to happen separately - meaning I don't think we need to block on
> making any change here.
>
> This would mean adding an `UnsafeSyncCell` / `RacyCell` (just a
> wrapper around `UnsafeCell` that always implements `Sync`), using
> `UnsafeSyncCell<{param_type}>` as the type here, and changing from
> `static mut` to just `static`.
>
> (I can take a look at doing this change for existing `static mut` in
> the near future).
>
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index efacca63c897..a65bd0233843 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -263,7 +263,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
>> # Compile Rust sources (.rs)
>> # ---------------------------------------------------------------------------
>>
>> -rust_allowed_features := new_uninit
>> +rust_allowed_features := new_uninit,const_mut_refs
>
> 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.
> 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!`.
> ---
>
> Other thought: would a wrapper type make more sense here? Something like this:
>
> ```
> /* implementation */
> struct ModParam<T>(UnsafeCell<T>);
>
> // `Parameter` is the existing `ModParameter` (could be
> // any name). It could be sealed.
> impl<T: Parameter> ModParam<T> {
> #[doc(hidden)] // used in the macro
> fn new(value: T) -> Self { ... }
>
> fn get(&self) -> T::Value { ... }
> fn set(&self, v: T::Value) { ... }
> }
> ```
>
> With usage:
>
> ```
> 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. 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.
But you can still do the modification of creating `ModParam` and using
that as the type of the static.
---
Cheers,
Benno
> MY_PARAM: i64 {
> default: 1,
> description: "foo",
> },
> }
>
> fn foo() {
> pr_info!("My param is {}", MY_PARAM.get());
> }
> ```
>
> Advantages I see:
> - You bring your own name, rather than being scoped and needing to
> remember the module name
> - You can check `ModParam` in the docs to see the API, rather than
> needing to refer to source for the exact signatures of `read` and
> `write`
> - The interface gets a bit more like a mutex or atomic
>
> - Trevor
