[Tarantool-patches] [PATCH luajit 0/3] Basic fixes for LuaJIT on ARM64 Macs

[Tarantool-patches] [PATCH luajit 0/3] Basic fixes for LuaJIT on ARM64 Macs

[Igor Munkin via Tarantool-patches]

This series consists of the three patches: the one for CMake build
machinery and two commits backported from the vanilla LuaJIT trunk[1].
If the latter commits are OK, they can be uses as an examples in
backporting procedure document[2] for the others to be adopted.

The first patch fixes the issue missed in #4862: CMake machinery fails
to locate system headers provided by SDK on MacOS platforms. The bug was
unnoticed for a while, since Tarantool testing environment doesn't use
Debug build with LUA_USE_ASSERT enabled on MacOS hosts. Anyway, even if
there was Debug build, there is a mess with the flags as a result of
#4862, so LuaJIT internal assertions would be enabled neither. The fix
contains of two parts:
1. Set the sysroot to SDK root directory if it is not set before.
2. Use ${CMAKE_C_FLAGS} in <LuaJITTestArch> auxiliary routine.

The second patch is a backport of 2e2fb8f[3]. When Apple released Macs
working on ARM64, the previous recipe in lj_arch.h for detecting Apple
platforms is not valid anymore. Fortunately, there is a system header
(i.e. TargetConditionals.h), provided by SDK with the proper defines to
be set. Since testing machinery assumes that LuaJIT is built with JIT
support being enabled unconditionally, a smoke test for it is also added
alongside with this patch.

