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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
Fixed your minor, branch is force-pushed. Here is the diff:
===
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/parse.lua b/tools/sysprof/parse.lua
index 749f70db..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: " .. tostring(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 c4aefef7..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: " .. tostring(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",
          LJS_CURRENT_VERSION,
          string.byte(version)
     ))
===

WBR,
Max
On Fri, Dec 29, 2023 at 03:27:46PM +0300, Sergey Bronnikov wrote:
> Hello, Max
>
>
> thanks for the patch. LGTM with minor comments
>
>
> On 12/6/23 21:55, Maxim Kokryashkin wrote:
>
>
> <snipped>
>
> > 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
> > @@ -237,7 +237,7 @@ function M.parse(reader, symbols)
> >     local _ = reader:read_octets(3)
> >     if magic ~= LJP_MAGIC then
> > -    error("Bad LJP format prologue: "..magic)
> > +    error("Bad LJP format prologue: " .. tostring(magic))
> >     end
>
> LJP is not a well-known abbreviation. Probably, it is better to write the
> error
>
> message without it. I see that LJP/LJS is used extensively in error message,
> so feel free to
>
> ignore or fix in a separate patch.
>
> >     if string.byte(version) ~= LJP_CURRENT_VERSION then
> > diff --git a/tools/utils/safe_event_reader.lua b/tools/utils/safe_event_reader.lua
> > new file mode 100644
> > index 00000000..39246a9d
> > --- /dev/null
> > +++ b/tools/utils/safe_event_reader.lua
> > @@ -0,0 +1,34 @@
> > +local bufread = require('utils.bufread')
> > +local symtab = require('utils.symtab')
> > +
> > +local function make_error_handler(fmt, inputfile)
> > +  return function(err)
> > +    io.stderr:write(string.format(fmt, inputfile))
> > +    io.stderr:write(string.format('\nOriginal error: %s', err))
> > +    os.exit(1, true)
> > +  end
> > +end
> > +
> > +local function safe_event_reader(parser, inputfile)
> > +  local _, reader = xpcall(
> > +    bufread.new,
> > +    make_error_handler('Failed to open %s.', inputfile),
> > +    inputfile
> > +  )
> > +
> > +  local _, symbols = xpcall(
> > +    symtab.parse,
> > +    make_error_handler('Failed to parse symtab from %s.', inputfile),
> > +    reader
> > +  )
> > +
> > +  local _, events = xpcall(
> > +    parser,
> > +    make_error_handler('Failed to parse profile data from %s.', inputfile),
> > +    reader,
> > +    symbols
> > +  )
> > +  return events, symbols
> > +end
> > +
> > +return safe_event_reader
> > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> > index 0c878e7a..c4aefef7 100644
> > --- a/tools/utils/symtab.lua
> > +++ b/tools/utils/symtab.lua
> > @@ -95,7 +95,7 @@ function M.parse(reader)
> >     local _ = reader:read_octets(3)
> >     if magic ~= LJS_MAGIC then
> > -    error("Bad LJS format prologue: "..magic)
> > +    error("Bad LJS format prologue: " .. tostring(magic))
>
> LJS is not a well-known abbreviation. Probably, it is better to write the
> error
>
> message without it.
>
> >     end
> >     if string.byte(version) ~= LJS_CURRENT_VERSION then