Add new DataMember constructors, extend, shift.#6
Add new DataMember constructors, extend, shift.#6Kiiyya wants to merge 2 commits intotommoa:masterfrom
Conversation
tommoa
left a comment
tommoa
left a comment
There was a problem hiding this comment.
Hey, thanks for your continued work on this.
I have a few comments and nitpicks about things before merging. Feel free to argue and disagree with me on them, it has been a while since I went through all this stuff!
| /// process, then following a bunch of pointers and offsets, which may be negative. | ||
| /// If you use CheatEngine and get a pointer of form "MyModule.dll + 0x12345678", plus a bunch | ||
| /// of offsets, then you want to put the base address of the module as `addr`, `0x12345678` as | ||
| /// the first offset, then any further offsets etc. |
There was a problem hiding this comment.
Could you add a newline after this? I think that when it gets rendered into HTML, single linebreaks are removed.
|
|
||
| /// Creates a new `DataMember` by appending some more offets. Useful when you have a data | ||
| /// structure, and want to refer to multiple fields in it, or use it as a starting point | ||
| /// for chasing down more pointers. |
| process: self.process, | ||
| _phantom: std::marker::PhantomData, | ||
| }; | ||
| let new = clone.offsets[self.offsets.len() - 1].wrapping_add(n_bytes as usize); |
There was a problem hiding this comment.
The number of offsets needs to be checked here, or else you'll panic if someone just called DataMember::new() and passed it to this function.
I'm not sure of the best way to handle this. Maybe returning an Option<DataMember<...>> and doing something like let new = clone.offsets.last_mut()?.wrapping_add(n_bytes as usize);?
There was a problem hiding this comment.
Another potential option would be turning it into extend() if there are no offsets in the existing data member.
| /// responsibility to make sure you know what you point to. | ||
| #[allow(clippy::cast_sign_loss)] | ||
| #[must_use] | ||
| pub fn extend<TNew>(&self, more_offsets: Vec<isize>) -> DataMember<TNew> { |
There was a problem hiding this comment.
Can these functions be put on the Memory trait instead? I think that they would also be useful on LocalMember.
| /// [`Pid`]: type.Pid.htm | ||
| #[must_use] | ||
| #[allow(clippy::cast_sign_loss)] | ||
| pub fn new_offset_relative(handle: ProcessHandle, offsets: Vec<isize>) -> Self { |
There was a problem hiding this comment.
Could you copy these functions to LocalMember as well?
| // So second_player.read() right now would just get you the pointer to the player. | ||
| let second_player = DataMember::<*mut Player>::new_addr( | ||
| handle, | ||
| (&game as *const _ as usize) + 8 + handle.get_pointer_width().pointer_width_bytes(), |
There was a problem hiding this comment.
I think you can do handle.get_pointer_width() as usize here.
| /// Returns the amount of bytes which a pointer takes up on this architecture. | ||
| /// E.g. 4 for 32bit, 8 for 64bit, etc. | ||
| #[must_use] | ||
| pub fn pointer_width_bytes(self) -> usize { |
There was a problem hiding this comment.
You can already cast Architecture to usize with architecture as usize. Perhaps what's needed instead is making that availability more clear in documentation?
There was a problem hiding this comment.
I thought casting an enum was pretty unintuitive, and even if documentation exists but someone stumbles upon it in the wild, they will have no clue what's going on. Especially when that person is new to rust too, and have no idea that it returns the Arch64Bit = 8 enum value.
I would instead rename get_pointer_width to get_arch, and keep the function I added (or rename it to pointer_width).
There was a problem hiding this comment.
That's the entire point of the enum being repr(u8), although I guess you'd probably need to read the nomicon to know that.
I'd probably prefer not adding another function, do you think it'd be okay to just add a doc comment to the Architecture struct with something along the lines of:
This enum is `repr(u8)`, which means that it can be treated exactly the same as a `u8` with the same value as the number of bytes in the architecture.
tommoa
force-pushed
the
master
branch
3 times, most recently
from
5573198 to
bf01b32
Compare
2 participants
Since the requested changes on #4 would have required me to undo a lot of stuff, I decided to make some new pull requests.
This is a small one, mostly just for convenience.