From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A19BB57A5CA; Tue, 31 Oct 2023 12:57:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A19BB57A5CA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1698746250; bh=f6CXVcD1x9JsJ+GZm6topNP/ofY3A5QpGO3EgQVgjVw=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=rsqb6tBfQg50Yf0DAdXfM9uFhSOJO53+qCDWMhAbc5JD0kf6MNMgumnUrLWXMYgO0 ceJJ/Hz340mtnqDENUL+Cce+d604Tv/6cYoJw+ApTziQDSLTZjgS0VTgtdAG0V6QdK EUfQSgqmRb0fD3QIempod2QXXn/4dOB4dJicAG54= Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [95.163.41.90]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9D3D857A5CA for ; Tue, 31 Oct 2023 12:57:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9D3D857A5CA Received: by smtp53.i.mail.ru with esmtpa (envelope-from ) id 1qxlV1-009njJ-1c; Tue, 31 Oct 2023 12:57:28 +0300 Message-ID: Date: Tue, 31 Oct 2023 12:57:27 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org, max.kokryashkin@gmail.com References: Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9C931D0C4A91E713033D93D6C96D04E910A66EE0728485C3300894C459B0CD1B9B9F2B91F1D4B4E49AED2D9ED7A006A7755B11E0FF8EA7D0EAE7198D1B9DA4060 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7207CDA5E227C0D0BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637801520E8C0CFD3888638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8471F624B54709EF0F1D82D0F1FFAB1D3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC60CDF180582EB8FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0AC5B80A05675ACD8F49F126DDB898E8D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3E3786DD2C77EBDAA9735652A29929C6CC4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB861051D4BA689FC2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FF841BD135A60A317A91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A58EFAA960D32846558CE697E220F700F86EF70D9CB5795A30F87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF637DE5C75CB2BAC698E08AF3B80359F3F36D8BA8B3763184EFC351FACBEBC3D835D245DF744597F1516A7BA5748856627EC9F84421603F6ED3A50CF76B3FB5F8A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPJ6ruIekffL2sLvsRkj7Eg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A76951E407029173B2540EAF68833FF92D9A21401CF93FD6FA55EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix C file generation in jit.bcsave. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 >> >> 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 0000000000002000 R luaJIT_BC_bcsaved_clib 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