[WIP] eval: Refactor eval.c #5119

Open
wants to merge 90 commits into
from

Projects

None yet

7 participants

@ZyX-I
Contributor
ZyX-I commented Jul 26, 2016

Ref #5081.

@ZyX-I
Contributor
ZyX-I commented Sep 4, 2016

This part should be mostly ready. Need to

  1. silence linter (376 new errors)
  2. add some unit tests

, but no more function moves in this PR.

@bfredl bfredl and 2 others commented on an outdated diff Sep 4, 2016
src/nvim/CMakeLists.txt
@@ -199,7 +200,7 @@ add_custom_command(OUTPUT ${GENERATED_UNICODE_TABLES}
)
add_custom_command(OUTPUT ${GENERATED_API_DISPATCH} ${API_METADATA}
- COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${PROJECT_SOURCE_DIR}/scripts ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}
+ COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${API_SOURCE_DIR} ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}
@bfredl
bfredl Sep 4, 2016 Member

Seems unnecessary to have a variable for every subdir, no? if we should follow the existing pattern
set(DEPRECATED_DISPATCH_FILE ${PROJECT_SOURCE_DIR}/src/nvim/api/deprecated_dispatch.lua but when thinking on it why not have set(NVIM_SOURCE_DIR ${PROJECT_SOURCE_DIR}/src/nvim) and then write ${NVIM_SOURCE_DIR}/api/deprecated_dispatch.lua, ${NVIM_SOURCE_DIR}/ex_cmds.lua etc in the build rules?

@ZyX-I
ZyX-I Sep 4, 2016 Contributor

I am wondering whether there is cmake variable which is like ${PROJECT_SOURCE_DIR}, but contains a directory where currently processed CMakeLists.txt lives. Such thing is better suited here.

@fwalch
fwalch Sep 4, 2016 Member

From http://www.cmake.org/Wiki/CMake_Useful_Variables:

CMAKE_CURRENT_LIST_DIR
(since 2.8.3) this is the directory of the listfile currently being processed.

@ZyX-I
Contributor
ZyX-I commented Sep 4, 2016

AppVeyor complains that

C:/projects/neovim/build/include/mbyte.h.generated.h:50:36: error: unknown type name 'WCHAR'
int utf8_to_utf16(const char _str, WCHAR *_strw) FUNC_ATTR_NONNULL_ALL;

, but I did not remove any headers from mbyte.h. Guess that there is some missing include in mbyte.h, where is this type defined on Windows (more precisely, what should I include, I see it defined in winnt.h in headers installed by mingw)?

@fwalch fwalch commented on an outdated diff Sep 4, 2016
.ci/common/test.sh
@@ -74,6 +74,10 @@ run_oldtests() {
valgrind_check "${LOG_DIR}"
}
+run_single_includes_tests() {
+ ${MAKE_CMD} -C "${BULID_DIR}" check-single-includes
@fwalch
fwalch Sep 4, 2016 Member

Typo on BUILD_DIR.

This line should probably go into .ci/build.bat, too.

@ZyX-I ZyX-I commented on an outdated diff Sep 4, 2016
test/unit/eval/typval_spec.lua
@@ -1,7 +0,0 @@
-local helpers = require('test.unit.helpers')
@ZyX-I
ZyX-I Sep 4, 2016 edited Contributor

Not that this file should not exist, it should not exist in that commit.

@justinmk justinmk and 1 other commented on an outdated diff Sep 4, 2016
.ci/run_tests.sh
@@ -19,4 +19,6 @@ run_oldtests
install_nvim
+run_single_includes_test
@justinmk
justinmk Sep 4, 2016 edited Member

why does this come after install_nvim ? can it go before near build_nvim (line 10) to fail the build faster?

@ZyX-I
ZyX-I Sep 4, 2016 Contributor

When I was writing this I was not yet sure that if additional executables were built they will not be installed, so wrote test after install test. Though I now know that they are not installed and it should be possible to put this call there as well.

@justinmk justinmk commented on an outdated diff Sep 5, 2016
src/nvim/CMakeLists.txt
@@ -339,4 +345,81 @@ add_library(nvim-test MODULE EXCLUDE_FROM_ALL ${NEOVIM_GENERATED_SOURCES}
target_link_libraries(nvim-test ${NVIM_LINK_LIBRARIES})
set_property(TARGET nvim-test APPEND_STRING PROPERTY COMPILE_FLAGS -DUNIT_TESTING)
+# Helper targets that verify that specific headers can be included alone (i.e.
+# do not depend on other includes)
+set(HEADER_CHECK_FILES)
+set(NO_SINGLE_CHECK_HEADERS
@justinmk
justinmk Sep 5, 2016 Member

consider renaming this to HEADER_CHECK_SKIPPED for parallel form with HEADER_CHECK_TARGETS et al.

@ZyX-I
Contributor
ZyX-I commented Sep 10, 2016

It appears that single includes tests used to fail just because I have written singular in place of plural. Wondering why the printed result was that inadequate: out of “.ci/run_tests.sh: line 13: run_single_includes_test: command not found” (with different line though) only “mand not found” was left.

@ZyX-I
Contributor
ZyX-I commented Sep 11, 2016

Appveyor errors are something inadequate:

In file included from C:/Projects/neovim-1kmqj/src/nvim/mbyte.h:7:0,
                 from C:/Projects/neovim-1kmqj/src/nvim/eval/typval.h:10,
                 from C:\Projects\neovim-1kmqj\src\nvim\eval\decode.c:5:
C:/msys64/mingw64/x86_64-w64-mingw32/include/winnt.h:291:11: error: unknown type name 'CONST'
   typedef CONST WCHAR *LPCWCH,*PCWCH;
           ^

(and ten thousand lines of errors like this): looks like if winnt.h could not be directly included at all.

@jamessan
Member
jamessan commented Sep 11, 2016 edited

looks like if winnt.h could not be directly included at all.

In my experience, Windows header's are pretty bad in terms of dependencies on other headers and include order. I'd suggest including Windows.h (which pulls in a lot of other relevant headers) and adding -DWIN32_LEAN_AND_MEAN to the preprocessor defines.

@ZyX-I
Contributor
ZyX-I commented Sep 18, 2016

Test linter has found an error which for some reason cannot be silenced by -- luacheck: ignore:

local function first_di(d)
  for _, di in dict_iter(d) do
    return di
  end
end

: error is “loop is executed at most once”. I can, of course, remove the syntax sugar behind for … in, but this code is cleaner.

src/nvim/CMakeLists.txt
@@ -204,14 +206,19 @@ add_custom_command(OUTPUT ${GENERATED_UNICODE_TABLES}
)
add_custom_command(OUTPUT ${GENERATED_API_DISPATCH} ${API_METADATA}
- COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${CMAKE_CURRENT_LIST_DIR} ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}
+ COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${CMAKE_CURRENT_LIST_DIR}/api ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}
@bfredl
bfredl Sep 25, 2016 Member

Isn't it more consistent with just ${CMAKE_CURRENT_LIST_DIR}? just drop the changes to gendispatch.lua and it should work.

@ZyX-I
ZyX-I Sep 25, 2016 Contributor

Why should it know about which directory file is located in?

@bfredl
bfredl Sep 25, 2016 Member

it already knows the leaf filename, why not the entire path in the source directory? Someone reading gendispatch.lua (with the other scripts) would like to know where the file is, this is easier if it is consistent with other scripts and use nvimsrcdir, instead of libdir which doesn't say much. Also if some script later wants to use .lua files in several sub-directories the existing pattern is simpler.

@ZyX-I ZyX-I referenced this pull request in Olivine-Labs/busted Sep 25, 2016
Closed

Support running each test file in a separate processes #278

@ZyX-I
Contributor
ZyX-I commented Sep 25, 2016

I have now big problems with busted and unit tests: Vim has lots of global state, including lists of lists for garbage collecting and writing my tests for typval.c leads to semi-random (stable on one system, but at different point in different systems) crashes related to garbage-collecting. I guess some will be fixed by using ffi.gc(…, nil) where I have something more sensible, but this leads to memory leaks and it does not look like it is going to resolve all crashes.

And in any case I do not want to guess whether my tests (not) works because of the state left from some previous test.

@justinmk
Member
justinmk commented Sep 25, 2016 edited

As a (very ugly) workaround we could add separate cmake targets to force a new busted process chain. E.g. make test-unit-eval-typval.

@ZyX-I
Contributor
ZyX-I commented Sep 25, 2016

I currently see the following options:

  1. Try rewriting the whole thing using manual-only memory management. Will yield leaks on failing tests, but who cares. Problems:
    1. Global state is not going anywhere, only crashes may go away.
    2. Still not sure whether this will remove crashes.
    3. Manual memory management is rather bothersome, relying on luajit GC is easier.
  2. Modification of 1: if crashes do not go away additionally use Neovim allocator and never ffi.new. I remember that this sometimes helped.
  3. Your variant: create a number of targets, one per file and make unittests target depend on them. Problems:
    1. Global state is still not going anywhere, but modifications are limited by one file only.
    2. Crashes are not removed: I tried running just my test with shuffling and it crashes occasionally. Would be much easier to resolve the issue though.
  4. Use posix module to actually do what busted does not. Specifically, posix.fork and something related. Problems:
    1. Global state… Well, it finally is going away, but only if all tests use this variant.
    2. Should take more time then other solutions: busted does not expect to work like this. Though we basically need to only rethrow errors, this should be possible.
    3. Cannot estimate time which will take me to make all tests succeed: never used this module.
    4. Not sure how it acts on Windows.

I personally like the fourth variant, except for Windows problem.

@ZyX-I
Contributor
ZyX-I commented Sep 25, 2016

So far it appears that last variant is not very hard:

diff --git a/test/unit/eval/typval_spec.lua b/test/unit/eval/typval_spec.lua
index dca6715..8d5e5f4 100644
--- a/test/unit/eval/typval_spec.lua
+++ b/test/unit/eval/typval_spec.lua
@@ -7,6 +7,7 @@ local neq = helpers.neq
 local ffi = helpers.ffi
 local cimport = helpers.cimport
 local to_cstr = helpers.to_cstr
+local gen_itp = helpers.gen_itp
 local set_logging_allocator = helpers.set_logging_allocator

 local list = eval_helpers.list
@@ -27,6 +28,8 @@ local null_string  = eval_helpers.null_string
 local lib = cimport('./src/nvim/eval/typval.h', './src/nvim/memory.h',
                     './src/nvim/mbyte.h', './src/nvim/garray.h')

+local itp = gen_itp(it)
+
 local function list_items(l)
   local lis = {}
   local li = l.lv_first
@@ -129,7 +132,7 @@ describe('typval.c', function()
   describe('list', function()
     describe('item', function()
       describe('alloc()/free()', function()
-        it('works', function()
+        itp('works', function()
           local li = li_alloc(true)
           neq(nil, li)
           lib.tv_list_item_free(li)
@@ -138,7 +141,7 @@ describe('typval.c', function()

<skip>

@@ -1063,7 +1066,7 @@ describe('typval.c', function()
       else return ffi.string(ga.ga_data)
       end
     end
-    it('works', function()
+    itp('works', function()
       local l
       l = list('boo', 'far')
       eq('boo far', list_join(l, ' '))
diff --git a/test/unit/helpers.lua b/test/unit/helpers.lua
index fbd11d6..6b34306 100644
--- a/test/unit/helpers.lua
+++ b/test/unit/helpers.lua
@@ -4,6 +4,9 @@ local Set = require('test.unit.set')
 local Preprocess = require('test.unit.preprocess')
 local Paths = require('test.config.paths')
 local global_helpers = require('test.helpers')
+local posix = require('posix')
+local assert = require('luassert')
+local say = require('say')

 local neq = global_helpers.neq
 local eq = global_helpers.eq
@@ -195,6 +198,52 @@ do
   main.event_init()
 end

+local function gen_itp(it)
+  local function just_fail(msg)
+    return false
+  end
+  say:set('assertion.just_fail.positive', '%s')
+  say:set('assertion.just_fail.negative', '%s')
+  assert:register('assertion', 'just_fail', just_fail,
+                  'assertion.just_fail.positive',
+                  'assertion.just_fail.negative')
+  local function itp(msg, func)
+    it(msg, function()
+      local rd, wr = posix.pipe()
+      local pid = posix.fork()
+      if pid == 0 then
+        posix.close(rd)
+        local err, msg = pcall(func)
+        msg = tostring(msg)
+        if not err then
+          posix.write(wr, ('-\n%05u\n%s'):format(#msg, msg))
+          posix.close(wr)
+          posix._exit(1)
+        else
+          posix.write(wr, '+\n')
+          posix.close(wr)
+          posix._exit(0)
+        end
+      else
+        posix.close(wr)
+        posix.wait(pid)
+        local res = posix.read(rd, 2)
+        eq(2, #res)
+        if res == '+\n' then
+          return
+        end
+        eq('-\n', res)
+        local len_s = posix.read(rd, 5)
+        local len = tonumber(len_s)
+        neq(0, len)
+        local err = posix.read(rd, len + 1)
+        assert.just_fail(err)
+      end
+    end)
+  end
+  return itp
+end
+
 return {
   cimport = cimport,
   cppimport = cppimport,
@@ -210,4 +259,5 @@ return {
   OK = OK,
   FAIL = FAIL,
   set_logging_allocator = set_logging_allocator,
+  gen_itp = gen_itp,
 }
  1. Cannot detect when error is error and when it is a failure (i.e. when test failed and when it is a bug).

  2. Needs modification to each and every test.

  3. Of course, this is slower. Though for some strange reason this slowdown may be measured by busted (≈0.1 seconds difference), but not by zsh’s time (no significant or even stable difference between total times in both cases).

  4. Error message is a bit weird:

    (string) '
    {error message}'
    
@ZyX-I
Contributor
ZyX-I commented Sep 25, 2016

Also problems:

  1. before_each, after_each, etc are run in the main process, not in the child. Whether or not this is a problem depends on the function run (e.g. manipulations with the file system may be done anywhere).
  2. Not sure whether this needs to be fixed, but neovim library is still imported in the main process. Should be easy to fix if needed, though first each test needs to be modified (but if modified correctly, there should be no problems with initializing library in either main or child process).
  3. Additional dependency.
@justinmk
Member

Unit tests currently cannot run on Windows, I guess it will be a long time if they ever do.

I guess some will be fixed by using ffi.gc(…, nil) where I have something more sensible, but this leads to memory leaks and it does not look like it is going to resolve all crashes.

I suppose eval_clear will not help, or could cause other uncertainty/problems?

@ZyX-I
Contributor
ZyX-I commented Sep 25, 2016 edited

@justinmk eval_clear() frees lots of stuff used by eval*, it is going to render Neovim library useless (and do not forget: only separate processes do something with global state presence problem). luaposix module has a good property: if I am not mistaking, posix._exit (BTW what “private” function starting with underscore is doing in official API documentation and examples?) is not going to trigger garbage collection (thus resolving any problems with GC), yet it should not yield memory leaks because everything is freed by the OS.

Actually I think that all tests should be modified in the following way:

  1. cimport should return an object which errors out when indexed unless indexed in a subprocess. Creating a copy of metatable and overriding some methods should do the job.
  2. Obviously, itp should set some global for 1. to work correctly.
  3. And all tests should use itp.
  4. gen_itp should be able to import and initialize library or work with a library that was initialized in the main process (first to check that tests are correct, second to run tests faster).
  5. After all modifications were done using ffi.gc to set GC function to anything else, but nil should be no longer needed.

And I forgot about one problem with non-nil GC: check_alloc_log may fail because allocation log was tainted by frees from GC which may run at any point (BTW, this may be one of the reasons why GC crashes: I am not sure that GC function is fine with calling C function which calls lua function in the middle of garbage collecting, yet without C layer this should be perfectly expected because it is really used).

@ZyX-I
Contributor
ZyX-I commented Oct 2, 2016

While determining why tests fail I found that src/nvim/ui.h includes api/private/defs.h. But it should include nvim/api/private/defs.h, so it appears that somebody is adding include directory that should not be added.

+ eq(false, lib.tv_list_equal(nil, l, false, true))
+ eq(false, lib.tv_list_equal(l, nil, true, true))
+
+ -- Yet NULL lists are equal themselves
@brcolow
brcolow Oct 2, 2016 Contributor

"are equal themselves" -> "are equal to themselves"

@ZyX-I
ZyX-I Oct 2, 2016 Contributor

No. If there is one NULL list then “NULL list is equal to itself”. If there are “many” NULL lists that for some reason can be considered different (semantically in first place, there really can only be a single NULL list) then “are equal themselves” because “different” lists are compared.

Your variant sounds like “there are many NULL lists, but we are comparing only same lists”. My variant sounds like “there are many NULL lists, we may be comparing different lists”. I like second variant more then “there is one NULL list which is compared to itself” (first variant from this comment) and consider your correction incorrect. Also because there is “As well as empty lists” below: both alternative variants make this phrase incorrect (empty lists are compared not only to themselves) and require altering it.

@ZyX-I
Contributor
ZyX-I commented Oct 2, 2016

Much better: only three failures out of eight. Looks like in one case it is compiled with wrong compiler (options):

error loading module 'posix.ctype' from file '/home/travis/nvim-deps/usr/lib/lua/5.1/posix.so':
    /home/travis/nvim-deps/usr/lib/lua/5.1/posix.so: wrong ELF class: ELFCLASS64

, BSD builds are failing with an unidentified error when testing tv_list_join().

@ZyX-I
Contributor
ZyX-I commented Oct 2, 2016

Apparently cmake -E env needs more recent CMake version (I have3.5.2-r1).

@ZyX-I
Contributor
ZyX-I commented Oct 3, 2016

And simply using env leads to results worse then when there was no env:

libtool: install: /usr/bin/install -c ext/posix/.libs/posix.so /home/travis/build/neovim/neovim/.deps/usr/lib/luarocks/rocks/luaposix/33.4.0-1/lib/posix.so
/usr/bin/install: cannot stat `ext/posix/.libs/posix.so': No such file or directory

in all tests, not just -m32 tests. Any ideas? Currently I see the following options to try:

  1. Ignore CC and LD, but check whether we are having 32-bit build and supply CFLAGS via env. Not checked at all.
  2. Do not use luaposix, select another bindings with fork. I found one option which requires binding nothing: ljsyscall, but here I have problems with luajit global state (specifically ffi.cdef calls done by ljsyscall make our cimport calls fail; I tried mocking ffi.cdef with part of cimport, but this does not work (though I found out that formatc is actually not able to handle C comments, at least not C comments from ljsyscall)).
  3. Do not use luaposix or ljsyscall, copy part of ljsyscall to Neovim (or fork it and edit).
  4. Try calling libc directly (ljsyscall does something else if I am not mistaking)
  5. Try building luaposix by means other then luarocks.
@justinmk justinmk referenced this pull request in neovim/neovim.github.io Oct 5, 2016
Open

News for people sponsoring via bountysource #141

@ZyX-I
Contributor
ZyX-I commented Nov 1, 2016

Sticking with 4 now: just use libc functions without any external rocks.

@ZyX-I
Contributor
ZyX-I commented Nov 1, 2016

tv_list_join() test failure on Mac OS has something to do with os_breakcheck: it prints OS_BREAKCHECK_START (just before calling os_breakcheck), but not OS_BREAKCHECK_END (just after).

@ZyX-I
Contributor
ZyX-I commented Nov 1, 2016

Specifically it crashes on uv_run() called in loop_poll_events() called in os_breakcheck(). So this may be a libuv bug.

@ZyX-I
Contributor
ZyX-I commented Nov 1, 2016

Checking for core dumps works and gives something like

* thread #1: tid = 0x0000, 0x00007fff862faf06 libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff862faf06 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff92d334ec libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fff96ec26df libsystem_c.dylib`abort + 129
    frame #3: 0x00000000012ba939 libnvim-test.so`uv__io_poll(loop=<unavailable>, timeout=<unavailable>) + 2137 at kqueue.c:184
    frame #4: 0x00000000012aa318 libnvim-test.so`uv_run(loop=0x00000000013417a8, mode=UV_RUN_NOWAIT) + 456 at core.c:354
    frame #5: 0x00000000010a1a24 libnvim-test.so`loop_poll_events(loop=0x00000000013417a8, ms=0) + 196 at loop.c:51
    frame #6: 0x00000000011b61e4 libnvim-test.so`os_breakcheck + 36 at input.c:150
    frame #7: 0x0000000001167985 libnvim-test.so`line_breakcheck + 101 at misc1.c:2674
    frame #8: 0x000000000109d1cd libnvim-test.so`list_join_inner(gap=0x000000000d495c88, l=0x0000000000107ab0, sep=" ", join_gap=0x00007fff5fbfe430) + 221 at typval.c:534
    frame #9: 0x000000000109d012 libnvim-test.so`tv_list_join(gap=0x000000000d495c88, l=0x0000000000107ab0, sep=" ") + 162 at typval.c:586
    frame #10: 0x0000000100005bff luajit`lj_vm_ffi_call + 132
    frame #11: 0x0000000100029979 luajit`lj_ccall_func(L=<unavailable>, cd=<unavailable>) + 2473 at lj_ccall.c:878
    frame #12: 0x000000010003fa6e luajit`lj_cf_ffi_meta___call(L=0x0000000000042378) + 78 at lib_ffi.c:229
    frame #13: 0x0000000100003cb7 luajit`lj_BC_FUNCC + 52
    frame #14: 0x00000001000155d6 luajit`lua_pcall(L=<unavailable>, nargs=<unavailable>, nresults=<unavailable>, errfunc=<unavailable>) + 182 at lj_api.c:1052
    frame #15: 0x00000001000020a9 luajit`docall(L=0x0000000000042378, narg=9, clear=0) + 89 at luajit.c:122
    frame #16: 0x00000001000018cd luajit`pmain [inlined] handle_script + 335 at luajit.c:289
    frame #17: 0x000000010000177e luajit`pmain(L=0x0000000000042378) + 1630 at luajit.c:538
    frame #18: 0x0000000100003cb7 luajit`lj_BC_FUNCC + 52
    frame #19: 0x00000001000156e8 luajit`lua_cpcall(L=<unavailable>, func=<unavailable>, ud=<unavailable>) + 40 at lj_api.c:1074
    frame #20: 0x000000010000101b luajit`main(argc=11, argv=0x00007fff5fbfe8f0) + 59 at luajit.c:566
    frame #21: 0x0000000100000fb8 luajit`_start + 230
    frame #22: 0x0000000100000ed1 luajit`start + 33
@ZyX-I
Contributor
ZyX-I commented Nov 4, 2016

Dunno why QB is failing on BSD, I do not get any core dumps (is QB using scripts for travis at all?) and I can’t reproduce the crash on a FreeBSD VM1, the only unit test failures I am getting are

Pending → /test/neovim/test/unit/os/fs_spec.lua @ 317
fs function file permissions os_fchown skipped (uv_fs_chown is no-op on Windows)

Failure → /test/neovim/test/unit/os/fs_spec.lua @ 328
fs function file permissions os_file_is_readable returns false if the file is not readable
/test/neovim/test/unit/os/fs_spec.lua:334: Expected objects to be the same.
Passed in:
(boolean) true
Expected:
(boolean) false

stack traceback:
        /test/neovim/test/unit/os/fs_spec.lua:334: in function </test/neovim/test/unit/os/fs_spec.lua:328>


Failure → /test/neovim/test/unit/os/fs_spec.lua @ 349
fs function file permissions os_file_is_writable returns 0 if the file is readonly
/test/neovim/test/unit/os/fs_spec.lua:355: Expected objects to be the same.
Passed in:
(number) 1
Expected:
(number) 0

stack traceback:
        /test/neovim/test/unit/os/fs_spec.lua:355: in function </test/neovim/test/unit/os/fs_spec.lua:349>


Failure → /test/neovim/test/unit/os/shell_spec.lua @ 54
shell functions os_system can echo some output (shell builtin)
/test/neovim/test/unit/os/shell_spec.lua:57: Expected objects to be the same.
Passed in:
(string) ''
Expected:
(string) 'some text'

stack traceback:
        /test/neovim/test/unit/os/shell_spec.lua:57: in function </test/neovim/test/unit/os/shell_spec.lua:54>


Failure → /test/neovim/test/unit/os/shell_spec.lua @ 61
shell functions os_system can deal with empty output
/test/neovim/test/unit/os/shell_spec.lua:65: Expected objects to be the same.
Passed in:
(number) -1
Expected:
(number) 0

stack traceback:
        /test/neovim/test/unit/os/shell_spec.lua:65: in function </test/neovim/test/unit/os/shell_spec.lua:61>


Failure → /test/neovim/test/unit/os/shell_spec.lua @ 68
shell functions os_system can pass input on stdin
/test/neovim/test/unit/os/shell_spec.lua:71: Expected objects to be the same.
Passed in:
(string) ''
Expected:
(string) 'some text
some other text'

stack traceback:
        /test/neovim/test/unit/os/shell_spec.lua:71: in function </test/neovim/test/unit/os/shell_spec.lua:68>


Failure → /test/neovim/test/unit/os/shell_spec.lua @ 75
shell functions os_system returns non-zero exit code
/test/neovim/test/unit/os/shell_spec.lua:77: Expected objects to be the same.
Passed in:
(number) -1
Expected:
(number) 2

stack traceback:
        /test/neovim/test/unit/os/shell_spec.lua:77: in function </test/neovim/test/unit/os/shell_spec.lua:75>

@jszakmeister ?

1 FreeBSD zyx-freebsd-vm 11.0-RELEASE-p1 FreeBSD 11.0-RELEASE-p1 #0 r306420: Thu Sep 29 01:43:23 UTC 2016 root@releng2.nyi.freebsd.org:/usr/obj/usr/src/sys/GENERIC amd64

@jszakmeister
Member

Dunno why QB is failing on BSD, I do not get any core dumps (is QB using scripts for travis at all?)

No, QB is not using the travis scripts--I don't want to run everything that Travis is because I wanted a quicker feedback cycle, and it was too hard to get good log messages about what was failing and get a decent report at the same time.

It looks like both FreeBSD and OS X failed because of this unit test on rev 1204c9c:

04:22:36,943 INFO  - not ok 143 - typval.c list join() works
04:22:36,943 INFO  - # ./test/unit/helpers.lua @ 357
04:22:36,943 INFO  - # Failure message: ./test/unit/helpers.lua:379: Expected objects to be the same.
04:22:36,943 INFO  - # Passed in:
04:22:36,943 INFO  - # (number) 0
04:22:36,943 INFO  - # Expected:
04:22:36,943 INFO  - # (number) 2
04:22:36,943 INFO  - # stack traceback:
04:22:36,943 INFO  - #     ./test/unit/helpers.lua:379: in function <./test/unit/helpers.lua:357>
@jszakmeister
Member

BTW, here's uname -a for the FreeBSD version the QB build agent is running:
FreeBSD neovim-qb-slave-freebsd-10-64bit 10.1-RELEASE-p26 FreeBSD 10.1-RELEASE-p26 #0: Wed Jan 13 20:59:29 UTC 2016 root@amd64-builder.daemonology.net:/usr/obj/usr/src/sys/GENERIC amd6

@ZyX-I
Contributor
ZyX-I commented Nov 4, 2016

@jszakmeister By “dunno” I did not mean I do not know the exact test, I mean that I do not know where it crashes. I think that it crashes in the same place OS X does, but QB scripts do not give any core dumps to prove this.

@ZyX-I
Contributor
ZyX-I commented Nov 5, 2016

It appears that our tests are more crashy then somebody thought: in addition to Mac OS functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode crashing somewhere (current code does not show backtraces on Mac OS X) linux builds also crash:

[ RUN      ] ...eovim/neovim/test/functional/terminal/altscreen_spec.lua @ 144: terminal altscreen after height is decreased by 2 and after exit restore buffer state
[       OK ] ...eovim/neovim/test/functional/terminal/altscreen_spec.lua @ 144: terminal altscreen after height is decreased by 2 and after exit restore buffer state (11.88 ms)
[----------] 6 tests from /home/travis/build/neovim/neovim/test/functional/terminal/altscreen_spec.lua (721.92 ms total)
[----------] Running tests from /home/travis/build/neovim/neovim/test/functional/terminal/api_spec.lua
[ RUN      ] ...uild/neovim/neovim/test/functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode
[       OK ] ...uild/neovim/neovim/test/functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode (76.91 ms)
[----------] 1 test from /home/travis/build/neovim/neovim/test/functional/terminal/api_spec.lua (136.82 ms total)
[----------] Running tests from /home/travis/build/neovim/neovim/test/functional/terminal/buffer_spec.lua
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 34: terminal buffer when a new file is edited will hide the buffer, ignoring the bufhidden option
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 34: terminal buffer when a new file is edited will hide the buffer, ignoring the bufhidden option (2.18 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 62: terminal buffer swap and undo does not create swap files
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 62: terminal buffer swap and undo does not create swap files (0.49 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 67: terminal buffer swap and undo does not create undofiles files
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 67: terminal buffer swap and undo does not create undofiles files (0.98 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 73: terminal buffer cannot be modified directly
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 73: terminal buffer cannot be modified directly (11.56 ms)
[New LWP 14261]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/home/travis/build/neovim/neovim/build/bin/nvim -u NONE -i NONE --cmd set noswa'.
Program terminated with signal 6, Aborted.
#0  0x00002b62f4d9c035 in __GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
Thread 1 (Thread 0x2b62f404fa00 (LWP 14261)):
#0  0x00002b62f4d9c035 in __GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
        resultvar = 0
        pid = <optimized out>
        selftid = 14261
#1  0x00002b62f4d9f79b in __GI_abort () at abort.c:91
        save_stage = 2
        act = {__sigaction_handler = {sa_handler = 0, sa_sigaction = 0}, sa_mask = {__val = {6, 47704014507096, 47704000749216, 550380534, 47704000574046, 0, 54, 4294967295, 4294967295, 47704014487968, 9910464, 9683008, 0, 4835703278458516699, 4294967295, 0}}, sa_flags = -201165004, sa_restorer = 0x2b6200000005}
        sigs = {__val = {32, 0 <repeats 15 times>}}
#2  0x000000000065ab87 in uv_mutex_lock (mutex=<optimized out>) at /home/travis/nvim-deps/build/src/libuv/src/unix/thread.c:154
No locals.
#3  0x00000000004b0daf in loop_schedule ()
No symbol table info available.
#4  0x0000000000638958 in ui_schedule_refresh ()
No symbol table info available.
#5  0x0000000000638de9 in ui_detach_impl ()
No symbol table info available.
#6  0x000000000044607c in remote_ui_disconnect ()
No symbol table info available.
#7  0x0000000000557882 in free_channel ()
No symbol table info available.
#8  0x0000000000557fe9 in decref ()
No symbol table info available.
#9  0x00000000005579b9 in close_cb ()
No symbol table info available.
#10 0x00000000004b4459 in close_cb ()
No symbol table info available.
#11 0x00000000006504c4 in uv__finish_close (handle=<optimized out>) at /home/travis/nvim-deps/build/src/libuv/src/unix/core.c:272
No locals.
#12 uv__run_closing_handles (loop=<optimized out>) at /home/travis/nvim-deps/build/src/libuv/src/unix/core.c:286
        p = <optimized out>
        q = <optimized out>
#13 uv_run (loop=0x9738c0, mode=UV_RUN_DEFAULT) at /home/travis/nvim-deps/build/src/libuv/src/unix/core.c:356
        timeout = <optimized out>
        r = <optimized out>
#14 0x00000000004b0ecb in loop_close ()
No symbol table info available.
#15 0x000000000052281f in event_teardown ()
No symbol table info available.
#16 0x00000000005904ae in mch_exit ()
No symbol table info available.
#17 0x00000000005233d3 in getout ()
No symbol table info available.
#18 0x00000000004d5ca5 in ex_quit_all ()
No symbol table info available.
#19 0x00000000004cda34 in do_one_cmd ()
No symbol table info available.
#20 0x00000000004ca5c0 in do_cmdline ()
No symbol table info available.
#21 0x00000000004c9c2a in do_cmdline_cmd ()
No symbol table info available.
#22 0x000000000044845a in nvim_command ()
No symbol table info available.
#23 0x00000000004387fa in handle_nvim_command ()
No symbol table info available.
#24 0x0000000000556f04 in on_request_event ()
No symbol table info available.
#25 0x00000000004b139b in multiqueue_process_events ()
No symbol table info available.
#26 0x00000000004639dc in insert_handle_key ()
No symbol table info available.
#27 0x0000000000463105 in insert_execute ()
No symbol table info available.
#28 0x00000000006164eb in state_enter ()
No symbol table info available.
#29 0x00000000004626ff in insert_enter ()
No symbol table info available.
#30 0x00000000004641ff in edit ()
No symbol table info available.
#31 0x000000000056abd9 in invoke_edit ()
No symbol table info available.
#32 0x000000000056ab6f in nv_edit ()
No symbol table info available.
#33 0x000000000055d1d2 in normal_execute ()
No symbol table info available.
#34 0x00000000006164eb in state_enter ()
No symbol table info available.
#35 0x000000000055b4fa in normal_enter ()
No symbol table info available.
#36 0x000000000052313c in main ()
No symbol table info available.
=============================== Core file ./core ===============================
Tests covered by this check: 6
./test/helpers.lua:138: assertion failed!
stack traceback:
    ./test/helpers.lua:138: in function 'check_cores'
    ./test/functional/helpers.lua:544: in function <./test/functional/helpers.lua:542>

and

[ RUN      ] ...eovim/neovim/test/functional/terminal/altscreen_spec.lua @ 125: terminal altscreen after height is decreased by 2 removes 2 lines from the bottom of the visible buffer
[       OK ] ...eovim/neovim/test/functional/terminal/altscreen_spec.lua @ 125: terminal altscreen after height is decreased by 2 removes 2 lines from the bottom of the visible buffer (26.04 ms)
[ RUN      ] ...eovim/neovim/test/functional/terminal/altscreen_spec.lua @ 144: terminal altscreen after height is decreased by 2 and after exit restore buffer state
[       OK ] ...eovim/neovim/test/functional/terminal/altscreen_spec.lua @ 144: terminal altscreen after height is decreased by 2 and after exit restore buffer state (10.82 ms)
[----------] 6 tests from /home/travis/build/neovim/neovim/test/functional/terminal/altscreen_spec.lua (539.08 ms total)
[----------] Running tests from /home/travis/build/neovim/neovim/test/functional/terminal/api_spec.lua
[ RUN      ] ...uild/neovim/neovim/test/functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode
[       OK ] ...uild/neovim/neovim/test/functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode (67.60 ms)
[----------] 1 test from /home/travis/build/neovim/neovim/test/functional/terminal/api_spec.lua (91.06 ms total)
[----------] Running tests from /home/travis/build/neovim/neovim/test/functional/terminal/buffer_spec.lua
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 34: terminal buffer when a new file is edited will hide the buffer, ignoring the bufhidden option
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 34: terminal buffer when a new file is edited will hide the buffer, ignoring the bufhidden option (2.22 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 62: terminal buffer swap and undo does not create swap files
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 62: terminal buffer swap and undo does not create swap files (0.39 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 67: terminal buffer swap and undo does not create undofiles files
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 67: terminal buffer swap and undo does not create undofiles files (0.38 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 73: terminal buffer cannot be modified directly
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 73: terminal buffer cannot be modified directly (11.52 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 86: terminal buffer sends data to the terminal when the "put" operator is used
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 86: terminal buffer sends data to the terminal when the "put" operator is used (23.29 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 112: terminal buffer sends data to the terminal when the ":put" command is used
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 112: terminal buffer sends data to the terminal when the ":put" command is used (24.33 ms)
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 138: terminal buffer can be deleted
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 138: terminal buffer can be deleted (10.26 ms)
[New LWP 32195]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/home/travis/build/neovim/neovim/build/bin/nvim -u NONE -i NONE --cmd set noswa'.
Program terminated with signal 6, Aborted.
#0  0x55577430 in __kernel_vsyscall ()
Thread 1 (Thread 0x5578a2c0 (LWP 32195)):
#0  0x55577430 in __kernel_vsyscall ()
No symbol table info available.
#1  0x5560fe9f in raise () from /lib32/libc.so.6
No symbol table info available.
#2  0x556133f5 in abort () from /lib32/libc.so.6
No symbol table info available.
#3  0x08260869 in uv_mutex_lock (mutex=0x8358124) at /home/travis/nvim-deps/build/src/libuv/src/unix/thread.c:154
No locals.
#4  0x080e1cea in loop_schedule ()
No symbol table info available.
#5  0x082400b9 in ui_schedule_refresh ()
No symbol table info available.
#6  0x08240489 in ui_detach_impl ()
No symbol table info available.
#7  0x080830b9 in remote_ui_disconnect ()
No symbol table info available.
#8  0x08176bfe in free_channel ()
No symbol table info available.
#9  0x081772be in decref ()
No symbol table info available.
#10 0x08176d22 in close_cb ()
No symbol table info available.
#11 0x080e48c9 in close_cb ()
No symbol table info available.
#12 0x082551a1 in uv__finish_close (handle=0x55a17420) at /home/travis/nvim-deps/build/src/libuv/src/unix/core.c:272
No locals.
#13 uv__run_closing_handles (loop=<optimized out>) at /home/travis/nvim-deps/build/src/libuv/src/unix/core.c:286
        p = 0x55a17420
        q = 0x55a17220
#14 uv_run (loop=0x8357e00, mode=UV_RUN_DEFAULT) at /home/travis/nvim-deps/build/src/libuv/src/unix/core.c:356
        timeout = <optimized out>
        r = <optimized out>
#15 0x080e1dfa in loop_close ()
No symbol table info available.
#16 0x08147ea0 in event_teardown ()
No symbol table info available.
#17 0x081a9f1c in mch_exit ()
No symbol table info available.
#18 0x081489c9 in getout ()
No symbol table info available.
#19 0x08102109 in ex_quit_all ()
No symbol table info available.
#20 0x080fafb7 in do_one_cmd ()
No symbol table info available.
#21 0x080f824e in do_cmdline ()
No symbol table info available.
#22 0x080f7946 in do_cmdline_cmd ()
No symbol table info available.
#23 0x0808507b in nvim_command ()
No symbol table info available.
#24 0x080773d5 in handle_nvim_command ()
No symbol table info available.
#25 0x0817635d in on_request_event ()
No symbol table info available.
#26 0x080e2223 in multiqueue_process_events ()
No symbol table info available.
#27 0x0809d14e in insert_handle_key ()
No symbol table info available.
#28 0x0809c859 in insert_execute ()
No symbol table info available.
#29 0x082229c8 in state_enter ()
No symbol table info available.
#30 0x0809bf07 in insert_enter ()
No symbol table info available.
#31 0x0809d966 in edit ()
No symbol table info available.
#32 0x081886b4 in invoke_edit ()
No symbol table info available.
#33 0x08188658 in nv_edit ()
No symbol table info available.
#34 0x0817bd59 in normal_execute ()
No symbol table info available.
#35 0x082229c8 in state_enter ()
No symbol table info available.
#36 0x0817a378 in normal_enter ()
No symbol table info available.
#37 0x08148794 in main ()
No symbol table info available.
=============================== Core file ./core ===============================
Tests covered by this check: 10
./test/helpers.lua:138: assertion failed!
stack traceback:
    ./test/helpers.lua:138: in function 'check_cores'
    ./test/functional/helpers.lua:544: in function <./test/functional/helpers.lua:542>

(these are third and fourth travis builds). I think I need to move commits which check core dumps to a separate PR.

@justinmk
Member
justinmk commented Nov 6, 2016

uv_mutex_init() sets PTHREAD_MUTEX_ERRORCHECK, seems we're acquiring the same mutex more than once on the same thread. Or something else: loop_close() destroys the mutex, then during ui_detach we're trying to schedule a refresh and acquiring the mutex. During ui_detach there's not much reason to refresh the UI if we are tearing down.

I think I need to move commits which check core dumps to a separate PR.

Sure. Want me to cherry pick that and see if I can fix the (linux) crashes?

@ZyX-I
Contributor
ZyX-I commented Nov 6, 2016

@justinmk This would be better idea, I am not quite familiar with our async stuff. I now got something more useful from OS X build:

[ RUN      ] ...uild/neovim/neovim/test/functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode
[       OK ] ...uild/neovim/neovim/test/functional/terminal/api_spec.lua @ 21: api qa! RPC request during insert-mode (121.54 ms)
========================= Core file /cores/core.44169 =========================
./test/helpers.lua:135: bad argument #1 to 'write' (string expected, got nil)
stack traceback:
    ./test/helpers.lua:135: in function 'check_cores'
    ./test/functional/helpers.lua:544: in function <./test/functional/helpers.lua:542>
[----------] 1 test from /Users/travis/build/neovim/neovim/test/functional/terminal/api_spec.lua (16230.57 ms total)
[----------] Running tests from /Users/travis/build/neovim/neovim/test/functional/terminal/buffer_spec.lua
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 34: terminal buffer when a new file is edited will hide the buffer, ignoring the bufhidden option
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 34: terminal buffer when a new file is edited will hide the buffer, ignoring the bufhidden option (9.66 ms)
========================= Core file /cores/core.44169 =========================
./test/helpers.lua:135: bad argument #1 to 'write' (string expected, got nil)
stack traceback:
    ./test/helpers.lua:135: in function 'check_cores'
    ./test/functional/helpers.lua:544: in function <./test/functional/helpers.lua:542>
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 62: terminal buffer swap and undo does not create swap files
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 62: terminal buffer swap and undo does not create swap files (3.79 ms)
========================= Core file /cores/core.44169 =========================
./test/helpers.lua:135: bad argument #1 to 'write' (string expected, got nil)
stack traceback:
    ./test/helpers.lua:135: in function 'check_cores'
    ./test/functional/helpers.lua:544: in function <./test/functional/helpers.lua:542>
[ RUN      ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 67: terminal buffer swap and undo does not create undofiles files
[       OK ] ...d/neovim/neovim/test/functional/terminal/buffer_spec.lua @ 67: terminal buffer swap and undo does not create undofiles files (3.29 ms)
========================= Core file /cores/core.44169 =========================
(lldb) target create "build/bin/nvim" --core "/cores/core.44169"
warning: (x86_64) /cores/core.44169 load command 83 LC_SEGMENT_64 has a fileoff + filesize (0x27728000) that extends beyond the end of the file (0x27727000), the segment will be truncated to match
warning: (x86_64) /cores/core.44169 load command 84 LC_SEGMENT_64 has a fileoff (0x27728000) that extends beyond the end of the file (0x27727000), ignoring this section
Core file '/cores/core.44169' (x86_64) was loaded.
(lldb) bt all
warning: could not load any Objective-C class information. This will significantly reduce the quality of type information available.
* thread #1: tid = 0x0000, 0x00007fff89e6af06 libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff89e6af06 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff968a34ec libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fff9aa326df libsystem_c.dylib`abort + 129
    frame #3: 0x0000000102584434 nvim`uv_mutex_lock(mutex=<unavailable>) + 20 at thread.c:154
    frame #4: 0x000000010237ccf4 nvim`loop_schedule(loop=0x0000000102644468, event=Event @ 0x00007fff5d91fe60) + 36 at loop.c:62
    frame #5: 0x000000010255a456 nvim`ui_schedule_refresh + 150 at ui.c:187
    frame #6: 0x000000010255ae10 nvim`ui_detach_impl(ui=0x00000000028d3380) + 224 at ui.c:263
    frame #7: 0x00000001022f7f8c nvim`remote_ui_disconnect(channel_id=2) + 140 at ui.c:45
    frame #8: 0x000000010244a448 nvim`free_channel(channel=0x000000000281b000) + 24 at channel.c:643
    frame #9: 0x000000010244898f nvim`decref(channel=0x000000000281b000) + 47 at channel.c:787
    frame #10: 0x0000000102447a3c nvim`close_cb(stream=0x000000000281b030, data=0x000000000281b000) + 28 at channel.c:664
    frame #11: 0x00000001023814f5 nvim`close_cb(handle=0x000000000281b030) + 149 at stream.c:108
    frame #12: 0x0000000102578cb4 nvim`uv_run [inlined] uv__finish_close(handle=<unavailable>) + 644 at core.c:272
    frame #13: 0x0000000102578c37 nvim`uv_run [inlined] uv__run_closing_handles(loop=<unavailable>) + 55 at core.c:286
    frame #14: 0x0000000102578c00 nvim`uv_run(loop=0x0000000102644468, mode=UV_RUN_DEFAULT) + 464 at core.c:356
    frame #15: 0x000000010237ce0d nvim`loop_close(loop=0x0000000102644468, wait=true) + 141 at loop.c:87
    frame #16: 0x000000010240a69c nvim`event_teardown + 108 at main.c:168
    frame #17: 0x00000001024909b1 nvim`mch_exit(r=0) + 49 at os_unix.c:146
    frame #18: 0x000000010240df13 nvim`getout(exitval=0) + 723 at main.c:627
    frame #19: 0x00000001023af8c1 nvim`ex_quit_all(eap=0x00007fff5d920428) + 257 at ex_docmd.c:5858
    frame #20: 0x000000010239fa88 nvim`do_one_cmd(cmdlinep=0x00007fff5d920728, sourcing=1, cstack=0x00007fff5d920730, fgetline=0x0000000000000000, cookie=0x0000000000000000) + 12456 at ex_docmd.c:2196
    frame #21: 0x000000010239b701 nvim`do_cmdline(cmdline="qa!", fgetline=0x0000000000000000, cookie=0x0000000000000000, flags=11) + 2737 at ex_docmd.c:601
    frame #22: 0x000000010239c676 nvim`do_cmdline_cmd(cmd="qa!") + 38 at ex_docmd.c:273
    frame #23: 0x00000001022fb5f2 nvim`nvim_command(command=(data = "qa!", size = 3), err=0x00007fff5d920de0) + 34 at vim.c:45
    frame #24: 0x00000001022e758f nvim`handle_nvim_command(channel_id=1, args=Array @ 0x00007fff5d920d20, error=0x00007fff5d920de0) + 287 at dispatch_wrappers.generated.h:1703
    frame #25: 0x0000000102449bca nvim`on_request_event(argv=0x00007fff5d921218) + 282 at channel.c:455
    frame #26: 0x000000010237d8f2 nvim`multiqueue_process_events(this=0x0000000002818120) + 162 at multiqueue.c:143
    frame #27: 0x00000001023291f3 nvim`insert_handle_key(s=0x00007fff5d921540) + 3171 at edit.c:965
    frame #28: 0x000000010231ca21 nvim`insert_execute(state=0x00007fff5d921540, key=-25341) + 1713 at edit.c:760
    frame #29: 0x00000001025323bb nvim`state_enter(s=0x00007fff5d921540) + 331 at state.c:58
    frame #30: 0x000000010231d64e nvim`insert_enter(s=0x00007fff5d921540) + 1966 at edit.c:454
    frame #31: 0x000000010231c317 nvim`edit(cmdchar=105, startln=false, count=1) + 455 at edit.c:1326
    frame #32: 0x000000010245f5d6 nvim`invoke_edit(cap=0x00007fff5d921748, repl=0, cmd=105, startln=0) + 118 at normal.c:7517
    frame #33: 0x00000001024587d5 nvim`nv_edit(cap=0x00007fff5d921748) + 725 at normal.c:7489
    frame #34: 0x000000010245621d nvim`normal_execute(state=0x00007fff5d9216a8, key=105) + 2061 at normal.c:1143
    frame #35: 0x00000001025323bb nvim`state_enter(s=0x00007fff5d9216a8) + 331 at state.c:58
    frame #36: 0x000000010244e647 nvim`normal_enter(cmdwin=false, noexmode=false) + 167 at normal.c:463
    frame #37: 0x000000010240b211 nvim`main(argc=7, argv=0x00007fff5d921978) + 2657 at main.c:542
    frame #38: 0x00007fff8fc035ad libdyld.dylib`start + 1
    frame #39: 0x00007fff8fc035ad libdyld.dylib`start + 1
Tests covered by this check: 1
./test/helpers.lua:145: assertion failed!
stack traceback:
    ./test/helpers.lua:145: in function 'check_cores'
    ./test/functional/helpers.lua:544: in function <./test/functional/helpers.lua:542>
@justinmk
Member
justinmk commented Nov 6, 2016 edited

This should fix it:

diff --git a/src/nvim/ui.c b/src/nvim/ui.c
index eb500414a73e..a65806ac11f6 100644
--- a/src/nvim/ui.c
+++ b/src/nvim/ui.c
@@ -259,7 +259,7 @@ void ui_detach_impl(UI *ui)
     shift_index++;
   }
-
-  if (--ui_count) {
+  if (--ui_count && !exiting) {
     ui_schedule_refresh();
   }
 }

And that's probably the best way. We already have a similar check in terminal.c:refresh_timer_cb().

@justinmk
Member

@ZyX-I Cherry-picked the core dump commits to #5678. The patch above seems to fix the issue.

@justinmk justinmk changed the title from [WIP] eval: Split eval.c into smaller files to [WIP] eval: Refactor eval.c Dec 4, 2016
@justinmk justinmk referenced this pull request Dec 4, 2016
Open

[WIP] VimL to lua translator #243

78 of 134 tasks complete
@ZyX-I
Contributor
ZyX-I commented Dec 4, 2016

It appears that while dictionary watchers used to work fine before #5664, that PR did not alter dictionary watchers. Also found a (somewhat expected) bug: v:_null_dict crashes dictwatcher* functions (#4615).

@ZyX-I
Contributor
ZyX-I commented Dec 18, 2016

Version before rebasing is in branch split-eval-old. Last rebase most likely does not work because I did not yet even check whether it compiles.

@ZyX-I ZyX-I added a commit to ZyX-I/neovim that referenced this pull request Jan 3, 2017
@ZyX-I ZyX-I tests: Add tests for partials dumping
Also fixed dumping of partials by encode_vim_to_object and added code which is 
able to work with partials and dictionaries to test/unit/eval/helpers.lua 
(mostly copied from #5119, except for partials handling).
136b382
ZyX-I added some commits Jul 26, 2016
@ZyX-I ZyX-I eval: Split eval.c into smaller files c071d5b
@ZyX-I ZyX-I *: Partial string handling refactoring
Main points:

- Replace `char_u` with `char` in some cases.
- Remove `str[len] = NUL` hack in some cases when `str` may be considered
  `const`.
4f83f99
@ZyX-I ZyX-I api/buffer: Add buffer_get_changedtick method d35282c
@ZyX-I ZyX-I functests: Add tests for some *buf* functions 9b2ea05
@ZyX-I ZyX-I os/fileio: Allow certain failures during file_fsync
According to the documentation fsync() may fail with EROFS or EINVAL if “file 
descriptor is bound to a special file which does not support synchronization” 
(e.g. /dev/stderr). This condition is completely valid in this case since main 
point of `file_fsync()` is dumping buffered input.
6391390
@ZyX-I ZyX-I os/fileio: Support appending to a file ffd4657
@ZyX-I ZyX-I eval: writefile: Give more adequate IO errors and do not call putc() dbcf974
@ZyX-I ZyX-I *: Move some dictionary functions to typval.h and use char*
Also fixes buffer reusage in setmatches() and complete().
789cb90
@ZyX-I ZyX-I eval: Make setmatches() return -1 in case of some failures faa28d6
@ZyX-I ZyX-I eval: Move get_dict_callback to typval.c bc0ae56
@ZyX-I ZyX-I eval: Move dict_set_keys_readonly to typval.c 745e97d
@ZyX-I ZyX-I eval: Move dict_add_list and dict_add_dict to typval.c 71d60e6
@ZyX-I ZyX-I eval: Split and move dict_add_nr_str to typval.c
Function was split into tv_dict_add_nr() and tv_dict_add_str().
f7872a0
@ZyX-I ZyX-I eval: Move get_float_arg to typval.h
Assuming `inline` is there for a reason, so it is kept and function was moved to
typval.h and not to typval.c which does not have problems with #including
message.h.
16954a0
@ZyX-I ZyX-I message,strings: Move vim_*printf functions to strings.c
Allows eval/typval.h to #include message.h.
882baf3
@ZyX-I ZyX-I eval,*: Move get_tv_string to typval.c
Function was renamed and changed to return `const char *`.
21bd812
@ZyX-I ZyX-I eval: Move get_tv_string_buf() to eval/typval.c 19161f9
@ZyX-I ZyX-I eval: Move get_tv_lnum and get_tv_float to eval/typval.h
Additionally

- Rename former tv_get_float to tv_get_float_chk due to name conflict (former 
  get_tv_float is better suited for being tv_get_float).
- Add E907 error to get_tv_float() and test that it is being raised when 
  appropriate.
c0b6cbe
@ZyX-I ZyX-I eval: Refactor get_tv_lnum_buf bc29578
@ZyX-I ZyX-I eval: Move get_tv_number[_chk] to eval/typval.c 1fe734e
@ZyX-I ZyX-I eval: Move free_tv to eval/typval.h, remove most of its usages 5d7c874
@ZyX-I ZyX-I fixup! eval: Move free_tv to eval/typval.h, remove most of its usages d2e7e03
@ZyX-I ZyX-I eval: Move remaining get_tv_string* functions to eval/typval.c 0497ea5
@ZyX-I ZyX-I eval: Remove eval_expr() completely f10c52d
@ZyX-I ZyX-I eval: Make sort always stable
Should fix test failures on QB:

    20:00:51,837 INFO  - not ok 420 - sort() sorts “wrong” values between -0.0001 and 0.0001, preserving order
    20:00:51,837 INFO  - # test/functional/eval/sort_spec.lua @ 21
    20:00:51,837 INFO  - # Failure message: test/functional/eval/sort_spec.lua:39: Expected objects to be the same.
    20:00:51,837 INFO  - # Passed in:
    20:00:51,837 INFO  - # (string) '[-1.0e-4, v:true, v:false, v:null, function('tr'), {'a': 42}, 'check', [], 1.0e-4]'
    20:00:51,837 INFO  - # Expected:
    20:00:51,837 INFO  - # (string) '[-1.0e-4, function('tr'), v:true, v:false, v:null, [], {'a': 42}, 'check', 1.0e-4]'
    20:00:51,837 INFO  - # stack traceback:
    20:00:51,837 INFO  - #     test/functional/eval/sort_spec.lua:39: in function <test/functional/eval/sort_spec.lua:22>
    20:00:51,837 INFO  - #
0090964
@ZyX-I ZyX-I misc1: Refactor ask_yesno() dc93082
@ZyX-I ZyX-I *: Silence linter a27e797
@ZyX-I ZyX-I typval.h: Allow non-var expressions in TV_DICT_ITER first argument 0018fe9
@ZyX-I ZyX-I functests: Fix test linter errors 530d9ed
@ZyX-I ZyX-I eval: Fix max_min functions
Found two bugs:

1. Multiple unneeded error messages, vim/vim#1039.
2. Unformatted error string, vim/vim#1040.
d15d03f
@ZyX-I ZyX-I functests: Remove unused variable 48fbaec
@ZyX-I ZyX-I ci: Check that `#include "*.h"` works as a single include
This is not as good as include-what-you-use, but, at least, guarantees that
header file did not forget to include something, at least through some other
included file.
1cb383c
@ZyX-I ZyX-I cmake: Use CMAKE_CURRENT_LIST_DIR and remove vars used only once cef469e
@ZyX-I ZyX-I message: Revise maxlen argument in msg_puts_attr_len
`attr` argument is enough to forbid putting message in history. Also forbid 
strings with NUL before `len` just in case (it appears that this does not ever 
happen).
efd0fd5
@ZyX-I ZyX-I message: Remove incorrect assertion
It was there only to prove that *now* `len` argument is not used to forbid 
putting message into history (i.e. that Neovim behaviour did not change). After 
this commit it should be possible to use msg_puts_attr_len with len>=0 to put 
message into history should new code need this.
dfeceb1
@ZyX-I ZyX-I ci: Fail single include tests faster, produce logs
FIXME: Remove `set -x`/`set +x` and `-DCMAKE_VERBOSE_MAKEFILE=on`. Other changes 
will not be touched.
ae05157
@ZyX-I ZyX-I ci: Disable verbose logging fa8553f
@ZyX-I ZyX-I ci: State that executables depend on all headers
Easier then determining which headers each executable depends on given that 
headers tend to include headers.
fee5901
@ZyX-I ZyX-I mbyte: Include winnt.h
According to [MSDN][1] it is defined in `WinNT.h` header file. Since
windows has case-insensitive FS and mingw has `winnt.h` header file it
is included all-lower.

[1]: https://msdn.microsoft.com/en-us//library/windows/desktop/aa383751(v=vs.85).aspx
4ffdae2
@ZyX-I ZyX-I eval: Move copy_tv to eval/typval 6a4ecf5
@ZyX-I ZyX-I eval/typval: Add missing includes, also add a script to find them
Contains unfinished attempt to integrate IWYU (ref #549). To finish it different 
job should be done, specifically:

- Instead of feeding IWYU with modified file a mirror source tree should be 
  created with the help of CMake which will contain modified sources. This 
  solves the problem with IWYU thinking that `*.generated.h` headers should be 
  included in place of `*` headers.
- Build IWYU as all other third-party utilities.
- Make modified sources avoid problems with `nvim/func_attr.h` includes and 
  various related tricks.

Current script may only be used for manual checks like this:

    ./scripts/check-includes.py \
        --generated-includes-dir build/include \
        --generated-includes-dir build/src/nvim/auto \
        --file src/nvim/eval/typval.c \
        -- -Isrc -Ibuild/include -Ibuild/src/nvim/auto \
           -DINCLUDE_GENERATED_DECLARATIONS

(it is also somewhat fine with `--file src/nvim/eval/typval.h`). I have no idea 
why (I mean, why developer think that these lines are needed, why they are 
suggested is pretty obvious: because there is typedef which mentions them before 
structs are defined), but for typval.h it reports, among other things, that it 
should add lines

    struct dictvar_S;
    struct listitem_S;
    struct listvar_S;
    struct listwatch_S;
7e7e232
@ZyX-I ZyX-I fixup! ci: Check that `#include "*.h"` works as a single include f0c24fd
@ZyX-I ZyX-I functests: Test tv_list_item_\* functions
To check that memory is free()d correctly also adds support for mocking calls to 
allocator.
b5c47be
@ZyX-I ZyX-I unittests: Add tests for list watchers and list alloc/free/unref 9ef11f5
@ZyX-I ZyX-I unittests: Add tests for tv_list_insert() 3acfb03
@ZyX-I ZyX-I mbyte: Include `windows.h` in place of `winnt.h` 36c23e7
@ZyX-I ZyX-I unittests: Add tests for tv_list_insert*()/…append*() functions e4f7ff0
@ZyX-I ZyX-I unittests: Test tv_list_copy
Also found some bugs:

1. var_item_copy() always fails to copy v:_null_list and v:_null_dict. Fixing 
   this should mean fixing `deepcopy(v:_null_list)` which should’ve been, but 
   was not listed in #4615. This also fixes `deepcopy(v:_null_dict)`.
2. var_item_copy() crashes when trying to copy NULL string with `conv != NULL`.
3. `conv` argument is ignored when copying list unless `deep` is true, but it 
   was not reflected in documentation.
4. `tv_dict_item_alloc_len()` allocated more memory then needed.
5. typvalt2lua was not able to handle self-referencing containers.
6633740
@ZyX-I ZyX-I functests: Add null_spec.lua from #4615
For now it is full of FIXMEs and tests for incorrect behaviour. Sorted out to
have FIXMEs in one place, commented crashing tests in other and correctly
working tests in the third one.
7bc7456
@ZyX-I ZyX-I eval/typval: Make tv_list_concat handle NULL lists correctly
Fixes some FIXMEs in eval/null_spec.lua.
679856c
@ZyX-I ZyX-I unittest: Test tv_list_concat() 2c9d7bf
@ZyX-I ZyX-I Fix warning: helpers.c: Dead store x2
Problem    : Dead store at lines 371 and 412.
Diagnostic : Harmless issue.
Rationale  : Just needed something with Object type for use in sizeof().
Resolution : Use struct constant instead.

Based on:
- https://neovim.io/doc/reports/clang/report-b47dcc.html
- https://neovim.io/doc/reports/clang/report-b2cfd4.html
2d63907
@ZyX-I ZyX-I eval: Fix linter errors 299e612
@ZyX-I ZyX-I eval/typval,tests: Fix extending list with itself, add tests
Adds unit test for tv_list_extend and regression test for extend() VimL 
function.
a75d316
@ZyX-I ZyX-I unit/eval/helpers: Do not reuse one and the same typvalt in tests 21f3b2b
@ZyX-I ZyX-I unittests: Fix tests crash
Tests crash at some point without
- `after_each(collectgarbage)` right before “typval.c list copy() copies list
  correctly and converts items” test.
- Commenting out that test.
- Adding `collectgarbage()` after the test (what actually this commit does).

Adding `collectgarbage()` to top-level `after_each` block right after
`restore_allocators` makes running this file crash even if it is run alone.
cdb9194
@ZyX-I ZyX-I unittests: Fix luacheck warnings 11ac012
@ZyX-I ZyX-I unittest: Test tv_list_join() 12db181
@ZyX-I ZyX-I unittest: Run tests in a separate process f29cb7b
@ZyX-I ZyX-I eval/typval: Add tv_list_equal() tests, compare NULL lists equal 620c90b
@ZyX-I ZyX-I ci: Fix check-single-includes test failure bd4ee0e
@ZyX-I ZyX-I unittests: Pause garbage collector while executing tests
Temporary (?) workaround for currently failing check_alloc_log tests.
6f4d76e
@ZyX-I ZyX-I ci: Make luaposix dependency look like mpack dependency 2f66a36
@ZyX-I ZyX-I ci: Supply LUAROCKS_BUILDARGS also via cmake -E env
Keeps ${LUAROCKS_BUILDARGS} as an argument to luarocks just in case.
67a5fed
@ZyX-I ZyX-I ci: Use just `env`, not `cmake -E env` 642e03d
@ZyX-I ZyX-I ci: Use libraries directory, not binaries directory e45909d
@ZyX-I ZyX-I ci: Try using ${HOSTDEPS_BIN_DIR}/luaposix because it used to work 047f835
@ZyX-I ZyX-I unittest: Try using syscall library instead (ffi-based) b589c8f
@ZyX-I ZyX-I unittest: Do not use syscall library, it does not work well with cimport 4d56499
@ZyX-I ZyX-I unittest: Use own bindings to libc syscall wrappers f8e69f0
@ZyX-I ZyX-I unittest: Log syscalls if requested 36e4316
@ZyX-I ZyX-I ci: Try checking for core dumps
Note: can’t use `dbg_cmd` string because I need arguments with spaces (i.e. `bt 
all` and `thread apply all bt full`).
e1ab29b
@ZyX-I ZyX-I unittest: Allow failing test to fail 44bf76b
@ZyX-I ZyX-I unittest: Add tests for tv_list_find*() functions
Additional modifications:

- More `const` qualifiers in tested functions.
- `tv_list_find_str()` second argument is more in-line with other 
  `tv_list_find*()` functions.
5506348
@ZyX-I ZyX-I unittests: Fix lint errors 0463f86
@ZyX-I ZyX-I unittest: Do not run failing test at all 0610e28
@ZyX-I ZyX-I unittest: Add tests for tv_list_idx_of_item 2eb592c
@ZyX-I ZyX-I eval/typval: More `const` qualifiers in `tv_dict*` function signatures e2fce6f
@ZyX-I ZyX-I mbyte: Include win_defs.h in place of windows.h
Rationale: win_defs.h deals with issues like `RGB()` macro redefined in
`macros.h`.
3d9343c
@ZyX-I ZyX-I *: Fix some Windows-specific warnings
Also fixed an error in path_fnamecmp().
3cad1e0
@ZyX-I ZyX-I functests: Fix buf_functions test on Windows 980e8fa
@ZyX-I ZyX-I ci: Better core dump checking
- Do not exclude any directories from `find` search, remove dumps before tests
  instead.
- Install `apport` on travis so that linux tests should produce core dumps
  (based on information from travis-ci/travis-ci#3754, not sure whether it still
  applies).
- Check cores in lua so that one has an idea which test is failing exactly. Do
  this only 10% of time on linux because traversing the file system is slow.

Unit tests are still not touched, though it is what `app` argument in
`check_cores` is for.

TODO? consider using `find`, it may be faster. Consider retiring `os.execute`,
      dealing with escaping is bad.
ac21445
@ZyX-I ZyX-I ci: Disable single_includes test
It has a habit of making build hang.
2ae498f
@ZyX-I ZyX-I tests: When checking for cores, ignore paths like core/exit_spec.lua ea5d2da
@ZyX-I ZyX-I eval: Move part of dictwatcher* functions to eval/typval 6645b44
@ZyX-I ZyX-I eval: Make dictionary watchers work with empty keys
Looks like dict_notifications_spec test used to depend on some state which 
should not be preserved. Changed all `setup()` calls to `before_each()` and 
added necessary state in addition to changes required to test empty keys.

Note: unit tests for tv_dict_watcher* are still needed.
f1da95d
@ZyX-I ZyX-I eval: Make sure that v:_null_dict does not crash dictwatcher*()
Ref #4615
8ec5e41
@ZyX-I
Contributor
ZyX-I commented Jan 11, 2017

Rebased the thing. Rebase is known to compile and pass tests locally until some point (check eval-clear-rebase-… tags in my repository), but not at the latest commit.

@justinmk
Member
justinmk commented Jan 11, 2017 edited

@ZyX-I Was there some change that made a69bc49 unnecessary (though it is still a valid change)? (re #5678, this was to fix this)

@ZyX-I
Contributor
ZyX-I commented Jan 11, 2017

@justinmk I did no additional changes, just rebased. And I know that rebase still will not compile because offending changes to C files were not in early commits where changes caused merge conflicts (thus they did not receive much attention).

ZyX-I added some commits Jan 13, 2017
@ZyX-I ZyX-I eval: Fix compilation
Tests still don’t pass.
39b8eaf
@ZyX-I ZyX-I eval/typval: Do not forget to free callback in tv_dict_watcher_remove 69c3f2b
@ZyX-I ZyX-I eval/typval: Fix copy-paste typo in tv_get_number_chk
That resulted in a failing minmax tests.
8f8480e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment