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 43BADB4A5C1; Thu, 4 Apr 2024 14:19:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 43BADB4A5C1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1712229569; bh=7Mpo8A0d0X1w/+039aC/lIZKFk8mfPuhBbi1y2blb+s=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=o4KiJy0yHeWwUcR5Cv8CM8u4h+maZ+FEyMLZDiCiZa/AXeED1AlOV54kxprd1r1GD Oa3jJTJr4/gilJ3wQ3Lt44Wno7a97ShslwNTPdyiCuOe+21WARoRzh5/nsF7dL8y35 3AXe9R4scRRMxgB1kl8fIQj8jeT8e6nf5Z8g5NUw= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (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 5E277B4A5C4 for ; Thu, 4 Apr 2024 14:19:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5E277B4A5C4 Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1rsL7u-000000058Qx-32k7; Thu, 04 Apr 2024 14:19:27 +0300 Message-ID: Date: Thu, 4 Apr 2024 14:19:26 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Maxim Kokryashkin , tarantool-patches@dev.tarantool.org, skaplun@tarantool.org References: <20240326160940.339283-1-m.kokryashkin@tarantool.org> Content-Language: en-US In-Reply-To: <20240326160940.339283-1-m.kokryashkin@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD96441E77B1D9F2D074A21705A51F494FDFFE49582455C4DEB182A05F538085040174A7DB299EB53E3D27678DDAA806314B2F628528F839AD8CE51CCDFCE521258F84BCC1498F71492 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F9D05773942AAE9CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E5A8E5ECB3FF24018638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D854F0DEE450C25820527F4587398CCF9AEFB3F21259B4CEAFCC7F00164DA146DAFE8445B8C89999728AA50765F790063793270F7220657A0A389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2D01283D1ACF37BA2D242C3BD2E3F4C64AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C319D302B05D018AA3BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFE478A468B35FE7671DD303D21008E298D5E8D9A59859A8B6D082881546D9349175ECD9A6C639B01B78DA827A17800CE7C7AF014947417506731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5F00A36317C7CD2D75002B1117B3ED696498018ABF56B0A22C66B2B37046EC955823CB91A9FED034534781492E4B8EEAD27E9584FBD6BDD31BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF9D983B4ED162B6B7E427AAC6147FA3BE883DD5B06B02933A8880707BCEEC77A0124E09ADABDE4174E2B85D705C0AC83B6DC19C64AC4B6397325CA1F8D1C70F597C74F526B5477A68C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXDAMXEVBmztiCdM0b15gOA== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B598C60B35AD9E961ABD27678DDAA806314B2F628528F839AD8B7CBEF92542CD7C8795FA72BAB74744FC77752E0C033A69EA16A481184E8BB1C9B38E6EA4F046BE03A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v1] 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Max thanks for the patch! LGTM with minor comments. On 3/26/24 19:09, Maxim Kokryashkin wrote: > Prior to this patch, there was no error-checking in profilers, > which resulted in raw Lua errors being reported in cases of > non-existing paths or corrupt file structures. This patch > makes use of the event reader module introduced in commit > df5a07218dba8cefa370499a1a54ba75fb73b99b ("profilers: introduce > event reader module") to add error handling, so all parsing > errors are now reported in a user-friendly manner. > > Tool CLI flag tests are adapted correspondingly to error message > changes. > > Resolves tarantool/tarantool#9217 > Part of tarantool/tarantool#5994 > --- > Branch: https://github.com/tarantool/luajit/tree/mkokryashkin/profile-parsers-refactoring-p2 > > .../gh-5688-tool-cli-flag.test.lua | 4 +- > ...17-profile-parsers-error-handling.test.lua | 90 +++++++++++++++++++ > tools/memprof.lua | 11 +-- > tools/memprof/parse.lua | 5 +- > tools/sysprof.lua | 10 +-- > tools/sysprof/parse.lua | 5 +- > tools/utils/symtab.lua | 6 +- > 7 files changed, 112 insertions(+), 19 deletions(-) > create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua > > diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > index 75293f11..8ead83b5 100644 > --- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua > @@ -42,7 +42,7 @@ local SMOKE_CMD_SET = { > local MEMPROF_CMD_SET = { > { > cmd = MEMPROF_PARSER .. BAD_PATH, > - like = 'fopen, errno: 2', > + like = 'Failed to open.*fopen, errno: 2', > }, > { > cmd = MEMPROF_PARSER .. TMP_BINFILE_MEMPROF, > @@ -61,7 +61,7 @@ local MEMPROF_CMD_SET = { > local SYSPROF_CMD_SET = { > { > cmd = SYSPROF_PARSER .. BAD_PATH, > - like = 'fopen, errno: 2', > + like = 'Failed to open.*fopen, errno: 2', > }, > { > cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF, > diff --git a/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua > new file mode 100644 > index 00000000..c2e0f0a6 > --- /dev/null > +++ b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua > @@ -0,0 +1,90 @@ > +local tap = require('tap') > +local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({ > + ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and > + jit.arch ~= 'x64', > + ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux', > + -- XXX: Tarantool integration is required to run this test properly. > + -- luacheck: no global > + ['No profile tools CLI option integration'] = _TARANTOOL, > +}) > + > +jit.off() > +jit.flush() > + > +local table_new = require('table.new') > +local utils = require('utils') > + > +local BAD_PATH = utils.tools.profilename('bad-path-tmp.bin') > +local NON_PROFILE_DATA = utils.tools.profilename('not-profile-data.tmp.bin') > +local CORRUPT_PROFILE = utils.tools.profilename('profdata.tmp.bin') > + > +local EXECUTABLE = utils.exec.luacmd(arg) > +local PARSERS = { > + memprof = EXECUTABLE .. ' -tm ', > + sysprof = EXECUTABLE .. ' -ts ', > +} > +local REDIRECT_OUTPUT = ' 2>&1' > + > +local TABLE_SIZE = 20 > + > +local TEST_CASES = { > + { > + path = BAD_PATH, > + err_msg = 'Failed to open' > + }, > + { > + path = NON_PROFILE_DATA, > + err_msg = 'Failed to parse symtab from' > + }, > + { > + path = CORRUPT_PROFILE, > + err_msg = 'Failed to parse profile data from' > + }, Nit: I would rename CORRUPT_PROFILE to CORRUPT_PROFILE_DATA for consitency with NON_PROFILE_DATA. > +} > + > +test:plan(2 * #TEST_CASES) > + > +local function generate_non_profile_data(path) > + local file = io.open(path, 'w') > + file:write('data') > + file:close() > +end > + > +local function generate_corrupt_profile(path) Nit: generate_corrupt_profile -> generate_corrupt_profile_data > + local res, err = misc.memprof.start(path) > + -- Should start successfully. > + assert(res, err) > + > + local _ = table_new(TABLE_SIZE, 0) > + _ = nil > + collectgarbage() > + > + res, err = misc.memprof.stop() > + -- Should stop successfully. > + assert(res, err) > + > + local file = io.open(path, 'r') > + local content = file:read('*all') > + file:close() > + local index = string.find(content, 'ljm') > + > + file = io.open(path, 'w') > + file:write(string.sub(content, 1, index - 1)) > + file:close() > +end > + > +generate_non_profile_data(NON_PROFILE_DATA) > +generate_corrupt_profile(CORRUPT_PROFILE) > + > +for _, case in ipairs(TEST_CASES) do > + for profiler, parser in pairs(PARSERS) do > + local path = case.path > + local err_msg = case.err_msg > + local output = io.popen(parser .. path .. REDIRECT_OUTPUT):read('*all') > + test:like(output, err_msg, string.format('%s: %s', profiler, err_msg)) > + end > +end > + > +os.remove(NON_PROFILE_DATA) > +os.remove(CORRUPT_PROFILE) > +test:done(true) > diff --git a/tools/memprof.lua b/tools/memprof.lua > index 537cb869..7d7e8e05 100644 > --- a/tools/memprof.lua > +++ b/tools/memprof.lua > @@ -10,10 +10,9 @@ > -- Major portions taken verbatim or adapted from the LuaVela. > -- Copyright (C) 2015-2019 IPONWEB Ltd. > > -local bufread = require "utils.bufread" > local memprof = require "memprof.parse" > local process = require "memprof.process" > -local symtab = require "utils.symtab" > +local evread = require "utils.evread" > local view = require "memprof.humanize" > > local stdout, stderr = io.stdout, io.stderr > @@ -108,9 +107,11 @@ local function parseargs(args) > end > > local function dump(inputfile) > - local reader = bufread.new(inputfile) > - local symbols = symtab.parse(reader) > - local events = memprof.parse(reader, symbols) > + -- XXX: This function exits with a non-zero exit code and > + -- prints an error message if it encounters any failure during > + -- the process of parsing. > + local events, symbols = evread(memprof.parse, inputfile) > + > if not config.leak_only then > view.profile_info(events, config) > end > diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua > index cc66a23e..11a79a1d 100644 > --- a/tools/memprof/parse.lua > +++ b/tools/memprof/parse.lua > @@ -206,12 +206,13 @@ function M.parse(reader, symbols) > local _ = reader:read_octets(3) > > if magic ~= LJM_MAGIC then > - error("Bad LJM format prologue: "..magic) > + error("Bad memprof event format prologue: "..magic) > end > > if string.byte(version) ~= LJM_CURRENT_VERSION then > error(string_format( > - "LJM format version mismatch: the tool expects %d, but your data is %d", > + "Memprof event format version mismatch:".. > + " the tool expects %d, but your data is %d", > LJM_CURRENT_VERSION, > string.byte(version) > )) > diff --git a/tools/sysprof.lua b/tools/sysprof.lua > index 8449b23f..420ca41c 100644 > --- a/tools/sysprof.lua > +++ b/tools/sysprof.lua > @@ -1,6 +1,5 @@ > -local bufread = require "utils.bufread" > +local evread = require "utils.evread" > local sysprof = require "sysprof.parse" > -local symtab = require "utils.symtab" > > local stdout, stderr = io.stdout, io.stderr > local match, gmatch = string.match, string.gmatch > @@ -78,9 +77,10 @@ local function parseargs(args) > end > > local function dump(inputfile) > - local reader = bufread.new(inputfile) > - local symbols = symtab.parse(reader) > - local events = sysprof.parse(reader, symbols) > + -- XXX: This function exits with a non-zero exit code and > + -- prints an error message if it encounters any failure during > + -- the process of parsing. > + local events = evread(sysprof.parse, inputfile) > > for stack, count in pairs(events) do > print(stack, count) > diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua > index 64c4a455..0e416d37 100755 > --- a/tools/sysprof/parse.lua > +++ b/tools/sysprof/parse.lua > @@ -237,12 +237,13 @@ function M.parse(reader, symbols) > local _ = reader:read_octets(3) > > if magic ~= LJP_MAGIC then > - error("Bad LJP format prologue: "..magic) > + error("Bad sysprof event format prologue: " .. tostring(magic)) > end > > if string.byte(version) ~= LJP_CURRENT_VERSION then > error(string_format( > - "LJP format version mismatch: the tool expects %d, but your data is %d", > + "Sysprof event format version mismatch:".. > + " the tool expects %d, but your data is %d", > LJP_CURRENT_VERSION, > string.byte(version) > )) > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index 0c878e7a..e80b9f33 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > @@ -95,13 +95,13 @@ function M.parse(reader) > local _ = reader:read_octets(3) > > if magic ~= LJS_MAGIC then > - error("Bad LJS format prologue: "..magic) > + error("Bad LuaJIT symbol table format prologue: " .. tostring(magic)) > end > > if string.byte(version) ~= LJS_CURRENT_VERSION then > error(string_format( > - "LJS format version mismatch:".. > - "the tool expects %d, but your data is %d", > + "LuaJIT symbol table format version mismatch:".. > + " the tool expects %d, but your data is %d", Nit: could be replaced with a single line patch after removing whitespace on a single line:    if string.byte(version) ~= LJS_CURRENT_VERSION then      error(string_format( -         "LJS format version mismatch:".. +         "LuaJIT symbol table format version mismatch: "..           "the tool expects %d, but your data is %d",           LJS_CURRENT_VERSION,           string.byte(version) > LJS_CURRENT_VERSION, > string.byte(version) > )) > -- > 2.44.0 >