Bug 185283 - [JSC][GTK][JSCONLY] Use capstone disassembler
Summary: [JSC][GTK][JSCONLY] Use capstone disassembler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 185423
  Show dependency treegraph
 
Reported: 2018-05-03 17:58 PDT by Yusuke Suzuki
Modified: 2018-05-09 04:53 PDT (History)
13 users (show)

See Also:


Attachments
Patch (17.10 MB, patch)
2018-05-04 00:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.10 MB, patch)
2018-05-04 01:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.10 MB, patch)
2018-05-04 01:31 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (22.66 MB, patch)
2018-05-04 02:02 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.15 MB, patch)
2018-05-04 05:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.15 MB, patch)
2018-05-04 18:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Non capstone source part (18.08 KB, patch)
2018-05-04 18:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.15 MB, patch)
2018-05-05 02:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Non source part of the patch for review (19.81 KB, patch)
2018-05-05 02:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.15 MB, patch)
2018-05-06 21:19 PDT, Yusuke Suzuki
mcatanzaro: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
no source ver (19.77 KB, patch)
2018-05-06 21:20 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.48 MB, application/zip)
2018-05-06 23:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (12.74 MB, application/zip)
2018-05-07 01:42 PDT, EWS Watchlist
no flags Details
Patch for landing (4.16 MB, patch)
2018-05-08 23:38 PDT, Yusuke Suzuki
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch + allow-tabs (4.16 MB, patch)
2018-05-08 23:54 PDT, Yusuke Suzuki
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch + allow-tabs + 1 (4.16 MB, patch)
2018-05-09 00:00 PDT, Yusuke Suzuki
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-05-03 17:58:19 PDT
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.
Comment 1 Yusuke Suzuki 2018-05-03 17:58:24 PDT
https://github.com/aquynh/capstone
Comment 2 Yusuke Suzuki 2018-05-03 17:59:32 PDT
For MIPS, ARM, ARMv7 environments.
Comment 3 Yusuke Suzuki 2018-05-04 00:49:04 PDT
Created attachment 339527 [details]
Patch
Comment 4 Yusuke Suzuki 2018-05-04 00:50:20 PDT
TABS in the source code is not annotated.
Comment 5 Yusuke Suzuki 2018-05-04 01:14:11 PDT
Created attachment 339528 [details]
Patch
Comment 6 Yusuke Suzuki 2018-05-04 01:31:00 PDT
Created attachment 339529 [details]
Patch
Comment 7 Yusuke Suzuki 2018-05-04 02:02:29 PDT
Created attachment 339530 [details]
Patch
Comment 8 Carlos Alberto Lopez Perez 2018-05-04 04:19:55 PDT
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?
Comment 9 Yusuke Suzuki 2018-05-04 04:43:35 PDT
(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
Comment 10 Dominik Inführ 2018-05-04 04:54:17 PDT
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.
Comment 11 Carlos Alberto Lopez Perez 2018-05-04 05:02:16 PDT
(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.
Comment 12 Yusuke Suzuki 2018-05-04 05:12:35 PDT
(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.
Comment 13 Yusuke Suzuki 2018-05-04 05:49:53 PDT
Created attachment 339537 [details]
Patch
Comment 14 Yusuke Suzuki 2018-05-04 05:50:47 PDT
Dropped X86, X86_64, ARM64 supports. Only enable ARMv7, ARM, and MIPS.
Comment 15 Carlos Alberto Lopez Perez 2018-05-04 07:00:28 PDT
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})
Comment 16 Carlos Alberto Lopez Perez 2018-05-04 07:04:17 PDT
(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})
Comment 17 Adrian Perez 2018-05-04 07:14:05 PDT
(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).
Comment 18 Michael Catanzaro 2018-05-04 09:49:56 PDT
(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.
Comment 19 Yusuke Suzuki 2018-05-04 18:41:47 PDT
Created attachment 339618 [details]
Patch
Comment 20 Yusuke Suzuki 2018-05-04 18:43:29 PDT
Created attachment 339619 [details]
Non capstone source part
Comment 21 Yusuke Suzuki 2018-05-04 18:44:37 PDT
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.
Comment 22 Yusuke Suzuki 2018-05-04 23:27:30 PDT
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
Comment 23 Yusuke Suzuki 2018-05-05 02:43:17 PDT
Created attachment 339641 [details]
Patch
Comment 24 Yusuke Suzuki 2018-05-05 02:44:25 PDT
Created attachment 339642 [details]
Non source part of the patch for review
Comment 25 Michael Catanzaro 2018-05-06 07:31:46 PDT
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
Comment 26 Yusuke Suzuki 2018-05-06 21:16:32 PDT
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.
Comment 27 Yusuke Suzuki 2018-05-06 21:19:12 PDT
Created attachment 339703 [details]
Patch
Comment 28 Yusuke Suzuki 2018-05-06 21:20:42 PDT
Created attachment 339704 [details]
no source ver
Comment 29 EWS Watchlist 2018-05-06 23:36:51 PDT
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
Comment 30 EWS Watchlist 2018-05-06 23:37:02 PDT
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
Comment 31 EWS Watchlist 2018-05-07 01:41:49 PDT
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
Comment 32 EWS Watchlist 2018-05-07 01:42:00 PDT
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
Comment 33 Yusuke Suzuki 2018-05-07 01:44:52 PDT
These errors are not related to this one since it just replaces disassembler, which is only used if we pass an option to JSC.
Comment 34 Yusuke Suzuki 2018-05-08 07:16:52 PDT
Ping reviews for Linux reviewers, especially maintainers for ARM, ARMv7, and MIPS :D
Comment 35 Dominik Inführ 2018-05-08 10:13:39 PDT
Looks good to me: I've built the patch and tested on ARMv7 and MIPS.
Comment 36 Yusuke Suzuki 2018-05-08 23:38:08 PDT
Created attachment 339932 [details]
Patch for landing
Comment 37 WebKit Commit Bot 2018-05-08 23:42:13 PDT
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
Comment 38 Yusuke Suzuki 2018-05-08 23:54:47 PDT
Created attachment 339936 [details]
Patch + allow-tabs
Comment 39 WebKit Commit Bot 2018-05-08 23:59:05 PDT
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
Comment 40 Yusuke Suzuki 2018-05-09 00:00:22 PDT
Created attachment 339937 [details]
Patch + allow-tabs + 1
Comment 41 WebKit Commit Bot 2018-05-09 00:03:06 PDT
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
Comment 42 Yusuke Suzuki 2018-05-09 04:42:12 PDT
Committed r231553: <https://trac.webkit.org/changeset/231553>
Comment 43 Radar WebKit Bug Importer 2018-05-09 04:53:52 PDT
<rdar://problem/40089576>