Runtime errors on PPC (big endian) #165

fragow · 2023-07-24T14:36:13Z

I adapt the OpenWrt firmware for our railway access points. The access point hardware is based on PowerPC CPUs of the type QorIQ T1023(32Bit) and T2081(64Bit). For the next OpenWrt merge to version v22.03.05 the ucode package for the firewall (fw4) had to work.
In the first attempts on x86 and an ARM platform, the 167 tests of the ucode Test-Suite ran error-free.

But the ucode output generated on the PPC target was faulty, causing the FW4 not to start correctly.

Unfortunately, I couldn't run the test suite on the PPC platform at first, because it only provides busybox-shell and busybox-utils and not the full versions of bash / diff / truncate that are required.

That's why I first adapted the test suite script 'run_tests.sh'. My 'OpenWrt' version 'run_tests_openwrt.sh' now uses readlink-coreutils, truncate-coreutils and diff-gnu and can run in the busybox shell. However, many of the 167 tests were incorrect.

I was able to localize the error on the function 'ucv_compare' from the source file types.c.
Here the (int) return value of the strcmp () function is converted into a character (int8_t). On an x86/arm this always seems to work fine. However, on a PPC platform (big endian), the conversion leads to unpredictable results.
I patched the spot like this:

diff --git a/types.c b/types.c
index 5758f74..eebf1b1 100644
--- a/types.c
+++ b/types.c
@@ -1999,6 +1999,7 @@ ucv_compare(int how, uc_value_t *v1, uc_value_t *v2, int *deltap)
 	int64_t n1, n2;
 	double d1, d2;
 	int8_t delta;
