From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96])
 (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 753A4446439
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 30 Sep 2020 01:41:51 +0300 (MSK)
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
References: <cover.1600862684.git.imun@tarantool.org>
 <6b7def49c9d2252425d3944cc4274c9a155ae9e9.1600862684.git.imun@tarantool.org>
Message-ID: <f903f4f2-1317-5121-c3f8-eeb179c0582b@tarantool.org>
Date: Wed, 30 Sep 2020 00:41:49 +0200
MIME-Version: 1.0
In-Reply-To: <6b7def49c9d2252425d3944cc4274c9a155ae9e9.1600862684.git.imun@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v2 2/2] lua: abort trace recording
 on fiber yield
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Igor Munkin <imun@tarantool.org>, Sergey Ostanevich <sergos@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Thanks for the patch!

Functionally all looks good. See one comment related not to Lua.

> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 8e98f7607..6e38f5734 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -1309,10 +1310,61 @@ tarantool_lua_utils_init(struct lua_State *L)
>  	return 0;
>  }
>  
> +/*
> + * XXX: There is already defined <panic> macro in say.h header
> + * (included in diag.h). As a result the call below is misexpanded
> + * and compilation fails with the corresponding error. To avoid
> + * this error the macro is temporary renamed and restored later.
> + * Compilation now fails for the following cases:
> + * * temporary name <gh1700_panic> is used in scope of this
> + *   translation unit
> + * * <panic> is not define, so this hack can be freely dropped
> + */
> +#if defined(panic) && !defined(gh1700_panic)
> +#  define gh1700_panic panic
> +#  undef panic
> +#else
> +#  error "Can't redefine <panic> macro"
> +#endif

Unfortunately, it won't work. Macros are not variables. You
can's assign them like that. I wish we could.

What you did here is you eliminated 'panic' macro, and added a
new macro 'gh1700_panic' whose value is "panic". Not what was
in the old 'panic', but exactly "panic". So the old value of 'panic'
is lost here.

Try to run gcc -E, or try to use gh1700_panic, and you will see it.
Below I tried to use it, but got an error.

	@@ -1363,6 +1363,7 @@ void cord_on_yield(void)
	 	 * lead to a failure on any next compiler phase.
	 	 */
	 	lj_trace_abort(g);
	+	gh1700_panic("message");
	 }
	tarantool/src/lua/utils.c:1366:2: error: implicit declaration of function 'panic' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        	gh1700_panic("message");

Also when you try to restore the old 'panic', it also won't work
again. As an alternative you can just do '#undef panic', since your
code is in the end of the file anyway, it won't break shit. Or make
a separate patch, which would turn panic() into a function.