Re: [Tarantool-patches] [PATCH luajit] Fix HREFK forwarding vs. table.clear().

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

<snipped>

> diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua
> new file mode 100644
> index 00000000..e662e0cc
> --- /dev/null
> +++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua

<snipped>

> +jit.opt.start('hotloop=1')
> +
> +-- First compile the trace with a clearing not-interesting table.
It is better to rephrase it the following way:
| First, compile the trace that clears the not-interesting table.
Otherwise, `clearing` seems to be an adjective referring to the table.
Here and below.
> +test_aref_fwd_tnew(2)
> +-- Now run the trace with the clearing table, from which we take
> +-- AREF.
Same rephrasing required. Here and below.
> +test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
> +
> +-- XXX: Reset hotcounters to avoid collisions.
> +jit.opt.start('hotloop=1')
> +
> +-- First compile the trace with a clearing not-interesting table.
> +test_aref_fwd_tdup(2)
> +-- Now run the trace with the clearing table, from which we take
> +-- AREF.
> +test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
> +
> +-- XXX: Reset hotcounters to avoid collisions.
> +jit.opt.start('hotloop=1')
> +
> +-- First compile the trace with a clearing not-interesting table.
> +test_href_fwd_tnew(2)
> +-- Now run the trace with the clearing table, from which we take
> +-- HREF.
> +test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
> +
> +-- XXX: Reset hotcounters to avoid collisions.
> +jit.opt.start('hotloop=1')
> +
> +-- First compile the trace with a clearing not-interesting table.
> +test_href_fwd_tdup(2)
> +-- Now run the trace with the clearing table, from which we take
> +-- HREF.
> +test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')

Maybe we can just iterate over those cases in a loop?
If it breaks the semantics, then feel free to ignore.

> +
> +local function test_not_forwarded_hrefk_val_from_newref(tab_number)
> +  local field_value_after_clear
> +  for _ = 1, NITERATIONS do
> +    -- Create a table on trace to make the optimization work.
> +    local tab = {}
> +    -- NEWREF to be forwarded.
> +    tab.hrefk = MAGIC
> +    -- Use an additional table to alias the created table with the
> +    -- `hrefk` key.
> +    local tab_array = {tab, {hrefk = 0}}
> +    table_clear(tab_array[tab_number])
> +    -- It should be `nil`, since it is cleared.
> +    field_value_after_clear = tab.hrefk
> +  end
> +  return field_value_after_clear
> +end
> +
> +-- XXX: Reset hotcounters to avoid collisions.
> +jit.opt.start('hotloop=1')
> +
> +-- First compile the trace with a clearing not-interesting table.
> +test_not_forwarded_hrefk_val_from_newref(2)
> +-- Now run the trace with the clearing table, from which we take
> +-- HREFK.
> +local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)
> +
> +test:is(value_from_cleared_tab, nil,
> +        'not forward the field value across table.clear')
> +
> +local function test_not_dropped_guard_on_hrefk(tab_number)
> +  local tab, field_value_after_clear
> +  for _ = 1, NITERATIONS do
> +    -- Create a table on trace to make the optimization work.
> +    tab = {hrefk = MAGIC}
> +    -- Use an additional table to alias the created table with the
> +    -- `hrefk` key.
> +    local tab_array = {tab, {hrefk = 0}}
> +    table_clear(tab_array[tab_number])
> +    -- It should be `nil`, since it is cleared.
> +    -- If the guard is dropped for HREFK, the value from the TDUP
> +    -- table is taken instead, without the type check. This leads
> +    -- to incorrect (swapped) returned values.
Typo: s/incorrect (swapped) returned/incorrectly returned (swapped)/
> +    field_value_after_clear = tab.hrefk
> +    tab.hrefk = MAGIC
> +  end
> +  return field_value_after_clear, tab.hrefk
> +end

Let's move those test case definitions to be right after the ones above,
that would be more consistent.
> +
> +-- XXX: Reset hotcounters to avoid collisions.
> +jit.opt.start('hotloop=1')
> +
> +-- First compile the trace with clearing not interesting table.
> +test_not_dropped_guard_on_hrefk(2)
> +-- Now run the trace with the clearing table, from which we take
> +-- HREFK.
> +local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)
> +
> +test:is(field_value_after_clear, nil, 'correct field value after table.clear')
> +test:is(tab_hrefk, MAGIC, 'correct value set in the table that was cleared')
> +
> +test:done(true)
> -- 
> 2.42.0
>