1594344 - Use the new Windows dump_syms in automation when building Firefox

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, task)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

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

Same story as bug 1588538 but for the builds on taskcluster. I'm not sufficiently familiar with taskcluster to figure out if we need the same changes for both bugs or not.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Bug 1594344 - Part 1: Add dump_syms toolchain. r=#firefox-build-system-reviewers

This defines a new dump_syms toolchain that uses the rust implementation of dump_syms.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Bug 1594344 - Part 2: Fetch dump_syms toolchain on Windows builds. r=#firefox-build-system-reviewers

This installs the rust-based dump_syms toolchain on Windows builds.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Bug 1594344 - Part 3: Use new dump_syms in automation. r=#firefox-build-system-reviewers,gsvelto

This optionally uses the rust-based dump_syms toolchain if it has been
installed, otherwise it uses the locally built version.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

There seems to be something wrong: the target.crashreporter-symbols-full.zip artifact in the build doesn't have symbols for xul. Grep'ing the log reveals that dump_syms did run and there's no error messages or warnings:

[task 2019-11-16T02:23:05.112Z] 02:23:05     INFO -  Beginning work for file: z:\build\build\src\obj-firefox\toolkit\library\build\xul.dll
[task 2019-11-16T02:23:05.113Z] 02:23:05     INFO -  Processing file: z:\build\build\src\obj-firefox\toolkit\library\build\xul.dll
[task 2019-11-16T02:23:05.113Z] 02:23:05     INFO -  z:\build\fetches\dump_syms\dump_syms.exe z:\build\build\src\obj-firefox\toolkit\library\build\xul.dll
[task 2019-11-16T02:23:05.113Z] 02:23:05     INFO -  PERFHERDER_DATA: {"framework": {"name": "build_metrics"}, "suites": [{"subtests": [{"alertChangeType": "absolute", "name": "num_static_constructors", "value": 0, "alertThreshold": 3}], "name": "compiler_metrics"}]}
[task 2019-11-16T02:23:05.113Z] 02:23:05     INFO -  Finished processing z:\build\build\src\obj-firefox\toolkit\library\build\xul.dll in 11.90s

This is quite suspicious.

Comment 5

[Eric Rahm [:erahm]]

(In reply to Gabriele Svelto [:gsvelto] from comment #4)

There seems to be something wrong: the target.crashreporter-symbols-full.zip artifact in the build doesn't have symbols for xul. Grep'ing the log reveals that dump_syms did run and there's no error messages or warnings:

Is that all builds or a specific type?

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

OK, there's a few issues:

• Debug builds don't have xul.sym, across the three architectures (x86, x86-64 and AArch64)
• AArch64 builds are missing the CFI directives, the old dump_syms generated them correctly
• There's an odd thing I noticed that was also present with the previous version and I don't know if it's a bug or a feature: we have all the FUNC entries, then all the PUBLIC entries, then a single FUNC entry that looks like this:

  FUNC 4d6d000 39 0 _IGeckoCustom_IID_Lookup(_GUID const*, int*)
  4d6d000 24 500 15413
  4d6d024 3 507 15413
  4d6d027 f 502 15413
  4d6d036 3 507 15413
  

  The signature is also slightly different than what we have in the generated code. The latter is not really an issue, it's just odd that it shows up at the end right before the CFI directives but after the PUBLIC ones.

Comment 7

[Calixte Denizet (:calixte)]

About the signature, it's probably because of that:
https://docs.microsoft.com/fr-fr/office/client-developer/outlook/mapi/iid
About the position of _IGeckoCustom_IID_Lookup, could it mean that this is unused but exported code ?

Comment 8

[Calixte Denizet (:calixte)]

About aarch64: after chatting with Gabriele, there is finally no issue here (he tested with a tree containing Nathan's patch for CFI stuff for aarch64).
So there is only the issue with debug builds.

Comment 9

[Calixte Denizet (:calixte)]

About debug builds:
There is an error in dump_syms when dumping function signatures containing an array where the base type has a null size:
https://github.com/bytecodealliance/cranelift/blob/master/cranelift-bforest/src/lib.rs#L129
and
https://github.com/rust-lang/rust/blob/master/src/libcore/array/mod.rs#L119

A patch in dump_syms is in review:
https://github.com/mozilla/dump_syms/pull/40

Comment 10

[Calixte Denizet (:calixte)]

Anyway it will be helpful to have dump_syms errors in the logs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1599066

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

OK, let's make this happen. New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=519bbf974191d71af789daa4eea5385d175fafd9

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36458762149d
Part 1: Add dump_syms toolchain. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/57b641bb4d4b
Part 2: Fetch dump_syms toolchain on Windows builds. r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/3208e2b16458
Part 3: Use new dump_syms in automation. r=froydnj

Comment 13

[Dorel Luca [:dluca]]

Backed out 3 changesets (bug 1594344) for Decision task failure on taskcluster/ci/toolchain/rust-size.yml. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280478893&repo=autoland&lineNumber=504

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=3208e2b16458750c5f14efcccfd4acb9bb5a0bf3

Backout:
https://hg.mozilla.org/integration/autoland/rev/617f7b891207ba317cdacbe3ee0c0d943ddc2c1c

Comment 14

[Eric Rahm [:erahm]]

It looks like phab ended up doing a rename instead of a copy for rust-size.yml?

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

There is definitely a bug in phabricator, I just updated the patches again and the copies on my machine are being turned into moves. I double-checked and that seems to be the problem.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

I'll fold the patches locally into just one and see what Phabricator thinks of it when starting from scratch.

Comment 17

[Gabriele Svelto [:gsvelto] (PTO)]

By manually removing and re-adding the affected files it seems like I made Phabricator happy, I'll push again and see what happens.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d32da5fc9482
Part 1: Add dump_syms toolchain. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/8514ded81b77
Part 2: Fetch dump_syms toolchain on Windows builds. r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/4e035d5e8d6c
Part 3: Use new dump_syms in automation. r=froydnj

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/d32da5fc9482
https://hg.mozilla.org/mozilla-central/rev/8514ded81b77
https://hg.mozilla.org/mozilla-central/rev/4e035d5e8d6c