From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <max.kokryashkin@gmail.com>, tarantool-patches@dev.tarantool.org, skaplun@tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v1] profilers: print user-friendly errors Date: Thu, 4 Apr 2024 14:19:26 +0300 [thread overview] Message-ID: <e68f9acc-1d94-41be-a246-4dcc0bab31b2@tarantool.org> (raw) In-Reply-To: <20240326160940.339283-1-m.kokryashkin@tarantool.org> Hi, Max thanks for the patch! LGTM with minor comments. On 3/26/24 19:09, Maxim Kokryashkin wrote: > Prior to this patch, there was no error-checking in profilers, > which resulted in raw Lua errors being reported in cases of > non-existing paths or corrupt file structures. This patch > makes use of the event reader module introduced in commit > df5a07218dba8cefa370499a1a54ba75fb73b99b ("profilers: introduce > event reader module") to add error handling, so all parsing > errors are now reported in a user-friendly manner. > > Tool CLI flag tests are adapted correspondingly to error message > changes. > > Resolves tarantool/tarantool#9217 > Part of tarantool/tarantool#5994 > --- > Branch: https://github.com/tarantool/luajit/tree/mkokryashkin/profile-parsers-refactoring-p2 > > .../gh-5688-tool-cli-flag.test.lua | 4 +- > ...17-profile-parsers-error-handling.test.lua | 90 +++++++++++++++++++ > tools/memprof.lua | 11 +-- > tools/memprof/parse.lua | 5 +- > tools/sysprof.lua | 10 +-- > tools/sysprof/parse.lua | 5 +- > tools/utils/symtab.lua | 6 +- > 7 files changed, 112 insertions(+), 19 deletions(-) > create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua > > diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > index 75293f11..8ead83b5 100644 > --- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > @@ -42,7 +42,7 @@ local SMOKE_CMD_SET = { > local MEMPROF_CMD_SET = { > { > cmd = MEMPROF_PARSER .. BAD_PATH, > - like = 'fopen, errno: 2', > + like = 'Failed to open.*fopen, errno: 2', > }, > { > cmd = MEMPROF_PARSER .. TMP_BINFILE_MEMPROF, > @@ -61,7 +61,7 @@ local MEMPROF_CMD_SET = { > local SYSPROF_CMD_SET = { > { > cmd = SYSPROF_PARSER .. BAD_PATH, > - like = 'fopen, errno: 2', > + like = 'Failed to open.*fopen, errno: 2', > }, > { > cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF, > diff --git a/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua > new file mode 100644 > index 00000000..c2e0f0a6 > --- /dev/null > +++ b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua > @@ -0,0 +1,90 @@ > +local tap = require('tap') > +local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({ > + ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and > + jit.arch ~= 'x64', > + ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux', > + -- XXX: Tarantool integration is required to run this test properly. > + -- luacheck: no global > + ['No profile tools CLI option integration'] = _TARANTOOL, > +}) > + > +jit.off() > +jit.flush() > + > +local table_new = require('table.new') > +local utils = require('utils') > + > +local BAD_PATH = utils.tools.profilename('bad-path-tmp.bin') > +local NON_PROFILE_DATA = utils.tools.profilename('not-profile-data.tmp.bin') > +local CORRUPT_PROFILE = utils.tools.profilename('profdata.tmp.bin') > + > +local EXECUTABLE = utils.exec.luacmd(arg) > +local PARSERS = { > + memprof = EXECUTABLE .. ' -tm ', > + sysprof = EXECUTABLE .. ' -ts ', > +} > +local REDIRECT_OUTPUT = ' 2>&1' > + > +local TABLE_SIZE = 20 > + > +local TEST_CASES = { > + { > + path = BAD_PATH, > + err_msg = 'Failed to open' > + }, > + { > + path = NON_PROFILE_DATA, > + err_msg = 'Failed to parse symtab from' > + }, > + { > + path = CORRUPT_PROFILE, > + err_msg = 'Failed to parse profile data from' > + }, Nit: I would rename CORRUPT_PROFILE to CORRUPT_PROFILE_DATA for consitency with NON_PROFILE_DATA. > +} > + > +test:plan(2 * #TEST_CASES) > + > +local function generate_non_profile_data(path) > + local file = io.open(path, 'w') > + file:write('data') > + file:close() > +end > + > +local function generate_corrupt_profile(path) Nit: generate_corrupt_profile -> generate_corrupt_profile_data > + local res, err = misc.memprof.start(path) > + -- Should start successfully. > + assert(res, err) > + > + local _ = table_new(TABLE_SIZE, 0) > + _ = nil > + collectgarbage() > + > + res, err = misc.memprof.stop() > + -- Should stop successfully. > + assert(res, err) > + > + local file = io.open(path, 'r') > + local content = file:read('*all') > + file:close() > + local index = string.find(content, 'ljm') > + > + file = io.open(path, 'w') > + file:write(string.sub(content, 1, index - 1)) > + file:close() > +end > + > +generate_non_profile_data(NON_PROFILE_DATA) > +generate_corrupt_profile(CORRUPT_PROFILE) > + > +for _, case in ipairs(TEST_CASES) do > + for profiler, parser in pairs(PARSERS) do > + local path = case.path > + local err_msg = case.err_msg > + local output = io.popen(parser .. path .. REDIRECT_OUTPUT):read('*all') > + test:like(output, err_msg, string.format('%s: %s', profiler, err_msg)) > + end > +end > + > +os.remove(NON_PROFILE_DATA) > +os.remove(CORRUPT_PROFILE) > +test:done(true) > diff --git a/tools/memprof.lua b/tools/memprof.lua > index 537cb869..7d7e8e05 100644 > --- a/tools/memprof.lua > +++ b/tools/memprof.lua > @@ -10,10 +10,9 @@ > -- Major portions taken verbatim or adapted from the LuaVela. > -- Copyright (C) 2015-2019 IPONWEB Ltd. > > -local bufread = require "utils.bufread" > local memprof = require "memprof.parse" > local process = require "memprof.process" > -local symtab = require "utils.symtab" > +local evread = require "utils.evread" > local view = require "memprof.humanize" > > local stdout, stderr = io.stdout, io.stderr > @@ -108,9 +107,11 @@ local function parseargs(args) > end > > local function dump(inputfile) > - local reader = bufread.new(inputfile) > - local symbols = symtab.parse(reader) > - local events = memprof.parse(reader, symbols) > + -- XXX: This function exits with a non-zero exit code and > + -- prints an error message if it encounters any failure during > + -- the process of parsing. > + local events, symbols = evread(memprof.parse, inputfile) > + > if not config.leak_only then > view.profile_info(events, config) > end > diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua > index cc66a23e..11a79a1d 100644 > --- a/tools/memprof/parse.lua > +++ b/tools/memprof/parse.lua > @@ -206,12 +206,13 @@ function M.parse(reader, symbols) > local _ = reader:read_octets(3) > > if magic ~= LJM_MAGIC then > - error("Bad LJM format prologue: "..magic) > + error("Bad memprof event format prologue: "..magic) > end > > if string.byte(version) ~= LJM_CURRENT_VERSION then > error(string_format( > - "LJM format version mismatch: the tool expects %d, but your data is %d", > + "Memprof event format version mismatch:".. > + " the tool expects %d, but your data is %d", > LJM_CURRENT_VERSION, > string.byte(version) > )) > diff --git a/tools/sysprof.lua b/tools/sysprof.lua > index 8449b23f..420ca41c 100644 > --- a/tools/sysprof.lua > +++ b/tools/sysprof.lua > @@ -1,6 +1,5 @@ > -local bufread = require "utils.bufread" > +local evread = require "utils.evread" > local sysprof = require "sysprof.parse" > -local symtab = require "utils.symtab" > > local stdout, stderr = io.stdout, io.stderr > local match, gmatch = string.match, string.gmatch > @@ -78,9 +77,10 @@ local function parseargs(args) > end > > local function dump(inputfile) > - local reader = bufread.new(inputfile) > - local symbols = symtab.parse(reader) > - local events = sysprof.parse(reader, symbols) > + -- XXX: This function exits with a non-zero exit code and > + -- prints an error message if it encounters any failure during > + -- the process of parsing. > + local events = evread(sysprof.parse, inputfile) > > for stack, count in pairs(events) do > print(stack, count) > diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua > index 64c4a455..0e416d37 100755 > --- a/tools/sysprof/parse.lua > +++ b/tools/sysprof/parse.lua > @@ -237,12 +237,13 @@ function M.parse(reader, symbols) > local _ = reader:read_octets(3) > > if magic ~= LJP_MAGIC then > - error("Bad LJP format prologue: "..magic) > + error("Bad sysprof event format prologue: " .. tostring(magic)) > end > > if string.byte(version) ~= LJP_CURRENT_VERSION then > error(string_format( > - "LJP format version mismatch: the tool expects %d, but your data is %d", > + "Sysprof event format version mismatch:".. > + " the tool expects %d, but your data is %d", > LJP_CURRENT_VERSION, > string.byte(version) > )) > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index 0c878e7a..e80b9f33 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > @@ -95,13 +95,13 @@ function M.parse(reader) > local _ = reader:read_octets(3) > > if magic ~= LJS_MAGIC then > - error("Bad LJS format prologue: "..magic) > + error("Bad LuaJIT symbol table format prologue: " .. tostring(magic)) > end > > if string.byte(version) ~= LJS_CURRENT_VERSION then > error(string_format( > - "LJS format version mismatch:".. > - "the tool expects %d, but your data is %d", > + "LuaJIT symbol table format version mismatch:".. > + " the tool expects %d, but your data is %d", Nit: could be replaced with a single line patch after removing whitespace on a single line: if string.byte(version) ~= LJS_CURRENT_VERSION then error(string_format( - "LJS format version mismatch:".. + "LuaJIT symbol table format version mismatch: ".. "the tool expects %d, but your data is %d", LJS_CURRENT_VERSION, string.byte(version) > LJS_CURRENT_VERSION, > string.byte(version) > )) > -- > 2.44.0 >
next prev parent reply other threads:[~2024-04-04 11:19 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-03-26 16:09 Maxim Kokryashkin via Tarantool-patches 2024-04-04 11:19 ` Sergey Bronnikov via Tarantool-patches [this message] 2024-04-04 13:33 ` Sergey Kaplun via Tarantool-patches 2024-04-08 9:33 ` Maxim Kokryashkin via Tarantool-patches 2024-04-08 15:42 ` Sergey Bronnikov via Tarantool-patches 2024-04-11 17:06 ` Sergey Kaplun 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=e68f9acc-1d94-41be-a246-4dcc0bab31b2@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v1] profilers: print user-friendly errors' \ /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