[Tarantool-patches] [PATCH luajit v2 0/6] profilers: refactor parsers

[Tarantool-patches] [PATCH luajit v2 0/6] profilers: refactor parsers

[Maxim Kokryashkin via Tarantool-patches]

Changes in v2:
- Fixed comments as per review by Sergey Kaplun

Branch: https://github.com/tarantool/luajit/tree/fckxorg/profile-parsers-refactoring
PR: https://github.com/tarantool/tarantool/pull/9438
Issues:
https://github.com/tarantool/tarantool/issues/5994
https://github.com/tarantool/tarantool/issues/9217


Maxim Kokryashkin (6):
  build: purge sysprof.collapse module
  build: fix tool components handling
  memprof: refactor `heap_chunk` data structure
  memprof: remove unused arguments
  memprof: introduce the `--human-readable` option
  profilers: print user-friendly errors

 Makefile.original                             | 16 +++-
 .../gh-5688-tool-cli-flag.test.lua            |  4 +-
 .../gh-5994-memprof-human-readable.test.lua   | 73 +++++++++++++++
 ...17-profile-parsers-error-handling.test.lua | 90 +++++++++++++++++++
 tools/CMakeLists.txt                          | 14 +--
 tools/memprof.lua                             | 33 ++++---
 tools/memprof/humanize.lua                    | 46 +++++++---
 tools/memprof/parse.lua                       | 22 +++--
 tools/sysprof.lua                             | 10 +--
 tools/sysprof/collapse.lua                    |  3 -
 tools/sysprof/parse.lua                       |  2 +-
 tools/utils/safe_event_reader.lua             | 34 +++++++
 tools/utils/symtab.lua                        |  2 +-
 13 files changed, 298 insertions(+), 51 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
 create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
 delete mode 100755 tools/sysprof/collapse.lua
 create mode 100644 tools/utils/safe_event_reader.lua

--
2.43.0

[Tarantool-patches] [PATCH luajit v2 1/6] build: purge sysprof.collapse module

[Maxim Kokryashkin via Tarantool-patches]

This module became unused as a result of commit
tarantool/tarantool@e2851883cd4c23d41bd9aec23fad06fd10d39c45,
so it can be purged safely from the LuaJIT sources.

Part of tarantool/tarantool#8700
---
 tools/CMakeLists.txt       | 6 ------
 tools/sysprof/collapse.lua | 3 ---
 2 files changed, 9 deletions(-)
 delete mode 100755 tools/sysprof/collapse.lua

diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index b951f767..1dfc39e4 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -60,9 +60,6 @@ if(LUAJIT_DISABLE_SYSPROF)
 else()
   add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
     sysprof/parse.lua
-    # TODO: This line is not deleted only for the sake of integrational
-    # testing. It should be deleted in the next series.
-    sysprof/collapse.lua
     sysprof.lua
     utils/bufread.lua
     utils/symtab.lua
@@ -71,9 +68,6 @@ else()
 
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/parse.lua
-      # TODO: This line is not deleted only for the sake of integrational
-      # testing. It should be deleted in the next series.
-      ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/collapse.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/sysprof
     PERMISSIONS
       OWNER_READ OWNER_WRITE
diff --git a/tools/sysprof/collapse.lua b/tools/sysprof/collapse.lua
deleted file mode 100755
index cac92f1a..00000000
--- a/tools/sysprof/collapse.lua
+++ /dev/null
@@ -1,3 +0,0 @@
--- FIXME: This file is not deleted only for the sake of
--- integrational testing. It should be deleted in the
--- next series.
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit v2 1/6] build: purge sysprof.collapse module

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 1/6] build: purge sysprof.collapse module

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patch! LGTM with a minor comment.

I'm not sure "build" is a correct prefix for a small commit description.

Probably "tools" reflects component better.

