shithub: rgbds

Download patch

ref: ca6149abcfd089393388ec7b83029d98b82966fc
parent: 54e5bf0f0c60b7ccc80215bfe10571267474e7f8
author: dbrotz <43593771+dbrotz@users.noreply.github.com>
date: Fri Jun 28 12:36:23 EDT 2019 

Fix signed integer overflow issues

It seemed that the consensus in our discussions of signed integer
overflow, which invokes undefined behavior in C, was that integer
arithmetic should be two's complement and there should be no warning for
overflows. I have implemented that by converting values to unsigned types
when appropriate. These changes will mostly preserve existing behavior,
except for a few cases that were being handled incorrectly before.

The case of dividing INT_MIN by -1 previously resulted in a CPU
exception and program termination. Now, that case is detected and results
in a warning and a value of INT_MIN.

Similarly, INT_MIN % -1 would have resulted in a CPU exception. Since this
is a mathematically valid operation with a result of 0, it now simply
gives that result without a warning.

I noticed that in rpn.c, there were attempts in certain operation handlers
to validate the nVal members of the source expressions even when the
expressions may have been relocatable expressions with meaningless numbers
for the nVal member. This could have caused spurious errors/warnings, so I
made those handlers confirm that isReloc is false before validating nVal.

Also, integer constants that are too large now result in a warning. The
post-conversion values have not been changed, in order to preserve
backward compatibility.


--- a/src/asm/constexpr.c
+++ b/src/asm/constexpr.c
@@ -66,7 +66,7 @@
 		result = value;
 		break;
 	case T_OP_SUB:
-		result = -value;
+		result = -(uint32_t)value;
 		break;
 	case T_OP_NOT:
 		result = ~value;
@@ -155,10 +155,10 @@
 			result = value1 != value2;
 			break;
 		case T_OP_ADD:
-			result = value1 + value2;
+			result = (uint32_t)value1 + (uint32_t)value2;
 			break;
 		case T_OP_SUB:
-			result = value1 - value2;
+			result = (uint32_t)value1 - (uint32_t)value2;
 			break;
 		case T_OP_XOR:
 			result = value1 ^ value2;
@@ -181,7 +181,7 @@
 				fatalerror("Shift by too big value: %d",
 					   value2);
 
-			result = value1 << value2;
+			result = (uint32_t)value1 << value2;
 			break;
 		case T_OP_SHR:
 			if (value2 < 0)
@@ -194,17 +194,25 @@
 			result = value1 >> value2;
 			break;
 		case T_OP_MUL:
-			result = value1 * value2;
+			result = (uint32_t)value1 * (uint32_t)value2;
 			break;
 		case T_OP_DIV:
 			if (value2 == 0)
 				fatalerror("Division by zero");
-			result = value1 / value2;
+			if (value1 == INT32_MIN && value2 == -1) {
+				warning("Division of min value by -1");
+				result = INT32_MIN;
+			} else {
+				result = value1 / value2;
+			}
 			break;
 		case T_OP_MOD:
 			if (value2 == 0)
 				fatalerror("Division by zero");
-			result = value1 % value2;
+			if (value1 == INT32_MIN && value2 == -1)
+				result = 0;
+			else
+				result = value1 % value2;
 			break;
 		case T_OP_FDIV:
 			result = math_Div(value1, value2);
--- a/src/asm/globlex.c
+++ b/src/asm/globlex.c
@@ -71,8 +71,9 @@
 
 static int32_t ascii2bin(char *s)
 {
-	int32_t radix = 10;
-	int32_t result = 0;
+	char *start = s;
+	uint32_t radix = 10;
+	uint32_t result = 0;
 	x2bin convertfunc = char2bin;
 
 	switch (*s) {
@@ -101,6 +102,9 @@
 		break;
 	}
 
+	const uint32_t max_q = UINT32_MAX / radix;
+	const uint32_t max_r = UINT32_MAX % radix;
+
 	if (*s == '\0') {
 		/*
 		 * There are no digits after the radix prefix
@@ -108,15 +112,39 @@
 		 */
 		yyerror("Invalid integer constant");
 	} else if (radix == 4) {
+		int32_t size = 0;
 		int32_t c;
 
 		while (*s != '\0') {
 			c = convertfunc(*s++);
 			result = result * 2 + ((c & 2) << 7) + (c & 1);
+			size++;
 		}
+
+		/*
+		 * Extending a graphics constant longer than 8 pixels,
+		 * the Game Boy tile width, produces a nonsensical result.
+		 */
+		if (size > 8) {
+			warning("Graphics constant '%s' is too long",
+				start);
+		}
 	} else {
-		while (*s != '\0')
-			result = result * radix + convertfunc(*s++);
+		bool overflow = false;
+
+		while (*s != '\0') {
+			int32_t digit = convertfunc(*s++);
+
+			if (result > max_q
+			 || (result == max_q && digit > max_r)) {
+				overflow = true;
+			}
+			result = result * radix + digit;
+		}
+
+		if (overflow)
+			warning("Integer constant '%s' is too large",
+				start);
 	}
 
 	return result;
--- a/src/asm/rpn.c
+++ b/src/asm/rpn.c
@@ -319,7 +319,7 @@
 	     const struct Expression *src2)
 {
 	joinexpr();
-	expr->nVal = (src1->nVal + src2->nVal);
+	expr->nVal = ((uint32_t)src1->nVal + (uint32_t)src2->nVal);
 	pushbyte(expr, RPN_ADD);
 }
 
