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 B3732665EDF; Mon, 11 Sep 2023 11:50:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B3732665EDF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1694422232; bh=rHMpW8/Cfnysih3/mQEdb9h4iZW/UJ0dlAMXAvNOK14=; 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=curOnBIVmcZXJXNsMIHLPvhOuLWv1zI/oHjxk2KxB8rpvgRXVRVAsgdFyyIZljiup 9qqUBMcXI4DguoNuEw5x/lgrnRgAFQjxGczJVfiKuIqxv2YpuXxobKv+nFdjtXFPbh rHPjaF1yJEUWChcxsMw+LP+5QMOS2y0+RD1+B9Bc= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (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 D22785DF790 for ; Mon, 11 Sep 2023 11:50:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D22785DF790 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1qfccn-00GpYA-17; Mon, 11 Sep 2023 11:50:29 +0300 Date: Mon, 11 Sep 2023 11:45:46 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org, max.kokryashkin@gmail.com Message-ID: References: <272d18bd-5333-1224-31c3-8d59475f6afc@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <272d18bd-5333-1224-31c3-8d59475f6afc@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96E142CFC92DB15CD90AAA51E9363FBE6A21DF977184D824F182A05F538085040ADE0E844E9740C22CF667F48D882D49F04B3035153F332EA399034577E89211D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78981306C6E927004EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E893C22CB255350D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8CCB5BB9B339367F3ACD6E239E520D98D117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC6CE7A4E25BAF3B969FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3776A0366D588B3C3117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CFC31085E2DFA87778BA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B6EDF998CB16CCEE6976E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249957A4DEDD2346B42E827F84554CEF50127C277FBC8AE2E8B2EE5AD8F952D28FBAAAE862A0553A39223F8577A6DFFEA7CA5583D5FF21A78E543847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A57723623E3B71DD8988CC190F78E29D7394DCD9D07DA569F0F87CCE6106E1FC07E67D4AC08A07B9B062B3BD3CC35DA588CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34C1E32F4AD4B2486BC5E2B07B85A9B820505DD9CF84597C121718E2260BE02B704A52F9EC90A31CA91D7E09C32AA3244CD7813E199EB81583CE04840D786844B9F94338140B71B8EEBAD658CF5C8AB4025DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGCuXuOfYMcbWeB2tqFlqdw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769F21C8A9B14F6A860CF667F48D882D49F49EBE2174CCC9479DEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader. 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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the fixes! On 07.09.23, Sergey Bronnikov wrote: > Hi, Sergey > > > On 9/5/23 17:10, Sergey Kaplun wrote: > > Hi, Sergey! > > Thanks for the patch! > > Please, consider my comments below. > > > > On 31.08.23, Sergey Bronnikov wrote: > >> From: Sergey Bronnikov > >> > > > >> +end > >> + > >> +local function get_file_name(file) > >> + return file:match("[^/]*$") > > Minor: it may match the empty string for a directory occasionally: > > | src/luajit -e 'print([["]]..("/tmp/"):match("[^/]*$")..[["]])' > > > Fixed. > > > | "" > > > > Nit: use single quotes instead of double quotes if possible. > > Without context it is difficult to get what is line you talk about. > > As I see everything is fine with quotes in version on the branch. > I'm talking about the same line (got this version from the branch): | +local function basename(path) | + return path:match("[^/]*$") | +end > > > > > Nit: `[^/\\]` is better since it also covers Windows. > > See > > | local dirname = arg[0]:gsub('([^/\\]+)$', '') > > Since we don't support Windows feel free to ignore. > > > >> +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) > > Minor: I suppose it should be renamed to `path_shared`, since on macOS > > they have the ".dyld" suffix for shared libs. Hence, we need to use the > > suffix in format of the shared library name too. You may take some > > inspiration from here [1]. > Fixed. Hmm, I don't see these updates on the branch [1]. Maybe you forgot to push your local changes to the repository (*)? > >> + > >> +local basedir = function(path) > >> + local sep = '/' > > Why do we need an additional variable here? > > For clarity. OK. > > > > > > Nit: Indent is 4 spaces instead of 2. > > > >> + return path:match('(.*' .. sep .. ')') or './' > > It's better to mention that the pattern matching is greedy, so we match > > until the last separator. > > > Updated. This is missing too (*). > > >> +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`. > > Nit: comment line width is more than 66 symbols > > Fixed. > > > >> +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)) > > Minor: The test flow is a little bit hard to read due to function > > declarations. Maybe it is better to declare all utility functions first > > and then use them one by one? This makes control flow easier to read. > Rearranged, take a look please. Yes, this is muchc clearer, thanks! > > > >> + > > > >> + -- It is required to cleanup LUA_PATH, otherwise > >> + -- LuaJIT loads Lua module, see tarantool-tests/utils/init.lua. > > Nit: comment line width is more than 66 symbols > Fixed too. > > > > Actually I don't understand from the comment what Lua module exactly is > > loaded. Maybe it's better to fix this behaviour? > What behaviour you want to fix? That some non-existing module was loaded with `makecmd()`, but since we have a workaround, maybe it isn't critical. Anyway, we should do it in a separate patch set. Feel free to ignore, as I've said. > > Feel free to ignore. > > > >> + LUA_PATH = '', > > [1]: https://github.com/tarantool/luajit/blob/f7e5e8abe396411065f8941d04879577e7fae175/test/tarantool-tests/lj-549-bytecode-loader.test.lua#L54 -- Best regards, Sergey Kaplun