On 12/6/23 21:55, Maxim Kokryashkin wrote:
> This module became unused as a result of commit
> tarantool/tarantool@e2851883cd4c23d41bd9aec23fad06fd10d39c45,
> so it can be purged safely from the LuaJIT sources.
>
> Part of tarantool/tarantool#8700
> ---
>   tools/CMakeLists.txt       | 6 ------
>   tools/sysprof/collapse.lua | 3 ---
>   2 files changed, 9 deletions(-)
>   delete mode 100755 tools/sysprof/collapse.lua
>
> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index b951f767..1dfc39e4 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
> @@ -60,9 +60,6 @@ if(LUAJIT_DISABLE_SYSPROF)
>   else()
>     add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
>       sysprof/parse.lua
> -    # TODO: This line is not deleted only for the sake of integrational
> -    # testing. It should be deleted in the next series.
> -    sysprof/collapse.lua
>       sysprof.lua
>       utils/bufread.lua
>       utils/symtab.lua
> @@ -71,9 +68,6 @@ else()
>   
>     install(FILES
>         ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/parse.lua
> -      # TODO: This line is not deleted only for the sake of integrational
> -      # testing. It should be deleted in the next series.
> -      ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/collapse.lua
>       DESTINATION ${LUAJIT_DATAROOTDIR}/sysprof
>       PERMISSIONS
>         OWNER_READ OWNER_WRITE
> diff --git a/tools/sysprof/collapse.lua b/tools/sysprof/collapse.lua
> deleted file mode 100755
> index cac92f1a..00000000
> --- a/tools/sysprof/collapse.lua
> +++ /dev/null
> @@ -1,3 +0,0 @@
> --- FIXME: This file is not deleted only for the sake of
> --- integrational testing. It should be deleted in the
> --- next series.

[Tarantool-patches] [PATCH luajit v2 2/6] build: fix tool components handling

[Maxim Kokryashkin via Tarantool-patches]

Prior to this patch, memprof/process.lua wasn't added to the
dependency list as a part of the memprof parser sources. Also,
it wasn't installed into the system along with other memprof
sources, which breaks the profile parser.

Also, the sysprof parser sources weren't handled by the
Makefile.original at all. The same applies to utils/avl.lua.
This patch fixes that, so now it's possible to properly handle
sysprof's parser.

Part of tarantool/tarantool#5994
---
 Makefile.original    | 14 +++++++++++---
 tools/CMakeLists.txt |  4 ++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Makefile.original b/Makefile.original
index e67b6aa8..2a56d648 100644
--- a/Makefile.original
+++ b/Makefile.original
@@ -40,6 +40,7 @@ INSTALL_JITLIB= $(INSTALL_LJLIBD)/jit
 INSTALL_TOOLSLIB= $(INSTALL_LJLIBD)
 INSTALL_UTILSLIB= $(INSTALL_TOOLSLIB)/utils
 INSTALL_MEMPROFLIB= $(INSTALL_TOOLSLIB)/memprof
+INSTALL_SYSPROFLIB= $(INSTALL_TOOLSLIB)/sysprof
 INSTALL_LMODD= $(INSTALL_SHARE)/lua
 INSTALL_LMOD= $(INSTALL_LMODD)/$(ABIVER)
 INSTALL_CMODD= $(INSTALL_LIB)/lua
@@ -68,10 +69,12 @@ INSTALL_PC= $(INSTALL_PKGCONFIG)/$(INSTALL_PCNAME)
 
 INSTALL_DIRS= $(INSTALL_BIN) $(INSTALL_LIB) $(INSTALL_INC) $(INSTALL_MAN) \
   $(INSTALL_PKGCONFIG) $(INSTALL_JITLIB) $(INSTALL_LMOD) $(INSTALL_CMOD) \
-  $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_TOOLSLIB)
+  $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_SYSPROFLIB) \
+  $(INSTALL_TOOLSLIB)
 UNINSTALL_DIRS= $(INSTALL_JITLIB) $(INSTALL_LJLIBD) $(INSTALL_INC) \
   $(INSTALL_LMOD) $(INSTALL_LMODD) $(INSTALL_CMOD) $(INSTALL_CMODD) \
-  $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_TOOLSLIB)
+  $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_SYSPROFLIB) \
+  $(INSTALL_TOOLSLIB)
 
 RM= rm -f
 MKDIR= mkdir -p
@@ -95,7 +98,8 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
 	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
 	      dis_mips64.lua dis_mips64el.lua vmdef.lua
 FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
-FILES_MEMPROFLIB= parse.lua humanize.lua
+FILES_MEMPROFLIB= parse.lua humanize.lua process.lua
+FILES_SYSPROFLIB= parse.lua
 FILES_TOOLSLIB= memprof.lua
 
 ifeq (,$(findstring Windows,$(OS)))
@@ -140,6 +144,7 @@ install: $(INSTALL_DEP)
 	cd src/jit && $(INSTALL_F) $(FILES_JITLIB) $(INSTALL_JITLIB)
 	cd tools/utils && $(INSTALL_F) $(FILES_UTILSLIB) $(INSTALL_UTILSLIB)
 	cd tools/memprof && $(INSTALL_F) $(FILES_MEMPROFLIB) $(INSTALL_MEMPROFLIB)
+	cd tools/sysprof && $(INSTALL_F) $(FILES_SYSPROFLIB) $(INSTALL_SYSPROFLIB)
 	cd tools && $(INSTALL_F) $(FILES_TOOLSLIB) $(INSTALL_TOOLSLIB)
 	@echo "==== Successfully installed LuaJIT $(VERSION) to $(PREFIX) ===="
 	@echo ""
@@ -162,6 +167,9 @@ uninstall:
 	for file in $(FILES_MEMPROFLIB); do \
 	  $(UNINSTALL) $(INSTALL_MEMPROFLIB)/$$file; \
 	  done
+	for file in $(FILES_SYSPROFLIB); do \
+	  $(UNINSTALL) $(INSTALL_SYSPROFLIB)/$$file; \
+	  done
 	for file in $(FILES_TOOLSLIB); do \
 	  $(UNINSTALL) $(INSTALL_TOOLSLIB)/$$file; \
 	  done
diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index 1dfc39e4..a4adc15b 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -14,6 +14,7 @@ else()
   add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS
     memprof/humanize.lua
     memprof/parse.lua
+    memprof/process.lua
     memprof.lua
     utils/avl.lua
     utils/bufread.lua
@@ -24,6 +25,7 @@ else()
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/memprof/humanize.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/memprof/parse.lua
+      ${CMAKE_CURRENT_SOURCE_DIR}/memprof/process.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/memprof
     PERMISSIONS
       OWNER_READ OWNER_WRITE
@@ -61,6 +63,7 @@ else()
   add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
     sysprof/parse.lua
     sysprof.lua
+    utils/avl.lua
     utils/bufread.lua
     utils/symtab.lua
   )
@@ -76,6 +79,7 @@ else()
     COMPONENT tools-parse-sysprof
   )
   install(FILES
+      ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/utils
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit v2 2/6] build: fix tool components handling

[Sergey Kaplun via Tarantool-patches]

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

On 06.12.23, Maxim Kokryashkin wrote:

<snipped>

> diff --git a/Makefile.original b/Makefile.original
> index e67b6aa8..2a56d648 100644
> --- a/Makefile.original
> +++ b/Makefile.original

<snipped>

> @@ -95,7 +98,8 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
>  	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
>  	      dis_mips64.lua dis_mips64el.lua vmdef.lua
>  FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
> -FILES_MEMPROFLIB= parse.lua humanize.lua
> +FILES_MEMPROFLIB= parse.lua humanize.lua process.lua

