From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 99878429FB4; Wed, 3 May 2023 11:32:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 99878429FB4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1683102743; bh=w0+HaeWnJzRpROSvDI+84wyOBk8Qs64g4oxLsaEeevE=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=dCyZ1vbjd1weJ62n4uObv2y+wrEq6p02F6TNtRcrpRcWv4N27CVaNVwLH+VaRbrWd uZy3DFpRG6r9mjG2A9dy0C2DGxcKoprABmXHj8PFOYiLK3ByLjz3+jVWfUJ+JnZxsQ nVJTqOLVHjENM3uIu3oyJpFZVwVrXM+z6oYSnZ5U= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [95.163.41.68]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id ECE1C354BEE for ; Wed, 3 May 2023 11:32:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ECE1C354BEE Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1pu7uP-002zRF-2c; Wed, 03 May 2023 11:32:21 +0300 Message-Id: <91DEE028-B10D-4B36-BBE2-FF18CE9AD514@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_F14FEC2D-7FFB-4C1B-9772-2932A251D5DE" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) Date: Wed, 3 May 2023 11:32:10 +0300 In-Reply-To: To: Sergey Kaplun References: <20230411203650.10125-1-skaplun@tarantool.org> <1681835638.341302219@f402.i.mail.ru> X-Mailer: Apple Mail (2.3731.500.231) X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9F572AEF33F1BC5851128A1613A14FC115F16707533FAA3AB182A05F538085040DA418B9C016C4087BA594130EF694BCD377E76E135EF133C70EF1BFE111B5ED0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AB524098FB2F2222EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379C6642364E0E74208638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C56C938F28346B1706DE9FFD826AA30B117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2E71C9CEE48A59EEA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520B28585415E75ADA9618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE437C869540D2AB0F03F1AB874ED890284AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C30D7879B950AD227EBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE798228CBAD4AC77F6731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A526DA44FEBB46D48EB07468A96F17E6F3A53FAAFAEFEB1E04F87CCE6106E1FC07E67D4AC08A07B9B01F9513A7CA91E555CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3468964757141B82E01FDFF9873B45BF501FB95964CB8D93734B20CD0193FDDEEE2D0C9A1BD791879F1D7E09C32AA3244CFF37CAE78DBB0ABEC3577E9EC2A29763A8CE788DE6831205FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojtUGOBMG7VVPbqoJUCTbapw== X-Mailru-Sender: 5AA3D5B9D8C486466A1CB9683971E69C22CBBA01B4BDFCBA56990EA7F3EABB3462FB2668C6B138E019381EE24192DF5555834048F03EF5D4C9A814A92B2E3B1BA4250FC3964EA4964198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: sergos via Tarantool-patches Reply-To: sergos Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --Apple-Mail=_F14FEC2D-7FFB-4C1B-9772-2932A251D5DE Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Hi! Thanks for the patch! Since test is x86_64 only - can we put an explicit skipcond then? Otherwise LGTM. Sergos. > On 2 May 2023, at 11:13, Sergey Kaplun wrote: >=20 > Hi, Max! > Thanks for the review! >=20 > On 18.04.23, Maxim Kokryashkin wrote: >>=20 >> Hi, Sergey! >> Thanks for the patch! >> LGTM, except for a few nits below and the single question. >>> =20 >>>> From: Mike Pall >>>>=20 >>>> Reported by Yichun Zhang. >>>>=20 >>>> (cherry picked from commit = 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1) >>>>=20 >>>> The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't = considered >>>> as 64 bit address. On GC64 mode it may lead to the following = assembly: >>> Typo: s/as 64 bit/a 64-bit/ >=20 > Fixed, thanks! >=20 >>>> | mov eax, edi >>>> so, high 32 bits of the reference are lost. >>> Typo: s/high/the high/ >>>>=20 >>>> This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider = `ASMREF_L` >>>> 64 bit long. Now the resulting assembly is the following: >>>> | mov rax, rdi >=20 > Fixed, thanks! >=20 > Branch is force-pushed. >=20 >>>>=20 >>>> False-positive `if` condition in is OK, since `op12` >>>> already initialized as 0. >>>>=20 >>>> False-positive `if` condition in , = , >>>> is OK, since `REF_NIL` is the last reference = before >>>> `REF_BASE` and this iteration of a cycle is still the last one. >>>>=20 >>>> Sergey Kaplun: >>>> * added the description and the test for the problem >>>>=20 >>>> Part of tarantool/tarantool#8516 >>>> --- >>>>=20 >>>> Branch: = https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l >>>> Related issues: >>>> * https://github.com/openresty/lua-resty-core/issues/144 >>>> * https://github.com/tarantool/tarantool/issues/8516 >>>> PR: https://github.com/tarantool/tarantool/pull/8553 >>>> ML: = https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-late= st-LuaJIT-v21-GC64-mode >>>>=20 >=20 > >=20 >>>> +local global_env >>>> +local _ >>>> +for i =3D 1, 4 do >>>> + -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`). >>>> + global_env =3D getfenv(0) >>>> + -- Need to reuse the register, to cause emitting of `mov` >>>> + -- instruction (see `ra_left()` in ). >>>> + _ =3D tostring(i) >>>> +end >>>> + >>>> +test:ok(global_env =3D=3D getfenv(0), 'IR_LREF assembling = correctness') >>>> + >>>> +os.exit(test:check() and 0 or 1) >>> Neither this test case, nor the original one from OpenResty fail = before the patch on OSX/ARM64. >>> Is it expected behavior or not? >=20 > Yes, I think that non x86_64 arches are unaffected, since they use > `ra_leftov()` instead. >=20 >>> On x86 GC64 it behaves as expected though. >>>> -- >>>> 2.34.1 >>> -- >>> Best regards, >>> Maxim Kokryashkin >>> =20 >=20 > --=20 > Best regards, > Sergey Kaplun --Apple-Mail=_F14FEC2D-7FFB-4C1B-9772-2932A251D5DE Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
Hi!

