Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>,
	Sergey Bronnikov <estetus@gmail.com>
Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader.
Date: Mon, 4 Sep 2023 12:31:21 +0300	[thread overview]
Message-ID: <1011cc5a-a628-450d-8c29-25972489bf0e@tarantool.org> (raw)
In-Reply-To: <ukoja5u3qk7rg3hn2j7gckacvcv7mwrxaejzzlm7ccto4btu7t@uza5lsm3eqij>

Hi, Maxim

On 9/1/23 12:42, Maxim Kokryashkin via Tarantool-patches wrote:


<snipped>

>> diff --git a/test/tarantool-tests/lj-549-bytecode-loader.test.lua b/test/tarantool-tests/lj-549-bytecode-loader.test.lua
>> new file mode 100644
>> index 00000000..889be80a
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-549-bytecode-loader.test.lua
>> @@ -0,0 +1,96 @@
>> +local tap = require('tap')
>> +local ffi = require('ffi')
>> +local utils = require('utils')
>> +local test = tap.test('lj-549-bytecode-loader'):skipcond({
>> +    -- ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
> Why this skipcond is commented out?

it is not needed anymore, so I removed it. Iterative patch is below:


--- a/test/tarantool-tests/lj-549-bytecode-loader.test.lua
+++ b/test/tarantool-tests/lj-549-bytecode-loader.test.lua
@@ -1,9 +1,7 @@
  local tap = require('tap')
  local ffi = require('ffi')
  local utils = require('utils')
-local test = tap.test('lj-549-bytecode-loader'):skipcond({
-    -- ['Test requires GC64 mode enabled'] = not 
require('ffi').abi('gc64'),
-})
+local test = tap.test('lj-549-bytecode-loader')

  test:plan(1)

>> +})
>> +
>> +test:plan(1)
>> +
>> +-- Test creates a shared library with LuaJIT bytecode,
>> +-- loads shared library as a Lua module and checks,
>> +-- that no crashes eliminated.
>> +--
>> +-- $ make HOST_CC='gcc -m32' TARGET_CFLAGS='-m32' \
>> +--                           TARGET_LDFLAGS='-m32' \
>> +--                           TARGET_SHLDFLAGS='-m32' \
>> +--                           -f Makefile.original
>> +-- $ echo 'print("test")' > a.lua
>> +-- $ LUA_PATH="src/?.lua;;" luajit -b a.lua a.c
>> +-- $ gcc -m32 -fPIC -shared a.c -o a.so
>> +-- $ luajit -e "require('a')"
>> +-- Program received signal SIGBUS, Bus error
>> +
>> +local function file_exists(fname)
>> +   return io.open(fname, 'r') or true and false
>> +end
>> +
>> +local function get_file_name(file)
>> +    return file:match("[^/]*$")
>> +end
>> +
>> +local stdout_msg = 'Lango team'
>> +local lua_code = ('print(%q)'):format(stdout_msg)
>> +local fpath = os.tmpname()
>> +local path_lua = ('%s.lua'):format(fpath)
>> +local path_c = ('%s.c'):format(fpath)
>> +local path_so = ('%s.so'):format(fpath)
>> +
>> +-- Create a file with a minimal Lua code.
>> +local fh = assert(io.open(path_lua, 'w'))
>> +fh:write(lua_code)
>> +fh:close()
>> +
>> +local module_name = assert(get_file_name(fpath))
>> +
>> +local basedir = function(path)
>> +    local sep = '/'
>> +    return path:match('(.*' .. sep .. ')') or './'
>> +end
>> +
>> +-- Create a C file with LuaJIT bytecode.
>> +-- We cannot use utils.makecmd, because command-line generated
>> +-- by `makecmd` contains `-e` that is incompatible with option `-b`.
>> +local function create_c_file(pathlua, pathc)
>> +  local lua_path = os.getenv('LUA_PATH')
>> +  local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
>> +  local cmd_fmt = 'LUA_PATH="%s" %s -b %s %s'
>> +  local cmd = (cmd_fmt):format(lua_path, lua_bin, pathlua, pathc)
>> +  local ret = os.execute(cmd)
>> +  assert(ret == 0, 'create a C file with bytecode')
>> +end
>> +
>> +create_c_file(path_lua, path_c)
>> +assert(file_exists(path_c))
>> +
>> +-- Compile C source code with LuaJIT bytecode to a shared library.
>> +-- `-m64` is not available on ARM64, see
>> +-- "3.18.1 AArch64 Options in the manual",
>> +-- https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html
>> +local cflags_64 = jit.arch == 'arm64' and '-march=armv8-a' or '-m64'
>> +local cflags = ffi.abi('32bit') and '-m32' or cflags_64
>> +local cc_cmd = ('cc %s -fPIC -shared %s -o %s'):format(cflags, path_c, path_so)
>> +local ph = io.popen(cc_cmd)
>> +ph:close()
> I suggest using the os.execute and checking the exit code.
> Popen is excessive here.