Minor: Please use the aphabetic order here instead.

> +FILES_SYSPROFLIB= parse.lua
>  FILES_TOOLSLIB= memprof.lua

I suppose we should include <sysprof.lua> in `FILES_TOOLSLIB`.

>  
>  ifeq (,$(findstring Windows,$(OS)))
> @@ -140,6 +144,7 @@ install: $(INSTALL_DEP)

<snipped>

> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index 1dfc39e4..a4adc15b 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 2/6] build: fix tool components handling

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed, branch is force-pushed.
See the iterative diff below:


On Tue, Dec 12, 2023 at 01:32:17PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the patch!
> Please, consider my comments below.
>
> On 06.12.23, Maxim Kokryashkin wrote:
>
> <snipped>
>
> > diff --git a/Makefile.original b/Makefile.original
> > index e67b6aa8..2a56d648 100644
> > --- a/Makefile.original
> > +++ b/Makefile.original
>
> <snipped>
>
> > @@ -95,7 +98,8 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
> >  	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
> >  	      dis_mips64.lua dis_mips64el.lua vmdef.lua
> >  FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
> > -FILES_MEMPROFLIB= parse.lua humanize.lua
> > +FILES_MEMPROFLIB= parse.lua humanize.lua process.lua
>
> Minor: Please use the aphabetic order here instead.
>
> > +FILES_SYSPROFLIB= parse.lua
> >  FILES_TOOLSLIB= memprof.lua
>
> I suppose we should include <sysprof.lua> in `FILES_TOOLSLIB`.
>
> >
> >  ifeq (,$(findstring Windows,$(OS)))
> > @@ -140,6 +144,7 @@ install: $(INSTALL_DEP)
>
> <snipped>
>
> > diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> > index 1dfc39e4..a4adc15b 100644
> > --- a/tools/CMakeLists.txt
> > +++ b/tools/CMakeLists.txt
>
> <snipped>

===
diff --git a/Makefile.original b/Makefile.original
index 2a56d648..d0834fe6 100644
--- a/Makefile.original
+++ b/Makefile.original
@@ -98,9 +98,9 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
 	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
 	      dis_mips64.lua dis_mips64el.lua vmdef.lua
 FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
-FILES_MEMPROFLIB= parse.lua humanize.lua process.lua
+FILES_MEMPROFLIB= humanize.lua parse.lua process.lua
 FILES_SYSPROFLIB= parse.lua
-FILES_TOOLSLIB= memprof.lua
+FILES_TOOLSLIB= memprof.lua sysprof.lua

 ifeq (,$(findstring Windows,$(OS)))
   HOST_SYS:= $(shell uname -s)
===

Re: [Tarantool-patches] [PATCH luajit v2 2/6] build: fix tool components handling

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 2/6] build: fix tool components handling

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patch, LGTM

On 12/6/23 21:55, Maxim Kokryashkin wrote:
> Prior to this patch, memprof/process.lua wasn't added to the
> dependency list as a part of the memprof parser sources. Also,
> it wasn't installed into the system along with other memprof
> sources, which breaks the profile parser.
>
> Also, the sysprof parser sources weren't handled by the
> Makefile.original at all. The same applies to utils/avl.lua.
> This patch fixes that, so now it's possible to properly handle
> sysprof's parser.
>
> Part of tarantool/tarantool#5994
<snipped>

[Tarantool-patches] [PATCH luajit v2 3/6] memprof: refactor `heap_chunk` data structure

[Maxim Kokryashkin via Tarantool-patches]

The memprof parser used to have the `heap_chunk` data structure
using numeric indices for its values, which is hardly readable
and maintainable. This patch replaces these numeric indices with
corresponding string keys.

Part of tarantool/tarantool#5994
---
 tools/memprof/parse.lua | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
index 42a601ef..cc66a23e 100644
--- a/tools/memprof/parse.lua
+++ b/tools/memprof/parse.lua
@@ -45,22 +45,30 @@ local function new_event(loc)
   }
 end
 
+local function new_heap_chunk(size, id, loc)
+  return {
+    size = size,
+    id = id,
+    loc = loc,
+  }
+end
+
 local function link_to_previous(heap_chunk, e, nsize)
   -- Memory at this chunk was allocated before we start tracking.
   if heap_chunk then
     -- Save Lua code location (line) by address (id).
-    if not e.primary[heap_chunk[2]] then
-      e.primary[heap_chunk[2]] = {
-        loc = heap_chunk[3],
+    if not e.primary[heap_chunk.id] then
+      e.primary[heap_chunk.id] = {
+        loc = heap_chunk.loc,
         allocated = 0,
         freed = 0,
         count = 0,
       }
     end
     -- Save information about delta for memory heap.
-    local location_data = e.primary[heap_chunk[2]]
+    local location_data = e.primary[heap_chunk.id]
     location_data.allocated = location_data.allocated + nsize
-    location_data.freed = location_data.freed + heap_chunk[1]
+    location_data.freed = location_data.freed + heap_chunk.size
     location_data.count = location_data.count + 1
   end
 end
@@ -95,7 +103,7 @@ local function parse_alloc(reader, asource, events, heap, symbols)
   e.num = e.num + 1
   e.alloc = e.alloc + nsize
 
-  heap[naddr] = {nsize, id, loc}
+  heap[naddr] = new_heap_chunk(nsize, id, loc)
 end
 
 local function parse_realloc(reader, asource, events, heap, symbols)
@@ -117,7 +125,7 @@ local function parse_realloc(reader, asource, events, heap, symbols)
   link_to_previous(heap[oaddr], e, nsize)
 
   heap[oaddr] = nil
-  heap[naddr] = {nsize, id, loc}
+  heap[naddr] = new_heap_chunk(nsize, id, loc)
 end
 
 local function parse_free(reader, asource, events, heap, symbols)
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit v2 3/6] memprof: refactor `heap_chunk` data structure

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 3/6] memprof: refactor `heap_chunk` data structure

