Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave.
@ 2023-11-09 13:06 Sergey Bronnikov via Tarantool-patches
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode Sergey Bronnikov via Tarantool-patches
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-11-09 13:06 UTC (permalink / raw)
  To: tarantool-patches, Sergey Kaplun, max.kokryashkin

From: Sergey Bronnikov <sergeyb@tarantool.org>

Changes in v2:

- added patch "Add 'cc' file type for saving bytecode."
- updated building test libraries
- add test for C library as well
- small refactoring in test/tarantool-tests/CMakeLists.txt

PR: https://github.com/tarantool/tarantool/pull/9276
Epic: https://github.com/tarantool/tarantool/issues/9145
Issue: none

Mike Pall (2):
  Add 'cc' file type for saving bytecode.
  Fix C file generation in jit.bcsave.

Sergey Bronnikov (1):
  test: set dependencies to tarantool-tests explicitly

 doc/running.html                              |  3 +-
 src/jit/bcsave.lua                            |  6 +--
 test/tarantool-tests/CMakeLists.txt           | 18 +++----
 .../lj-551-bytecode-c-broken-macro.test.lua   | 16 +++++++
 .../CMakeLists.txt                            | 47 +++++++++++++++++++
 .../bcsaved.lua                               |  3 ++
 6 files changed, 77 insertions(+), 16 deletions(-)
 create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
 create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua

-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode.
  2023-11-09 13:06 [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
@ 2023-11-09 13:08 ` Sergey Bronnikov via Tarantool-patches
  2023-11-13  7:04   ` Sergey Kaplun via Tarantool-patches
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly Sergey Bronnikov via Tarantool-patches
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-11-09 13:08 UTC (permalink / raw)
  To: tarantool-patches, Sergey Kaplun, max.kokryashkin

From: Mike Pall <mike>

Contributed by Sergey Bronnikov. #1105

(cherry picked from commit e826d0c101d750fac8334d71e221c50d8dbe236c)

The patch adds a file type `cc` that allows exporting byte code directly
to a file with extension `.cc`.

Sergey Bronnikov:
* added the description

Part of tarantool/tarantool#9145
---
 doc/running.html   | 3 ++-
 src/jit/bcsave.lua | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/doc/running.html b/doc/running.html
index 666b0abc..7868efab 100644
--- a/doc/running.html
+++ b/doc/running.html
@@ -124,7 +124,8 @@ file name:
 </p>
 <ul>
 <li><tt>c</tt> &mdash; C source file, exported bytecode data.</li>
-<li><tt>h</tt> &mdash; C header file, static bytecode data.</li>
+<li><tt>cc</tt> &mdash; C++ source file, exported bytecode data.</li>
+<li><tt>h</tt> &mdash; C/C++ header file, static bytecode data.</li>
 <li><tt>obj</tt> or <tt>o</tt> &mdash; Object file, exported bytecode data
 (OS- and architecture-specific).</li>
 <li><tt>raw</tt> or any other extension &mdash; Raw bytecode file (portable).
diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
index 41081184..48b2329c 100644
--- a/src/jit/bcsave.lua
+++ b/src/jit/bcsave.lua
@@ -37,7 +37,7 @@ Save LuaJIT bytecode: luajit -b[options] input output
   --        Stop handling options.
   -         Use stdin as input and/or stdout as output.
 
-File types: c h obj o raw (default)
+File types: c cc h obj o raw (default)
 ]]
   os.exit(1)
 end
@@ -63,7 +63,7 @@ end
 ------------------------------------------------------------------------------
 
 local map_type = {
-  raw = "raw", c = "c", h = "h", o = "obj", obj = "obj",
+  raw = "raw", c = "c", cc = "c", h = "h", o = "obj", obj = "obj",
 }
 
 local map_arch = {
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly
  2023-11-09 13:06 [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode Sergey Bronnikov via Tarantool-patches
@ 2023-11-09 13:08 ` Sergey Bronnikov via Tarantool-patches
  2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 3/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
  2024-02-29  7:40 ` [Tarantool-patches] [PATCH luajit 0/3][v2] " Igor Munkin via Tarantool-patches
  3 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-11-09 13:08 UTC (permalink / raw)
  To: tarantool-patches, Sergey Kaplun, max.kokryashkin

From: Sergey Bronnikov <sergeyb@tarantool.org>

In testsuite `tarantool-tests` we are using CMake macro `BuildTestCLib`
for building test C libraries. This macro builds a list `TESTLIBS` that
contains all libraries required for tarantool tests. However, CMake
skips adding C library to `TESTLIBS` on building test C library in the
following commit.

The patch removes `TESTLIBS` and adds each test C library as dependence
to CMake target `tarantool-tests` explicitly using `add_dependence`
command.
---
 test/tarantool-tests/CMakeLists.txt | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index c15d6037..d46e69a0 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -10,6 +10,10 @@ if(NOT PROVE)
   return()
 endif()
 
+add_custom_target(tarantool-tests
+  DEPENDS ${LUAJIT_TEST_BINARY}
+)
+
 macro(BuildTestCLib lib sources)
   add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources})
   target_include_directories(${lib} PRIVATE
@@ -37,12 +41,7 @@ macro(BuildTestCLib lib sources)
       CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}"
     )
   endif()
