Bug 185283 – [JSC][GTK][JSCONLY] Use capstone disassembler

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Currently, MIPS disassembler is lacking.
And we need to maintain ARM, ARMv7 assemblers etc.
This bug attempt to replace them with capstone disassembler[1].
Capstone is well maintained, licensed under BSD, works well in all the platforms, and does not depend on external third party libraries.

[Yusuke Suzuki]

https://github.com/aquynh/capstone

[Yusuke Suzuki]

For MIPS, ARM, ARMv7 environments.

[Yusuke Suzuki]

Created attachment 339527 [details]
Patch

[Yusuke Suzuki]

TABS in the source code is not annotated.

[Yusuke Suzuki]

Created attachment 339528 [details]
Patch

[Yusuke Suzuki]

Created attachment 339529 [details]
Patch

[Yusuke Suzuki]

Created attachment 339530 [details]
Patch

[Carlos Alberto Lopez Perez]

Instead of bundling this on thirdparty maybe it will be a better idea to make this optional at build time with a dependency on libcapstone?
At least on the GTK port we try hard to not bundle dependencies that can be provided by the distribution, and I see libcapstone3 is available on Debian/Ubuntu.

Is there any advantage by bundling this?

[Yusuke Suzuki]

(In reply to Carlos Alberto Lopez Perez from comment #8)
> Instead of bundling this on thirdparty maybe it will be a better idea to
> make this optional at build time with a dependency on libcapstone?
> At least on the GTK port we try hard to not bundle dependencies that can be
> provided by the distribution, and I see libcapstone3 is available on
> Debian/Ubuntu.
> 
> Is there any advantage by bundling this?

For JSCOnly port, bundling is necessary because of the original goal of JSCOnly port: no dependent libs except for ICU. So anyway, we need to have them in the tree.

For GTK and WPE, we can use external libraries if we only use capstone for ARM and MIPS. capstone development is done in “next” branch, and currently shipped libcapstone3 is very old one (new instruction support is only done in “next” branch, which version is 4.x.x). It means that it is not good to use it for ARM64 and X86_64: in these architectures, we relatively want to use newly added instructions (e.g. ARM v8.3). If we use external library, we cannot add them even if we contribute to capstone upstream.

I think,

1. JSCOnly port must bundle them anyway.
2. GTK and WPE use the external one, and use it only for MIPS and ARM.

would be fine

[Dominik Inführ]

Just wanted to add that using capstone is a great idea. I've built your patch on ARMv7 and MIPS and it works as expected.

[Carlos Alberto Lopez Perez]

(In reply to Yusuke Suzuki from comment #9)
> (In reply to Carlos Alberto Lopez Perez from comment #8)
> > Instead of bundling this on thirdparty maybe it will be a better idea to
> > make this optional at build time with a dependency on libcapstone?
> > At least on the GTK port we try hard to not bundle dependencies that can be
> > provided by the distribution, and I see libcapstone3 is available on
> > Debian/Ubuntu.
> > 
> > Is there any advantage by bundling this?
> 
> For JSCOnly port, bundling is necessary because of the original goal of
> JSCOnly port: no dependent libs except for ICU. So anyway, we need to have
> them in the tree.
> 
> For GTK and WPE, we can use external libraries if we only use capstone for
> ARM and MIPS. capstone development is done in “next” branch, and currently
> shipped libcapstone3 is very old one (new instruction support is only done
> in “next” branch, which version is 4.x.x). It means that it is not good to
> use it for ARM64 and X86_64: in these architectures, we relatively want to
> use newly added instructions (e.g. ARM v8.3). If we use external library, we
> cannot add them even if we contribute to capstone upstream.
> 
> I think,
> 
> 1. JSCOnly port must bundle them anyway.
> 2. GTK and WPE use the external one, and use it only for MIPS and ARM.
> 
> would be fine

Interesting.

I see even on Fedora and Arch they ship libcapstone3.

Perhaps we can bundle this for GTK and WPE as well and try to unbundle it when libcapstone4 is more broadly available? I don't have a strong opinion on this.

[Yusuke Suzuki]

(In reply to Carlos Alberto Lopez Perez from comment #11)
> Interesting.
> 
> I see even on Fedora and Arch they ship libcapstone3.
> 
> Perhaps we can bundle this for GTK and WPE as well and try to unbundle it
> when libcapstone4 is more broadly available? I don't have a strong opinion
> on this.

As long as I can see, "next" branch is split from "master" one year ago, and version 4 is not shipped officially.
I guess that the current libcapstone3 is one year old in terms of new features (except for adding bug fixes).
Typical capstone users (e.g. radare2) use "next" source directly instead of shipped version 3.

To make the patch small, first, I would like to have bundled capstone for MIPS and ARM to drop ARMLLVMDisassembler support.

[Yusuke Suzuki]

Created attachment 339537 [details]
Patch

[Yusuke Suzuki]

Dropped X86, X86_64, ARM64 supports. Only enable ARMv7, ARM, and MIPS.

[Carlos Alberto Lopez Perez]

The review is pretty hard with such big patches, perhaps it makes things easier to split this in two patches? first only land only the third-party code (that should require almost no review as is just importing third-party code) and then review the integration of it on the build system ?

Also I don't think we should enable this unconditionally. What is the value of this for the users not interested in debugging JSC generated assembly code?

I guess its ok to enable it by default for developer builds, so I would set instead (at least for GTK and WPE):

- SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE TRUE)
+ SET_AND_EXPOSE_TO_BUILD(ENABLE_DEVELOPER_MODE ${DEVELOPER_MODE})

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #15)
> - SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE TRUE)
> + SET_AND_EXPOSE_TO_BUILD(ENABLE_DEVELOPER_MODE ${DEVELOPER_MODE})

I meant:

- SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE TRUE)
+ SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE ${DEVELOPER_MODE})

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #16)
> (In reply to Carlos Alberto Lopez Perez from comment #15)
> > - SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE TRUE)
> > + SET_AND_EXPOSE_TO_BUILD(ENABLE_DEVELOPER_MODE ${DEVELOPER_MODE})
> 
> I meant:
> 
> - SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE TRUE)
> + SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE ${DEVELOPER_MODE})

