Re: [Tarantool-patches] [PATCH v2 2/2] lua: abort trace recording on fiber yield

[Vladislav Shpilevoy <v.shpilevoy@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.