rust: changes to allow 2024 edition #1150

crop2000 · 2025-05-31T15:07:38Z

No description provided.

crop2000 · 2025-05-31T15:20:58Z

@bluenote10 @sletz this is the same pr as the one mentioned in #1005 it changes the scope of what is considered unsafe rust code and with this solves the warning that turned into an error in the 2024 edition of rust. this change is backwards compatible to older rust editions.

bluenote10 · 2025-06-02T07:23:30Z

I would need to take a closer, but on first glance I'm somewhat torn: As far as I can see the error in the 2024 edition is a genuine one, i.e., the current logic has undefined behavior. Just silencing it with more unsafe is technically unsound, because one should only use unsafe if one has other means of guaranteeing that the code is valid, which is (likely?) not the case here. It could make it harder to actually come up with a proper solution, and could even hide other UB issues?

crop2000 · 2025-06-02T11:04:14Z

The code is valid if you can garanty that you don't initialize the class while you use it elsewhere. To get close to a sound solution one needs to deprecate the init method.

bluenote10 · 2025-06-02T11:48:47Z

The code is valid if you can garanty that you don't initialize the class while you use it elsewhere.

The "if" in that sentence is exactly the reason why it is unsound. For instance, from the Rustonomicon:

We say that such a correct unsafely implemented function is sound, meaning that safe code cannot cause Undefined Behavior through it [...].

Similarly, if we'd stick to the safety comments convention, we'd exactly lack a justification why that unsafe block is safe as far as I can see. As long as there is a condition like if you can garanty that you don't initialize the class while you use it elsewhere then you would have to pass the unsafe to the outside.

I'd assume that would be pretty inconvenient, but the warning/error is genuine, and just claiming safety where there isn't doesn't really solve the problem. I need to find some time to look into the other ideas mentioned in #1005.

crop2000 · 2025-06-02T15:08:32Z

The current implementation (with and without my changes) has the following relevant qualities:
Tables

have no overhead when accessed for reading (no atomic locks etc.)
can be read by multiple instances
can be read without overhead from multiple threads
can be initialized using regular functions.
Right now I have no idea how to archive this without major changes to Faust without mutable static.

If we accept that we have a mutable static we should restrict the possibility to mutate it.
A:
We could add an additional static variable that counts the instances of dsp objects and blocks mutation of the variable via class_init if instances exists. This change would be a breaking change.
B:
Currently the re-initialization of class variables has no reasonable effect. Because sample rate changes do not affect tables: #1151 . so one could simple not execute the content of class_init() if it had been already initialized (also using a static atomic variable). this solution is backward compatible. But it would imply that #1151 stay unsolved or is solved in a way that limits the use of Sample rate.

static instance_count: AtomicUsize = AtomicUsize::new(0); //for A
static init_lock: AtomicBool = AtomicBool::new(false); //for B

...

    pub fn class_init(sample_rate: i32) {
        // A: this solution breaks init
        if instance_count.load(Ordering::Relaxed) == 0 {
            let mut sig0: SinOscSIG0 = newSinOscSIG0();
            sig0.instance_initSinOscSIG0(sample_rate);
            sig0.fillSinOscSIG0(65536, &raw mut ftbl0SinOscSIG0);
        } else {
            panic!("don't try to init while dsp is in use")
        }

        // B: solution that works until fSampleRate use becomes possible
        if !init_lock.swap(true, Ordering::Relaxed) {
            let mut sig0: SinOscSIG0 = newSinOscSIG0();
            sig0.instance_initSinOscSIG0(sample_rate);
            sig0.fillSinOscSIG0(65536, &raw mut ftbl0SinOscSIG0);
        }
    }

(i think issue #737 is also related because the use of sample rate becomes more likely if one has more complex table filling functions.)

crop2000 · 2025-06-02T17:03:17Z

if we wrap those ideas into a type and provide functions then the meaning doesn't really change just the scope of the unsafe clauses.
here realized for solution B

static ftbl0SinOscSIG0_th: UnsafeTableHolder = UnsafeTableHolder {
    table: UnsafeCell::new([0.0; 65536]),
    lock: AtomicUsize::new(0),
};
fn ftbl0SinOscSIG0_th_ref() -> &'static [f32] {
    unsafe { &mut *ftbl0SinOscSIG0_th.table.get() }
}
fn ftbl0SinOscSIG0_th_mut_ref() -> &'static mut [f32] {
    assert!(
        !(ftbl0SinOscSIG0_th.lock.load(Ordering::Relaxed) > 0),
        "don't try"
    );
    unsafe { &mut *ftbl0SinOscSIG0_th.table.get() }
}

bluenote10 · 2025-06-02T20:14:25Z

By using thread_local it would be possible to avoid any unsafe altogether. Here is a small example. Given the following dsp code:

import("stdfaust.lib");

envelope(gate) = gate : en.adsr(0.01, 0.01, 0.3, 0.1);
synth(gate, freq) = envelope(gate) * os.osc(freq);

