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 assembly parser #9
Conversation
Uses the combine crate to parse ubpf-style assembly to Instruction structs. A future assembler module will consume these, check that the instruction and operands are valid, and output ebpf::Insn.
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.
Hi @rlane, many thanks for this first PR! The parser looks very clean. The minor comments I added below were returned by rust-clippy (Rust linter), would you mind fixing them before we merge? Or I can add a commit once it's merged if you prefer me to do it.
src/asm_parser.rs
Outdated
_ => 1, | ||
}); | ||
let hex = | ||
string("0x").with(many1(hex_digit())).map(|x: String| i64::from_str_radix(&x, 16).unwrap()); |
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.
Single line in file that is more than 100 character long, do you think we could split it?
src/asm_parser.rs
Outdated
fn operand<I>(input: I) -> ParseResult<Operand, I> | ||
where I: Stream<Item = char> | ||
{ | ||
let register_operand = parser(register).map(|x: i64| Operand::Register(x)); |
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.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: redundant closure found, #[warn(redundant_closure)] on by default
--> src/asm_parser.rs:66:49
|
66 | let register_operand = parser(register).map(|x: i64| Operand::Register(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove closure as shown:
| let register_operand = parser(register).map(Operand::Register);
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#redundant_closure
src/asm_parser.rs
Outdated
where I: Stream<Item = char> | ||
{ | ||
let register_operand = parser(register).map(|x: i64| Operand::Register(x)); | ||
let immediate = parser(integer).map(|x: i64| Operand::Integer(x)); |
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.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: redundant closure found, #[warn(redundant_closure)] on by default
--> src/asm_parser.rs:67:41
|
67 | let immediate = parser(integer).map(|x: i64| Operand::Integer(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove closure as shown:
| let immediate = parser(integer).map(Operand::Integer);
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#redundant_closure
src/asm_parser.rs
Outdated
fn format_info(info: &Info<char, &str>) -> String { | ||
match *info { | ||
Info::Token(x) => format!("{:?}", x), | ||
Info::Range(ref x) => format!("{:?}", x), |
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.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: this pattern creates a reference to a reference, #[warn(needless_borrow)] on by default
--> src/asm_parser.rs:92:21
|
92 | Info::Range(ref x) => format!("{:?}", x),
| ^^^^^
|
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrow
src/asm_parser.rs
Outdated
Info::Token(x) => format!("{:?}", x), | ||
Info::Range(ref x) => format!("{:?}", x), | ||
Info::Owned(ref x) => x.clone(), | ||
Info::Borrowed(ref x) => x.to_string(), |
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.
$ cargo clippy -- -A doc_markdown -A match_same_arms
[...]
warning: this pattern creates a reference to a reference, #[warn(needless_borrow)] on by default
--> src/asm_parser.rs:94:24
|
94 | Info::Borrowed(ref x) => x.to_string(),
| ^^^^^
|
= help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrow
(Note we do need to keep the ref
on line 93, though.)
src/lib.rs
Outdated
|
||
pub mod disassembler; | ||
pub mod ebpf; | ||
pub mod helpers; | ||
pub mod asm_parser; |
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.
Ah also, can we keep includes in alphabetical order (in each one of the crate/public module/private module section)? Also, for dependencies in the Cargo.toml
file?
Thanks for the pointer to rust-clippy. Pushed a few commits. |
Thanks a lot! Merging. |
Uses the combine crate to parse ubpf-style assembly to Instruction
structs. A future assembler module will consume these, check that
the instruction and operands are valid, and output ebpf::Insn.