[Sergey Bronnikov via Tarantool-patches]

Hello, Max

thanks for the patch! LGTM

On 12/6/23 21:55, Maxim Kokryashkin wrote:
> The memprof parser used to have the `heap_chunk` data structure
> using numeric indices for its values, which is hardly readable
> and maintainable. This patch replaces these numeric indices with
> corresponding string keys.
>
> Part of tarantool/tarantool#5994
> ---


<snipped>

[Tarantool-patches] [PATCH luajit v2 4/6] memprof: remove unused arguments

[Maxim Kokryashkin via Tarantool-patches]

Both `humanize.profile_info` and `process.form_heap_delta`
don't need the symtab, so the argument can be dropped.

Part of tarantool/tarantool#5994
---
 tools/memprof.lua | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/memprof.lua b/tools/memprof.lua
index e23bbf24..955a1327 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -102,9 +102,9 @@ local function dump(inputfile)
   local symbols = symtab.parse(reader)
   local events = memprof.parse(reader, symbols)
   if not leak_only then
-    view.profile_info(events, symbols)
+    view.profile_info(events)
   end
-  local dheap = process.form_heap_delta(events, symbols)
+  local dheap = process.form_heap_delta(events)
   view.leak_info(dheap)
   view.aliases(symbols)
   -- XXX: The second argument is required to properly close Lua
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit v2 4/6] memprof: remove unused arguments

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 4/6] memprof: remove unused arguments

[Sergey Bronnikov via Tarantool-patches]

Hello, Max

thanks for the patch! LGTM

On 12/6/23 21:55, Maxim Kokryashkin wrote:
> Both `humanize.profile_info` and `process.form_heap_delta`
> don't need the symtab, so the argument can be dropped.
>
> Part of tarantool/tarantool#5994
> ---
>   tools/memprof.lua | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index e23bbf24..955a1327 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -102,9 +102,9 @@ local function dump(inputfile)
>     local symbols = symtab.parse(reader)
>     local events = memprof.parse(reader, symbols)
>     if not leak_only then
> -    view.profile_info(events, symbols)
> +    view.profile_info(events)
>     end
> -  local dheap = process.form_heap_delta(events, symbols)
> +  local dheap = process.form_heap_delta(events)
>     view.leak_info(dheap)
>     view.aliases(symbols)
>     -- XXX: The second argument is required to properly close Lua

[Tarantool-patches] [PATCH luajit v2 5/6] memprof: introduce the `--human-readable` option

[Maxim Kokryashkin via Tarantool-patches]

Prior to this patch, memprof could report only the raw
amount of bytes, which is hard to read. This patch adds the
`--human-readable` CLI option to the memprof parser, so the
memory is reported in KiB, MiB, or GiB, depending on what's
the biggest reasonable unit.

This patch also refactors the options mechanism for parser,
so all of the options are passed into parser's modules as a
single config table instead of being handled individually.

Part of tarantool/tarantool#5994
---
 .../gh-5994-memprof-human-readable.test.lua   | 73 +++++++++++++++++++
 tools/memprof.lua                             | 20 +++--
 tools/memprof/humanize.lua                    | 46 +++++++++---
 3 files changed, 123 insertions(+), 16 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua

