Since r183319, __builtin_mul_overflow is used with gcc or clang in WTF/wtf/CheckedArithmetic.h This leads to a link failure when WebKit is built on Linux with clang and the libgcc runtime: BINXX WPELauncher output/staging/usr/lib/libWPEWebKit-0.1.so: undefined reference to `__mulodi4' collect2: error: ld returned 1 exit status This is because clang generates code using the __mulodi4 symbol for __builtin_mul_overflow. But this symbol is available only in compiler-rt, and not in the libgcc runtime used by most Linux distributions of clang. See also this upstream clang bug: https://bugs.llvm.org/show_bug.cgi?id=28629
Created attachment 351411 [details] Patch
Comment on attachment 351411 [details] Patch Hm, not sure what I think about having a USE() that is local to a single file. Let's see what other reviewers think. Use #undef at the bottom of the header, at least.
Comment on attachment 351411 [details] Patch r=me I saw Michael's concern but I think it's OK here because no one else needs it except for this code. I agree that it is good form to #undef it at the bottom of the file (especially because we build with unified sources).
I compiled a simple C source file (outside WebKit) using __builtin_mul_overflow with 32 or 64 bit arguments. arm-linux-gnu-gcc 8.1.1 : - 32 bit arguments : optimized code using smull - 64 bit arguments : complex code aarch64-linux-gnu-gcc 8.1.1 : - 32 bit arguments : optimized code using smull, similar to ARM Clang 7.0 ARM : - 32 bit arguments : optimized code, but using mul + smmul (might be slower than gcc) - 64 bit arguments : call __mulodi4 Clang 7.0 AArch 64 : - 32 bit arguments : optimized code, using smull - 64 bit arguments : optimized code using mul + smulh gcc x86 (-m32) : - 32 bit arguments : optimized code, using imul and testing overflow flag - 64 bit arguments : complex code gcc x86_64 : - 32 bit arguments : optimized code, using imul and testing overflow flag - 64 bit arguments : optimized code, using imul and testing overflow flag Clang 7.0 x86 (-m32) : - 32 bit arguments : optimized code, using imul and testing overflow flag - 64 bit arguments : call __mulodi4 Clang 7.0 x86_64 : - 32 bit arguments : optimized code, using imul and testing overflow flag - 64 bit arguments : optimized code, using imul and testing overflow flag tl,dr : - for 32 bit arguments, Clang should be OK (even if the ARM implementation is perhaps worse than gcc, it should be better than the C) - for 64 bit arguments, Clang cannot produce the code so calls __mulodi4 on 32-bit architectures So : - we probably need to keep using the builtin with Clang on 32-bit architectures for the 1st and 2nd case if the arguments are 32-bit (so we need more template specializations) - we need to check if the compilers are able to use the 32-bit builtin for the 3rd case (int, unsigned specialization) in the code
Created attachment 351497 [details] disassembled MediaTime.o on ARMv7 Here is the disassembled MediaTime.o from my ARMv7 build with clang. It is the only user of the __mulodi4 symbol in my release build of WPE 2.22. TiledBackingStore::adjustForContentsRect() also uses WTF::safeMultiply(), but does not generate a call to __mulodi4.
Is there any chance of merging this patch? I just noted that this problem is still there compiling for Clang 5/7 and 9 for ARMv7 targets. The patch proposed it seems to fix the problem so could be great to have it integrated in master.
Committed r272140: <https://trac.webkit.org/changeset/272140> All reviewed patches have been landed. Closing bug and clearing flags on attachment 351411 [details].
<rdar://problem/73825378>