Bug 124761 – Combine SymbolTable and SharedSymbolTable

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 217651 [details]
the patch

[Filip Pizlo]

Created attachment 217652 [details]
the patch

[EFL EWS Bot]

Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/33728010

[Build Bot]

Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/33608018

[Build Bot]

Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/33718011

[kov's GTK+ EWS bot]

Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/33618006

[EFL EWS Bot]

Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/33948001

[Build Bot]

Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/33778006

[Geoffrey Garen]

Comment on attachment 217652 [details]
the patch

Approach looks good to me, but build is broken:

Undefined symbols for architecture x86_64:
  "__ZNK3JSC7JSValuecvPMS0_PvEv", referenced from:
      __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
      __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o
  "__ZNK3JSC7JSValue6asCellEv", referenced from:
      __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
      __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o

Maybe you need to #include JSValueInlines.h?

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 217652 [details])
> Approach looks good to me, but build is broken:
> 
> Undefined symbols for architecture x86_64:
>   "__ZNK3JSC7JSValuecvPMS0_PvEv", referenced from:
>       __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
>       __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o
>   "__ZNK3JSC7JSValue6asCellEv", referenced from:
>       __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
>       __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o

Yeah I have a fix locally, just haven't uploaded it yet.

> 
> Maybe you need to #include JSValueInlines.h?

We shouldn't ever include that file.  The correct thing to do is to make sure that every .cpp file includes "Operations.h", which pulls in a bunch of these BlahInlines.h files.  I had forgotten to add such an include to SymbolTable.cpp.

(I did a refactoring about a year ago that made Opertions.h work this way.  It was because Ossy and I ended up reaching the end of the line for JSC's include dependency graph - there was a refactoring for which no valid #include order in VM.h existed; one variant would break Darwin and another would break everything else.  It was impossible to work out why crap broke.  So with a bunch of help I moved a lot of inline methods into Inlines.h files, but that created a situations where we would have to include like three different BlahInlines.h files in every .cpp file.  So I instituted the "Operations.h is your friend" rule, so that if you include that, you get the inlines you need.)

[Geoffrey Garen]

> > Maybe you need to #include JSValueInlines.h?
> 
> We shouldn't ever include that file.  The correct thing to do is to make sure that every .cpp file includes "Operations.h", which pulls in a bunch of these BlahInlines.h files.

Sounds like an OK rule. This kind of rule would be easier to communicate if we had a file with a clearer name. Maybe JSCInlines.h? We could either factor out the #includes from Operations.h's into a new file, or we could rename Operations.h to JSCInlines.h.

[Filip Pizlo]

Created attachment 217710 [details]
the patch

[Geoffrey Garen]

Comment on attachment 217710 [details]
the patch

r=me

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/159713