Re: [Tarantool-patches] [PATCH luajit 4/4] profilers: print user-friendly errors

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the patch!
Please consider my comments below.

On 04.12.23, 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 adds
> error handling, so all parsing errors are now reported in a
> user-friendly manner.
> 
> Event parsing is now moved into a separate profiler-agnostic
> module.
> 
> Tool CLI flag tests are adapted correspondingly to error message
> changes.
> 
> Resolves tarantool/tarantool#9217
> Part of tarantool/tarantool#5994
> ---
>  .../gh-5688-tool-cli-flag.test.lua            |  4 +-
>  ...17-profile-parsers-error-handling.test.lua | 79 +++++++++++++++++++
>  tools/memprof.lua                             | 47 +++++++++--
>  tools/sysprof.lua                             | 42 ++++++++--
>  tools/sysprof/parse.lua                       |  2 +-
>  tools/utils/symtab.lua                        |  2 +-
>  6 files changed, 158 insertions(+), 18 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..ec958031 100644
> --- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua

<snipped>

> 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..9a818086
> --- /dev/null
> +++ b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
> @@ -0,0 +1,79 @@
> +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,
> +})
> +
> +test:plan(6)

We can use here `#TEST_CASES * 2` instead of `6`.

> +

<snipped>

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index acadbc17..a04608b8 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -10,11 +10,11 @@
>  -- 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 view = require "memprof.humanize"
> +local bufread = require('utils.bufread')
> +local symtab = require('utils.symtab')
> +local memprof = require('memprof.parse')
> +local process = require('memprof.process')
> +local view = require('memprof.humanize')

This part of refactoring is excess within this patch.

>  
>  local stdout, stderr = io.stdout, io.stderr
>  local match, gmatch = string.match, string.gmatch
> @@ -106,10 +106,41 @@ local function parseargs(args)
>    return args[args.argn]
>  end
>  
> +local function make_error_handler(inputfile, fmt)

Looks like (fmt, inputfile) order is better.

> +  return function()

I'm a little bit confused that there is no verbose error description.
I suppose, that we should report the given error too.

> +    io.stderr:write(string.format(fmt, inputfile))
> +    os.exit(1, true)
> +  end
> +end
> +
> +local function safe_event_reader(inputfile)
> +  local _, reader = xpcall(
> +    bufread.new,
> +    make_error_handler(inputfile, 'Failed to open %s.'),
> +    inputfile
> +  )
> +
> +  local _, symbols = xpcall(
> +    symtab.parse,
> +    make_error_handler(inputfile, 'Failed to parse symtab from %s.'),
> +    reader
> +  )
> +
> +  local _, events = xpcall(
> +    memprof.parse,
> +    make_error_handler(inputfile, 'Failed to parse profile data from %s.'),
> +    reader,
> +    symbols
> +  )
> +  return events, symbols
> +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 = safe_event_reader(inputfile)
> +
>    if not config.leak_only then
>      view.profile_info(events, config)
>    end
> diff --git a/tools/sysprof.lua b/tools/sysprof.lua
> index 8449b23f..d2efcd7f 100644
> --- a/tools/sysprof.lua
> +++ b/tools/sysprof.lua
> @@ -1,6 +1,6 @@
> -local bufread = require "utils.bufread"
> -local sysprof = require "sysprof.parse"
> -local symtab = require "utils.symtab"
> +local bufread = require('utils.bufread')
> +local symtab = require('utils.symtab')
> +local sysprof = require('sysprof.parse')

This part of refactoring is excess within this patch.

>  
>  local stdout, stderr = io.stdout, io.stderr
>  local match, gmatch = string.match, string.gmatch
> @@ -77,10 +77,40 @@ local function parseargs(args)
>    return args[args.argn]
>  end
>  
> +local function make_error_handler(inputfile, fmt)

Looks like (fmt, inputfile) order is better.

> +  return function()

I'm a little bit confused that there is no verbose error description.
I suppose, that we should report the given error too.

> +    io.stderr:write(string.format(fmt, inputfile))
> +    os.exit(1, true)
> +  end
> +end
> +
> +local function safe_event_reader(inputfile)
> +  local _, reader = xpcall(
> +    bufread.new,
> +    make_error_handler(inputfile, 'Failed to open %s.'),
> +    inputfile
> +  )
> +
> +  local _, symbols = xpcall(
> +    symtab.parse,
> +    make_error_handler(inputfile, 'Failed to parse symtab from %s.'),
> +    reader
> +  )
> +
> +  local _, events = xpcall(
> +    sysprof.parse,
> +    make_error_handler(inputfile, 'Failed to parse profile data from %s.'),
> +    reader,
> +    symbols
> +  )
> +  return events, symbols
> +end

This part is literally the same as for the sysprof parser (except the parser
itself). May be we should separate it from the parsers code?

> +
>  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 = safe_event_reader(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..749f70db 100755
> --- a/tools/sysprof/parse.lua
> +++ b/tools/sysprof/parse.lua

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun