search

EleonoreMizo / fmtconv

Format conversion tools for Vapoursynth and Avisynth+
Do What The F*ck You Want To Public License
67 stars 14 forks source link

version 27 fail to build with static assertion failed: 128-bit atomic operations are not lock-fre #31

Closed marillat closed 2 years ago

marillat commented 2 years ago

Debian unstable amd64, gcc 11.2.0 vapoursynth 57

libtool: compile:  g++ -DPACKAGE_NAME=\"fmtconv\" -DPACKAGE_TARNAME=\"fmtconv\" -DPACKAGE_VERSION=\"r27\" "-DPACKAGE_STRING=\"fmtconv r27\"" "-DPACKAGE_BUGREPORT=\"http://forum.doom9.org/showthread.php?t=166504\"" "-DPACKAGE_URL=\"http://forum.doom9.org/showthread.php?t=166504\"" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DSTDC_HEADERS=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -I. -Wdate-time -D_FORTIFY_SOURCE=2 -std=c++14 -O3 -g3 -DNDEBUG -mfpmath=sse -msse2 -mcx16 -Wall -Wextra -Wshadow -Wunused -Wnull-dereference -Wvla -Wstrict-aliasing -Wuninitialized -Wunused-parameter -Wreorder -Wsign-compare -Wunreachable-code -Wconversion -Wno-sign-conversion -Wredundant-decls -Wno-ignored-attributes -Wno-expansion-to-defined -I./../../src -g -O2 -ffile-prefix-map=/home/marillat/src/fmtconv-27=. -fstack-protector-strong -Wformat -Werror=format-security -c ../../src/fmtcl/Dither.cpp  -fPIC -DPIC -o ../../src/fmtcl/.libs/Dither.o
In file included from ./../../src/conc/Interlocked.h:132,
                 from ./../../src/conc/AtomicPtr.hpp:27,
                 from ./../../src/conc/AtomicPtr.h:123,
                 from ./../../src/conc/CellPool.h:30,
                 from ./../../src/conc/ObjPool.h:44,
                 from ./../../src/fmtcl/Dither.h:26,
                 from ../../src/fmtcl/Dither.cpp:29:
./../../src/conc/Interlocked.hpp: In static member function 'static void conc::Interlocked::cas(conc::Interlocked::Data128&, volatile Data128&, const Data128&, const Data128&)':
./../../src/conc/Interlocked.hpp:329:43: error: static assertion failed: 128-bit atomic operations are not lock-free
  329 |                 __atomic_always_lock_free (sizeof (dest), nullptr),
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
make: *** [Makefile:1779: ../../src/fmtcl/Dither.lo] Error 1
EleonoreMizo commented 2 years ago

Thanks for the report. In src/conc/Interlocked.hpp, line 328, could you try to replace:

    static_assert (
        __atomic_always_lock_free (sizeof (dest), nullptr),
        "128-bit atomic operations are not lock-free"
    );
    old = comp;
    __atomic_compare_exchange_n (
        &dest, &old, excg,
        false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST
    );

with:

    old = __sync_val_compare_and_swap (&dest, comp, excg);

and tell me if it works better? Thanks

You could also try with clang: configure --enable-clang

marillat commented 2 years ago

This patch is correct ? If yes this doesn't build. See the build error after the patch ?

--- fmtconv-dmo-27.orig/src/conc/Interlocked.hpp
+++ fmtconv-dmo-27/src/conc/Interlocked.hpp
@@ -325,7 +325,7 @@ void        Interlocked::cas (Data128 &old, vol

 #elif defined (__GNUC__)

-       static_assert (
+       old = __sync_val_compare_and_swap (&dest, comp, excg);
                __atomic_always_lock_free (sizeof (dest), nullptr),
                "128-bit atomic operations are not lock-free"
        );

GCC output.

libtool: compile:  g++ -DPACKAGE_NAME=\"fmtconv\" -DPACKAGE_TARNAME=\"fmtconv\" -DPACKAGE_VERSION=\"r27\" "-DPACKAGE_STRING=\"fmtconv r27\"" "-DPACKAGE_BUGREPORT=\"http://forum.doom9.org/showthread.php?t=166504\"" "-DPACKAGE_URL=\"http://forum.doom9.org/showthread.php?t=166504\"" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DSTDC_HEADERS=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -I. -Wdate-time -D_FORTIFY_SOURCE=2 -std=c++14 -O3 -g3 -DNDEBUG -mfpmath=sse -msse2 -mcx16 -Wall -Wextra -Wshadow -Wunused -Wnull-dereference -Wvla -Wstrict-aliasing -Wuninitialized -Wunused-parameter -Wreorder -Wsign-compare -Wunreachable-code -Wconversion -Wno-sign-conversion -Wredundant-decls -Wno-ignored-attributes -Wno-expansion-to-defined -I./../../src -g -O2 -ffile-prefix-map=/home/marillat/src/fmtconv-27=. -fstack-protector-strong -Wformat -Werror=format-security -c ../../src/fmtcl/Dither.cpp  -fPIC -DPIC -o ../../src/fmtcl/.libs/Dither.o
In file included from ./../../src/conc/Interlocked.h:132,
                 from ./../../src/conc/AtomicPtr.hpp:27,
                 from ./../../src/conc/AtomicPtr.h:123,
                 from ./../../src/conc/CellPool.h:30,
                 from ./../../src/conc/ObjPool.h:44,
                 from ./../../src/fmtcl/Dither.h:26,
                 from ../../src/fmtcl/Dither.cpp:29:
./../../src/conc/Interlocked.hpp: In static member function 'static void conc::Interlocked::cas(conc::Interlocked::Data128&, volatile Data128&, const Data128&, const Data128&)':
./../../src/conc/Interlocked.hpp:329:43: warning: left operand of comma operator has no effect [-Wunused-value]
  329 |                 __atomic_always_lock_free (sizeof (dest), nullptr),
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
./../../src/conc/Interlocked.hpp:330:62: error: expected ';' before ')' token
  330 |                 "128-bit atomic operations are not lock-free"
      |                                                              ^
      |                                                              ;
  331 |         );
      |         ~                                                     
./../../src/conc/Interlocked.hpp:331:10: warning: right operand of comma operator has no effect [-Wunused-value]
  331 |         );
      |          ^
make: *** [Makefile:1779: ../../src/fmtcl/Dither.lo] Error 1
marillat commented 2 years ago

Well me be the patch is supposed to be this one ?

--- a/src/conc/Interlocked.hpp
+++ b/src/conc/Interlocked.hpp
@@ -325,15 +325,7 @@ void   Interlocked::cas (Data128 &old, vol

 #elif defined (__GNUC__)

-   static_assert (
-       __atomic_always_lock_free (sizeof (dest), nullptr),
-       "128-bit atomic operations are not lock-free"
-   );
-   old = comp;
-   __atomic_compare_exchange_n (
-       &dest, &old, excg,
-       false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST
-   );
+   old = __sync_val_compare_and_swap (&dest, comp, excg);

 #else
EleonoreMizo commented 2 years ago

Yes, the second one.

marillat commented 2 years ago

This fix this issue for amd64, arm64, armhf and i 86 arches but fail with armel for gcc and clang.

libtool: compile:  clang++ -DPACKAGE_NAME=\"fmtconv\" -DPACKAGE_TARNAME=\"fmtconv\" -DPACKAGE_VERSION=\"r27\" "-DPACKAGE_STRING=\"fmtconv r27\"" "-DPACKAGE_BUGREPORT=\"http://forum.doom9.org/showthread.php?t=166504\"" "-DPACKAGE_URL=\"http://forum.doom9.org/showthread.php?t=166504\"" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DSTDC_HEADERS=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -I. -Wdate-time -D_FORTIFY_SOURCE=2 -std=c++14 -O3 -g3 -DNDEBUG -ftree-vectorize -mfpu=neon -Wall -Wextra -Wshadow -Wunused -Wnull-dereference -Wvla -Wstrict-aliasing -Wuninitialized -Wunused-parameter -Wreorder -Wsign-compare -Wunreachable-code -Wconversion -Wno-sign-conversion -Wshadow-all -Wshorten-64-to-32 -Wint-conversion -Wconditional-uninitialized -Wconstant-conversion -Wunused-private-field -Wbool-conversion -Wextra-semi -Wnullable-to-nonnull-conversion -Wno-unused-private-field -Wno-unused-command-line-argument -I./../../src -g -O2 -ffile-prefix-map=/home/marillat/fmtconv-dmo-27=. -fstack-protector-strong -Wformat -Werror=format-security -c ../../src/fmtcl/Dither.cpp  -fPIC -DPIC -o ../../src/fmtcl/.libs/Dither.o
In file included from ../../src/fmtcl/Dither.cpp:29:
In file included from ./../../src/fmtcl/Dither.h:26:
In file included from ./../../src/conc/ObjPool.h:44:
In file included from ./../../src/conc/CellPool.h:33:
In file included from ./../../src/conc/LockFreeStack.h:45:
In file included from ./../../src/conc/AtomicPtrIntPair.h:40:
In file included from ./../../src/conc/Interlocked.h:132:
./../../src/conc/Interlocked.hpp:105:2: error: static_assert failed due to requirement '__atomic_always_lock_free(sizeof (dest), nullptr)' "32-bit atomic operations are not lock-free"
        static_assert (
        ^
./../../src/conc/Interlocked.hpp:223:2: error: static_assert failed due to requirement '__atomic_always_lock_free(sizeof (dest), nullptr)' "64-bit atomic operations are not lock-free"
        static_assert (
        ^
2 errors generated.
make: *** [Makefile:1779: ../../src/fmtcl/Dither.lo] Error 1
EleonoreMizo commented 2 years ago

Thank you so much for the testing. It seems armel architecture doesn’t have lock-free primitives, so I think I’ll just remove the static_assert() so the compiler will make use of the non-lock-free fallback. This will be sub-optimal but should work.

marillat commented 2 years ago

master commit 3a78392 fail to build for arm64

ERROR: compile ../../src/fmtcl/Dither.cpp on localhost failed
In file included from ./../../src/conc/Interlocked.h:132,
                 from ./../../src/conc/AtomicPtrIntPair.h:40,
                 from ./../../src/conc/LockFreeStack.h:45,
                 from ./../../src/conc/CellPool.h:33,
                 from ./../../src/conc/ObjPool.h:44,
                 from ./../../src/fmtcl/Dither.h:26,
                 from ../../src/fmtcl/Dither.cpp:29:
./../../src/conc/Interlocked.hpp: In static member function 'static void conc::Interlocked::cas(conc::Interlocked::Data128&, volatile Data128&, const Data128&, const Data128&)':
./../../src/conc/Interlocked.hpp:335:51: error: static assertion failed: 128-bit atomic operations are not lock-free
  335 |                         __atomic_always_lock_free (sizeof (dest), nullptr),
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
make: *** [Makefile:1805: ../../src/fmtcl/Dither.lo] Error 1
EleonoreMizo commented 2 years ago

The hell… Is 86d5b90bf81c3a36223910dcc0cbb0d88fc80a90 better?

marillat commented 2 years ago

Yes, build for all arches.