Tarantool development patches archive
 help / color / mirror / Atom feed
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
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 --]

  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 \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git