Re: [Tarantool-patches] [PATCH luajit] Optionally return PC position in jit.util.tracesnap().

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,
Please consider remaining comments below.

On 16.10.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review! See my comments below.
> 
> Sergey

<snipped>

> >
> >> Sergey Bronnikov:
> >> * added the description and the test for the feature
> >> ---
> >>
> >> Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-tracesnap-getpos
> >>
> >>   src/lib_jit.c                                 |  9 +-
> >>   .../lj-1472-tracesnap-getpos.test.lua         | 89 +++++++++++++++++++
> >>   2 files changed, 97 insertions(+), 1 deletion(-)
> >>   create mode 100644 test/tarantool-tests/lj-1472-tracesnap-getpos.test.lua
> >>
> >> diff --git a/src/lib_jit.c b/src/lib_jit.c
> >> index 9f870f68f..5a0d38611 100644
> >> --- a/src/lib_jit.c
> >> +++ b/src/lib_jit.c

<snipped>

> >> +-- Return the name of the bytecode operation for the given
> >> +-- instruction, that is encoded in the low byte.
> >> +local function opname(ins)
> >> +  local oidx = BC_NAME_LENGTH * bit.band(ins, 0xff)
> >> +  local name = vmdef.bcnames:sub(oidx + 1, oidx + BC_NAME_LENGTH)
> >> +  return (name:gsub('%s+$', ''))
> > Minor: I suggest the more readable:
> > | return name:match('%a+')
> > Feel free to ignore.
> 
> this will break testcase:
> 
> tarantool> a = "FORL  "
> ---
> ...
> 
> tarantool> a:gsub('%a+', '')
> ---
> - '  '
> - 1
> ...
> 
> tarantool> a:gsub('%s+', '')
> ---
> - FORL
> - 1
> ...

Don't get your example:

| src/luajit -e 'local str = "FORL   " print(str:match("%a+"), (str:gsub("%s+$", "")))'
| FORL    FORL

<snipped>

> >> +local snap, pos = jutil.tracesnap(1, 0, true)
> >> +test:istable(snap, 'the snapshot table is returned')
> >> +test:is(pos, loop_entry, 'the PC position points to the loop entry')
> >> +
> >> +local _, pos_without = jutil.tracesnap(1, 0)
> >> +test:is(pos_without, nil, 'no PC position without getpos')
> >> +
> >> +local _, pos_false = jutil.tracesnap(1, 0, false)
> >> +test:is(pos_false, nil, 'no PC position with getpos=false')
> >> +
> >> +local all_valid = true
> >> +for sn = 0, info.nexit - 1 do
> >> +  local _, p = jutil.tracesnap(1, sn, true)
> >> +  if type(p) ~= 'number' or p < 0 or p >= nbc or
> >> +     jutil.funcbc(payload, p) == nil then
> >> +    all_valid = false
> >> +  end
> >> +end
> >> +test:ok(all_valid, 'positions of all snapshots are valid offsets')
> >> +
> >> +test:is(select('#', jutil.tracesnap(1, info.nexit, true)), 0,
> >> +        'out-of-range snapshot returns nothing')
> >> +
> > It would be nice to add any correctness test.
> > The most trivial in mind is the following:
> >
> > | src/luajit -Ohotloop=1 -e '
> > |   local jutil = require"jit.util"
> > |   local function trivial()
> > |     local a = 0
> > |     return a
> > |   end
> > |   trivial()
> > |   trivial()
> > |
> > |   local _, b = jutil.tracesnap(1, 0, true)
> > |   print(b)
> > |   local _, b = jutil.tracesnap(1, 1, true)
> > |   print(b)
> > | '
> > | 1
> > | 2
> >
> > The snapshot is emitted at the function entry and before returning the
> > result which makes it trivial to detect:
> >
> > | ---- TRACE 1 start (command line):3
> > | 0001  KSHORT   0   0
> > | 0002  RET1     0   2
> > | ---- TRACE 1 IR
> > | ....        SNAP   #0   [ ---- ]
> > | ....        SNAP   #1   [ ---- +0   ]
> >
> > Don't forget to reset the hotcounters during the recording of this
> > example.
> 
> ok, thanks, added:
> 
>   })
> 
> -test:plan(8)
> +test:plan(10)
> 
>   -- Entries in `vmdef.bcnames` are 6 characters long.
>   local BC_NAME_LENGTH = 6
> @@ -87,4 +87,21 @@ test:ok(all_valid, 'positions of all snapshots are 
> valid offsets')
>   test:is(select('#', jutil.tracesnap(1, info.nexit, true)), 0,
>           'out-of-range snapshot returns nothing')
> 
> +-- The same correctness check for a trace without a loop: the
> +-- first and the last snapshots of the trivial function point to
> +-- its first and second bytecode instructions, respectively.
> +jit.flush()

You should update the hotloop counters after flush to avoid clashing.

> +local function trivial()
> +  local a = 0
> +  return a
> +end
> +trivial()
> +trivial()
> +
> +local _, pos_first = jutil.tracesnap(1, 0, true)
> +test:is(pos_first, 1, 'the first snapshot points to the first instruction')
> +
> +local _, pos_last = jutil.tracesnap(1, 1, true)
> +test:is(pos_last, 2, 'the second snapshot points to the second 
> instruction')
> +
>   test:done(true)
> 
> >
> >> +test:done(true)
> >> -- 
> >> 2.51.0
> >>

-- 
Best regards,
Sergey Kaplun