`bool` to JS?

[notEvil]

Hi,

I see strange behavior with wrapped JS functions dealing with bool. With a clean Linux build (sh ./scripts/provision.sh LINUX and BOARD=LINUX make), ./espruino fed with isNaN("0.1") prints true

>isNaN("0.1")
=true
>isNaN("0.0") 
=false

With the return type change to int (in .c, .h and JSON), it returns 0 as expected

>isNaN("0.1")
=0
>isNaN("0.0")
=0

For the same reason, some tests fail.

OS: Arch gcc: 12.2.1 20230201

I tried to track a "memory leak" in my C code (memory fills up with 0 lock vars), but can't trust my build fixed in https://github.com/espruino/Espruino/commit/cf97ac90419ef6a011e28e861823ea54c98746c1

[gfwilliams]

Interesting, thanks. Maybe you could try building with -m32 (for a 32 bit build) and see if that helps?

I guess it could be a problem with argument passing on 64 bit... On ARM/etc builds it seems fine, but I do remember hitting some strange issue with the emscripten build and functions that returned bools

[notEvil]

It does fix the issue, thanks!

edit: different issue, see https://github.com/espruino/Espruino/issues/2328 Now the tests fail at ``` espruino: ----------------------------- TEST /home/user/git/t/tests/test_typed_array_sort.js ... ASSERT(jsvGetLocks(var) < 15) FAILED AT src/jsvar.c:778 #1[r1,l2] Object { #2[r1,l2] ASSERT(jsvGetLocks(var) < 15) FAILED AT src/jsvar.c:758 EXITING. ``` in 2v16 and master (slightly different line numbers) but thats less troublesome I guess :)

[notEvil]

There is more and probably related to build config. When I run

function a() {
  var a = '12345678911234567892123456789312345';
}
E.dumpLockedVars();

with ./espruino /path/to/js I get

#4[r0,l1] Array(0) [ ]
#5[r0,l1] String [1 blocks] "" 
#14[r0,l2] String [8 blocks] "function a() {\n  a = '1234567891123456789212345678931234567894123456789512345678961234567897123456789812345';\n}\nE.dumpLockedVars();\n" 
#25[r0,l0] Unknown 58 
#26[r0,l1] NewChild PARENT:  #29[r4,l2] NativeFunction 0x565c75c0 (1) { }CHILD: String [2 blocks] "dumpLockedVars"  #27[r1,l1] NativeFunction 0x5660e025 (0) { } 
ASSERT(ref <= jsVarsSize) FAILED AT src/jsvar.c:218
  #1[r1,l2] Object { 
    #2[r1,l2] Name String [1 blocks] "\xFF"      #3[r1,l1] Object { 
        #6[r1,l2] Name String [2 blocks] "timers"          #8[r2,l0] Array(0) [ ] 
        #9[r1,l2] Name String [2 blocks] "watches"          #11[r2,l0] Array(0) [ ] 
      } 
    #13[r1,l2] Name String [1 blocks] "quit"      #12[r1,l0] NativeFunction 0x56603a8d (0) { } 
    #22[r1,l2] Name String [1 blocks] "a"      #23[r1,l0] Function { 
        #24[r1,l2] Name String [1 blocks] "\xFFcod"          #32[r1,l0] FlatString [5 blocks] "a = '1234567891123456789212345678931234567894123456789512345678961234567897123456789812345'9"  
      } 
    #28[r1,l2] Name String [1 blocks] "E"      #29[r4,l2] NativeFunction 0x565c75c0 (1) { } 
  }
EXITING.

The assert fails with ref >>> jsVarsSize. When the string is one char shorter the assert succeeds (the function is significant). Can you reproduce this?

edit: added var in function

[gfwilliams]

That's odd. I get:

#4[r0,l1] Array(0) [ ]
#5[r0,l1] String [1 blocks] "" 
#12[r0,l2] String [5 blocks] "function a() {\n  var a = '12345678911234567892123456789312345';\n}\nE.dumpLockedVars();\n" 
#25[r1,l1] NativeFunction 0x7e279da2 (0) { }
#26[r0,l1] NewChild PARENT:  #23[r4,l2] NativeFunction 0x7e2106f9 (1) { }CHILD: String [2 blocks] "dumpLockedVars"  #25[r1,l1] NativeFunction 0x7e279da2 (0) { } 

But no assertion (64 bit Linux build). The dump itself looks fine in my case.

[notEvil]

With a 64bit build (without 'CFLAGS+=-m32', 'LDFLAGS+=-m32', 'DEFINES+=-DUSE_CALLFUNCTION_HACK', # For testing 32 bit builds in ./boards/LINUX.py) the string must be longer

function a() {
  var a = '1234567891123456789212345678931234567894123456789512345678961234567897';
}
E.dumpLockedVars();

[gfwilliams]

Interesting - yes, I see that too with the longer string! That is quite concerning - I'm pretty sure that wouldn't have been an issue previously so maybe something has regressed?

I'm afraid I'm off work for the next week so I may not get a chance to look into this properly though

[notEvil]

Found it!

diff --git a/src/jsvar.c b/src/jsvar.c
index aac08bdc..804c0aaa 100644
--- a/src/jsvar.c
+++ b/src/jsvar.c
@@ -4099,6 +4099,9 @@ void jsvDumpLockedVars() {
         jsvGarbageCollectMarkUsed(var);
         jsvTrace(var, 0);
       }
+      // if we have a flat string, skip that many blocks
+      if (jsvIsFlatString(var))
+        i = (JsVarRef)(i+jsvGetFlatStringBlocks(var));
     }
   }
   isMemoryBusy = MEM_NOT_BUSY;

edit: off topic, see https://github.com/espruino/Espruino/pull/2325 While "print debugging" I saw that StringExt are freed in ascending order. This leads to reversed links and hence lost room for flat strings. See https://github.com/espruino/Espruino/blob/f173832d935f87033f560b88de4af4f3a0fa1cde/src/jsvar.c#L671 For instance, with 1: String 2: StringExt 3: StringExt 4: first empty the links become 1 -> 3 -> 2 -> 4

[gfwilliams]

Argh - thanks for spotting that! Did you want to do a PR for that change or shall I do it?

Thanks for #2325 and #2329 as well - I'm just catching up after last week off, but I'll try and look into this later this week. It might be the free can actually be done a bit more efficiently (turn the stringexts into a linked list in order, and then add that to the free list right at the end rather than calling jsvFreePtrInternal multiple times).

[notEvil]

Argh - thanks for spotting that! Did you want to do a PR for that change or shall I do it?

Its a minor fix, so you can add it to one of your commits :)

It might be the free can actually be done a bit more efficiently (turn the stringexts into a linked list in order, and then add that to the free list right at the end rather than calling jsvFreePtrInternal multiple times).

True, but I don't know if its really worth it (long chains of StringExt are rare due to flat strings).

[gfwilliams]

Thanks - just added that tweak. I also looked at the isNaN thing and it looks like on 64 bit the bool return value maybe gets passed back as a byte even though we're getting a 32 bit number back, so I could change result!=0 to result&1 and it seems to work fine for bools now.

I'll do some tests on the StringExt stuff when I get a moment - anything we can do to reduce fragmentation is good, even if it doesn't happen that much :)

[notEvil]

https://github.com/espruino/Espruino/pull/2325/commits/6c2870cabfdbe04e3c4a9e1f2acd2ccb6692bb60 feels like taking away all the fun, sry!

[gfwilliams]

:) Thanks - that looks really tidy. Just merged!