Merge #64

64: Track initialised yk locations r=ltratt a=Pavel-Durov

Partially fixes (only for serial compilation)  - #62

# Changes

- Remove reallocarray in favour of calloc
- Change YkLocations to be stored as pointers
- Change lyk interface to reflect its "hooks" functionality
- Move tests to a separate test script and enable serialised all.lua test suite
- Added logging in lyk module for ease of debugging
- Updated readme

`test.sh` was tested for resiliency multiple times:
```shell
$ try_repeat -v 100 sh ./test.sh
``` 
# Issues

## Uninitialised locations memory 
`reallocarray` does not initialise memory with default zero bytes locations as `calloc`. 
Moving to `calloc` and using pointers allowed to check for NULL with default values set at initialisation time.

## Tests

When lua tests are executed one by one as single files it produces different results from when it's executed via `all.lua` test suite.

Example:

```shell

$ YKD_SERIALISE_COMPILATION=1 ../src/lua -e"_U=true" ./main.lua 
...
../src/lua: ./main.lua:343: assertion failed!
stack traceback:
        [C]: in function 'assert'
        ./main.lua:343: in main chunk
        [C]: in ?
```
When run through `all.lua` it passes:
```shell
$ YKD_SERIALISE_COMPILATION=1 ../src/lua -e"_U=true" ./all.lua 
...
***** FILE 'main.lua'*****
...
***** FILE 'gc.lua'*****
...
```
Since `all.lua` is what we base the stability of yklua, I think we should include it in the tests as is, currently only serialised compilation works.

