[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #12 from Andreas Grois --- I think the MR is done for now. I've modified the Demo, Example and Templates to compile with the immutable API, modified the tutorial and readme and written a changelog. While working on the Demo I found another issue, with Tree Models, new_data_ready() and threading. The auto-generated C++ code was accessing the data model from the thread that was emitting new_data_ready() before sending the Signal, causing an unexpected access to data owned by the UI-thread. Projects could have worked around this by storing their data behind an RwLock, but given that the access to the data is unnecessary for the connection between new_data_ready() and fetch_more() to work (only the internal Id is forwarded to Rust for fetch_more(), all other data is discarded anyhow), I now removed the model-access from the worker-thread. Trees still have other Signals that access the model from the thread emitting the Signal, but those Signals are directly related to user-facing data. If those are to be emitted from a worker thread, the data has to be behind an RwLock/Mutex anyhow. In any case, I've removed the WIP status from the MR, and will now start using the modified version of the binding generator in my own project. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #11 from Andreas Grois --- Thanks! I'll start modifying Demo, ToDo and the tutorial code snippets then. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #10 from Jos van den Oever --- And yes to 0.4.0 in case of merge indeed. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #9 from Jos van den Oever --- Yes, the resulting master should be as much functional as the current one. I'll have a look this week to see what the means. I've two projects I'd like to test with these changes, one public (mailmodel) and one private. Commit squashing is not something I've a strong opinion on. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #8 from Andreas Grois --- Correction: The cmake build currently actually works. I didn't try it before, assuming it would just overwrite the Demo's Rust bindings, but obviously that's not the case. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #7 from Andreas Grois --- I think the first draft of the changes is ready for an initial review, in case you would like to merge them: https://invent.kde.org/sdk/rust-qt-binding-generator/-/merge_requests/2 It's still marked as WIP, because there are a few things that still need attention before merging is possible: - First of all: Do you actually want to merge that change? If not, I'm perfectly happy with having it on a separate branch. It's quite a breaking change after all, given it touches the function signature of all setters and some getters, so everyone switching to the changed version needs to update a lot of Rust code... - Only the generator and the tests have been changed up to now. I guess I should update Demo and Todos example as well? Just so the cmake build does actually work again, and to see if the changes cause any serious road-blocks missed by the tests. - What about the documentation? Should I update the code samples included in the Tutorial texts as well? Or add a note that the examples were written for version 0.3.6 and newer versions might require slightly different function signatures? - While speaking about version numbers: With this breaking change the version number should probably be incremented to 0.4.0, I guess? - When merging, the commits should probably be squashed. The main program always "worked" more or less, but the tests were most of the time in broken state, and neither Demo nor Example are currently compiling. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087
--- Comment #6 from Andreas Grois ---
To be honest I haven't spent much thought on the RefCell idea.
The plan would have been to replace Box::into_raw(Box::new(d_{})) by
Box::into_raw(Box::new(RefCell::new(d_{}))) and all usages of that pointer,
which now typically are (&*ptr), by (*ptr).borrow().
As those methods are only ever called from C++, I had assumed that all calls
originate from the UI thread, but that's a restriction that the Rust bindings
probably shouldn't impose on the C++ side...
A Mutex at that point in the bindings would again impose unnecessary
restrictions, as it would deadlock in all situations where there currently is
Undefined Behaviour.
What might work would be an RwLock, but the more I think about it, the less I
think doing that is worthwhile, given that the original issue, aliasing a
mutable pointer, is already resolved by limiting the code to immutable
references, and any kind of guard introduced at that location would probably
just serve as a debugging tool.
For the moment I think I'll just leave the calls from C++ to Rust as they are,
with raw pointers/references.
--
You are receiving this mail because:
You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #5 from Jos van den Oever --- That all seems a sensible. The code was originally written when I was less familiar with rust's rules. The rust_by_function as the only way to get data is more verbose in implementation.rs and might have performance overhead, but the binding to a GUI is not the place to worry about that. What would the RefCell code look like? I'm asking because I'm modifying a model from multiple threads, so a Mutex might be more appropriate. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #4 from Andreas Grois --- I've had a look at the linked branch. That does not only contain the fix for the mutability/aliasing issue, but functionality (replaceable objects) which I don't want/need for my use case. I've started a new branch based off current master, where I'm planning to make the following changes: o) Make every self be instead. o) Remove RustByFunction. In an immuatable context RustByFunction is the only reasonable choice for complex (non-object) types, as one can't leak a reference out of a RefCell's borrow or Mutex' lock. Since RustByFunction anyhow only works for (non-object) complex types on master, removing that user-choice does not remove functionality (that would not get lost anyhow by the switch to immutable contexts). o) Remove mut function support, for obvious reasons. o) Make functions and list/tree item properties that return non-object complex types use a similar function signature as property getters with rust-by-function. This is again because one cannot leak references out of locks/borrows. I'm also considering to wrap the creation of rust objects (what is currently just a heap allocation) in a RefCell so it can catch aliasing violations at runtime (think: some broken C++ code calling _free() as reaction to a signal sent by the same object). That should not be necessary from a correctness standpoint (again: if I didn't misunderstand something), but it'd be a nice additional safeguard. I can't make any promises when (or even if) I will finish those changes though (it's after all a spare-time project for me, and today is the last day of my vacation...), but I aim to have a first draft (code + tests) pushed to gitlab at some point during next week. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #3 from Andreas Grois --- Sorry, this is not really relevant to the bug, but I just have to correct myself here. I was too tired yesterday to realize that also the self -> situation can cause issues. Actually #406178 describes such a call stack. With Rust's aliasing rules the compiler can assume that the call to C++ (which does not take any references into the memory pointed to by self) is independent of self, so it need not guarantee that the memory referenced by self is in a consistent (or up-to-date) state when it triggers the signal. -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 --- Comment #2 from Andreas Grois --- I was not aware that there is already work ongoing in that regards. Is this bug report then really a duplicate of 406178? Using internal mutability with a Mutex indeed sounds like the most viable approach. It has the drawback of causing runtime errors, but they should be reproducible, so easy to find/fix. I'll grab that branch tomorrow and have a closer look. I'm also considering just sticking to current master for now, as I haven't had any actual issues in my current project, and just noticed that aliasing problem while writing a wrapper for an Emitter. (I'm getting self -> in the call stacks, but I don't see how that could cause troubles, with no mutation happening on the aliased memory - but I might misunderstand something here.) -- You are receiving this mail because: You are watching all bug changes.
[rust-qt-binding-generator] [Bug 448087] rust-qt-binding-generator: Undefined Behaviour: One can obtain a mutable reference to a Rust struct from an immutable reference.
https://bugs.kde.org/show_bug.cgi?id=448087 Jos van den Oever changed: What|Removed |Added Status|REPORTED|CONFIRMED Ever confirmed|0 |1 --- Comment #1 from Jos van den Oever --- I agree with the analysis. This is a fundamental problem that I did not realize before. One can get rid of the undefined behavior by changing all ' self' to '' in the interface and leave the problem for the implementer. (This is also what kdebuac.rhn describes in https://bugs.kde.org/show_bug.cgi?id=406178 ) They might then use a Mutex. That would then lead to a deadlock instead of undefined behavior. The idea of a QueuedConnection would not solve the problem general problem. One would need to guarantee that any code that calls into C++ does not call back in Rust. The only safe way is to have only immutable references. The read-only experimental branch might be the solution: https://gitlab.com/rhn/rust-qt-bindings-generator/-/commits/simplify_ro -- You are receiving this mail because: You are watching all bug changes.
