Closed
Bug 1464501
Opened 7 years ago
Closed 6 years ago
Report section sizes in Mach-O and PE binaries
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
(Keywords: in-triage)
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
+++ 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
Assignee | ||
Comment 1•6 years ago
|
||
The `rust-size` repo now supports ELF, Mach-O, and PE. ted, how would we get rust-size built and included with our toolchain?
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Flags: needinfo?(ted)
Comment 2•6 years ago
|
||
Like sccache?
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #2)
> Like sccache?
Have any links? I'm not sure how that's handled.
Comment 4•6 years ago
|
||
Search for sccache in taskcluster/ci/toolchains/*.yml, and the corresponding scripts in taskcluster/scripts/misc
Assignee | ||
Comment 5•6 years ago
|
||
Mike, does this look right to you? Do I have to jump through any other hoops to get it installed on the build machine?
Attachment #8984322 -
Flags: review?(mh+mozilla)
Comment 6•6 years ago
|
||
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.
Attachment #8984322 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #8984323 -
Flags: review?(mh+mozilla)
Updated•6 years ago
|
Attachment #8984323 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
(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.
Flags: needinfo?(ted)
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
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
Flags: needinfo?(mh+mozilla)
Comment 14•6 years ago
|
||
in mozharness, I think you'll have to hardcode the path, deriving from mozharness's knowledge of the topsrcdir.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 15•6 years ago
|
||
Assignee | ||
Comment 16•6 years ago
|
||
(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.
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #8984625 -
Flags: review?(nfroyd)
Assignee | ||
Updated•6 years ago
|
Attachment #8984328 -
Attachment is obsolete: true
Assignee | ||
Comment 18•6 years ago
|
||
Comment 19•6 years ago
|
||
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?
Attachment #8984625 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 20•6 years ago
|
||
(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.
Assignee | ||
Comment 21•6 years ago
|
||
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•6 years ago
|
||
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 #
Flags: needinfo?(erahm)
Assignee | ||
Comment 23•6 years ago
|
||
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9c26abfd1b73
https://hg.mozilla.org/mozilla-central/rev/05c128083f25
https://hg.mozilla.org/mozilla-central/rev/c6d82575895c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Comment 25•6 years ago
|
||
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.
Flags: needinfo?(erahm)
Comment 26•6 years ago
|
||
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?
Assignee | ||
Comment 27•6 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•