Skip to content

Commit

Permalink
Fix: correct X86 implementation of rotations
Browse files Browse the repository at this point in the history
Addresses problem reported in #1287
  • Loading branch information
uxmal committed Sep 5, 2023
1 parent 6f6ac78 commit 8297e84
Show file tree
Hide file tree
Showing 29 changed files with 50,572 additions and 50,462 deletions.
63 changes: 36 additions & 27 deletions src/Arch/X86/Rewriter/X86Rewriter.Alu.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1062,40 +1062,49 @@ private void RewritePush(DataType dataWidth, Expression expr)
m.Assign(orw.StackAccess(sp, dataWidth), rhs);
}

private void RewriteRotation(IntrinsicProcedure operation, bool useCarry, bool left)
private void RewriteRotation(IntrinsicProcedure operation, Expression sh)
{
var cy = binder.EnsureFlagGroup(Registers.C);
m.Assign(cy, m.Ne0(m.And(SrcOp(0), sh)));
Expression p;
var src0 = SrcOp(0);
var src1 = SrcOp(1);
p = m.Fn(
operation.MakeInstance(src0.DataType, src1.DataType),
src0, src1);
m.Assign(SrcOp(0), p);
}

private void RewriteRotationWithCarry(IntrinsicProcedure operation, Expression sh)
{
Identifier? t;
Expression sh;
if (left)
{
sh = m.ISub(
Constant.Create(instrCur.Operands[1].Width, instrCur.Operands[0].Width.BitSize),
SrcOp(1));
}
else
{
sh = SrcOp(1);
}
sh = m.Shl(Constant.Create(instrCur.Operands[0].Width, 1), sh);
var cy = binder.EnsureFlagGroup(Registers.C);
t = binder.CreateTemporary(PrimitiveType.Bool);
m.Assign(t, m.Ne0(m.And(SrcOp(0), sh)));
m.Assign(t, cy);
m.Assign(cy, m.Ne0(m.And(SrcOp(0), sh)));
Expression p;
var src0 = SrcOp(0);
var src1 = SrcOp(1);
if (useCarry)
{
p = m.Fn(
operation.MakeInstance(src0.DataType, src1.DataType),
src0, src1, binder.EnsureFlagGroup(Registers.C));
}
else
{
p = m.Fn(
operation.MakeInstance(src0.DataType, src1.DataType),
src0, src1);
}
p = m.Fn(
operation.MakeInstance(src0.DataType, src1.DataType),
src0, src1, t);
m.Assign(SrcOp(0), p);
m.Assign(binder.EnsureFlagGroup(Registers.C), t);
}

private Expression RotateMaskLeft()
{
var ops = instrCur.Operands;
Expression sh = m.ISub(
Constant.Create(ops[1].Width, ops[0].Width.BitSize),
SrcOp(1));
return m.Shl(Constant.Create(ops[0].Width, 1), sh);
}

private Expression RotateMaskRight()
{
Expression sh = SrcOp(1);
sh = m.Shl(Constant.Create(instrCur.Operands[0].Width, 1), m.ISub(sh, 1));
return sh;
}

