Bug 180637

Summary: undefined reference to 'JSC::B3::BasicBlock::fallThrough() const
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: JavaScriptCoreAssignee: Alejandro G. Castro <alex>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bugs-noreply, commit-queue, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=179914
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Michael Catanzaro
Reported 2017-12-10 13:15:59 PST
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.
Attachments
Patch (1.49 KB, patch)
2018-01-24 00:55 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
Patch (1.53 KB, patch)
2018-01-24 01:17 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
Patch (1.44 KB, patch)
2018-01-24 08:08 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2017-12-11 08:16:59 PST
(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.
Michael Catanzaro
Comment 2 2018-01-22 05:54:44 PST
Probably fixed by r226971.
Michael Catanzaro
Comment 3 2018-01-22 05:57:02 PST
(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.
Alejandro G. Castro
Comment 4 2018-01-23 07:33:44 PST
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?
Alejandro G. Castro
Comment 5 2018-01-24 00:55:12 PST
Created attachment 332130 [details] Patch
Alejandro G. Castro
Comment 6 2018-01-24 01:17:44 PST
Created attachment 332135 [details] Patch
Alejandro G. Castro
Comment 7 2018-01-24 01:27:46 PST
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.
Michael Catanzaro
Comment 8 2018-01-24 07:06:22 PST
Comment on attachment 332135 [details] 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....
Mark Lam
Comment 9 2018-01-24 07:24:55 PST
Comment on attachment 332135 [details] 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.
Alejandro G. Castro
Comment 10 2018-01-24 08:04:05 PST
(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.
Alejandro G. Castro
Comment 11 2018-01-24 08:08:05 PST
Created attachment 332166 [details] Patch
Michael Catanzaro
Comment 12 2018-01-24 17:56:22 PST
Comment on attachment 332166 [details] Patch OK, this is what Mark wanted.
WebKit Commit Bot
Comment 13 2018-01-25 01:06:33 PST
Comment on attachment 332166 [details] Patch Clearing flags on attachment: 332166 Committed r227597: <https://trac.webkit.org/changeset/227597>
WebKit Commit Bot
Comment 14 2018-01-25 01:06:35 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2018-01-25 01:07:28 PST
<rdar://problem/36858899>
Note You need to log in before you can comment on or make changes to this bug.