Summary: | clang Linux build cannot link because of __builtin_mul_overflow | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Olivier Blin <olivier.blin> | ||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, dbates, ews-watchlist, guijemont, loic.yhuel, mark.lam, mcatanzaro, psaavedra, sam, webkit-bug-importer, ysuzuki, zan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Olivier Blin
2018-10-02 10:14:36 PDT
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]. |