491035 - Generate rich symbols for core libraries

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Stuart Morgan]

Right now we get no file/line info for core libraries. I suspect the secret sauce here is Smokey's bug 488512 comment 17.

Comment 1

[Stuart Morgan]

Attachment: Makefile change [landed]

I built with the two flag export lines and Camino's smybol file was 25x larger, so I think it worked ;) A random dylib check didn't show any changes, but that may just be par for the course with the pre-DWARF builds (I couldn't find the FF 3.0.x nightly build cycle to check, for some reason).

The only change we need other than to the tinderbox is that I hadn't considered the fact that all the core files in Camino have a different source root too, so we need a bit more post-processing to fix those paths.

On the tinderbox side I think we should remove the arguments to optimize when we add the flags exporting; the same arguments should apply to us, and the default for darwin was made O2 in that bug anyway, so I would expect the differences to be minor, but desirable.

Comment 2

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

Comment on attachment 377412 [details] [diff] [review]
Makefile change [landed]

r=ardissone

Comment 3

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

Attachment: static universal release tinderbox mozconfig changes [landed]

> On the tinderbox side I think we should remove the arguments to optimize when
> we add the flags exporting; the same arguments should apply to us, and the
> default for darwin was made O2 in that bug anyway, so I would expect the
> differences to be minor, but desirable.

Like so?  This just does the three static universal release configs (no one's building maya's right now, but it won't hurt).

Comment 4

[Stuart Morgan]

Comment on attachment 377459 [details] [diff] [review]
static universal release tinderbox mozconfig changes [landed]

Looks right to me, and I landed the Makefile change, so this should be good to land.

Comment 5

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

Comment on attachment 377459 [details] [diff] [review]
static universal release tinderbox mozconfig changes [landed]

Landed, so tonight's nightly should have better symbols.

We'll need to ask mento if we should port the --enable-optimize change to the shared opt configs, too.

Leaving this bug open for possible symbols improvements for the Core dylibs (middle paragraph in comment 1).

Comment 6

[Stuart Morgan]

Somehow this didn't work on the tinderbox like it did on my machine. I'll have to try to compare the builds and see if I can figure out why.

Comment 7

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

I'm not positive this is relevant here, but just tossing it out: 

For the longest time I thought the switch to dwarf in Core depended on lots of Mozilla code changes to get it working, but re-reading bug 421534 and bug 465329, it looks like it was just shebs writing the breakpad dwarf support in the one bug, and breakpad upgrade+moz_crashreporter integration tweaks in the other, no other Core changes, as far as I can tell.  The mozilla-central/1.9.1 versions of symbolstore.py have additional dSYM support (including support for pulling dSYMs for debugging, bug 420474); we could apply your fork to it and land the new, improved fork.

All of which is just a long-winded way of saying, why not just go "-gdwarf-2" instead, and have dSYMs all around?  (This is also me assuming that passing "-gdwarf-2" will cause the right thing to happen when building core files, but maybe not.)

Comment 8

[Stuart Morgan]

That could certainly be worth a shot; I just assumed there had been a bunch of build changes too.

I wonder though if we are running into something along the lines of bug 448616 (see the latter part of the bug); maybe the libraries are being stripped before we run our symbol step. If it's something happening as part of the tinderbox script, that would explain why it worked on my machine.

I'll dig through the logs tomorrow, but I'll also play with doing a dSYM-based build and see what happens, since even if it is a stripping order issue using dSYM should sidestep that.

Comment 9

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

(In reply to comment #8)
> I wonder though if we are running into something along the lines of bug 448616
> (see the latter part of the bug); maybe the libraries are being stripped before
> we run our symbol step. If it's something happening as part of the tinderbox
> script, that would explain why it worked on my machine.

Oh, that could be; errant stripping was one of my thoughts.  I had thought I had seen (in going through a million bugs) a patch to flight.mk that was supposed to have avoided that issue, though.

Wait, that would only explain the Core dylibs and not the current problem, since we're generating the Camino.app dSYMs per-arch, well before unification, right? (Also, I'm pretty sure Talkback symbol generation runs at the same point, but I haven't pored through both logs or that fun perl recently, and it's certainly possible someone inserted the new code in the wrong place.)

Comment 10

[Stuart Morgan]

You're right, that's nonsense.

I have a new theory though: I think I'm an idiot. On Leopard, the default debugging format is DWARF. On Tiger, I'm almost positive it's stabs. So on my machine, -gfull generates DWARF symbols for all the core static libraries, then when we link those in to Camino and Xcode runs dsymutil to generate the dSYM file, we get everything. But on our (Tiger) build machine, we (if I'm right) get stabs symbols in the static libs, and when dsymutil runs it naturally ignores them.

Assuming that's correct, then we should should do comment 7, which would be like the build I'm doing locally but with good dylib symbols (which I do get if I manually run the newer symbolstore script on the core part of one of my builds). The only question at that point will be whether the DWARF support is good enough in Tiger; I'm not sure if the core build was ever tested with that combo. We'll see ;)

(Also, just to round out comment 8 being worthless, -gdwarf-2 isn't a "dSYM-based" build, it's integrated DWARF; the dSYM creation is a separate step that, in the case of core builds, is done by symbolstore.py rather than the build.)

Comment 11

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