+	int ret;
 
 	/* at least one operand is null and we compare for equality or inequality ... */
 	if ((!v1 || !v2) && (how == I_EQ || how == I_NE)) {
@@ -2007,7 +2008,11 @@ ucv_compare(int how, uc_value_t *v1, uc_value_t *v2, int *deltap)
 
 	/* ... otherwise if both operands are strings, compare bytewise ... */
 	else if (t1 == UC_STRING && t2 == UC_STRING) {
-		delta = strcmp(ucv_string_get(v1), ucv_string_get(v2));
+		/* Do not cast return value of stncmp as int8_t on PPC (big-endian) ! */
+		ret = strcmp(ucv_string_get(v1), ucv_string_get(v2));
+		if (ret == 0) delta = 0;
+		if (ret > 0) delta = 1;
+		if (ret < 0) delta = -1;
 	}
 
 	/* handle non-string cases... */

With this patch, at least 157 out of 167 tests in the test suite were error-free.

I then found a similar error in 'lib/fs.c'. Here the (int) return value of isatty() is directly converted into a 'ucv_boolean_new(isatty(fd))'. This also does not work correctly on the PPC. The following patch:

diff --git a/lib/fs.c b/lib/fs.c
index 831137d..461810d 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -356,7 +356,7 @@ static uc_value_t *
 uc_fs_isatty(uc_vm_t *vm, size_t nargs)
 {
 	FILE **fp = uc_fn_this("fs.file");
-	int fd;
+	int fd, ret;
 
 	if (!fp || !*fp)
 		err_return(EBADF);
@@ -366,7 +366,12 @@ uc_fs_isatty(uc_vm_t *vm, size_t nargs)
 	if (fd == -1)
 		err_return(errno);
 
-	return ucv_boolean_new(isatty(fd));
+	/* Do not convert isatty return value into bool on PPC (big-endian) */
+	ret = isatty(fd);
+	if (ret == 1)
+		return ucv_boolean_new(true);
+	else
+		return ucv_boolean_new(false);
 }
 
 static uc_value_t *
--

With this patch, all 167 tests in the Test-Suite were error-free.

I added another patch in types.c that didn't trigger a Test-Suite error. The difference of an INT pointer subtraction can definitely be larger than: int8_t delta;

diff --git a/types.c b/types.c
index 4391880..bfda5d8 100644
--- a/types.c
+++ b/types.c
@@ -2000,6 +2000,7 @@ ucv_compare(int how, uc_value_t *v1, uc_value_t *v2, int *deltap)
 	double d1, d2;
 	int8_t delta;
 	int ret;
+	intptr_t diff;
 
 	/* at least one operand is null and we compare for equality or inequality ... */
 	if ((!v1 || !v2) && (how == I_EQ || how == I_NE)) {
@@ -2021,7 +2022,11 @@ ucv_compare(int how, uc_value_t *v1, uc_value_t *v2, int *deltap)
 		/* ... both operands are of the same, non-scalar type... */
 		if (t1 == t2 && !ucv_is_scalar(v1)) {
 			/* ... compare memory addrs */
-			delta = (intptr_t)v1 - (intptr_t)v2;
+			/* Do not cast (long int) difference to int8 type. */
+			diff = (intptr_t)v1 - (intptr_t)v2;
+			if (diff == 0) delta = 0;
+			if (diff > 0) delta = 1;
+			if (diff < 0) delta = -1;
 		}
 
 		/* ... operands are of different type or at least one is scalar... */
--

The patches could also be interesting for other architectures, but at least for OpenWrt ./target/linux/mpc85xx'.
If desired I can provide my test script: test/custom/run_tests_openwrt.sh.
Here is the version for which we use the patches.

PKG_SOURCE_DATE:=2023-06-06
PKG_SOURCE_VERSION:=c7d84aae09691a99ae3db427c0b2463732ef84f4
PKG_MIRROR_HASH:=97339d563e988d57ed02f5fd27722c908d616c2cec8b1f0479f42bc53dfd0aa1
PKG_MAINTAINER:=Jo-Philipp Wich <jo@mein.io>

The text was updated successfully, but these errors were encountered:

- Don't implicitly convert intptr_t difference to int8_t but perform explicit comparisons - Don't implicitly convert strcmp() int result to int8_t Fixes: #165 Signed-off-by: Jo-Philipp Wich <jo@mein.io>

Reportedly, automatic conversion of the `isatty()` int result value to a bool does not work correctly on PPC. Explicitly compare the result value with `1` to infer the boolean result value. Fixes: #165 Signed-off-by: Jo-Philipp Wich <jo@mein.io>

jow- · 2023-07-25T12:39:03Z

Thank you for your report and the analysis of the problems. Frankly I don't understand the isatty() issue unless on PPC isatty() can return some other value besides 1 or 0 (-1 maybe?). I added an explicit comparison with == 1 to address this.

fragow · 2023-07-25T13:44:13Z

Thanks for the quick reaction and for adjusting the patches. I was able to compile and run it out on PPC without any problems. (pulled Merge: 4bee0ef 093684d)
In the meantime, the custom test cases suite have probably also changed, which means that the following test cases are now faulty.

25_ltrim ................................ OK
26_rtrim ................................ OK
27_sprintf .............................. OK
28_printf ............................... OK
29_require .............................. !
Testcase #4: Expected stderr did not match:
--- Expected stderr
+++ Resulting stderr
@@ -1,7 +1,7 @@
 Runtime error: Unable to compile source file './files/require/test/broken.uc':
 
   | Syntax error: Expecting label
-  | In line 3, byte 1:
+  | In line 2, byte 10:
   |
   |  `return {`
   |           ^-- Near here
---
29_require .............................. FAILED (1/4)
30_iptoarr .............................. OK
...OK
34_json ................................. OK
35_include .............................. !
Testcase #6: Expected stderr did not match:
--- Expected stderr
+++ Resulting stderr
@@ -1,7 +1,7 @@
 Runtime error: Unable to compile source file './files/broken.uc':
 
   | Syntax error: Expecting label
-  | In line 4, byte 1:
+  | In line 3, byte 11:
   |
   |  `    return {`
   |   Near here --^
---
35_include .............................. FAILED (1/7)
36_render ............................... !
Testcase #6: Expected stderr did not match:
--- Expected stderr
+++ Resulting stderr
@@ -1,7 +1,7 @@
 Runtime error: Unable to compile source file './files/broken.uc':
 
   | Syntax error: Expecting label
-  | In line 4, byte 1:
+  | In line 3, byte 11:
   |
   |  `    return {`
   |   Near here --^
---
36_render ............................... FAILED (1/6)
...OK
62_loadfile ............................. !
Testcase #7: Expected stderr did not match:
--- Expected stderr
+++ Resulting stderr
@@ -1,7 +1,7 @@
 Runtime error: Unable to compile source file './files/test6.uc':
 
   | Syntax error: Expecting expression
-  | In line 2, byte 1:
+  | In line 1, byte 5:
   |
   |  `1 +`
   |      ^-- Near here
---
62_loadfile ............................. FAILED (1/7)
...OK
13_split_by_string_leading_trailing ..... OK
14_incomplete_expression_at_eof ......... !
Testcase #1: Expected stderr did not match:
--- Expected stderr
+++ Resulting stderr
@@ -1,5 +1,5 @@
 Syntax error: Expecting expression
-In line 2, byte 1:
+In line 1, byte 7:
 
  `{% 1+`
        ^-- Near here
---
14_incomplete_expression_at_eof ......... FAILED (1/1)
...OK
24_compiler_local_for_loop_declaration .. OK
25_lexer_shifted_offsets ................ !
Testcase #1: Expected stderr did not match:
--- Expected stderr
+++ Resulting stderr
@@ -1,5 +1,5 @@
 Error
-In line 3, byte 13:
+In line 3, byte 12:
 
  `    die("Error");`
   Near here -----^
---
25_lexer_shifted_offsets ................ FAILED (1/1)
...
Ran 167 tests, 161 okay, 6 failures

However, a run with the old custom test cases from (PKG_SOURCE_VERSION:=c7d84aae09691a99ae3db427c0b2463732ef84f4) is error-free. I therefore assume that the PPC specific patches work well (also the (isatty(fd) == 1).

Maybe the custom test suite should possibly be adjusted.

ynezz · 2023-07-25T13:48:22Z

which means that the following test cases are now faulty.

This is happening on PPC?

Maybe the custom test suite should possibly be adjusted.

FYI that custom test suite passes on x86/64 https://github.com/jow-/ucode/actions/runs/5656546401/job/15324077119#step:3:1805

fragow · 2023-07-25T14:04:20Z

Yes, that's true on my x86/64, that was my first test, no errors occur.
The errors are only apply on a PPC T1023 with OpenWrt toolchain-powerpc_e500mc_gcc-8.4.0_glibc.
I have yet to compare the differences.

fragow · 2023-07-26T06:20:25Z

In my tests yesterday I accidentally had an old version of libucode.so. As a result, the function 'update_line()' from lexer.c was not up to date, which led to the line counting error. With the newly compiled version, 167 tests are now also okay on the PPC. Thank you for the effort.

jow- closed this as completed in 4f4f38f Jul 25, 2023

jow- mentioned this issue Oct 23, 2023

Erroneous equality test in tracing mode #177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime errors on PPC (big endian) #165

Runtime errors on PPC (big endian) #165

fragow commented Jul 24, 2023

jow- commented Jul 25, 2023

fragow commented Jul 25, 2023

ynezz commented Jul 25, 2023

fragow commented Jul 25, 2023

fragow commented Jul 26, 2023

Runtime errors on PPC (big endian) #165

Runtime errors on PPC (big endian) #165

Comments

fragow commented Jul 24, 2023

jow- commented Jul 25, 2023

fragow commented Jul 25, 2023

ynezz commented Jul 25, 2023

fragow commented Jul 25, 2023

fragow commented Jul 26, 2023