diff --git a/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
new file mode 100644
index 00000000..e34291be
--- /dev/null
+++ b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
@@ -0,0 +1,73 @@
+local tap = require('tap')
+local test = tap.test('gh-5994-memprof-human-readable'):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,
+})
+
+local utils = require('utils')
+local TMP_BINFILE_MEMPROF = utils.tools.profilename('memprofdata.tmp.bin')
+local PARSE_CMD = utils.exec.luacmd(arg) .. ' -tm '
+
+local function generate_output(bytes)
+  local res, err = misc.memprof.start(TMP_BINFILE_MEMPROF)
+  -- Should start successfully.
+  assert(res, err)
+
+  -- luacheck: no unused
+  local _ = string.rep('_', bytes)
+
+  res, err = misc.memprof.stop()
+  -- Should stop successfully.
+  assert(res, err)
+end
+
+local TEST_SET = {
+  {
+    bytes = 2049,
+    match = '%dB',
+    hr = false,
+    name = 'non-human-readable mode is correct'
+  },
+  {
+    bytes = 100,
+    match = '%dB',
+    hr = true,
+    name = 'human-readable mode: bytes'
+  },
+  {
+    bytes = 2560,
+    match = '%d+%.%d%dKiB',
+    hr = true,
+    name = 'human-readable mode: float'
+  },
+  {
+    bytes = 2048,
+    match = '%dKiB',
+    hr = true,
+    name = 'human-readable mode: KiB'
+  },
+  {
+    bytes = 1024 * 1024,
+    match = '%dMiB',
+    hr = true,
+    name = 'human-readable mode: MiB'
+  },
+  -- XXX: The test case for GiB is not implemented because it is
+  -- OOM-prone for non-GC64 builds.
+}
+
+test:plan(#TEST_SET)
+
+for _, params in ipairs(TEST_SET) do
+  generate_output(params.bytes)
+  local cmd = PARSE_CMD .. (params.hr and ' --human-readable ' or '')
+  local output = io.popen(cmd .. TMP_BINFILE_MEMPROF):read('*all')
+  test:like(output, params.match, params.name)
+end
+
+os.remove(TMP_BINFILE_MEMPROF)
+test:done(true)
diff --git a/tools/memprof.lua b/tools/memprof.lua
index 955a1327..537cb869 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -22,6 +22,12 @@ local match, gmatch = string.match, string.gmatch
 -- Program options.
 local opt_map = {}
 
+-- Default config for the memprof parser.
+local config = {
+  leak_only = false,
+  human_readable = false,
+}
+
 function opt_map.help()
   stdout:write [[
 luajit-parse-memprof - parser of the memory usage profile collected
@@ -34,14 +40,18 @@ luajit-parse-memprof [options] memprof.bin
 Supported options are:
 
   --help                            Show this help and exit
+  --human-readable                  Use KiB/MiB/GiB notation instead of bytes
   --leak-only                       Report only leaks information
 ]]
   os.exit(0)
 end
 
-local leak_only = false
 opt_map["leak-only"] = function()
-  leak_only = true
+  config.leak_only = true
+end
+
+opt_map["human-readable"] = function()
+  config.human_readable = true
 end
 
 -- Print error and exit with error status.
@@ -101,11 +111,11 @@ local function dump(inputfile)
   local reader = bufread.new(inputfile)
   local symbols = symtab.parse(reader)
   local events = memprof.parse(reader, symbols)
-  if not leak_only then
-    view.profile_info(events)
+  if not config.leak_only then
+    view.profile_info(events, config)
   end
   local dheap = process.form_heap_delta(events)
-  view.leak_info(dheap)
+  view.leak_info(dheap, config)
   view.aliases(symbols)
   -- XXX: The second argument is required to properly close Lua
   -- universe (i.e. invoke <lua_close> before exiting).
diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
index 5b289ce3..a0b1693a 100644
--- a/tools/memprof/humanize.lua
+++ b/tools/memprof/humanize.lua
@@ -5,7 +5,29 @@
 
 local M = {}
 
-function M.render(events)
+local function human_readable_bytes(bytes)
+  local units = {"B", "KiB", "MiB", "GiB"}
+  local magnitude = 1
+
+  while bytes >= 1024 and magnitude < #units do
+    bytes = bytes / 1024
+    magnitude = magnitude + 1
+  end
+  local is_int = math.floor(bytes) == bytes
+  local fmt = is_int and "%d%s" or "%.2f%s"
+
+  return string.format(fmt, bytes, units[magnitude])
+end
+
+local function format_bytes(bytes, config)
+  if config.human_readable then
+    return human_readable_bytes(bytes)
+  else
+    return string.format('%dB', bytes)
+  end
+end
+
+function M.render(events, config)
   local ids = {}
 
   for id, _ in pairs(events) do
@@ -18,11 +40,11 @@ function M.render(events)
 
   for i = 1, #ids do
     local event = events[ids[i]]
-    print(string.format("%s: %d events\t+%d bytes\t-%d bytes",
+    print(string.format("%s: %d events\t+%s\t-%s",
       event.loc,
       event.num,
-      event.alloc,
-      event.free
+      format_bytes(event.alloc, config),
+      format_bytes(event.free, config)
     ))
 
     local prim_loc = {}
@@ -40,21 +62,21 @@ function M.render(events)
   end
 end
 
-function M.profile_info(events)
+function M.profile_info(events, config)
   print("ALLOCATIONS")
-  M.render(events.alloc)
+  M.render(events.alloc, config)
   print("")
 
   print("REALLOCATIONS")
-  M.render(events.realloc)
+  M.render(events.realloc, config)
   print("")
 
   print("DEALLOCATIONS")
-  M.render(events.free)
+  M.render(events.free, config)
   print("")
 end
 
-function M.leak_info(dheap)
+function M.leak_info(dheap, config)
   local leaks = {}
   for line, info in pairs(dheap) do
     -- Report "INTERNAL" events inconsistencies for profiling
@@ -71,8 +93,10 @@ function M.leak_info(dheap)
   print("HEAP SUMMARY:")
   for _, l in pairs(leaks) do
     print(string.format(
-      "%s holds %d bytes: %d allocs, %d frees",
-      l.line, l.dbytes, dheap[l.line].nalloc,
+      "%s holds %s: %d allocs, %d frees",
+      l.line,
+      format_bytes(l.dbytes, config),
+      dheap[l.line].nalloc,
       dheap[l.line].nfree
     ))
   end
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit v2 5/6] memprof: introduce the `--human-readable` option

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 5/6] memprof: introduce the `--human-readable` option

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patch!

LGTM

On 12/6/23 21:55, Maxim Kokryashkin wrote:
> Prior to this patch, memprof could report only the raw
> amount of bytes, which is hard to read. This patch adds the
> `--human-readable` CLI option to the memprof parser, so the
> memory is reported in KiB, MiB, or GiB, depending on what's
> the biggest reasonable unit.
>
> This patch also refactors the options mechanism for parser,
> so all of the options are passed into parser's modules as a
> single config table instead of being handled individually.
>
> Part of tarantool/tarantool#5994
> ---
>   .../gh-5994-memprof-human-readable.test.lua   | 73 +++++++++++++++++++
>   tools/memprof.lua                             | 20 +++--
>   tools/memprof/humanize.lua                    | 46 +++++++++---
>   3 files changed, 123 insertions(+), 16 deletions(-)
>   create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
>
> diff --git a/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
> new file mode 100644
> index 00000000..e34291be
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua
> @@ -0,0 +1,73 @@
> +local tap = require('tap')
> +local test = tap.test('gh-5994-memprof-human-readable'):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,
> +})
> +
> +local utils = require('utils')
> +local TMP_BINFILE_MEMPROF = utils.tools.profilename('memprofdata.tmp.bin')
> +local PARSE_CMD = utils.exec.luacmd(arg) .. ' -tm '
> +
> +local function generate_output(bytes)
> +  local res, err = misc.memprof.start(TMP_BINFILE_MEMPROF)
> +  -- Should start successfully.
> +  assert(res, err)
> +
> +  -- luacheck: no unused
> +  local _ = string.rep('_', bytes)
> +
> +  res, err = misc.memprof.stop()
> +  -- Should stop successfully.
> +  assert(res, err)
> +end
> +
> +local TEST_SET = {
> +  {
> +    bytes = 2049,
> +    match = '%dB',
> +    hr = false,
> +    name = 'non-human-readable mode is correct'
> +  },
> +  {
> +    bytes = 100,
> +    match = '%dB',
> +    hr = true,
> +    name = 'human-readable mode: bytes'
> +  },
> +  {
> +    bytes = 2560,
> +    match = '%d+%.%d%dKiB',
> +    hr = true,
> +    name = 'human-readable mode: float'
> +  },
> +  {
> +    bytes = 2048,
> +    match = '%dKiB',
> +    hr = true,
> +    name = 'human-readable mode: KiB'
> +  },
> +  {
> +    bytes = 1024 * 1024,
> +    match = '%dMiB',
> +    hr = true,
> +    name = 'human-readable mode: MiB'
> +  },
> +  -- XXX: The test case for GiB is not implemented because it is
> +  -- OOM-prone for non-GC64 builds.
> +}
> +
> +test:plan(#TEST_SET)
> +
> +for _, params in ipairs(TEST_SET) do
> +  generate_output(params.bytes)
> +  local cmd = PARSE_CMD .. (params.hr and ' --human-readable ' or '')
> +  local output = io.popen(cmd .. TMP_BINFILE_MEMPROF):read('*all')
> +  test:like(output, params.match, params.name)
> +end
> +
> +os.remove(TMP_BINFILE_MEMPROF)
> +test:done(true)
> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index 955a1327..537cb869 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua
> @@ -22,6 +22,12 @@ local match, gmatch = string.match, string.gmatch
>   -- Program options.
>   local opt_map = {}
>   
> +-- Default config for the memprof parser.
> +local config = {
> +  leak_only = false,
> +  human_readable = false,
> +}
> +
>   function opt_map.help()
>     stdout:write [[
>   luajit-parse-memprof - parser of the memory usage profile collected
> @@ -34,14 +40,18 @@ luajit-parse-memprof [options] memprof.bin
>   Supported options are:
>   
>     --help                            Show this help and exit
> +  --human-readable                  Use KiB/MiB/GiB notation instead of bytes
>     --leak-only                       Report only leaks information
>   ]]
>     os.exit(0)
>   end
>   
> -local leak_only = false
>   opt_map["leak-only"] = function()
> -  leak_only = true
> +  config.leak_only = true
> +end
> +
> +opt_map["human-readable"] = function()
> +  config.human_readable = true
>   end
>   
>   -- Print error and exit with error status.
> @@ -101,11 +111,11 @@ local function dump(inputfile)
>     local reader = bufread.new(inputfile)
>     local symbols = symtab.parse(reader)
>     local events = memprof.parse(reader, symbols)
> -  if not leak_only then
> -    view.profile_info(events)
> +  if not config.leak_only then
> +    view.profile_info(events, config)
>     end
>     local dheap = process.form_heap_delta(events)
> -  view.leak_info(dheap)
> +  view.leak_info(dheap, config)
>     view.aliases(symbols)
>     -- XXX: The second argument is required to properly close Lua
>     -- universe (i.e. invoke <lua_close> before exiting).
> diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua
> index 5b289ce3..a0b1693a 100644
> --- a/tools/memprof/humanize.lua
> +++ b/tools/memprof/humanize.lua
> @@ -5,7 +5,29 @@
>   
>   local M = {}
>   
> -function M.render(events)
> +local function human_readable_bytes(bytes)
> +  local units = {"B", "KiB", "MiB", "GiB"}
> +  local magnitude = 1
> +
> +  while bytes >= 1024 and magnitude < #units do
> +    bytes = bytes / 1024
> +    magnitude = magnitude + 1
> +  end
> +  local is_int = math.floor(bytes) == bytes
> +  local fmt = is_int and "%d%s" or "%.2f%s"
> +
> +  return string.format(fmt, bytes, units[magnitude])
> +end
> +
> +local function format_bytes(bytes, config)
> +  if config.human_readable then
> +    return human_readable_bytes(bytes)
> +  else
> +    return string.format('%dB', bytes)
> +  end
> +end
> +
> +function M.render(events, config)
>     local ids = {}
>   
>     for id, _ in pairs(events) do
> @@ -18,11 +40,11 @@ function M.render(events)
>   
>     for i = 1, #ids do
>       local event = events[ids[i]]
> -    print(string.format("%s: %d events\t+%d bytes\t-%d bytes",
> +    print(string.format("%s: %d events\t+%s\t-%s",
>         event.loc,
>         event.num,
> -      event.alloc,
> -      event.free
> +      format_bytes(event.alloc, config),
> +      format_bytes(event.free, config)
>       ))
>   
>       local prim_loc = {}
> @@ -40,21 +62,21 @@ function M.render(events)
>     end
>   end
>   
> -function M.profile_info(events)
> +function M.profile_info(events, config)
>     print("ALLOCATIONS")
> -  M.render(events.alloc)
> +  M.render(events.alloc, config)
>     print("")
>   
>     print("REALLOCATIONS")
> -  M.render(events.realloc)
> +  M.render(events.realloc, config)
>     print("")
>   
>     print("DEALLOCATIONS")
> -  M.render(events.free)
> +  M.render(events.free, config)
>     print("")
>   end
>   
> -function M.leak_info(dheap)
> +function M.leak_info(dheap, config)
>     local leaks = {}
>     for line, info in pairs(dheap) do
>       -- Report "INTERNAL" events inconsistencies for profiling
> @@ -71,8 +93,10 @@ function M.leak_info(dheap)
>     print("HEAP SUMMARY:")
>     for _, l in pairs(leaks) do
>       print(string.format(
> -      "%s holds %d bytes: %d allocs, %d frees",
> -      l.line, l.dbytes, dheap[l.line].nalloc,
> +      "%s holds %s: %d allocs, %d frees",
> +      l.line,
> +      format_bytes(l.dbytes, config),
> +      dheap[l.line].nalloc,
>         dheap[l.line].nfree
>       ))
>     end

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

[Maxim Kokryashkin via Tarantool-patches]

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
---
 Makefile.original                             |  2 +-
 .../gh-5688-tool-cli-flag.test.lua            |  4 +-
 ...17-profile-parsers-error-handling.test.lua | 90 +++++++++++++++++++
 tools/CMakeLists.txt                          |  4 +
 tools/memprof.lua                             | 11 +--
 tools/sysprof.lua                             | 10 +--
 tools/sysprof/parse.lua                       |  2 +-
 tools/utils/safe_event_reader.lua             | 34 +++++++
 tools/utils/symtab.lua                        |  2 +-
 9 files changed, 144 insertions(+), 15 deletions(-)
 create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
 create mode 100644 tools/utils/safe_event_reader.lua

diff --git a/Makefile.original b/Makefile.original
index 2a56d648..1ef6fbe6 100644
--- a/Makefile.original
+++ b/Makefile.original
@@ -97,7 +97,7 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
 	      dis_x86.lua dis_x64.lua dis_arm.lua dis_arm64.lua \
 	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
 	      dis_mips64.lua dis_mips64el.lua vmdef.lua
-FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
+FILES_UTILSLIB= avl.lua bufread.lua safe_event_reader.lua symtab.lua
 FILES_MEMPROFLIB= parse.lua humanize.lua process.lua
 FILES_SYSPROFLIB= parse.lua
 FILES_TOOLSLIB= memprof.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
@@ -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',
   },
   {
     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',
   },
   {
     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'
+  },
+}
+
+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)
+  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/CMakeLists.txt b/tools/CMakeLists.txt
index a4adc15b..7f728787 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -18,6 +18,7 @@ else()
     memprof.lua
     utils/avl.lua
     utils/bufread.lua