process = synth;

I've made the following manual edits to the generated code (full code example):

Instead of using plain/global statics, I'm using a thread-local static:

The fillDspSIG0 method doesn't have to change at all, it can still take a table: &mut [FaustFloat] as argument, filling it as before. The usage in class_init becomes:

The usage inside compute comes down to ftbl0DspSIG0.with_borrow(|ftbl0DspSIG0_borrow| { ftbl0DspSIG0_borrow[...]). For performance reasons it would be good to do the borrow not on every sample in the for loop, but rather pull the borrow out of the loop like this:

Notice that there are no unsafe usages needed at all, because everything can be done entirely in safe Rust.

The only assumption is that the DSP is consistently used from the same thread. Considering that this is pretty much the standard in audio programming, I'd be tempted to go for a fully safe thread local solution instead of trying to cook up a sound unsafe usage.

crop2000 · 2025-06-02T20:46:55Z

The problem is that this is not a helpful solution for the multi thread case which is the only case that I am interested in.

bluenote10 · 2025-06-02T21:10:09Z

The problem is that this is not a helpful solution for the multi thread case which is the only case that I am interested in.

It could be extended to work there as well, by also storing a initialized: bool in the thread local storage and running the initialization if it's uninitialized, then setting it to true.

crop2000 · 2025-06-02T21:23:51Z

Are interested to implement this?

crop2000 · 2025-06-02T21:33:03Z

I see that you use RefCell adds the overhead of a atomic check at every access that's not good. and not necessary.

bluenote10 · 2025-06-03T06:54:19Z

Are interested to implement this?

I can look into it, but it may take a while until I get to it.

I see that you use RefCell adds the overhead of a atomic check at every access that's not good. and not necessary.

A RefCell doesn't use an atomic bool, just a regular bool (because cells are not sync), and that's quite negligible according to my benchmarks. The Rust code runs exactly at C++ speed or even faster.

In fact, I also want to look into Mutex or RwLock. I could imagine that if the locking happens on the outer scope of compute (and not on sample level) it is also completely negligible. If the performance is good enough there is little reason not to go for Mutex/RwLock.

and not necessary.

It is necessary. We must not violate Rust's core safety model. We will not be able to solve "static mut" without any kind of check and declare it "safe" as far as I can see.

crop2000 · 2025-06-03T09:15:13Z

I suggested checks in this discussion.

bluenote10 · 2025-06-03T11:12:25Z

I suggested checks in this discussion.

Yes, and they looked promising, but they essentially looked like an attempt at re-implementing what e.g. Mutex already provides in the standard library. Getting these things right is hard. The general recommendation is to not even try, but stick to the safe high-level abstractions the standard library provides. Since this use case looks pretty much standard, there probably no good reason to mess with unsafe directly.

crop2000 · 2025-06-03T11:37:23Z

This pr does not make the code less correct than before.
Are you open that this change (without the proposed locks) is merged until you implemented your solution?

bluenote10

This pr does not make the code less correct than before. Are you open that this change (without the proposed locks) is merged until you implemented your solution?

If it simplifies things for you, we can go ahead and merge it.

Conceptually it's just a step in the wrong direction. You're right that the code was unsound all the time. The purpose of this warning/error is however to point out that it is unsound, and to solve it properly. Adding an unsound unsafe { ... } block like we do now is basically what the warning/error was trying to prevent. In a sense, an invalid logic with a warning is marginally "better" than an invalid logic where the warning has been incorrectly suppressed.

compiler/generator/rust/rust_code_container.cpp

bluenote10 · 2025-06-03T20:35:05Z

compiler/generator/rust/rust_code_container.cpp

@@ -273,7 +275,7 @@ void RustCodeContainer::produceClass()
        tab(n + 1, *fOut);
        *fOut << "#[cfg_attr(not(target_os = \"windows\"), link(name = \"m\"))]";
        tab(n + 1, *fOut);
-        *fOut << "extern \"C\" {";
+        *fOut << "unsafe extern \"C\" {";


What is the reason for adding these unsafe here?

Edition 2024 changed rule to make it explicit.

crop2000 · 2025-06-04T18:34:37Z

@sletz what is your opinion?

crop2000 force-pushed the rust_2024 branch from a9b2eeb to b2de608 Compare May 31, 2025 15:19

bluenote10 reviewed Jun 3, 2025

View reviewed changes

crop2000 force-pushed the rust_2024 branch from b2de608 to 9cbd4d4 Compare June 3, 2025 23:35

rust: changes to allow 2024 edition

f80cbba

crop2000 force-pushed the rust_2024 branch from 9cbd4d4 to f80cbba Compare June 4, 2025 08:49

bluenote10 mentioned this pull request Jun 4, 2025

[draft] safe static mut based on RwLock #1152

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rust: changes to allow 2024 edition #1150

rust: changes to allow 2024 edition #1150

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rust: changes to allow 2024 edition #1150

Are you sure you want to change the base?

rust: changes to allow 2024 edition #1150

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!