duskos

commit b1968b19c4d94c75649409191792e8dbd161969d
parent db256f2dfad6de68510b228c97cefd798cf80df2
Author: Virgil Dupras <hsoft@hardcoded.net>
Date:   Sat, 17 Dec 2022 12:04:25 -0500

comp/c: fix submarine bug with "typeunsigned?"

the word "typeunsigned?" assumed a simple type but was used in cases where it
was possible to have a CType. This was broken since forever, but because so far
didn't cause problems, by chance. With the introduction of "uchar" "ushort" and
"uint" typedefs, we added a level of type indirection that wasn't there before
and activated the bug, which is *very* tricky to debug because the result of a
buggy "typeunsigned?" depends on the type's place in memory.

I've been tracking this bug for 2 days, thinking it was some really deep memory
corruption bug. Nope, just a stupid bug.

The test added in this commit doesn't fail with the unamended code... by chance.
If you do something like add a "?f<< /comp/c/pp.fs" line at the top of
/tests/all.fs, then you'll get the fail, under i386 only, which is fixed by this
commit.

Diffstat:
M
fs/comp/c/pp.fs
 | 
1
+
M
fs/comp/c/type.fs
 | 
7
++++---
M
fs/tests/comp/c/cc.fs
 | 
2
++
M
fs/tests/comp/c/test.c
 | 
6
++++++

4 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/comp/c/pp.fs b/fs/comp/c/pp.fs
@@ -8,6 +8,7 @@
 \ then processed regularly until the end of string, at which point we go back to
 \ our previous feed. This system works recursively.
 \ Macro are stored as null-terminated ranges, *not* regular strings.
+?f<< /lib/str.fs
 ?f<< /lib/arena.fs
 ?f<< /comp/c/feed.fs
 
diff --git a/fs/comp/c/type.fs b/fs/comp/c/type.fs
@@ -58,9 +58,6 @@ STORAGE_MEM value curstorage
 : _arena curstorage STORAGE_MEM <> curstatic or if _tarena else _parena then ;
 
 4 stringlist typenames "void" "char" "short" "int"
-: typeunsigned? ( type -- flags ) 4 rshift 1 and ;
-: typesigned! ( type -- type ) $f and ;
-: typeunsigned! ( type -- type ) $10 or ;
 : type*lvl ( type -- lvl ) 3 and ;
 \ TODO: have the _assert in type*lvl! it's not possible now because it breaks
 \ the CC vms.
@@ -148,6 +145,10 @@ struct[ CType
   : :append ( other self -- ) 2dup :size swap to offset llappend ;
 ]struct
 
+: ensurebasetype ( type -- type )
+  dup ctype? if ctype' CType type ensurebasetype then ;
+: typeunsigned? ( type -- flags ) ensurebasetype 4 rshift 1 and ;
+
 \ Typedefs are dictionary entries in the "typedefs" dicts, which contain a 4b
 \ value representing the type it aliases.
 create typedefs 0 , 0 c, \ this is a dict link
diff --git a/fs/tests/comp/c/cc.fs b/fs/tests/comp/c/cc.fs
@@ -89,6 +89,8 @@ binop9 $1234 #eq
 $72 2 binop11 $1c #eq
 12 5 binop12 17 #eq scnt not #
 54 5 binop12 -1 #eq scnt not #
+1 42 binop13 1 #eq
+-1 42 binop13 0 #eq
 structop1 44 #eq
 structop2 45 #eq
 structop3 42 #eq
diff --git a/fs/tests/comp/c/test.c b/fs/tests/comp/c/test.c
@@ -360,6 +360,12 @@ char binop11(char a, char b) {
 int binop12(int a, int b) {
 	return a < 42 ? adder(a, b) : -1 ;
 }
+// This one fixes a bug where "typeunsigned?" didn't account for the possibility
+// of getting a CType in parameter. But this test doesn't reliably fail. It
+// depends on where in memory the CType is placed.
+int binop13(ushort a, ushort b) {
+	return a < b;
+}
 short structop1() {
     globdata.bar += 2;
     return globdata.bar;