Hi, Sergey! Thanks for the fixes! LGTM -- Best regards, Maxim Kokryashkin     >  >>Hi, Maxim! >>Thanks for the review! >>Fixed your comments inline and force-pushed the branch. >>See the iterative patch below. >> >>On 10.11.23, Maxim Kokryashkin wrote: >>> Hi, Sergey! >>> Thanks for the patch! >>> Please consider my comments below. >>> >>> >>> >>> > 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 >>> >>> >>> >>> > +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. >> >>Fixed, thanks! >> >>> > +test_aref_fwd_tnew(2) >>> > +-- Now run the trace with the clearing table, from which we take >>> > +-- AREF. >>> Same rephrasing required. Here and below. >> >>Changed to "run the trace and clear the table ...". >> >>> > +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. >> >>Yes, the loop itself can have side effects, so I prefere to ignore it. >> >>> >> >> >> >>> > +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)/ >> >>Fixed. >> >>> > + 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. >> >>Done. >> >>> > + >>> > +-- 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 >>> > >> >>=================================================================== >>diff --git a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua >>index e662e0cc..e1fbc2ed 100644 >>--- a/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua >>+++ b/test/tarantool-tests/lj-792-hrefk-table-clear.test.lua >>@@ -83,41 +83,6 @@ local function test_href_fwd_tdup(tab_number) >>   return field_value_after_clear >> end >>  >>-jit.opt.start('hotloop=1') >>- >>--- First compile the trace with a clearing not-interesting table. >>-test_aref_fwd_tnew(2) >>--- Now run the trace with the clearing table, from which we take >>--- AREF. >>-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') >>- >> local function test_not_forwarded_hrefk_val_from_newref(tab_number) >>   local field_value_after_clear >>   for _ = 1, NITERATIONS do >>@@ -135,18 +100,6 @@ local function test_not_forwarded_hrefk_val_from_newref(tab_number) >>   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 >>@@ -159,19 +112,62 @@ local function test_not_dropped_guard_on_hrefk(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. >>+ -- to incorrectly returned (swapped) values. >>     field_value_after_clear = tab.hrefk >>     tab.hrefk = MAGIC >>   end >>   return field_value_after_clear, tab.hrefk >> end >>  >>+jit.opt.start('hotloop=1') >>+ >>+-- First, compile the trace that clears the not-interesting table. >>+test_aref_fwd_tnew(2) >>+-- Now run the trace and clear the table, from which we take AREF. >>+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 that clears the not-interesting table. >>+test_aref_fwd_tdup(2) >>+-- Now run the trace and clear the 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 that clears the not-interesting table. >>+test_href_fwd_tnew(2) >>+-- Now run the trace and clear the 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 that clears the not-interesting table. >>+test_href_fwd_tdup(2) >>+-- Now run the trace and clear the table, from which we take HREF. >>+test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP') >>+ >>+-- XXX: Reset hotcounters to avoid collisions. >>+jit.opt.start('hotloop=1') >>+ >>+-- First, compile the trace that clears the not-interesting table. >>+test_not_forwarded_hrefk_val_from_newref(2) >>+-- Now run the trace and clear the 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') >>+ >> -- XXX: Reset hotcounters to avoid collisions. >> jit.opt.start('hotloop=1') >>  >>--- First compile the trace with clearing not interesting table. >>+-- First, compile the trace that clears the not-interesting table. >> test_not_dropped_guard_on_hrefk(2) >>--- Now run the trace with the clearing table, from which we take >>+-- Now run the trace and clear the table, from which we take >> -- HREFK. >> local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1) >>  >>=================================================================== >> >>-- >>Best regards, >>Sergey Kaplun >