1539806 - Cranelift: Support disassembly for IONFLAGS=codegen

Status: RESOLVED FIXED

(Core :: Javascript: WebAssembly, enhancement, P3)

Description

[Lars T Hansen [:lth]]

It's a useful debugging tool -- clif-util does not know much about the baldrdash API and won't show us the code we really get -- and it's not much work to support it.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1539806 - Disassemble cranelift-compiled code. r?bbouvier

We hook into the cranelift pipeline so that we can disassemble the
generated code for a function after the function has been compiled,
with prologue and epilogue in place.

This patch forks the disassembly framework already in cranelift so as
to adapt it to SpiderMonkey's current and future needs (stderr
printing, AT&T assembler syntax, other formatting).

A couple of finesses missing here are proper printing of non-code data
(everything in the output range is disassembled at this point) and
support for file output.

Comment 2

[Lars T Hansen [:lth]]

I have a POC working with zydis; this turns out to work fairly well. Zydis is large too but not as large. Integration was not terribly painful either, I just need to do some cleanup before posting a patch.

Zydis may also be able to use the normal jitspew output code, which would be good.

Comment 3

[Lars T Hansen [:lth]]

Upstream PRs for zydis and its zycore-c library. Only the first of these is truly necessary, the others are for hygiene.

https://github.com/zyantific/zydis/pull/115
https://github.com/zyantific/zydis/pull/116
https://github.com/zyantific/zycore-c/pull/11

Comment 4

[Lars T Hansen [:lth]]

The zydis crew has taken all my patches, which will simplify pulling new zydis versions a little.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1539806-zydis.patch

This is the source for Zydis, incorporated in the way explained in the README that is first in the patch. Together with the next three patches this provides support for IONFLAGS=codegen for cranelift code in SpiderMonkey. The patch is too big for moz-phab so I'm just attaching it here but I'm asking for a review even so. Only the files in the top-level directory of this patch are mine; all the others are from the Zydis distribution as explained in the README.

Comment 6

[Lars T Hansen [:lth]]

Attachment: Bug 1539806 - Fix a strict-aliasing error in zydis. r?froydnj

Since we compile with -Wall -Werror=strict-aliasing in some
configurations we need to adapt the zydis source slightly.

This fix has been submitted upstream as
https://github.com/zyantific/zydis/pull/115/.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1539806 - Hook zydis into our source code and build system. r=froydnj

This changes check_spidermonkey_style.py to account for Zydis and
introduces zydis/moz.build and zydis/zydis.h to build Zydis and to
expose the API.

Depends on D27849

Comment 8

[Lars T Hansen [:lth]]

Attachment: Bug 1539806 - Use zydis to disassemble cranelift code. r=bbouvier

This performs the actual disassembly by calling the disassembler from
Cranelift.

Since the disassembler emits to a buffer we can use the jitspew for
printing, thus we get output-to-file etc for free.

Depends on D27850

Comment 9

[Benjamin Bouvier [:bbouvier]]

Comment on attachment 9058858 [details] [diff] [review]
bug1539806-zydis.patch

Review of attachment 9058858 [details] [diff] [review]:
-----------------------------------------------------------------

I only read the README.md, and assume the other files have just been imported the way it's described in the readme, so rs=me for this part.

(I don't think there's a need for security teams to be involved, since this is only for Spidermonkey developers usage.)

Thanks for writing the readme with instructions explaining how to update, and thanks for importing this code.

::: js/src/zydis/README.md
@@ +2,5 @@
> +
> +Zydis x64/x86 disassembler imported from github on 2 April 2019, see
> +https://github.com/zyantific/zydis and https://github.com/zyantific/zycore-c.
> +
> +Zydis is MIT licensed code, see Zydis/LICENSE and Zycore/LICENSE.

I assumed you've checked with legal that this can be included in Gecko?

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 9058858 [details] [diff] [review]
bug1539806-zydis.patch

Review of attachment 9058858 [details] [diff] [review]:
-----------------------------------------------------------------

If I had been thinking, I would have noticed that we also have udis86 imported somewhere for webreplay, and we could have gotten some use out of reusing that.

Like bbouvier, I am assuming that all this code has been integrated according to the steps outlined in `README.md`.

f+ only because I think this stuff should be scripted.

::: js/src/zydis/README.md
@@ +14,5 @@
> +
> +## Integrating new versions of Zydis
> +
> +The procedure for pulling a new version is as follows.  Let's assume your
> +Firefox source code is in the directory `$MOZ/`.

I would love love love if most or all of this could be converted to a script that one could run to pull zydis.  At the very least, the "copy and rename" step should be scripted, ideally the "update include paths" could be scripted, and bonus points would be updating the moz.build file automatically.  Such a script is standard practice for vendoring third-party code nowadays.