-  # XXX: Append the lib to be built to the dependency list.
-  # Unfortunately, there is no convenient way in CMake to extend
-  # the list in parent scope other than join two strings with
-  # semicolon. If one finds the normal way to make it work, feel
-  # free to reach me.
-  set(TESTLIBS "${lib};${TESTLIBS}" PARENT_SCOPE)
+  add_dependencies(tarantool-tests ${lib})
   # Add the directory where the lib is built to the list with
   # entries for LUA_CPATH environment variable, so LuaJIT can find
   # and load it. See the comment about extending the list in the
@@ -133,11 +132,6 @@ else()
   list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
 endif()
 
-# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
-# with dependecies are set in scope of BuildTestLib macro.
-add_custom_target(tarantool-tests
-  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
-)
 add_custom_command(TARGET tarantool-tests
   COMMENT "Running Tarantool tests"
   COMMAND
@@ -154,4 +148,3 @@ add_custom_command(TARGET tarantool-tests
       ${LUA_TEST_FLAGS}
   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
 )
-
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH luajit 3/3][v2] Fix C file generation in jit.bcsave.
  2023-11-09 13:06 [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode Sergey Bronnikov via Tarantool-patches
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly Sergey Bronnikov via Tarantool-patches
@ 2023-11-09 13:08 ` Sergey Bronnikov via Tarantool-patches
  2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
  2024-02-29  7:40 ` [Tarantool-patches] [PATCH luajit 0/3][v2] " Igor Munkin via Tarantool-patches
  3 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-11-09 13:08 UTC (permalink / raw)
  To: tarantool-patches, Sergey Kaplun, max.kokryashkin

From: Mike Pall <mike>

Thanks to codicodi.

(cherry picked from commit 62903bacf4abbb1c6df0f395553eeaf1f68352c6)

LuaJIT allows exporting bytecode to a C file using the option `-b`, see
[1]. For building a generated C file in C++ projects, C file uses a
macro `__cplusplus` [2], but this macro was broken by commit
a9dd47b7fcde95bbca06485c50326b7b90d930a8 ("Extend -b to generate
c/h/obj/o files with embedded bytecode."). With this breakage, C/C++
compiler removes the definition of an array with bytecode, and the
resulted object file has missed a symbol with bytecode.

The patch fixes the broken macro.

Note, test test/lua-Harness-tests/411-luajit.t checks the precense of
the macro `__cplusplus` in the 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
---
 src/jit/bcsave.lua                            |  2 +-
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-551-bytecode-c-broken-macro.test.lua   | 16 +++++++
 .../CMakeLists.txt                            | 47 +++++++++++++++++++
 .../bcsaved.lua                               |  3 ++
 5 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
 create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua

diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
index 48b2329c..a287d675 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 d46e69a0..6f29c827 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -62,6 +62,7 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/stripped)
 add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
 add_subdirectory(gh-6189-cur_L)
 add_subdirectory(lj-416-xor-before-jcc)
+add_subdirectory(lj-551-bytecode-c-broken-macro)
 add_subdirectory(lj-601-fix-gc-finderrfunc)
 add_subdirectory(lj-727-lightuserdata-itern)
 add_subdirectory(lj-802-panic-at-mcode-protfail)
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
new file mode 100644
index 00000000..1cfcb4b1
--- /dev/null
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
@@ -0,0 +1,16 @@
+local tap = require('tap')
+local test = tap.test('lj-551-bytecode-c-broken-macro')
+
+test:plan(4)
+
+local function check_module(t, module_name)
+  local ok, module = pcall(require, module_name)
+  local message = ('symbol "%s" is available in a library'):format(module_name)
+  t:ok(ok, message)
+  t:is(module.a, 0)
+end
+
+check_module(test, 'bcsaved_lib_c')
+check_module(test, 'bcsaved_lib_cc')
+
+test:done(true)
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
new file mode 100644
index 00000000..732a7e4d
--- /dev/null
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
@@ -0,0 +1,47 @@
+enable_language(CXX)
+
+set(LUA_FILE_ORIG ${CMAKE_CURRENT_SOURCE_DIR}/bcsaved.lua)
+
+make_lua_path(LUA_PATH
+  PATHS
+    ${PROJECT_SOURCE_DIR}/src/?.lua
+    ${PROJECT_SOURCE_DIR}/src/jit/?.lua
+)
+
+macro(BuildBCLib file_ext)
+  set(LIB_NAME "bcsaved_lib_${file_ext}")
+  set(LUA_FILE "${CMAKE_CURRENT_BINARY_DIR}/${LIB_NAME}.lua")
+  set(GENERATED_FILE "${LIB_NAME}.${file_ext}")
+
+  add_custom_target(copy_lua_${file_ext}
+    COMMAND ${CMAKE_COMMAND} -E
+      copy
+        ${LUA_FILE_ORIG}
+        ${LUA_FILE}
+    DEPENDS ${LUA_FILE_ORIG}
+    BYPRODUCTS ${LUA_FILE}
+    COMMENT "Copying Lua module ${LIB_NAME} to a build directory"
+    VERBATIM
+  )
+
+  add_custom_target(export_bc_${file_ext}
+    COMMAND ${CMAKE_COMMAND} -E
+      env
+        LUA_PATH=${LUA_PATH}
+      ${LUAJIT_BINARY} -b ${LUA_FILE} ${GENERATED_FILE}
+    COMMAND ${CMAKE_COMMAND} -E
+      remove ${LUA_FILE}
+    DEPENDS luajit-main ${LUA_FILE}
+    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+    BYPRODUCTS ${GENERATED_FILE}
+    COMMENT "Exporting bytecode to a ${file_ext} file"
+    VERBATIM
+  )
+
+  BuildTestCLib(${LIB_NAME} ${GENERATED_FILE})
+  add_dependencies(${LIB_NAME} export_bc_${file_ext})
+  add_dependencies(tarantool-tests ${LIB_NAME})
+endmacro()
+
+BuildBCLib("c")
+BuildBCLib("cc")
diff --git a/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua
new file mode 100644
index 00000000..ac5ef1c0
--- /dev/null
+++ b/test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua
@@ -0,0 +1,3 @@
+return {
+  a = 0,
+}
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode.
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode Sergey Bronnikov via Tarantool-patches
@ 2023-11-13  7:04   ` Sergey Kaplun via Tarantool-patches
  2023-11-15  9:39     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-11-13  7:04 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches, max.kokryashkin

Hi, Sergey!
Thanks for the patch.
LGTM, after fixing my nits below.

On 09.11.23, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Contributed by Sergey Bronnikov. #1105

Minor: I suggest dropping #1105 here to avoid spamming to the ticket
during development.

> 
> (cherry picked from commit e826d0c101d750fac8334d71e221c50d8dbe236c)
> 
> The patch adds a file type `cc` that allows exporting byte code directly
> to a file with extension `.cc`.

Typo: s/extension/the extension/

I suppose it is worth mentioning that the test will be added within
future commits?

> 
> Sergey Bronnikov:
> * added the description
> 
> Part of tarantool/tarantool#9145
> ---

<snipped>

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly Sergey Bronnikov via Tarantool-patches
@ 2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
  2023-11-15 11:34     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-11-13  7:10 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches

Hi, Sergey!
Thanks for the patch!
LGTM, after fixing my comments below.

On 09.11.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> In testsuite `tarantool-tests` we are using CMake macro `BuildTestCLib`

Typo: s/`tarantool-tests`/`tarantool-tests`,/
Typo: s/CMake/the CMake/

> for building test C libraries. This macro builds a list `TESTLIBS` that
> contains all libraries required for tarantool tests. However, CMake
> skips adding C library to `TESTLIBS` on building test C library in the

Typo: s/adding C library/adding the C library/
Nit: s/on/when/
Typo: s/building test C library/building the test C library/

> following commit.
> 
> The patch removes `TESTLIBS` and adds each test C library as dependence

Typo: s/dependence/a dependency/

> to CMake target `tarantool-tests` explicitly using `add_dependence`

Typo: s/using/using the/

> command.
> ---
>  test/tarantool-tests/CMakeLists.txt | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index c15d6037..d46e69a0 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -10,6 +10,10 @@ if(NOT PROVE)
>    return()
>  endif()
>  
> +add_custom_target(tarantool-tests
> +  DEPENDS ${LUAJIT_TEST_BINARY}
> +)
> +
>  macro(BuildTestCLib lib sources)
>    add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources})
>    target_include_directories(${lib} PRIVATE
> @@ -37,12 +41,7 @@ macro(BuildTestCLib lib sources)
>        CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}"
>      )
>    endif()
> -  # XXX: Append the lib to be built to the dependency list.
> -  # Unfortunately, there is no convenient way in CMake to extend
> -  # the list in parent scope other than join two strings with
> -  # semicolon. If one finds the normal way to make it work, feel
> -  # free to reach me.
> -  set(TESTLIBS "${lib};${TESTLIBS}" PARENT_SCOPE)
> +  add_dependencies(tarantool-tests ${lib})
>    # Add the directory where the lib is built to the list with
>    # entries for LUA_CPATH environment variable, so LuaJIT can find
>    # and load it. See the comment about extending the list in the
> @@ -133,11 +132,6 @@ else()
>    list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>  endif()
>  
> -# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list

I suppose that information about LUA_PATH and LD_LIBRARY_PATH may stay.

> -# with dependecies are set in scope of BuildTestLib macro.
> -add_custom_target(tarantool-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
> -)
>  add_custom_command(TARGET tarantool-tests
>    COMMENT "Running Tarantool tests"
>    COMMAND
> @@ -154,4 +148,3 @@ add_custom_command(TARGET tarantool-tests
>        ${LUA_TEST_FLAGS}
>    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>  )
> -
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 3/3][v2] Fix C file generation in jit.bcsave.
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 3/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
@ 2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-11-13  7:10 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches, max.kokryashkin

Hi, Sergey!
Thanks for the fixes!
LGTM!


-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode.
  2023-11-13  7:04   ` Sergey Kaplun via Tarantool-patches
@ 2023-11-15  9:39     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-11-15  9:39 UTC (permalink / raw)
  To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches, max.kokryashkin

Hello, Sergey

commit message updated and force-pushed.

Sergey

On 11/13/23 10:04, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch.
> LGTM, after fixing my nits below.
>
> On 09.11.23, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Contributed by Sergey Bronnikov. #1105
> Minor: I suggest dropping #1105 here to avoid spamming to the ticket
> during development.

Fixed.


>
>> (cherry picked from commit e826d0c101d750fac8334d71e221c50d8dbe236c)
>>
>> The patch adds a file type `cc` that allows exporting byte code directly
>> to a file with extension `.cc`.
> Typo: s/extension/the extension/
Fixed.
>
> I suppose it is worth mentioning that the test will be added within
> future commits?
Added.
>
>> Sergey Bronnikov:
>> * added the description
>>
>> Part of tarantool/tarantool#9145
>> ---
> <snipped>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly
  2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
@ 2023-11-15 11:34     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-11-15 11:34 UTC (permalink / raw)
  To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches, max.kokryashkin

Hello, Sergey


please see my answers below

Updates applied and force-pushed.


On 11/13/23 10:10, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, after fixing my comments below.
>
> On 09.11.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> In testsuite `tarantool-tests` we are using CMake macro `BuildTestCLib`
> Typo: s/`tarantool-tests`/`tarantool-tests`,/
> Typo: s/CMake/the CMake/
Fixed.
>
>> for building test C libraries. This macro builds a list `TESTLIBS` that
>> contains all libraries required for tarantool tests. However, CMake
>> skips adding C library to `TESTLIBS` on building test C library in the
> Typo: s/adding C library/adding the C library/
> Nit: s/on/when/
> Typo: s/building test C library/building the test C library/
Fixed.
>
>> following commit.
>>
>> The patch removes `TESTLIBS` and adds each test C library as dependence
> Typo: s/dependence/a dependency/
Fixed.
>
>> to CMake target `tarantool-tests` explicitly using `add_dependence`
> Typo: s/using/using the/
Fixed.
>
>> command.
>> ---
>>   test/tarantool-tests/CMakeLists.txt | 17 +++++------------
>>   1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>> index c15d6037..d46e69a0 100644
>> --- a/test/tarantool-tests/CMakeLists.txt
>> +++ b/test/tarantool-tests/CMakeLists.txt
>> @@ -10,6 +10,10 @@ if(NOT PROVE)
>>     return()
>>   endif()
>>   
>> +add_custom_target(tarantool-tests
>> +  DEPENDS ${LUAJIT_TEST_BINARY}
>> +)
>> +
>>   macro(BuildTestCLib lib sources)
>>     add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources})
>>     target_include_directories(${lib} PRIVATE
>> @@ -37,12 +41,7 @@ macro(BuildTestCLib lib sources)
>>         CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}"
>>       )
>>     endif()
>> -  # XXX: Append the lib to be built to the dependency list.
>> -  # Unfortunately, there is no convenient way in CMake to extend
>> -  # the list in parent scope other than join two strings with
>> -  # semicolon. If one finds the normal way to make it work, feel
>> -  # free to reach me.
>> -  set(TESTLIBS "${lib};${TESTLIBS}" PARENT_SCOPE)
>> +  add_dependencies(tarantool-tests ${lib})
>>     # Add the directory where the lib is built to the list with
>>     # entries for LUA_CPATH environment variable, so LuaJIT can find
>>     # and load it. See the comment about extending the list in the
>> @@ -133,11 +132,6 @@ else()
>>     list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>>   endif()
>>   
>> -# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> I suppose that information about LUA_PATH and LD_LIBRARY_PATH may stay.