Thanks for = the patch!

Since test is x86_64 only - can we = put an explicit skipcond then?

Otherwise = LGTM.

Sergos.

On 2 May 2023, at 11:13, Sergey Kaplun = <skaplun@tarantool.org> wrote:

Hi, Max!
Thanks for the = review!

On 18.04.23, Maxim = Kokryashkin wrote:

Hi, Sergey!
Thanks for the = patch!
LGTM, except for a few nits below and the single = question.
 
From: Mike Pall <mike>

Reported by Yichun = Zhang.

(cherry picked from commit = 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)

The `ASMREF_L` = reference is defined as `REF_NIL`, so it isn't considered
as 64 bit = address. On GC64 mode it may lead to the following = assembly:
Typo: s/as 64 bit/a = 64-bit/

Fixed, = thanks!

| mov eax, edi
so, high 32 bits of the reference are = lost.
Typo: s/high/the high/

This patch adds `IRT_NIL` to `IRT_IS64` mask, to = consider `ASMREF_L`
64 bit long. Now the resulting assembly is the = following:
| mov rax, = rdi

Fixed, = thanks!

Branch is = force-pushed.


False-positive `if` condition in <src/lj_asm.c> = is OK, since `op12`
already initialized as 0.

False-positive = `if` condition in <src/lj_opt_sink.c>, = <src/lj_opt_split.c>,
<src/lj_record.c> is OK, since = `REF_NIL` is the last reference before
`REF_BASE` and this iteration = of a cycle is still the last one.

Sergey Kaplun:
* added the = description and the test for the problem

Part of = tarantool/tarantool#8516
---

Branch: =  https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-= l
Related issues:
* =  https://github.com/openresty/lua-resty-core/issues/144
* =  https://github.com/tarantool/tarantool/issues/8516
PR: =  https://github.com/tarantool/tarantool/pull/8553
ML: =  https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-th= e-latest-LuaJIT-v21-GC64-mode


<snipped>

+local global_env
+local _
+for i =3D 1, 4 do
+ = -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`).
+ = global_env =3D getfenv(0)
+ -- Need to reuse the register, to cause = emitting of `mov`
+ -- instruction (see `ra_left()` in = <src/lj_asm.c>).
+ _ =3D = tostring(i)
+end
+
+test:ok(global_env =3D=3D getfenv(0), = 'IR_LREF assembling correctness')
+
+os.exit(test:check() and 0 or = 1)
Neither this test case, nor the original one from = OpenResty fail before the patch on OSX/ARM64.
Is it expected = behavior or not?

Yes, I think that non = x86_64 arches are unaffected, since they use
`ra_leftov()` = instead.

On x86 GC64 = it behaves as expected though.
--
2.34.1
--
Best regards,
Maxim = Kokryashkin
 

-- 
Best regards,
Sergey = Kaplun

= --Apple-Mail=_F14FEC2D-7FFB-4C1B-9772-2932A251D5DE--