replaced, iterative patch is below

--- a/test/tarantool-tests/lj-549-bytecode-loader.test.lua
+++ b/test/tarantool-tests/lj-549-bytecode-loader.test.lua
@@ -68,8 +68,8 @@ assert(file_exists(path_c))
  local cflags_64 = jit.arch == 'arm64' and '-march=armv8-a' or '-m64'
  local cflags = ffi.abi('32bit') and '-m32' or cflags_64
  local cc_cmd = ('cc %s -fPIC -shared %s -o %s'):format(cflags, path_c, 
path_so)
-local ph = io.popen(cc_cmd)
-ph:close()
+local rc = os.execute(cc_cmd)
+assert(rc == 0)
  assert(file_exists(path_so))

  -- Load shared library as a Lua module.

>
>> +assert(file_exists(path_so))
>> +
>> +-- Load shared library as a Lua module.
>> +local lua_cpath = ('"/tmp/?.so;"'):format(basedir(fpath))
>> +assert(file_exists(path_so))
>> +local cmd = utils.exec.makecmd(arg, {
>> +    script = ('-e "require([[%s]])"'):format(module_name),
>> +    env = {
>> +        LUA_CPATH = lua_cpath,
>> +        -- It is required to cleanup LUA_PATH, otherwise
>> +        -- LuaJIT loads Lua module, see tarantool-tests/utils/init.lua.
>> +        LUA_PATH = '',
>> +    },
>> +})
>> +local res = cmd()
>> +test:ok(res == stdout_msg, 'bytecode loader works')
>> +
>> +os.remove(path_lua)
>> +os.remove(path_c)
>> +os.remove(path_so)
>> +
>> +os.exit(test:check() and 0 or 1)
> `test:done` should be used instead.

Fixed.


@@ -91,4 +91,4 @@ os.remove(path_lua)
  os.remove(path_c)
  os.remove(path_so)

-os.exit(test:check() and 0 or 1)
+test:done(true)

>> -- 
>> 2.34.1
>>

  reply	other threads:[~2023-09-04  9:31 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-31 11:29 [Tarantool-patches] [PATCH luajit 0/2][v2] " Sergey Bronnikov via Tarantool-patches
2023-08-31 11:30 ` [Tarantool-patches] [PATCH luajit 1/2][v2] " Sergey Bronnikov via Tarantool-patches
2023-08-31 11:49   ` Sergey Bronnikov via Tarantool-patches
2023-09-01  9:42   ` Maxim Kokryashkin via Tarantool-patches
2023-09-04  9:31     ` Sergey Bronnikov via Tarantool-patches [this message]
2023-09-05  6:34       ` Maxim Kokryashkin via Tarantool-patches
2023-09-05 14:10   ` Sergey Kaplun via Tarantool-patches
2023-09-07 15:21     ` Sergey Bronnikov via Tarantool-patches
2023-09-11  8:45       ` Sergey Kaplun via Tarantool-patches
2023-09-12 10:20         ` Sergey Bronnikov via Tarantool-patches
2023-10-31 11:30           ` Sergey Kaplun via Tarantool-patches
2023-09-05 14:12   ` Sergey Kaplun via Tarantool-patches
2023-09-07  7:06     ` Sergey Bronnikov via Tarantool-patches
2023-08-31 11:32 ` [Tarantool-patches] [PATCH luajit 2/2][v2] Followup fix for " Sergey Bronnikov via Tarantool-patches
2023-09-01 10:05   ` Maxim Kokryashkin via Tarantool-patches
2023-09-04 16:34     ` Sergey Bronnikov via Tarantool-patches
2023-09-05  6:45       ` Maxim Kokryashkin via Tarantool-patches
2023-09-05 12:55   ` Sergey Kaplun via Tarantool-patches
2023-09-07  7:04     ` Sergey Bronnikov via Tarantool-patches
2023-09-11  9:26       ` Sergey Kaplun via Tarantool-patches
2023-09-12 10:30         ` Sergey Bronnikov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1011cc5a-a628-450d-8c29-25972489bf0e@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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