private void RewriteRorx()
Expand Down
8 changes: 4 additions & 4 deletions src/Arch/X86/Rewriter/X86Rewriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -614,11 +614,11 @@ public IEnumerator<RtlInstructionCluster> GetEnumerator()
case Mnemonic.pushw: RewritePush(PrimitiveType.Word16, SrcOp(0)); break;
case Mnemonic.pxor: RewritePxor(false); break;
case Mnemonic.vpxor: RewritePxor(true); break;
case Mnemonic.rcl: RewriteRotation(CommonOps.RolC, true, true); break;
case Mnemonic.rcl: RewriteRotationWithCarry(CommonOps.RolC, RotateMaskLeft()); break;
case Mnemonic.rcpps: RewritePackedUnaryop(rcpp_intrinsic, PrimitiveType.Real32); break;
case Mnemonic.rcr: RewriteRotation(CommonOps.RorC, true, false); break;
case Mnemonic.rol: RewriteRotation(CommonOps.Rol, false, true); break;
case Mnemonic.ror: RewriteRotation(CommonOps.Ror, false, false); break;
case Mnemonic.rcr: RewriteRotationWithCarry(CommonOps.RorC, RotateMaskRight()); break;
case Mnemonic.rol: RewriteRotation(CommonOps.Rol, RotateMaskLeft()); break;
case Mnemonic.ror: RewriteRotation(CommonOps.Ror, RotateMaskRight()); break;
case Mnemonic.rorx: RewriteRorx(); break;
case Mnemonic.rdmsr: RewriteRdmsr(); break;
case Mnemonic.rdpmc: RewriteRdpmc(); break;
Expand Down
3 changes: 2 additions & 1 deletion src/Core/Storage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
namespace Reko.Core
{
/// <summary>
/// Encapsulates architecture-dependent storage mechanisms for an identifier.
/// A <see cref="Storage"/> is a value type that encapsulates
/// architecture-dependent storage locations for <see cref="Identifier"/>s.
/// </summary>
public abstract class Storage
{
Expand Down
18 changes: 9 additions & 9 deletions src/Decompiler/Analysis/ConditionCodeEliminator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ public void Transform()
this.aliases.Clear();
ClosureOfUsingStatements(sidGrf, uses, aliases);
trace.Inform("CCE: Tracing {0}", sidGrf.DefStatement.Instruction);

foreach (var u in uses)
{
try
Expand Down Expand Up @@ -122,13 +121,9 @@ public HashSet<SsaIdentifier> ClosureOfReachingDefinitions(SsaIdentifier sidUse)
while (wl.TryGetWorkItem(out var sid))
{
var def = sid.DefStatement;
if (def != null)
{
if (visited.Contains(def))
continue;
visited.Add(def);
}
switch (def?.Instruction)
if (!visited.Add(def))
continue;
switch (def.Instruction)
{
case Assignment ass:
switch (ass.Src)
Expand Down Expand Up @@ -226,7 +221,7 @@ private static bool IsCopyWithOptionalCast(Identifier grf, Statement stm)
BinaryExpression bin when bin.Operator.Type == OperatorType.Or => bin.Left == grf || bin.Right == grf,
_ => false,
};
}
}

private BinaryExpression CmpExpressionToZero(Expression e)
{
Expand Down Expand Up @@ -268,6 +263,11 @@ private BinaryExpression CmpExpressionToZero(Expression e)
continue;
}
}
if (sid.Identifier.Storage.Equals(ssa.Procedure.Architecture.CarryFlag))
{
if (cc == ConditionCode.UGE)
e = e.Invert();
}
if (gf.IsNegated)
e = e.Invert();

Expand Down
2 changes: 2 additions & 0 deletions src/Decompiler/Analysis/ValuePropagator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ public bool Transform(Statement stm)
{
bool changed;
evalCtx.Statement = stm;
if (stm.Address.Offset == 0x28A9)
_ = this; //$DEBUG
trace.Verbose("From: {0}", stm.Instruction.ToString());
(stm.Instruction, changed) = stm.Instruction.Accept(this);
trace.Verbose(" To: {0}", stm.Instruction.ToString());
Expand Down
8 changes: 8 additions & 0 deletions src/UnitTests/Arch/Pdp/Pdp11/RewriterTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,14 @@ public void Pdp11Rw_com()
"4|L--|C = true");
}

[Test]
public void Pdp11Rw_rol()
{
Given_UInt16s(0x0C41);
AssertCode(
"@@@");
}

[Test]
public void Pdp11Rw_rts_r5()
{
Expand Down
38 changes: 34 additions & 4 deletions src/UnitTests/Arch/X86/Rewriter/X86Rewriter_32bitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -298,15 +298,45 @@ public void X86rw_cmp_Ev_Ib()
"1|L--|SCZO = cond(Mem0[edi:word32] - 0xFFFFFFFF<32>)");
}

