Hi, Sergey!
Thanks for the fixes. LGTM now.
 
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Please consider my answers below.
>>
>>On 15.05.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> Please consider my comments below.
>>>  
>>> > 
>>> >>This commit adds simple parser for traces to be analyzed in the test.
>>> >Typo: s/adds/adds a/
>>
>>Fixed.
>>
>>> >>For now nothing too fancy at all -- just start `jit.dump` to temporary
>>> >Typo: s/temporary/a temporary/
>>
>>Fixed.
>>
>>> >>file and parse and remove this file when dump is stopped. The array
>>> >>with resulting traces is returned.
>>> >>
>>> >>For now, just find a single IR by pattern and return ref number and IR,
>>> >>if exists. More functionality may be added if it will be necessary for
>>> >Typo: s/if exists/if they exist/
>>> >Typo: s/it will be/it is/
>>
>>Fixed, thanks!
>>
>>> >>tests.
>>> >>---
>>> >> test/tarantool-tests/utils/jit_parse.lua | 156 +++++++++++++++++++++++
>>> >> 1 file changed, 156 insertions(+)
>>> >> create mode 100644 test/tarantool-tests/utils/jit_parse.lua
>>> >>
>>> >>diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua
>>> >>new file mode 100644
>>> >>index 00000000..fe8e0e08
>>> >>--- /dev/null
>>> >>+++ b/test/tarantool-tests/utils/jit_parse.lua
>>> >>@@ -0,0 +1,156 @@
>>
>><snipped>
>>
>>> >>+local function parse_trace_hdr(ctx, line, trace_num, status)
>>> >>+ local traces = ctx.traces
>>> >>+ if status == 'start' then
>>> >>+ local trace = trace_new(trace_num)
>>> >>+ trace.parent, trace.parent_exitno = line:match('start (%d+)/(%d+)')
>>> >>+ -- XXX: Assume, that source line can't contain spaces.
>>> >>+ -- For example, it's not "(command line)".
>>> >>+ trace.start_loc = line:match(' ([^%s]+)$')
>>> >>+ traces[trace_num] = trace
>>> >>+ ctx.parsing_trace = trace_num
>>> >>+ ctx.parsing = 'bc'
>>> >>+ elseif status == 'stop' then
>>> >>+ assert(ctx.parsing_trace == trace_num)
>>> >>+ ctx.parsing_trace = nil
>>> >>+ ctx.parsing = nil
>>> >>+ elseif status == 'abort' then
>>> >>+ assert(ctx.parsing_trace == trace_num)
>>> >>+ ctx.parsing_trace = nil
>>> >>+ ctx.parsing = nil
>>> >>+ traces[trace_num] = nil
>>> >Two branches below are almost identical. Is it possible to avoid
>>> >duplication?
>>
>>I suppose, no, especially since we may want to do some more work for
>>aborted traces (I just don't sure for now).
>>
>>> >Side note: Also, I like it much more when switch-like constructions
>>> >in Lua are implemented with tables, because it seems to be cleaner.
>>> >Feel free to ignore.
>>
>>Rewritten in this way, see the iterative patch below. Branch is
>>force-pushed.
>>
>>===================================================================
>>diff --git a/test/tarantool-tests/utils/jit_parse.lua b/test/tarantool-tests/utils/jit_parse.lua
>>index fe8e0e08..7e8f879e 100644
>>--- a/test/tarantool-tests/utils/jit_parse.lua
>>+++ b/test/tarantool-tests/utils/jit_parse.lua
>>@@ -64,9 +64,9 @@ local function parse_mcode(trace, line)
>>   trace.mcode[#trace.mcode + 1] = {addr = addr, instruction = instruction,}
>> end
>> 
>>-local function parse_trace_hdr(ctx, line, trace_num, status)
>>- local traces = ctx.traces
>>- if status == 'start' then
>>+local header_handlers = {
>>+ start = function(ctx, line, trace_num)
>>+ local traces = ctx.traces
>>     local trace = trace_new(trace_num)
>>     trace.parent, trace.parent_exitno = line:match('start (%d+)/(%d+)')
>>     -- XXX: Assume, that source line can't contain spaces.
>>@@ -75,23 +75,27 @@ local function parse_trace_hdr(ctx, line, trace_num, status)
>>     traces[trace_num] = trace
>>     ctx.parsing_trace = trace_num
>>     ctx.parsing = 'bc'
>>- elseif status == 'stop' then
>>+ end,
>>+ stop = function(ctx, line, trace_num)
>>+ local traces = ctx.traces
>>     assert(ctx.parsing_trace == trace_num)
>>     ctx.parsing_trace = nil
>>     ctx.parsing = nil
>>- elseif status == 'abort' then
>>+ end,
>>+ abort = function(ctx, line, trace_num)
>>+ local traces = ctx.traces
>>     assert(ctx.parsing_trace == trace_num)
>>     ctx.parsing_trace = nil
>>     ctx.parsing = nil
>>     traces[trace_num] = nil
>>- elseif status == 'IR' then
>>+ end,
>>+ IR = function(ctx, line, trace_num)
>>     ctx.parsing = 'IR'
>>- elseif status == 'mcode' then
>>+ end,
>>+ mcode = function(ctx, line, trace_num)
>>     ctx.parsing = 'mcode'
>>- else
>>- error('Unknown trace status: ' .. status)
>>- end
>>-end
>>+ end,
>>+}
>> 
>> local function parse_line(ctx, line)
>>   if line == '' then
>>@@ -105,7 +109,11 @@ local function parse_line(ctx, line)
>> 
>>   local trace_num, status = line:match('TRACE (%d+) (%w+)')
>>   if trace_num then
>>- parse_trace_hdr(ctx, line, tonumber(trace_num), status)
>>+ if (header_handlers[status]) then
>>+ header_handlers[status](ctx, line, tonumber(trace_num))
>>+ else
>>+ error('Unknown trace status: ' .. status)
>>+ end
>>     return
>>   end
>>===================================================================
>>
>>> >>+ elseif status == 'IR' then
>>> >>+ ctx.parsing = 'IR'
>>> >>+ elseif status == 'mcode' then
>>> >>+ ctx.parsing = 'mcode'
>>> >>+ else
>>> >>+ error('Unknown trace status: ' .. status)
>>> >>+ end
>>> >>+end
>>> >>+
>>
>><snipped>
>>
>>> >>+
>>> >>+local JDUMP_FILE
>>> >>+
>>> >>+local function parse_jit_dump()
>>> >>+ local ctx = {traces = {}}
>>> >>+ for line in io.lines(JDUMP_FILE) do
>>> >I think we should perform error checking for that io operation.
>>
>>It is done under the hood:
>>| $ src/luajit -e 'for _ in io.lines("unexisted") do end '
>>| src/luajit: (command line):1: bad argument #1 to 'lines' (unexisted: No such file or directory)
>>| stack traceback:
>>| [C]: in function 'lines'
>>| (command line):1: in main chunk
>>| [C]: at 0x55b73c281064
>>
>>> >>+ parse_line(ctx, line)
>>> >>+ end
>>> >>+ return ctx.traces
>>> >>+end
>>> >>+
>>> >>+M.start = function(flags)
>>> >>+ assert(JDUMP_FILE == nil, 'jit_parse is already running')
>>> >>+ -- Always use plain text output.
>>> >>+ flags = flags .. 'T'
>>> >Nit: What if either `A` or `H` is present in the flags? I believe, no
>>> >one is going to do such things, but anyway it is worth considering
>>> >as a thing to check. Feel free to ignore.
>>
>>It is rewritten by the last flag (i.e. appended `T`).
>>
>>> >>+ JDUMP_FILE = os.tmpname()
>>> >>+ jdump.start(flags, JDUMP_FILE)
>>> >>+end
>>> >>+
>>> >>+M.finish = function()
>>> >>+ assert(JDUMP_FILE ~= nil, 'jit_parse is not running')
>>> >>+ jdump.off()
>>> >>+ local traces = parse_jit_dump()
>>> >>+ os.remove(JDUMP_FILE)
>>> >>+ JDUMP_FILE = nil
>>> >>+ return traces
>>> >>+end
>>> >>+
>>> >>+-- Turn off compilation for these modules to avoid side effects.
>>> >>+jit.off(true, true)
>>> >>+jit.off(jdump.on, true)
>>> >>+jit.off(jdump.off, true)
>>> >>+
>>> >>+return M
>>> >>--
>>> >>2.34.1
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
>