Closed Bug 378463 Opened 13 years ago Closed 13 years ago

build time logic to extract debug symbols on OS X


(Toolkit :: Crash Reporting, defect)

Not set





(Reporter: ted, Assigned: ted)




(1 file, 4 obsolete files)

The current build logic only works on Win32.  Shebs, I know you looked at this, do you have anything you could post here as a starting point?
Ok, this patch works for me.  It makes us build the dump_syms tool, and use it in |make buildsymbols|.  I had to tweak a few things in, since the .sym file MODULE line changed a bit.  That means I'll have to checkin a new dump_syms.exe for Windows as well.  I don't know how this interacts with Universal builds.  I do know that the OS X dump_syms offers an architecture flag, so you can call it with |-a ppc| or |-a x86|, so it might be that we need to run it one per arch.  Mark, any insight you can offer on that?
A universal build is really just two thin builds.  Figure out if you're targeting x86 or ppc and always call dump_syms with an architecture argument.  You should also make sure that your dump_syms is built with the host tools, not the target tools, to ensure that it'll run on a ppc build host during the x86 cross portion of a universal build.
Yeah, I figured I needed to change that to a HOST_PROGRAM.  My only concern is that we're running dump_syms in a post-build step:

So how do we know what objdir to make in?
Depends on: 378926
Nevermind, bsmedberg pointed out this lovely bit of Tinderbox code to me:

So my current plan is:
1) In mozilla/, detect universal builds, and pass -a ppc,i386 to
2) In, loop running dump_syms for each arch specified after -a, and pass that to dump_syms.

If someone is doing a non-universal build, and I don't pass -a to dump_syms, it will just do the right thing, right?
Attached patch with universal build support (obsolete) — Splinter Review
Ok, this adds universal build support.  dump_syms is built as a HOST_PROGRAM, the list of architectures is passed to, and it calls dump_syms once per arch.  In addition, this patch changes the UB to not strip the binaries when they are merged into a UB, so we can dump the UB.  They will still get stripped in the final packaging.
Attachment #262878 - Attachment is obsolete: true
Attachment #263047 - Flags: review?(mark)
Blocks: 379117
Attached patch with universal build support (obsolete) — Splinter Review
Whoops, forgot to diff new files.  As I mentioned before, I will also need to checkin a new dump_syms.exe along with this patch.
Attachment #263047 - Attachment is obsolete: true
Attachment #263142 - Flags: review?(mark)
Attachment #263047 - Flags: review?(mark)
Comment on attachment 263142 [details] [diff] [review]
with universal build support

>+SYM_FIND_CMD := /bin/find . -path "./dist" -prune -o -name "*.exe" \

You don't need quotes around ./dist any more than you'd need them around, say, -prune.  You also don't need the ./ .

>+ -o -name "*.dll" -o -name "*.EXE" | sed "s/\.[^\.]*$$/\.pdb/"

Indent a bit more to make it clearer that it's a continuation?

>+# we want to copy PDB files on Windows
>+DUMP_SYMS_BIN := $(topsrcdir)/toolkit/airbag/tools/win32/dump_syms.exe
>+ifeq ($(OS_ARCH),Darwin)
>+# need to pass arch flags for universal builds
>+MAKE_SYM_STORE_ARGS := -a "ppc i386"
>+SYM_DIST := $(DIST)/universal

This might not work with a pure cross build - say you're on a ppc host building for x86, the ppc dump_syms will try to dump ppc, but there is no ppc code to dump.  How about setting MAKE_SYM_STORE_ARGS to -a whatever depending on the target architecture in the non-UNIVERSAL_BINARY case?

>Index: toolkit/airbag/tools/
> # file.pdb/GUID+age/file.sym

We've taken to referring to what we used to call guid+age or uuid+age as "identifier" now, since there might not be uuid or guid components at all.  You may want to say so somewhere.
Attachment #263142 - Flags: review?(mark) → review+
Attached patch with mark's comments (obsolete) — Splinter Review
Addressed Mark's comments, I use -a $(OS_TEST) in the non-universal case.  I need to make sure this still works on Win32, and then I'll check it in.
Attachment #263142 - Attachment is obsolete: true
Checked in.
Closed: 13 years ago
Resolution: --- → FIXED
I backed this out due to bustage.

~/firefox/mozilla> cvs commit -m "backout patch for bug 378463 due to bustage"  toolkit/airbag/airbag/src/tools/mac/dump_syms toolkit/xre/ build/macosx/universal/ toolkit/airbag/tools/ toolkit/airbag/airbag/src/common/mac/ toolkit/airbag/airbag/src/common toolkit/airbag/ toolkit/airbag/tools/win32/dump_syms.exe 
cvs commit: Examining toolkit/airbag/airbag/src/tools/mac/dump_syms
cvs commit: Examining toolkit/airbag/airbag/src/tools/mac/dump_syms/dump_syms.xcodeproj
cvs commit: Examining toolkit/airbag/airbag/src/common
cvs commit: Examining toolkit/airbag/airbag/src/common/linux
cvs commit: Examining toolkit/airbag/airbag/src/common/mac
cvs commit: Examining toolkit/airbag/airbag/src/common/windows
Checking in toolkit/xre/;
/cvsroot/mozilla/toolkit/xre/,v  <--
new revision: 1.71; previous revision: 1.70
Checking in build/macosx/universal/;
/cvsroot/mozilla/build/macosx/universal/,v  <--
new revision: 1.6; previous revision: 1.5
Checking in toolkit/airbag/tools/;
/cvsroot/mozilla/toolkit/airbag/tools/,v  <--
new revision: 1.4; previous revision: 1.3
Checking in toolkit/airbag/airbag/src/common/mac/;
/cvsroot/mozilla/toolkit/airbag/airbag/src/common/mac/,v  <--
new revision: 1.3; previous revision: 1.2
Checking in toolkit/airbag/;
/cvsroot/mozilla/toolkit/airbag/,v  <--
new revision: 1.8; previous revision: 1.7
Checking in;
/cvsroot/mozilla/,v  <--
new revision: 1.372; previous revision: 1.371
Checking in toolkit/airbag/tools/win32/dump_syms.exe;
/cvsroot/mozilla/toolkit/airbag/tools/win32/dump_syms.exe,v  <--  dump_syms.exe
new revision: 1.3; previous revision: 1.2
Removing toolkit/airbag/airbag/src/tools/mac/dump_syms/;
/cvsroot/mozilla/toolkit/airbag/airbag/src/tools/mac/dump_syms/,v  <--
new revision: delete; previous revision: 1.1
Removing toolkit/airbag/airbag/src/common/;
/cvsroot/mozilla/toolkit/airbag/airbag/src/common/,v  <--
new revision: delete; previous revision: 1.1
Resolution: FIXED → ---
Attached patch also fix testSplinter Review
This fixes the test Makefile as well, which is what caused the bustage last night.  Checked in again.
Attachment #263363 - Attachment is obsolete: true
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.