flamewing / asl-releases

Improved/bugfixed version of Alfred Arnold's The Macro Assembler AS
http://john.ccac.rwth-aachen.de:8000/as/
GNU General Public License v2.0
20 stars 2 forks source link

Operator priority in expressions is broken #23

Open vladikcomper opened 2 years ago

vladikcomper commented 2 years ago

I've noticed a serious bug when compiling s2disasm with the recent version of AS (v.1.42 beta [build 211] at the time of writing).

Some of the assembly options (defined as variables) were broken due to incorrect evaluation of their expressions.

Take the following example:

      21/       0 : =$2                  gameRevision = 2
      22/       0 : =$1                  allOptimizations = 1
      23/       0 : =$0                  removeJmpTos = 0|gameRevision=2|allOptimizations

The last variable, removeJmpTos, should obviously evaluate to $1 (true) given the expression: gameRevision is indeed 2, and if that's not enough, allOptimizations are also non-zero. However, it's mistakenly evaluated as $0 (false).

Interestingly, the following expressions still work as intended:

      23/       0 : =$1                  removeJmpTos = 0|(gameRevision=2)|allOptimizations

and:

      23/       0 : =$1                  removeJmpTos = 0|gameRevision=2

Which leads to a conclusion that it's either a bug in expression evaluator, or priorities of = and | operators are incorrect, as if the expression in question were internally resolved to this instead:

      23/       0 : =$0                  removeJmpTos = 0|(gameRevision=(2|allOptimizations))

The incorrect ordering of operators would explain the bug, as it would make sense that this expression is falsy: it's not true that gameRevision=(2|allOptimizations), because 2|allOptimizations evaluates to 3 and gameRevision obviously equals to 2 in the example above.

vladikcomper commented 2 years ago

Further testing proves it's likely an operator priority issue:

If gameRevision is set to 3, the expression is now truthy:

      21/       0 : =$2                  gameRevision = 3
      22/       0 : =$1                  allOptimizations = 1
      23/       0 : =$1                  removeJmpTos = 0|gameRevision=2|allOptimizations

This is because due to incorrect operator order it's resolved as follows:

0|gameRevision=2|allOptimizations
0|(gameRevision=(2|allOptimizations))
0|(gameRevision=3)
0|1
1
flamewing commented 2 years ago

From what I can tell, this is intended behavior, since it is documented that the comparison operators have lower precedence; but it is counter-intuitive, and I will fix this so it works as one might expect. For the record, this line:

0|gameRevision=2|allOptimizations

gets parsed as

(0|gameRevision)=(2|allOptimizations)

and not as

0|(gameRevision=2)|allOptimizations

However, fixing this will involve rewriting the expression parser, because unary minus operator is implemented in a hackish way and does not have a precedence of its own. effectively sharing it with the binary minus operator. This means that I can make at most one of these work with reassigned priorities:

    if a=-1
    if a=b-1

Unless I rewrite the expression parser, that is.

In the interim, I updated the disassembly to use parenthesis to get the correct result.