From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader. Date: Tue, 1 Aug 2023 12:53:20 +0300 [thread overview] Message-ID: <d3389ce1-dbd1-9a4d-0b80-fed78c35d5f9@tarantool.org> (raw) In-Reply-To: <twtiwmws2usa7m6hgvvhhdx3vjpl2cwrqcq3cqh5dgtpahq4ew@533fncu3hriu> 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 <assert.h> >> +#include <stdint.h> >> +#include <stddef.h> >> +#include <string.h> >> +#include <stdlib.h> >> +#include <stdio.h> >> + >> +#include <lua.h> >> +#include <lualib.h> >> +#include <lauxlib.h> >> + >> +#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 >>
next prev parent reply other threads:[~2023-08-01 9:53 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-07-25 16:34 [Tarantool-patches] [PATCH 0/2] Fix " Sergey Bronnikov via Tarantool-patches 2023-07-25 16:36 ` [Tarantool-patches] [PATCH 1/2] " Sergey Bronnikov via Tarantool-patches 2023-07-31 12:01 ` Maxim Kokryashkin via Tarantool-patches 2023-08-01 9:56 ` Sergey Bronnikov via Tarantool-patches 2023-08-06 11:09 ` Sergey Kaplun via Tarantool-patches 2023-08-15 8:51 ` Maxim Kokryashkin via Tarantool-patches 2023-07-25 16:37 ` [Tarantool-patches] [PATCH 2/2] Followup fix for " Sergey Bronnikov via Tarantool-patches 2023-07-31 12:20 ` Maxim Kokryashkin via Tarantool-patches 2023-08-01 9:53 ` Sergey Bronnikov via Tarantool-patches [this message] 2023-08-14 8:14 ` 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=d3389ce1-dbd1-9a4d-0b80-fed78c35d5f9@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergeyb@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] Followup fix for 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