1464501 - Report section sizes in Mach-O and PE binaries

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1463296 +++

Currently bug 1463296 only supports ELF binaries due to the default version of `size` available on our Linux build machines. It would be helpful to have a tool that supports Mach-O and PE binary formats as well.

Additionally `size` does not report .symtab and .strtab so it would be nice to use this tool for ELF binaries as well. Ted has a PoC up [1] that supports ELF; the underlying library, goblin [2], also supports Mach-O and PE so it should be straightforward to add support for those.

When we add the tool we should update our reporting code to include relevant Mach-O and PE sections.

[1] https://github.com/luser/rust-size
[2] https://github.com/m4b/goblin

Comment 1

[Eric Rahm [:erahm]]

The `rust-size` repo now supports ELF, Mach-O, and PE. ted, how would we get rust-size built and included with our toolchain?

Comment 2

[Mike Hommey [:glandium]]

Like sccache?

Comment 3

[Eric Rahm [:erahm]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Like sccache?

Have any links? I'm not sure how that's handled.

Comment 4

[Mike Hommey [:glandium]]

Search for sccache in taskcluster/ci/toolchains/*.yml, and the corresponding scripts in taskcluster/scripts/misc

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 1: Add rust-size toolchain

Mike, does this look right to you? Do I have to jump through any other hoops to get it installed on the build machine?

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8984322 [details] [diff] [review]
Part 1: Add rust-size toolchain

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

You just need to add dependencies in the build types you want the tool installed.

::: browser/config/tooltool-manifests/win64/rust-size-build.manifest
@@ +1,3 @@
> +[
> +  {
> +    "version": "Visual Studio 2017 15.4.2 / SDK 10.0.15063.0",

I don't really see a reason not to reuse the sccache manifest (maybe renaming it in the process).

::: taskcluster/ci/toolchain/linux.yml
@@ +512,5 @@
> +        symbol: TL(rust-size)
> +        tier: 1
> +    worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> +    worker:
> +        max-run-time: 36000

maybe don't cargo cult 10 hours as a max-run-time :)

::: taskcluster/scripts/misc/build-rust-size.sh
@@ +1,5 @@
> +#!/bin/bash
> +set -x -e -v
> +
> +OWNER=luser
> +PROJECT=rust-size

If you ask me, "rust-size" is a terrible name.

@@ +17,5 @@
> +    UPLOAD_DIR=$WORKSPACE/public/build
> +    WIN_WORKSPACE="$(pwd -W)"
> +    COMPRESS_EXT=bz2
> +
> +    # TODO(erham): This probably isn't necessary but probably doesn't mattter

typos

@@ +32,5 @@
> +. taskcluster/scripts/misc/tooltool-download.sh
> +
> +PATH="$PWD/rustc/bin:$PATH"
> +
> +git clone https://github.com/${OWNER}/${PROJECT} ${PROJECT}

git clone -n ; not sure why I didn't put it in the sccache script.

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 2: Install rust-size toolchain

Comment 8

[Eric Rahm [:erahm]]

Attachment: Part 3: Use rust-size to get section sizes

Comment 9

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d85ad3d32bfa76d2d6c9210c757e504d166dce

Comment 10

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e41908a6437f214a93958bda1420237bb1f03b3c

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #6)
> ::: taskcluster/scripts/misc/build-rust-size.sh
> @@ +1,5 @@
> > +#!/bin/bash
> > +set -x -e -v
> > +
> > +OWNER=luser
> > +PROJECT=rust-size
> 
> If you ask me, "rust-size" is a terrible name.

It totally is. I just hacked it up as a prototype clone of size(1) originally. We can rename it/move it to the Mozilla GitHub org/whatever.

Comment 12

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c25d19f9790ee89af414a0005ac4508f56cc146e

Comment 13

[Eric Rahm [:erahm]]

Okay I've got the toolchain compiling after dealing with two issues:
#1) The version of rust available doesn't support `main` returning a result, I just updated rust-size to have an old-style `main`
#2) There was an issue on Windows where the parent workspace directory had a Cargo.toml file in it that made `cargo` sad. I hacked around it by just renaming the file during compilation of `rust-size`. My guess is one of the dependent toolchains is zipbombing the root workspace dir.

At this point `rust-size` appears to be installed (I verified in the build log [1]) but is not in the path AFAICT, so we're not using it.

Mike, is there a proper way to get the path of `rust-size` in buildbase.py?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=182571211&repo=try&lineNumber=780

Comment 14

[Mike Hommey [:glandium]]

in mozharness, I think you'll have to hardcode the path, deriving from mozharness's knowledge of the topsrcdir.

Comment 15

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ecbe08d75d15a3b324eb882552e79527bbe3438

Comment 16

[Eric Rahm [:erahm]]

(In reply to Mike Hommey [:glandium] from comment #14)
> in mozharness, I think you'll have to hardcode the path, deriving from
> mozharness's knowledge of the topsrcdir.

Yeah that's a bummer, I think it's ending up in $(topsrcdir)/rust-size/rust-size. In mozharness parlance that's probably self.query_abs_dirs()['abs_src_dir']/rust-size/rust-size. Hopefully that's true across platforms.

Comment 17

[Eric Rahm [:erahm]]

Attachment: Part 3: Use rust-size to get section sizes

Comment 18

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1816d610aedaeaf1a9fc47c827373fac666141eb

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8984625 [details] [diff] [review]
Part 3: Use rust-size to get section sizes

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

I think this is OK, but it seems like if we accidentally removed rust-size from some of these builds, we'd start losing information, which would be bad.

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +1607,5 @@
>          from StringIO import StringIO
>  
> +        # Check for `size` programs
> +        # `rust_size` is our cross platform version of size. It should be
> +        # installed by tooltool in $abs_src_dir/rust-size/rust-size

Should we be yelling if we can't find rust-size, since it's our only option on Windows and Mac?  Should we even have the `size` fallback?

@@ +1618,5 @@
>              if size_prog:
>                  break
>  
>          if not size_prog:
>              self.info("Couldn't find `size` program")

i.e. We'd probably turn this into a hard error if we did some of the above.

@@ +1674,5 @@
>              'avcodec': ('libmozavcodec.so', 'mozavcodec.dll', 'libmozavcodec.dylib'),
>              'avutil': ('libmozavutil.so', 'mozavutil.dll', 'libmozavutil.dylib')
>          }
>          # TODO(erahm): update for windows and osx. As-is we only have support
>          # for `size` on debian which gives us linux and android.

Maybe we should update this comment now?

Comment 20

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> Comment on attachment 8984625 [details] [diff] [review]
> Part 3: Use rust-size to get section sizes
> 
> Review of attachment 8984625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is OK, but it seems like if we accidentally removed rust-size
> from some of these builds, we'd start losing information, which would be bad.
> 
> ::: testing/mozharness/mozharness/mozilla/building/buildbase.py
> @@ +1607,5 @@
> >          from StringIO import StringIO
> >  
> > +        # Check for `size` programs
> > +        # `rust_size` is our cross platform version of size. It should be
> > +        # installed by tooltool in $abs_src_dir/rust-size/rust-size
> 
> Should we be yelling if we can't find rust-size, since it's our only option
> on Windows and Mac?  Should we even have the `size` fallback?

The problem is this can be called on systems where we didn't install rust-size, and that's okay. Once we add sheriffing we'll just get an alert when the sizes drop to zero (particularly if we remove the `size` fallback). `size` does give us the ability to report more obscure toolchains, but `rust-size` probably covers all we really care about. I'll go ahead and remove it for now.

> @@ +1618,5 @@
> >              if size_prog:
> >                  break
> >  
> >          if not size_prog:
> >              self.info("Couldn't find `size` program")
> 
> i.e. We'd probably turn this into a hard error if we did some of the above.
> 
> @@ +1674,5 @@
> >              'avcodec': ('libmozavcodec.so', 'mozavcodec.dll', 'libmozavcodec.dylib'),
> >              'avutil': ('libmozavutil.so', 'mozavutil.dll', 'libmozavutil.dylib')
> >          }
> >          # TODO(erahm): update for windows and osx. As-is we only have support
> >          # for `size` on debian which gives us linux and android.
> 
> Maybe we should update this comment now?

Yeah I'll remove it.

Comment 21

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e891ab259427a65b92a880478d6884abf0d4a281
Bug 1464501 - Part 1: Add rust-size toolchain. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e85e1b2f6fa2478a79a51700e604b5e0c8a9414
Bug 1464501 - Part 2: Install rust-size toolchain. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/132442e735694677536e1f09180f45c77325500c
Bug 1464501 - Part 3: Use rust-size to get section sizes. r=froydnj

Comment 22

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (bug 1464501) for build bustages. 

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b55a54cf88c377f00013b7799f30f749ef059ba

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=132442e735694677536e1f09180f45c77325500c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=183095694 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183095694&repo=mozilla-inbound&lineNumber=133200

23:31:36    FATAL - Uncaught exception: Traceback (most recent call last):
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\base\script.py", line 2080, in run
23:31:36    FATAL -     self.run_action(action)
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\base\script.py", line 2019, in run_action
23:31:36    FATAL -     self._possibly_run_method(method_name, error_if_missing=True)
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\base\script.py", line 1959, in _possibly_run_method
23:31:36    FATAL -     return getattr(self, method_name)()
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\mozilla\building\buildbase.py", line 1221, in build
23:31:36    FATAL -     self._generate_build_stats()
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\mozilla\building\buildbase.py", line 1724, in _generate_build_stats
23:31:36    FATAL -     perfherder_data['suites'].extend(self._get_binary_metrics())
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\mozilla\building\buildbase.py", line 1666, in _get_binary_metrics
23:31:36    FATAL -     section_details = self._get_sections(lib, section_interests)
23:31:36    FATAL -   File "z:\build\build\src\testing\mozharness\mozharness\mozilla\building\buildbase.py", line 1632, in _get_sections
23:31:36    FATAL -     parsed = json.loads(output)
23:31:36    FATAL -   File "c:\mozilla-build\python\lib\json\__init__.py", line 339, in loads
23:31:36    FATAL -     return _default_decoder.decode(s)
23:31:36    FATAL -   File "c:\mozilla-build\python\lib\json\decoder.py", line 364, in decode
23:31:36    FATAL -     obj, end = self.raw_decode(s, idx=_w(s, 0).end())
23:31:36    FATAL -   File "c:\mozilla-build\python\lib\json\decoder.py", line 382, in raw_decode
23:31:36    FATAL -     raise ValueError("No JSON object could be decoded")
23:31:36    FATAL - ValueError: No JSON object could be decoded
23:31:36    FATAL - Running post_fatal callback...
23:31:36    ERROR - setting return code to 2 because fatal was called
23:31:36  WARNING - setting return code to 2
23:31:36    FATAL - Exiting -1
23:31:36     INFO - Running post-run listener: _summarize
23:31:36    ERROR - # TBPL FAILURE #
23:31:36     INFO - [mozharness: 2018-06-13 23:31:36.771000Z] FxDesktopBuild summary:
23:31:36    ERROR - # TBPL FAILURE #

Comment 23

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c26abfd1b73ad8301b48dff282716e352e66efd
Bug 1464501 - Part 1: Add rust-size toolchain. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/05c128083f25afdc0be8bc2d344e43f1dc5acc19
Bug 1464501 - Part 2: Install rust-size toolchain. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d82575895c86a073d95cb2157e1e9a17d5c834
Bug 1464501 - Part 3: Use rust-size to get section sizes. r=froydnj

Comment 24

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9c26abfd1b73
https://hg.mozilla.org/mozilla-central/rev/05c128083f25
https://hg.mozilla.org/mozilla-central/rev/c6d82575895c

Comment 25

[Eric Rahm [:erahm]]

For some reason `rust-size` is failing to parse the windows asan binary:

> 23:31:36     INFO - Getting output from command: ['z:\\build\\build\\src\\rust-size\\rust-size.exe', 'z:\\build\\build\\src\\obj-firefox\\dist\\bin\\xul.dll']
> 23:31:36     INFO - Copy/paste: z:\build\build\src\rust-size\rust-size.exe z:\build\build\src\obj-firefox\dist\bin\xul.dll
> 23:31:36     INFO - Reading from file tmpfile_stdout
> 23:31:36     INFO - Output received:
> 23:31:36     INFO -  Error: Scroll(BadInput { size: 295929526, msg: "invalid utf8" })

I added a try block on the python side to handle bad output from `rust-size` and re-landed.

Comment 26

[(not currently active) Ted Mielczarek]

That's exciting! I can take a poke at that at some point. Can you link a build log where that's breaking? Looks like something in goblin doesn't like a string in the binary?

Comment 27

[Eric Rahm [:erahm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26)
> That's exciting! I can take a poke at that at some point. Can you link a
> build log where that's breaking? Looks like something in goblin doesn't like
> a string in the binary?

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=183095694&repo=mozilla-inbound&lineNumber=133191-133196
Binary it's choking on: https://queue.taskcluster.net/v1/task/GvTR1xdlQAiHnbbCec_VQA/runs/0/artifacts/public/build/target.zip

I'm not sure if it's actually a problem with the binary or just the windows+clang environment.