1270091 - fix_stack_using_bpsyms.py should handle parsing errors more gracefully

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[(not currently active) Ted Mielczarek]

in bug 1268617, rillian pushed a patch that caused dump_syms to generate .sym files with FUNC lines that were missing function names, like:
`FUNC 3eee0 c0 0 `

fix_stack_using_bpsyms.py barfs on parsing these:
https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/tools/rb/fix_stack_using_bpsyms.py#46
```
14:49:03     INFO -    File "C:\slave\test\build\tests\bin\fix_stack_using_bpsyms.py", line 46, in __init__
14:49:03     INFO -      (junk, rva, size, ss, name) = line.split(None, 4)
14:49:03 INFO - ValueError: need more than 4 values to unpack 
```
(from https://treeherder.mozilla.org/logviewer.html#?job_id=27162094&repo=mozilla-inbound#L26653 )

The parser should handle these more gracefully so that we get useful output instead of a Python stacktrace.

Comment 1

[(not currently active) Ted Mielczarek]

The symbol file in that log is xul.sym from https://queue.taskcluster.net/v1/task/CQo_Tn5fSkeiwe5Ae7C6vg/artifacts/public/build/firefox-49.0a1.en-US.win64.crashreporter-symbols.zip .

Comment 2

[Ralph Giles (:rillian)]

Attachment: Handle missing function names

Here's a minimal patch to work around the exception with the `rustc -g` output. A better fix would be to catch ValueError on the whole parse stanza and do something sensible, but I'd like to land this in the short term to unblock bug 1266309.

Comment 3

[Ralph Giles (:rillian)]

Does seem to work on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf5299f221b

Comment 4

[Dan Minor [:dminor]]

Comment on attachment 8750912 [details] [diff] [review]
Handle missing function names

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

::: tools/rb/fix_stack_using_bpsyms.py
@@ +48,5 @@
>          # http://code.google.com/p/google-breakpad/wiki/SymbolFiles
>          if line.startswith("FUNC "):
>            # FUNC <address> <size> <stack_param_size> <name>
> +          bits = line.split(None, 4)
> +          expandListTo(bits, 5)

I think you could just do:

if len(bits) < 5:
    bits.append('unnamed_function')

and avoid having to add the expandListTo function.

Comment 5

[Ralph Giles (:rillian)]

Attachment: Handle missing function names v2

Thanks. I guess we don't have to handle the case with empty `size` or `ss`. Carrying forward r=dminor.

Comment 6

[Dan Minor [:dminor]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #5)
> Created attachment 8750950 [details] [diff] [review]
> Handle missing function names v2
> 
> Thanks. I guess we don't have to handle the case with empty `size` or `ss`.

I was thinking it best to fix the problem at hand and save a larger cleanup for later, rather than do a partial cleanup now. I'm not familiar with this code, it would be better to have Ted review anything larger than this :)

Comment 7

[Ralph Giles (:rillian)]

Ok, thanks. Giving it a try now that inbound is open again.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6e362858f2

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b6e362858f2

Comment 10

[Nicholas Nethercote [inactive]]

rillian, can we remove the leave-open keyword and close this bug?