Reverted the hunk:


--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -132,6 +132,8 @@ else()
    list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
  endif()

+# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
+# with dependecies are set in scope of BuildTestLib macro.
  add_custom_command(TARGET tarantool-tests
    COMMENT "Running Tarantool tests"
    COMMAND

>> -# with dependecies are set in scope of BuildTestLib macro.
>> -add_custom_target(tarantool-tests
>> -  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
>> -)
>>   add_custom_command(TARGET tarantool-tests
>>     COMMENT "Running Tarantool tests"
>>     COMMAND
>> @@ -154,4 +148,3 @@ add_custom_command(TARGET tarantool-tests
>>         ${LUA_TEST_FLAGS}
>>     WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>>   )
>> -
>> -- 
>> 2.34.1
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave.
  2023-11-09 13:06 [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
                   ` (2 preceding siblings ...)
  2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 3/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
@ 2024-02-29  7:40 ` Igor Munkin via Tarantool-patches
  3 siblings, 0 replies; 10+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2024-02-29  7:40 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches

Sergey,

Thanks for the series! I've touched the patchset here[1] and there[2],
to make it LGTM.

On 09.11.23, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Changes in v2:
> 
> - added patch "Add 'cc' file type for saving bytecode."
> - updated building test libraries
> - add test for C library as well
> - small refactoring in test/tarantool-tests/CMakeLists.txt
> 
> PR: https://github.com/tarantool/tarantool/pull/9276
> Epic: https://github.com/tarantool/tarantool/issues/9145
> Issue: none
> 
> Mike Pall (2):
>   Add 'cc' file type for saving bytecode.
>   Fix C file generation in jit.bcsave.
> 
> Sergey Bronnikov (1):
>   test: set dependencies to tarantool-tests explicitly
> 
>  doc/running.html                              |  3 +-
>  src/jit/bcsave.lua                            |  6 +--
>  test/tarantool-tests/CMakeLists.txt           | 18 +++----
>  .../lj-551-bytecode-c-broken-macro.test.lua   | 16 +++++++
>  .../CMakeLists.txt                            | 47 +++++++++++++++++++
>  .../bcsaved.lua                               |  3 ++
>  6 files changed, 77 insertions(+), 16 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro.test.lua
>  create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/lj-551-bytecode-c-broken-macro/bcsaved.lua
> 
> -- 
> 2.34.1
> 

[1]: https://github.com/tarantool/luajit/commit/de66d19
[2]: https://github.com/tarantool/luajit/commit/01df6ae

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-02-29  7:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-09 13:06 [Tarantool-patches] [PATCH luajit 0/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 1/3][v2] Add 'cc' file type for saving bytecode Sergey Bronnikov via Tarantool-patches
2023-11-13  7:04   ` Sergey Kaplun via Tarantool-patches
2023-11-15  9:39     ` Sergey Bronnikov via Tarantool-patches
2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly Sergey Bronnikov via Tarantool-patches
2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
2023-11-15 11:34     ` Sergey Bronnikov via Tarantool-patches
2023-11-09 13:08 ` [Tarantool-patches] [PATCH luajit 3/3][v2] Fix C file generation in jit.bcsave Sergey Bronnikov via Tarantool-patches
2023-11-13  7:10   ` Sergey Kaplun via Tarantool-patches
2024-02-29  7:40 ` [Tarantool-patches] [PATCH luajit 0/3][v2] " Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox