Rust bindings refactor #6309
Open
Rust bindings refactor #6309
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moves a bunch of stuff out of src/types.rs that did not belong: - Confidence - Variable - Function specific stuff - Refactored InstructionInfo, see the msp430 and riscv examples. - Renamed Function::from_raw to Function::ref_from_raw and fixed places where the ref was incremented twice - Fixed FunctionRecognizer leaking functions (see above) - Fixed some apis incorrectly returning Result where Option is clearer - Started to move destructured types to the From trait for converting to an from ffi types, see Location for an example - Started to remove bad module level imports (importing std modules like mem all over the place) - Moved some wrapper handle types to named handle field (this improves readability), see CoreArchitecture for an example - Removed some unintuitive getters, this is bad practice for Rust code, just access the field directly, see DataVariable for an example - General code cleanup, purposely did not run rustfmt, that will be a single seperate commit
- Fixed invalid views being able to invoke UB when dealing with databases - Cleaned up some helper code in dwarf_import - Fixed inverted is_null checks causing crashes! Oops!
Still a WIP, I think branch info is still invalid, need to figure out the issue there. - Fixed some invalid Ref lifetimes when constructing indirectly, see Array<DataVariable> for example - Added some more comments - Removed some "magic" functions like MLIL Function::ssa_variables There are still a bunch of invalid lifetimes that aren't crashing us due to the usage of those API's not living long enough. But they ARE an issue.
Trying to comment more TODO's as I go along. - Renamed llil::Function to llil::LowLevelILFunction for clarity and consistency - Take base structures by ref in StructureBuilder::set_base_structures to prevent premature drops - Added more raw to wrapper conversions - Removed UB prone apis - Getting rid of more core module references, use std! - Removed improper Guard usage in array impls for wrapper types with no context - Untangling the UB of the Label api, still only half done :/
- Misc formatting - Made Logger ref counted - Fixed leaking name of logger every time something was logged - Fixed the last (hopefully) of the unresolved labels - Simplified some code
componentArray was never freed
When linking you must depend on the -sys crate. This is because linker arguments (what says where to find binaryninjacore) are NOT transitive. The top level application crate MUST provide it. For more information see: - rust-lang/cargo#9554 (comment) - oxidecomputer/omicron#225
Use cargo to manage the git repo ref instead
This is where all shipped public plugins that are not arch/view/platform/lang will be at from now on Originally they were in the rust workspace, meaning they all shared a Cargo.lock which is ill-advised.
- More clarification on plugin/executable requirements - Made examples actually rust examples - Add Display impl for InstructionTextToken - Renamed feature "noexports" to "no_exports"
This really did bother me
We don't register a compatible log sink so they will just get sent into the void
This is being done in the hopes of curbing the multi-thousand line files that will occur once we flesh out the tests
Still need to add support for running tests in ci
- Architecture id's are now typed accordingly - Fix some clippy lints - Make instruction index public in LLIL - Removed useless helper functions - LLIL expressions and instruction indexes are now typed accordingly
This should show binaryninjacore-sys alongside binaryninja crate
- Remove lazy_static dependency - Remove hacky impl Debug for Type and just use the display impl - Add more debug impls - Reorder some top level namespace items - Add first type test
- Added main thread handler api - Register a headless main thread handler by default in initialization - Refactor QualifiedName to be properly owned - Loosened some type constraints on some apis involving QualifiedName - Fixed some apis that were crashing due to incorrect param types - Removed extern crate cruft for log crate - Simplified headless initialization using more wrapper apis - Fixed segments leaking because of no ref wrapper, see BinaryViewExt::segment_at - Added rstest to manage headless init in unit tests - Added some more unit tests - Refactored demangler api to be more ergonomic - Fixed minidump plugin not building
- Fixup usage of QualifiedName in plugins - Make QualifiedName more usable
- Normalized modules - Split some code out into own files - Fixed some UB in type archive and projects - Improved API around type archives and projects substantially - Added ProgressExecutor abstraction for functions which have a progress callback - Improved background task documentation and added unit tests - Added worker thread api and unit tests - Moved some owned types to ref types, this is still not complete, but this is the path forward. - Add external location/library accessors to the binary view - Added some misc documentation - Replaced mod.rs with the module name source file Still need to normalize some paths and also update some documentation surrounding that change.
We were creating multiple background tasks with the same progress text on multiple threads
- Fixed progress executor freeing itself after one iteration - Updated the last of the doc imports - Moved mainthread to main_thread - Made project creation and opening failable We could probably AsRef<ProgressExecutor> to get around the allocation and free inside the function bodies, not high priority as those functions are long running anyways.
|
I don't know if |
- Updated README to clarify offline documentation - Refactored settings api - Added settings example to show dumping settings value and specific properties - Use the workspace to depend on binaryninja and binaryninjacore-sys - Remove binaryninjacore-sys as a workspace member (its not really required)
- Rename Label to LowLevelILLabel - Update the functions label map automatically This fixed a major api blunder where the label map is returned as a reference and originally resulted in UB prone lifetime semantics. It was temporarily "fixed" with explicit label updates in the architecture implementation code. But that was less than ideal and was easy to mess up. Now the label map will be updated automatically as the location of labels is now tracked.
- Get rid of RawFunctionViewType - Add better Debug impl for Function
- Fixed the documentation icon using local files (thank you @mkrasnitski) - Fixed labels being updated and overwriting the label location used to update the label map
|
Before this is merged we should add a migration guide somewhere, most likely in the body of the first PR message. We need to wrap this up this week, there are a few downstream items that depend on this, namely WARP (see: type containers / type archives) We should put a pin in any further breaking changes and document the modules that are going to be changed in future releases (after next stable). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Opening this PR for public discussion. This needs more testing and also lacks a few major changes that make sense to land alongside the already existing set of breaking changes.
The Rust bindings are being heavily refactored for the 4.3 release. This is to address many of the shortcomings and historical issues, namely around maintaining and safety.
Some of the additions of this branch include:
rustfmt)cargo run --example)RegisterId)Migration Guide
TODO