I agree with Carlos here. The disassembler will be mostly useful for
developers and/or debug builds, so making USE_CAPSTONE depend one
DEVELOPER_MODE seems like a good idea.

Also, I think it's okay to import the code into the WebKit repository,
and later on figure out how to allow using an out-of-tree version of
Capstone for those cases where the system provides packages which are
recent enough (none for now, as it seems most/all GNU/Linux distros
package Capstone 3.x).

[Michael Catanzaro]

(In reply to Adrian Perez from comment #17)
> Also, I think it's okay to import the code into the WebKit repository,
> and later on figure out how to allow using an out-of-tree version of
> Capstone for those cases where the system provides packages which are
> recent enough (none for now, as it seems most/all GNU/Linux distros
> package Capstone 3.x).

That would be similar to how we handled the unbundling of libwoff and libbrotli. In retrospect, that was a huge mistake: it would have been better to have never bundled these libraries in the first place. We're now in a situation where WebKit in Ubuntu used to support woff fonts, but no longer does. Instead, we should work on packaging the new libcapstone4 in distributions, and ensure we support building without it for the foreseeable future.

However, Carlos Lopez already found another solution to this problem: DEVELOPER_MODE. If it's only used in DEVELOPER_MODE, then bundling is no problem at all. Just please make sure it's excluded from tarballs in Tools/gtk/manifest.txt.in and Tools/wpe/manifest.txt.in.

[Yusuke Suzuki]

Created attachment 339618 [details]
Patch

[Yusuke Suzuki]

Created attachment 339619 [details]
Non capstone source part

[Yusuke Suzuki]

Uploaded non source part separately for review.
And use DEVELOPER_MODE for GTK and WPE.
For JSCOnly port, USE_CAPSTONE is always enabled for MIPS, ARM, and ARMv7.

[Yusuke Suzuki]

After this is landed, I'll drop ARMv7 disassembler[1], which is already covered by capstone.

[1]: https://lists.webkit.org/pipermail/webkit-dev/2018-May/029981.html

[Yusuke Suzuki]

Created attachment 339641 [details]
Patch

[Yusuke Suzuki]

Created attachment 339642 [details]
Non source part of the patch for review

[Michael Catanzaro]

Comment on attachment 339642 [details]
Non source part of the patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=339642&action=review

> Tools/gtk/manifest.txt.in:53
> +exclude Source/ThirdParty/capstone

Keep it alphabetized, please. :P

> Tools/wpe/manifest.txt.in:53
> +exclude Source/ThirdParty/capstone

Ditto

[Yusuke Suzuki]

Comment on attachment 339642 [details]
Non source part of the patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=339642&action=review

>> Tools/gtk/manifest.txt.in:53
>> +exclude Source/ThirdParty/capstone
> 
> Keep it alphabetized, please. :P

Fixed.

>> Tools/wpe/manifest.txt.in:53
>> +exclude Source/ThirdParty/capstone
> 
> Ditto

Fixed.

[Yusuke Suzuki]

Created attachment 339703 [details]
Patch

[Yusuke Suzuki]

Created attachment 339704 [details]
no source ver

[EWS Watchlist]

Comment on attachment 339703 [details]
Patch

Attachment 339703 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7591375

New failing tests:
http/tests/misc/resource-timing-resolution.html
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 339707 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 339703 [details]
Patch

Attachment 339703 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7592056

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 339708 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Yusuke Suzuki]

These errors are not related to this one since it just replaces disassembler, which is only used if we pass an option to JSC.

[Yusuke Suzuki]

Ping reviews for Linux reviewers, especially maintainers for ARM, ARMv7, and MIPS :D

[Dominik Inführ]

Looks good to me: I've built the patch and tested on ARMv7 and MIPS.

[Yusuke Suzuki]

Created attachment 339932 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 339932 [details]
Patch for landing

Rejecting attachment 339932 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 339932, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
gs.webkit.org/attachment.cgi?id=339932
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Failed to find the property value for the SVN property "allow-tabs": "Index: Source/ThirdParty/capstone/Source/.appveyor.yml
". at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 1519, <ARGV> line 1033.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/7620585

[Yusuke Suzuki]

Created attachment 339936 [details]
Patch + allow-tabs

[WebKit Commit Bot]

Comment on attachment 339936 [details]
Patch + allow-tabs

Rejecting attachment 339936 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 339936, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
bugs.webkit.org/attachment.cgi?id=339936
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Failed to find the property value for the SVN property "allow-tabs": "Index: Source/ThirdParty/capstone/Source/COMPILE.TXT
". at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 1519, <ARGV> line 1749.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/7620708

[Yusuke Suzuki]

Created attachment 339937 [details]
Patch + allow-tabs + 1

[WebKit Commit Bot]

Comment on attachment 339937 [details]
Patch + allow-tabs + 1

Rejecting attachment 339937 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 339937, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ching: https://bugs.webkit.org/attachment.cgi?id=339937
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Did not find end of header block corresponding to index path "Source/ThirdParty/capstone/Source/RELEASE_NOTES". at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 995, <ARGV> line 4761.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/7620733

[Yusuke Suzuki]

Committed r231553: <https://trac.webkit.org/changeset/231553>

[Radar WebKit Bug Importer]

<rdar://problem/40089576>