493392 - Get fallback symbols for DWARF-less binaries in symbolstore.py

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Stuart Morgan]

While switching Camino's core build over to DWARF, I noticed that we lost all symbols for a bunch of core dylibs. For whatever reason (flag overriding in those targets? I haven't investigated) those binaries don't seem to be built with DWARF symbols. Before the switch, we at least had function information for those binaries. Compare for example:

libnss3, latest FF 3.0 nightly:
  http://symbols.mozilla.org/symbols_ffx/libnss3.dylib/81A57BE49A8BEF933B3A608EA0C4A2220/libnss3.dylib.sym
libnss3, latest FF trunk nightly:
  http://symbols.mozilla.org/symbols_ffx/libnssutil3.dylib/BABE1A3F954416309F8184DB3DBA037F0/libnssutil3.dylib.sym

While it would obviously be ideal to fix the real problem of them not having DWARF symbols, the situation can be significantly improved in the short term with a simple change to symbolstore.py to detect this case and fall back to running dump_syms on the binary directly.

I can't easily post a patch since it's mingled in with some hacks you wouldn't want (and I don't have a trunk tree), but it would be really easy for someone to recreate it:
- Change the main ProcessFile to return the number of lines it processed, excluding the MODULE line, instead of a boolean.
- Change the Mac ProcessFile to check for a return value of 0 and fall back to
  Dumper.ProcessFile(self, file)
in that case. (I also chose to log to stderr when that happened, since it will make it much easier to for someone down the road to figure out why those symbol files don't have line information).
- Change the other callers to convert 0/non-0 to True-False so it doesn't propagate.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'm wondering if this problem (either a implementing workaround or fixing the underlying problem of completely missing symbols for various libs) is something that should block Firefox 3.5?

Comment 2

[Samuel Sidler (old account; do not CC)]

(In reply to comment #1)
> I'm wondering if this problem (either a implementing workaround or fixing the
> underlying problem of completely missing symbols for various libs) is something
> that should block Firefox 3.5?

Dunno, but I'll nominate it until Ted comes by and says otherwise. :)

Comment 3

[(not currently active) Ted Mielczarek]

I probably wouldn't block on it. I mean, nobody has noticed it until now anyway. This is fallout from the DWARF support changes. It used to be we just ran dump_syms directly on the binary. Now we run dsymutil and then run dump_syms on the .dSYM.

Anyway, there's a different bug filed on not having NSS symbols, which is a worse issue.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> Anyway, there's a different bug filed on not having NSS symbols, which is a
> worse issue.

That's really what I thought was critical; I looked but couldn't find any bug, so I didn't think it was known.

Comment 5

[(not currently active) Ted Mielczarek]

It's bug 468701.

Comment 6

[Florian Quèze [:florian]]

Attachment: patch following the idea in comment 0

I tried to follow the instructions given in comment 0.

I'm interested by this idea because I haven't managed to get binaries containing valid DWARF symbols when building on PPC (a build with the exact same mozconfig produces correct symbols when built on a Mac Intel) and the build machine that produces Instantbird nightlies is a G5.

Comment 7

[Stuart Morgan]

I was looking at my script again today and realized that since the caller only cares about whether or not the number of lines is non-zero, it really makes more sense to have ProcessFile return a boolean still rather than having all the callers do the "> 0 -> True" translation. Not a big deal, but it makes for a cleaner change.

Comment 8

[Florian Quèze [:florian]]

Attachment: patch v2

This patch changes ProcessFile to return True if there is at least one line that is not a MODULE or FILE line.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Wouldn't hold the FF3.5 release for this, especially since the proposed fix doesn't affect the code we ship.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 378283 [details] [diff] [review]
patch v2

Could you give me a patch with more context? This one is a bit hard to read.

Comment 11

[Florian Quèze [:florian]]

Attachment: patch v2 (with more context)

Same patch with 8 lines of context.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 378283 [details] [diff] [review]
patch v2

Thanks for adding context here.

                             # pass through all other lines unchanged
                             f.write(line)
+                            result = True

I think you should move this down two lines, after cmd.close().

+            print >> sys.stderr, "Couldn't read DWARF symbols in: ", dsymbundle

I'd prefer it if you used string interpolation here, like "... %s" % dsymbundle. I find that easier to read.

r=me with those nits addressed.

Comment 13

[Florian Quèze [:florian]]

Attachment: patch v3

(In reply to comment #12)

> +                            result = True
> 
> I think you should move this down two lines, after cmd.close().

I don't think so. Moving this line like you suggest would still fix the issue I have with my invalid DWARF symbols on PPC builds, but it wouldn't do anything for the case of NSS.
In the case of NSS, there is a single MODULE line, like for example:
MODULE mac ppc 4E24E711977C98DEB9F4DF6FE4E94EEC0 libfreebl3.dylib

I don't think we want to return True in this case.
If I don't move this line in the patch, we will return True only if there is at least one line that is not a MODULE or FILE line. I think it's what we want here. I added a comment about this in the patch.

Requesting review again to ensure that you see this comment.

> I'd prefer it if you used string interpolation here, like "... %s" %
> dsymbundle. I find that easier to read.
Fixed.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 382104 [details] [diff] [review]
patch v3

Ah, thanks for the additional informative comment there.

Comment 15

[Florian Quèze [:florian]]

http://hg.mozilla.org/mozilla-central/rev/19e50a3ed48c