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

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

[Sergey Bronnikov via Tarantool-patches]

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].
For building generated C file in C++ projects C file uses a macro
`__cplusplus` [2], but this macro was broken by commit a9dd47b7fcde
("Extend -b to generate c/h/obj/o files with embedded bytecode."). With
this breakage C/C++ compiler removes definition of array with bytecode
and resulted object file has missed a symbol with bytecode.

The patch fixes broken macro.

Note, test test/lua-Harness-tests/411-luajit.t checks a precense of
macro `__cplusplus` in generated C file, however it doesn't catch the
bug.

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 ++++
 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)
 
 # 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')
+
+test:plan(3)
+
+local module_name = 'lj_lib_xxx'
+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: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)
+
+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}
+  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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

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>

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

[Sergey Kaplun via Tarantool-patches]

Hello, Sergey!
Thanks for the fixes!
See my comment below.

On 31.10.23, Sergey Bronnikov wrote:
> 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)
> >>

<snipped>

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

The test still doesn't fail before the commit.
The reason is that the C file is compiled by C compiler, not the C++
compiler (so this macro is irrelevant).

I suppose that we should add a .cpp file for the test too (and add the
LINKER_LANGUAGE CXX [1] property for this target if its necessary).

[1]: https://cmake.org/cmake/help/latest/prop_tgt/LINKER_LANGUAGE.html#linker-language

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3395 bytes --]

Hello, Sergey

On 10/31/23 16:21, Sergey Kaplun wrote:


<snipped>

>> --- 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"
> The test still doesn't fail before the commit.
> The reason is that the C file is compiled by C compiler, not the C++
> compiler (so this macro is irrelevant).
Fixed, thanks.
>
> I suppose that we should add a .cpp file for the test too (and add the
> LINKER_LANGUAGE CXX [1] property for this target if its necessary).

Unfortunately, setting property with LINKER_LANGUAGE is not enough.

Additionally one need setting CXX language for a project or enable CXX 
in required CMakeLists.txt

and it is better to have .cc extension for a file too. Otherwise CMake 
fails to build a file by CXX compiler.

I believe you will not against to backporting 
e826d0c101d750fac8334d71e221c50d8dbe236c ("Add 'cc' file type for saving 
bytecode.")

as well because commit enables saving bc to .cc file directly.


Final patch:

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 45857386..1baf6ec8 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,7 +1,9 @@
+enable_language(CXX)
+
  set(LIB_NAME "bcsaved_clib")
  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)
+set(CXX_FILE ${LIB_NAME}.cc)

  make_lua_path(LUA_PATH
    PATHS
@@ -24,14 +26,14 @@ add_custom_target(export_bc
    COMMAND ${CMAKE_COMMAND} -E
      env
        LUA_PATH=${LUA_PATH}
-    ${LUAJIT_BINARY} -b ${LUA_FILE} ${C_FILE}
+    ${LUAJIT_BINARY} -b ${LUA_FILE} ${CXX_FILE}
    COMMAND ${CMAKE_COMMAND} -E
      remove ${LUA_FILE}
    DEPENDS luajit-main ${LUA_FILE}
-  BYPRODUCTS ${C_FILE}
+  BYPRODUCTS ${CXX_FILE}

  -  COMMENT "Exporting bytecode to a C file"
  + COMMENT "Exporting bytecode to a CXX file"
    VERBATIM
  )

-BuildTestCLib(${LIB_NAME} ${C_FILE})
+BuildTestCLib(${LIB_NAME} ${CXX_FILE})
  add_dependencies(${LIB_NAME} export_bc)

>
> [1]:https://cmake.org/cmake/help/latest/prop_tgt/LINKER_LANGUAGE.html#linker-language
>

[-- Attachment #2: Type: text/html, Size: 5231 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, for the patch itself, just a question:

Why do you remove the C file to check? I suppose it's good to check
creation of a C file too. Thus we can avoid some regressions in the
future.

On 08.11.23, Sergey Bronnikov wrote:
> Hello, Sergey
> 
> On 10/31/23 16:21, Sergey Kaplun wrote:
> 
<snipped>

> >>    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"
> > The test still doesn't fail before the commit.
> > The reason is that the C file is compiled by C compiler, not the C++
> > compiler (so this macro is irrelevant).
> Fixed, thanks.
> >
> > I suppose that we should add a .cpp file for the test too (and add the
> > LINKER_LANGUAGE CXX [1] property for this target if its necessary).
> 
> Unfortunately, setting property with LINKER_LANGUAGE is not enough.
> 
> Additionally one need setting CXX language for a project or enable CXX 
> in required CMakeLists.txt
> 
> and it is better to have .cc extension for a file too. Otherwise CMake 
> fails to build a file by CXX compiler.

Nice investigation, thanks!

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.0 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/9832
[2]: https://github.com/tarantool/tarantool/pull/9833
[3]: https://github.com/tarantool/tarantool/pull/9834

-- 
Best regards,
Sergey Kaplun