Re: [Tarantool-patches] [PATCH] build: fix linker flags for executable on MacOS

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for your review!

On 28.04.21, Sergey Kaplun wrote:
> 
> Hi, Igor!
> 
> Thanks for the patch!
> LGTM, except a few nits below.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> On 28.04.21, Igor Munkin wrote:
> > This patch fixes inaccuracy in Tarantool build configuration introduced
> > by commit 07c83aab5c066ca75c149112b331b4dbb81b3f38 ('build: adjust
> > LuaJIT build system'). All those MacOS-related tweaks for __PAGEZERO
> > size and preferred load address for the bundle are necessary only for
> > builds with 32-bit GC area on 64-bit host. The only case fitting these
> > conditions is x86_64 with no LUAJIT_ENABLE_GC64. All other 64-bit builds
> > use 64-bit GC area unconditionally.
> > 
> > Part of #5983
> > Needed for #5629
> > Follows up #4862
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > This patch partially fixes the build on M1. I tested it on tntmac07
> > alongside with the changes Nikita made for libcoro[1]. As a result
> > Tarantool has been successfully built, but fails to start. CI looks to
> > be OK[2] except the known problems with ASAN[3].
> > 
> > Issue: https://github.com/tarantool/tarantool/issues/5983
> > Branch: https://github.com/tarantool/tarantool/tree/imun/gh-5983-fix-build-on-m1
> > 
> > [1]: https://github.com/tarantool/tarantool/commit/309ce38
> > [2]: https://github.com/tarantool/tarantool/commit/5465d7b
> > [3]: https://github.com/tarantool/tarantool/issues/6031
> > 
> > 
> >  cmake/luajit.cmake | 20 +++++++++++++++-----
> 
> Should we add corresponding ChangeLog entry?

Ugh... At first, I have no idea what do we need to write for this patch.
This is a partial fix for the build on M1. There is more work to be
done. The issue is not resolved by it, so it's too odd to me to add the
ChangeLog entry for such changes. BTW, as we discussed in chat, the
target branch for these patches is not the default one, but
wip-m1/master[1]. I guess, we can add everything necessary, when
Tarantool passes the tests on M1.

> 
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> > index 3d37164e8..9390d0dfd 100644
> > --- a/cmake/luajit.cmake
> > +++ b/cmake/luajit.cmake
> > @@ -73,11 +73,21 @@ if(ENABLE_ASAN)
> >      add_definitions(-DLUAJIT_USE_ASAN=1)
> >  endif()
> >  
> > -if(TARGET_OS_DARWIN)
> > -    # Necessary to make LuaJIT (and Tarantool) work on Darwin, see
> > -    # http://luajit.org/install.html.
> > -    set(CMAKE_EXE_LINKER_FLAGS
> > -        "${CMAKE_EXE_LINKER_FLAGS} -pagezero_size 10000 -image_base 100000000")
> > +if(TARGET_OS_DARWIN AND NOT LUAJIT_ENABLE_GC64)
> > +    # XXX: This is not the best idea to build LuaJIT on MacOS
> > +    # with GC64 disabled. But nobody will stop you from this.
> > +    # You are warned. For more info see the following issue.
> 
> Typo: s/For more info/For more info,/

OK, fixed.

> 
> > +    # https://github.com/tarantool/tarantool/issues/2643
> > +    message(WARNING "LUAJIT_ENABLE_GC64 is disabled for MacOS. "
> > +                    "See #2643, why this is not a good idea.")
> 
> Nit: may be the full link is better here (like we do for coredumps).
> Feel free to ignore.

If it makes someone happier. Diff with both fixes is below:

================================================================================

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 9390d0dfd..6a94327a7 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -76,10 +76,11 @@ endif()
 if(TARGET_OS_DARWIN AND NOT LUAJIT_ENABLE_GC64)
     # XXX: This is not the best idea to build LuaJIT on MacOS
     # with GC64 disabled. But nobody will stop you from this.
-    # You are warned. For more info see the following issue.
+    # You are warned. For more info, see the following issue.
     # https://github.com/tarantool/tarantool/issues/2643
     message(WARNING "LUAJIT_ENABLE_GC64 is disabled for MacOS. "
-                    "See #2643, why this is not a good idea.")
+                    "If one wants to know why this is not a good idea, "
+                    "see https://github.com/tarantool/tarantool/issues/2643.")
     # XXX: Necessary to make LuaJIT (and hence Tarantool) work on
     # Darwin/x86_64, see the following links for more info:
     # * http://luajit.org/install.html#embed

================================================================================

> 
> > +    # XXX: Necessary to make LuaJIT (and hence Tarantool) work on
> > +    # Darwin/x86_64, see the following links for more info:
> > +    # * http://luajit.org/install.html#embed
> > +    # * https://github.com/tarantool/luajit/blob/789820a/cmake/SetTargetFlags.cmake
> > +    if(CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64")
> > +        set(CMAKE_EXE_LINKER_FLAGS
> > +            "${CMAKE_EXE_LINKER_FLAGS} -pagezero_size 10000 -image_base 100000000")
> > +    endif()
> >  endif()
> >  
> >  # Define the locations for LuaJIT sources and artefacts.
> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://github.com/tarantool/tarantool/tree/wip-m1/master

-- 
Best regards,
IM