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.

Sign up for GitHub

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

Jump to bottom

iasl disassembler: C-style disassembly for Subtract(Y, X, X) is incorrect #75

Closed
RehabMan opened this issue May 28, 2015 · 6 comments
Closed

iasl disassembler: C-style disassembly for Subtract(Y, X, X) is incorrect #75

RehabMan opened this issue May 28, 2015 · 6 comments

Comments

@RehabMan
Copy link

Consider this example:

DefinitionBlock ("test.aml", "DSDT", 2, "TEST", "TEST", 0)
{
    Method(TEST)
    {
        Store(0xFF00, Local0)
        Store(Local0, Local1)
        Subtract(0xFFFF, Local1, Local1)
        Subtract(0xFFFF, Local1, Local2)
    }
}

Save to test.dsl, compile with 'iasl test.dsl'. Disassembly with 'iasl -dl test.aml' result (correct):

DefinitionBlock ("test.aml", "DSDT", 2, "TEST", "TEST", 0x00000000)
{
    Method (TEST, 0, NotSerialized)
    {
        Store (0xFF00, Local0)
        Store (Local0, Local1)
        Subtract (0xFFFF, Local1, Local1)
        Subtract (0xFFFF, Local1, Local2)
    }
}

Disassembly with 'iasl -d test.aml' result:

DefinitionBlock ("test.aml", "DSDT", 2, "TEST", "TEST", 0x00000000)
{
    Method (TEST, 0, NotSerialized)
    {
        Local0 = 0xFF00
        Local1 = Local0
        Local1 -= 0xFFFF
        Local2 = (0xFFFF - Local1)
    }
}

The line:

        Local1 -= 0xFFFF

It is wrong. Should be (as the case with Local2 result):

     Local1 = (0xFFFF - Local1)
@acpibob
Copy link
Contributor

acpibob commented May 28, 2015

We will start looking at this.
Thanks.

@debox1
Copy link
Contributor

debox1 commented May 29, 2015

Thanks for reporting this. The code did not properly handle disassembly of non-commutative compound operations (subtraction, division, and modulo). The following patch should fix it.

