Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>,
	Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] test: add utility for parsing `jit.dump`
Date: Tue, 16 May 2023 13:55:47 +0300	[thread overview]
Message-ID: <4959c9e3-e0e3-7d26-1a7f-836849cecf4c@tarantool.org> (raw)
In-Reply-To: <44354d682d3cabf5a3ab1a268f900d7e6f981020.1683720396.git.skaplun@tarantool.org>

Hello, Sergey

Thanks for the patch! See my three comments inline.

On 5/10/23 15:34, Sergey Kaplun wrote:
> This commit adds simple parser for traces to be analyzed in the test.
> For now nothing too fancy at all -- just start `jit.dump` to temporary
> 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
> 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

The main purpose of this script is using in tests. It could be dangerous 
to leave it without tests at all,

breakage in it could require time for debugging someday. From other side 
I believe no one wants

writing tests for test infrastructure. I suggest add at least a couple 
of unit tests

that will parse trace in plain text format and check that results are 
equal to expected.

> @@ -0,0 +1,156 @@
> +local jdump = require('jit.dump')
> +
> +local M = {}
> +
> +local trace_mt = {}
> +trace_mt.__index = trace_mt
> +
> +-- Find and return the first IR ref and value matched the pattern
> +-- for the trace.
> +trace_mt.has_ir = function(trace, ir_pattern)
> +  for i = 1, #trace.ir do
> +    local ir = trace.ir[i]
> +    if ir:match(ir_pattern) then
> +      return i, ir
> +    end
> +  end
> +end
> +
> +local function trace_new(n)
> +  return setmetatable({
> +    number = n,
> +    parent = nil,
> +    parent_exitno = nil,
> +    start_loc = nil,
> +    bc = {},
> +    ir = {},
> +    mcode = {},
> +    snaps = {},
> +  }, trace_mt)
> +end
> +
> +local function parse_bc(trace, line)
> +  trace.bc[#trace.bc + 1] = line
> +end
> +
> +local function parse_snap(trace, n_snap, line)
> +  assert(trace.snaps[n_snap] == nil)
> +  trace[n_snap] = {ref = #trace.ir + 1, slots = line:match('%[(.*)%]$'),}
> +end
> +
> +local function parse_ir(trace, line)
> +  local n_snap = line:match('SNAP%s+#(%d+)')
> +  if n_snap then
> +    parse_snap(trace, n_snap, line)
> +    return
> +  end
> +
> +  local current_ins, ir = line:match('^(%d+)%s+(.*)$')
> +  current_ins = tonumber(current_ins)
> +  -- Insert NOP instruction hidden in IR dump.
> +  if current_ins ~= #trace.ir + 1 then
> +    trace.ir[#trace.ir + 1] = 'nil NOP'
> +  end
> +  assert(current_ins == #trace.ir + 1)
> +  trace.ir[current_ins] = ir
> +end
> +
> +local function parse_mcode(trace, line)
> +  -- Skip loop label.
> +  if line == '-> LOOP:' then
> +    return
> +  end
> +  local addr, instruction = line:match('^(%w+)%s+(.*)$')
> +  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 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
> +  elseif status == 'IR' then
> +    ctx.parsing = 'IR'
> +  elseif status == 'mcode' then
> +    ctx.parsing = 'mcode'
> +  else
> +    error('Unknown trace status: ' .. status)
> +  end
> +end
> +
> +local function parse_line(ctx, line)
> +  if line == '' then
> +    return
> +  end
> +
> +  if line:match('TRACE flush') then
> +    ctx.traces = {}
> +    return
> +  end
> +
> +  local trace_num, status = line:match('TRACE (%d+) (%w+)')
> +  if trace_num then
> +    parse_trace_hdr(ctx, line, tonumber(trace_num), status)
> +    return
> +  end
> +
> +  assert(ctx.parsing_trace)
> +
> +  local trace = ctx.traces[ctx.parsing_trace]
> +  if ctx.parsing == 'bc' then
> +    parse_bc(trace, line)
> +  elseif ctx.parsing == 'IR' then
> +    parse_ir(trace, line)
> +  elseif ctx.parsing == 'mcode' then
> +    parse_mcode(trace, line)
> +  end
> +end
> +
> +local JDUMP_FILE
> +
> +local function parse_jit_dump()
> +  local ctx = {traces = {}}
> +  for line in io.lines(JDUMP_FILE) do
> +    parse_line(ctx, line)
> +  end
> +  return ctx.traces
> +end
> +
> +M.start = function(flags)

Comment with description of function's arguments will be useful here and 
M.finish().

For internal functions too, but it is up to you.

> +  assert(JDUMP_FILE == nil, 'jit_parse is already running')
> +  -- Always use plain text output.
> +  flags = flags .. '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)

I would put these three lines on top of file or even disable/enable 
compilation in M.start() and M.finish().

Without enabling compilation back you are mutating environment on using 
this script and

this can be unexpected for those who will use scripts.

> +
> +return M

  parent reply	other threads:[~2023-05-16 10:55 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-10 12:34 [Tarantool-patches] [PATCH luajit 0/2] Fix canonicalization of +-0.0 keys for IR_NEWREF Sergey Kaplun via Tarantool-patches
2023-05-10 12:34 ` [Tarantool-patches] [PATCH luajit 1/2] test: add utility for parsing `jit.dump` Sergey Kaplun via Tarantool-patches
2023-05-15 11:11   ` Maxim Kokryashkin via Tarantool-patches
2023-05-15 12:00     ` Maxim Kokryashkin via Tarantool-patches
2023-05-21  7:47       ` Sergey Kaplun via Tarantool-patches
2023-05-21  7:39     ` Sergey Kaplun via Tarantool-patches
2023-05-22  7:04       ` Sergey Kaplun via Tarantool-patches
2023-05-29 13:55       ` Maxim Kokryashkin via Tarantool-patches
2023-05-16 10:55   ` Sergey Bronnikov via Tarantool-patches [this message]
2023-05-22  7:02     ` Sergey Kaplun via Tarantool-patches
2023-05-22  9:14       ` Sergey Kaplun via Tarantool-patches
2023-05-10 12:34 ` [Tarantool-patches] [PATCH luajit 2/2] Fix canonicalization of +-0.0 keys for IR_NEWREF Sergey Kaplun via Tarantool-patches
2023-05-15 12:05   ` Maxim Kokryashkin via Tarantool-patches
2023-05-20 15:03     ` Sergey Kaplun via Tarantool-patches
2023-05-16 12:17   ` Sergey Bronnikov via Tarantool-patches
2023-05-20 14:54     ` Sergey Kaplun via Tarantool-patches
2023-05-22  7:55       ` Sergey Bronnikov via Tarantool-patches
2023-06-27 13:28 ` [Tarantool-patches] [PATCH luajit 1/3] test: split utils.lua into several modules Igor Munkin via Tarantool-patches
2023-06-27 13:35   ` Igor Munkin via Tarantool-patches
2023-06-28 11:36   ` Sergey Kaplun via Tarantool-patches
2023-06-28 16:07     ` Igor Munkin via Tarantool-patches
2023-07-04 17:10 ` [Tarantool-patches] [PATCH luajit 0/2] Fix canonicalization of +-0.0 keys for IR_NEWREF Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4959c9e3-e0e3-7d26-1a7f-836849cecf4c@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 1/2] test: add utility for parsing `jit.dump`' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox