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

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

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