[Tarantool-patches] [PATCH luajit] Fix C file generation in jit.bcsave.
Sergey Bronnikov
sergeyb at tarantool.org
Tue Oct 31 12:57:27 MSK 2023
Hello,
On 10/23/23 14:12, Sergey Kaplun wrote:
> 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/
Fixed.
>
>> For building generated C file in C++ projects C file uses a macro
> Typo: s/generated/a generated/
> Typo: s/projects/projects, the/
Fixed.
>> `__cplusplus` [2], but this macro was broken by commit a9dd47b7fcde
> Nit: please, use full commit hash.
Fixed.
>
>> ("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,/
Fixed.
>
>> and resulted object file has missed a symbol with bytecode.
> Typo: s/resulted/the resulted/
Fixed.
>> 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/
Fixed.
>
>> macro `__cplusplus` in generated C file, however it doesn't catch the
> Typo: s/generated/the generated/
> Typo: s/however/however,/
Fixed.
>
>> bug.
> Side note: I suppose it is, but then the workaround was added:
> https://framagit.org/fperrad/lua-Harness/-/commit/b30ee70664fe366484642e049eb6a4660adc8a06
>
ok, thanks.
>> 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.
Renamed.
>
>> 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?
Sure.
>
> | 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.
reverted change for lj-flush-on-trace, made per request in the previous
comment
>>
>> # 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`?
Should be.
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
@@ -1,5 +1,5 @@
local tap = require('tap')
-local test = tap.test('lj-jit-bcsave-c-generation')
+local test = tap.test('lj-551-bytecode-c-broken-macro')
test:plan(3)
>
>> +
>> +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)?
Fixed.
diff --git
a/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
b/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
index a3c975df..4702724d 100644
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
@@ -3,7 +3,7 @@ local test = tap.test('lj-551-bytecode-c-broken-macro')
test:plan(3)
-local module_name = 'lj_lib_xxx'
+local module_name = 'bcsaved-clib'
local ok, module = pcall(require, module_name)
local message = ('symbol "%s" is available in a
library'):format(module_name)
test:ok(ok == true, message)
diff --git
a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
index aa715067..c9210176 100644
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
@@ -1,4 +1,4 @@
-set(LIB_NAME "lj_lib_xxx")
+set(LIB_NAME "bcsaved-clib")
set(LUA_FILE ${CMAKE_CURRENT_SOURCE_DIR}/${LIB_NAME}.lua)
set(C_FILE ${LIB_NAME}.c)
set(CXX_FILE ${LIB_NAME}.cc)
bcsaved-lib is a bad name because:
tarantool> require("bcsaved-clib")
---
- error: "error loading module 'bcsaved-clib' from file
'./bcsaved-clib.so':\n\t/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved-clib.so:
undefined symbol: luaopen_clib"
...
replaced with bcsaved_clib
>
>> +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()`.
Fixed.
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
@@ -6,7 +6,7 @@ test:plan(3)
local module_name = 'bcsaved-clib'
local ok, module = pcall(require, module_name)
local message = ('symbol "%s" is available in a
library'):format(module_name)
-test:ok(ok == true, message)
+test:ok(ok, message)
test:is(module.a, 1)
test:is(module.b, 2)
> Also, you may renamed `ok` to `isok` if you want.
No, I don't want.
>> +test:is(module.a, 1)
>> +test:is(module.b, 2)
> Why only one value isn't enough?
You are right, single value is more than enough.
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
@@ -6,8 +6,7 @@ test:plan(3)
local module_name = 'bcsaved-clib'
local ok, module = pcall(require, module_name)
local message = ('symbol "%s" is available in a
library'):format(module_name)
-test:ok(ok == true, message)
-test:is(module.a, 1)
-test:is(module.b, 2)
+test:ok(ok, message)
+test:is(module.a, 0)
test:done(true)
diff --git
a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved-clib.lua
b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved-clib.lua
index 591dfc8b..ac5ef1c0 100644
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved-clib.lua
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved-clib.lua
@@ -1,4 +1,3 @@
return {
- a = 1,
- b = 2,
+ a = 0,
}
Additionally changed "1" to a "0" because "1" could be a reason of
questions like "Why positive integer value and not a neutral zero?"
>
>> +
>> +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.
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
@@ -1,7 +1,6 @@
set(LIB_NAME "bcsaved-clib")
set(LUA_FILE ${CMAKE_CURRENT_SOURCE_DIR}/${LIB_NAME}.lua)
set(C_FILE ${LIB_NAME}.c)
-set(CXX_FILE ${LIB_NAME}.cc)
make_lua_path(LUA_PATH
PATHS
>
>> +
>> +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):
formatted in a bit another way:
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
@@ -9,7 +9,10 @@ make_lua_path(LUA_PATH
)
add_custom_target(export_bc
- COMMAND ${CMAKE_COMMAND} -E env LUA_PATH=${LUA_PATH} ${LUAJIT_BINARY}
-b ${LUA_FILE} ${C_FILE}
+ COMMAND ${CMAKE_COMMAND} -E
+ env
+ LUA_PATH=${LUA_PATH}
+ ${LUAJIT_BINARY} -b ${LUA_FILE} ${C_FILE}
DEPENDS luajit-main ${LUA_FILE}
BYPRODUCTS ${C_FILE}
COMMENT "Exporting bytecode to a C file"
>
> | 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?
This is a good observation, thanks!
We cannot use different for exported C file and Lua module,
because file name is used as a symbol in shared library:
$ nm
build/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved_clib.so
<snipped>
0000000000002000 R luaJIT_BC_bcsaved_clib
<snipped>
I did it in the following way: Lua module in source dir has name
bcsaved.lua, then we copying this file to
a current binary dir with another name bcsaved_clib.lua, then we export
bc code to C and *remove* Lua module
in a current binary dir:
diff --git
a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
index cd685b5b..45857386 100644
--- a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
@@ -1,5 +1,6 @@
set(LIB_NAME "bcsaved_clib")
-set(LUA_FILE ${CMAKE_CURRENT_SOURCE_DIR}/${LIB_NAME}.lua)
+set(LUA_FILE_ORIG ${CMAKE_CURRENT_SOURCE_DIR}/bcsaved.lua)
+set(LUA_FILE ${CMAKE_CURRENT_BINARY_DIR}/${LIB_NAME}.lua)
set(C_FILE ${LIB_NAME}.c)
make_lua_path(LUA_PATH
@@ -8,11 +9,24 @@ make_lua_path(LUA_PATH
${PROJECT_SOURCE_DIR}/src/jit/?.lua
)
+add_custom_target(copy_lua
+ COMMAND ${CMAKE_COMMAND} -E
+ copy
+ ${LUA_FILE_ORIG}
+ ${LUA_FILE}
+ DEPENDS ${LUA_FILE_ORIG}
+ BYPRODUCTS ${LUA_FILE}
+ COMMENT "Copying Lua module"
+ VERBATIM
+)
+
add_custom_target(export_bc
COMMAND ${CMAKE_COMMAND} -E
env
LUA_PATH=${LUA_PATH}
${LUAJIT_BINARY} -b ${LUA_FILE} ${C_FILE}
+ COMMAND ${CMAKE_COMMAND} -E
+ remove ${LUA_FILE}
DEPENDS luajit-main ${LUA_FILE}
BYPRODUCTS ${C_FILE}
COMMENT "Exporting bytecode to a C file"
diff --git
a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved_clib.lua
b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua
similarity index 100%
rename from
test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved_clib.lua
rename to test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua
<snipped>
More information about the Tarantool-patches
mailing list