Pcode parser rewrite #425
Pcode parser rewrite #425
Conversation
… next instruction
There was a problem hiding this comment.
- The function
process_pcode_relative_jumpneeds to be refactored, because it is simply too long and that makes the function quite difficult to understand. - I did not review the unit test code, since this first review is already quite long. So review of the unit test code has to wait until the next review.
| } | ||
| Some(JmpTarget::Relative(_)) => { | ||
| if instr.contains_relative_jump_to_next_instruction() { | ||
| jump_targets.insert(instructions.peek().expect("Relative jump to next instruction, but block as no next instruction.").get_u64_address()); |
There was a problem hiding this comment.
This .expect(...) is problematic, because the case will happen in practice! For example, because the next instruction is already the start of a new block. So if peeking fails we should compute the next address from the current address. Either as instruction address + instruction size or by directly parsing the fallthrough address from Ghidra. We may have to add corresponding data to the stuff we parse from Ghidra if it is not already contained in the instruction data.
|
|
||
| // Special case: Block ends without jump. | ||
| // If all instructions are processed and current block is not new, add to blocks. | ||
| // TODO: Add branch here? |
There was a problem hiding this comment.
For this TODO I am not sure: Is this part of this PR or will it be handled in the next one? I would guess that it should be handled in this PR.
| // MIPS: Delay slots for C-Jumps. Ghidra verlegt Delay ggf vor, sodas fallthrough ggf. falsch? (alter code lookup) | ||
| panic!("Jump target address to consecutive instruction is not provided.") |
There was a problem hiding this comment.
In the long run we probably will have to remove this panic!(...) and generate a non-panicking log message instead. But if we want to keep the panic!(...) in the short term, the code comment should be a little bit more verbose and in english. Just to make sure that anyone reading the code can understand the issue here.
| // Pcode definition distinguishes between `location` and `offset`. | ||
| // Note: $(GHIDRA_PATH)/docs/languages/html/pcodedescription.html#cpui_branch | ||
| // Currently, the IR does not distinguishes these cases. | ||
| // We do nothing here. | ||
| match jmp_type { | ||
| BRANCH | CBRANCH | CALL => (), // case `location` | ||
| BRANCHIND | CALLIND | CALLOTHER | RETURN => (), // case `offset` | ||
| } |
There was a problem hiding this comment.
This code looks like it does nothing... ;-)
There was a problem hiding this comment.
Right, I left it there for future logic to "guess" locations. This might be interesting for fixing indirect jump targets..., but it is a long shot.
src/cwe_checker_lib/src/ghidra_pcode/pcode_op_simple/jumps/mod.rs
Outdated
Show resolved
Hide resolved
src/cwe_checker_lib/src/ghidra_pcode/pcode_op_simple/jumps/mod.rs
Outdated
Show resolved
Hide resolved
src/cwe_checker_lib/src/ghidra_pcode/pcode_op_simple/jumps/mod.rs
Outdated
Show resolved
Hide resolved
…nstruction, compute address
…e_checker into pcode_parser_rewrite
Enkelmann
merged commit 6076926
into
pcode_extracting_and_parsing_collection
Add translation for blocks from pcode into IR. Pcode relative jumps are considered and required splitting of blocks is implemented.