Running `all.lua` prior to these changes results in error (`main/56c5787799b876f36babbae24e9afc025806b831`):
```
$ YKD_SERIALISE_COMPILATION=1 gdb -ex 'r' -ex 'bt' --args ../src/lua -e"_U=true" ./all.lua 
...
***** FILE 'main.lua'*****

Program received signal SIGSEGV, Segmentation fault.
core::sync::atomic::AtomicUsize::fetch_sub (self=<optimised out>) at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/sync/atomic.rs:2575
2575    /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/sync/atomic.rs: No such file or directory.
#0  core::sync::atomic::AtomicUsize::fetch_sub (self=<optimised out>) at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/sync/atomic.rs:2575
#1  alloc::sync::{impl#33}::drop<lock_api::mutex::Mutex<parking_lot::raw_mutex::RawMutex, ykrt::location::HotLocation>, alloc::alloc::Global> (self=0x7fffffffb410)
    at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/alloc/src/sync.rs:2370
#2  0x00007ffff7b0d4ab in core::ptr::drop_in_place<alloc::sync::Arc<lock_api::mutex::Mutex<parking_lot::raw_mutex::RawMutex, ykrt::location::HotLocation>, alloc::alloc::Global>> () at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/ptr/mod.rs:497
#3  0x00007ffff7b0067e in core::mem::drop<alloc::sync::Arc<lock_api::mutex::Mutex<parking_lot::raw_mutex::RawMutex, ykrt::location::HotLocation>, alloc::alloc::Global>> (_x=...) at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/mem/mod.rs:987
#4  0x00007ffff7b121f0 in ykrt::location::{impl#1}::drop (self=0x7fffffffb468) at ykrt/src/location.rs:197
#5  0x00007ffff7affa9b in core::ptr::drop_in_place<ykrt::location::Location> () at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/ptr/mod.rs:497
#6  0x00007ffff7aff6bd in core::mem::drop<ykrt::location::Location> (_x=...) at /rustc/ef85656a10657ba5e4f7fe2931a4ca6293138d51/library/core/src/mem/mod.rs:987
#7  0x00007ffff7b004bd in ykcapi::yk_location_drop (loc=...) at ykcapi/src/lib.rs:90
#8  0x000000000088aa0e in free_loc (f=<optimised out>, i=<optimised out>, idx=<optimised out>) at lyk.c:66
#9  0x000000000088aba5 in yk_free_locactions (f=0x928cc0) at lyk.c:76
--Type <RET> for more, q to quit, c to continue without paging--
#10 0x0000000000807738 in luaF_freeproto (L=0x916e38, f=0x928cc0) at lfunc.c:276
#11 0x0000000000809fde in freeobj (L=0x916e38, o=0x928cc0) at lgc.c:767
#12 0x0000000000817145 in sweepgen (L=0x916e38, g=<optimised out>, p=<optimised out>, limit=<optimised out>, pfirstold1=<optimised out>) at lgc.c:1106
#13 0x0000000000816713 in youngcollection (L=0x916e38, g=0x916f00) at lgc.c:1239
#14 0x000000000081557d in genstep (L=0x916e38, g=0x916f00) at lgc.c:1434
#15 0x0000000000815044 in luaC_step (L=0x916e38) at lgc.c:1686
#16 0x00000000007e4447 in lua_pushstring (L=0x916e38, s=<optimised out>) at lapi.c:553
#17 0x000000000089e60c in findloader (L=0x916e38, name=0x928df8 "tracegc") at loadlib.c:641
#18 0x000000000089de0a in ll_require (L=0x916e38) at loadlib.c:666
#19 0x00000000007fda75 in precallC (L=0x916e38, func=<optimised out>, nresults=<optimised out>, f=0x89db80 <ll_require>) at ldo.c:506
#20 0x00000000007fe016 in luaD_precall (L=0x916e38, func=0x923780, nresults=1) at ldo.c:569
#21 0x0000000000883f68 in luaV_execute (L=0x916e38, ci=<optimised out>) at lvm.c:1655
#22 0x00000000007feb3b in ccall (L=0x916e38, func=<optimised out>, nResults=<optimised out>, inc=<optimised out>) at ldo.c:609
#23 0x00000000007fec61 in luaD_callnoyield (L=0x916e38, func=0x917730, nResults=-1) at ldo.c:627
#24 0x00000000007eaf03 in f_call (L=0x916e38, ud=<optimised out>) at lapi.c:1041
#25 0x00000000007f8be7 in luaD_rawrunprotected (L=0x916e38, f=0x7eae40 <f_call>, ud=0x7ffff2487308) at ldo.c:144
#26 0x0000000000801406 in luaD_pcall (L=0x916e38, func=0x7eae40 <f_call>, u=0x7ffff2487308, old_top=<optimised out>, ef=<optimised out>) at ldo.c:926
#27 0x00000000007ea9ec in lua_pcallk (L=0x916e38, nargs=<optimised out>, nresults=<optimised out>, errfunc=<optimised out>, ctx=<optimised out>, k=<optimised out>)
    at lapi.c:1067
#28 0x00000000007dc623 in docall (L=0x916e38, narg=0, nres=-1) at lua.c:160
#29 0x00000000007dbd24 in handle_script (L=0x916e38, argv=<optimised out>) at lua.c:255
#30 0x00000000007d9fe3 in pmain (L=0x916e38) at lua.c:634
#31 0x00000000007fda75 in precallC (L=0x916e38, func=<optimised out>, nresults=<optimised out>, f=0x7d97f0 <pmain>) at ldo.c:506
#32 0x00000000007fe0c8 in luaD_precall (L=0x916e38, func=0x9176f0, nresults=1) at ldo.c:572
#33 0x00000000007fea7f in ccall (L=0x916e38, func=0x9176f0, nResults=1, inc=<optimised out>) at ldo.c:607
#34 0x00000000007fec61 in luaD_callnoyield (L=0x916e38, func=0x9176f0, nResults=1) at ldo.c:627
#35 0x00000000007eaf03 in f_call (L=0x916e38, ud=<optimised out>) at lapi.c:1041
#36 0x00000000007f8be7 in luaD_rawrunprotected (L=0x916e38, f=0x7eae40 <f_call>, ud=0x7ffff2487058) at ldo.c:144
#37 0x0000000000801406 in luaD_pcall (L=0x916e38, func=0x7eae40 <f_call>, u=0x7ffff2487058, old_top=<optimised out>, ef=<optimised out>) at ldo.c:926
#38 0x00000000007ea9ec in lua_pcallk (L=0x916e38, nargs=<optimised out>, nresults=<optimised out>, errfunc=<optimised out>, ctx=<optimised out>, k=<optimised out>)
    at lapi.c:1067
#39 0x00000000007d94b0 in main (argc=<optimised out>, argv=<optimised out>) at lua.c:660
```

That's cause we're freeing uninitialised yk locations.


Co-authored-by: Pavel Durov <[email protected]>

.buildbot.sh

@@ -31,23 +31,4 @@ cd ..
 export YK_BUILD_TYPE=debug
 make -j `nproc`
 
