[Tarantool-patches] [PATCH luajit v1] profilers: print user-friendly errors
Sergey Bronnikov
sergeyb at tarantool.org
Thu Apr 4 14:19:26 MSK 2024
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
>
More information about the Tarantool-patches
mailing list