[Test]
public void X86Rw_rcl_1()
{
Run32bitTest("D0 D8"); // ror al,1h
AssertCode(
"0|L--|10000000(2): 3 instructions",
"1|L--|v4 = C",
"2|L--|C = (al & 1<8> << 1<8> - 1<8>) != 0<8>",
"3|L--|al = __rcr<byte,byte>(al, 1<8>, v4)");
}

[Test]
public void X86Rw_rol_1()
{
Run32bitTest("D0 C0"); // ror al,1h
AssertCode(
"0|L--|10000000(2): 2 instructions",
"1|L--|C = (al & 1<8> << 8<8> - 1<8>) != 0<8>",
"2|L--|al = __rol<byte,byte>(al, 1<8>)");
}

[Test]
public void X86rw_rol_Eb()
{
Run32bitTest("C0C0C0");
AssertCode(
"0|L--|10000000(3): 3 instructions",
"1|L--|v3 = (al & 1<8> << 8<8> - 0xC0<8>) != 0<8>",
"2|L--|al = __rol<byte,byte>(al, 0xC0<8>)",
"3|L--|C = v3");
"0|L--|10000000(3): 2 instructions",
"1|L--|C = (al & 1<8> << 8<8> - 0xC0<8>) != 0<8>",
"2|L--|al = __rol<byte,byte>(al, 0xC0<8>)");
}

[Test]
public void X86Rw_ror_1()
{
Run32bitTest("D0 C8"); // ror al,1h
AssertCode(
"0|L--|10000000(2): 2 instructions",
"1|L--|C = (al & 1<8> << 1<8> - 1<8>) != 0<8>",
"2|L--|al = __ror<byte,byte>(al, 1<8>)");
}

[Test]
Expand Down
77 changes: 75 additions & 2 deletions src/UnitTests/Decompiler/Analysis/ConditionCodeEliminatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1234,7 +1234,7 @@ def r2
var r2 = m.Reg32("r2", 2);
var r3 = m.Reg32("r3", 3);
var psw = RegisterStorage.Reg32("psw", 4);
var C = m.Frame.EnsureFlagGroup(new FlagGroupStorage(psw, 0x1, "C", PrimitiveType.Bool));
var C = m.Frame.EnsureFlagGroup(m.Architecture.CarryFlag);
m.Assign(r2, m.Shr(r2, 1));
m.Assign(C, m.Cond(r2));
Expand Down Expand Up @@ -1271,7 +1271,7 @@ def eax
var SZ = m.Frame.EnsureFlagGroup(new FlagGroupStorage(psw, 0x0C, "SZ", PrimitiveType.Byte));
var SZO = m.Frame.EnsureFlagGroup(new FlagGroupStorage(psw, 0x0E, "SZO", PrimitiveType.Byte));
var O = m.Frame.EnsureFlagGroup(new FlagGroupStorage(psw, 0x02, "O", PrimitiveType.Bool));
var C = m.Frame.EnsureFlagGroup(new FlagGroupStorage(psw, 0x01, "C", PrimitiveType.Bool));
var C = m.Frame.EnsureFlagGroup(m.Architecture.CarryFlag);
m.Label("foo");
m.Assign(eax, m.Or(eax, eax));
m.Assign(SZ, m.Cond(eax));
Expand All @@ -1281,5 +1281,78 @@ def eax
m.Return();
});
}

