From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com> Cc: tml <tarantool-patches@dev.tarantool.org>, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3] lua/utils: fix fiber->fid print in cord_on_yield Date: Mon, 17 May 2021 10:36:42 +0300 [thread overview] Message-ID: <20210517073642.GK3944@tarantool.org> (raw) In-Reply-To: <YJ5p0ilfsQN8iDoo@grain> Cyrill, On 14.05.21, Cyrill Gorcunov wrote: > On Fri, May 14, 2021 at 01:41:15PM +0300, Igor Munkin wrote: > > Cyrill, > > > > Thanks for the fixes! Just a couple of nits. > > > > On 14.05.21, Cyrill Gorcunov via Tarantool-patches wrote: > > > This fixes a nit in commit 6af473778 > > > (fiber: use uint64_t for fiber IDs). > > > > If you want to mention this commit, please use the full hash and enclose > > I don't understand why you need a full hash? When the reference is generated > the git makes sure that id being generated is long enough among existing > commits to be parseble. But sure, no problem. Fun fact: I see no words regarding this in our contribution guide, so I have no answer for you, only examples. > > > the commit subject into single quotes: it's the common way used in repo. > > You can find an example here[1] (but GitHub strips the hash while > > rendering the webpage). > > > > > > > > Since lua_pushfstring doesn't support %llu format it > > > breaks the test > > > > Minor: It would be nice to refer Lua Reference Manual[2] then. > > Sure, thanks! > > > > + snprintf(buf, sizeof(buf), > > > + "fiber %llu is switched while running the" > > > + " compiled code (it's likely a function with" > > > + " a yield underneath called via LuaJIT FFI)", > > > + (long long)fiber()->fid); > > > > Why do you cast the value to long long (i.e. signed value), but use the > > unsigned %llu modifier? This might be OK, but it's not clear to me. > > Because only _size_ of argument does matter, and language standart > guarantees that signed and unsigned arguments are counterparts. Thus > instead of big "unsigned long long" specificator you can write a > shorthand "long long". I see now, thanks for clarification! > > Force pushed an update. Now LGTM, thanks. > --- > From: Cyrill Gorcunov <gorcunov@gmail.com> > Date: Fri, 14 May 2021 13:18:13 +0300 > > This fixes a nit in commit 6af4737789042b1e6673c421686bb4394a61f4a1 > ("fiber: use uint64_t for fiber IDs"). > > Since lua_pushfstring doesn't support %llu format [1] it > breaks the test > > app-tap/gh-1700-abort-recording-on-fiber-switch.test.lua > > [1] https://www.lua.org/manual/5.1/manual.html#lua_pushfstring > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> > --- > src/lua/utils.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/src/lua/utils.c b/src/lua/utils.c > index 0fbe700fc..3ce821374 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -1341,6 +1341,7 @@ void cord_on_yield(void) > * code misbehaviour and failures, so stop its execution. > */ > if (unlikely(tvref(g->jit_base))) { > + char buf[256]; > /* > * XXX: mcode is executed only in scope of Lua > * world and one can obtain the corresponding Lua > @@ -1348,10 +1349,12 @@ void cord_on_yield(void) > */ > struct lua_State *L = fiber()->storage.lua.stack; > assert(L != NULL); > - lua_pushfstring(L, "fiber %llu is switched while running the" > - " compiled code (it's likely a function with" > - " a yield underneath called via LuaJIT FFI)", > - (long long)fiber()->fid); > + snprintf(buf, sizeof(buf), > + "fiber %llu is switched while running the" > + " compiled code (it's likely a function with" > + " a yield underneath called via LuaJIT FFI)", > + (long long)fiber()->fid); > + lua_pushstring(L, buf); > if (g->panic) > g->panic(L); > exit(EXIT_FAILURE); > @@ -1376,11 +1379,14 @@ void cord_on_yield(void) > * GC hook is active and the platform is forced to stop. > */ > if (unlikely(g->hookmask & HOOK_GC)) { > + char buf[128]; > struct lua_State *L = fiber()->storage.lua.stack; > assert(L != NULL); > - lua_pushfstring(L, "fiber %d is switched while running GC" > - " finalizer (i.e. __gc metamethod)", > - fiber()->fid); > + snprintf(buf, sizeof(buf), > + "fiber %llu is switched while running GC" > + " finalizer (i.e. __gc metamethod)", > + (long long)fiber()->fid); > + lua_pushstring(L, buf); > if (g->panic) > g->panic(L); > exit(EXIT_FAILURE); > -- > 2.31.1 > -- Best regards, IM
next prev parent reply other threads:[~2021-05-17 7:58 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-14 10:19 [Tarantool-patches] [PATCH] lua/utils: fix fiber->fid print in Lua tracing Cyrill Gorcunov via Tarantool-patches 2021-05-14 10:32 ` [Tarantool-patches] [PATCH v2] lua/utils: fix fiber->fid print in cord_on_yield Cyrill Gorcunov via Tarantool-patches 2021-05-14 10:41 ` Igor Munkin via Tarantool-patches 2021-05-14 12:15 ` [Tarantool-patches] [PATCH v3] " Cyrill Gorcunov via Tarantool-patches 2021-05-17 7:36 ` Igor Munkin via Tarantool-patches [this message] 2021-05-17 8:22 ` Igor Munkin via Tarantool-patches 2021-05-17 8:22 ` Alexander V. Tikhonov 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=20210517073642.GK3944@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=imun@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v3] lua/utils: fix fiber->fid print in cord_on_yield' \ /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