Post by jaybert on May 15, 2014 4:19:44 GMT -8
Bug description:
When executing
ROL/ROR/RCL/RCR mem/reg, imm
the program counter (reg_ip) is incremented by one byte too many.
I have only tested this on ROL, but analysis of the code shows it should occur for ROR, RCR and RCL as well.
Analysis:
The original code with the defect is:
However the set flags type for these op codes are 0 (no flags to be updated).
The shift instructions require the SZP flags to be updated, the rotate instructions do not.
To set the flags correctly, when executing a shift instruction the op code is reset to op code 10h (ADC) to pick up the flag type.
This seems like an odd way to achieve the result to me - am I missing something?
However, ADC has a base size of 2, not 3 (the same as the other rotate instruction variants).
To account for this there is a ++reg_ip in the code if "extra" is set. The "extra" variable is only set for op codes C0h, C1h.
The problem occurs because the op code is not reset for the rotate instructions, so the size of 3 is for the original op code used, but reg_ip is increased for all op codes C0h and C1h, regardless of whether it is a shift or a rotate.
This causes reg_ip to be increased by one more than it should be when executing the rotate instructions for op codes C0h and C1h.
Solution:
As the only thing being used from the ADC op code is the flags setting, just set the "set_flags_type" to FLAGS_UPDATE_SZP for shift instructions, rather than change the op code.
Now the op code base size is always the correct size as it always comes from the original op code, so the ++reg_ip can be removed.
As far as I can tell this operates correctly - unless there is something I missed about why set_opcode(0x10) was used in the original code.
All programs I have tested seem to run correctly.
Modify the code as follows:
This code should also be slightly faster than the original.
When executing
ROL/ROR/RCL/RCR mem/reg, imm
the program counter (reg_ip) is incremented by one byte too many.
I have only tested this on ROL, but analysis of the code shows it should occur for ROR, RCR and RCL as well.
Analysis:
The original code with the defect is:
OPCODE 12: // ROL|ROR|RCL|RCR|SHL|SHR|???|SAR reg/mem, 1/CL/imm (80186)
scratch2_uint = SIGN_OF(mem[rm_addr]),
scratch_uint = extra ? // xxx reg/mem, imm
++reg_ip,
(char)i_data1
: // xxx reg/mem, CL
i_d
? 31 & regs8[REG_CL]
: // xxx reg/mem, 1
1;
if (scratch_uint)
{
if (i_reg < 4) // Rotate operations
scratch_uint %= i_reg / 2 + TOP_BIT,
R_M_OP(scratch2_uint, =, mem[rm_addr]);
if (i_reg & 1) // Rotate/shift right operations
R_M_OP(mem[rm_addr], >>=, scratch_uint);
else // Rotate/shift left operations
R_M_OP(mem[rm_addr], <<=, scratch_uint);
if (i_reg > 3) // Shift operations
set_opcode(0x10); // Decode like ADC
if (i_reg > 4) // SHR or SAR
set_CF(op_dest >> (scratch_uint - 1) & 1);
}In the BIOS tables, OP codes C0h and C1h (the 80186 shift mem/reg, imm instructions) are set to a base OP code length of 3 bytes, which is correct.However the set flags type for these op codes are 0 (no flags to be updated).
The shift instructions require the SZP flags to be updated, the rotate instructions do not.
To set the flags correctly, when executing a shift instruction the op code is reset to op code 10h (ADC) to pick up the flag type.
This seems like an odd way to achieve the result to me - am I missing something?
However, ADC has a base size of 2, not 3 (the same as the other rotate instruction variants).
To account for this there is a ++reg_ip in the code if "extra" is set. The "extra" variable is only set for op codes C0h, C1h.
The problem occurs because the op code is not reset for the rotate instructions, so the size of 3 is for the original op code used, but reg_ip is increased for all op codes C0h and C1h, regardless of whether it is a shift or a rotate.
This causes reg_ip to be increased by one more than it should be when executing the rotate instructions for op codes C0h and C1h.
Solution:
As the only thing being used from the ADC op code is the flags setting, just set the "set_flags_type" to FLAGS_UPDATE_SZP for shift instructions, rather than change the op code.
Now the op code base size is always the correct size as it always comes from the original op code, so the ++reg_ip can be removed.
As far as I can tell this operates correctly - unless there is something I missed about why set_opcode(0x10) was used in the original code.
All programs I have tested seem to run correctly.
Modify the code as follows:
OPCODE 12: // ROL|ROR|RCL|RCR|SHL|SHR|???|SAR reg/mem, 1/CL/imm (80186)
scratch2_uint = SIGN_OF(mem[rm_addr]),
scratch_uint = extra ? // xxx reg/mem, imm
(char)i_data1
: // xxx reg/mem, CL
i_d
? 31 & regs8[REG_CL]
: // xxx reg/mem, 1
1;
if (scratch_uint)
{
if (i_reg < 4) // Rotate operations
scratch_uint %= i_reg / 2 + TOP_BIT,
R_M_OP(scratch2_uint, =, mem[rm_addr]);
if (i_reg & 1) // Rotate/shift right operations
R_M_OP(mem[rm_addr], >>=, scratch_uint);
else // Rotate/shift left operations
R_M_OP(mem[rm_addr], <<=, scratch_uint);
if (i_reg > 3) // Shift operations
set_flags_type = FLAGS_UPDATE_SZP; // Shift instructions affect SZP
if (i_reg > 4) // SHR or SAR
set_CF(op_dest >> (scratch_uint - 1) & 1);
}
This code should also be slightly faster than the original.
