Kevin Wolf <kw...@redhat.com> writes:
> Am 22.10.2024 um 08:00 hat Junjie Mao geschrieben:
>>
>> Paolo Bonzini <pbonz...@redhat.com> writes:
>>
>> > On Tue, Oct 22, 2024 at 4:12 AM Junjie Mao <junjie....@hotmail.com> wrote:
>> >> > + ($type:ident => $body:block) => {
>> >> > + const _: () = {
>> >> > + #[used]
>> >> > + #[cfg_attr(
>> >> > + not(any(target_vendor = "apple", target_os =
>> >> > "windows")),
>> >> > + link_section = ".init_array"
>> >> > + )]
>> >> > + #[cfg_attr(target_vendor = "apple", link_section =
>> >> > "__DATA,__mod_init_func")]
>> >> > + #[cfg_attr(target_os = "windows", link_section =
>> >> > ".CRT$XCU")]
>> >> > + pub static LOAD_MODULE: extern "C" fn() = {
>> >> > + extern "C" fn init_fn() {
>> >>
>> >> init_fn() should be unsafe fn according to the signature of
>> >> register_module_init.
>> >
>> > I think it *can* be unsafe (which bindgen does by default). It's
>> > always okay to pass a non-unsafe function as unsafe function pointer:
>> >
>> > fn f() {
>> > println!("abc");
>> > }
>> >
>> > fn g(pf: unsafe fn()) {
>> > unsafe {
>> > pf();
>> > }
>> > }
>> >
>> > fn main() {
>> > g(f);
>> > }
>> >
>> >> Being unsafe fn also makes sense here because it
>> >> is the caller, not init_fn() itself, that is responsible for the safety of
>> >> the unsafe code in the body.
>> >
>> > Isn't it the opposite? Since the caller of module_init! is responsible
>> > for the safety, init_fn() itself can be safe.
>>
>> My understanding is that:
>>
>> 1. init_fn() is called by QEMU QOM subsystem with certain timing
>> (e.g., called after main()) and concurrency (e.g., all init_fn()
>> are called sequentially) preconditions.
>
> Though these conditions are not a matter of safety, but of correctness.
>
The "no concurrency" condition is related to Rust safety. Deep in
type_register_static(), which is commonly used in such init_fn(), there
is a modification to mutable statics in type_table_get():
static GHashTable *type_table_get(void)
{
static GHashTable *type_table;
if (type_table == NULL) {
type_table = g_hash_table_new(g_str_hash, g_str_equal);
}
return type_table;
}
It'll cause data race when multiple init_fn() are concurrently called,
and data races has UB.
Also, glib hashtables are not automatically thread-safe [1]:
... individual data structure instances are not automatically locked
for performance reasons. For example, you must coordinate accesses
to the same GHashTable from multiple threads.
[1] https://docs.gtk.org/glib/threads.html
I don't have evidence yet for the relationship between timing condition
and Rust safety, though.
>> 2. The caller of module_init! is responsible for justifying the safety
>> of their $body under the preconditions established in #1.
>>
>> "unsafe fn" in Rust is designed to mark functions with safety
>> preconditions so that the compiler can raise an error if the caller
>> forgets that it has those preconditions to uphold [1].
>
> I don't think we expect to have preconditions here that are required for
> safety (in the sense with which the term is used in Rust).
>
> But safe code is not automatically correct.
>
> If you added "unsafe" for every function that requires some
> preconditions to be met so that it functions correctly (instead of only
> so that it doesn't have undefined behaviour on the language level), then
> you would annotate almost every function as "unsafe".
>
> I think the rule of thumb for marking a function unsafe is when you have
> unsafe code inside of it whose safety (not correctness!) depends on a
> condition that the caller must reason about because you can't guarantee
> it in the function itself.
I fully agree with your rules for "unsafe fn" uses.
--
Best Regards
Junjie Mao
>
> This macro should probably only be used with code that can guarantee
> the safety of its unsafe blocks in itself. The C side of constructors
> can't make many guarantees anyway, and there is nobody who would
> reason about them.
>
> Kevin
