shithub: rgbds

Info  •  Files  •  Log  •  Branches

rgbasm: Check the values of operands in bit shifts

The tests are not exhaustive, there are some conditions that aren't
checked. The tests are based in the C standard rules about undefined
behaviour.

This is a compatibility break but, hopefully, all projects are using
sane values. If not, there is no guarantee that the projects will build
in any platform where RGBDS can be compiled, so it would be better to
fix them.

Even though, technically, the left shift of a negative value is always
undefined, some projects rely on its current behaviour. This is the
reason why this doesn't cause a fatal error.

Signed-off-by: Antonio Niño Díaz <antonio_nd@outlook.com>

--- a/src/asm/asmy.y

+++ b/src/asm/asmy.y

@@ -1264,19 +1264,38 @@

 		| const T_OP_XOR const			{ $$ = $1 ^ $3; }

 		| const T_OP_OR const			{ $$ = $1 | $3; }

 		| const T_OP_AND const			{ $$ = $1 & $3; }

-		| const T_OP_SHL const			{ $$ = $1 << $3; }

-		| const T_OP_SHR const			{ $$ = $1 >> $3; }

+		| const T_OP_SHL const

+		{

+			if ($1 < 0)

+				warning("Left shift of negative value: %d", $1);

+

+			if ($3 < 0)

+				fatalerror("Shift by negative value: %d", $3);

+			else if ($3 >= 32)

+				fatalerror("Shift by too big value: %d", $3);

+

+			$$ = $1 << $3;

+		}

+		| const T_OP_SHR const

+		{

+			if ($3 < 0)

+				fatalerror("Shift by negative value: %d", $3);

+			else if ($3 >= 32)

+				fatalerror("Shift by too big value: %d", $3);

+

+			$$ = $1 >> $3;

+		}

 		| const T_OP_MUL const			{ $$ = $1 * $3; }

 		| const T_OP_DIV const

 		{

 			if ($3 == 0)

-				fatalerror("division by zero");

+				fatalerror("Division by zero");

 			$$ = $1 / $3;

 		}

 		| const T_OP_MOD const

 		{

 			if ($3 == 0)

-				fatalerror("division by zero");

+				fatalerror("Division by zero");

 			$$ = $1 % $3;

 		}

 		| T_OP_ADD const %prec NEG		{ $$ = +$2; }

--- a/src/asm/rpn.c

+++ b/src/asm/rpn.c

@@ -330,6 +330,15 @@

 	     const struct Expression *src2)

 {

 	joinexpr();

+

+	if (src1->nVal < 0)

+		warning("Left shift of negative value: %d", src1->nVal);

+

+	if (src2->nVal < 0)

+		fatalerror("Shift by negative value: %d", src2->nVal);

+	else if (src2->nVal >= 32)

+		fatalerror("Shift by too big value: %d", src2->nVal);

+

 	expr->nVal = (expr->nVal << src2->nVal);

 	pushbyte(expr, RPN_SHL);

 }

@@ -338,6 +347,11 @@

 	     const struct Expression *src2)

 {

 	joinexpr();

+	if (src2->nVal < 0)

+		fatalerror("Shift by negative value: %d", src2->nVal);

+	else if (src2->nVal >= 32)

+		fatalerror("Shift by too big value: %d", src2->nVal);

+

 	expr->nVal = (expr->nVal >> src2->nVal);

 	pushbyte(expr, RPN_SHR);

 }

@@ -355,7 +369,7 @@

 {

 	joinexpr();

 	if (src2->nVal == 0)

-		fatalerror("division by zero");

+		fatalerror("Division by zero");

 	expr->nVal = (expr->nVal / src2->nVal);

 	pushbyte(expr, RPN_DIV);

@@ -366,7 +380,7 @@

 {

 	joinexpr();

 	if (src2->nVal == 0)

-		fatalerror("division by zero");

+		fatalerror("Division by zero");

 	expr->nVal = (expr->nVal % src2->nVal);

 	pushbyte(expr, RPN_MOD);

--- a/test/asm/divzero-instr.out

+++ b/test/asm/divzero-instr.out

@@ -1,2 +1,2 @@

 ERROR: divzero-instr.asm(2):

-    division by zero

+    Division by zero

--- a/test/asm/divzero-section-bank.out

+++ b/test/asm/divzero-section-bank.out

@@ -1,2 +1,2 @@

 ERROR: divzero-section-bank.asm(1):

-    division by zero

+    Division by zero