diff --git a/source/components/disassembler/dmcstyle.c b/source/components/disassembler/dmcstyle.c
index 951af09..a69f5f9 100644
--- a/source/components/disassembler/dmcstyle.c
+++ b/source/components/disassembler/dmcstyle.c
@@ -409,23 +409,70 @@ AcpiDmCheckForSymbolicOpcode (
          */
         AcpiDmPromoteTarget (Op, Target);

-        /*
-         * Check for possible conversion to a "Compound Assignment".
-         *
-         * Determine if either operand is the same as the target
-         * and display compound assignment operator and other operand.
-         */
-        if ((AcpiDmIsTargetAnOperand (Target, Child1, TRUE)) ||
-            (AcpiDmIsTargetAnOperand (Target, Child2, TRUE)))
+        /* Check operands for conversion to a "Compound Assignment" */
+
+        switch (Op->Common.AmlOpcode)
         {
-            Target->Common.OperatorSymbol =
-                AcpiDmGetCompoundSymbol (Op->Common.AmlOpcode);
+            /* Commutative operators */

-            /* Convert operator to compound assignment */
+        case AML_ADD_OP:
+        case AML_MULTIPLY_OP:
+        case AML_SHIFT_LEFT_OP:
+        case AML_SHIFT_RIGHT_OP:
+        case AML_BIT_AND_OP:
+        case AML_BIT_OR_OP:
+        case AML_BIT_XOR_OP:
+            /*
+             * For the commutative operators, we can convert to a
+             * compound statement only if at least one (either) operand
+             * is the same as the target.
+             *
+             *      Add (A, B, A) --> A += B
+             *      Add (B, A, A) --> A += B
+             *      Add (B, C, A) --> A = (B + C)
+             */
+            if ((AcpiDmIsTargetAnOperand (Target, Child1, TRUE)) ||
+                (AcpiDmIsTargetAnOperand (Target, Child2, TRUE)))
+            {
+                Target->Common.OperatorSymbol =
+                    AcpiDmGetCompoundSymbol (Op->Common.AmlOpcode);

-            Op->Common.DisasmFlags |= ACPI_PARSEOP_COMPOUND;
-            Child1->Common.OperatorSymbol = NULL;
-            return (TRUE);
+                /* Convert operator to compound assignment */
+
+                Op->Common.DisasmFlags |= ACPI_PARSEOP_COMPOUND;
+                Child1->Common.OperatorSymbol = NULL;
+                return (TRUE);
+            }
+            break;
+
+            /* Non-commutative operators */
+
+        case AML_SUBTRACT_OP:
+        case AML_DIVIDE_OP:
+        case AML_MOD_OP:
+            /*
+             * For the non-commutative operators, we can convert to a
+             * compound statement only if the target is the same as the
+             * first operand.
+             *
+             *      Subtract (A, B, A) --> A -= B
+             *      Subtract (B, A, A) --> A = (B - A)
+             */
+            if ((AcpiDmIsTargetAnOperand (Target, Child1, TRUE)))
+            {
+                Target->Common.OperatorSymbol =
+                    AcpiDmGetCompoundSymbol (Op->Common.AmlOpcode);
+
+                /* Convert operator to compound assignment */
+
+                Op->Common.DisasmFlags |= ACPI_PARSEOP_COMPOUND;
+                Child1->Common.OperatorSymbol = NULL;
+                return (TRUE);
+            }
+            break;
+
+        default:
+            break;
         }

         /*

@RehabMan
Copy link
Author

I thought this might affect other operators. Thanks for taking a detailed look.

But regarding the patch, the shift operators are non-commutative. For example, ShiftLeft(1,0) != ShiftLeft(0,1).

So, current code translates:

ShiftLeft(1, Local1, Local1)

to

Local1 <<= One

Which, of course, is not correct. Should be:

Local1 = (1 << Local1)

@acpibob
Copy link
Contributor

acpibob commented May 29, 2015

Another good catch. We think you deserve the "bug of the month" award for these, since they got past us not once but twice.

Examples and new patch below.

    ShiftLeft(1, A, A)
    ShiftLeft(A, 2, A)
    ShiftRight(0x80, A, A)
    ShiftRight(A, 0xA0, A)

    A = (One << A)
    A <<= 0x02
    A = (0x80 >> A)
    A >>= 0xA0

diff --git a/source/components/disassembler/dmcstyle.c b/source/components/disassembler/dmcstyle.c
index 951af09..3762e82 100644
--- a/source/components/disassembler/dmcstyle.c
+++ b/source/components/disassembler/dmcstyle.c
@@ -409,23 +409,70 @@ AcpiDmCheckForSymbolicOpcode (
*/
AcpiDmPromoteTarget (Op, Target);

  •    /*

  •     \* Check for possible conversion to a "Compound Assignment".

  •     *

  •     \* Determine if either operand is the same as the target

  •     \* and display compound assignment operator and other operand.

  •     */

  •    if ((AcpiDmIsTargetAnOperand (Target, Child1, TRUE)) ||

  •        (AcpiDmIsTargetAnOperand (Target, Child2, TRUE)))

  •    /\* Check operands for conversion to a "Compound Assignment" */

  •    switch (Op->Common.AmlOpcode)
 {

  •        Target->Common.OperatorSymbol =

  •            AcpiDmGetCompoundSymbol (Op->Common.AmlOpcode);

  •        /* Commutative operators */

  •        /* Convert operator to compound assignment */

  •    case AML_ADD_OP:

  •    case AML_MULTIPLY_OP:

  •    case AML_BIT_AND_OP:

  •    case AML_BIT_OR_OP:

  •    case AML_BIT_XOR_OP:

  •        /*

  •         \* For the commutative operators, we can convert to a

  •         \* compound statement only if at least one (either) operand

  •         \* is the same as the target.

  •         *

  •         \*      Add (A, B, A) --> A += B

  •         \*      Add (B, A, A) --> A += B

  •         \*      Add (B, C, A) --> A = (B + C)

  •         */

  •        if ((AcpiDmIsTargetAnOperand (Target, Child1, TRUE)) ||

  •            (AcpiDmIsTargetAnOperand (Target, Child2, TRUE)))

  •        {

  •            Target->Common.OperatorSymbol =

  •                AcpiDmGetCompoundSymbol (Op->Common.AmlOpcode);

  •        Op->Common.DisasmFlags |= ACPI_PARSEOP_COMPOUND;

  •        Child1->Common.OperatorSymbol = NULL;

  •        return (TRUE);

  •            /\* Convert operator to compound assignment */

  •            Op->Common.DisasmFlags |= ACPI_PARSEOP_COMPOUND;

  •            Child1->Common.OperatorSymbol = NULL;

  •            return (TRUE);

  •        }

  •        break;

  •        /\* Non-commutative operators */

  •    case AML_SUBTRACT_OP:

  •    case AML_DIVIDE_OP:

  •    case AML_MOD_OP:

  •    case AML_SHIFT_LEFT_OP:

  •    case AML_SHIFT_RIGHT_OP:

  •        /*

  •         \* For the non-commutative operators, we can convert to a

  •         \* compound statement only if the target is the same as the

  •         \* first operand.

  •         *

  •         \*      Subtract (A, B, A) --> A -= B

  •         \*      Subtract (B, A, A) --> A = (B - A)

  •         */

  •        if ((AcpiDmIsTargetAnOperand (Target, Child1, TRUE)))

  •        {

  •            Target->Common.OperatorSymbol =

  •                AcpiDmGetCompoundSymbol (Op->Common.AmlOpcode);

  •            /\* Convert operator to compound assignment */

  •            Op->Common.DisasmFlags |= ACPI_PARSEOP_COMPOUND;

  •            Child1->Common.OperatorSymbol = NULL;

  •            return (TRUE);

  •        }

  •        break;

  •    default:

  •        break;
 }

 /*

@acpibob acpibob closed this as completed May 29, 2015
@acpibob
Copy link
Contributor

acpibob commented May 29, 2015

Checking this in to the git tree.

RehabMan referenced this issue May 29, 2015
@acpibob
Disassembler: Fix support for non-commutative symbolic operators.
5bc9437 
Care must be taken when disassembling operators like subtract and
shift left. The operators cannot be interchanged, so there are
additional restrictions on when these operators can be converted
to a compound assignment. David Box.
@RehabMan
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@acpibob @RehabMan @debox1