Skip to content
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

i8085: set parity flag from arithmetic and logical operations #6435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hjanetzek
Copy link

@hjanetzek hjanetzek commented Apr 19, 2024

Decompilation of the IBM System/23 DataMaster RST0 test routine showed that P flag was never properly set.

I've used this reference to check for operations changing the P flag:
https://gpbarkot.org.in/download/file/ihoN4LlRHP.pdf
with ctrl-f 'S, Z, P are modified to reflect the result of the operation'

Before
shot-2024-04-19_13-42-58
After
shot-2024-04-19_22-40-32

@GhidorahRex
Copy link
Collaborator

There are several additional operations that set the P flag (notably all the arithmetic instructions which are listed as modifying all flags). That slide deck is also missing flag updates from several instructions such as INR and DCR. Check out the MCS80/85 Users Manual from 1983. That is much more thorough.

@hjanetzek
Copy link
Author

Thanks @GhidorahRex I'll goo through all these again.

It appears there were previous attempts in 8085.slaspec for setting P_flag at, e.g. at https://github.com/NationalSecurityAgency/ghidra/blob/master/Ghidra/Processors/8085/data/languages/8085.slaspec#L415C1-L419C2

:DAA  is op0_8=0x27 {
	A = BCDadjust(A);
	setResultFlags(A);
	P_flag = hasEvenParity(A);
}

Are there 'Pseudo Instructions' like BCDadjust and hasEvenParity that the decompiler knows to interpret?

Should these commented lines be removed?
https://github.com/NationalSecurityAgency/ghidra/blob/master/Ghidra/Processors/8085/data/languages/8085.slaspec#L82

macro setAddCarryFlags(op1,op2) {
	CY_flag = (carry(op1,zext(CY_flag)) || carry(op2,op1 + zext(CY_flag)));
#	P_flag = (scarry(op1,CY_flag) || scarry(op2,op1 + CY_flag));
#   AC_flag = ??
}
macro setAddFlags(op1,op2) {
	CY_flag = carry(op1,op2);
#	P_flag = scarry(op1,op2);
#   AC_flag = ??
}

This looks wrong to me unless increment/decrement have a different meaning for parity result. Are these correct?
https://github.com/NationalSecurityAgency/ghidra/blob/master/Ghidra/Processors/8085/data/languages/8085.slaspec#L387

:INR reg3_3  is op6_2=0x0 & reg3_3 & bits0_3=0x4 {
	P_flag = (reg3_3 == 0x7f);
	reg3_3 = reg3_3 + 1;
	setResultFlags(reg3_3);
}

:INR (HL)  is op0_8=0x34 & HL {
	val:1 = *:1 HL;
	P_flag = (val == 0x7f);
	val = val + 1;
	*:1 HL = val;
	setResultFlags(val);
}

:DCR reg3_3  is op6_2=0x0 & reg3_3 & bits0_3=0x5 {
	P_flag = (reg3_3 == 0x80);
	reg3_3 = reg3_3 - 1;
	setResultFlags(reg3_3);
}

:DCR (HL)  is op0_8=0x35 & HL {
	val:1 = *:1 HL;
	P_flag = (val == 0x80);
	val = val - 1;
	*:1 HL = val;
	setResultFlags(val);
}

@hjanetzek
Copy link
Author

In MCS80/85 Users Manual it first states that parity reflects the state of the accumulator
shot-2024-04-24_23-07-49
And later that the parity flag reflects the result of an operation - so for :INR (HL) it means the parity of the value at HL ?
shot-2024-04-24_23-06-32

@GhidorahRex
Copy link
Collaborator

@hjanetzek Those calculations are completely wrong. The ones you've introduced are correct however.

There are pseudo pcode ops. These can either be abstract or they can be defined by the emulator. In this case, the BCDAdjust and hasEvenParity operations are abstract. The decompiler treats them as as an abstract function call more or less.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Processor/8085 Status: Triage Information is being gathered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants