From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A622FD31FD8; Wed, 17 Apr 2024 19:04:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A622FD31FD8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1713369876; bh=PKRfydQZ4O8eL4jwqknFSPY9jyfimEcFKCTJF4/onDk=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=OLxAG0ZGj5h3T/XZMZhJRieNu7gVOrvMV2ts+BQiYeDIr8QdsX/UhM/OjNpIev/bD Bo1On5s3hsESw2RgVvZWJsLLsOVLQ6JtUWBa4l2N512dKLTvh6OvOQTeUnsBk+fK6K UGaxq2U5WDe9CrQ31XIvpBvN+qOTySBnh8dGENZs= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [95.163.41.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 417FFD31FD8 for ; Wed, 17 Apr 2024 19:04:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 417FFD31FD8 Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1rx7lv-0000000FJRJ-14wK; Wed, 17 Apr 2024 19:04:31 +0300 Date: Wed, 17 Apr 2024 19:00:27 +0300 To: Maxim Kokryashkin Message-ID: References: <121161805fff1e9b5855f1092bc16c24af3da3de.1712182830.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <121161805fff1e9b5855f1092bc16c24af3da3de.1712182830.git.m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9F97E3C14763C38E26C4CC23159DC9624CBDEDF1F545A2F39182A05F53808504002E5B43F9C491B31411046492FDDF80698F4BCCFA1BA160F66B37BDEEB48F18EF5EC2512189958EF X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75263010198C72082EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637222645A8BD7B05AE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88F17E5CC22C15950D72A948B813C68A82825D1350840C15DCC7F00164DA146DAFE8445B8C89999728AA50765F7900637DCE3DBD6F8E38AFD389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8A9FF340AA05FB58CF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD182315B90ADEC130413A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735BA6625F88748EAEFC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5099766F5B3BA92485002B1117B3ED696E9CFF4904AD79C9FAD0703CEB2EF9A27823CB91A9FED034534781492E4B8EEADB05233B9BC4759D3 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF1E8F2155574C282E36DA8B67E62EBA5D19B4AA27009C52E59C68DF19BBC139C7007A74C46802367A4AA6A4DA8F63241639C73D0586FB70F34CDF12D34D0200EBE077B6D84AEEDD0F5F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKLjB1EikxboBTTNwqv/80w== X-DA7885C5: 7566DB981FFCB5E1F255D290C0D534F9AC614BAC601AA91A17BD3F343DD8DA608D678A27CAC5FE575B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE33B6394EF90A9A9E7FFB150D6F69403AFC41245D32A1145799E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v6 2/2] test: add tests for debugging extensions X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the patch! Please consider my comments below. On 04.04.24, Maxim Kokryashkin wrote: > From: Maksim Kokryashkin > > This patch adds tests for LuaJIT debugging > extensions for lldb and gdb. > --- > .flake8rc | 4 + > test/CMakeLists.txt | 1 + > .../CMakeLists.txt | 80 ++++++ > .../debug-extension-tests.py | 250 ++++++++++++++++++ > 4 files changed, 335 insertions(+) > create mode 100644 test/LuaJIT-debug-extensions-tests/CMakeLists.txt > create mode 100644 test/LuaJIT-debug-extensions-tests/debug-extension-tests.py I see no updates of the CI actions for these tests, so tests are skipped if the CI runner has no installed python|lldb|gdb. > > diff --git a/.flake8rc b/.flake8rc > index 13e6178f..6766ed41 100644 > --- a/.flake8rc > +++ b/.flake8rc > @@ -3,3 +3,7 @@ extend-ignore = > # XXX: Suppress F821, since we have autogenerated names for > # 'ptr' type complements in luajit_lldb.py. > F821 > +per-file-ignores = > + # XXX: Flake considers regexp special characters to be > + # escape sequences. > + test/LuaJIT-debug-extensions-tests/debug-extension-tests.py:W605 Do we need this ignore? IINM, we can just use the `r` prefix for all regex strings that produce the warning. See [1]. > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 19726f5a..a3b48939 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -148,6 +148,7 @@ add_subdirectory(PUC-Rio-Lua-5.1-tests) > add_subdirectory(lua-Harness-tests) > add_subdirectory(tarantool-c-tests) > add_subdirectory(tarantool-tests) > +add_subdirectory(LuaJIT-debug-extensions-tests) I suggest renaming like "LuaJIT-debug-extensions-tests" -> "tarantool-debugger-tests" to be consistent with other of our tests (tarantool-tests, tarantool-c-tests) since the debugger itself is a part of our test suite, not LuaJIT's. Also, please notice that entries are alphabetically sorted. > > # Each testsuite has its own CMake target, but combining these > # target into a single one is not desired, because each target > diff --git a/test/LuaJIT-debug-extensions-tests/CMakeLists.txt b/test/LuaJIT-debug-extensions-tests/CMakeLists.txt > new file mode 100644 > index 00000000..9ac626ec > --- /dev/null > +++ b/test/LuaJIT-debug-extensions-tests/CMakeLists.txt > @@ -0,0 +1,80 @@ > +SET(TEST_SUITE_NAME "LuaJIT-dbg-extension-tests") > +add_test_suite_target(LuaJIT-dbg-extension-tests > + LABELS ${TEST_SUITE_NAME} > + DEPENDS ${LUAJIT_TEST_BINARY} > +) > + > +# Debug info is required for testing of extensions. Strictly saying we can have debug symbols for the "RelWithDebInfo" build too. OTOH, values in registers return instead of the desired output for the build, so skipping the test for non-Debug builds is not a bad idea after all (at least until we noticed some regression related to this build). Adjust a comment if you still don't want to test the RelWithDebInfo build. > +if(NOT (CMAKE_BUILD_TYPE MATCHES Debug)) > + message(WARNING > + "not a DEBUG build, LuaJIT-lldb-extension-tests and " I see a lot of usage of these names below. It will be better to set variables for them. Also, the names were changed after the previous review iteration, so please update the names to be up-to-date. > + "LuaJIT-gdb-extension-tests are dummy" > + ) > + return() > +endif() > + > +# MacOS asks for permission to debug a process even when the > +# machine is set into development mode. To solve the issue, > +# it is required to add relevant users to the `_developer` user > +# group in MacOS. Disabled for now. Minor: I suppose we should add the corresponding ticket to the tarantool-qa repo. It's understandable that MacOS isn't a top priority, so probably this will not be done soon, but having a ticket with a rationale and using the link to it here will still be nice. > +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND DEFINED ENV{CI}) > + message(WARNING > + "Interactive debugging is unavailable for macOS CI builds," > + "LuaJIT-lldb-extension-tests is dummy" > + ) > + return() > +endif() > + > +find_package(PythonInterp) Please fix the following warning policy. | CMake Warning (dev) at test/LuaJIT-debug-extensions-tests/CMakeLists.txt:28 (find_package): | Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules | are removed. Run "cmake --help-policy CMP0148" for policy details. Use | the cmake_policy command to set the policy and suppress this warning. | | This warning is for project developers. Use -Wno-dev to suppress it. IINM, you may workaround this for old CMake versions like the following [1]: | if(POLICY CMP0148) | # This policy is not known to old CMake. | cmake_policy(SET CMP0148 OLD) | endif() > +if(NOT PYTHONINTERP_FOUND) > + message(WARNING > + "`python` is not found, LuaJIT-lldb-extension-tests and " > + "LuaJIT-gdb-extension-tests are dummy" > + ) > + return() > +endif() > + > +set(DEBUGGER_TEST_ENV > + "LUAJIT_TEST_BINARY=${LUAJIT_TEST_BINARY}" > + # Suppresses __pycache__ generation. > + "PYTHONDONTWRITEBYTECODE=1" As an alternative it may be done via the -B flag. Since the -E flag will(?) ignore the value of this env variable, I suggest to using flags instead. > + "DEBUGGER_EXTENSION_PATH=${PROJECT_SOURCE_DIR}/src/luajit_dbg.py" > +) > + > +set(TEST_SCRIPT_PATH > + ${PROJECT_SOURCE_DIR}/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py Why don't use ${CMAKE_CURRENT_SOURCE_DIR} instead? > +) > + > +find_program(GDB gdb) > +if(GDB) > + set(test_title "test/${TEST_SUITE_NAME}/gdb") > + set(GDB_TEST_ENV ${DEBUGGER_TEST_ENV} "DEBUGGER_COMMAND=${GDB}") > + add_test(NAME "${test_title}" > + COMMAND ${PYTHON_EXECUTABLE} ${TEST_SCRIPT_PATH} I think it will be nice to add the -E option here to avoid interference with the user's env variables that are set locally. > + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} > + ) > + set_tests_properties("${test_title}" PROPERTIES > + ENVIRONMENT "${GDB_TEST_ENV}" > + LABELS ${TEST_SUITE_NAME} > + DEPENDS LuaJIT-dbg-extension-tests-deps > + ) > +else() > + message(WARNING "`gdb' is not found, so LuaJIT-gdb-extension-tests is dummy") > +endif() > + > +find_program(LLDB lldb) May we use some macro here to avoid copy-pasting? I suggest using the following prototype: | macro(AddTestDebugger target dbg) | # ... | endmacro() Side note: The macro presumes that target goes first to be similar to the rest of the CMake semantics. > +if(LLDB) > diff --git a/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py b/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py > new file mode 100644 > index 00000000..6ef87473 > --- /dev/null > +++ b/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py > @@ -0,0 +1,250 @@ > +# This file provides tests for LuaJIT debug extensions for lldb and gdb. > +import os > +import re > +import subprocess > +import sys > +import tempfile > +import unittest > + > +from threading import Timer > + > +LEGACY = re.match(r'^2\.', sys.version) Have we any runners with Python 2? If the answer is "no", do we need these workarounds if there are no guarantees that tests will work for Python 2? > + > +LUAJIT_BINARY = os.environ['LUAJIT_TEST_BINARY'] > +EXTENSION = os.environ['DEBUGGER_EXTENSION_PATH'] > +DEBUGGER = os.environ['DEBUGGER_COMMAND'] > +LLDB = 'lldb' in DEBUGGER > +TIMEOUT = 10 Side note: I suppose the timeout should be adjusted after adding the CI checks. > + > +RUN_CMD_FILE = '-s' if LLDB else '-x' > +INFERIOR_ARGS = '--' if LLDB else '--args' It will be nice to add some flags to avoid loading of users .gdbinit (`--nx` [3][4]) and .lldbinit (`--no-lldbinit` [5]). > +PROCESS_RUN = 'process launch' if LLDB else 'r' > +LOAD_EXTENSION = ( > + 'command script import {ext}' if LLDB else 'source {ext}' > +).format(ext=EXTENSION) > + > + > +def persist(data): > + tmp = tempfile.NamedTemporaryFile(mode='w') > + tmp.write(data) > + tmp.flush() > + return tmp > + > + > +class TestCaseBase(unittest.TestCase): > + @classmethod > + def construct_cmds(cls): > + return '\n'.join([ > + 'b {loc}'.format(loc=cls.location), > + PROCESS_RUN, > + 'n', > + LOAD_EXTENSION, > + cls.extension_cmds.strip(), > + 'q', > + ]) > + > + @classmethod > + def setUpClass(cls): > + cmd_file = persist(cls.construct_cmds()) > + script_file = persist(cls.lua_script) > + process_cmd = [ > + DEBUGGER, > + RUN_CMD_FILE, > + cmd_file.name, > + INFERIOR_ARGS, > + LUAJIT_BINARY, > + script_file.name, > + ] > + cls.output = filter_debugger_output(execute_process(process_cmd)) > + cmd_file.close() > + script_file.close() It would be nice to mention that `NamedTemporaryFile()` has the `delete=True` default to remove those files. > + > + def check(self): > + if LEGACY: > + self.assertRegexpMatches(self.output, self.pattern.strip()) > + else: > + self.assertRegex(self.output, self.pattern.strip()) > + > + > +class TestLoad(TestCaseBase): > + extension_cmds = '' > + location = 'lj_cf_print' > + lua_script = 'print(1)' I suppose we may use these values of `location` and `lua_script` as defaults. It is usefull for base tests and simplifies reading. Also, we may add the comment there that `print()` isn't actually executed since we quit before it finishes, so it's not spoiling the output. > + pattern = ( > + 'lj-tv command intialized\n' > + 'lj-state command intialized\n' > + 'lj-arch command intialized\n' > + 'lj-gc command intialized\n' > + 'lj-str command intialized\n' > + 'lj-tab command intialized\n' > + 'lj-stack command intialized\n' > + 'LuaJIT debug extension is successfully loaded\n' > + ) > + > + > +class TestLJArch(TestCaseBase): > + ) > + > + > +class TestLJState(TestCaseBase): > + ) > + > + > +class TestLJGC(TestCaseBase): > + ) > + > + > +class TestLJStack(TestCaseBase): > + extension_cmds = 'lj-stack' > + location = 'lj_cf_print' > + lua_script = 'print(1)' > + pattern = ( > + '-+ Red zone:\s+\d+ slots -+\n' > + '(0x[a-zA-Z0-9]+\s+\[(S|\s)(B|\s)(T|\s)(M|\s)\] VALUE: nil\n?)*\n' I suppose you mean 0x[a-fA-F0-9]? Here and below. I see that this pattern for hexademic address is used a lot. Can we predefine it above? Same for double usage of the stack pattern. > + '-+ Stack:\s+\d+ slots -+\n' > + '(0x[A-Za-z0-9]+(:0x[A-Za-z0-9]+)?\s+' > + '\[(S|\s)(B|\s)(T|\s)(M|\s)\].*\n?)+\n' > + ) > + > + > +class TestLJTV(TestCaseBase): > + location = 'lj_cf_print' > + lua_script = 'print(1)' > + extension_cmds = ( > + 'lj-tv L->base\n' > + 'lj-tv L->base + 11\n' > + ) > + > + lua_script = ( > + 'local ffi = require("ffi")\n' > + 'print(\n' > + ' nil,\n' > + ' false,\n' > + ' true,\n' > + ' "hello",\n' > + ' {1},\n' > + ' 1,\n' > + ' 1.1,\n' > + ' coroutine.create(function() end),\n' > + ' ffi.new("int*"),\n' > + ' function() end,\n' > + ' print,\n' > + ' require\n' > + ')\n' It would be nice to add a check for userdata too, using `newproxy()`. Also, it would be nice to sort the inputs in LJ_T* order. See , for example. > + ) > + > + pattern = ( > + 'nil\n' > + 'false\n' > + 'true\n' > + 'string \"hello\" @ 0x[a-zA-Z0-9]+\n' > + 'table @ 0x[a-zA-Z0-9]+ \(asize: \d+, hmask: 0x[a-zA-Z0-9]+\)\n' > + '(number|integer) .*1.*\n' > + 'number 1.1\d+\n' > + 'thread @ 0x[a-zA-Z0-9]+\n' > + 'cdata @ 0x[a-zA-Z0-9]+\n' > + 'Lua function @ 0x[a-zA-Z0-9]+, [0-9]+ upvalues, .+:[0-9]+\n' > + 'fast function #[0-9]+\n' > + 'C function @ 0x[a-zA-Z0-9]+\n' > + ) > + > + > +class TestLJStr(TestCaseBase): I've got the following timeouts locally, when build in debug mode: | 3/3 Test #2: test/LuaJIT-dbg-extension-tests/gdb ....***Failed 304.54 sec | test (__main__.TestLJArch.test) ... ok | test (__main__.TestLJGC.test) ... ok | test (__main__.TestLJStack.test) ... ok | test (__main__.TestLJState.test) ... ok | setUpClass (__main__.TestLJStr) ... ERROR | test (__main__.TestLJTV.test) ... ok | test (__main__.TestLJTab.test) ... ok | test (__main__.TestLoad.test) ... ok | test (__main__.TestLJTab.test) ... ok | | ====================================================================== | ERROR: setUpClass (__main__.TestLJStr) | ---------------------------------------------------------------------- | Traceback (most recent call last): | File "/home/burii/reviews/luajit/lj-dbg/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py", line 88, in setUpClass | cls.output = filter_debugger_output(execute_process(process_cmd)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | File "/home/burii/reviews/luajit/lj-dbg/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py", line 50, in execute_process | process = subprocess.run(cmd, capture_output=True, timeout=TIMEOUT) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | File "/usr/lib/python3.11/subprocess.py", line 550, in run | stdout, stderr = process.communicate(input, timeout=timeout) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | File "/usr/lib/python3.11/subprocess.py", line 1209, in communicate | stdout, stderr = self._communicate(input, endtime, timeout) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | File "/usr/lib/python3.11/subprocess.py", line 2114, in _communicate | self._check_timeout(endtime, orig_timeout, stdout, stderr) | File "/usr/lib/python3.11/subprocess.py", line 1253, in _check_timeout | raise TimeoutExpired( The problem is that the `fname` contains invalid address at the moment of breakpoint (for the Debug build), so the error is raised. | /tmp/tmpltp1awoe:5: Error in sourced command file: The following dirty-patch fixes the issue for me: | class TestLJStr(TestCaseBase): | - extension_cmds = 'lj-str fname' | + extension_cmds = 'n\nlj-str fname' I suppose other places where we use local variables should be adjusted as well. > + extension_cmds = 'lj-str fname' > + location = 'lj_cf_dofile' > + lua_script = 'pcall(dofile("name"))' I suppose you mean: | pcall(dofile, "name") If we are not interested in the status of child process we may just avoid the `pcall()` at all. Also, I suppose we may use something from the `string` built-in library to check the `lj-str` command. > + pattern = 'String: .* \[\d+ bytes\] with hash 0x[a-zA-Z0-9]+' > + > + > +class TestLJTab(TestCaseBase): > + extension_cmds = 'lj-tab t' > + location = 'lj_cf_unpack' > + lua_script = 'unpack({1; a = 1})' > + pattern = ( > + 'Array part: 3 slots\n' > + '0x[a-zA-Z0-9]+: \[0\]: nil\n' > + '0x[a-zA-Z0-9]+: \[1\]: .+ 1\n' > + '0x[a-zA-Z0-9]+: \[2\]: nil\n' > + 'Hash part: 2 nodes\n' > + '0x[a-zA-Z0-9]+: { string "a" @ 0x[a-zA-Z0-9]+ } => ' > + '{ .+ 1 }; next = 0x0\n' > + '0x[a-zA-Z0-9]+: { nil } => { nil }; next = 0x0\n' > + ) > + > + > +for test_cls in TestCaseBase.__subclasses__(): > + test_cls.test = lambda self: self.check() > + > +if __name__ == '__main__': > + unittest.main(verbosity=2) > -- > 2.44.0 > [1]: https://www.flake8rules.com/rules/W605.html [2]: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Policies.html#supporting-multiple-cmake-versions [3]: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Mode-Options.html [4]: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Initialization-Files.html#Initialization-Files [5]: https://lldb.llvm.org/man/lldb.html -- Best regards, Sergey Kaplun