@@ -327,7 +327,7 @@
 	     const struct Expression *src2)
 {
 	joinexpr();
-	expr->nVal = (src1->nVal - src2->nVal);
+	expr->nVal = ((uint32_t)src1->nVal - (uint32_t)src2->nVal);
 	pushbyte(expr, RPN_SUB);
 }
 
@@ -360,15 +360,18 @@
 {
 	joinexpr();
 
-	if (src1->nVal < 0)
-		warning("Left shift of negative value: %d", src1->nVal);
+	if (!expr->isReloc) {
+		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);
+		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 = (src1->nVal << src2->nVal);
+		expr->nVal = ((uint32_t)src1->nVal << src2->nVal);
+	}
+
 	pushbyte(expr, RPN_SHL);
 }
 
@@ -376,12 +379,16 @@
 	     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 = (src1->nVal >> src2->nVal);
+	if (!expr->isReloc) {
+		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 = (src1->nVal >> src2->nVal);
+	}
+
 	pushbyte(expr, RPN_SHR);
 }
 
@@ -389,7 +396,7 @@
 	     const struct Expression *src2)
 {
 	joinexpr();
-	expr->nVal = (src1->nVal * src2->nVal);
+	expr->nVal = ((uint32_t)src1->nVal * (uint32_t)src2->nVal);
 	pushbyte(expr, RPN_MUL);
 }
 
@@ -397,10 +404,19 @@
 	     const struct Expression *src2)
 {
 	joinexpr();
-	if (src2->nVal == 0)
-		fatalerror("Division by zero");
 
-	expr->nVal = (src1->nVal / src2->nVal);
+	if (!expr->isReloc) {
+		if (src2->nVal == 0)
+			fatalerror("Division by zero");
+
+		if (src1->nVal == INT32_MIN && src2->nVal == -1) {
+			warning("Division of min value by -1");
+			expr->nVal = INT32_MIN;
+		} else {
+			expr->nVal = (src1->nVal / src2->nVal);
+		}
+	}
+
 	pushbyte(expr, RPN_DIV);
 }
 
@@ -408,10 +424,17 @@
 	     const struct Expression *src2)
 {
 	joinexpr();
-	if (src2->nVal == 0)
-		fatalerror("Division by zero");
 
-	expr->nVal = (src1->nVal % src2->nVal);
+	if (!expr->isReloc) {
+		if (src2->nVal == 0)
+			fatalerror("Division by zero");
+
+		if (src1->nVal == INT32_MIN && src2->nVal == -1)
+			expr->nVal = 0;
+		else
+			expr->nVal = (src1->nVal % src2->nVal);
+	}
+
 	pushbyte(expr, RPN_MOD);
 }
 
@@ -418,7 +441,7 @@
 void rpn_UNNEG(struct Expression *expr, const struct Expression *src)
 {
 	*expr = *src;
-	expr->nVal = -expr->nVal;
+	expr->nVal = -(uint32_t)expr->nVal;
 	pushbyte(expr, RPN_UNSUB);
 }
 
--- /dev/null
+++ b/test/asm/overflow.asm
@@ -1,0 +1,42 @@
+SECTION "sec", ROM0
+
+print_x: MACRO
+	printv x
+	printt "\n"
+ENDM
+
+x = 2147483647
+x = x + 1
+	dl 2147483647+1
+	print_x
+
+x = -2147483648
+x = x - 1
+	dl -2147483648-1
+	print_x
+
+x = -2147483648
+x = x * -1
+	dl -2147483648 * -1
+	print_x
+
+x = -2147483648
+x = x / -1
+	dl -2147483648 / -1
+	print_x
+
+x = -2147483648
+x = x % -1
+	dl -2147483648 % -1
+	print_x
+
+x = -1
+x = x << 1
+	dl -1 << 1
+	print_x
+
+x = 4294967295
+x = 4294967296
+
+x = `33333333
+x = `333333333
--- /dev/null
+++ b/test/asm/overflow.out
@@ -1,0 +1,18 @@
+warning: overflow.asm(24):
+    Division of min value by -1
+warning: overflow.asm(25):
+    Division of min value by -1
+warning: overflow.asm(34):
+    Left shift of negative value: -1
+warning: overflow.asm(35):
+    Left shift of negative value: -1
+warning: overflow.asm(39):
+    Integer constant '4294967296' is too large
+warning: overflow.asm(42):
+    Graphics constant '`333333333' is too long
+$80000000
+$7FFFFFFF
+$80000000
+$80000000
+$0
+$FFFFFFFE
--- /dev/null
+++ b/test/asm/overflow.out.pipe
@@ -1,0 +1,18 @@
+warning: -(24):
+    Division of min value by -1
+warning: -(25):
+    Division of min value by -1
+warning: -(34):
+    Left shift of negative value: -1
+warning: -(35):
+    Left shift of negative value: -1
+warning: -(39):
+    Integer constant '4294967296' is too large
+warning: -(42):
+    Graphics constant '`333333333' is too long
+$80000000
+$7FFFFFFF
+$80000000
+$80000000
+$0
+$FFFFFFFE