PPC Support #204
PPC Support #204
Conversation
|
I'm still going through the comments (super helpful) - there are a few "heavier" changes that I might want to separate out into their own commits, so only things that I've 👍 are things that I have resolved. (Some things I commented on but did not change yet) |
| Register("cr0_eq"), | ||
| Register("cr0_so"), | ||
| ] + reg | ||
| return reg |
There was a problem hiding this comment.
Hm, pre-existing: the
elif (len(mn_parts) >= 2 and mn_parts[1] == "d") or mnemonic == "ldc1":
set_reg(target.other_f64_reg(), SecondF64Half())
case isn't covered here. All these mismatches scare me, given that they may result in subtle errors with phis. I wonder if we can assert somehow that output_regs_for_instr returns the exact set of output registers... or make that somehow obviously true by construction, but that seems hard given current architecture.
There was a problem hiding this comment.
I fixed the case for these mnemonics, but I agree it'd be nice to revisit this and figure out how to remove the duplication between output_regs_for_instr and process_instr.
src/translate.py
Outdated
| branch_condition = arch.instrs_decctr_branches[mnemonic](args) | ||
|
|
||
| ctr = Register("ctr") | ||
| set_reg(ctr, BinaryOp.int(args.regs[ctr], "-", Literal(1))) |
There was a problem hiding this comment.
I believe the decrement happens before the comparison. Maybe we should add an asm pattern that splits this into two fictive instructions (decrement + conditional branch) to reduce the number of instruction categories?
There was a problem hiding this comment.
I did this refactor, but it did end up making the output a bit worse...
a8ac096#diff-425a544ce90858581aeed48b74e4a718eacbaf51919d7af56c1be05eaf802686
And despite the ordering bug here, most of the C output was "correct" before (I think it could be from a bug from the way that phi assignments happen before branch_conditions are evaluated?)
There was a problem hiding this comment.
I see... The worse output is unfortunate, albeit in line with how regular for (i = 0; i < N; i++) output also tends to look (do { ... temp = phi + 1; phi = temp; } while (temp < lim);). It would be nice to improve that if we can figure out a good way. If it's a big worsening we could consider keeping the incorrect version, e.g. by changing the asm pattern to move $temp, $ctr, addi $ctr, $ctr, -1, <branch> $temp, though it'd be a bit uncomfortable. I wonder if there's cases where that gives buggy output in practice, e.g. non-loop occurrences of $ctr.
And yeah, phi assignments are treated as happening all at once at the end of the block, past branch_condition's, which isn't possible to represent in C. That's definitely something that would be nice to fix, it's tripped people up in the past, but it's not obvious how. Would need temps for phis or some more involved refactoring of the whole phi/temp system. Or some wallpapering hacks for the most common cases.
There was a problem hiding this comment.
Yeah - I like that if we decide we want to go back to the incorrect version, it's possible to do so by modifying BranchCtrPatter (we don't need to bring back instrs_branch_decctr).
- Remove `PPC:` from comments in `arch_ppc.py`
- Rename `read` -> `read_exact` helper for clarity
- Store `demangled_str` instead of the `CxxSymbol` in `GlobalSymbol` to
keep MWCC specifics under a tighter abstraction.
- Fix bug in refacotring `handle_addi`
- In `output_regs_for_instr`:
- Replace `mnemonic == "jal"` with `instrs_fn_call` to handle PPC
- Add cases for `instrs_store_update`/`instrs_load_update`
- (NB: Need to revisit & re-audit)
- Assert that registers returned by `function_return` exactly match
those declared in `all_return_regs`
- Remove `.weaken_void_ptr()` in parsing demangled types to be more
consistent with CTypes parsing.
- Assert that `ELLIPSIS` does not show up outside of function args when
parsing demangled types
- Add "b" to instrs_ignore in PPC
- Remove "b" handling from instrs_jumps
- Clarify rlwinm instruction, factor out an assert
| if not isinstance(update, AddressMode): | ||
| raise DecompFailure( | ||
| f"Unhandled store-and-update arg in {instr}: {update!r}" | ||
| ) |
There was a problem hiding this comment.
This error occurs for all storex's, right? Would be nice to unify code for that with loadx. (We can fix this later.)
| # varargs functions. Decompiling multiple functions at once | ||
| # would help. | ||
| # TODO: don't do this in the middle of the argument list, | ||
| # except for f12 if a0 is passed and such. |
There was a problem hiding this comment.
This comment will eventually need to be updated, and I wonder if we want to share code with the mips implementation. Can wait for later though, I know this part is very much wip.
| "cmpwi": lambda a, op: BinaryOp.sintptr_cmp(a.reg(0), op, a.imm(1)), | ||
| "cmplw": lambda a, op: BinaryOp.uintptr_cmp(a.reg(0), op, a.reg(1)), | ||
| "cmplwi": lambda a, op: BinaryOp.uintptr_cmp(a.reg(0), op, a.imm(1)), | ||
| # TODO: There is a difference in how these two instructions handle NaN |
There was a problem hiding this comment.
(mips has a similar difference and handles this by swapping op -> {'>': '<', '<': '>', '!=': '==', '==': '!='}[op] and calling .negate() on the result in the u case. I don't know if the ppc behavior is exactly comparable)
- Move get_branch_target into ArchAsm (shared between MIPS/PPC) - Remove LENGTH_TWO/LENGTH_THREE normalization from PPC - Tidy up "PPC:" comments - Add isync - Remove break/trapuv.fictive - Logically order instructions in instrs_destination_first map
- Change `normalize_instruction` to a `@classmethod`
- Remove `$f2` from `all_return_regs` in PPC
- Normalize `cmp{,l}w{,i}` instructions to have 3 args (implicit cr0)
- Assert instrs_ppc_compare have cr0 as first arg
- Comment make_storex/handle_loadx
- Emit `MIPS2C_OVERFLOW(...)` when reading `cr0_so` instead of an error
- Require rA != rD in load-and-update instructions
- Clean up comments around PPC macros
- Remove `& 0xFFFF` from `literal@l` values
- Simplify `sym-_SDA_BASE_(r13)` parsing
- Split bdnz/bdz instructions into a pair (decrement; then branch)
- Removes the whole `instrs_decctr_branches` map
- This changes the output
- Add docstrings to all `Cxx*` classes - Document `CxxSymbol` parsing - Add quick unit tests - Fix `OPS` to match with underscores - Fix parsing unqualified symbol references in templates
Everywhere in `translate.py` where we manually parse an instruction mnemonic (i.e. that isn't abstracted in `Arch`), use an arch-prefixed version instead. (ex: `"mips:jr"` instead of `"jr"`) This makes it easier to follow the code, and avoids situations where mnemonic names may overlap between architectures. Also add missing case for `ldc1` to `output_regs_for_instr`
This map should now contains all instructions that have special behaior for `$r0`, including unimplemented instructions. This also changes the set into a dict to support instructions where `$r0` is the first argument.
|
Okay, I think I've responded to all the comments and marked them with 👍! As mentioned in Discord, for comments that we're punting to later, I marked them with 🚀 and added a link to them in the PR description. |
This is the big PR that merges in the work of the
ppc2cppbranch to add support for PPC & C++. Progress was primarily tracked in #187.This branch has one commit on top of
ppc2cppwhich changes the default target arch & makes some small changes to the website. (The default target onppc2cppisppc-mwcc-c++, but on themips_to_cmainline we want to usemips-ido-cas the default.)Known caveats (including comments marked with 🚀)
PpcArch.function_abi()is relatively basic, and does not support passing args on the stack.likely_regscommentmake_storexwithhandle_loadxcommentoutput_regs_for_instrto avoid deviating fromprocess_instrcommentDiffs from OOT/MM (MIPS-only). The only diffs are related to error messages, and some constant folding (
0x10000 + 0x4908-->0x14908).