Fix: correct X86 implementation of rotations

Addresses problem reported in #1287

src/Arch/X86/Rewriter/X86Rewriter.Alu.cs

@@ -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()

src/Arch/X86/Rewriter/X86Rewriter.cs

@@ -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;

src/Core/Storage.cs

@@ -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
     {

src/Decompiler/Analysis/ConditionCodeEliminator.cs

@@ -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
@@ -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)
@@ -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)
 		{
@@ -268,6 +263,11 @@ public Expression UseGrfConditionally(
                             continue;
                         }
                     }
+                    if (sid.Identifier.Storage.Equals(ssa.Procedure.Architecture.CarryFlag))
+                    {
+                        if (cc == ConditionCode.UGE)
+                            e = e.Invert();
+                    }
                     if (gf.IsNegated)
                         e = e.Invert();
 

src/Decompiler/Analysis/ValuePropagator.cs

@@ -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());

src/UnitTests/Arch/Pdp/Pdp11/RewriterTests.cs

@@ -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()
         {

src/UnitTests/Arch/X86/Rewriter/X86Rewriter_32bitTests.cs

@@ -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]

src/UnitTests/Decompiler/Analysis/ConditionCodeEliminatorTests.cs

@@ -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));
@@ -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));
@@ -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();
+            });
+        }
+
     }
 }

src/tests/Arch/X86/RwPseudoProcs.exp

@@ -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
@@ -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)

src/tests/Arch/X86/RwSequenceShifts.exp

@@ -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
@@ -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: