All instructions lifted (Z80 arch)#10
Conversation
bugfix: add perform_get_address_size(self) to ColecoView feature: lifted RST, conditional CALL, CPL, NEG, RES, SET
Added opcodes DI EI HALT IM OUTI OUTD OTIR OTDR SCF SLA SRL Added extra flag sets on SRA Implemented overflow flag for ADC (copied from SBB implementation)
Goal is to pretend the F part of the AF register doesn't exist. Pushing and popping in single bytes tidies the POP slightly, and moved to using il.flag_bit() which gives same results as PUSHFQ/POPFQ in the built-in x64 lifter
|
Very cool! Thanks for the PR! |
|
I will be reviewing this |
galenbwill
changed the title
|
Hi guys, any chance someone could take a look at this? I can break it into smaller pieces if that makes life easier |
|
I will prioritize reviewing it. |
|
Dang, I'm sorry we let this one lapse. We weren't tracking it on our main tasking board. I'm moving it there so we don't lose it again. My apologies. |
|
Better late than never :) |
xusheng6
left a comment
xusheng6
left a comment
There was a problem hiding this comment.
I found a few issues in the code. Once these are fixed, it should be good to go.
Sorry again for our late resoonse. I will respond promptly once you get these addressed!
| temp0 = LLIL_TEMP(0) | ||
| il.append(il.intrinsic([ILRegister(il.arch, temp0)], "in", [il.reg(1, 'C')])) | ||
| # save to (HL) | ||
| il.append(il.store(1, il.reg(2, 'HL'), temp0)) |
There was a problem hiding this comment.
This should be il.reg(1, temp0) instead of just temp0. temp0 is the index of the temp register, using it directly does not refer to the register. This also happens on line 757, 773, 792
| tmp = il.sub(2, il.reg(2, 'BC'), il.const(2, 1)) | ||
| il.append(il.set_reg(2, 'BC', tmp)) | ||
| il.append(il.set_flag('pv', il.compare_not_equal(2, il.reg(2, 'BC'), il.const(2, 0)))) | ||
| il.append(il.if_expr(il.or_expr(1, il.flag('z'), il.neg_expr(1, il.flag('pv'))), label_done, label_loop)) |
There was a problem hiding this comment.
neg_expr does arithemetic negation, not logical, so I believe the condition is wrong. Same pattern on line 544
| # if lower nybble > 9 OR H flag set then we diff by 0x06 | ||
| label_add_6 = LowLevelILLabel() | ||
| label_dont_add_6 = LowLevelILLabel() | ||
| lower_nybble = il.xor_expr(1, il.const(1, 0x0F), il.reg(1, 'A')) |
There was a problem hiding this comment.
I think this should be an and instead of xor, in order to extract the bits
Also as a comment, for instructions like DAA, we usually (e.g., for x86) just lift it as an intrinsic rather than spelling out the entire computation. I am not against your doing it like this since z80 is simple enough, so it is practical to do it for all instructions, but I am a bit worried that it hurts readability than helps it. After all this is merely a suggestion, you do not have to change anything
| il.append(il.set_flag('s', il.test_bit(1, temp0, il.const(1, 1<<7)))) | ||
| il.append(il.set_flag('z', il.test_bit(1, temp0, il.const(1, 1<<6)))) | ||
| il.append(il.set_flag('h', il.test_bit(1, temp0, il.const(1, 1<<4)))) | ||
| il.append(il.set_flag('pv', il.test_bit(1, temp0, il.const(1, 1<<2)))) | ||
| il.append(il.set_flag('n', il.test_bit(1, temp0, il.const(1, 1<<1)))) | ||
| il.append(il.set_flag('c', il.test_bit(1, temp0, il.const(1, 1)))) |
There was a problem hiding this comment.
il.test_bit tests the bit at the given index, there is no need to do 1<<7 -- just pass in il.const(1, 7). Same pattern at line 681
| # decrement B | ||
| tmp = il.reg(1, 'B') | ||
| tmp = il.add(1, tmp, il.const(1,-1)) | ||
| tmp = il.sub(1, tmp, il.const(1, 1), 'z') |
There was a problem hiding this comment.
This will incorrectly update the value of the z flag, which DJNZ does not modify. You could try something like this:
result = il.sub(1, il.reg(1, 'B'), il.const(1, 1))
il.append(il.set_reg(1, 'B', result))
condition = il.compare_not_equal(
1,
il.reg(1, 'B'),
il.const(1, 0)
)
il.append(il.if_expr(condition, label_loop, label_continue))
| # BC = BC - 1 | ||
| tmp = il.sub(2, il.reg(2, 'BC'), il.const(2, 1)) | ||
| il.append(il.set_reg(2, 'BC', tmp)) | ||
| il.append(il.set_flag('pv', il.compare_equal(2, il.reg(2, 'BC'), il.const(2, 0)))) |
There was a problem hiding this comment.
I think the condition here is inverted -- it should be il.compare_not_equal
5 participants
No description provided.