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 807775630AE; Tue, 1 Aug 2023 12:53:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 807775630AE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1690883603; bh=/tnflDFY3sXSQSLRgHxPB5ZrQ1ybGtaNaMO9ZzTuO08=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=FmQB4xeQiwaRQaC/lRfvUJnY5TtBO5/Cr7YOQlWrV7JzzF4hrxUSlDuc6T1F8gn0+ NSBLbC0qAjixPsbVu643ZH5s1cV5M11PdnQqrQ4q7ZzTfuXiAvZ1fK3BDLLKnowC3Y JDVsdq1H7OHpYk8e7oYuM5Ki7F+0t+KsnlyASAT8= Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [95.163.41.76]) (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 38BFD54734D for ; Tue, 1 Aug 2023 12:53:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 38BFD54734D Received: by smtp35.i.mail.ru with esmtpa (envelope-from ) id 1qQm48-0097oO-1S for tarantool-patches@dev.tarantool.org; Tue, 01 Aug 2023 12:53:21 +0300 Message-ID: Date: Tue, 1 Aug 2023 12:53:20 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: tarantool-patches@dev.tarantool.org References: <738d30841b4fb39c094e3fe53317c8f7915e268a.1690300762.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD920919EE866B1AD24D9883F3FE6AB02BF9D9F339A451BA04F00894C459B0CD1B979799C92A401B95105629D636B2CB54A29429DFB68813368DB5804FDCCB62C7C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B264C8851FD8E810EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370BACBAB4C30C4AEB8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89847ABFB39C48D5AF4C10A0F4B549E7B117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAE9A1BBD95851C5BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735204B6963042765DA4BE5D25F19253116ADD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE26055571C92BF10FB1CA5D0BF4193578D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE32D01283D1ACF37BA6E0066C2D8992A16C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CD2DCF9CF1F528DBC2E808ACE2090B5E1725E5C173C3A84C327ED053E960B195E089D37D7C0E48F6C8AA50765F79006378534F7D0DA3B5694EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A56536DFA68D22020E767CB3D5DBF0ECADDBA333413C0F5145F87CCE6106E1FC07E67D4AC08A07B9B06A1CB4668A9CA5FACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742D3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFAC0FF58578A69F7453B076E76FD6C9072421E8493645287951359AEE7EB32BCFACEF76868E8EE94E337391BE08E8D56EF9E435B4370828D761D93033A3824A10CAACD699CDC6F98202C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxgaGEV50waLycHCebLfI+g== X-Mailru-Sender: 49D287FBCBBF3A5C10E199891D96F0693F41091CCC9B9206FB6DDE3D39B5DFE3B0D440EF7BA41D00645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/2] Followup fix for 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Max! thanks for your comments. See my answers. On 7/31/23 15:20, Maxim Kokryashkin via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On Tue, Jul 25, 2023 at 07:37:01PM +0300, Sergey Bronnikov via Tarantool-patches wrote: >> From: sergeyb@tarantool.org >> >> (cherry-picked from commit e49863eda13d095b1a78fd4ca0fd3a6a9a17d782) > The description is missing. As we've already discussed offline, it is generally > not a good practice to describe a commit in the previous commit. So it would be > great if you moved the relevant part of the description here instead. Description added to the second commit message as well. > >> Sergey Bronnikov: >> * added the partial test for this and a previous patch >> --- >> src/lj_lex.c | 1 + >> test/tarantool-c-tests/lj-549-lua_load.test.c | 146 ++++++++++++++++++ >> 2 files changed, 147 insertions(+) >> create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c >> >> diff --git a/src/lj_lex.c b/src/lj_lex.c >> index 82e4ba6f..161d862e 100644 >> --- a/src/lj_lex.c >> +++ b/src/lj_lex.c >> @@ -51,6 +51,7 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls) >> if (sz >= LJ_MAX_BUF) { >> if (sz != ~(size_t)0) lj_err_mem(ls->L); >> sz = ~(uintptr_t)0 - (uintptr_t)p; >> + if (sz >= LJ_MAX_BUF) sz = LJ_MAX_BUF-1; >> ls->endmark = 1; >> } >> ls->pe = p + sz; >> diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c b/test/tarantool-c-tests/lj-549-lua_load.test.c >> new file mode 100644 >> index 00000000..4fb144cf >> --- /dev/null >> +++ b/test/tarantool-c-tests/lj-549-lua_load.test.c > As I have already mentioned in the previous patch, it passes with no problems > whatsoever before the patch on x32 machine, so I am not sure we need to add it. Test for a public function is better than nothing. I would left it, but we can also wait for opinion of the second reviewer and then decide. > >> @@ -0,0 +1,146 @@ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> + >> +#include "test.h" >> +#include "utils.h" >> + >> +/* Need for skipcond. */ >> +#include "lj_arch.h" >> + >> +/* Defined in lj_def.h. */ >> +#define LJ_MAX_MEM32 0x7fffff00 /* Max. 32 bit memory allocation. */ >> +#define LJ_MAX_BUF LJ_MAX_MEM32 /* Max. buffer length. */ >> + >> +#define UNUSED(x) ((void)(x)) >> + >> +/** >> + * Array with bytecode was generated using commands below: >> + * >> + * cat << EOF > a.lua >> + * local a = 1 >> + * EOF >> + * luajit -b a.lua a.h >> + */ >> +#define luaJIT_BC_sample_SIZE 22 >> +static const unsigned char luaJIT_BC_sample[] = { >> +27,76,74,2,2,15,2,0,1,0,0,0,2,41,0,1,0,75,0,1,0,0 > Missing spaces. Fixed. >> +}; >> + >> +/** >> + * Function generates a huge chunk with "bytecode" with a size bigger than > Typo: s/with "bytecode"/of "bytecode"/ Fixed. >> + * LJ_MAX_BUF. Generated chunk must enable endmark in a Lex state. >> + */ >> +static const char * >> +bc_reader_with_endmark(lua_State *L, void *data, size_t *size) >> +{ >> + UNUSED(data); >> + static char *bc_chunk = NULL; >> + free(bc_chunk); >> + >> + int bc_chunk_size = (size_t)0; > We ussually follow the C89 standard, which mentions that variables must be > declared in the beginning of the block. Here and below. Fixed. >> + bc_chunk = malloc(bc_chunk_size); >> + assert(bc_chunk != NULL); >> + >> + /** >> + * Put a chunk with a valid bytecode to the beginning of allocated region. > Typo: s/to the beginning/at the beginning/ > Typo: s/of allocated/of the allocated/ Fixed. >> + * lua_load automatically detects whether the chunk is text or binary, > Typo: s/lua_load/`lua_load`/ Feel free to ignore, though. Fixed. >> + * and loads it accordingly. We need a trace for bytecode input, >> + * so it is necessary to deceive a check in lj_lex_setup, that makes a >> + * sanity check and detects whether input is bytecode or text by first char. > Typo: s/by first/by the first/ Fixed. >> + * Strictly speaking it is enough to put LUA_SIGNATURE[0] as a first > Typo: s/Strictly speaking/Strictly speaking,/ > Typo: s/a first/the first/ Fixed. >> + * symbol in produced chunk. > Typo: s/in produced/in the produced/ Fixed. >> + */ >> + memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE); >> + >> + *size = bc_chunk_size; >> + >> + return bc_chunk; >> +} >> + >> +static int bc_loader_with_endmark(void *test_state) >> +{ >> + lua_State *L = test_state; >> + void *ud = NULL; >> + int res = lua_load(L, bc_reader_with_endmark, ud, "endmark"); >> + >> + /* Make sure we passed condition with lj_err_mem in a function lex_more. */ > Typo: s/passed condition/passed the condition/ > Typo: s/in a function/in the function/ Fixed. >> + assert_true(res != LUA_ERRMEM); >> + >> + return TEST_EXIT_SUCCESS; >> +} >> + >> +enum bc_emission_state { >> + EMIT_BC, >> + EMIT_EOF, >> +}; >> + >> +typedef struct { >> + enum bc_emission_state state; >> +} dt; >> + >> +/** >> + * Function returns bytecode chunk on the first call and NULL and size equals > Typo: s/returns bytecode/returns a bytecode/ > Typo: s/equals/equal/ Fixed. >> + * to zero on the second call. Triggers END_OF_STREAM flag in a function > Typo: s/Triggers/Triggers the/ > Typo: s/in a function/in the function/ Fixed. >> + * lex_more. >> + */ >> +static const char * >> +bc_reader_with_eof(lua_State *L, void *data, size_t *size) >> +{ >> + UNUSED(data); >> + UNUSED(L); >> + dt *test_data = (dt *)data; >> + if (test_data->state == EMIT_EOF) { >> + *size = 0; >> + return NULL; >> + } >> + >> + char *bc_chunk = malloc(luaJIT_BC_sample_SIZE); >> + memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE); >> + *size = luaJIT_BC_sample_SIZE; >> + >> + return bc_chunk; >> +} >> + >> +static int bc_loader_with_eof(void *test_state) >> +{ >> + lua_State *L = test_state; >> + dt test_data = {0}; >> + test_data.state = EMIT_BC; >> + int res = lua_load(L, bc_reader_with_eof, &test_data, "eof"); >> + assert_true(res = LUA_ERRSYNTAX); >> + if (res == LUA_OK) { >> + lua_pcall(L, 0, 0, 0); >> + } >> + >> + return TEST_EXIT_SUCCESS; >> +} >> + >> +int main(void) >> +{ >> + if (LJ_GC64 || !LUAJIT_ARCH_X64 || !LJ_TARGET_LINUX) >> + /** >> + * lua_load source code is common on all platforms, >> + * when bytecode is not portable. >> + * So test runs on Linux/x86_64 only and skipped on other > Typo: s/So test/So the test/ > Typo: s/and skipped/and is skipped/ Fixed. >> + * platforms. >> + */ >> + return skip_all("Enabled on Linux/x86_64 with disabled GC64"); > We prefer to run tests like that on all paltforms, just to be sure. We usually > exclude tests from platforms only if those are completely irrelevant to the test, > or there is some kind of major issue. Yes, for this test the rule was ignored because bytecode is not portable and following the rule will make maintenance difficult. Therefore supported platforms limited by x86_64 only. >> + >> + lua_State *L = utils_lua_init(); >> + const struct test_unit tgroup[] = { >> + test_unit_def(bc_loader_with_endmark), >> + test_unit_def(bc_loader_with_eof) >> + }; >> + >> + const int test_result = test_run_group(tgroup, L); >> + utils_lua_close(L); >> + return test_result; >> +} >> -- >> 2.34.1 >>