+    utils/safe_event_reader.lua
     utils/symtab.lua
   )
   list(APPEND LUAJIT_TOOLS_DEPS tools-parse-memprof)
@@ -36,6 +37,7 @@ else()
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua
+      ${CMAKE_CURRENT_SOURCE_DIR}/utils/safe_event_reader.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/utils
     PERMISSIONS
@@ -65,6 +67,7 @@ else()
     sysprof.lua
     utils/avl.lua
     utils/bufread.lua
+    utils/safe_event_reader.lua
     utils/symtab.lua
   )
   list(APPEND LUAJIT_TOOLS_DEPS tools-parse-sysprof)
@@ -81,6 +84,7 @@ else()
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua
+      ${CMAKE_CURRENT_SOURCE_DIR}/utils/safe_event_reader.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/utils
     PERMISSIONS
diff --git a/tools/memprof.lua b/tools/memprof.lua
index 537cb869..d491b3dc 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 safe_event_reader = require "utils.safe_event_reader"
 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 = safe_event_reader(memprof.parse, 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..9c0c23c9 100644
--- a/tools/sysprof.lua
+++ b/tools/sysprof.lua
@@ -1,6 +1,5 @@
-local bufread = require "utils.bufread"
+local safe_event_reader = require "utils.safe_event_reader"
 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 = safe_event_reader(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..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
 
   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))
   end
 
   if string.byte(version) ~= LJS_CURRENT_VERSION then