[Test(Description = "Treat condition codes as simple bit flags if they're not the result of Cond pseudo-expressions")]
public void CceSingleBitTest_noCond_true()
{
var sExp =
#region Expected
@"// ProcedureBuilder
// Return size: 0
define ProcedureBuilder
ProcedureBuilder_entry:
// succ: foo
foo:
eax_1 = 0x123400<32>()
branch (eax_1 & 1<32>) != 0<32> foo
// succ: l1 foo
l1:
return
// succ: ProcedureBuilder_exit
ProcedureBuilder_exit:
";
#endregion
RunStringTest(sExp, m =>
{
var eax = m.Reg32("eax", 1);
var psw = RegisterStorage.Reg32("psw", 4);
var C = m.Frame.EnsureFlagGroup(m.Architecture.CarryFlag);
var tmp = m.Frame.CreateTemporary(PrimitiveType.Bool);
m.Label("foo");
m.Assign(eax, m.Fn(m.Word32(0x00123400)));
m.Assign(C, m.Ne0(m.And(eax, 1)));
m.Assign(eax, m.Fn(CommonOps.Ror, eax, m.Byte(1)));
m.BranchIf(m.Test(ConditionCode.ULT, C), "foo");
m.Return();
});
}

[Test(Description = "Treat condition codes as simple bit flags if they're not the result of Cond pseudo-expressions")]
public void CceSingleBitTest_noCond_false()
{
var sExp =
#region Expected
@"// ProcedureBuilder
// Return size: 0
define ProcedureBuilder
ProcedureBuilder_entry:
// succ: foo
foo:
eax_1 = 0x123400<32>()
branch (eax_1 & 1<32>) == 0<32> foo
// succ: l1 foo
l1:
return
// succ: ProcedureBuilder_exit
ProcedureBuilder_exit:
";
#endregion
RunStringTest(sExp, m =>
{
var eax = m.Reg32("eax", 1);
var psw = RegisterStorage.Reg32("psw", 4);
var C = m.Frame.EnsureFlagGroup(m.Architecture.CarryFlag);
var tmp = m.Frame.CreateTemporary(PrimitiveType.Bool);
m.Label("foo");
m.Assign(eax, m.Fn(m.Word32(0x00123400)));
m.Assign(C, m.Ne0(m.And(eax, 1)));
m.Assign(eax, m.Fn(CommonOps.Ror, eax, m.Byte(1)));
m.BranchIf(m.Test(ConditionCode.UGE, C), "foo");
m.Return();
});
}

}
}
14 changes: 6 additions & 8 deletions src/tests/Arch/X86/RwPseudoProcs.exp
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@
// si:si
// dx:dx
// al:al
// v9:v9
// bx:bx
// C:C
// v12:v12
// bx:bx
// v11:v11
// cx:cx
// SZO:SZO
// di:di
Expand All @@ -30,12 +29,11 @@ fn0C00_0000_entry:
l0C00_0000:
si = bp + 12<i16>
__out<byte>(dx, al)
v9 = (bx & 1<16> << 1<8>) != 0<16>
C = (bx & 1<16> << 1<8> - 1<8>) != 0<16>
bx = __ror<word16,byte>(bx, 1<8>)
C = v9
v12 = (cx & 1<16> << 1<8>) != 0<16>
cx = __rcr<word16,byte>(cx, 1<8>, C)
C = v12
v11 = C
C = (cx & 1<16> << 1<8> - 1<8>) != 0<16>
cx = __rcr<word16,byte>(cx, 1<8>, v11)
dx = dx + 1<16>
SZO = cond(dx)
al = __in<byte>(dx)
Expand Down
8 changes: 4 additions & 4 deletions src/tests/Arch/X86/RwSequenceShifts.exp
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
// ds:ds
// edx:edx
// SCZO:SCZO
// v8:v8
// C:C
// v9:v9
// Top:Top
// return address size: 2
define fn0C00_0000
Expand All @@ -29,9 +29,9 @@ l0C00_0000:
edx = Mem0[ds:0x104<16>:word32]
eax = eax << 1<32>
SCZO = cond(eax)
v8 = (edx & 1<32> << 0x20<8> - 1<8>) != 0<32>
edx = __rcl<word32,byte>(edx, 1<8>, C)
C = v8
v9 = C
C = (edx & 1<32> << 0x20<8> - 1<8>) != 0<32>
edx = __rcl<word32,byte>(edx, 1<8>, v9)
return
// succ: fn0C00_0000_exit
fn0C00_0000_exit:
Expand Down
Loading

0 comments on commit 8297e84

Please sign in to comment.