Re: [Tarantool-patches] [PATCH luajit v6 2/2] test: add tests for debugging extensions

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the patch!
Please consider my comments below.

On 04.04.24, Maxim Kokryashkin wrote:
> From: Maksim Kokryashkin <max.kokryashkin@gmail.com>
> 
> 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 <optimized> 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)

<snipped>

> 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
> +
> +

<snipped>

> +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):

<snipped>

> +    )
> +
> +
> +class TestLJState(TestCaseBase):

<snipped>

> +    )
> +
> +
> +class TestLJGC(TestCaseBase):

<snipped>

> +    )
> +
> +
> +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'

<snipped>

> +        '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
<test/tarantool-tests/lj-351-print-tostring-number.test.lua>, 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