[Tarantool-patches] [PATCH luajit] Fix C file generation in jit.bcsave.
Sergey Kaplun
skaplun at tarantool.org
Mon Oct 23 14:12:59 MSK 2023
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On 19.10.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb at tarantool.org>
>
> Thanks to codicodi.
>
> (cherry picked from commit 62903bacf4abbb1c6df0f395553eeaf1f68352c6)
>
> LuaJIT allows exporting bytecode to a C file using option `-b`, see [1].
Typo: s/option/the option/
> For building generated C file in C++ projects C file uses a macro
Typo: s/generated/a generated/
Typo: s/projects/projects, the/
> `__cplusplus` [2], but this macro was broken by commit a9dd47b7fcde
Nit: please, use full commit hash.
> ("Extend -b to generate c/h/obj/o files with embedded bytecode."). With
> this breakage C/C++ compiler removes definition of array with bytecode
Typo: s/breakage/breakage, the/
Typo: s/definition/the definition/
Typo: s/array/an array/
Typo: s/bytecode/bytecode,/
> and resulted object file has missed a symbol with bytecode.
Typo: s/resulted/the resulted/
>
> The patch fixes broken macro.
Typo: s/broken/the broken/
>
> Note, test test/lua-Harness-tests/411-luajit.t checks a precense of
Typo: s/a/the/
Typo: s/of/of the/
> macro `__cplusplus` in generated C file, however it doesn't catch the
Typo: s/generated/the generated/
Typo: s/however/however,/
> bug.
Side note: I suppose it is, but then the workaround was added:
https://framagit.org/fperrad/lua-Harness/-/commit/b30ee70664fe366484642e049eb6a4660adc8a06
>
> Sergey Bronnikov:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
>
> 1. https://luajit.org/running.html
> 2. https://en.cppreference.com/w/cpp/preprocessor/replace
> ---
> PR: https://github.com/tarantool/tarantool/pull/9276
> Epic: https://github.com/tarantool/tarantool/issues/9145
> Issue: none
>
> src/jit/bcsave.lua | 2 +-
> test/tarantool-tests/CMakeLists.txt | 1 +
> .../lj-bytecode-c-broken-macro.test.lua | 13 ++++++++++++
> .../lj-bytecode-c-broken-macro/CMakeLists.txt | 21 +++++++++++++++++++
> .../lj-bytecode-c-broken-macro/lj_lib_xxx.lua | 4 ++++
This is the "ticket" for the fix, please rename the test, and its name
(lj-551): https://github.com/LuaJIT/LuaJIT/pull/551.
> 5 files changed, 40 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/lj-bytecode-c-broken-macro.test.lua
> create mode 100644 test/tarantool-tests/lj-bytecode-c-broken-macro/CMakeLists.txt
> create mode 100644 test/tarantool-tests/lj-bytecode-c-broken-macro/lj_lib_xxx.lua
>
> diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
> index 41081184..5a12c7d0 100644
> --- a/src/jit/bcsave.lua
> +++ b/src/jit/bcsave.lua
> @@ -133,7 +133,7 @@ local function bcsave_c(ctx, output, s)
> local fp = savefile(output, "w")
> if ctx.type == "c" then
> fp:write(format([[
> -#ifdef _cplusplus
> +#ifdef __cplusplus
> extern "C"
> #endif
> #ifdef _WIN32
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index c15d6037..838e359a 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -68,6 +68,7 @@ add_subdirectory(lj-727-lightuserdata-itern)
> add_subdirectory(lj-802-panic-at-mcode-protfail)
> add_subdirectory(lj-flush-on-trace)
> add_subdirectory(lj-1004-oom-error-frame)
> +add_subdirectory(lj-bytecode-c-broken-macro)
Minor: May you, please also sort alphabetically the mentioned
directories?
| add_subdirectory(lj-802-panic-at-mcode-protfail)
| add_subdirectory(lj-bytecode-c-broken-macro)
| add_subdirectory(lj-flush-on-trace)
And lj-1004 should go on top.
I understand, that this will add unrelated changes, so feel free to
ignore refactoring part. Nevertheless, lj-bytecode-c-broken-macro should
go before lj-flush-on-trace.
>
> # The part of the memory profiler toolchain is located in tools
> # directory, jit, profiler, and bytecode toolchains are located
> diff --git a/test/tarantool-tests/lj-bytecode-c-broken-macro.test.lua b/test/tarantool-tests/lj-bytecode-c-broken-macro.test.lua
> new file mode 100644
> index 00000000..2047adac
> --- /dev/null
> +++ b/test/tarantool-tests/lj-bytecode-c-broken-macro.test.lua
> @@ -0,0 +1,13 @@
> +local tap = require('tap')
> +local test = tap.test('lj-jit-bcsave-c-generation')
Should it be `bytecode-c-broken-macro`?
> +
> +test:plan(3)
> +
> +local module_name = 'lj_lib_xxx'
Nit: Is it better to name the library something like 'bcsaved-clib' (to
match library and test name)?
> +local ok, module = pcall(require, module_name)
> +local message = ('symbol "%s" is available in a library'):format(module_name)
> +test:ok(ok == true, message)
Nit: `test:ok(ok)` looks better, since there is only `true` or `false`
can be returned from the `pcall()`.
Also, you may renamed `ok` to `isok` if you want.
> +test:is(module.a, 1)
> +test:is(module.b, 2)
Why only one value isn't enough?
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-bytecode-c-broken-macro/CMakeLists.txt b/test/tarantool-tests/lj-bytecode-c-broken-macro/CMakeLists.txt
> new file mode 100644
> index 00000000..aa715067
> --- /dev/null
> +++ b/test/tarantool-tests/lj-bytecode-c-broken-macro/CMakeLists.txt
> @@ -0,0 +1,21 @@
> +set(LIB_NAME "lj_lib_xxx")
> +set(LUA_FILE ${CMAKE_CURRENT_SOURCE_DIR}/${LIB_NAME}.lua)
> +set(C_FILE ${LIB_NAME}.c)
> +set(CXX_FILE ${LIB_NAME}.cc)
CXX file is defined not used.
> +
> +make_lua_path(LUA_PATH
> + PATHS
> + ${PROJECT_SOURCE_DIR}/src/?.lua
> + ${PROJECT_SOURCE_DIR}/src/jit/?.lua
> +)
> +
> +add_custom_target(export_bc
> + COMMAND ${CMAKE_COMMAND} -E env LUA_PATH=${LUA_PATH} ${LUAJIT_BINARY} -b ${LUA_FILE} ${C_FILE}
May you please to split ${CMAKE_COMMAND} to the next line with additonal
indentation level as the following (see other test targets for example):
| COMMAND ${CMAKE_COMMAND} -E
| env
| LUA_PATH=${LUA_PATH}
| ${LUAJIT_BINARY} -b ${LUA_FILE} ${C_FILE}
Also, may you please add a comment why `LUA_PATH` is better here instead
`LUA_TEST_ENV` (we need only to run the corresponding `jit.` modules)?
I suppose that this test is passes even before the patch because its
required Lua library instead compiled C library. Maybe it is better to
use different names for them to avoid collisions?
> + DEPENDS luajit-main ${LUA_FILE}
> + BYPRODUCTS ${C_FILE}
> + COMMENT "Exporting bytecode to a C file"
> + VERBATIM
> +)
> +
> +BuildTestCLib(${LIB_NAME} ${C_FILE})
> +add_dependencies(${LIB_NAME} export_bc)
> diff --git a/test/tarantool-tests/lj-bytecode-c-broken-macro/lj_lib_xxx.lua b/test/tarantool-tests/lj-bytecode-c-broken-macro/lj_lib_xxx.lua
> new file mode 100644
> index 00000000..591dfc8b
> --- /dev/null
> +++ b/test/tarantool-tests/lj-bytecode-c-broken-macro/lj_lib_xxx.lua
> @@ -0,0 +1,4 @@
> +return {
> + a = 1,
> + b = 2,
> +}
> --
> 2.34.1
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list