Bug 190208 – clang Linux build cannot link because of __builtin_mul_overflow

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Olivier Blin]

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

[Olivier Blin]

Created attachment 351411 [details]
Patch

[Michael Catanzaro]

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.

[Mark Lam]

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).

[Loïc Yhuel]

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

[Olivier Blin]

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.

[Pablo Saavedra]

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.

[EWS]

Committed r272140: <https://trac.webkit.org/changeset/272140>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 351411 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73825378>