From: Vitaliia Ioffe via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Sergey Ostanevich" <sergos@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix write barrier in BC_USETS.
Date: Wed, 11 Aug 2021 08:57:00 +0300 [thread overview]
Message-ID: <1628661420.573874146@f730.i.mail.ru> (raw)
In-Reply-To: <4BF6CB05-7904-4918-81BA-73149399FD04@tarantool.org>
[-- Attachment #1: Type: text/plain, Size: 10545 bytes --]
Hi team,
QA LGTM
--
Vitaliia Ioffe
>Вторник, 10 августа 2021, 19:41 +03:00 от Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
>
>Hi!
>
>Thanks for the patch, LGTM.
>
>I have no other explanation than that ‘missed flag as a result of compare’.
>But you did it better - took time for me to get to the point.
>BTW, will make easier to grasp with a simple C equivalent, like
>
>- if (iswite(str) || closed != 0)
>+ if (iswite(str) || closed == 0)
>
>Sergos.
>
>
>
>>On 9 Aug 2021, at 19:01, Sergey Kaplun < skaplun@tarantool.org > wrote:
>>Igor, thanks for the feedback!
>>
>>On 08.08.21, Igor Munkin wrote:
>>>Sergey,
>>>
>>>Thanks for the fixes! See some new comments below.
>>>
>>>On 01.08.21, Sergey Kaplun wrote:
>>>>Igor,
>>>>
>>>>Thanks for the review!
>>>>Update commit message on the branch, considering you comments.
>>>Got it, but I still have some more comments regarding it.
>>>
>>>
>>>>See answers to you questions below.
>>>>
>>><snipped>
>>>
>>>
>>>
>>>>>>| ccmp TMP0w, #0, #0, ne
>>>>>>| beq <1 // branch out from barrier movement
>>>>>>`TMP0w` contains `upvalue->closed` field. If it equals NULL (the first
>>>>>>`#0`). The second zero is the value of NZCV condition flags set if the
>>>>>>condition (`ne`) is FALSE [1][2]. If the set value is not white, then
>>>>>>flags are set to zero and branch is not taken (no Zero flag). If it
>>>>>>happens at propagate or atomic GC State and the `lj_gc_barrieruv()`
>>>>>>function is called then the gray value to set is marked as white. That
>>>>>>leads to the assertion failure in the `gc_mark()` function.
>>>>>OK, I understand almost nothing from the part above. Here are the
>>>>>comments:
>>>>>1. "If it equals NULL (the first `#0`)", then what?
>>>>My bad:
>>>>I mean here:
>>>>If it equals NULL (the first `#0`), then the upvalue is open.
>>>So why do you use NULL instead of 0? The field is uint8_t type, so 0 is
>>>much clearer.
>>Changed.
>>
>>
>>>>Added this.
>>>>
>>>>>2. Just to check we are on the same page: the second "immediate"
>>>>>mentioned in docs[1] is NZCV?
>>>>Yes.
>>>>
>>>>> Then beq <1 branch is not taken since
>>>>>(TMP0w != 0) is FALSE (i.e. upvalue is not closed), but zero flag in
>>>>>NZCV value is not set?
>>>>Yes.
>>>>
>>>>> So how does the color of the value to be stored
>>>>>relate to this control flow?
>>>>This NZCV value isn't set if the upvalue is white, because condition is
>>>>of the following instruction
>>>>
>>>>| tst TMP1w, #LJ_GC_WHITES // iswhite(str)
>>>>
>>>>is TRUE. So the <1 branch is taken, because the upvalue is closed.
>>>Well... I can't imagine how I needed to find this... This relates mostly
>>>to ARM docs you've mentioned, but it would be nice to describe this
>>>behaviour in the commit message (since you're writing a verbose one).
>>>
>>>
>>>>>3. AFAICS, if the branch is not taken and <lj_gc_barrieruv> is called at
>>>>>propagate or atomic phase, the value is colored either to gray or black.
>>>>Yes, that leads to the assertion failure mentioned in the ticket in the
>>>>LuaJIT upstream.
>>>>
>>>>
>>>>
>>>>>>This patch changes yielded NZCV condition flag to 4 (Zero flag is up) to
>>>>>>take the correct branch after `ccmp` instruction.
>>>>>>
>>>>>>Sergey Kaplun:
>>>>>>* added the description and the test for the problem
>>>>>>
>>>>>>[1]: https://developer.arm.com/documentation/dui0801/g/pge1427897656225
>>>>>>[2]: https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/condition-codes-1-condition-flags-and-codes
>>>>>Minor: Why #5629 is not mentioned?
>>>>Added.
>>>Considering everything above, I propose the following wording:
>>>| Contributed by Javier Guerra Giraldez.
>>>|
>>>| (cherry picked from commit c785131ca5a6d24adc519e5e0bf1b69b671d912f)
>>>|
>>>|
>>>| Closed upvalues are never gray. Hence when closed upvalue is marked, it
>>>| is marked as black. Black objects can't refer white objects, so for
>>>| storing a white value in a closed upvalue, we need to move the barrier
>>>| forward and color our value to gray by using `lj_gc_barrieruv()`. This
>>>| function can't be called on closed upvalues with non-white values since
>>>| there is no need to mark it again.
>>>|
>>>| USETS bytecode for arm64 architecture has the incorrect NZCV condition
>>>| flag value in the instruction that checks the upvalue is closed:
>>>| | tst TMP1w, #LJ_GC_WHITES
>>>| | ccmp TMP0w, #0, #0, ne
>>>| | beq <1 // branch out from barrier movement
>>>| `TMP0w` contains `upvalue->closed` field, so the upvalue is open if this
>>>| field equals to zero (the first one in `ccmp`). The second zero is the
>>>| value of NZCV condition flags[1] yielded if the specified condition
>>>| (`ne`) is met for the current values of the condition flags[2]. Hence,
>>>| if the value to be stored is not white (`TMP1w` holds its color), then
>>>| the condition is FALSE and all flags bits are set to zero so branch is
>>>| not taken (Zero flag is not set). If this happens at propagate or atomic
>>>| GC phase, the `lj_gc_barrieruv()` function is called and the gray value
>>>| to be set is marked like if it is white. That leads to the assertion
>>>| failure in the `gc_mark()` function.
>>>|
>>>| This patch changes NZCV condition flag to 4 (Zero flag is set) to take
>>>| the correct branch after `ccmp` instruction.
>>>|
>>>| Sergey Kaplun:
>>>| * added the description and the test for the problem
>>>|
>>>| [1]: https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/condition-codes-1-condition-flags-and-codes
>>>| [2]: https://developer.arm.com/documentation/dui0801/g/pge1427897656225
>>>|
>>>| Part of tarantool/tarantool#5629
>>Updated, as you've suggested.
>>
>>
>>
>>
>>><snipped>
>>>
>>>
>>>
>>>>>>src/vm_arm64.dasc | 2 +-
>>>>>>...6-arm64-incorrect-check-closed-uv.test.lua | 38 +++++++++++++++++++
>>>>>>2 files changed, 39 insertions(+), 1 deletion(-)
>>>>>>create mode 100644 test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>>>>>>
>>>>><snipped>
>>>>>
>>>>>>diff --git a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>>>>>>new file mode 100644
>>>>>>index 00000000..b757133f
>>>>>>--- /dev/null
>>>>>>+++ b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>>>>>>@@ -0,0 +1,38 @@
>>>>>>+local tap = require('tap')
>>>>>>+
>>>>>>+local test = tap.test('lj-426-arm64-incorrect-check-closed-uv')
>>>>>>+test:plan(1)
>>>>>>+
>>>>>>+-- Test file to demonstrate LuaJIT USETS bytecode incorrect
>>>>>>+-- behaviour on arm64 in case when non-white object is set to
>>>>>>+-- closed upvalue.
>>>>>>+-- See also, https://github.com/LuaJIT/LuaJIT/issues/426 .
>>>>>>+
>>>>>>+-- First, create a closed upvalue.
>>>>>>+do
>>>>>Minor: I'm not sure, we need a separate lexical block here. Could you
>>>>>please clarify the reason in the comment?
>>>>We need a closed upvalue. I suppose that it is the simpiest way to
>>>>create one. Please, provide a simplier example if you know one.
>>>My bad. Yes, the easiest way to emit UCLO bytecode is using a separate
>>>lexical block.
>>>
>>>
>>>
>>>>>>+ local uv -- luacheck: no unused
>>>>>>+ -- The function's prototype is created with the following
>>>>>>+ -- constants at chunk parsing. After adding this constant to
>>>>>>+ -- the function's prototype it will be marked as gray during
>>>>>>+ -- propogate phase.
>>>>>Then what does it test, if the constant is marked as gray? Will this
>>>>>string be white later?
>>>>It shouldn't be white, it should be gray, otherwise the aforementioned
>>>>condition is TRUE (remember, we need FALSE).
>>>Again, PEBKAC, thanks for the explanation.
>>>
>>>
>>>
>>>>>>+ local function usets() uv = '' end
>>>>>>+ _G.usets = usets
>>>>>>+end
>>>>>>+
>>>>>>+-- Set GC state to GCpause.
>>>>>>+collectgarbage()
>>>>>>+-- Do GC step as often as possible.
>>>>>>+collectgarbage('setstepmul', 100)
>>>>>Minor: Don't get, why you need to make GC less aggressive for the test.
>>>>>The test is run, until propagate phase is finished.
>>>>More likely, that it is run, until the upvalue is marked as black
>>>>during traversing (with the bug). I can remove this line if you insist.
>>>Drop it, please. I can't even *feel* its effect ;)
>>Done.
>>
>>
>>
>>
>>>>>>+
>>>>>>+-- We don't know on what exactly step our upvalue is marked as
>>>>>>+-- black and USETS become dangerous, so just check it at each
>>>>>>+-- step.
>>>>>>+-- Don't need to do the full GC cycle step by step.
>>>Minor: It would be nice to drop a few words about string and upvalue
>>>colours during this loop, but it's up to you.
>>Added.
>>
>>The iterative patch is the following:
>>
>>===================================================================
>>diff --git a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>>index b757133f..4cdf1211 100644
>>--- a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>>+++ b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>>@@ -21,9 +21,10 @@ end
>>
>>-- Set GC state to GCpause.
>>collectgarbage()
>>--- Do GC step as often as possible.
>>-collectgarbage('setstepmul', 100)
>>
>>+-- We want to wait for the situation, when upvalue is black,
>>+-- the string is gray. Both conditions are satisfied, when the
>>+-- corresponding `usets()` function is marked, for example.
>>-- We don't know on what exactly step our upvalue is marked as
>>-- black and USETS become dangerous, so just check it at each
>>-- step.
>>===================================================================
>>
>>
>>>>>>+local old_steps_atomic = misc.getmetrics().gc_steps_atomic
>>>>>>+while (misc.getmetrics().gc_steps_atomic == old_steps_atomic) do
>>>>>>+ collectgarbage('step')
>>>>>>+ usets() -- luacheck: no global
>>>>>>+end
>>>>>>+
>>>>>>+test:ok(true)
>>>>>>+os.exit(test:check() and 0 or 1)
>>>>>>--
>>>>>>2.31.0
>>>>>>
>>>>>[1]: https://lists.tarantool.org/tarantool-patches/20210719073632.12008-1-skaplun@tarantool.org/T/#u
>>>>>
>>>>>--
>>>>>Best regards,
>>>>>IM
>>>>--
>>>>Best regards,
>>>>Sergey Kaplun
>>>--
>>>Best regards,
>>>IM
>>--
>>Best regards,
>>Sergey Kaplun
[-- Attachment #2: Type: text/html, Size: 37774 bytes --]
next prev parent reply other threads:[~2021-08-11 5:57 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-07 14:36 Sergey Kaplun via Tarantool-patches
2021-08-01 10:39 ` Igor Munkin via Tarantool-patches
2021-08-01 17:00 ` Sergey Kaplun via Tarantool-patches
2021-08-08 19:28 ` Igor Munkin via Tarantool-patches
2021-08-09 16:01 ` Sergey Kaplun via Tarantool-patches
2021-08-09 19:46 ` Igor Munkin via Tarantool-patches
2021-08-10 16:40 ` Sergey Ostanevich via Tarantool-patches
2021-08-11 5:57 ` Vitaliia Ioffe via Tarantool-patches [this message]
2021-08-11 7:22 ` 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=1628661420.573874146@f730.i.mail.ru \
--to=tarantool-patches@dev.tarantool.org \
--cc=sergos@tarantool.org \
--cc=v.ioffe@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix write barrier in BC_USETS.' \
/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