(In reply to comment #10)
> The only question at that point will be whether the DWARF support is
> good enough in Tiger; I'm not sure if the core build was ever tested with that
> combo. We'll see ;)

SeaMonkey machines were all 10.4-based until KaiRo got a new Xserve recently, and I didn't see any more mentions of Sm issues after he was told to upgrade to Xcode 2.5.  So long as there really weren't Core build/source changes (outside of breakpad) required for DWARF support, I think we should be good. :)

Comment 12

[Stuart Morgan]

Okay, so the last hurdle is that the new symbolstore.py expects to be either making .dSYMs or finding it where it would make them. Our build layout is unexpected to it, so it's unhappy.

I'll spend some time seeing if we can massage things into the right arrangement for it in a temp folder. (Of course we *could* land it as a separate copy and use the old one on our stuff and the new one on core, but since that's stupid it's my option of last resort).

Comment 13

[Stuart Morgan]

Okay, I think I've beaten it this time. The approach is:
1) Switch to -gdwarf-2 in the tinderbox scripts
2) Turn off copy-on-strip in Release.xcconfig
3) Update our symbolstore.py to a modified version of the trunk copy, rather than the branch copy.

Part of (3) is making it understand that if we give it the contents of a dSYM bundle as a parameter (as we currently do in our Makefile) that it should use that, which is how I got around comment 12. A bit hacky, since it means we still can't just turn it loose on a directory, but it works. (Another option here would be to switch us from dwarf-with-dsym to dwarf, and let symbolstore.py do its own dSYM generation as it was designed to do; I don't have a good argument for the way I chose, it just seemed less likely to cause some other unexpected problem to pop up in the game of whack-a-mole that this whole process has turned into. Oh, and the Breakpad build is set up for dSYM, so this way let us feed those dSYMs in instead of mucking with their build.)

Another part of (3) is the change I describe in bug 493392, which means no regressions for the non-DWARF dylibs and frameworks. Yay! (We still might want to hack Sparkle and Growl into doing DWARF; not sure it's worth it.)

(2) is optional, since we could instead run on the dylibs in their original locations. That seemed like more pain though, since I don't think we want symbols for everything in dist/bin, and maintaining a list by hand would suck. Packaging strips anyway, so there's no harm I can think of.

This is all working on my Tiger machine, so I think we're good to land it once the stars align (we need to land my stuff and change the tinderbox at the same time).

Comment 14

[Stuart Morgan]

Attachment: symbolstore diff from m-c

Since the CVS diff will be confusing, here's a diff of the new symbolstore.py I'm going to land as compared to the mozilla-central trunk version of symbolstore.py I based it on.

Comment 15

[Stuart Morgan]

Attachment: config change

And for completeness, here's the change to Release.xcconfig to prevent stripping libraries and frameworks as we copy them.

Comment 16

[Stuart Morgan]

Landed parts (2) and (3) on CVS trunk. Once Smokey does (1), we can see what happens :)

Comment 17

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

Attachment: static universal release tinderbox mozconfig changes for DWARF [landed]

For extra completeness, here's the tinderbox mozconfig changes we landed.

Comment 18

[Stuart Morgan]

symbolstore.py needed another small tweak, which I landed, to remove an import of a module that's not part of the Tiger python distribution. If we ever try to use --copy_debug, it will explode, but hopefully we won't want it before we're on Leopard.

(On the offchance we do, we'd probably need to do a macports installation of python--turns out that's why I didn't discover this problem in my testing.)

Comment 19

[Stuart Morgan]

We win! Symbols for static core libraries are now good, symbols for now-DWARF things are the same. If we decide we want improved symbols for Sparkle and/or Growl, we'll file a new bug.

Comment 20

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

bp-32946d73-810a-44c6-8173-497332090518
http://pastebin.mozilla.org/651716

See also http://symbols.mozilla.org/symbols_camino/Camino/5C8DB473B0CE4536B586C658A3FFCCD30/Camino.sym

Verified win!

Comment 21

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

As an added bonus, it looks like we got about a 90ms Ts win on cb-xserve01 from this (due to Xcode stripping out symbols when making the dSYM, or to DWARF being more efficient than stabs, or...something).  

The bundle is also about 1 MB smaller than the build I have on hand from last week.

Comment 22

[philippe (part-time)]

Js performance also improved a bit. Removing the '-02' flag out of the mozconfig made the  biggest difference, I suspect. 
As a sample, below are some results for the Sunspider test suite. Other js text improved as well.

http://www2.webkit.org/perf/sunspider-0.9/sunspider.html
best 3 out of 10 runs

build
0514	3795.4ms	3784.0ms	3783.6ms	// before the changes
0515	3689.4ms	3675.2ms	3667.8ms

0517 	3664.6ms	3688.6ms	3662.0ms
0518	3633.2ms	3624.2ms	3628.0ms

GP0518: 3536.4ms	3537.0ms	3524.0ms

GP0518 = Firefox 3.011pre 05/18


Are there any other flags in the Camino build process that may override or conflict with low level optimisations deep inside Gecko ?

Comment 23

[Stuart Morgan]

(In reply to comment #19)
> If we decide we want improved symbols for Sparkle and/or
> Growl, we'll file a new bug.

I filed bug 495513 to track that.