From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 3D66D716A53; Tue, 5 Dec 2023 12:40:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3D66D716A53 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1701769218; bh=rVOGzkPdIR6zQwhGdrs66KUIlwoXfoz9W91X+bXDJbY=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=JenB4OGW48AkLQNIsL3M4+7279Xe9ES8T+NmqV/OUwGeS6SlIxqBR/q0pWsXuRiMx 6BPSLWBnMSIb6oDmmjmmm9ghD5qiQx2G1rpZXClBNQEu6g0vUugwe2QCYIAYj3eY1z gsnMlWqdAKqm5Cff0BJUVU7Xx2BFNjlasSLdppQY= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [95.163.41.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AF687716A53 for ; Tue, 5 Dec 2023 12:40:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AF687716A53 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1rARuZ-00B6hu-0p; Tue, 05 Dec 2023 12:40:16 +0300 Date: Tue, 5 Dec 2023 12:35:42 +0300 To: Maxim Kokryashkin Message-ID: References: <471c196302e1153f4493d429ad3b3d19b60b8fd5.1701696044.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <471c196302e1153f4493d429ad3b3d19b60b8fd5.1701696044.git.m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91486559A3ABF84E7CB0A098B493E0346EE397C45C3170F78182A05F5380850400CD3BB7C690F28B3E5D7163CE105D2C46AAF5345049F6587E6BA9A5505FDAB8C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE763424119D34F5CBFEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B12C6B1582157D838638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D829818A486747FD3F8D0F9A548ADABFEB117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC974A882099E279BDA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C088D2E8BEBF93D4B068655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C3B6C7E47A292E8D043847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5A9A66B03AB122C3113FA695BE0E7C2159BBB080C10DF431BF87CCE6106E1FC07E67D4AC08A07B9B01F9513A7CA91E555CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFE62DBF5E366F0CADFC2527FA9D53CD1CE72AF834A8E7DBC939D74D63A3F307566D10329F4AAC8FAE9157FEB932BFB23D06AE09C8C4E0D92CFBED2CBDED753E92A74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJEmydpeiNq++3IAF4zsQQg== X-DA7885C5: CB46360A85EA9883F3D1334613FC9AFD2F55F623523904BF4DF4DF84C75F7D65262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE333B9D101EF644A4A7E307EE9AAE09CFAC0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/4] profilers: print user-friendly errors X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > 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`. > + > 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 > -- > 2.43.0 > -- Best regards, Sergey Kaplun