After the addition of ENABLE_UNIFIED_BUILDS, when trying to build JSC I get a lot of compilation errors due to missing #include's that are included transitively from other sources when bundled. Compiler: GCC 8.2.1 Revision: 239568
Created attachment 358181 [details] WIP Still needs some other fixes and a few #include's need to be reviewed.
Created attachment 358558 [details] Progress: only failing when linking testapi After this patch we can compile up to libJavaScriptCore.so but when linking the testapi executable it fails with many missing symbols (all from *Inline.h files). Yusuke, maybe you have some idea on why this is happening? Ps: does not happen with unified sources enabled. lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Wasm::SignatureInformation::singleton()' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::StructureRareData::setCachedOwnKeys(JSC::VM&, JSC::JSImmutableButterfly*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::StructureRareData::cachedOwnKeysIgnoringSentinel() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::StructureRareData::cachedOwnKeys() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::JSValue(JSC::JSValue::JSUndefinedTag)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::encode(JSC::JSValue)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::SlotVisitor::appendValuesHidden(JSC::WriteBarrierBase<JSC::Unknown, WTF::DumbValueTraits<JSC::Unknown> > con st*, unsigned long)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::isNumber() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::isInt32() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'void JSC::JIT::privateCompilePutByValWithCachedId<JSC::OpPutByValDirect>(JSC::ByValInfo*, JSC::ReturnAddressPtr, JSC::PutKind, JSC::Identifier const&)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'void JSC::JIT::privateCompilePutByValWithCachedId<JSC::OpPutByVal>(JSC::ByValInfo*, JSC::ReturnAddressPtr, JSC::P utKind, JSC::Identifier const&)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'void JSC::JIT::privateCompilePutByVal<JSC::OpPutByValDirect>(JSC::ByValInfo*, JSC::ReturnAddressPtr, JSC::JITArra yMode)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'void JSC::JIT::privateCompilePutByVal<JSC::OpPutByVal>(JSC::ByValInfo*, JSC::ReturnAddressPtr, JSC::JITArrayMode) ' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::ExecState::isStackOverflowFrame() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Strong<JSC::Unknown>::Strong(JSC::VM&, JSC::JSValue)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Strong<JSC::JSGlobalObject>::Strong(JSC::VM&, JSC::JSGlobalObject*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::IsoSubspace::allocateNonVirtual(JSC::VM&, unsigned long, JSC::GCDeferralContext*, JSC::AllocationFailureMode )' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::RegisterID* JSC::BytecodeGenerator::emitEqualityOp<JSC::OpLess>(JSC::RegisterID*, JSC::RegisterID*, JSC::Reg isterID*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::RegisterID* JSC::BytecodeGenerator::emitEqualityOp<JSC::OpEq>(JSC::RegisterID*, JSC::RegisterID*, JSC::Regis terID*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::UnlinkedMetadataTable::addEntry(JSC::OpcodeID)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSCell::structure(JSC::VM&) const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::isNull() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Structure::prototypeForLookup(JSC::JSGlobalObject*, JSC::JSCell*) const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::isEmpty() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::decode(long)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::HeapCell::vm() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::JSValue(JSC::JSCell*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'void JSC::SlotVisitor::append<JSC::CodeBlock, WTF::DumbPtrTraits<JSC::CodeBlock> >(JSC::WriteBarrierBase<JSC::CodeBlock, WTF::DumbPtrTraits<JSC::CodeBlock> > const&)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::SlotVisitor::appendUnbarriered(JSC::JSCell*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::SlotVisitor::vm()' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Heap::writeBarrier(JSC::JSCell const*, JSC::JSCell*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::asCell() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::JSValue::operator bool() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Heap::isMarked(void const*)' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Heap::mutatorFence()' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Heap::vm() const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Structure::prototypeForLookup(JSC::JSGlobalObject*) const' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<40u>(JSC::VM*, char const (&) [40u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<32u>(JSC::VM*, char const (&) [32u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<34u>(JSC::VM*, char const (&) [34u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<49u>(JSC::VM*, char const (&) [49u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<29u>(JSC::VM*, char const (&) [29u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<36u>(JSC::VM*, char const (&) [36u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<24u>(JSC::VM*, char const (&) [24u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<3u>(JSC::VM*, char const (&) [3u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<41u>(JSC::VM*, char const (&) [41u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<39u>(JSC::VM*, char const (&) [39u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<16u>(JSC::VM*, char const (&) [16u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<13u>(JSC::VM*, char const (&) [13u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<12u>(JSC::VM*, char const (&) [12u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<9u>(JSC::VM*, char const (&) [9u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<14u>(JSC::VM*, char const (&) [14u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<10u>(JSC::VM*, char const (&) [10u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<8u>(JSC::VM*, char const (&) [8u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<28u>(JSC::VM*, char const (&) [28u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<31u>(JSC::VM*, char const (&) [31u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<15u>(JSC::VM*, char const (&) [15u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<17u>(JSC::VM*, char const (&) [17u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<5u>(JSC::VM*, char const (&) [5u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<11u>(JSC::VM*, char const (&) [11u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<25u>(JSC::VM*, char const (&) [25u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<27u>(JSC::VM*, char const (&) [27u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<22u>(JSC::VM*, char const (&) [22u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<23u>(JSC::VM*, char const (&) [23u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<6u>(JSC::VM*, char const (&) [6u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<7u>(JSC::VM*, char const (&) [7u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<26u>(JSC::VM*, char const (&) [26u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<20u>(JSC::VM*, char const (&) [20u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<18u>(JSC::VM*, char const (&) [18u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<21u>(JSC::VM*, char const (&) [21u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<4u>(JSC::VM*, char const (&) [4u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<19u>(JSC::VM*, char const (&) [19u])' lib/libJavaScriptCore.so.1.0.0: error: undefined reference to 'JSC::Identifier JSC::Identifier::fromString<33u>(JSC::VM*, char const (&) [33u])' Source/JavaScriptCore/shell/CMakeFiles/testapi.dir/__/API/tests/testapi.cpp.o:testapi.cpp:function TestAPI::basicSymbol(): error: undefined reference to 'JSValueIsSymbol' Source/JavaScriptCore/shell/CMakeFiles/testapi.dir/__/API/tests/testapi.cpp.o:testapi.cpp:function WTF::SharedTaskFunctor<void (TestAPI&), testCAPIViaCpp::{lambda(TestAPI&)#2}> ::run(TestAPI&): error: undefined reference to 'JSValueIsSymbol'
This is the reason I recommended using 'inline' specifier (Bug 191626 Comment 13).
(In reply to Fujii Hironori from comment #3) > This is the reason I recommended using 'inline' specifier (Bug 191626 > Comment 13). Actually this is interesting. Can you post the discussion about it in webkit-dev mailing list? If we need to change the Coding Style Guide, I think webkit-dev is the best place to discuss it.
Got it. I'd like to create a proposal patch before having a discussion to make certain.
It turned out it isn't easy as I expected (Bug 193481). Please ignore my comments. Sorry for disturbing.
Looks like the linking errors are mostly due to the following: 1. <path>/a.h: templated method is declared 2. <path>/a.cpp: templated method is defined and called with template param Foo 3. <path>/b.cpp: templated method is called with template param Bar Which happens to work when unified sources are enabled because the implementation files are adjacent. Steph on our team noticed that technically one could add the following line after the method definition in a.cpp: > template ... SomeClass::someMethod<Bar>(...); But I'm assuming this would be less desirable than transplanting the definition to a more appropriate location. :P
Now, ALWAYS_INLINE seems to be of no use, but for something like... > LocalAllocator.cpp.obj : error LNK2019: unresolved external symbol "public: class JSC::HeapCell * __cdecl JSC::FreeList::allocate<class <lambda_8437a926922bdf66125377eabab8b83d> >(class <lambda_8437a926922bdf66125377eabab8b83d> const &)" (??$allocate@V<lambda_8437a926922bdf66125377eabab8b83d>@@@FreeList@JSC@@QEAAPEAVHeapCell@1@AEBV@@@Z) referenced in function "private: void * __cdecl JSC::LocalAllocator::tryAllocateIn(class JSC::MarkedBlock::Handle *)" (?tryAllocateIn@LocalAllocator@JSC@@AEAAPEAXPEAVHandle@MarkedBlock@2@@Z) ...we could just move the definition to the declaration site: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/heap/FreeListInlines.h#L33-L50 https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/heap/FreeList.h#L72-L73 But this gets kind of out of hand for something like... > JITOperations.cpp.obj : error LNK2019: unresolved external symbol "private: void __cdecl JSC::JIT::privateCompilePutByVal<struct JSC::OpPutByVal>(class JSC::ConcurrentJSLocker const &,struct JSC::ByValInfo *,class JSC::ReturnAddressPtr,enum JSC::JITArrayMode)" (??$privateCompilePutByVal@UOpPutByVal@JSC@@@JIT@JSC@@AEAAXAEBVConcurrentJSLocker@1@PEAUByValInfo@1@VReturnAddressPtr@1@W4JITArrayMode@1@@Z) referenced in function "public: static void __cdecl JSC::JIT::compilePutByVal(class JSC::ConcurrentJSLocker const &,class JSC::VM *,class JSC::CodeBlock *,struct JSC::ByValInfo *,class JSC::ReturnAddressPtr,enum JSC::JITArrayMode)" (?compilePutByVal@JIT@JSC@@SAXAEBVConcurrentJSLocker@2@PEAVVM@2@PEAVCodeBlock@2@PEAUByValInfo@2@VReturnAddressPtr@2@W4JITArrayMode@2@@Z) ...in which case moving privateCompilePutByVal from JITPropertyAccess.cpp to JIT.h leads to a pretty severe domino effect. :(
Created attachment 366870 [details] Working solution (sufficient for MSVC but not clang)
Attachment 366870 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/JSObjectRef.h:706: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
This should be a working solution. The first few changes described in the ChangeLog are likely unacceptable as-is, but hopefully this suffices to determine what we should actually do.
Comment on attachment 366870 [details] Working solution (sufficient for MSVC but not clang) View in context: https://bugs.webkit.org/attachment.cgi?id=366870&action=review > Source/JavaScriptCore/ChangeLog:11 > + testapi.cpp complains that it doesn't know about JSValueMakeSymbol / JSValueIsSymbol; > + moving their declarations evidently makes the problem go away. That's bizarre. Can you compile with -E in clang and see if they are there?
Comment on attachment 366870 [details] Working solution (sufficient for MSVC but not clang) Forgot to mention that the current patch is a working solution *for WinCairo*. I'm unaware of a way to disable unified builds via Xcode, but since JSCOnly is Cmake-based, we can do this on Mac: > Tools/Scripts/build-jsc --jsc-only --cmakeargs="-DENABLE_UNIFIED_BUILDS=OFF" As it turns out, the Mac situation is far more dire, with another 50+ "missing" symbols defined in *Inlines.h. 😰
Hmm, apparently even WinCairo behaves similarly (44 additional unresolved externals) when building with clang-cl. (Funny that MSVC is easier to please in this scenario, though that may not actually be a good thing.) --- (In reply to Keith Miller from comment #12) > That's bizarre. Can you compile with -E in clang and see if they are there? I can look into this, though it's slightly tricky as that particular issue only appeared after I addressed all of the other unresolved externals.
Another interesting point (which I didn't see earlier because MSVC is not as kind): > ..\..\Source\JavaScriptCore\heap/FreeList.h(73,15): error: function 'JSC::FreeList::allocate<(lambda at ..\..\Source\JavaScriptCore\heap\LocalAllocator.cpp:235:9)>' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage > HeapCell* allocate(const Func& slowPath); > ^ > ..\..\Source\JavaScriptCore\heap\LocalAllocator.cpp(234,31): note: used here > void* result = m_freeList.allocate( > ^ This particular case is an outright clang error since it's definitionally broken (as it's impossible to "prepare" an implementation when the template parameter is a lambda). So I guess the overall task is what to do about the dozens of other cases that aren't *definitionally* broken but have the exact same translation unit spillage problem.
(In reply to Ross Kirsling from comment #14) > (In reply to Keith Miller from comment #12) > > > Source/JavaScriptCore/ChangeLog:11 > > > + testapi.cpp complains that it doesn't know about JSValueMakeSymbol / JSValueIsSymbol; > > > + moving their declarations evidently makes the problem go away. > > > > That's bizarre. Can you compile with -E in clang and see if they are there? > > I can look into this, though it's slightly tricky as that particular issue > only appeared after I addressed all of the other unresolved externals. Hmm, this case is probably the same idea as the others after all. For instance, if the linker can't see this method definition: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/IdentifierInlines.h#L96-L100 Then we *could* move it to the declaration (as I've done in the current patch): https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/Identifier.h#L113-L114 But it also suffices just to move it under the class declaration: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/Identifier.h#L205 And so in both cases, the problem occurs when the method definition is not in the header where the declaration occurs, but rather in a second header that includes the first. Refocusing on Identifier::fromString then, if I compile Identifier.cpp and BuiltinNames.cpp with -E as you suggested, sure enough, Identifier.cpp.o contains the source of IdentifierInlines.h but BuiltinNames.cpp.o does not.
Created attachment 366897 [details] Patch
So I guess the lesson learned here is "when you forget to include *Inlines.h, you get linking errors and not compiler errors". The two legitimately tricky cases are of the type I initially mentioned (template method defined in a.cpp but consumed in b.cpp), but the rest all turned out to be missing includes after all.
Created attachment 366909 [details] Patch
Somehow, the previous patch was sufficient for Mac (JSCOnly) but not enough to please WinCairo clang-cl. This one should cover all the bases.
Created attachment 366946 [details] Patch
Rebased since bug 196647 was landed without noticing this one.
Comment on attachment 366946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366946&action=review Can we have a way to avoid "JSCInlines.h" repeatedly? It means that one of the header in JSCInlines.h is modified, so many C++ source code will be recompiled :( > Source/JavaScriptCore/b3/B3OptimizeAssociativeExpressionTrees.cpp:34 > #include "B3InsertionSet.h" When we include XXXInlines.h, we omit XXX.h include. > Source/JavaScriptCore/runtime/Operations.h:27 > #include "JSCJSValue.h" Ditto.
(In reply to Yusuke Suzuki from comment #23) > Comment on attachment 366946 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=366946&action=review > > Can we have a way to avoid "JSCInlines.h" repeatedly? It means that one of > the header in JSCInlines.h is modified, so many C++ source code will be > recompiled :( That's a great question for which I don't have an answer, but I noticed that JSCInlines.h itself claims that > It's good style to make sure that every .cpp file includes JSCInlines.h. (https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSCInlines.h#L28-L30) Now, when a .cpp file doesn't actually use the symbol that its .cpp.o is missing, I tried to include the *Inlines.h from the header that does use the symbol (most notably, by including JSCJSValueInlines.h from Operations.h, I avoided having to add includes to about 20 DFG*.cpp files), but if this is the "wrong way" then please let me know. :)
Two further points: - Since I interpreted the quote above as a JSC best practice, I intentionally included JSCInlines.h instead of listing more specific *Inlines.h includes in .cpp files. Happy to reverse this approach if it's preferred. - Importantly, I avoided including JSCInlines.h in .h files and just included the more specific *Inlines.h instead (such as when including JSCJSValueInlines.h from Operations.h as mentioned above).
Created attachment 367011 [details] Patch
Comment on attachment 367011 [details] Patch r=me.
Comment on attachment 367011 [details] Patch Clearing flags on attachment: 367011 Committed r244088: <https://trac.webkit.org/changeset/244088>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49743722>