180637 2017-12-10 13:15:59 -0800 undefined reference to 'JSC::B3::BasicBlock::fallThrough() const 2018-01-25 01:07:28 -0800 1 1 1 Unclassified WebKit JavaScriptCore Other PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=179914 InRadar P2 Normal --- 1 mcatanzaro alex alex bugs-noreply commit-queue ews-watchlist keith_miller mark.lam mcatanzaro msaboff saam webkit-bug-importer zan oldest_to_newest 1380086 0 mcatanzaro 2017-12-10 13:15:59 -0800 This build failure occurs in debug builds with the following compiler flags set: os.environ['CFLAGS'] = '-m64 -mtune=generic -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1' os.environ['CXXFLAGS'] = '-m64 -mtune=generic -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1' os.environ['LDFLAGS'] = '-Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld' $ cat /usr/lib/rpm/redhat/redhat-hardened-cc1 *cc1_options: + %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}} $ cat /usr/lib/rpm/redhat/redhat-hardened-ld *self_spec: + %{!static:%{!shared:%{!r:-pie}}} These compiler flags are commonly-used in Linux distros. However, I'm not sure which particular flag is to blame for the failure. Our test bots do not set these flags and are all happy. The failure: [609/2808] Linking CXX executable bin/testair FAILED: bin/testair : && /usr/lib64/ccache/c++ -fdiagnostics-color=always -Wno-expansion-to-defined -Wno-noexcept-type -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wextra -Wall -m64 -mtune=generic -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fno-strict-aliasing -fno-exceptions -std=c++14 -fno-rtti -gsplit-dwarf -g -L/home/mcatanzaro/Projects/GNOME/install/lib -Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -fuse-ld=gold -Wl,--disable-new-dtags -Wl,--gdb-index -rdynamic Source/JavaScriptCore/shell/CMakeFiles/testair.dir/__/b3/air/testair.cpp.o -o bin/testair -Wl,-rpath,/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/lib -ldl lib/libjavascriptcoregtk-4.0.so.18.7.1 lib/libWTFGTK.a -lglib-2.0 lib/libbmalloc.a -ldl -licudata -licuuc -lpthread -lgio-2.0 -lgobject-2.0 -lz -licui18n -lglib-2.0 && : lib/libjavascriptcoregtk-4.0.so.18.7.1: error: undefined reference to 'JSC::B3::BasicBlock::fallThrough() const' collect2: error: ld returned 1 exit status Removing the inline keyword and moving the functions to B3BasicBlock.cpp solves the problem. Of course, symbols required from outside JSC cannot be marked inline. But it's not clear to me why the fallThrough symbols are being required for testair, and why that is only the case with the above compiler flags. 1380205 1 mcatanzaro 2017-12-11 08:16:59 -0800 (In reply to Michael Catanzaro from comment #0) > This build failure occurs in debug builds with the following compiler flags > set: It's probably not related to compiler flags at all, but to DEVELOPER_MODE. Likely the same as bug #179914. 1391614 2 mcatanzaro 2018-01-22 05:54:44 -0800 Probably fixed by r226971. 1391616 3 mcatanzaro 2018-01-22 05:57:02 -0800 (In reply to Michael Catanzaro from comment #0) > This build failure occurs in debug builds with the following compiler flags > set Also: I think this occurred only in debug builds without DEVELOPER_MODE set. Those probably can't be expected to work, anyway. 1392057 4 alex 2018-01-23 07:33:44 -0800 I can reproduce it, the problem seems to be B3SwitchValue.cpp: ... BasicBlock* SwitchValue::fallThrough(const BasicBlock* owner) { ASSERT(hasFallThrough()); BasicBlock* fallThrough = owner->successor(owner->numSuccessors() - 1).block(); ASSERT(fallThrough == owner->fallThrough().block()); return fallThrough; } ... The ASSERT is just compiled for Debug, that is why it just happens in Debug, if we use RELEASE_ASSERT it also happens on Release. In the ASSERT we use the fallThrough function that is just declared in the B3BasicBlock.h header without the inline, so the compiler seems to leave the symbol undefined when compiling the UnifiedSource and be happy about it. But when merging the Unified object bundles it does not add the inline definition found later, and when using the library to link the binaries the symbol is still undefined because de implementation was inline. Adding the include to that file solves the problem, or adding the include to that unified bundle manually. I'm adding Keith to check the proper solution to this in case it is a known limitation of the unified compilation, it seems a problem when declaring a function not inline and implementing it later inline? 1392472 5 332130 alex 2018-01-24 00:55:12 -0800 Created attachment 332130 Patch 1392479 6 332135 alex 2018-01-24 01:17:44 -0800 Created attachment 332135 Patch 1392487 7 alex 2018-01-24 01:27:46 -0800 It is the -O2, you can see Michael compilation in the first comment, that makes the inlines real inlines, if you compile Debug with O0 it should not happen because the compiler does not inline the functions. And in release it does not happen because it is an ASSERT, if you use RELEASE_ASSERT everyone should be able to reproduce it becuase Release does use -O2 commonly. In my case and Michaels the compiler package decides to use O2 also in Debug, JFTR it is Fedora in my case. 1392541 8 332135 mcatanzaro 2018-01-24 07:06:22 -0800 Comment on attachment 332135 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332135&action=review > Source/JavaScriptCore/b3/B3SwitchValue.cpp:32 > +#include "B3BasicBlockInlines.h" I agree that this patch is right. I guess the theory behind splitting *Inlines.h into separate files is that .cpp files that don't use the inline functions don't need to #include them? That is, it's just to speed up the build a little bit? This seems like a bad idea.... 1392549 9 332135 mark.lam 2018-01-24 07:24:55 -0800 Comment on attachment 332135 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332135&action=review >> Source/JavaScriptCore/b3/B3SwitchValue.cpp:32 >> +#include "B3BasicBlockInlines.h" > > I agree that this patch is right. > > I guess the theory behind splitting *Inlines.h into separate files is that .cpp files that don't use the inline functions don't need to #include them? That is, it's just to speed up the build a little bit? This seems like a bad idea.... This fix is incorrect. The proper fix is to replace the #include of B3BasicBlock.h with B3BasicBlockInlines.h. The reason for putting inline functions in an Inlines.h file is not primarily for speeding up compilation, but rather to workaround circular dependencies. As a general rule, header files should never #include Inlines.h files. Only .cpp files should #include them. There may be 1 or 2 exception to this (eg in JSCInlines.h), but those are special cases for specific reasons. 1392574 10 alex 2018-01-24 08:04:05 -0800 (In reply to Mark Lam from comment #9) > This fix is incorrect. The proper fix is to replace the #include of > B3BasicBlock.h with B3BasicBlockInlines.h. > > The reason for putting inline functions in an Inlines.h file is not > primarily for speeding up compilation, but rather to workaround circular > dependencies. As a general rule, header files should never #include > Inlines.h files. Only .cpp files should #include them. There may be 1 or 2 > exception to this (eg in JSCInlines.h), but those are special cases for > specific reasons. Makes sense, thanks for the comment Mark, I'll rework the patch. 1392576 11 332166 alex 2018-01-24 08:08:05 -0800 Created attachment 332166 Patch 1392867 12 332166 mcatanzaro 2018-01-24 17:56:22 -0800 Comment on attachment 332166 Patch OK, this is what Mark wanted. 1392951 13 332166 commit-queue 2018-01-25 01:06:33 -0800 Comment on attachment 332166 Patch Clearing flags on attachment: 332166 Committed r227597: <https://trac.webkit.org/changeset/227597> 1392952 14 commit-queue 2018-01-25 01:06:35 -0800 All reviewed patches have been landed. Closing bug. 1392954 15 webkit-bug-importer 2018-01-25 01:07:28 -0800 <rdar://problem/36858899> 332130 2018-01-24 00:55:12 -0800 2018-01-24 01:17:40 -0800 Patch bug-180637-20180124095511.patch text/plain 1526 alex U3VidmVyc2lvbiBSZXZpc2lvbjogMjI3Mjg3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBm NTk0NWRhZGNhY2VkMDM4MmM2ZjAxYzg1YWNhYWMwNWM5ZWE2ZDNkLi43NWJlMGUwMWQxYTIxZWZl N2RiZDU1ZjExZGJjYTAyOTBkMGJiOGFmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxOC0wMS0yNCAgQWxlamFuZHJvIEcuIENhc3RybyAgPGFsZXhAaWdhbGlh LmNvbT4KKworICAgICAgICB1bmRlZmluZWQgcmVmZXJlbmNlIHRvICdKU0M6OkIzOjpCYXNpY0Js b2NrOjpmYWxsVGhyb3VnaCgpIGNvbnN0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xODA2MzcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBXZSBuZWVkIHRvIG1ha2Ugc3VyZSB0aGUgaW1wbGVtZW50YXRpb24g b2YgdGhlIGlubGluZSBmdW5jdGlvbnMgaXMKKyAgICAgICAgY29tcGlsZWQgd2hlbiB3ZSBjb21w aWxlIHRoZSBjb2RlIHVzaW5nIHRoZSBmdW5jdGlvbiwgbm93IHRoYXQgdGhlCisgICAgICAgIGNv bXBpbGF0aW9uIGlzIGRpdmlkZWQsIG9yIHdlIGNvdWxkIGVuZCB1cCB3aXRoIHVuZGVmaW5lZCBz eW1ib2xzCisgICAgICAgIHdoZW4gdGhlIGRlY2xhcmF0aW9uIGlzIG5vdCBpbmxpbmVkLCBhdCBs ZWFzdCB3aXRoIHNvbWUgY29tcGlsZXJzLgorCisgICAgICAgICogYjMvQjNTd2l0Y2hWYWx1ZS5j cHA6IEFkZGVkIHRoZSBpbmNsdWRlIGZvciB0aGUgaW5saW5lIGZ1bmN0aW9ucyBpbXBsZW1lbnRh dGlvbi4KKwogMjAxOC0wMS0yMSAgV2Vuc29uIEhzaWVoICA8d2Vuc29uX2hzaWVoQGFwcGxlLmNv bT4KIAogICAgICAgICBBZGQgYSBuZXcgZmVhdHVyZSBmbGFnIGZvciBFWFRSQV9aT09NX01PREUg YW5kIHJlaW50cm9kdWNlIEFkZGl0aW9uYWxGZWF0dXJlRGVmaW5lcy5oCmRpZmYgLS1naXQgYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvYjMvQjNTd2l0Y2hWYWx1ZS5jcHAgYi9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvYjMvQjNTd2l0Y2hWYWx1ZS5jcHAKaW5kZXggNTVhNTVlOWQwNzU4NGUzZDM0ZjVj N2RkNmRlZWE5NzY2YWUwNGY2Zi4uZjQ2YjYxODg2NTIwZDA4NGFlNmE4ODI3MmE2ZGY1Y2IxZjIz NDRiMiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2IzL0IzU3dpdGNoVmFsdWUu Y3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9CM1N3aXRjaFZhbHVlLmNwcApAQCAt MjksNiArMjksNyBAQAogI2lmIEVOQUJMRShCM19KSVQpCiAKICNpbmNsdWRlICJCM0Jhc2ljQmxv Y2suaCIKKyNpbmNsdWRlICJCM0Jhc2ljQmxvY2tJbmxpbmVzLmgiCiAjaW5jbHVkZSA8d3RmL0xp c3REdW1wLmg+CiAKIG5hbWVzcGFjZSBKU0MgeyBuYW1lc3BhY2UgQjMgewo= 332135 2018-01-24 01:17:44 -0800 2018-01-24 08:08:01 -0800 Patch bug-180637-20180124101743.patch text/plain 1565 alex U3VidmVyc2lvbiBSZXZpc2lvbjogMjI3Mjg3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBm NTk0NWRhZGNhY2VkMDM4MmM2ZjAxYzg1YWNhYWMwNWM5ZWE2ZDNkLi5kMmY4MWU0MmE5NDI5YTlh ZjljY2NmZmM1Y2FmNTcyYjI3MDU1NGEwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxOC0wMS0yNCAgQWxlamFuZHJvIEcuIENhc3RybyAgPGFsZXhAaWdhbGlh LmNvbT4KKworICAgICAgICB1bmRlZmluZWQgcmVmZXJlbmNlIHRvICdKU0M6OkIzOjpCYXNpY0Js b2NrOjpmYWxsVGhyb3VnaCgpIGNvbnN0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xODA2MzcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBXZSBuZWVkIHRvIG1ha2Ugc3VyZSB0aGUgaW1wbGVtZW50YXRpb24g b2YgdGhlIGlubGluZSBmdW5jdGlvbnMgaXMKKyAgICAgICAgY29tcGlsZWQgd2hlbiB3ZSBjb21w aWxlIHRoZSBjb2RlIHVzaW5nIHRoZSBmdW5jdGlvbiwgbm93IHRoYXQgdGhlCisgICAgICAgIGNv bXBpbGF0aW9uIGlzIGRpdmlkZWQsIG9yIHdlIGNvdWxkIGVuZCB1cCB3aXRoIHVuZGVmaW5lZCBz eW1ib2xzCisgICAgICAgIHdoZW4gdGhlIGRlY2xhcmF0aW9uIGlzIG5vdCBpbmxpbmVkLCBhdCBs ZWFzdCB3aXRoIHNvbWUgY29tcGlsZXJzCisgICAgICAgIGFuZCBvcHRpbWl6YXRpb25zIGVuYWJs ZWQgLU8yLgorCisgICAgICAgICogYjMvQjNTd2l0Y2hWYWx1ZS5jcHA6IEFkZGVkIHRoZSBpbmNs dWRlIGZvciB0aGUgaW5saW5lIGZ1bmN0aW9ucyBpbXBsZW1lbnRhdGlvbi4KKwogMjAxOC0wMS0y MSAgV2Vuc29uIEhzaWVoICA8d2Vuc29uX2hzaWVoQGFwcGxlLmNvbT4KIAogICAgICAgICBBZGQg YSBuZXcgZmVhdHVyZSBmbGFnIGZvciBFWFRSQV9aT09NX01PREUgYW5kIHJlaW50cm9kdWNlIEFk ZGl0aW9uYWxGZWF0dXJlRGVmaW5lcy5oCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYjMvQjNTd2l0Y2hWYWx1ZS5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYjMvQjNTd2l0 Y2hWYWx1ZS5jcHAKaW5kZXggNTVhNTVlOWQwNzU4NGUzZDM0ZjVjN2RkNmRlZWE5NzY2YWUwNGY2 Zi4uZjQ2YjYxODg2NTIwZDA4NGFlNmE4ODI3MmE2ZGY1Y2IxZjIzNDRiMiAxMDA2NDQKLS0tIGEv U291cmNlL0phdmFTY3JpcHRDb3JlL2IzL0IzU3dpdGNoVmFsdWUuY3BwCisrKyBiL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9iMy9CM1N3aXRjaFZhbHVlLmNwcApAQCAtMjksNiArMjksNyBAQAogI2lm IEVOQUJMRShCM19KSVQpCiAKICNpbmNsdWRlICJCM0Jhc2ljQmxvY2suaCIKKyNpbmNsdWRlICJC M0Jhc2ljQmxvY2tJbmxpbmVzLmgiCiAjaW5jbHVkZSA8d3RmL0xpc3REdW1wLmg+CiAKIG5hbWVz cGFjZSBKU0MgeyBuYW1lc3BhY2UgQjMgewo= 332166 2018-01-24 08:08:05 -0800 2018-01-25 01:06:33 -0800 Patch bug-180637-20180124170803.patch text/plain 1477 alex U3VidmVyc2lvbiBSZXZpc2lvbjogMjI3NDgzCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj NzYwYjAyMGIxYjY2NTFkMTZiOGQzYjQ0MGVhOTU3N2ZhNzgyZmYyLi5kMDIyOWRlZjA3ZGVlMTRl NGUwYTI5YTVjMjQwOGQ5YmNmNjYwNmZiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxOC0wMS0yNCAgQWxlamFuZHJvIEcuIENhc3RybyAgPGFsZXhAaWdhbGlh LmNvbT4KKworICAgICAgICB1bmRlZmluZWQgcmVmZXJlbmNlIHRvICdKU0M6OkIzOjpCYXNpY0Js b2NrOjpmYWxsVGhyb3VnaCgpIGNvbnN0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xODA2MzcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBXZSBuZWVkIHRvIG1ha2Ugc3VyZSB0aGUgaW1wbGVtZW50YXRpb24g b2YgdGhlIGlubGluZSBmdW5jdGlvbnMgaXMKKyAgICAgICAgY29tcGlsZWQgd2hlbiB3ZSBjb21w aWxlIHRoZSBjb2RlIHVzaW5nIHRoZSBmdW5jdGlvbiwgbm93IHRoYXQgdGhlCisgICAgICAgIGNv bXBpbGF0aW9uIGlzIGRpdmlkZWQsIG9yIHdlIGNvdWxkIGVuZCB1cCB3aXRoIHVuZGVmaW5lZCBz eW1ib2xzCisgICAgICAgIHdoZW4gdGhlIGRlY2xhcmF0aW9uIGlzIG5vdCBpbmxpbmVkLCBhdCBs ZWFzdCB3aXRoIHNvbWUgY29tcGlsZXJzCisgICAgICAgIGFuZCBvcHRpbWl6YXRpb25zIGVuYWJs ZWQgLU8yLgorCisgICAgICAgICogYjMvQjNTd2l0Y2hWYWx1ZS5jcHA6IHJlcGxhY2UgdGhlIGlu Y2x1ZGUuCisKIDIwMTgtMDEtMjMgIFl1c3VrZSBTdXp1a2kgIDx1dGF0YW5lLnRlYUBnbWFpbC5j b20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgc3VwcHJlc3MgR0NDIHdhcm5pbmdzCmRpZmYgLS1n aXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYjMvQjNTd2l0Y2hWYWx1ZS5jcHAgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvYjMvQjNTd2l0Y2hWYWx1ZS5jcHAKaW5kZXggNTVhNTVlOWQwNzU4NGUz ZDM0ZjVjN2RkNmRlZWE5NzY2YWUwNGY2Zi4uZjg3MDliNDM4NjVjOTc3NGY0YTg4YTIzMDViNTA1 OTI0ZjUwYmExYiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2IzL0IzU3dpdGNo VmFsdWUuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9CM1N3aXRjaFZhbHVlLmNw cApAQCAtMjgsNyArMjgsNyBAQAogCiAjaWYgRU5BQkxFKEIzX0pJVCkKIAotI2luY2x1ZGUgIkIz QmFzaWNCbG9jay5oIgorI2luY2x1ZGUgIkIzQmFzaWNCbG9ja0lubGluZXMuaCIKICNpbmNsdWRl IDx3dGYvTGlzdER1bXAuaD4KIAogbmFtZXNwYWNlIEpTQyB7IG5hbWVzcGFjZSBCMyB7Cg==