-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a trait/derive macro for transforming Rust type definitions into Binja Type
objects
#5256
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! Sorry for the delay in getting back to you. This looks cleaner than I ever imagined it would be, and I'd love to get this merged.
The following need to occur before this can be merged:
- @psifertex : Please let us know what we need to do about licensing; This PR adds dependencies for API consumers who explicitly turn on an additional feature (in their Cargo.toml) and does not add any dependencies to the build (unless/until we start using this feature internally). One concern I have at a cursory glance is that Elain only specifies that it's MIT/Apache in their Cargo.toml and doesn't otherwise have an actual copy of the license in the source code/repo.
- @mkrasnitski : I'm not going to be comfortable merging this until we have tests to ensure it doesn't go stale... But given that it generates API code that'll get checked by the compiler every build, I'm uniquely unconcerned about runtime bugs! I think adding something to the examples that has as much code coverage in the macro as possible will suffice.
- @mkrasnitski : This has to be one of the best documented PRs we've ever gotten. Please copy-pasta that into some doc strings too. Feel free to add to the developer guide, even, if you think this necessitates that.
- @mkrasnitski : Please add the
#[name = "..."]
thing. - @mkrasnitski : Similarly, I like the idea of the struct decorator for pointer widths rather than specifying on each field.
Other feedback/questions:
- I'm not sure I understand how
#[repr(align)]
works. Could you please provide an example? - I'm fine with
#[repr(C)]
and#[repr(<int>)]
(I think we support u128 as a type in the core btw, and I think this repr solution is actually really nice for the enums), but is there a way to omit#[named]
? Is it possible to fall back on to being named if the type is otherwise unrecognized?
...this PR also actually has me thinking about a #[tokio::main]
sorta thing that'll do the script initialization/shutdown for you.
For sure, I'll look into adding some tests. I probably should have included them from the start since I was using a standalone Rust plugin to test how the generated types would look in the UI. I see that
Yeah, this is a good idea in principle. I'm thinking there should be a single attribute here instead of two (
Decorating a type declaration with // Assuming we know T
const ALIGN: usize = std::mem::align_of::<T>();
const SIZE: usize = std::mem::size_of::<T>();
#[repr(C)]
#[repr(align(ALIGN))] // invalid syntax - we use `elain` to accomplish this
struct Mock {
t: [u8; SIZE],
} We've basically created a type with arbitrary size and alignment, since we can change the values of the
There might be some cases where users might want to keep the type definition inline rather than reference a named type. I think for that reason, always falling back to the Rust name as a default might be too opinionated. |
Thanks for the additional feedback. I already mentioned this in Slack, but don't worry about
Personally I'm fine with
What about the opposite then? Add a |
IMO this feels slightly awkward, since then the default behavior with no attributes would be to create a named type using the Rust type's name, and then providing
For some reason, I think if we wrap all three in a Btw, should the value of |
dd8f0c0
to
5f20ed2
Compare
5f20ed2
to
27bd9aa
Compare
Alright, I think all the behavioral changes are taken care of. To briefly demonstrate: a type that derives #[derive(AbstractType)]
#[repr(u64)]
enum E {
One = 1,
Two = 2,
}
#[derive(AbstractType)]
#[repr(C)]
#[binja(pointer_width = 8)]
struct S {
first: u64,
#[binja(named)]
second: *const E,
} What remains to be done is adding documentation and tests. Ideally I would like #5435 to be resolved and then rebase on top of it, however that might take some time so in the meantime I'll go with the original suggestion of adding to |
699b92f
to
cfb8a64
Compare
eb25577
to
9334c6f
Compare
We accomplish this by generating alternative struct layouts with swapped out field types, and recursively calculating size and alignment using associated constants on the `AbstractType` trait
The attribute supports three properties: - Specifying pointer width is done for the whole struct/union now instead of each field using a `#[binja(pointer_width = <int>)]` property. - Specifying named fields is done either with `#[binja(name = "...")]`, or `#[binja(named)]` which will use the name of the Rust type as a default value.
9334c6f
to
d1205c9
Compare
@KyleMiles This PR should be ready for re-review now. In total, the new changes since you last reviewed are the commit range 98fde41 through 9a23a8e (comparison). In summary:
The only thing missing at this point is support for fields of type |
`StructureBuilder::insert` doesn't affect the alignment of the type, so we need to always explicitly specify alignment.
d1205c9
to
9a23a8e
Compare
I have discussed this with @mkrasnitski and the plan forward for this is as a separate crate. We need to actually version the We should aim to have a myriad of helper crates instead of maintaining large effort abstractions/helpers in this repository. This will allow greater flexibility and a clear line between where the API ends and the helpers begin, something that is not clear within our python bindings. We are going to keep this PR open until the above is at-least partially completed. |
Supersedes #4545.
This time around I was able to see this work through to a good enough state to not publish as a draft. Obviously, I would very much like feedback on this, as introducing a proc-macro into the codebase adds a clear extra maintenance burden that normal Rust code doesn't come with.
Overview
This PR adds the following trait:
Implementing this trait allows creating a
Type
object corresponding to the layout and representation of whatever type implements it. For example, take the following type:Calling
S::resolve_type()
produces the following type (shown after defining it in a view):Derive macro
The accompanying derive macro is very convenient for automatically generating implementations of the
AbstractType
trait, as long as the deriving type meets the following requirements:#[repr(C)]
, and must have named fields.#[repr(C)]
but instead#[repr(<int>)]
(e.g. u8, u64). Also, variants must not contain any data, and must have an explicit discriminant. Note thatusize
andisize
are not supported as enum reprs.Alignment
The derive macro supports
#[repr(packed)]
and#[repr(align)]
on types and will propagate that information into the generated BinjaType
(i.e. marking a struct#[repr(packed)]
will result in a__packed
type in Binja).Named types
Named types are supported by decorating a field with a
#[binja(named)]
attribute. As an example:The result of
S::resolve_type()
is the following type:Note that defining
S
in a view does not require thatE
already be defined. As long asE
is eventually defined, then clicking on the type ofsecond
will redirect to the correct type.Pointers
If a struct contains pointer fields (of type
*const T
or*mut T
) it must be decorated with#[binja(pointer_width = <int>)]
. Since pointers can change width depending on architecture, users must specify the width ahead of time. As an example, if we modifyS
from above to look like:Then we will get the following: