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 1474E674153; Mon, 23 Oct 2023 14:17:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1474E674153 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1698059851; bh=qMGPp90kTJOeYNKWIm8TlXIL7d//YNnZSzt++Y4701s=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=PGZ3WGddwtFWxUJ+qbCsfPq3GWZ9VdLB/xZAT3Puy+vdryKw2LIMk62NCGKphOjaR KsyHGwEkm1+PDDASRpjzrvcsXiuYkxgepdbkFFoZP6ZzdkWQ8PRfNY+ypdL83DNqFA DpG+aX5RTSvFVlUsarLTRbkrgFJxsv/u06GlZZGU= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 10019674153 for ; Mon, 23 Oct 2023 14:17:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 10019674153 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1qusw4-0000nY-2W; Mon, 23 Oct 2023 14:17:28 +0300 Date: Mon, 23 Oct 2023 14:12:59 +0300 To: Sergey Bronnikov Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9C197A170B57C5E43C7ADFC575A26AA7F794C4B0F5EEF40A000894C459B0CD1B99FA20D02EB16FE99326F3F3BE06DE6D91865EA71FEC04B69BB3D1907BEA9FCEE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE754E958D11587BC68EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373D332FFE8BBF4EB58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D806416D4B7702BF6E88464E20BBCF57AA117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC55D5BE2F85BDEC5FA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F28451B159A507268D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C0406C186E56A1B26068655334FD4449CB33AC447995A7AD1857739F23D657EF2BD5E8D9A59859A8B67122B551D37F75FD089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5256340BE45DE886072BE6A20A6E92E6A3A6B84EFDB8A853EF87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF89088439A1A612F5A9725264D185D59BD6CCBF3F941B2E33A026BEBCD45DAB17A2F8A0A94712AD9DA8B50B9382ADB12AA38BA8246C6A032CA2B42C49228FB3FBA74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqlVu258LHAGTfkb88AaOfw== X-DA7885C5: 477A8F68B82F7BF5AD8F32F0A0AE253B5EBF2F44B586D288382A54D05C56824B262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B4712E650C91F1D0DFE913B28004F482D0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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/ > 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