@@ +25,5 @@
> +
> +```
> +mdkir $ZYDISTMP
> +cd $ZYDISTMP
> +git clone --recursive https://github.com/zyantific/zydis.git

I think the best practice here is to pin a specific version of the repo...but I see that we don't do that consistently, so maybe never mind.

Comment 11

[Lars T Hansen [:lth]]

(In reply to Nathan Froyd [:froydnj] from comment #10)

Comment on attachment 9058858 [details] [diff] [review]
bug1539806-zydis.patch

Review of attachment 9058858 [details] [diff] [review]:

f+ only because I think this stuff should be scripted.

Yeah, that was inevitable... I'll see what I can do.

@@ +25,5 @@

+```
+mdkir $ZYDISTMP
+cd $ZYDISTMP
+git clone --recursive https://github.com/zyantific/zydis.git

I think the best practice here is to pin a specific version of the
repo...but I see that we don't do that consistently, so maybe never mind.

You mean in the clone command or in some other way? Do explain.

Comment 12

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)

Comment on attachment 9058858 [details] [diff] [review]
bug1539806-zydis.patch

Review of attachment 9058858 [details] [diff] [review]:

::: js/src/zydis/README.md
@@ +2,5 @@

+Zydis x64/x86 disassembler imported from github on 2 April 2019, see
+https://github.com/zyantific/zydis and https://github.com/zyantific/zycore-c.
+
+Zydis is MIT licensed code, see Zydis/LICENSE and Zycore/LICENSE.

I assumed you've checked with legal that this can be included in Gecko?

Will be doing that after technical review; or, I can do it now since you've both agreed in principle to this. Anyway from some initial investigation it does not look like it will be a problem.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Lars T Hansen [:lth] from comment #11)

(In reply to Nathan Froyd [:froydnj] from comment #10)

@@ +25,5 @@

+```
+mdkir $ZYDISTMP
+cd $ZYDISTMP
+git clone --recursive https://github.com/zyantific/zydis.git

I think the best practice here is to pin a specific version of the
repo...but I see that we don't do that consistently, so maybe never mind.

You mean in the clone command or in some other way? Do explain.

I was thinking something like:

https://searchfox.org/mozilla-central/source/taskcluster/scripts/misc/build-sccache.sh#36-40

I was trying to find an instance of it in various vendoring scripts, but could not.

Comment 14

[Lars T Hansen [:lth]]

Attachment: bug1539806-zydis-v2.patch

OK, this is the base zydis patch updated in the following ways:

• there's a script zydis/update.sh that performs all the tasks associated with updating, including generating moz.build and rewriting include paths
• as a result, I've incorporated the autogenerated moz.build in this patch and removed int from a later patch in the queue
• the updater script also saves the rev that was pulled in a text file, and that is in this patch too
• the README.md has been updated

I've pulled a slightly newer version from github, it has my upstreamed fixes, which simplifies the updater script a little and obviates the later aliasing fix patch.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 9059855 [details] [diff] [review]
bug1539806-zydis-v2.patch

Review of attachment 9059855 [details] [diff] [review]:
-----------------------------------------------------------------

WFM, thank you!

Comment 16

[Lars T Hansen [:lth]]

Attachment: bug1539806-zydis-v3.patch

Final base patch, less the change to about:license (separate patch out for review). Carrying froydnj's r+.

Comment 17

[Lars T Hansen [:lth]]

Attachment: Bug 1539806 - Include zydis in about:license. r?mhoye

Before landing, this small patch to about:license will be incorporated
into the quite large patch (too large for moz-phab) that lands the
zydis disassembler source in the tree. That patch has been reviewed
separately.

The following commit message will be used for the combined patch.

------- 8< ----------------------------------------------

The Zydis disassembler is a configurable MIT-licensed decoder and
formatter for x64 and x86.

The code comes from two github repositories:

https://github.com/zyantific/zydis
https://github.com/zyantific/zycore-c

In accompanying licensing material (included in this patch) the
authors are identified as these:

Copyright (c) 2014-2019 Florian Bernd
Copyright (c) 2014-2019 Joel Höner

We will use Zydis to disassemble code generated by the Cranelift JIT.

This source has been taken from the master branch of Zydis on github
and incorporated here in the manner described in README.md in this
directory. Effectively: a subset of files have been incorporated and
the files are in new locations in a flatter tree. README.md has
instructions for repeating the job.

The files in the root Zydis directory js/src/zydis are largely
authored by me but in some cases incorporate code from the Zydis
repository on github and are therefore given a Zydis copyright when
appropriate.

Depends on D27852

Comment 18

[Lars T Hansen [:lth]]

Mike, review ping?

Comment 19

[Mike Hoye [:mhoye]]

Sorry for the delay - had to push back on the "most" phrasing in the license file change. It'll be a quick fix (and I'll turn around an r+ quickly this time, sorry) but we need to be specific.

Comment 20

[Lars T Hansen [:lth]]

Mike, re-review ping after requested wording change? I'd really like to land this.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c44e595b0468
Incorporate the Zydis disassembler source. r=froydnj, r=mhoye
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6e7c86447a
Hook zydis into our source code and build system.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf46ac2cd64
Use zydis to disassemble cranelift code.  r=bbouvier

Comment 22

[Bogdan Tara[:bogdan_tara]]

https://hg.mozilla.org/mozilla-central/rev/c44e595b0468
https://hg.mozilla.org/mozilla-central/rev/7f6e7c86447a
https://hg.mozilla.org/mozilla-central/rev/fdf46ac2cd64