Re: [Tarantool-patches] [PATCH luajit] Fix C file generation in jit.bcsave.

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

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

On 19.10.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@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