From d945c5811c158c8d032fed2bd320e33663d90af8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Antonio=20Ni=C3=B1o=20D=C3=ADaz?= <antonio_nd@outlook.com>
Date: Sat, 28 Apr 2018 01:30:00 +0100
Subject: [PATCH] rgbasm: Check the values of operands in bit shifts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 src/asm/asmy.y                    | 27 +++++++++++++++++++++++----
 src/asm/rpn.c                     | 18 ++++++++++++++++--
 test/asm/divzero-instr.out        |  2 +-
 test/asm/divzero-section-bank.out |  2 +-
 4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/src/asm/asmy.y b/src/asm/asmy.y
index 93d3f0f4..e0e89b07 100644
--- a/src/asm/asmy.y
+++ b/src/asm/asmy.y
@@ -1264,19 +1264,38 @@ const		: T_ID					{ $$ = sym_GetConstantValue($1); }
 		| 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; }
diff --git a/src/asm/rpn.c b/src/asm/rpn.c
index 7e963d3f..508a563d 100644
--- a/src/asm/rpn.c
+++ b/src/asm/rpn.c
@@ -330,6 +330,15 @@ void rpn_SHL(struct Expression *expr, const struct Expression *src1,
 	     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 @@ void rpn_SHR(struct Expression *expr, const struct Expression *src1,
 	     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 @@ void rpn_DIV(struct Expression *expr, const struct Expression *src1,
 {
 	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 @@ void rpn_MOD(struct Expression *expr, const struct Expression *src1,
 {
 	joinexpr();
 	if (src2->nVal == 0)
-		fatalerror("division by zero");
+		fatalerror("Division by zero");
 
 	expr->nVal = (expr->nVal % src2->nVal);
 	pushbyte(expr, RPN_MOD);
diff --git a/test/asm/divzero-instr.out b/test/asm/divzero-instr.out
index 09041a59..e1e0e3e2 100644
--- 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
diff --git a/test/asm/divzero-section-bank.out b/test/asm/divzero-section-bank.out
index b81d7254..b382e48f 100644
--- 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