-cd tests
-# YKFIXME: The JIT can't yet run the test suite, but the following commented
-# commands are what we are aiming at having work.
-#
-# ../src/lua -e"_U=true" all.lua
-# YKD_SERIALISE_COMPILATION=1 ../src/lua -e"_U=true" all.lua
-#
-# (Adding `YKD_SERIALISE_COMPILATION` exercises different threading behaviour
-# that could help to shake out more bugs)
-#
-# Until we can run `all.lua` reliably, we just run the tests that we know to
-# run within reasonable time).
-LUA=../src/lua
-for serialise in 0 1; do
-    for test in api bwcoercion closure code events \
-        gengc pm tpack tracegc vararg goto cstack locals; do
-        echo "### YKD_SERIALISE_COMPILATION=$serialise $test.lua ###"
-        YKD_SERIALISE_COMPILATION=$serialise ${LUA} -e"_U=true" ${test}.lua
-    done
-done
+sh test.sh

README.md

@@ -37,6 +37,15 @@ cd tests # navigate to tests directory
 
 ```shell
 run_docker_ci_job # local path to https://github.com/softdevteam/buildbot_config/blob/master/bin/run_docker_ci_job
+
+```
+## Debugging
+
+Use `LYK_VERBOSE` environment variable to print LYK (lua yk) debug logs:
+```shell
+LYK_VERBOSE=1 gdb --batch -ex 'r' -ex 'bt' --args ../src/lua all.lua 
+LYK_VERBOSE=1 ../src/lua all.lua 
+LYK_VERBOSE=1 sh ./test.sh
 ```
 
 ### State of Tests

src/Makefile

@@ -21,7 +21,8 @@ SYSCFLAGS=
 SYSLDFLAGS=
 SYSLIBS=
 
-MYCFLAGS=
+# YKFIXME: LYK_DEBUG is a debug flag for the Yk integration. Once YkLua is stable we can remove it.
+MYCFLAGS=-DLYK_DEBUG
 MYLDFLAGS=
 MYLIBS=
 MYOBJS=

src/lcode.c

@@ -391,7 +391,7 @@ int luaK_code (FuncState *fs, Instruction i) {
                   MAX_INT, "opcodes");
   f->code[fs->pc++] = i;
 #ifdef USE_YK
-  yk_set_location(f, i, idx, fs->pc);
+  yk_ok_instruction_loaded(f, i, idx);
 #endif
   savelineinfo(fs, f, fs->ls->lastline);
   return idx;  /* index of new instruction */

src/lfunc.c