-- 
2.43.0

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

[Sergey Kaplun via Tarantool-patches]

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

On 06.12.23, Maxim Kokryashkin wrote:

<snipped>

> ---
>  Makefile.original                             |  2 +-
>  .../gh-5688-tool-cli-flag.test.lua            |  4 +-
>  ...17-profile-parsers-error-handling.test.lua | 90 +++++++++++++++++++
>  tools/CMakeLists.txt                          |  4 +
>  tools/memprof.lua                             | 11 +--
>  tools/sysprof.lua                             | 10 +--
>  tools/sysprof/parse.lua                       |  2 +-
>  tools/utils/safe_event_reader.lua             | 34 +++++++
>  tools/utils/symtab.lua                        |  2 +-
>  9 files changed, 144 insertions(+), 15 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
>  create mode 100644 tools/utils/safe_event_reader.lua

I suggest renaming to the <evread.lua> so it will be match <bufread.lua>.

> 
> diff --git a/Makefile.original b/Makefile.original
> index 2a56d648..1ef6fbe6 100644
> --- a/Makefile.original
> +++ b/Makefile.original

<snipped>

> 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
> @@ -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',
>    },
>    {
>      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',
>    },
>    {
>      cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF,

After adding the original error message, these changes appear not to be
necessary.

We can still add matching with updated error messages to the `fopen()`
error too. Like the following:

| like = 'Failed to open.*fopen, errno: 2'

> 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

<snipped>

> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index a4adc15b..7f728787 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index 537cb869..d491b3dc 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua

<snipped>

> diff --git a/tools/sysprof.lua b/tools/sysprof.lua
> index 8449b23f..9c0c23c9 100644
> --- a/tools/sysprof.lua
> +++ b/tools/sysprof.lua

<snipped>

> 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))

