From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix emit_rma(). Date: Mon, 3 Apr 2023 09:02:16 +0000 [thread overview] Message-ID: <ZCqWGIv9Vv6OWaGZ@tarantool.org> (raw) In-Reply-To: <20230322082739.25391-1-skaplun@tarantool.org> Sergey, Thanks for the patch! LGTM, considering the fixes for Max comments. On 22.03.23, Sergey Kaplun via Tarantool-patches wrote: > From: Mike Pall <mike> > > (cherry picked from commit 7e662e4f87134f1e84f7bea80933e033c5bf53a3) > > The accessing of memory address for some operation `emit_rma()` may be > encoded in one of the following ways: > a. If the offset of the accessing address from the dispatch table > (pinned to r14 that is not changed while trace execution) fits into > 32-bit, then encode this as an access to 32-bit displacement > relative to r14. > b. If the offset of the accessing address from the mcode (i.e. rip) > fits into 32-bit, then encode this as an access to 32-bit > displacement relative to rip (considering long mode specifics and > `RID_RIP` hack). > c. If the address doesn't fit into 32-bit one and we use `mov` or > `movsd`, then encode 64-bit load from this address. > d. Elsewhere, encode it as an access to 32-bit (the address should fit > into 32-bit one) displacement (the only option for non-GC64 mode). > > So, each instruction in GC64 mode differs from `mov` or `movsd` should > be encoded via the last option. But if we got a 64-bit address with a > big enough offset it can't be encoded and the assertion in `ptr2addr()` > will fail. > > There are several cases, when `emit_rma()` is used with non `mov` > instruction: > * `IR_LDEXP` with `fld` instruction for loading constant > number `TValue` by address. > * `IR_OBAR` with the corresponding `test` instruction on > `marked` field of `GCobj`. > All these instructions require an additional register to store value by > address. We can't truly allocate a register here due to possibility to > break IR assembling which depends on specific register usage. So, we use > and restore r14 here for emitting. > > Also, this patch removes `movsd` from condition from the `x86Op` type > check, as far as it never uses for the `emit_rma()` routine (see also > `emit_loadk64()` for details). > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#8069 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-emit-rma > PR: https://github.com/tarantool/tarantool/pull/8477 > Related issue: https://github.com/tarantool/tarantool/issues/8069 > > AFAICS, other places with `emit_rma()` usage are not related to the > patch as far as they take an offset for the address of JIT constants > stored in `jit_State`, so it always be near enough to dispatch. > > Side note: you may check test-correctness of the last check with GC by > changing the corresponding condition check on `GC_WHITES` in asm_obar to > CC_NZ (like it will be treated for incorrect check). Be carefull, member > that instructions are emitted from bottom to top! > > src/lj_emit_x86.h | 24 ++++- > test/tarantool-tests/fix-emit-rma.test.lua | 102 +++++++++++++++++++++ > 2 files changed, 123 insertions(+), 3 deletions(-) > create mode 100644 test/tarantool-tests/fix-emit-rma.test.lua > <snipped> > -- > 2.34.1 > -- Best regards, IM
next prev parent reply other threads:[~2023-04-03 9:10 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-03-22 8:27 Sergey Kaplun via Tarantool-patches 2023-03-23 9:36 ` Maxim Kokryashkin via Tarantool-patches 2023-03-24 13:05 ` Sergey Kaplun via Tarantool-patches 2023-03-24 14:42 ` Maxim Kokryashkin via Tarantool-patches 2023-04-03 9:02 ` Igor Munkin via Tarantool-patches [this message] 2023-05-25 6:16 ` 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=ZCqWGIv9Vv6OWaGZ@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix emit_rma().' \ /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