@@ -251,7 +251,7 @@ Proto *luaF_newproto (lua_State *L) {
   f->sizep = 0;
   f->code = NULL;
 #ifdef USE_YK
-  f->yklocs = NULL;
+  yk_on_newproto(f);
 #endif
   f->sizecode = 0;
   f->lineinfo = NULL;
@@ -273,7 +273,7 @@ Proto *luaF_newproto (lua_State *L) {
 
 void luaF_freeproto (lua_State *L, Proto *f) {
 #ifdef USE_YK
-  yk_free_locactions(f);
+  yk_on_proto_free(f);
 #endif
   luaM_freearray(L, f->code, f->sizecode);
   luaM_freearray(L, f->p, f->sizep);

src/lobject.h

@@ -556,7 +556,8 @@ typedef struct Proto {
   TValue *k;  /* constants used by the function */
   Instruction *code;  /* opcodes */
 #ifdef USE_YK
-  YkLocation *yklocs; /* JIT locations */
+  YkLocation **yklocs; /* JIT locations */
+  int yklocs_size;
 #endif
   struct Proto **p;  /* functions defined inside the function */
   Upvaldesc *upvalues;  /* upvalue information */

src/lundump.c

@@ -267,7 +267,7 @@ static void loadFunction (LoadState *S, Proto *f, TString *psource) {
   f->maxstacksize = loadByte(S);
   loadCode(S, f);
   #ifdef USE_YK
-  yk_set_locations(f);
+  yk_on_proto_loaded(f);
   #endif
   loadConstants(S, f);
   loadUpvalues(S, f);

src/lyk.c

@@ -6,75 +6,148 @@
 #include "ldebug.h"
 #include <stdlib.h>
 #include <yk.h>
-
-#define _DEFAULT_SOURCE /* for reallocarray() */
-
+#include "stdio.h"
 /*
  *  Function prototypes in Lua are loaded through two methods:
  *    1.  They are loaded from text source via the `luaY_parser`.
  *    2.  They can also be loaded from binary representation using `luaU_undump`, where
  *        prototypes are previously dumped and saved.
- * 
- *  Yk tracing locations are allocated using both of these methods 
+ *
+ *  Yk tracing locations are allocated using both of these methods
  *  using `yk_set_location` and `yk_set_locations` respectively.
-*/
+ */
+
+#ifdef LYK_DEBUG
+static int LYK_VERBOSE = -1;
+
+int is_verbose() {
+  if (LYK_VERBOSE == -1) {
+    LYK_VERBOSE = getenv("LYK_VERBOSE") != NULL;
+  }
+  return LYK_VERBOSE;
+}
+
+void print_proto_info(char *msg, Proto *f) {
+  if (!is_verbose()){
+    return;
+  }
+  char *source = NULL;
+  if (f->source != NULL) {
+    source = getstr(f->source);
+  }
+  char *vars = NULL;
+  if (f->locvars != NULL && f->locvars->varname != NULL) {
+    vars = getstr(f->locvars->varname);
+  }
+  printf("[LYK] %s. \t f:\t%p \t source: %s: \t vars: %s", msg, f, source, vars);
+  if (f->yklocs != NULL) {
+    printf("\t sizecode: %d", f->sizecode);
+  }
+  printf("\n");
+}
+#endif // LYK_DEBUG
+
+void yk_on_newproto(Proto *f) {
+  #ifdef LYK_DEBUG
+  if (is_verbose()) {
+    printf("[LYK] yk_new_proto %p\n", f);
+  }
+  #endif // LYK_DEBUG
+  f->yklocs = NULL;
+}
 
 /*
  *  Is the instruction `i` the start of a loop?
  *
- *  YKFIXME: Numeric and Generic loops can be identified by `OP_FORLOOP` and `OP_TFORLOOP` opcodes. 
- *  Other loops like while and repeat-until are harder to identify since they are based on 
+ *  YKFIXME: Numeric and Generic loops can be identified by `OP_FORLOOP` and `OP_TFORLOOP` opcodes.
+ *  Other loops like while and repeat-until are harder to identify since they are based on
  *  `OP_JMP` instruction.
  */
 int is_loop_start(Instruction i) {
   return (GET_OPCODE(i) == OP_FORLOOP || GET_OPCODE(i) == OP_TFORLOOP);
 }
 
-inline YkLocation *yk_lookup_ykloc(CallInfo *ci, Instruction *pc){
+inline YkLocation *yk_lookup_ykloc(CallInfo *ci, Instruction *pc) {
   YkLocation *ykloc = NULL;
   lua_assert(isLua(ci));
   Proto *p = ci_func(ci)->p;
   lua_assert(p->code <= pc && pc <= p->code + p->sizecode);
   if (is_loop_start(*pc)) {
-    ykloc = &p->yklocs[pc - p->code];
+    ykloc = p->yklocs[pc - p->code];
   }
   return ykloc;
 }
 
-inline void yk_set_location(Proto *f, Instruction i, int idx, int pc) {
-  // YKOPT: Reallocating for every instruction is inefficient.
-  f->yklocs = reallocarray(f->yklocs, pc, sizeof(YkLocation));
-  lua_assert(f->yklocs != NULL && "Expected yklocs to be defined!");
-  if (is_loop_start(i))
-  {
-    f->yklocs[idx] = yk_location_new();
+void set_location(Proto *f, int i) {
+  YkLocation *loc = (YkLocation *)malloc(sizeof(YkLocation));
+  lua_assert(loc != NULL && "Expected loc to be defined!");
+  *loc = yk_location_new();
+  f->yklocs[i] = loc;
+  #ifdef LYK_DEBUG
+  if (is_verbose()){
+    printf("[LYK] yk_location_new. %p->yklocs[%d]=%p\n", f, i, loc);
   }
+  #endif // LYK_DEBUG
 }
 
-inline void yk_set_locations(Proto *f) {
-  f->yklocs = calloc(f->sizecode, sizeof(YkLocation));
-  lua_assert(f->yklocs != NULL && "Expected yklocs to be defined!");
-  for (int i = 0; i < f->sizecode; i++){
-     if (is_loop_start(i)){
-      f->yklocs[i] = yk_location_new();
+inline void yk_ok_instruction_loaded(Proto *f, Instruction i, int idx) {
+  // YKOPT: Reallocating for every instruction is inefficient.
+  YkLocation **new_locations = calloc(f->sizecode, sizeof(YkLocation *));
+  lua_assert(new_locations != NULL && "Expected yklocs to be defined!");
+
+  // copy previous locations over
+  if (f->yklocs != NULL) {
+    for (int i = 0; i < f->yklocs_size; i++) {
+      if (f->yklocs[i] != NULL) {
+        new_locations[i] = f->yklocs[i];
+      }
+      else {
+        new_locations[i] = NULL;
+      }
     }
+    free(f->yklocs);
   }
+  f->yklocs = new_locations;
+  f->yklocs_size = f->sizecode;
+  #ifdef LYK_DEBUG
+  if (is_loop_start(i)) {
+    set_location(f, idx);
+  }
+  #endif // LYK_DEBUG
 }
 
-void free_loc(Proto *f, Instruction i, int idx) {
-  if (is_loop_start(i)) {
-    yk_location_drop(f->yklocs[idx]);
+inline void yk_on_proto_loaded(Proto *f) {
+  #ifdef LYK_DEBUG
+  print_proto_info("yk_set_locations", f);
+  #endif // LYK_DEBUG
+  f->yklocs = calloc(f->sizecode, sizeof(YkLocation *));
+  lua_assert(f->yklocs != NULL && "Expected yklocs to be defined!");
+  f->yklocs_size = f->sizecode;
+  for (int i = 0; i < f->sizecode; i++) {
+    if (is_loop_start(i)) {
+      set_location(f, i);
+    }
   }
 }
 
-inline void yk_free_locactions(Proto *f) {
-  // YK locations are initialised as close as possible to the function loading, 
+inline void yk_on_proto_free(Proto *f) {
+  // YK locations are initialised as close as possible to the function loading,
   // However, this load can fail before we initialise `yklocs`.
   // This NULL check is a workaround for that.
-  if (f->yklocs != NULL) {
+  if (f->yklocs != NULL)  {
     for (int i = 0; i < f->sizecode; i++) {
-      free_loc(f, f->code[i], i);
+      YkLocation *loc = f->yklocs[i];
+      if (loc != NULL) {
+        #ifdef LYK_DEBUG
+        if (is_verbose()){
+          printf("[LYK] yk_location_drop. %p->yklocs[%d]=%p\n", f, i, loc);
+        }
+        #endif // LYK_DEBUG
+        yk_location_drop(*loc);
+        free(loc);
+      }
     }
+
     free(f->yklocs);
     f->yklocs = NULL;
   }

src/lyk.h

@@ -5,11 +5,13 @@
 #include "lopcodes.h"
 #include "lstate.h"
 
-void yk_set_location(Proto *f, Instruction i, int idx, int pc);
+void yk_on_newproto(Proto *f);
 
-void yk_set_locations(Proto *f);
+void yk_ok_instruction_loaded(Proto *f, Instruction i, int idx);
 
-void yk_free_locactions(Proto *f);
+void yk_on_proto_loaded(Proto *f);
+
+void yk_on_proto_free(Proto *f);
 
 YkLocation* yk_lookup_ykloc(CallInfo *ci, Instruction *pc);
 

test.sh

@@ -0,0 +1,29 @@
+#!/bin/sh
+
+set -e
+
+cd tests
+
+# YKFIXME: The JIT can't yet run the test suite, but the following commented
+# commands are what we are aiming at having work.
+#
+# ../src/lua -e"_U=true" all.lua
+# YKD_SERIALISE_COMPILATION=1 ../src/lua -e"_U=true" all.lua
+#
+# (Adding `YKD_SERIALISE_COMPILATION` exercises different threading behaviour
+# that could help to shake out more bugs)
+#
+# Until we can run `all.lua` reliably, we just run the tests that we know to
+# run within reasonable time).
+
+LUA=../src/lua
+
+# Non-serialised compilation tests
+# YKFIXME: The following tests are known to work with non-serialised JIT
+for test in api bwcoercion closure code events \
+    gengc pm tpack tracegc vararg goto cstack locals; do
+    YKD_SERIALISE_COMPILATION=0 ${LUA} -e"_U=true" ${test}.lua
+done
+
+# Serialised compilation tests
+YKD_SERIALISE_COMPILATION=1 ${LUA} -e"_U=true" all.lua