Typo: s/%s/%s\n/

> +    os.exit(1, true)
> +  end

For now the output is the following:

| 14:09:05 jobs:0 burii@root:~/reviews/luajit/refactor-profilers$ 
| >>> LUA_PATH="src/?.lua;tools/?.lua;;" src/luajit -ts /tmp/memprof.bin
| Failed to parse profile data from /tmp/memprof.bin.
| Original error: tools/sysprof/parse.lua:240: Bad LJP format prologue: ljm14:30:50 jobs:0 burii@root:~/reviews/luajit/refactor-profilers$[1] 
| >>>

I suppose the following format is more readable:

| Failed to parse profile data from /tmp/memprof.bin:
| 	tools/sysprof/parse.lua:240: Bad LJP format prologue: ljm

The ident level is one tab.

> +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

Nit: If we return this function at once, do we need to name it?
Feel free to ignore.

> 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

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed, branch is force-pushed, tarantool PR is updated.
Iterative diff:
===
diff --git a/Makefile.original b/Makefile.original
index 73995130..4a1e1d9d 100644
--- a/Makefile.original
+++ b/Makefile.original
@@ -97,7 +97,7 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
 	      dis_x86.lua dis_x64.lua dis_arm.lua dis_arm64.lua \
 	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
 	      dis_mips64.lua dis_mips64el.lua vmdef.lua
-FILES_UTILSLIB= avl.lua bufread.lua safe_event_reader.lua symtab.lua
+FILES_UTILSLIB= avl.lua bufread.lua evread.lua symtab.lua
 FILES_MEMPROFLIB= humanize.lua parse.lua process.lua
 FILES_SYSPROFLIB= parse.lua
 FILES_TOOLSLIB= memprof.lua sysprof.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 ec958031..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 = 'Failed to open',
+    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 = 'Failed to open',
+    like = 'Failed to open.*fopen, errno: 2',
   },
   {
     cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF,
diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index 7f728787..695c079a 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -18,7 +18,7 @@ else()
     memprof.lua
     utils/avl.lua
     utils/bufread.lua
-    utils/safe_event_reader.lua
+    utils/evread.lua
     utils/symtab.lua
   )
   list(APPEND LUAJIT_TOOLS_DEPS tools-parse-memprof)
@@ -37,7 +37,7 @@ else()
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua
-      ${CMAKE_CURRENT_SOURCE_DIR}/utils/safe_event_reader.lua
+      ${CMAKE_CURRENT_SOURCE_DIR}/utils/evread.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/utils
     PERMISSIONS
@@ -67,7 +67,7 @@ else()
     sysprof.lua
     utils/avl.lua
     utils/bufread.lua
-    utils/safe_event_reader.lua
+    utils/evread.lua
     utils/symtab.lua
   )
   list(APPEND LUAJIT_TOOLS_DEPS tools-parse-sysprof)
@@ -84,7 +84,7 @@ else()
   install(FILES
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua
-      ${CMAKE_CURRENT_SOURCE_DIR}/utils/safe_event_reader.lua
+      ${CMAKE_CURRENT_SOURCE_DIR}/utils/evread.lua
       ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua
     DESTINATION ${LUAJIT_DATAROOTDIR}/utils
     PERMISSIONS
diff --git a/tools/memprof.lua b/tools/memprof.lua
index d491b3dc..7d7e8e05 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -12,7 +12,7 @@

 local memprof = require "memprof.parse"
 local process = require "memprof.process"
-local safe_event_reader = require "utils.safe_event_reader"
+local evread = require "utils.evread"
 local view = require "memprof.humanize"

 local stdout, stderr = io.stdout, io.stderr
@@ -110,7 +110,7 @@ local function dump(inputfile)
   -- 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(memprof.parse, inputfile)
+  local events, symbols = evread(memprof.parse, inputfile)

   if not config.leak_only then
     view.profile_info(events, config)
diff --git a/tools/sysprof.lua b/tools/sysprof.lua
index 9c0c23c9..420ca41c 100644
--- a/tools/sysprof.lua
+++ b/tools/sysprof.lua
@@ -1,4 +1,4 @@
-local safe_event_reader = require "utils.safe_event_reader"
+local evread = require "utils.evread"
 local sysprof = require "sysprof.parse"

 local stdout, stderr = io.stdout, io.stderr
@@ -80,7 +80,7 @@ local function dump(inputfile)
   -- 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(sysprof.parse, inputfile)
+  local events = evread(sysprof.parse, inputfile)

   for stack, count in pairs(events) do
     print(stack, count)
diff --git a/tools/utils/safe_event_reader.lua b/tools/utils/evread.lua
similarity index 82%
rename from tools/utils/safe_event_reader.lua
rename to tools/utils/evread.lua
index 39246a9d..394a4a03 100644
--- a/tools/utils/safe_event_reader.lua
+++ b/tools/utils/evread.lua
@@ -4,12 +4,12 @@ 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))
+    io.stderr:write(string.format('\n\t%s\n', err))
     os.exit(1, true)
   end
 end

-local function safe_event_reader(parser, inputfile)
+return function(parser, inputfile)
   local _, reader = xpcall(
     bufread.new,
     make_error_handler('Failed to open %s.', inputfile),
@@ -30,5 +30,3 @@ local function safe_event_reader(parser, inputfile)
   )
   return events, symbols
 end
-
-return safe_event_reader
===

Regards,
Maxim Kokryashkin

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

Max, thanks for corerections! LGTM

On 1/9/24 16:54, Maxim Kokryashkin wrote:
> 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