Bug 72855 – JavaScriptCore: Mark all exported symbols in the header file automatically.

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

[Hajime Morrita]

I'm writing a tool for that:
https://github.com/omo/ListExportables
Will announce this effort in webkit-dev shortly.

[Hajime Morrita]

Created attachment 116040 [details]
Patch

[Hajime Morrita]

This is just a preliminary result. What should land would be generated  from the ToT at that timing.

[Hajime Morrita]

Created attachment 116064 [details]
Patch

[Hajime Morrita]

Comment on attachment 116064 [details]
Patch

Uploaded to the wrong bug...

[Hajime Morrita]

Created attachment 116700 [details]
It's ready to review

[WebKit Review Bot]

Attachment 116700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/OpaqueJSString.h..." exit_code: 1

Last 3072 characters of output:
[readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/PropertyDescriptor.h:50:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/MainThread.h:49:  The parameter name "paused" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSLock.h:96:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/text/StringImpl.h:279:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSGlobalData.h:301:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ByteArray.h:86:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:136:  The parameter name "mutex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/SamplingTool.h:223:  Missing spaces around =  [whitespace/operators] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:79:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:80:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:81:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/JavaScriptCore/wtf/text/AtomicString.h:57:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "entry" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 25 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Blocking bugs being removed, now this is ready for the review for applying the rewrite!
You can validate the build by applying the the preliminary patch on Bug 72854,
which disables export list.

Note that
- The rewrite is based on svn r101220.
- Reported style error is false positive. touched line already contained such errors.

[Gyuyoung Kim]

Comment on attachment 116700 [details]
It's ready to review

Attachment 116700 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10639428

[Early Warning System Bot]

Comment on attachment 116700 [details]
It's ready to review

Attachment 116700 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10643406

[Hajime Morrita]

Oops. I need to change WebCore/config.h... Filed at Bug 73191.

[Mark Rowe (bdash)]

The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.

[Hajime Morrita]

(In reply to comment #11)
> The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.

Basically I agree with you that forcing JS_INLINE is ugly.

Fortunately, JS_INLINE will be used in limited places:
We need it only for inline method whose class has exported constructors and a vtable.

Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline.
I can help such inline to non-inline conversion if you are OK.

We need JS_INLINE (explicit "hidden" visibility annotation) because
there is no way to make vtables visible using source annotation except marking classes as visible.

I think this is a reasonable compromise compared to using export lists.
But I'd love to hear your suggestion.

Yet another idea is to #define inline ...

[Collabora GTK+ EWS bot]

Comment on attachment 116700 [details]
It's ready to review

Attachment 116700 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10678739

[David Levin]

(In reply to comment #12)
> (In reply to comment #11)
> > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.
> 
> Basically I agree with you that forcing JS_INLINE is ugly.
> 
> Fortunately, JS_INLINE will be used in limited places:
> We need it only for inline method whose class has exported constructors and a vtable.
> 
> Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline.
> I can help such inline to non-inline conversion if you are OK.
> 
> We need JS_INLINE (explicit "hidden" visibility annotation) because
> there is no way to make vtables visible using source annotation except marking classes as visible.
> 
> I think this is a reasonable compromise compared to using export lists.
> But I'd love to hear your suggestion.
> 
> Yet another idea is to #define inline ...

Does the build break if I get it wrong? (I would consider this a nice feature.)

In order words, how can we help people to get this right? (Both when to include it and when not to include it.)

PS I love the idea!

[Hajime Morrita]

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.
> > 
> > Basically I agree with you that forcing JS_INLINE is ugly.
> > 
> > Fortunately, JS_INLINE will be used in limited places:
> > We need it only for inline method whose class has exported constructors and a vtable.
> > 
> > Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline.
> > I can help such inline to non-inline conversion if you are OK.
> > 
> > We need JS_INLINE (explicit "hidden" visibility annotation) because
> > there is no way to make vtables visible using source annotation except marking classes as visible.
> > 
> > I think this is a reasonable compromise compared to using export lists.
> > But I'd love to hear your suggestion.
> > 
> > Yet another idea is to #define inline ...
> 
> Does the build break if I get it wrong? (I would consider this a nice feature.)
> 
> In order words, how can we help people to get this right? (Both when to include it and when not to include it.)
> 
Oops. Sorry for the slow response...
For Mac the build will simply fail because of the checker script.
For Chromium, maybe we can improve our clang plugin to detect this.
For other port, I have no idea. I guess it's possible to write a checker script which is similar to Mac's,
but I'm not sure. Fortunately the EWS has chromium bot so it would help other port developers.

> PS I love the idea!
Thanks!

[Hajime Morrita]

Comment on attachment 116700 [details]
It's ready to review

clearing r? until the blocking bugs are fixed.

[Hajime Morrita]

Created attachment 121986 [details]
Patch

[WebKit Review Bot]

Attachment 121986 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/OpaqueJSString.h..." exit_code: 1

Last 3072 characters of output:
ty/parameter_name] [5]
Source/JavaScriptCore/runtime/Identifier.h:123:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:54:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/PropertyDescriptor.h:55:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/MainThread.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/wtf/MainThread.h:52:  The parameter name "paused" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSLock.h:96:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/text/StringImpl.h:278:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSGlobalData.h:315:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ByteArray.h:86:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:124:  The parameter name "mutex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/SamplingTool.h:223:  Missing spaces around =  [whitespace/operators] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:85:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:86:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:87:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/JavaScriptCore/wtf/text/AtomicString.h:57:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "entry" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 24 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Finally, the patch is ready!
I confirmed that this can be buit even without exp file on SL (The exp list removal patch is on Bug 72854)
Thanks for the clean API design of JSC, we don't need any extra INLINE macros.

After landing this change, I'll attack both exp file (on Mac) and def file (on Windows).

Note that style red is false positive. That has been there before this change.

[WebKit Review Bot]

Comment on attachment 121986 [details]
Patch

Attachment 121986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11207108

[Hajime Morrita]

Darin, thanks you for the review!
I'll fix the chromium bot problem (it should be done by a separate patch) then land this.

[Hajime Morrita]

Created attachment 122369 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122369 [details]
Patch for landing

Rejecting attachment 122369 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ring.h
patching file Source/JavaScriptCore/wtf/text/StringBuilder.h
patching file Source/JavaScriptCore/wtf/text/StringImpl.h
patching file Source/JavaScriptCore/wtf/text/WTFString.h
patching file Source/JavaScriptCore/wtf/unicode/Collator.h
patching file Source/JavaScriptCore/wtf/unicode/UTF8.h
patching file Source/JavaScriptCore/yarr/Yarr.h
patching file Source/JavaScriptCore/yarr/YarrPattern.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/11142891

[Hajime Morrita]

Committed r104900: <http://trac.webkit.org/changeset/104900>