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)

3 Branch
enhancement
Not set
normal

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)

+++ 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
Keywords: in-triage
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)
Like sccache?
(In reply to Mike Hommey [:glandium] from comment #2) > Like sccache? Have any links? I'm not sure how that's handled.
Search for sccache in taskcluster/ci/toolchains/*.yml, and the corresponding scripts in taskcluster/scripts/misc
Attached patch Part 1: Add rust-size toolchain (deleted) — Splinter Review
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 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+
Attachment #8984323 - Flags: review?(mh+mozilla)
Attachment #8984323 - Flags: review?(mh+mozilla) → review+
Attached patch Part 3: Use rust-size to get section sizes (obsolete) (deleted) — Splinter Review
(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)
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)
in mozharness, I think you'll have to hardcode the path, deriving from mozharness's knowledge of the topsrcdir.
Flags: needinfo?(mh+mozilla)
(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.
Attachment #8984625 - Flags: review?(nfroyd)
Attachment #8984328 - Attachment is obsolete: true
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+
(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.
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)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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)
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?
(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.
Depends on: 1484406
Keywords: in-triage
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: