Hello, Sergey,

thanks for fixes! LGTM

On 6/5/25 10:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> See my answers below.
> Fixed your comments regarding the commit message and force-pushed the
> branch.
>
> On 04.06.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for the patch!
>>
>> On 6/3/25 20:35, Sergey Kaplun wrote:
>>> This patch provides the LUAJIT_USE_PERFTOOLS flag via the CMake build
>>> system. It allows you to avoid the definition of the cognominal macro
>> it's better to write impersonally: It allows avoiding the definition of ...
>>
>> Feel free to ignore.
> Rephrased as has been suggested.
>
>>
>>> definition via CMAKE_C_FLAGS to use integration with the Linux perf
>>> tools interface [1] to resolve symbols for traces generated by a JIT.
>>>
>>> It may be used like the following:
>>>
>>> | perf record script -f luajit test.lua
>> seems command is incorrect, because it does not work for me:
> Ooops. I forget that the `--force` flag has been removed [1].
> Fixed.
>
> The resulting commit message is the following:
>
> | build: provide LUAJIT_USE_PERFTOOLS option
> |
> | This patch provides the LUAJIT_USE_PERFTOOLS flag via the CMake build
> | system. It allows avoiding the definition of the cognominal macro
> | definition via CMAKE_C_FLAGS to use integration with the Linux perf
> | tools interface [1] to resolve symbols for traces generated by a JIT.
> |
> | It may be used like the following:
> |
> | | perf record script luajit test.lua
> | | perf report -s symbol
> |
> | [1]:https://github.com/torvalds/linux/blob/master/tools/perf/Documentation/jit-interface.txt
> |
> | Resolves tarantool/tarantool#11300
>
>> <snipped>
>>
>> script: unexpected number of arguments
>> Try 'script --help' for more information.
>> <snipped>
>>
>> I've used instead:
>>
>> $ sudo perf record -F 2000 ./build/src/luajit fib.lua
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.024 MB perf.data (8 samples) ]
>>
>>> | perf report -s symbol
>>>
>> and "perf report /tmp/perf-1699839.map" to check that luajit report
>> symbols in map file.
> Don't get this part.
It is a note after testing the patch, please disregard it
>
> <snipped>
>
>>> Branch:https://github.com/tarantool/luajit/tree/skaplun/gh-11300-use-perftools-flag
>>> Issue:https://github.com/tarantool/tarantool/issues/11300
>>>
>>>    CMakeLists.txt | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>> index 854b3613..c0da4362 100644
>>> --- a/CMakeLists.txt
>>> +++ b/CMakeLists.txt
>>> @@ -259,6 +259,14 @@ if(LUAJIT_USE_VALGRIND)
>>>      AppendFlags(TARGET_C_FLAGS -DLUAJIT_USE_VALGRIND)
>>>    endif()
>>>    
>>> +# This creates a symbol table of the JIT-compiled code in
>>> +# a </tmp/perf-%d.map> (%d = pid of process) file. It should be
>>> +# used with Linux perf tools. See <src/lj_trace.c> for details.
>>> +option(LUAJIT_USE_PERFTOOLS "Linux perf JIT support" OFF)
>>> +if(LUAJIT_USE_PERFTOOLS)
>>> +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_USE_PERFTOOLS)
>>> +endif()
>> Adding a CMake flag means that we support it in our fork (users will
>> rely on this functionality).
>>
>> Do want a regression test for this option?
> I've honestly don't see a way to conveniently check for it, and it looks
> like overkill for now (since its functionality is rather frugal).
> Moreover, perf annotate is not working as expected with that.
>
> I'm glad to hear any ideas of yours about it.
>
We can build LuaJIT in CI with enabled macro, but without test

it does not guarantee anything. Ok, let's leave it without test for now.

>>> +
>>>    # This is the client for the GDB JIT API. GDB 7.0 or higher is
>>>    # required to make use of it. See lj_gdbjit.c for details.
>>>    # Enabling this causes a non-negligible overhead, even when not
> [1]:http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=4a4d371a4dfbd3b84a7eab8d535d4c7c3647b09e
>