1275424 - Put Rust standard library VCS info into Breakpad symbol files to get proper source links in crash reports

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Ralph Giles (:rillian)]

`rustc --version --verbose` will report the git commit hash it was built from. We want to record this at configure time and use it to generate source links in crash reports.

Currently we get links like `obj-firefox/media/src/libcore/result.rs:687` mistakenly directed to hg.mozilla.org. This should instead be recognized as rust libstd code and directed to e.g. https://github.com/rust-lang/rust/blob/db2939409/src/libcore/result.rs#l687 for rust 1.8.0 stable.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Part 1: Record rust commit hash in configure

I wasn't successful in cargo-culting a python port of rust.m4, so just do this in shell.

Ted, calling AC_SUBST(RUSTC_HASH) puts the variable in config.status. Can we read that directly when generating the symbol table? Any other suggestions of how to get it into the scripts?

Comment 2

[(not currently active) Ted Mielczarek]

symbolstore.py already imports `buildconfig` and uses buildconfig.substs[...] for a few things:
https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py

We could do the same thing here. I'm not sure how exactly we'd identify Rust sources right now, if we don't have full paths to the source files we'd have to have some heuristics.

Related, I should finish bug 1266368.

Comment 3

[(not currently active) Ted Mielczarek]

I finally spent a little time poking through the rustc source, and with a lot of help from eddyb on #rust-internals I filed an issue on the relative paths coming out of libcore:
https://github.com/rust-lang/rust/issues/34179

Comment 4

[Ralph Giles (:rillian)]

Attachment: Bug 1275424 - Allow `map` in the moz.configure sandbox.

This is useful for parsing semi-machinable command output into dicts.

Review commit: https://reviewboard.mozilla.org/r/62172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62172/

Comment 5

[Ralph Giles (:rillian)]

Attachment: Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

This is hopefully more reliable than parsing just the summary line,
and makes available other keys like the commit-hash which we'd like
to pass to the debug symbol automation.

Review commit: https://reviewboard.mozilla.org/r/62174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62174/

Comment 6

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/62174/#review58942

::: build/moz.configure/rust.configure:25
(Diff revision 1)
>      try:
> -        # TODO: We should run `rustc --version -v` and parse that output instead.
> -        version = Version(subprocess.check_output(
> -            [rustc, '--version']
> -        ).splitlines()[0].split()[1])
> -        return version
> +        out = subprocess.check_output(
> +            [rustc, '--version', '--verbose']
> +        ).splitlines()
> +        summary = out[0]
> +        details = dict(map(str.strip, line.split(':', 2)) for line in out[1:])

You could rewrite that as:

details = dict((s.strip() for s in line.split(':', 2)) for line in out[1:])

and not require map.

::: build/moz.configure/rust.configure:26
(Diff revision 1)
> -        # TODO: We should run `rustc --version -v` and parse that output instead.
> -        version = Version(subprocess.check_output(
> -            [rustc, '--version']
> -        ).splitlines()[0].split()[1])
> -        return version
> +        out = subprocess.check_output(
> +            [rustc, '--version', '--verbose']
> +        ).splitlines()
> +        summary = out[0]
> +        details = dict(map(str.strip, line.split(':', 2)) for line in out[1:])
> +        return (summary, details)

That makes the function output not very nice, and, considering the contents of summary overlap with details, why return both?

It would also be nicer if the "release" field was Version()ed so that dependees don't need to do it themselves.

Comment 7

[Ralph Giles (:rillian)]

(In reply to Mike Hommey [:glandium] from comment #6)

> details = dict((s.strip() for s in line.split(':', 2)) for line in out[1:])

Ok, thanks!

> That makes the function output not very nice, and, considering the contents
> of summary overlap with details, why return both?

So I can report the summary line? I guess it doesn't matter much, I've just found quoting actual output in the configure log helpful in past debugging.

> It would also be nicer if the "release" field was Version()ed so that
> dependees don't need to do it themselves.

I started out writing a class to encapsulate this, but that wasn't allowed by the sandbox either. :/

Comment 8

[Mike Hommey [:glandium]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #6)
> 
> > details = dict((s.strip() for s in line.split(':', 2)) for line in out[1:])
> 
> Ok, thanks!
> 
> > That makes the function output not very nice, and, considering the contents
> > of summary overlap with details, why return both?
> 
> So I can report the summary line? I guess it doesn't matter much, I've just
> found quoting actual output in the configure log helpful in past debugging.

If you want it for debugging, you can just log it from the rustc_version function... with log.debug().

> > It would also be nicer if the "release" field was Version()ed so that
> > dependees don't need to do it themselves.
> 
> I started out writing a class to encapsulate this, but that wasn't allowed
> by the sandbox either. :/

Now that you mention it, it would also be nicer as a namespace() than as a dict.

something like:

details = dict(s.strip() for s in line.split(':', 2)) for line in out[1:])
details['release'] = Version(details['release'])
return namespace(**details)

Or

details = {}
for line in out[1:]:
   k, v = line.split(':', 2)
   details[k.strip()] = Version(v.strip()) if k.strip() == 'release' else v.strip()
return namespace(**details)

Or

return namespace(**{
           k: Version(v) if k == 'release' else v
           for line in out[1:]
           for k, v in [(s.strip() for s in line.split(':', 2))]
       })

Or a variant along these lines

Comment 9

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #8)
> If you want it for debugging, you can just log it from the rustc_version
> function... with log.debug().

If you want something to visually print to the user, you can re-derive it from what you have in details. In fact, the summary is too verbose, most people will only care about the version itself, not the commit date, hash and the build date.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8767769 [details]
Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/1-2/

Comment 11

[Ralph Giles (:rillian)]

Sorry, didn't see your follow-up comments. Will look at the namespace idea tomorrow, I wasn't familiar with them.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > If you want it for debugging, you can just log it from the rustc_version
> > function... with log.debug().
> 
> If you want something to visually print to the user, you can re-derive it
> from what you have in details. In fact, the summary is too verbose, most
> people will only care about the version itself, not the commit date, hash
> and the build date.

Also note you weren't actually using the summary :)

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8767769 [details]
Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/2-3/

Comment 14

[Ralph Giles (:rillian)]

log.debug() the summary line. Nice how that only prints on exception!

Only return the keys we care about in a namespace.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8767769 [details]
Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

https://reviewboard.mozilla.org/r/62174/#review58984

::: build/moz.configure/rust.configure:24
(Diff revisions 1 - 3)
> -def rustc_version(rustc):
> +def rustc_info(rustc):
>      try:
>          out = subprocess.check_output(
>              [rustc, '--version', '--verbose']
>          ).splitlines()
> -        summary = out[0]
> +        log.debug(out[0])

If you don't care about the output when no error occurs, you could replace both subprocess.check_output + log.debug with check_cmd_output() (same arguments and return value as subprocess.check_output), which will output the whole output for rustc --version --verbose if it returns a non-zero error code.

::: build/moz.configure/rust.configure:27
(Diff revisions 1 - 3)
> +            version=Version(info['release']),
> +            commit=info['commit-hash'],

Technically, this might fail if release and/or commit-hash are not in the output for some reason, and there is nothing that would catch this error and make it nicer to the user. In fact, out[0] could fail as well.

Comment 16

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/62174/#review59008

::: build/moz.configure/rust.configure:25
(Diff revision 3)
>      try:
> -        # TODO: We should run `rustc --version -v` and parse that output instead.
> -        version = Version(subprocess.check_output(
> -            [rustc, '--version']
> -        ).splitlines()[0].split()[1])
> -        return version
> +        out = subprocess.check_output(
> +            [rustc, '--version', '--verbose']
> +        ).splitlines()
> +        log.debug(out[0])
> +        info = dict((s.strip() for s in line.split(':', 2)) for line in out[1:])

BTW, this should be line.split(':', 1)

Comment 17

[Mike Hommey [:glandium]]

Note: this is what the output looks for a distro rustc (Debian):

$ rustc --version --verbose
rustc 1.9.0
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.9.0

Comment 18

[Ralph Giles (:rillian)]

(In reply to Mike Hommey [:glandium] from comment #15)
> Comment on attachment 8767769 [details]

> If you don't care about the output when no error occurs, you could replace
> both subprocess.check_output + log.debug with check_cmd_output() (same
> arguments and return value as subprocess.check_output), which will output
> the whole output for rustc --version --verbose if it returns a non-zero
> error code.

Ok, that's cleaner. Thanks.

> > +            version=Version(info['release']),
> > +            commit=info['commit-hash'],
> 
> Technically, this might fail if release and/or commit-hash are not in the
> output for some reason, and there is nothing that would catch this error and
> make it nicer to the user. In fact, out[0] could fail as well.

Yes. I tried adding `except KeyError` and `except IndexError` clauses to the `try` but those aren't exposed to the sandbox. A generic `expect:` anything clause is too general and hides the stack trace if something unexpected happens. I can do is old-fashioned `if len(out) < 2` checks and `info.get()` dictionary lookups with explicit fallbacks?

> BTW, this should be line.split(':', 1)

Oops.

> Note: this is what the output looks for a distro rustc (Debian):
>
> $ rustc --version --verbose
> rustc 1.9.0
> [...]
> commit-hash: unknown

Charming. So we'll have to handle 'unknown' from the start, but at least I know what the fallback default should be.

Comment 19

[Ralph Giles (:rillian)]

Comment on attachment 8767769 [details]
Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/3-4/

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8767769 [details]
Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

https://reviewboard.mozilla.org/r/62174/#review59320

::: build/moz.configure/rust.configure:20
(Diff revision 4)
> -    try:
> -        # TODO: We should run `rustc --version -v` and parse that output instead.
> +        if len(out) < 2:
> +            die('Short output getting rustc version.')

Since you're not using out[0] directly, it's not going to be a problem. Interestingly, open-ended ranges work on empty lists even when their start is > 0, so out[1:] won't raise an exception. It will only return an empty list. In which case, you'll fall back on Version('0') and 'unknown', as if len(out) was > 1 but no info was there.

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 8767769 [details]
Bug 1275424 - Parse `rustc --version --verbose` in moz.configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/4-5/

Comment 22

[Ralph Giles (:rillian)]

Update to implement comment #20. I haven't gotten the VCSInfo updates working since; will land this patch in a separate bug since it does not harm.

Comment 23

[(not currently active) Ted Mielczarek]

Resummarizing. I fixed the issue I mentioned in comment 3 a while back, so we have almost enough information to do this properly. For any given source file in the debug info we need to be able to map it to a (repository, changeset, relative path within the repository). For source files in m-c this is easy because we have the info of the repo we're building from handy, and we have $topsrcdir. For code from the Rust standard library we can get the commit hash from rustc as this patch proposes, and we'll be getting absolute paths from the machine that originally compiled the libstd in use, but we'll need to figure out how to map those to the Rust repo and get their relative paths. If the paths on the Rust build machines are stable then we can just have a hardcoded list in symbolstore.py and use that. I don't really have a better idea offhand, short of adding the Rust $topsrcdir to the compiler's `--version -v` output.

Comment 24

[(not currently active) Ted Mielczarek]

Attachment: bug 1275424 - hardcode Rust source paths in symbolstore.py.

This gives us source file names with repository info in our generated
symbol files, so that crash reports on crash-stats can link to the
correct source files for files from the Rust standard library.

I've hardcoded the source paths that the Rust project uses, which is
not my favorite thing, but there's no simple way to get this information
otherwise.

Review commit: https://reviewboard.mozilla.org/r/152300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152300/

Comment 25

[(not currently active) Ted Mielczarek]

I tested this on a local mac build and I got lines in XUL.sym like:
FILE 16215 git:github.com/rust-lang/rust:libstd/collections/hash/map.rs:9f2abadca2d065bf81772cb84981d0a22d8e98b3

I pushed this to try so I can look at the generated symbols from other platforms to verify those.

I looked up some arbitrary crash reports with `rust_panic` on the stack to verify that the source paths I hardcoded match what we're currently seeing:
https://crash-stats.mozilla.com/report/index/0820107c-38a1-401a-8091-53e0b0170623
https://crash-stats.mozilla.com/report/index/35db4048-99ba-46d3-b9fb-a0f930170623
https://crash-stats.mozilla.com/report/index/f1fba4dc-abbb-4651-a9d7-6d7160170622

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8880935 [details]
bug 1275424 - hardcode Rust source paths in symbolstore.py.

https://reviewboard.mozilla.org/r/152300/#review157398

This seems a bit fragile. But if it is the best we can do, it is the best we can do.

Comment 27

[(not currently active) Ted Mielczarek]

I think we'll be able to do better in the future. Rust recently implemented source path remapping for debug info:
https://github.com/rust-lang/rust/pull/41508

If they enabled that for the official builds they could map the source paths to some fixed prefix.

Comment 28

[(not currently active) Ted Mielczarek]

I spot-checked the symbols from the try builds and they all have paths that map to the rust-lang repo, so I think this will be fine, if inelegant.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b06dbb28dd4
hardcode Rust source paths in symbolstore.py. r=gps

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b06dbb28dd4