[Tarantool-patches] [PATCH luajit] Fix Clang build.

[Tarantool-patches] [PATCH luajit] Fix Clang build.

[Maksim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

(cherry-picked from commit 384d6d56f4a3841fdef607a511dda92a579af2ff)

The `__GNUC__` macro signals that GNU extensions are supported
by a compiler. Some versions of the clang compiler
(clang 10.0.0 targeting the MSVC ABI, for example) may not
define that macro. However, they still support the GNU
extensions. This patch fixes the build for those compilers
by checking the `__clang__` macro value alongside `__GNUC__`.

Part of the patch relevant to 'lj_err.c' is omitted since all
of the required changes were already backported in the scope
of the patch b6d285214db9892aaef838950d200f395c93fe2d ('Cleanup
and enable external unwinding for more platforms.').

No tests were added since the issue is relevant for a very
specific type of very old clang 10.0.0, which is not really
relevant for us.

Maxim Kokryashkin:
* added the description for the problem

Part of tarantool/tarantool#9145
---
PR: https://github.com/tarantool/tarantool/pull/9215
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-595-fix-clang-build
Issue: https://github.com/luajit/luajit/issues/595
 src/lj_alloc.c    | 2 +-
 src/lj_def.h      | 4 ++--
 src/lj_emit_x86.h | 2 +-
 src/lj_ircall.h   | 2 +-
 src/lj_mcode.c    | 2 +-
 src/lj_strfmt.h   | 2 +-
 src/lj_strscan.c  | 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/lj_alloc.c b/src/lj_alloc.c
index 9e2fb1f6..f82c9854 100644
--- a/src/lj_alloc.c
+++ b/src/lj_alloc.c
@@ -610,7 +610,7 @@ static int has_segment_link(mstate m, msegmentptr ss)
   noncontiguous segments are added.
 */
 #define TOP_FOOT_SIZE\
-  (align_offset(chunk2mem(0))+pad_request(sizeof(struct malloc_segment))+MIN_CHUNK_SIZE)
+  (align_offset(TWO_SIZE_T_SIZES)+pad_request(sizeof(struct malloc_segment))+MIN_CHUNK_SIZE)

 /* ---------------------------- Indexing Bins ---------------------------- */

diff --git a/src/lj_def.h b/src/lj_def.h
index ba4dcc9d..a5bca6b0 100644
--- a/src/lj_def.h
+++ b/src/lj_def.h
@@ -120,7 +120,7 @@ typedef uintptr_t BloomFilter;
 #define bloomset(b, x)	((b) |= bloombit((x)))
 #define bloomtest(b, x)	((b) & bloombit((x)))

-#if defined(__GNUC__) || defined(__psp2__)
+#if defined(__GNUC__) || defined(__clang__) || defined(__psp2__)

 #define LJ_NORET	__attribute__((noreturn))
 #define LJ_ALIGN(n)	__attribute__((aligned(n)))
@@ -182,7 +182,7 @@ static LJ_AINLINE uint64_t lj_bswap64(uint64_t x)
 {
   return ((uint64_t)lj_bswap((uint32_t)x)<<32) | lj_bswap((uint32_t)(x>>32));
 }
-#elif (__GNUC__ > 4) || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
+#elif (__GNUC__ > 4) || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3) || __clang__
 static LJ_AINLINE uint32_t lj_bswap(uint32_t x)
 {
   return (uint32_t)__builtin_bswap32((int32_t)x);
diff --git a/src/lj_emit_x86.h b/src/lj_emit_x86.h
index eaef17fc..f4990151 100644
--- a/src/lj_emit_x86.h
+++ b/src/lj_emit_x86.h
@@ -45,7 +45,7 @@ static LJ_AINLINE MCode *emit_op(x86Op xo, Reg rr, Reg rb, Reg rx,
     *(uint32_t *)(p+delta-5) = (uint32_t)xo;
     return p+delta-5;
   }
-#if defined(__GNUC__)
+#if defined(__GNUC__) || defined(__clang__)
   if (__builtin_constant_p(xo) && n == -2)
     p[delta-2] = (MCode)(xo >> 24);
   else if (__builtin_constant_p(xo) && n == -3)
diff --git a/src/lj_ircall.h b/src/lj_ircall.h
index 9c195918..7746965a 100644
--- a/src/lj_ircall.h
+++ b/src/lj_ircall.h
@@ -332,7 +332,7 @@ extern double lj_vm_sfmax(double a, double b);
 #endif

 #if LJ_HASFFI && LJ_NEED_FP64 && !(LJ_TARGET_ARM && LJ_SOFTFP)
-#ifdef __GNUC__
+#if defined(__GNUC__) || defined(__clang__)
 #define fp64_l2d __floatdidf
 #define fp64_ul2d __floatundidf
 #define fp64_l2f __floatdisf
diff --git a/src/lj_mcode.c b/src/lj_mcode.c
index a88d16bd..33ca2396 100644
--- a/src/lj_mcode.c
+++ b/src/lj_mcode.c
@@ -44,7 +44,7 @@ void lj_mcode_sync(void *start, void *end)
   sys_icache_invalidate(start, (char *)end-(char *)start);
 #elif LJ_TARGET_PPC
   lj_vm_cachesync(start, end);
-#elif defined(__GNUC__)
+#elif defined(__GNUC__) || defined(__clang__)
   __clear_cache(start, end);
 #else
 #error "Missing builtin to flush instruction cache"
diff --git a/src/lj_strfmt.h b/src/lj_strfmt.h
index 0e1d8946..d3b3a0ae 100644
--- a/src/lj_strfmt.h
+++ b/src/lj_strfmt.h
@@ -118,7 +118,7 @@ LJ_FUNC GCstr * LJ_FASTCALL lj_strfmt_obj(lua_State *L, cTValue *o);
 LJ_FUNC const char *lj_strfmt_pushvf(lua_State *L, const char *fmt,
 				     va_list argp);
 LJ_FUNC const char *lj_strfmt_pushf(lua_State *L, const char *fmt, ...)
-#ifdef __GNUC__
+#if defined(__GNUC__) || defined(__clang__)
   __attribute__ ((format (printf, 2, 3)))
 #endif
   ;
diff --git a/src/lj_strscan.c b/src/lj_strscan.c
index 47fab239..ae8945e1 100644
--- a/src/lj_strscan.c
+++ b/src/lj_strscan.c
@@ -79,7 +79,7 @@ static void strscan_double(uint64_t x, TValue *o, int32_t ex2, int32_t neg)
   /* Avoid double rounding for denormals. */
   if (LJ_UNLIKELY(ex2 <= -1075 && x != 0)) {
     /* NYI: all of this generates way too much code on 32 bit CPUs. */
-#if defined(__GNUC__) && LJ_64
+#if (defined(__GNUC__) || defined(__clang__)) && LJ_64
     int32_t b = (int32_t)(__builtin_clzll(x)^63);
 #else
     int32_t b = (x>>32) ? 32+(int32_t)lj_fls((uint32_t)(x>>32)) :
--
2.39.3 (Apple Git-145)

Re: [Tarantool-patches] [PATCH luajit] Fix Clang build.

[Sergey Bronnikov via Tarantool-patches]

On 10/3/23 18:11, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> (cherry-picked from commit 384d6d56f4a3841fdef607a511dda92a579af2ff)
>
<snipped>

Thanks, LGTM.

Re: [Tarantool-patches] [PATCH luajit] Fix Clang build.

[Sergey Kaplun via Tarantool-patches]

Hi, Max!
Thanks for the patch!
Please, consider my comment below.

On 03.10.23, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 

<snipped>

> 
> No tests were added since the issue is relevant for a very
> specific type of very old clang 10.0.0, which is not really
> relevant for us.

Indeed, but we should add the testing of clang build to our CI.
I suppose we may check the following matrix:
- [GC64, !GC64]
- [Debug, Release]
- Linux only
ARM64 may be omitted for now since it isn't our target platform.

It should be done aside from ASAN testing just to verify correctness
under this compiler. Should be easily added to the `test-luajit` matrix
in the <.github/workflows/testing.yml>:

> 
> Maxim Kokryashkin:
> * added the description for the problem
> 
> Part of tarantool/tarantool#9145
> ---
> PR: https://github.com/tarantool/tarantool/pull/9215
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-595-fix-clang-build
> Issue: https://github.com/luajit/luajit/issues/595

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix Clang build.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for you review.
Here is the diff with changes, branch is force-pushed.

===
diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml
index 2c637124..cb4ba57b 100644
--- a/.github/workflows/testing.yml
+++ b/.github/workflows/testing.yml
@@ -35,6 +35,7 @@ jobs:
         BUILDTYPE: [Debug, Release]
         GC64: [ON, OFF]
         OS: [Linux, macOS]
+        CC: [gcc, clang]
         include:
           - BUILDTYPE: Debug
             CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
@@ -45,12 +46,18 @@ jobs:
             GC64: OFF
           - OS: macOS
             GC64: OFF
+          - OS: macOS
+            CC: gcc
+          - ARCH: ARM64
+            OS: Linux
+            CC: clang
     runs-on: [self-hosted, regular, '${{ matrix.OS }}', '${{ matrix.ARCH }}']
     name: >
       LuaJIT
       (${{ matrix.OS }}/${{ matrix.ARCH }})
       ${{ matrix.BUILDTYPE }}
       GC64:${{ matrix.GC64 }}
+      CC:${{ matrix.CC }}
     steps:
       - uses: actions/checkout@v3
         with:
@@ -64,6 +71,7 @@ jobs:
         if: ${{ matrix.OS == 'macOS' }}
       - name: configure
         run: >
+          CC=${{ matrix.CC }}
           cmake -S . -B ${{ env.BUILDDIR }}
           -G Ninja
           ${{ matrix.CMAKEFLAGS }}
===
On Mon, Oct 09, 2023 at 12:13:51PM +0300, Sergey Kaplun wrote:
> Hi, Max!
> Thanks for the patch!
> Please, consider my comment below.
> 
> On 03.10.23, Maksim Kokryashkin wrote:
> > From: Mike Pall <mike>
> > 
> 
> <snipped>
> 
> > 
> > No tests were added since the issue is relevant for a very
> > specific type of very old clang 10.0.0, which is not really
> > relevant for us.
> 
> Indeed, but we should add the testing of clang build to our CI.
> I suppose we may check the following matrix:
> - [GC64, !GC64]
> - [Debug, Release]
> - Linux only
> ARM64 may be omitted for now since it isn't our target platform.
> 
> It should be done aside from ASAN testing just to verify correctness
> under this compiler. Should be easily added to the `test-luajit` matrix
> in the <.github/workflows/testing.yml>:
> 
> > 
> > Maxim Kokryashkin:
> > * added the description for the problem
> > 
> > Part of tarantool/tarantool#9145
> > ---
> > PR: https://github.com/tarantool/tarantool/pull/9215
> > Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-595-fix-clang-build
> > Issue: https://github.com/luajit/luajit/issues/595
> 
> <snipped>
> 
> -- 
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix Clang build.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix Clang build.

[Igor Munkin via Tarantool-patches]

Max,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 03.10.23, Maksim Kokryashkin via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> (cherry-picked from commit 384d6d56f4a3841fdef607a511dda92a579af2ff)
> 
> The `__GNUC__` macro signals that GNU extensions are supported
> by a compiler. Some versions of the clang compiler
> (clang 10.0.0 targeting the MSVC ABI, for example) may not
> define that macro. However, they still support the GNU
> extensions. This patch fixes the build for those compilers
> by checking the `__clang__` macro value alongside `__GNUC__`.
> 
> Part of the patch relevant to 'lj_err.c' is omitted since all
> of the required changes were already backported in the scope
> of the patch b6d285214db9892aaef838950d200f395c93fe2d ('Cleanup
> and enable external unwinding for more platforms.').
> 
> No tests were added since the issue is relevant for a very
> specific type of very old clang 10.0.0, which is not really
> relevant for us.
> 
> Maxim Kokryashkin:
> * added the description for the problem
> 
> Part of tarantool/tarantool#9145
> ---
> PR: https://github.com/tarantool/tarantool/pull/9215
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-595-fix-clang-build
> Issue: https://github.com/luajit/luajit/issues/595
>  src/lj_alloc.c    | 2 +-
>  src/lj_def.h      | 4 ++--
>  src/lj_emit_x86.h | 2 +-
>  src/lj_ircall.h   | 2 +-
>  src/lj_mcode.c    | 2 +-
>  src/lj_strfmt.h   | 2 +-
>  src/lj_strscan.c  | 2 +-
>  7 files changed, 8 insertions(+), 8 deletions(-)
> 

<snipped>

> --
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
IM