The third patch is a backport of 521b367[4]. This patch fixes the issue
introduced by commit 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203 ('OSX/iOS:
Handle iOS simulator and ARM64 Macs.'). Within the mentioned commit
LJ_TARGET_IOS define is set via Apple system header to enable several
features (e.g. JIT and external unwinder) on ARM64 Macs, but its usage
was not adjusted source-wide. This is done for FFI machinery within this
commit. Since all LJ_TARGET_IOS usage is done with LJ_TARGET_ARM64
define being set, we can simply replace all entries with LJ_TARGET_OSX.

Branch: https://github.com/tarantool/luajit/tree/imun/gh-5983-fix-build-on-m1
Issues:
* https://github.com/tarantool/tarantool/issues/5983
* https://github.com/tarantool/tarantool/issues/5629
* https://github.com/tarantool/tarantool/issues/4862
CI looks to be OK[5] except the known problems with ASAN[6].

[1]: https://github.com/LuaJIT/LuaJIT
[2]: https://github.com/tarantool/tarantool/wiki/LuaJIT-backporting-guidelines
[3]: https://github.com/LuaJIT/LuaJIT/commit/2e2fb8f
[4]: https://github.com/LuaJIT/LuaJIT/commit/521b367
[5]: https://github.com/tarantool/tarantool/commit/fba91e1
[6]: https://github.com/tarantool/tarantool/issues/6031

Igor Munkin (1):
  build: pass sysroot to MacOS SDK

Mike Pall (2):
  OSX/iOS: Handle iOS simulator and ARM64 Macs.
  FFI/ARM64/OSX: Fix vararg call handling.

 CMakeLists.txt                                   |  9 +++++++++
 cmake/LuaJITUtils.cmake                          |  7 ++++++-
 src/lj_arch.h                                    |  8 +++++++-
 src/lj_ccall.c                                   |  8 ++++----
 src/lj_ccallback.c                               |  2 +-
 .../gh-5983-jit-library-smoke-tests.test.lua     | 11 +++++++++++
 .../lj-695-ffi-vararg-call.test.lua              | 16 ++++++++++++++++
 7 files changed, 54 insertions(+), 7 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
 create mode 100644 test/tarantool-tests/lj-695-ffi-vararg-call.test.lua

-- 
2.25.0

[Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches]

There were issues with configuring LuaJIT on Apple machines, since
<LuaJITTestArch> CMake auxiliary routine fails to locate system headers
(e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
platform detection fails and LuaJIT configuration ends with the fatal
error. This patch adds the necessary flags to help the routine to find
the required system headers.

Relates to tarantool/tarantool#5629
Needed for tarantool/tarantool#5983
Follows up tarantool/tarantool#4862

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

 CMakeLists.txt          | 9 +++++++++
 cmake/LuaJITUtils.cmake | 7 ++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5348e043..110a989f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -114,6 +114,15 @@ if(LUAJIT_ENABLE_WARNINGS)
   )
 endif()
 
+# Set sysroot settings on OSX to find SDK with the system headers.
+# XXX: Obviously, there is no need in this setup if everything is
+# already set via CMAKE_C_FLAGS or in parent project build system.
+if(CMAKE_OSX_SYSROOT AND CMAKE_C_SYSROOT_FLAG AND
+  NOT "${CMAKE_C_FLAGS}" MATCHES "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}"
+)
+  AppendFlags(CMAKE_C_FLAGS "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}")
+endif()
+
 # Auxiliary flags for main targets (libraries, binaries).
 AppendFlags(TARGET_C_FLAGS
   -D_FILE_OFFSET_BITS=64
diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
index d9f8b12a..8ff26a6a 100644
--- a/cmake/LuaJITUtils.cmake
+++ b/cmake/LuaJITUtils.cmake
@@ -1,8 +1,13 @@
 function(LuaJITTestArch outvar strflags)
+  # XXX: Compiler flags are also required in this routine. It can
+  # use e.g. external headers, which location is specified either
+  # implicitly (within CMake machinery) or explicitly (manually by
+  # configuration options).
+  set(TESTARCH_C_FLAGS "${CMAKE_C_FLAGS} ${strflags}")
   # XXX: <execute_process> simply splits the COMMAND argument by
   # spaces with no further parsing. At the same time GCC is bad in
   # argument handling, so let's help it a bit.
-  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${strflags})
+  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${TESTARCH_C_FLAGS})
   # TODO: It would be nice to drop a few words, why do we use this
   # approach instead of CMAKE_HOST_SYSTEM_PROCESSOR variable.
   execute_process(
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch!
LGTM, except ignorable questions below.

On 11.05.21, Igor Munkin wrote:
> There were issues with configuring LuaJIT on Apple machines, since
> <LuaJITTestArch> CMake auxiliary routine fails to locate system headers
> (e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
> platform detection fails and LuaJIT configuration ends with the fatal
> error. This patch adds the necessary flags to help the routine to find
> the required system headers.
> 
> Relates to tarantool/tarantool#5629
> Needed for tarantool/tarantool#5983
> Follows up tarantool/tarantool#4862
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
>  CMakeLists.txt          | 9 +++++++++
>  cmake/LuaJITUtils.cmake | 7 ++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 5348e043..110a989f 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -114,6 +114,15 @@ if(LUAJIT_ENABLE_WARNINGS)
>    )
>  endif()
>  
> +# Set sysroot settings on OSX to find SDK with the system headers.
> +# XXX: Obviously, there is no need in this setup if everything is
> +# already set via CMAKE_C_FLAGS or in parent project build system.

Typo: s/parent project/a parent project/

> +if(CMAKE_OSX_SYSROOT AND CMAKE_C_SYSROOT_FLAG AND

I found nothing about CMAKE_C_SYSROOT_FLAG by looking in CMake variable list

| $ cmake --help-variable-list | grep SYSROOT
| CMAKE_OSX_SYSROOT
| CMAKE_SYSROOT
| CMAKE_SYSROOT_COMPILE
| CMAKE_SYSROOT_LINK

or documentation[1][2]. Looks like it is an internal CMake variable and
it can be silently renamed in future versions. In my opinion, it is
better to use -isysroot directly here, or at last drop a comment about it.
Also, this variable is set by project() (and I suppose enable_language()
too) call, and this fact should be mentioned (to avoid using it
and LuaJITTestArch() before project() is called).

Also you may take a look to Mike's solution here[3].

Thoughts?

Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
Moreover, CMAKE_OSX_SYSROOT should "computed based on the
CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
And by itself, it means the minimum possible IOS version, where an
application may run [5]. May be we should fix this too with a separate
issue and patch (to protect users from building LuaJIT on ancient
devices)?

> +  NOT "${CMAKE_C_FLAGS}" MATCHES "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}"

Why must it match exactly the same default SYSROOT? Why user can't
define something custom if he wants?

Feel free to ignore.

> +)
> +  AppendFlags(CMAKE_C_FLAGS "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}")
> +endif()

Also, as far as this misbehaviour is occuring only for LuaJITTestArch()
macro we may avoid tweaking of global CMAKE_C_FLAGS, but just append it
to TESTARCH_C_FLAGS if it is necessary.

Feel free to ignore.

> +
>  # Auxiliary flags for main targets (libraries, binaries).
>  AppendFlags(TARGET_C_FLAGS
>    -D_FILE_OFFSET_BITS=64
> diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> index d9f8b12a..8ff26a6a 100644
> --- a/cmake/LuaJITUtils.cmake
> +++ b/cmake/LuaJITUtils.cmake
> @@ -1,8 +1,13 @@
>  function(LuaJITTestArch outvar strflags)
> +  # XXX: Compiler flags are also required in this routine. It can
> +  # use e.g. external headers, which location is specified either
> +  # implicitly (within CMake machinery) or explicitly (manually by
> +  # configuration options).
> +  set(TESTARCH_C_FLAGS "${CMAKE_C_FLAGS} ${strflags}")
>    # XXX: <execute_process> simply splits the COMMAND argument by
>    # spaces with no further parsing. At the same time GCC is bad in
>    # argument handling, so let's help it a bit.
> -  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${strflags})
> +  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${TESTARCH_C_FLAGS})
>    # TODO: It would be nice to drop a few words, why do we use this
>    # approach instead of CMAKE_HOST_SYSTEM_PROCESSOR variable.
>    execute_process(
> -- 
> 2.25.0
> 

[1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
[2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
[3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
[4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
[5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for such precise review! I've answered the questions below and
re-implemented the fix (the new patch is at the end), so hold your LGTM
for a while.

On 11.05.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> LGTM, except ignorable questions below.
> 
> On 11.05.21, Igor Munkin wrote:
> > There were issues with configuring LuaJIT on Apple machines, since
> > <LuaJITTestArch> CMake auxiliary routine fails to locate system headers
> > (e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
> > platform detection fails and LuaJIT configuration ends with the fatal
> > error. This patch adds the necessary flags to help the routine to find
> > the required system headers.
> > 
> > Relates to tarantool/tarantool#5629
> > Needed for tarantool/tarantool#5983
> > Follows up tarantool/tarantool#4862
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> >  CMakeLists.txt          | 9 +++++++++
> >  cmake/LuaJITUtils.cmake | 7 ++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 5348e043..110a989f 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -114,6 +114,15 @@ if(LUAJIT_ENABLE_WARNINGS)
> >    )
> >  endif()
> >  
> > +# Set sysroot settings on OSX to find SDK with the system headers.
> > +# XXX: Obviously, there is no need in this setup if everything is
> > +# already set via CMAKE_C_FLAGS or in parent project build system.
> 
> Typo: s/parent project/a parent project/

Dropped the comment in the new patch. Ignoring.

> 
> > +if(CMAKE_OSX_SYSROOT AND CMAKE_C_SYSROOT_FLAG AND
> 
> I found nothing about CMAKE_C_SYSROOT_FLAG by looking in CMake variable list
> 
> | $ cmake --help-variable-list | grep SYSROOT
> | CMAKE_OSX_SYSROOT
> | CMAKE_SYSROOT
> | CMAKE_SYSROOT_COMPILE
> | CMAKE_SYSROOT_LINK
> 
> or documentation[1][2]. Looks like it is an internal CMake variable and
> it can be silently renamed in future versions. In my opinion, it is
> better to use -isysroot directly here, or at last drop a comment about it.
> Also, this variable is set by project() (and I suppose enable_language()
> too) call, and this fact should be mentioned (to avoid using it
> and LuaJITTestArch() before project() is called).

Nice catch, thanks! I didn't look into the docs, just took this part
from Tarantool CMake. Changed to -isysroot.

> 
> Also you may take a look to Mike's solution here[3].

Well, we're using CMake to avoid this complexity. Anyway, you can run
CMake with --trace-expand to see that CMake uses xcrun underneath.

> 
> Thoughts?
> 
> Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
> Moreover, CMAKE_OSX_SYSROOT should "computed based on the
> CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
> And by itself, it means the minimum possible IOS version, where an
> application may run [5]. May be we should fix this too with a separate
> issue and patch (to protect users from building LuaJIT on ancient
> devices)?

Sorry, I don't get this one. What exactly is bothering you here?

> 
> > +  NOT "${CMAKE_C_FLAGS}" MATCHES "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}"
> 
> Why must it match exactly the same default SYSROOT? Why user can't
> define something custom if he wants?

CMake tries to locate MacOS SDK by itself. If user specify something
it's either the same or something additional. There is nothing critical
to add the one found by CMake, IMHO. Anyway, this nit is irrelevant,
considering the changes below.

> 
> Feel free to ignore.

Ignoring.

> 
> > +)
> > +  AppendFlags(CMAKE_C_FLAGS "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}")
> > +endif()
> 
> Also, as far as this misbehaviour is occuring only for LuaJITTestArch()
> macro we may avoid tweaking of global CMAKE_C_FLAGS, but just append it
> to TESTARCH_C_FLAGS if it is necessary.

Unfortunately, CMake is a crap. At first, these flags are required at
the configuration phase, but everything works fine at the build phase.
As a result of investigation, I've found that CMake emits[1] these flags
(both -arch and -isysroot) to a separate flags.make file located deep
into CMakeFiles/ directory. Moreover, they are not available via CMake
variables (used at configuration phase). They are just <fprintf>-ed to
the freaking file being included in build.make. Nice work, dudes! If
they wanted the most foolproof solution, they made it!

However, thanks to CMake authors for their tests: I've taken the recipe
there[2], so you can consider this as "Официальная позиция CMake".
Again, the changes are below.

> 
> Feel free to ignore.
> 
> > +
> >  # Auxiliary flags for main targets (libraries, binaries).
> >  AppendFlags(TARGET_C_FLAGS
> >    -D_FILE_OFFSET_BITS=64
> > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > index d9f8b12a..8ff26a6a 100644
> > --- a/cmake/LuaJITUtils.cmake
> > +++ b/cmake/LuaJITUtils.cmake
> > @@ -1,8 +1,13 @@
> >  function(LuaJITTestArch outvar strflags)
> > +  # XXX: Compiler flags are also required in this routine. It can
> > +  # use e.g. external headers, which location is specified either
> > +  # implicitly (within CMake machinery) or explicitly (manually by
> > +  # configuration options).
> > +  set(TESTARCH_C_FLAGS "${CMAKE_C_FLAGS} ${strflags}")
> >    # XXX: <execute_process> simply splits the COMMAND argument by
> >    # spaces with no further parsing. At the same time GCC is bad in
> >    # argument handling, so let's help it a bit.
> > -  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${strflags})
> > +  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${TESTARCH_C_FLAGS})
> >    # TODO: It would be nice to drop a few words, why do we use this
> >    # approach instead of CMAKE_HOST_SYSTEM_PROCESSOR variable.
> >    execute_process(
> > -- 
> > 2.25.0
> > 
> 
> [1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
> [2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
> [3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
> [4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
> [5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html
> 
> -- 
> Best regards,
> Sergey Kaplun

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

build: pass sysroot to MacOS SDK

There were issues with configuring LuaJIT on Apple machines, since
<LuaJITTestArch> CMake auxiliary routine fails to locate system headers
(e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
platform detection fails and LuaJIT configuration ends with the fatal
error. This patch adds the necessary flags to help the routine to find
the required system headers.

Needed for tarantool/tarantool#6065
Relates to tarantool/tarantool#5629
Follows up tarantool/tarantool#4862

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 cmake/LuaJITUtils.cmake | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
index d9f8b12a..3497edc4 100644
--- a/cmake/LuaJITUtils.cmake
+++ b/cmake/LuaJITUtils.cmake
@@ -1,4 +1,12 @@
 function(LuaJITTestArch outvar strflags)
+  # XXX: This routine uses external headers (e.g. system ones),
+  # which location is specified either implicitly (within CMake
+  # machinery) or explicitly (manually by configuration options).
+  # Need -isysroot flag on recentish MacOS after command line
+  # tools no longer provide headers in /usr/include.
+  if(CMAKE_OSX_SYSROOT)
+    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
+  endif()
   # XXX: <execute_process> simply splits the COMMAND argument by
   # spaces with no further parsing. At the same time GCC is bad in
   # argument handling, so let's help it a bit.

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

[1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
[2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28


-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the fixes!

Please fix the nit below. Otherwise, LGTM.

On 13.05.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for such precise review! I've answered the questions below and
> re-implemented the fix (the new patch is at the end), so hold your LGTM
> for a while.
> 
> On 11.05.21, Sergey Kaplun wrote:

<snipped>

> > Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
> > Moreover, CMAKE_OSX_SYSROOT should "computed based on the
> > CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
> > And by itself, it means the minimum possible IOS version, where an
> > application may run [5]. May be we should fix this too with a separate
> > issue and patch (to protect users from building LuaJIT on ancient
> > devices)?
> 
> Sorry, I don't get this one. What exactly is bothering you here?

Just friendly reminder. It's not related to the patch. Just a side
note. :) I remembered about it only because it is part of OSX twists.
There is no need to do it in this patch, but maybe later?

> 

<snipped>

> > > +)
> > > +  AppendFlags(CMAKE_C_FLAGS "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}")
> > > +endif()
> > 
> > Also, as far as this misbehaviour is occuring only for LuaJITTestArch()
> > macro we may avoid tweaking of global CMAKE_C_FLAGS, but just append it
> > to TESTARCH_C_FLAGS if it is necessary.
> 
> Unfortunately, CMake is a crap. At first, these flags are required at
> the configuration phase, but everything works fine at the build phase.
> As a result of investigation, I've found that CMake emits[1] these flags
> (both -arch and -isysroot) to a separate flags.make file located deep
> into CMakeFiles/ directory. Moreover, they are not available via CMake
> variables (used at configuration phase). They are just <fprintf>-ed to
> the freaking file being included in build.make. Nice work, dudes! If
> they wanted the most foolproof solution, they made it!
> 
> However, thanks to CMake authors for their tests: I've taken the recipe
> there[2], so you can consider this as "Официальная позиция CMake".
> Again, the changes are below.

Thanks for the investigation and clarification!

> 

<snipped>

> > 
> > [1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
> > [2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
> > [3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
> > [4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
> > [5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> ================================================================================
> 
> build: pass sysroot to MacOS SDK
> 
> There were issues with configuring LuaJIT on Apple machines, since
> <LuaJITTestArch> CMake auxiliary routine fails to locate system headers
> (e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
> platform detection fails and LuaJIT configuration ends with the fatal
> error. This patch adds the necessary flags to help the routine to find
> the required system headers.
> 
> Needed for tarantool/tarantool#6065
> Relates to tarantool/tarantool#5629
> Follows up tarantool/tarantool#4862

What's about 5983? Shouldn't it be mentioned too?

> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  cmake/LuaJITUtils.cmake | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> index d9f8b12a..3497edc4 100644
> --- a/cmake/LuaJITUtils.cmake
> +++ b/cmake/LuaJITUtils.cmake
> @@ -1,4 +1,12 @@
>  function(LuaJITTestArch outvar strflags)
> +  # XXX: This routine uses external headers (e.g. system ones),
> +  # which location is specified either implicitly (within CMake
> +  # machinery) or explicitly (manually by configuration options).
> +  # Need -isysroot flag on recentish MacOS after command line
> +  # tools no longer provide headers in /usr/include.
> +  if(CMAKE_OSX_SYSROOT)

Nit: This variable should be taken into account only for OS X
platforms [1]. I suppose not only by CMake, but by our build system too.

I've got the error on Linux if I try to build LuaJIT like the following
(by mistake of course):

| $ uname -s
| Linux
| $ cmake . -DCMAKE_OSX_SYSROOT="/tmp/blablabalblablbalalbla" -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug
| In file included from lua.h:14,
|                  from lj_arch.h:9:
| /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9:16: fatal error: stdint.h: No such file or directory
|     9 | # include_next <stdint.h>
|       |                ^~~~~~~~~~
| compilation terminated.
| CMake Error at cmake/LuaJITUtils.cmake:48 (message):
|   [LuaJITArch] Unsupported target architecture
| Call Stack (most recent call first):
|   cmake/SetTargetFlags.cmake:16 (LuaJITArch)
|   src/CMakeLists.txt:17 (include)

> +    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
> +  endif()
>    # XXX: <execute_process> simply splits the COMMAND argument by
>    # spaces with no further parsing. At the same time GCC is bad in
>    # argument handling, so let's help it a bit.
> 
> ================================================================================
> 
> [1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
> [2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28
> 
> 
> -- 
> Best regards,
> IM

[1]: https://cmake.org/cmake/help/v3.0/variable/CMAKE_OSX_SYSROOT.html

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches]

Sergey,

On 14.05.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the fixes!
> 
> Please fix the nit below. Otherwise, LGTM.

Thanks for your review. Unfortunately, this is not a nit (at least I
have no idea how to handle it). Consider the answers below.

> 
> On 13.05.21, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for such precise review! I've answered the questions below and
> > re-implemented the fix (the new patch is at the end), so hold your LGTM
> > for a while.
> > 
> > On 11.05.21, Sergey Kaplun wrote:
> 
> <snipped>
> 
> > > Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
> > > Moreover, CMAKE_OSX_SYSROOT should "computed based on the
> > > CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
> > > And by itself, it means the minimum possible IOS version, where an
> > > application may run [5]. May be we should fix this too with a separate
> > > issue and patch (to protect users from building LuaJIT on ancient
> > > devices)?
> > 
> > Sorry, I don't get this one. What exactly is bothering you here?
> 
> Just friendly reminder. It's not related to the patch. Just a side
> note. :) I remembered about it only because it is part of OSX twists.
> There is no need to do it in this patch, but maybe later?

But I still don't get what we need to do...

> 

<snipped>

> 
> > > 
> > > [1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
> > > [2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
> > > [3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
> > > [4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
> > > [5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html
> > > 
> > > -- 
> > > Best regards,
> > > Sergey Kaplun
> > 
> > ================================================================================
> > 
> > build: pass sysroot to MacOS SDK
> > 
> > There were issues with configuring LuaJIT on Apple machines, since
> > <LuaJITTestArch> CMake auxiliary routine fails to locate system headers
> > (e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
> > platform detection fails and LuaJIT configuration ends with the fatal
> > error. This patch adds the necessary flags to help the routine to find
> > the required system headers.
> > 
> > Needed for tarantool/tarantool#6065
> > Relates to tarantool/tarantool#5629
> > Follows up tarantool/tarantool#4862
> 
> What's about 5983? Shouldn't it be mentioned too?

No, since there is a separate issue this patch is needed for (#6065).

> 
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  cmake/LuaJITUtils.cmake | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > index d9f8b12a..3497edc4 100644
> > --- a/cmake/LuaJITUtils.cmake
> > +++ b/cmake/LuaJITUtils.cmake
> > @@ -1,4 +1,12 @@
> >  function(LuaJITTestArch outvar strflags)
> > +  # XXX: This routine uses external headers (e.g. system ones),
> > +  # which location is specified either implicitly (within CMake
> > +  # machinery) or explicitly (manually by configuration options).
> > +  # Need -isysroot flag on recentish MacOS after command line
> > +  # tools no longer provide headers in /usr/include.
> > +  if(CMAKE_OSX_SYSROOT)
> 
> Nit: This variable should be taken into account only for OS X
> platforms [1]. I suppose not only by CMake, but by our build system too.
> 
> I've got the error on Linux if I try to build LuaJIT like the following
> (by mistake of course):
> 
> | $ uname -s
> | Linux
> | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp/blablabalblablbalalbla" -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug

Sigh. After all, I've tried for three patches, seems like ninety...

BTW, I see failures even on MacOS:
| $ uname -msr
| Darwin 20.3.0 arm64
| $ cmake . -DCMAKE_OSX_SYSROOT="/tmp"
| -- The C compiler identification is AppleClang 12.0.0.12000032
| -- Detecting C compiler ABI info
| -- Detecting C compiler ABI info - failed
| -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc
| -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - broken
| CMake Error at /opt/homebrew/Cellar/cmake/3.20.1/share/cmake/Modules/CMakeTestCCompiler.cmake:66 (message):
|   The C compiler
|
|     "/Library/Developer/CommandLineTools/usr/bin/cc"
|
|   is not able to compile a simple test program.
|
|   It fails with the following output:
|
|     Change Dir: /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp
|
|     Run Build Command(s):/usr/bin/make -f Makefile cmTC_da00c/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_da00c.dir/build.make CMakeFiles/cmTC_da00c.dir/build
|     Building C object CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o
|     /Library/Developer/CommandLineTools/usr/bin/cc   -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -MD -MT CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -MF CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -c /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp/testCCompiler.c
|     Linking C executable cmTC_da00c
|     /opt/homebrew/Cellar/cmake/3.20.1/bin/cmake -E cmake_link_script CMakeFiles/cmTC_da00c.dir/link.txt --verbose=1
|     /Library/Developer/CommandLineTools/usr/bin/cc  -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -o cmTC_da00c
|     ld: library not found for -lSystem
|     clang: error: linker command failed with exit code 1 (use -v to see invocation)
|     make[1]: *** [cmTC_da00c] Error 1
|     make: *** [cmTC_da00c/fast] Error 2
|
|
|
|
|
|   CMake will not be able to correctly generate this project.
| Call Stack (most recent call first):
|   CMakeLists.txt:12 (project)
|
|
| -- Configuring incomplete, errors occurred!
| See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeOutput.log".
| See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeError.log".

I have no idea how to fix this, so I propose to classify manual
definition of CMAKE_OSX_SYSROOT as PEBKAC and move on. Thoughts?

> | In file included from lua.h:14,
> |                  from lj_arch.h:9:
> | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9:16: fatal error: stdint.h: No such file or directory
> |     9 | # include_next <stdint.h>
> |       |                ^~~~~~~~~~
> | compilation terminated.
> | CMake Error at cmake/LuaJITUtils.cmake:48 (message):
> |   [LuaJITArch] Unsupported target architecture
> | Call Stack (most recent call first):
> |   cmake/SetTargetFlags.cmake:16 (LuaJITArch)
> |   src/CMakeLists.txt:17 (include)
> 
> > +    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
> > +  endif()
> >    # XXX: <execute_process> simply splits the COMMAND argument by
> >    # spaces with no further parsing. At the same time GCC is bad in
> >    # argument handling, so let's help it a bit.
> > 
> > ================================================================================
> > 
> > [1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
> > [2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28
> > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://cmake.org/cmake/help/v3.0/variable/CMAKE_OSX_SYSROOT.html
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Sergey Kaplun via Tarantool-patches]

Igor,

On 17.05.21, Igor Munkin wrote:
> Sergey,
> 
> On 14.05.21, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the fixes!
> > 
> > Please fix the nit below. Otherwise, LGTM.
> 
> Thanks for your review. Unfortunately, this is not a nit (at least I
> have no idea how to handle it). Consider the answers below.
> 
> > 
> > On 13.05.21, Igor Munkin wrote:
> > > Sergey,
> > > 
> > > Thanks for such precise review! I've answered the questions below and
> > > re-implemented the fix (the new patch is at the end), so hold your LGTM
> > > for a while.
> > > 
> > > On 11.05.21, Sergey Kaplun wrote:
> > 
> > <snipped>
> > 
> > > > Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
> > > > Moreover, CMAKE_OSX_SYSROOT should "computed based on the
> > > > CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
> > > > And by itself, it means the minimum possible IOS version, where an
> > > > application may run [5]. May be we should fix this too with a separate
> > > > issue and patch (to protect users from building LuaJIT on ancient
> > > > devices)?
> > > 
> > > Sorry, I don't get this one. What exactly is bothering you here?
> > 
> > Just friendly reminder. It's not related to the patch. Just a side
> > note. :) I remembered about it only because it is part of OSX twists.
> > There is no need to do it in this patch, but maybe later?
> 
> But I still don't get what we need to do...

I suppose it will be nice to add CMAKE_OSX_DEPLOYMENT_TARGET. Like Mike
does in Makefile: MACOSX_DEPLOYMENT_TARGET is set to 10.4. In the
separate patch, of course.

> 
> > 
> 
> <snipped>
> 
> > 
> > > > 
> > > > [1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
> > > > [2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
> > > > [3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
> > > > [4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
> > > > [5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html
> > > > 
> > > > -- 
> > > > Best regards,
> > > > Sergey Kaplun
> > > 
> > > ================================================================================
> > > 
> > > build: pass sysroot to MacOS SDK
> > > 
> > > There were issues with configuring LuaJIT on Apple machines, since
> > > <LuaJITTestArch> CMake auxiliary routine fails to locate system headers
> > > (e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
> > > platform detection fails and LuaJIT configuration ends with the fatal
> > > error. This patch adds the necessary flags to help the routine to find
> > > the required system headers.
> > > 
> > > Needed for tarantool/tarantool#6065
> > > Relates to tarantool/tarantool#5629
> > > Follows up tarantool/tarantool#4862
> > 
> > What's about 5983? Shouldn't it be mentioned too?
> 
> No, since there is a separate issue this patch is needed for (#6065).
> 
> > 
> > > 
> > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > ---
> > >  cmake/LuaJITUtils.cmake | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > > index d9f8b12a..3497edc4 100644
> > > --- a/cmake/LuaJITUtils.cmake
> > > +++ b/cmake/LuaJITUtils.cmake
> > > @@ -1,4 +1,12 @@
> > >  function(LuaJITTestArch outvar strflags)
> > > +  # XXX: This routine uses external headers (e.g. system ones),
> > > +  # which location is specified either implicitly (within CMake
> > > +  # machinery) or explicitly (manually by configuration options).
> > > +  # Need -isysroot flag on recentish MacOS after command line
> > > +  # tools no longer provide headers in /usr/include.
> > > +  if(CMAKE_OSX_SYSROOT)
> > 
> > Nit: This variable should be taken into account only for OS X
> > platforms [1]. I suppose not only by CMake, but by our build system too.
> > 
> > I've got the error on Linux if I try to build LuaJIT like the following
> > (by mistake of course):
> > 
> > | $ uname -s
> > | Linux
> > | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp/blablabalblablbalalbla" -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug
> 
> Sigh. After all, I've tried for three patches, seems like ninety...
> 
> BTW, I see failures even on MacOS:
> | $ uname -msr
> | Darwin 20.3.0 arm64
> | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp"
> | -- The C compiler identification is AppleClang 12.0.0.12000032
> | -- Detecting C compiler ABI info
> | -- Detecting C compiler ABI info - failed
> | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc
> | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - broken
> | CMake Error at /opt/homebrew/Cellar/cmake/3.20.1/share/cmake/Modules/CMakeTestCCompiler.cmake:66 (message):
> |   The C compiler
> |
> |     "/Library/Developer/CommandLineTools/usr/bin/cc"
> |
> |   is not able to compile a simple test program.
> |
> |   It fails with the following output:
> |
> |     Change Dir: /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp
> |
> |     Run Build Command(s):/usr/bin/make -f Makefile cmTC_da00c/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_da00c.dir/build.make CMakeFiles/cmTC_da00c.dir/build
> |     Building C object CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o
> |     /Library/Developer/CommandLineTools/usr/bin/cc   -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -MD -MT CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -MF CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -c /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp/testCCompiler.c
> |     Linking C executable cmTC_da00c
> |     /opt/homebrew/Cellar/cmake/3.20.1/bin/cmake -E cmake_link_script CMakeFiles/cmTC_da00c.dir/link.txt --verbose=1
> |     /Library/Developer/CommandLineTools/usr/bin/cc  -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -o cmTC_da00c
> |     ld: library not found for -lSystem
> |     clang: error: linker command failed with exit code 1 (use -v to see invocation)
> |     make[1]: *** [cmTC_da00c] Error 1
> |     make: *** [cmTC_da00c/fast] Error 2
> |
> |
> |
> |
> |
> |   CMake will not be able to correctly generate this project.
> | Call Stack (most recent call first):
> |   CMakeLists.txt:12 (project)
> |
> |
> | -- Configuring incomplete, errors occurred!
> | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeOutput.log".
> | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeError.log".
> 
> I have no idea how to fix this, so I propose to classify manual
> definition of CMAKE_OSX_SYSROOT as PEBKAC and move on. Thoughts?

Why we can't just change the check to the following?

| if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)

At least it fixes the case I mentioned above, doesn't it? And we
definitely can't protect users from wrong sysroot paths on OS X systems
like in your example. This case looks like "Сам себе злобный Буратино".

> 
> > | In file included from lua.h:14,
> > |                  from lj_arch.h:9:
> > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9:16: fatal error: stdint.h: No such file or directory
> > |     9 | # include_next <stdint.h>
> > |       |                ^~~~~~~~~~
> > | compilation terminated.
> > | CMake Error at cmake/LuaJITUtils.cmake:48 (message):
> > |   [LuaJITArch] Unsupported target architecture
> > | Call Stack (most recent call first):
> > |   cmake/SetTargetFlags.cmake:16 (LuaJITArch)
> > |   src/CMakeLists.txt:17 (include)
> > 
> > > +    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
> > > +  endif()
> > >    # XXX: <execute_process> simply splits the COMMAND argument by
> > >    # spaces with no further parsing. At the same time GCC is bad in
> > >    # argument handling, so let's help it a bit.
> > > 
> > > ================================================================================
> > > 
> > > [1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
> > > [2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28
> > > 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > [1]: https://cmake.org/cmake/help/v3.0/variable/CMAKE_OSX_SYSROOT.html
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches]

Sergey,

On 18.05.21, Sergey Kaplun wrote:
> Igor,
> 
> On 17.05.21, Igor Munkin wrote:

<snipped>

> > > > > Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
> > > > > Moreover, CMAKE_OSX_SYSROOT should "computed based on the
> > > > > CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
> > > > > And by itself, it means the minimum possible IOS version, where an
> > > > > application may run [5]. May be we should fix this too with a separate
> > > > > issue and patch (to protect users from building LuaJIT on ancient
> > > > > devices)?
> > > > 
> > > > Sorry, I don't get this one. What exactly is bothering you here?
> > > 
> > > Just friendly reminder. It's not related to the patch. Just a side
> > > note. :) I remembered about it only because it is part of OSX twists.
> > > There is no need to do it in this patch, but maybe later?
> > 
> > But I still don't get what we need to do...
> 
> I suppose it will be nice to add CMAKE_OSX_DEPLOYMENT_TARGET. Like Mike
> does in Makefile: MACOSX_DEPLOYMENT_TARGET is set to 10.4. In the
> separate patch, of course.

So what problem does this solve? BTW, this is dropped in the new version
of LuaJIT Makefile[1].

> 

<snipped>

> > > > > 
> > > > > [1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
> > > > > [2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
> > > > > [3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
> > > > > [4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
> > > > > [5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html
> > > > > 

<snipped>

> > > > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > > > index d9f8b12a..3497edc4 100644
> > > > --- a/cmake/LuaJITUtils.cmake
> > > > +++ b/cmake/LuaJITUtils.cmake
> > > > @@ -1,4 +1,12 @@
> > > >  function(LuaJITTestArch outvar strflags)
> > > > +  # XXX: This routine uses external headers (e.g. system ones),
> > > > +  # which location is specified either implicitly (within CMake
> > > > +  # machinery) or explicitly (manually by configuration options).
> > > > +  # Need -isysroot flag on recentish MacOS after command line
> > > > +  # tools no longer provide headers in /usr/include.
> > > > +  if(CMAKE_OSX_SYSROOT)
> > > 
> > > Nit: This variable should be taken into account only for OS X
> > > platforms [1]. I suppose not only by CMake, but by our build system too.
> > > 
> > > I've got the error on Linux if I try to build LuaJIT like the following
> > > (by mistake of course):
> > > 
> > > | $ uname -s
> > > | Linux
> > > | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp/blablabalblablbalalbla" -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug
> > 
> > Sigh. After all, I've tried for three patches, seems like ninety...
> > 
> > BTW, I see failures even on MacOS:
> > | $ uname -msr
> > | Darwin 20.3.0 arm64
> > | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp"
> > | -- The C compiler identification is AppleClang 12.0.0.12000032
> > | -- Detecting C compiler ABI info
> > | -- Detecting C compiler ABI info - failed
> > | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc
> > | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - broken
> > | CMake Error at /opt/homebrew/Cellar/cmake/3.20.1/share/cmake/Modules/CMakeTestCCompiler.cmake:66 (message):
> > |   The C compiler
> > |
> > |     "/Library/Developer/CommandLineTools/usr/bin/cc"
> > |
> > |   is not able to compile a simple test program.
> > |
> > |   It fails with the following output:
> > |
> > |     Change Dir: /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp
> > |
> > |     Run Build Command(s):/usr/bin/make -f Makefile cmTC_da00c/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_da00c.dir/build.make CMakeFiles/cmTC_da00c.dir/build
> > |     Building C object CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o
> > |     /Library/Developer/CommandLineTools/usr/bin/cc   -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -MD -MT CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -MF CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -c /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp/testCCompiler.c
> > |     Linking C executable cmTC_da00c
> > |     /opt/homebrew/Cellar/cmake/3.20.1/bin/cmake -E cmake_link_script CMakeFiles/cmTC_da00c.dir/link.txt --verbose=1
> > |     /Library/Developer/CommandLineTools/usr/bin/cc  -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -o cmTC_da00c
> > |     ld: library not found for -lSystem
> > |     clang: error: linker command failed with exit code 1 (use -v to see invocation)
> > |     make[1]: *** [cmTC_da00c] Error 1
> > |     make: *** [cmTC_da00c/fast] Error 2
> > |
> > |
> > |
> > |
> > |
> > |   CMake will not be able to correctly generate this project.
> > | Call Stack (most recent call first):
> > |   CMakeLists.txt:12 (project)
> > |
> > |
> > | -- Configuring incomplete, errors occurred!
> > | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeOutput.log".
> > | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeError.log".
> > 
> > I have no idea how to fix this, so I propose to classify manual
> > definition of CMAKE_OSX_SYSROOT as PEBKAC and move on. Thoughts?
> 
> Why we can't just change the check to the following?

For what? The problem still exists on MacOS. Furthermore, the problem
with the same nature (from my point of view).

> 
> | if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)
> 
> At least it fixes the case I mentioned above, doesn't it? And we
> definitely can't protect users from wrong sysroot paths on OS X systems
> like in your example. This case looks like "Сам себе злобный Буратино".

Yes, it fixes. But it does not on MacOS. Hence, why do we need to handle
this only on Linux? Moreover, who will set CMAKE_OSX_SYSROOT (not for
the testing reason) on Linux? There is even *OSX* in the variable name.
I am more concerned with the fact, one can break the build with this on
MacOS. Unfortunately, it seems in this case we can do nothing with it.

Sorry, but I see no arguments for solving this particular case.

Ignoring.

> 
> > 
> > > | In file included from lua.h:14,
> > > |                  from lj_arch.h:9:
> > > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9:16: fatal error: stdint.h: No such file or directory
> > > |     9 | # include_next <stdint.h>
> > > |       |                ^~~~~~~~~~
> > > | compilation terminated.
> > > | CMake Error at cmake/LuaJITUtils.cmake:48 (message):
> > > |   [LuaJITArch] Unsupported target architecture
> > > | Call Stack (most recent call first):
> > > |   cmake/SetTargetFlags.cmake:16 (LuaJITArch)
> > > |   src/CMakeLists.txt:17 (include)
> > > 
> > > > +    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
> > > > +  endif()
> > > >    # XXX: <execute_process> simply splits the COMMAND argument by
> > > >    # spaces with no further parsing. At the same time GCC is bad in
> > > >    # argument handling, so let's help it a bit.
> > > > 
> > > > ================================================================================
> > > > 
> > > > [1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
> > > > [2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28
> > > > 
> > > > 
> > > > -- 
> > > > Best regards,
> > > > IM
> > > 
> > > [1]: https://cmake.org/cmake/help/v3.0/variable/CMAKE_OSX_SYSROOT.html
> > > 
> > > -- 
> > > Best regards,
> > > Sergey Kaplun
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://github.com/LuaJIT/LuaJIT/commit/8961a92

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches]

As a result of offline discussion with Sergey, I've finally understood
the docs and fixed the issue. Diff is below:

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

diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
index 3497edc4..29c425de 100644
--- a/cmake/LuaJITUtils.cmake
+++ b/cmake/LuaJITUtils.cmake
@@ -4,7 +4,12 @@ function(LuaJITTestArch outvar strflags)
   # machinery) or explicitly (manually by configuration options).
   # Need -isysroot flag on recentish MacOS after command line
   # tools no longer provide headers in /usr/include.
-  if(CMAKE_OSX_SYSROOT)
+  # XXX: According to CMake documentation[1], CMAKE_OSX_SYSROOT
+  # variable *should* be ignored on the plaforms other than MacOS.
+  # It is ignored by CMake, but since this routine is extension
+  # it also should follow this policy.
+  # [1]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
+  if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)
     set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
   endif()
   # XXX: <execute_process> simply splits the COMMAND argument by

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

On 18.05.21, Igor Munkin wrote:
> Sergey,
> 

<snipped>

> 
> > > > > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > > > > index d9f8b12a..3497edc4 100644
> > > > > --- a/cmake/LuaJITUtils.cmake
> > > > > +++ b/cmake/LuaJITUtils.cmake
> > > > > @@ -1,4 +1,12 @@
> > > > >  function(LuaJITTestArch outvar strflags)
> > > > > +  # XXX: This routine uses external headers (e.g. system ones),
> > > > > +  # which location is specified either implicitly (within CMake
> > > > > +  # machinery) or explicitly (manually by configuration options).
> > > > > +  # Need -isysroot flag on recentish MacOS after command line
> > > > > +  # tools no longer provide headers in /usr/include.
> > > > > +  if(CMAKE_OSX_SYSROOT)
> > > > 
> > > > Nit: This variable should be taken into account only for OS X
> > > > platforms [1]. I suppose not only by CMake, but by our build system too.
> > > > 
> > > > I've got the error on Linux if I try to build LuaJIT like the following
> > > > (by mistake of course):
> > > > 
> > > > | $ uname -s
> > > > | Linux
> > > > | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp/blablabalblablbalalbla" -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug
> > > 
> > > Sigh. After all, I've tried for three patches, seems like ninety...
> > > 
> > > BTW, I see failures even on MacOS:
> > > | $ uname -msr
> > > | Darwin 20.3.0 arm64
> > > | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp"
> > > | -- The C compiler identification is AppleClang 12.0.0.12000032
> > > | -- Detecting C compiler ABI info
> > > | -- Detecting C compiler ABI info - failed
> > > | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc
> > > | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - broken
> > > | CMake Error at /opt/homebrew/Cellar/cmake/3.20.1/share/cmake/Modules/CMakeTestCCompiler.cmake:66 (message):
> > > |   The C compiler
> > > |
> > > |     "/Library/Developer/CommandLineTools/usr/bin/cc"
> > > |
> > > |   is not able to compile a simple test program.
> > > |
> > > |   It fails with the following output:
> > > |
> > > |     Change Dir: /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp
> > > |
> > > |     Run Build Command(s):/usr/bin/make -f Makefile cmTC_da00c/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_da00c.dir/build.make CMakeFiles/cmTC_da00c.dir/build
> > > |     Building C object CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o
> > > |     /Library/Developer/CommandLineTools/usr/bin/cc   -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -MD -MT CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -MF CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -c /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp/testCCompiler.c
> > > |     Linking C executable cmTC_da00c
> > > |     /opt/homebrew/Cellar/cmake/3.20.1/bin/cmake -E cmake_link_script CMakeFiles/cmTC_da00c.dir/link.txt --verbose=1
> > > |     /Library/Developer/CommandLineTools/usr/bin/cc  -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -o cmTC_da00c
> > > |     ld: library not found for -lSystem
> > > |     clang: error: linker command failed with exit code 1 (use -v to see invocation)
> > > |     make[1]: *** [cmTC_da00c] Error 1
> > > |     make: *** [cmTC_da00c/fast] Error 2
> > > |
> > > |
> > > |
> > > |
> > > |
> > > |   CMake will not be able to correctly generate this project.
> > > | Call Stack (most recent call first):
> > > |   CMakeLists.txt:12 (project)
> > > |
> > > |
> > > | -- Configuring incomplete, errors occurred!
> > > | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeOutput.log".
> > > | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeError.log".
> > > 
> > > I have no idea how to fix this, so I propose to classify manual
> > > definition of CMAKE_OSX_SYSROOT as PEBKAC and move on. Thoughts?
> > 
> > Why we can't just change the check to the following?
> 
> For what? The problem still exists on MacOS. Furthermore, the problem
> with the same nature (from my point of view).
> 
> > 
> > | if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)
> > 
> > At least it fixes the case I mentioned above, doesn't it? And we
> > definitely can't protect users from wrong sysroot paths on OS X systems
> > like in your example. This case looks like "Сам себе злобный Буратино".
> 
> Yes, it fixes. But it does not on MacOS. Hence, why do we need to handle
> this only on Linux? Moreover, who will set CMAKE_OSX_SYSROOT (not for
> the testing reason) on Linux? There is even *OSX* in the variable name.
> I am more concerned with the fact, one can break the build with this on
> MacOS. Unfortunately, it seems in this case we can do nothing with it.
> 
> Sorry, but I see no arguments for solving this particular case.
> 
> Ignoring.
> 
> > 
> > > 
> > > > | In file included from lua.h:14,
> > > > |                  from lj_arch.h:9:
> > > > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9:16: fatal error: stdint.h: No such file or directory
> > > > |     9 | # include_next <stdint.h>
> > > > |       |                ^~~~~~~~~~
> > > > | compilation terminated.
> > > > | CMake Error at cmake/LuaJITUtils.cmake:48 (message):
> > > > |   [LuaJITArch] Unsupported target architecture
> > > > | Call Stack (most recent call first):
> > > > |   cmake/SetTargetFlags.cmake:16 (LuaJITArch)
> > > > |   src/CMakeLists.txt:17 (include)
> > > > 
> > > > > +    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
> > > > > +  endif()
> > > > >    # XXX: <execute_process> simply splits the COMMAND argument by
> > > > >    # spaces with no further parsing. At the same time GCC is bad in
> > > > >    # argument handling, so let's help it a bit.
> > > > > 
> > > > > ================================================================================
> > > > > 
> > > > > [1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
> > > > > [2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28
> > > > > 
> > > > > 
> > > > > -- 
> > > > > Best regards,
> > > > > IM
> > > > 
> > > > [1]: https://cmake.org/cmake/help/v3.0/variable/CMAKE_OSX_SYSROOT.html
> > > > 
> > > > -- 
> > > > Best regards,
> > > > Sergey Kaplun
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> [1]: https://github.com/LuaJIT/LuaJIT/commit/8961a92
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 8588 bytes --]

Hi! 

I believe we move from patch to two files originally
+++ b/CMakeLists.txt
+++ b/cmake/LuaJITUtils.cmake

to just one, in 432cdf62303b0d609525acc84b01b92ae468d327
+++ b/cmake/LuaJITUtils.cmake

I expect to see a ‘v2’ in such a case - for the forthcoming patches.

Anyways, if I read the correct patch above from the imun/gh-5983-fix-build-on-m1
branch then it’s LGTM with just a typo fix.

Sergos


> On 19 May 2021, at 14:38, Igor Munkin <imun@tarantool.org> wrote:
> 
> As a result of offline discussion with Sergey, I've finally understood
> the docs and fixed the issue. Diff is below:
> 
> ================================================================================
> 
> diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> index 3497edc4..29c425de 100644
> --- a/cmake/LuaJITUtils.cmake
> +++ b/cmake/LuaJITUtils.cmake
> @@ -4,7 +4,12 @@ function(LuaJITTestArch outvar strflags)
>   # machinery) or explicitly (manually by configuration options).
>   # Need -isysroot flag on recentish MacOS after command line
>   # tools no longer provide headers in /usr/include.
> -  if(CMAKE_OSX_SYSROOT)
> +  # XXX: According to CMake documentation[1], CMAKE_OSX_SYSROOT
> +  # variable *should* be ignored on the plaforms other than MacOS.
					  platforms
> +  # It is ignored by CMake, but since this routine is extension
> +  # it also should follow this policy.
> +  # [1]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html <https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html>
> +  if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)
>     set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
>   endif()
>   # XXX: <execute_process> simply splits the COMMAND argument by
> 
> ================================================================================
> 
> On 18.05.21, Igor Munkin wrote:
>> Sergey,
>> 
> 
> <snipped>
> 
>> 
>>>>>> diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
>>>>>> index d9f8b12a..3497edc4 100644
>>>>>> --- a/cmake/LuaJITUtils.cmake
>>>>>> +++ b/cmake/LuaJITUtils.cmake
>>>>>> @@ -1,4 +1,12 @@
>>>>>> function(LuaJITTestArch outvar strflags)
>>>>>> +  # XXX: This routine uses external headers (e.g. system ones),
>>>>>> +  # which location is specified either implicitly (within CMake
>>>>>> +  # machinery) or explicitly (manually by configuration options).
>>>>>> +  # Need -isysroot flag on recentish MacOS after command line
>>>>>> +  # tools no longer provide headers in /usr/include.
>>>>>> +  if(CMAKE_OSX_SYSROOT)
>>>>> 
>>>>> Nit: This variable should be taken into account only for OS X
>>>>> platforms [1]. I suppose not only by CMake, but by our build system too.
>>>>> 
>>>>> I've got the error on Linux if I try to build LuaJIT like the following
>>>>> (by mistake of course):
>>>>> 
>>>>> | $ uname -s
>>>>> | Linux
>>>>> | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp/blablabalblablbalalbla" -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug
>>>> 
>>>> Sigh. After all, I've tried for three patches, seems like ninety...
>>>> 
>>>> BTW, I see failures even on MacOS:
>>>> | $ uname -msr
>>>> | Darwin 20.3.0 arm64
>>>> | $ cmake . -DCMAKE_OSX_SYSROOT="/tmp"
>>>> | -- The C compiler identification is AppleClang 12.0.0.12000032
>>>> | -- Detecting C compiler ABI info
>>>> | -- Detecting C compiler ABI info - failed
>>>> | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc
>>>> | -- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - broken
>>>> | CMake Error at /opt/homebrew/Cellar/cmake/3.20.1/share/cmake/Modules/CMakeTestCCompiler.cmake:66 (message):
>>>> |   The C compiler
>>>> |
>>>> |     "/Library/Developer/CommandLineTools/usr/bin/cc"
>>>> |
>>>> |   is not able to compile a simple test program.
>>>> |
>>>> |   It fails with the following output:
>>>> |
>>>> |     Change Dir: /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp
>>>> |
>>>> |     Run Build Command(s):/usr/bin/make -f Makefile cmTC_da00c/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_da00c.dir/build.make CMakeFiles/cmTC_da00c.dir/build
>>>> |     Building C object CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o
>>>> |     /Library/Developer/CommandLineTools/usr/bin/cc   -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -MD -MT CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -MF CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -c /Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeTmp/testCCompiler.c
>>>> |     Linking C executable cmTC_da00c
>>>> |     /opt/homebrew/Cellar/cmake/3.20.1/bin/cmake -E cmake_link_script CMakeFiles/cmTC_da00c.dir/link.txt --verbose=1
>>>> |     /Library/Developer/CommandLineTools/usr/bin/cc  -arch arm64 -isysroot /tmp -mmacosx-version-min=11.1 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_da00c.dir/testCCompiler.c.o -o cmTC_da00c
>>>> |     ld: library not found for -lSystem
>>>> |     clang: error: linker command failed with exit code 1 (use -v to see invocation)
>>>> |     make[1]: *** [cmTC_da00c] Error 1
>>>> |     make: *** [cmTC_da00c/fast] Error 2
>>>> |
>>>> |
>>>> |
>>>> |
>>>> |
>>>> |   CMake will not be able to correctly generate this project.
>>>> | Call Stack (most recent call first):
>>>> |   CMakeLists.txt:12 (project)
>>>> |
>>>> |
>>>> | -- Configuring incomplete, errors occurred!
>>>> | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeOutput.log".
>>>> | See also "/Users/tntmac07.tarantool.i/imun/tarantool-luajit/CMakeFiles/CMakeError.log".
>>>> 
>>>> I have no idea how to fix this, so I propose to classify manual
>>>> definition of CMAKE_OSX_SYSROOT as PEBKAC and move on. Thoughts?
>>> 
>>> Why we can't just change the check to the following?
>> 
>> For what? The problem still exists on MacOS. Furthermore, the problem
>> with the same nature (from my point of view).
>> 
>>> 
>>> | if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)
>>> 
>>> At least it fixes the case I mentioned above, doesn't it? And we
>>> definitely can't protect users from wrong sysroot paths on OS X systems
>>> like in your example. This case looks like "Сам себе злобный Буратино".
>> 
>> Yes, it fixes. But it does not on MacOS. Hence, why do we need to handle
>> this only on Linux? Moreover, who will set CMAKE_OSX_SYSROOT (not for
>> the testing reason) on Linux? There is even *OSX* in the variable name.
>> I am more concerned with the fact, one can break the build with this on
>> MacOS. Unfortunately, it seems in this case we can do nothing with it.
>> 
>> Sorry, but I see no arguments for solving this particular case.
>> 
>> Ignoring.
>> 
>>> 
>>>> 
>>>>> | In file included from lua.h:14,
>>>>> |                  from lj_arch.h:9:
>>>>> | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9:16: fatal error: stdint.h: No such file or directory
>>>>> |     9 | # include_next <stdint.h>
>>>>> |       |                ^~~~~~~~~~
>>>>> | compilation terminated.
>>>>> | CMake Error at cmake/LuaJITUtils.cmake:48 (message):
>>>>> |   [LuaJITArch] Unsupported target architecture
>>>>> | Call Stack (most recent call first):
>>>>> |   cmake/SetTargetFlags.cmake:16 (LuaJITArch)
>>>>> |   src/CMakeLists.txt:17 (include)
>>>>> 
>>>>>> +    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
>>>>>> +  endif()
>>>>>>   # XXX: <execute_process> simply splits the COMMAND argument by
>>>>>>   # spaces with no further parsing. At the same time GCC is bad in
>>>>>>   # argument handling, so let's help it a bit.
>>>>>> 
>>>>>> ================================================================================
>>>>>> 
>>>>>> [1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
>>>>>> [2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28
>>>>>> 
>>>>>> 
>>>>>> -- 
>>>>>> Best regards,
>>>>>> IM
>>>>> 
>>>>> [1]: https://cmake.org/cmake/help/v3.0/variable/CMAKE_OSX_SYSROOT.html
>>>>> 
>>>>> -- 
>>>>> Best regards,
>>>>> Sergey Kaplun
>>>> 
>>>> -- 
>>>> Best regards,
>>>> IM
>>> 
>>> -- 
>>> Best regards,
>>> Sergey Kaplun
>> 
>> [1]: https://github.com/LuaJIT/LuaJIT/commit/8961a92
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 36154 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches]

Sergos,

Thanks for your review!

On 19.05.21, Sergey Ostanevich wrote:
> Hi! 
> 
> I believe we move from patch to two files originally
> +++ b/CMakeLists.txt
> +++ b/cmake/LuaJITUtils.cmake
> 
> to just one, in 432cdf62303b0d609525acc84b01b92ae468d327
> +++ b/cmake/LuaJITUtils.cmake
> 
> I expect to see a ‘v2’ in such a case - for the forthcoming patches.

Ouch, my bad. There were so many iterative patches that even if all
changes are in the same files, it's worth to send v2 series.

> 
> Anyways, if I read the correct patch above from the imun/gh-5983-fix-build-on-m1
> branch then it’s LGTM with just a typo fix.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> Sergos
> 
> 
> > On 19 May 2021, at 14:38, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > As a result of offline discussion with Sergey, I've finally understood
> > the docs and fixed the issue. Diff is below:
> > 
> > ================================================================================
> > 
> > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > index 3497edc4..29c425de 100644
> > --- a/cmake/LuaJITUtils.cmake
> > +++ b/cmake/LuaJITUtils.cmake
> > @@ -4,7 +4,12 @@ function(LuaJITTestArch outvar strflags)
> >   # machinery) or explicitly (manually by configuration options).
> >   # Need -isysroot flag on recentish MacOS after command line
> >   # tools no longer provide headers in /usr/include.
> > -  if(CMAKE_OSX_SYSROOT)
> > +  # XXX: According to CMake documentation[1], CMAKE_OSX_SYSROOT
> > +  # variable *should* be ignored on the plaforms other than MacOS.
> 					  platforms

Fixed, thanks!

> > +  # It is ignored by CMake, but since this routine is extension
> > +  # it also should follow this policy.
> > +  # [1]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html <https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html>
> > +  if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_OSX_SYSROOT)
> >     set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
> >   endif()
> >   # XXX: <execute_process> simply splits the COMMAND argument by
> > 
> > ================================================================================
> > 

<snipped>

> > 
> > -- 
> > Best regards,
> > IM
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit 2/3] OSX/iOS: Handle iOS simulator and ARM64 Macs.

[Igor Munkin via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from commit 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203)

After Apple released Macs working on ARM64, the previous recipe in
lj_arch.h for detecting various Apple platforms is not valid anymore.
Fortunately, there is a system header (i.e. TargetConditionals.h),
provided by SDK with the proper defines to be set. Starting from this
patch, LuaJIT identifies Apple hosts via this header.

Since testing machinery assumes that LuaJIT is built with JIT support
being enabled unconditionally, a smoke test for it is also added
alongside with this patch.

Igor Munkin:
* added the description and the test for the problem
* backported the original patch to tarantool/luajit repo

Part of tarantool/tarantool#5629
Relates to tarantool/tarantool#5983

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 src/lj_arch.h                                         |  8 +++++++-
 .../gh-5983-jit-library-smoke-tests.test.lua          | 11 +++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua

diff --git a/src/lj_arch.h b/src/lj_arch.h
index d8676e92..5bf0afb8 100644
--- a/src/lj_arch.h
+++ b/src/lj_arch.h
@@ -69,6 +69,7 @@
 #elif defined(__linux__)
 #define LUAJIT_OS	LUAJIT_OS_LINUX
 #elif defined(__MACH__) && defined(__APPLE__)
+#include "TargetConditionals.h"
 #define LUAJIT_OS	LUAJIT_OS_OSX
 #elif (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \
        defined(__NetBSD__) || defined(__OpenBSD__) || \
@@ -103,10 +104,15 @@
 #define LJ_TARGET_WINDOWS	(LUAJIT_OS == LUAJIT_OS_WINDOWS)
 #define LJ_TARGET_LINUX		(LUAJIT_OS == LUAJIT_OS_LINUX)
 #define LJ_TARGET_OSX		(LUAJIT_OS == LUAJIT_OS_OSX)
-#define LJ_TARGET_IOS		(LJ_TARGET_OSX && (LUAJIT_TARGET == LUAJIT_ARCH_ARM || LUAJIT_TARGET == LUAJIT_ARCH_ARM64))
 #define LJ_TARGET_POSIX		(LUAJIT_OS > LUAJIT_OS_WINDOWS)
 #define LJ_TARGET_DLOPEN	LJ_TARGET_POSIX
 
+#if TARGET_OS_IPHONE
+#define LJ_TARGET_IOS		1
+#else
+#define LJ_TARGET_IOS		0
+#endif
+
 #ifdef __CELLOS_LV2__
 #define LJ_TARGET_PS3		1
 #define LJ_TARGET_CONSOLE	1
diff --git a/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua b/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
new file mode 100644
index 00000000..b23dd712
--- /dev/null
+++ b/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
@@ -0,0 +1,11 @@
+local tap = require('tap')
+
+local test = tap.test('gh-5983-jit-library-smoke-tests')
+test:plan(1)
+
+-- Just check whether LuaJIT is built with JIT support. Otherwise,
+-- <jit.on> raises an error that is handled via <pcall> and passed
+-- as a second argument to the assertion.
+test:ok(pcall(jit.on))
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit 2/3] OSX/iOS: Handle iOS simulator and ARM64 Macs.

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch! LGTM!

On 11.05.21, Igor Munkin wrote:
> From: Mike Pall <mike>

Side note: I am a little bit confused by this line (I thought that it is
a part of the commit message). May be it is better to drop it or to
write like:

| # From: Mike Pall <mike>

> 
> (cherry picked from commit 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203)
> 
> After Apple released Macs working on ARM64, the previous recipe in
> lj_arch.h for detecting various Apple platforms is not valid anymore.
> Fortunately, there is a system header (i.e. TargetConditionals.h),
> provided by SDK with the proper defines to be set. Starting from this
> patch, LuaJIT identifies Apple hosts via this header.
> 
> Since testing machinery assumes that LuaJIT is built with JIT support
> being enabled unconditionally, a smoke test for it is also added
> alongside with this patch.
> 
> Igor Munkin:
> * added the description and the test for the problem
> * backported the original patch to tarantool/luajit repo
> 
> Part of tarantool/tarantool#5629
> Relates to tarantool/tarantool#5983
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---

<snipped>


-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/3] OSX/iOS: Handle iOS simulator and ARM64 Macs.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review! I've made some minor changes to associate this
patch with the proper issue[1]. Diff is below:

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

diff --git a/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua b/test/tarantool-tests/gh-6065-jit-library-smoke-tests.test.lua
similarity index 82%
rename from test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
rename to test/tarantool-tests/gh-6065-jit-library-smoke-tests.test.lua
index b23dd712..7110e351 100644
--- a/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
+++ b/test/tarantool-tests/gh-6065-jit-library-smoke-tests.test.lua
@@ -1,6 +1,6 @@
 local tap = require('tap')
 
-local test = tap.test('gh-5983-jit-library-smoke-tests')
+local test = tap.test('gh-6065-jit-library-smoke-tests')
 test:plan(1)
 
 -- Just check whether LuaJIT is built with JIT support. Otherwise,

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

On 11.05.21, Sergey Kaplun wrote:
> 
> Hi, Igor!
> 
> Thanks for the patch! LGTM!

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

> 
> On 11.05.21, Igor Munkin wrote:
> > From: Mike Pall <mike>
> 
> Side note: I am a little bit confused by this line (I thought that it is
> a part of the commit message). May be it is better to drop it or to
> write like:

Hm, this looks like a common practice if *you* send *someone's* patch to
be applied. IIRC, git am takes the entry below as commit author.

> 
> | # From: Mike Pall <mike>
> 
> > 
> > (cherry picked from commit 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203)
> > 
> > After Apple released Macs working on ARM64, the previous recipe in
> > lj_arch.h for detecting various Apple platforms is not valid anymore.
> > Fortunately, there is a system header (i.e. TargetConditionals.h),
> > provided by SDK with the proper defines to be set. Starting from this
> > patch, LuaJIT identifies Apple hosts via this header.
> > 
> > Since testing machinery assumes that LuaJIT is built with JIT support
> > being enabled unconditionally, a smoke test for it is also added
> > alongside with this patch.
> > 
> > Igor Munkin:
> > * added the description and the test for the problem
> > * backported the original patch to tarantool/luajit repo
> > 

Mentioned the issue[1]:
| Resolves tarantool/tarantool#6065

> > Part of tarantool/tarantool#5629
> > Relates to tarantool/tarantool#5983
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> 
> <snipped>
> 
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://github.com/tarantool/tarantool/issues/6065

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/3] OSX/iOS: Handle iOS simulator and ARM64 Macs.

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3082 bytes --]

Thanks for the patch, LGTM.

Sergos

> On 11 May 2021, at 14:03, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergey,
> 
> Thanks for your review! I've made some minor changes to associate this
> patch with the proper issue[1]. Diff is below:
> 
> ================================================================================
> 
> diff --git a/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua b/test/tarantool-tests/gh-6065-jit-library-smoke-tests.test.lua
> similarity index 82%
> rename from test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
> rename to test/tarantool-tests/gh-6065-jit-library-smoke-tests.test.lua
> index b23dd712..7110e351 100644
> --- a/test/tarantool-tests/gh-5983-jit-library-smoke-tests.test.lua
> +++ b/test/tarantool-tests/gh-6065-jit-library-smoke-tests.test.lua
> @@ -1,6 +1,6 @@
> local tap = require('tap')
> 
> -local test = tap.test('gh-5983-jit-library-smoke-tests')
> +local test = tap.test('gh-6065-jit-library-smoke-tests')
> test:plan(1)
> 
> -- Just check whether LuaJIT is built with JIT support. Otherwise,
> 
> ================================================================================
> 
> On 11.05.21, Sergey Kaplun wrote:
>> 
>> Hi, Igor!
>> 
>> Thanks for the patch! LGTM!
> 
> Added your tag:
> | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>>
> 
>> 
>> On 11.05.21, Igor Munkin wrote:
>>> From: Mike Pall <mike>
>> 
>> Side note: I am a little bit confused by this line (I thought that it is
>> a part of the commit message). May be it is better to drop it or to
>> write like:
> 
> Hm, this looks like a common practice if *you* send *someone's* patch to
> be applied. IIRC, git am takes the entry below as commit author.
> 
>> 
>> | # From: Mike Pall <mike>
>> 
>>> 
>>> (cherry picked from commit 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203)
>>> 
>>> After Apple released Macs working on ARM64, the previous recipe in
>>> lj_arch.h for detecting various Apple platforms is not valid anymore.
>>> Fortunately, there is a system header (i.e. TargetConditionals.h),
>>> provided by SDK with the proper defines to be set. Starting from this
>>> patch, LuaJIT identifies Apple hosts via this header.
>>> 
>>> Since testing machinery assumes that LuaJIT is built with JIT support
>>> being enabled unconditionally, a smoke test for it is also added
>>> alongside with this patch.
>>> 
>>> Igor Munkin:
>>> * added the description and the test for the problem
>>> * backported the original patch to tarantool/luajit repo
>>> 
> 
> Mentioned the issue[1]:
> | Resolves tarantool/tarantool#6065
> 
>>> Part of tarantool/tarantool#5629
>>> Relates to tarantool/tarantool#5983
>>> 
>>> Signed-off-by: Igor Munkin <imun@tarantool.org <mailto:imun@tarantool.org>>
>>> ---
>> 
>> <snipped>
>> 
>> 
>> -- 
>> Best regards,
>> Sergey Kaplun
> 
> [1]: https://github.com/tarantool/tarantool/issues/6065 <https://github.com/tarantool/tarantool/issues/6065>
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 32648 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/3] OSX/iOS: Handle iOS simulator and ARM64 Macs.

[Igor Munkin via Tarantool-patches]

Sergos,

Thanks for your review!

On 14.05.21, Sergey Ostanevich wrote:
> Thanks for the patch, LGTM.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> Sergos
> 

<snipped>

> 

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Igor Munkin via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Igor Munkin.

(cherry picked from commit 521b367567dc5d91d7f9ae29c257998953e24e53)

This patch fixes the issue introduced by commit
2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203 ('OSX/iOS: Handle iOS simulator
and ARM64 Macs.'). Within the mentioned commit LJ_TARGET_IOS define is
set via Apple system header to enable several features (e.g. JIT and
external unwinder) on ARM64 Macs, but its usage was not adjusted
source-wide. This is done for FFI machinery within this commit.

Since all LJ_TARGET_IOS usage is done with LJ_TARGET_ARM64 define being
set, we can simply replace all occurrences with LJ_TARGET_OSX.

Igor Munkin:
* added the description and the test for the problem

Part of tarantool/tarantool#5629
Relates to tarantool/tarantool#5983

Reported-by: Nikita Pettik <korablev@tarantool.org>
Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 src/lj_ccall.c                                   |  8 ++++----
 src/lj_ccallback.c                               |  2 +-
 .../lj-695-ffi-vararg-call.test.lua              | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
 create mode 100644 test/tarantool-tests/lj-695-ffi-vararg-call.test.lua

diff --git a/src/lj_ccall.c b/src/lj_ccall.c
index 5c252e5b..06d1b3bd 100644
--- a/src/lj_ccall.c
+++ b/src/lj_ccall.c
@@ -334,7 +334,7 @@
   isfp = sz == 2*sizeof(float) ? 2 : 1;
 
 #define CCALL_HANDLE_REGARG \
-  if (LJ_TARGET_IOS && isva) { \
+  if (LJ_TARGET_OSX && isva) { \
     /* IOS: All variadic arguments are on the stack. */ \
   } else if (isfp) {  /* Try to pass argument in FPRs. */ \
     int n2 = ctype_isvector(d->info) ? 1 : n*isfp; \
@@ -344,10 +344,10 @@
       goto done; \
     } else { \
       nfpr = CCALL_NARG_FPR;  /* Prevent reordering. */ \
-      if (LJ_TARGET_IOS && d->size < 8) goto err_nyi; \
+      if (LJ_TARGET_OSX && d->size < 8) goto err_nyi; \
     } \
   } else {  /* Try to pass argument in GPRs. */ \
-    if (!LJ_TARGET_IOS && (d->info & CTF_ALIGN) > CTALIGN_PTR) \
+    if (!LJ_TARGET_OSX && (d->info & CTF_ALIGN) > CTALIGN_PTR) \
       ngpr = (ngpr + 1u) & ~1u;  /* Align to regpair. */ \
     if (ngpr + n <= maxgpr) { \
       dp = &cc->gpr[ngpr]; \
@@ -355,7 +355,7 @@
       goto done; \
     } else { \
       ngpr = maxgpr;  /* Prevent reordering. */ \
-      if (LJ_TARGET_IOS && d->size < 8) goto err_nyi; \
+      if (LJ_TARGET_OSX && d->size < 8) goto err_nyi; \
     } \
   }
 
diff --git a/src/lj_ccallback.c b/src/lj_ccallback.c
index 846827b1..9158c2d3 100644
--- a/src/lj_ccallback.c
+++ b/src/lj_ccallback.c
@@ -406,7 +406,7 @@ void lj_ccallback_mcode_free(CTState *cts)
       nfpr = CCALL_NARG_FPR;  /* Prevent reordering. */ \
     } \
   } else { \
-    if (!LJ_TARGET_IOS && n > 1) \
+    if (!LJ_TARGET_OSX && n > 1) \
       ngpr = (ngpr + 1u) & ~1u;  /* Align to regpair. */ \
     if (ngpr + n <= maxgpr) { \
       sp = &cts->cb.gpr[ngpr]; \
diff --git a/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua b/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua
new file mode 100644
index 00000000..04be1998
--- /dev/null
+++ b/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua
@@ -0,0 +1,16 @@
+local tap = require('tap')
+
+local test = tap.test('lj-695-ffi-vararg-call')
+test:plan(2)
+
+local ffi = require('ffi')
+local str = ffi.new('char[256]')
+ffi.cdef('int sprintf(char *str, const char *format, ...)')
+local strlen = ffi.C.sprintf(str, 'try vararg function: %s:%.2f(%d) - %llu',
+                             'imun', 9, 9LL, -1ULL)
+
+local result = ffi.string(str)
+test:is(#result, strlen)
+test:is(result, 'try vararg function: imun:9.00(9) - 18446744073709551615')
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 11.05.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> LGTM!

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

Also mentioned the issue[1]:
| Resolves tarantool/tarantool#6066

> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://github.com/tarantool/tarantool/issues/6066

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Sergey Ostanevich via Tarantool-patches]

Hi!

I can’t get this then

  src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS

How’s that survived in 2.1 branch?

regards,
Sergos


> On 11 May 2021, at 14:31, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergey,
> 
> Thanks for your review!
> 
> On 11.05.21, Sergey Kaplun wrote:
>> Hi, Igor!
>> 
>> Thanks for the patch!
>> LGTM!
> 
> Added your tag:
> | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
> 
> Also mentioned the issue[1]:
> | Resolves tarantool/tarantool#6066
> 
>> 
>> -- 
>> Best regards,
>> Sergey Kaplun
> 
> [1]: https://github.com/tarantool/tarantool/issues/6066
> 
> -- 
> Best regards,
> IM

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Igor Munkin via Tarantool-patches]

Sergos,

On 12.05.21, Sergey Ostanevich wrote:
> Hi!
> 
> I can’t get this then
> 
>   src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS

There is a verbose comment nearby[1] and the corresponding issue[2].

> 
> How’s that survived in 2.1 branch?
> 
> regards,
> Sergos
> 
> 
> > On 11 May 2021, at 14:31, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > Sergey,
> > 
> > Thanks for your review!
> > 
> > On 11.05.21, Sergey Kaplun wrote:
> >> Hi, Igor!
> >> 
> >> Thanks for the patch!
> >> LGTM!
> > 
> > Added your tag:
> > | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
> > 
> > Also mentioned the issue[1]:
> > | Resolves tarantool/tarantool#6066
> > 
> >> 
> >> -- 
> >> Best regards,
> >> Sergey Kaplun
> > 
> > [1]: https://github.com/tarantool/tarantool/issues/6066
> > 
> > -- 
> > Best regards,
> > IM
> 

[1]: https://github.com/LuaJIT/LuaJIT/blob/v2.1/src/lj_prng.c#L113-L119
[2]: https://github.com/LuaJIT/LuaJIT/issues/668

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

Well, this doesn't help me with

> Since all LJ_TARGET_IOS usage is done with LJ_TARGET_ARM64 define being
> set, we can simply replace all occurrences with LJ_TARGET_OSX.

alongside with

> src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS

because if we apply first then the second will evaluate into

src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_OSX

which is always false?

Also there are still operable 32bit apps, including games, so iOS still supports 32bits - can this change cause problems? I believe Mike doesn’t care too much, so it can easily slip through.

Sergos



> On 13 May 2021, at 00:59, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergos,
> 
> On 12.05.21, Sergey Ostanevich wrote:
>> Hi!
>> 
>> I can’t get this then
>> 
>>  src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS
> 
> There is a verbose comment nearby[1] and the corresponding issue[2].
> 
>> 
>> How’s that survived in 2.1 branch?
>> 
>> regards,
>> Sergos
>> 
>> 
>>> On 11 May 2021, at 14:31, Igor Munkin <imun@tarantool.org> wrote:
>>> 
>>> Sergey,
>>> 
>>> Thanks for your review!
>>> 
>>> On 11.05.21, Sergey Kaplun wrote:
>>>> Hi, Igor!
>>>> 
>>>> Thanks for the patch!
>>>> LGTM!
>>> 
>>> Added your tag:
>>> | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
>>> 
>>> Also mentioned the issue[1]:
>>> | Resolves tarantool/tarantool#6066
>>> 
>>>> 
>>>> -- 
>>>> Best regards,
>>>> Sergey Kaplun
>>> 
>>> [1]: https://github.com/tarantool/tarantool/issues/6066
>>> 
>>> -- 
>>> Best regards,
>>> IM
>> 
> 
> [1]: https://github.com/LuaJIT/LuaJIT/blob/v2.1/src/lj_prng.c#L113-L119
> [2]: https://github.com/LuaJIT/LuaJIT/issues/668
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 4227 bytes --]

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Igor Munkin via Tarantool-patches]

Sergos,

On 13.05.21, Sergey Ostanevich wrote:
> Well, this doesn't help me with
> 
> > Since all LJ_TARGET_IOS usage is done with LJ_TARGET_ARM64 define being
> > set, we can simply replace all occurrences with LJ_TARGET_OSX.

OK, as a result of offline discussion I've finally got it (hope you
too). Everything written above relates only to FFI sources that are
touched within this changeset (consider FFI prefix in commit subject).

> 
> alongside with
> 
> > src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS

Strictly saying, this commit[1] has not been backported yet.

> 
> because if we apply first then the second will evaluate into
> 
> src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_OSX
> 
> which is always false?
> 
> Also there are still operable 32bit apps, including games, so iOS
> still supports 32bits - can this change cause problems? I believe Mike
> doesn’t care too much, so it can easily slip through.

As we discussed, 32-bit platforms are not touched in this commit, so
nothing criminal.

Anyway, I see this is unclear to you, but everything is OK for Sergey.
Do I need to reword this part of commit message or even drop it?

> 
> Sergos
> 
> 
> 
> > On 13 May 2021, at 00:59, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > Sergos,
> > 
> > On 12.05.21, Sergey Ostanevich wrote:
> >> Hi!
> >> 
> >> I can’t get this then
> >> 
> >>  src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS
> > 
> > There is a verbose comment nearby[1] and the corresponding issue[2].
> > 
> >> 
> >> How’s that survived in 2.1 branch?
> >> 
> >> regards,
> >> Sergos
> >> 
> >> 
> >>> On 11 May 2021, at 14:31, Igor Munkin <imun@tarantool.org> wrote:
> >>> 
> >>> Sergey,
> >>> 
> >>> Thanks for your review!
> >>> 
> >>> On 11.05.21, Sergey Kaplun wrote:
> >>>> Hi, Igor!
> >>>> 
> >>>> Thanks for the patch!
> >>>> LGTM!
> >>> 
> >>> Added your tag:
> >>> | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
> >>> 
> >>> Also mentioned the issue[1]:
> >>> | Resolves tarantool/tarantool#6066
> >>> 
> >>>> 
> >>>> -- 
> >>>> Best regards,
> >>>> Sergey Kaplun
> >>> 
> >>> [1]: https://github.com/tarantool/tarantool/issues/6066
> >>> 
> >>> -- 
> >>> Best regards,
> >>> IM
> >> 
> > 
> > [1]: https://github.com/LuaJIT/LuaJIT/blob/v2.1/src/lj_prng.c#L113-L119
> > [2]: https://github.com/LuaJIT/LuaJIT/issues/668
> > 
> > -- 
> > Best regards,
> > IM
> 

[1]: https://github.com/LuaJIT/LuaJIT/commit/7877369

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]



> On 13 May 2021, at 13:44, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergos,
> 
> On 13.05.21, Sergey Ostanevich wrote:
>> Well, this doesn't help me with
>> 
>>> Since all LJ_TARGET_IOS usage is done with LJ_TARGET_ARM64 define being
>>> set, we can simply replace all occurrences with LJ_TARGET_OSX.
> 
> OK, as a result of offline discussion I've finally got it (hope you
> too). Everything written above relates only to FFI sources that are
> touched within this changeset (consider FFI prefix in commit subject).
> 
>> 
>> alongside with
>> 
>>> src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS
> 
> Strictly saying, this commit[1] has not been backported yet.
> 
>> 
>> because if we apply first then the second will evaluate into
>> 
>> src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_OSX
>> 
>> which is always false?
>> 
>> Also there are still operable 32bit apps, including games, so iOS
>> still supports 32bits - can this change cause problems? I believe Mike
>> doesn’t care too much, so it can easily slip through.
> 
> As we discussed, 32-bit platforms are not touched in this commit, so
> nothing criminal.
> 
> Anyway, I see this is unclear to you, but everything is OK for Sergey.
> Do I need to reword this part of commit message or even drop it?

Just update the wording

All LJ_TARGET_IOS uses in FFI sources are done with LJ_TARGET_ARM64 define being
set, so we can simply replace these occurrences with LJ_TARGET_OSX.

And it’s LGTM.

Sergos
> 
>> 
>> Sergos
>> 
>> 
>> 
>>> On 13 May 2021, at 00:59, Igor Munkin <imun@tarantool.org> wrote:
>>> 
>>> Sergos,
>>> 
>>> On 12.05.21, Sergey Ostanevich wrote:
>>>> Hi!
>>>> 
>>>> I can’t get this then
>>>> 
>>>> src/lj_prng.c:112:#if LJ_TARGET_OSX && !LJ_TARGET_IOS
>>> 
>>> There is a verbose comment nearby[1] and the corresponding issue[2].
>>> 
>>>> 
>>>> How’s that survived in 2.1 branch?
>>>> 
>>>> regards,
>>>> Sergos
>>>> 
>>>> 
>>>>> On 11 May 2021, at 14:31, Igor Munkin <imun@tarantool.org> wrote:
>>>>> 
>>>>> Sergey,
>>>>> 
>>>>> Thanks for your review!
>>>>> 
>>>>> On 11.05.21, Sergey Kaplun wrote:
>>>>>> Hi, Igor!
>>>>>> 
>>>>>> Thanks for the patch!
>>>>>> LGTM!
>>>>> 
>>>>> Added your tag:
>>>>> | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
>>>>> 
>>>>> Also mentioned the issue[1]:
>>>>> | Resolves tarantool/tarantool#6066
>>>>> 
>>>>>> 
>>>>>> -- 
>>>>>> Best regards,
>>>>>> Sergey Kaplun
>>>>> 
>>>>> [1]: https://github.com/tarantool/tarantool/issues/6066
>>>>> 
>>>>> -- 
>>>>> Best regards,
>>>>> IM
>>>> 
>>> 
>>> [1]: https://github.com/LuaJIT/LuaJIT/blob/v2.1/src/lj_prng.c#L113-L119
>>> [2]: https://github.com/LuaJIT/LuaJIT/issues/668
>>> 
>>> -- 
>>> Best regards,
>>> IM
>> 
> 
> [1]: https://github.com/LuaJIT/LuaJIT/commit/7877369 <https://github.com/LuaJIT/LuaJIT/commit/7877369>
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 20436 bytes --]

Re: [Tarantool-patches] [PATCH luajit 3/3] FFI/ARM64/OSX: Fix vararg call handling.

[Igor Munkin via Tarantool-patches]

Sergos,

On 14.05.21, Sergey Ostanevich wrote:
> 

<snipped>

> > Anyway, I see this is unclear to you, but everything is OK for Sergey.
> > Do I need to reword this part of commit message or even drop it?
> 
> Just update the wording

Copy.

> 
> All LJ_TARGET_IOS uses in FFI sources are done with LJ_TARGET_ARM64 define being
> set, so we can simply replace these occurrences with LJ_TARGET_OSX.

Here is the final wording:
| FFI/ARM64/OSX: Fix vararg call handling.
|
| Thanks to Igor Munkin.
|
| (cherry picked from commit 521b367567dc5d91d7f9ae29c257998953e24e53)
|
| This patch fixes the issue introduced by commit
| 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203 ('OSX/iOS: Handle iOS simulator
| and ARM64 Macs.'). Within the mentioned commit LJ_TARGET_IOS define is
| set via Apple system header to enable several features (e.g. JIT and
| external unwinder) on ARM64 Macs, but its usage was not adjusted
| source-wide. This is done for FFI machinery within this commit.
|
| All LJ_TARGET_IOS uses in FFI sources are done with LJ_TARGET_ARM64
| define being set, so we can simply replace these occurrences with
| LJ_TARGET_OSX.
|
| Igor Munkin:
| * added the description and the test for the problem
|
| Resolves tarantool/tarantool#6066
| Part of tarantool/tarantool#5629
| Relates to tarantool/tarantool#5983
|
| Reported-by: Nikita Pettik <korablev@tarantool.org>
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>
| Signed-off-by: Igor Munkin <imun@tarantool.org>

> 
> And it’s LGTM.

Also added your tag, thanks!

> 
> Sergos

<snipped>

> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/3] Basic fixes for LuaJIT on ARM64 Macs

[Igor Munkin via Tarantool-patches]

I've checked the patchset into tarantool branch in tarantool/luajit
and bumped a new version in master.

Furthermore, I've checked the first patch of the series ('build: pass
sysroot to MacOS SDK') into other long-term branches in tarantool/luajit
and bumped a new version in 1.10, 2.7 and 2.8 also.

On 11.05.21, Igor Munkin wrote:
> This series consists of the three patches: the one for CMake build
> machinery and two commits backported from the vanilla LuaJIT trunk[1].
> If the latter commits are OK, they can be uses as an examples in
> backporting procedure document[2] for the others to be adopted.
> 
> The first patch fixes the issue missed in #4862: CMake machinery fails
> to locate system headers provided by SDK on MacOS platforms. The bug was
> unnoticed for a while, since Tarantool testing environment doesn't use
> Debug build with LUA_USE_ASSERT enabled on MacOS hosts. Anyway, even if
> there was Debug build, there is a mess with the flags as a result of
> #4862, so LuaJIT internal assertions would be enabled neither. The fix
> contains of two parts:
> 1. Set the sysroot to SDK root directory if it is not set before.
> 2. Use ${CMAKE_C_FLAGS} in <LuaJITTestArch> auxiliary routine.
> 
> The second patch is a backport of 2e2fb8f[3]. When Apple released Macs
> working on ARM64, the previous recipe in lj_arch.h for detecting Apple
> platforms is not valid anymore. Fortunately, there is a system header
> (i.e. TargetConditionals.h), provided by SDK with the proper defines to
> be set. Since testing machinery assumes that LuaJIT is built with JIT
> support being enabled unconditionally, a smoke test for it is also added
> alongside with this patch.
> 
> The third patch is a backport of 521b367[4]. This patch fixes the issue
> introduced by commit 2e2fb8f6b5118e1a7996b76600c6ee98bfd5f203 ('OSX/iOS:
> Handle iOS simulator and ARM64 Macs.'). Within the mentioned commit
> LJ_TARGET_IOS define is set via Apple system header to enable several
> features (e.g. JIT and external unwinder) on ARM64 Macs, but its usage
> was not adjusted source-wide. This is done for FFI machinery within this
> commit. Since all LJ_TARGET_IOS usage is done with LJ_TARGET_ARM64
> define being set, we can simply replace all entries with LJ_TARGET_OSX.
> 
> Branch: https://github.com/tarantool/luajit/tree/imun/gh-5983-fix-build-on-m1
> Issues:
> * https://github.com/tarantool/tarantool/issues/5983
> * https://github.com/tarantool/tarantool/issues/5629
> * https://github.com/tarantool/tarantool/issues/4862
> CI looks to be OK[5] except the known problems with ASAN[6].
> 
> [1]: https://github.com/LuaJIT/LuaJIT
> [2]: https://github.com/tarantool/tarantool/wiki/LuaJIT-backporting-guidelines
> [3]: https://github.com/LuaJIT/LuaJIT/commit/2e2fb8f
> [4]: https://github.com/LuaJIT/LuaJIT/commit/521b367
> [5]: https://github.com/tarantool/tarantool/commit/fba91e1
> [6]: https://github.com/tarantool/tarantool/issues/6031
> 
> Igor Munkin (1):
>   build: pass sysroot to MacOS SDK
> 
> Mike Pall (2):
>   OSX/iOS: Handle iOS simulator and ARM64 Macs.
>   FFI/ARM64/OSX: Fix vararg call handling.
> 

<snipped>

> 
> -- 
> 2.25.0
> 

-- 
Best regards,
IM