Bug 193073

Summary: JSC should build successfully even with -DENABLE_UNIFIED_BUILDS=OFF
Product: WebKit Reporter: Carlos Bentzen <cadubentzen>
Component: JavaScriptCoreAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: cadubentzen, commit-queue, don.olmstead, ews-watchlist, hi, Hironori.Fujii, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, saam, stephan.szabo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193481
Attachments:
Description Flags
WIP
none
Progress: only failing when linking testapi
none
Working solution (sufficient for MSVC but not clang)
none
Patch
none
Patch
none
Patch
none
Patch none

Description Carlos Bentzen 2019-01-02 05:32:07 PST 
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
Comment 1 Carlos Bentzen 2019-01-02 05:34:20 PST 
Created attachment 358181 [details]
WIP

Still needs some other fixes and a few #include's need to be reviewed.
Comment 2 Carlos Bentzen 2019-01-07 17:05:38 PST 
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'
Comment 3 Fujii Hironori 2019-01-07 17:13:57 PST 
This is the reason I recommended using 'inline' specifier (Bug 191626 Comment 13).
Comment 4 Yusuke Suzuki 2019-01-10 11:11:54 PST 
(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.
Comment 5 Fujii Hironori 2019-01-10 18:36:24 PST 
Got it. I'd like to create a proposal patch before having a discussion to make certain.
Comment 6 Fujii Hironori 2019-01-15 19:25:06 PST 
It turned out it isn't easy as I expected (Bug 193481). Please ignore my comments. Sorry for disturbing.
Comment 7 Ross Kirsling 2019-04-05 15:54:05 PDT 
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
Comment 8 Ross Kirsling 2019-04-05 16:27:06 PDT 
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. :(
Comment 9 Ross Kirsling 2019-04-05 20:34:42 PDT 
Created attachment 366870 [details]
Working solution (sufficient for MSVC but not clang)
Comment 10 EWS Watchlist 2019-04-05 20:37:06 PDT Comment hidden (obsolete)
Comment 11 Ross Kirsling 2019-04-05 20:38:16 PDT 
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 12 Keith Miller 2019-04-06 10:05:25 PDT 
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 13 Ross Kirsling 2019-04-06 11:26:37 PDT 
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. 😰
Comment 14 Ross Kirsling 2019-04-06 12:39:26 PDT 
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.
Comment 15 Ross Kirsling 2019-04-06 12:52:48 PDT 
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.
Comment 16 Ross Kirsling 2019-04-06 14:57:03 PDT 
(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.
Comment 17 Ross Kirsling 2019-04-06 19:15:02 PDT 
Created attachment 366897 [details]
Patch
Comment 18 Ross Kirsling 2019-04-06 19:24:05 PDT 
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.
Comment 19 Ross Kirsling 2019-04-07 16:16:35 PDT 
Created attachment 366909 [details]
Patch
Comment 20 Ross Kirsling 2019-04-07 16:20:28 PDT 
Somehow, the previous patch was sufficient for Mac (JSCOnly) but not enough to please WinCairo clang-cl.

This one should cover all the bases.
Comment 21 Ross Kirsling 2019-04-08 10:15:11 PDT 
Created attachment 366946 [details]
Patch
Comment 22 Ross Kirsling 2019-04-08 10:17:31 PDT 
Rebased since bug 196647 was landed without noticing this one.
Comment 23 Yusuke Suzuki 2019-04-08 17:16:34 PDT 
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.
Comment 24 Ross Kirsling 2019-04-08 17:36:26 PDT 
(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. :)
Comment 25 Ross Kirsling 2019-04-08 19:08:01 PDT 
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).
Comment 26 Ross Kirsling 2019-04-08 19:11:11 PDT 
Created attachment 367011 [details]
Patch
Comment 27 Keith Miller 2019-04-09 11:21:31 PDT 
Comment on attachment 367011 [details]
Patch

r=me.
Comment 28 WebKit Commit Bot 2019-04-09 11:50:06 PDT 
Comment on attachment 367011 [details]
Patch

Clearing flags on attachment: 367011

Committed r244088: <https://trac.webkit.org/changeset/244088>
Comment 29 WebKit Commit Bot 2019-04-09 11:50:08 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2019-04-09 11:51:45 PDT 
<rdar://problem/49743722>