Closed Bug 378463 Opened 13 years ago Closed 13 years ago

build time logic to extract debug symbols on OS X

Categories

(Toolkit :: Crash Reporting, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(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 make_symbol_store.pl, 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:
http://mxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#1298

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:
http://mxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#1213

So my current plan is:
1) In mozilla/Makefile.in, detect universal builds, and pass -a ppc,i386 to make-symbol-store.pl
2) In make-symbol-store.pl, 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 make_symbol_store.pl, and it calls dump_syms once per arch.  In addition, this patch changes the UB flight.mk 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
>+MAKE_SYM_STORE_ARGS := -c
>+DUMP_SYMS_BIN := $(topsrcdir)/toolkit/airbag/tools/win32/dump_syms.exe
>+endif
>+ifeq ($(OS_ARCH),Darwin)
>+# need to pass arch flags for universal builds
>+ifdef UNIVERSAL_BINARY
>+MAKE_SYM_STORE_ARGS := -a "ppc i386"
>+SYM_DIST := $(DIST)/universal
>+else
>+MAKE_SYM_STORE_ARGS :=
>+SYM_DIST := $(DIST)
>+endif

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/make_symbol_store.pl
> # 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.
Status: NEW → RESOLVED
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/Makefile.in build/macosx/universal/flight.mk toolkit/airbag/tools/make_symbol_store.pl toolkit/airbag/airbag/src/common/mac/Makefile.in toolkit/airbag/airbag/src/common toolkit/airbag/Makefile.in Makefile.in 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/Makefile.in;
/cvsroot/mozilla/toolkit/xre/Makefile.in,v  <--  Makefile.in
new revision: 1.71; previous revision: 1.70
done
Checking in build/macosx/universal/flight.mk;
/cvsroot/mozilla/build/macosx/universal/flight.mk,v  <--  flight.mk
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/airbag/tools/make_symbol_store.pl;
/cvsroot/mozilla/toolkit/airbag/tools/make_symbol_store.pl,v  <--  make_symbol_store.pl
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/airbag/airbag/src/common/mac/Makefile.in;
/cvsroot/mozilla/toolkit/airbag/airbag/src/common/mac/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/airbag/Makefile.in;
/cvsroot/mozilla/toolkit/airbag/Makefile.in,v  <--  Makefile.in
new revision: 1.8; previous revision: 1.7
done
Checking in Makefile.in;
/cvsroot/mozilla/Makefile.in,v  <--  Makefile.in
new revision: 1.372; previous revision: 1.371
done
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
done
Removing toolkit/airbag/airbag/src/tools/mac/dump_syms/Makefile.in;
/cvsroot/mozilla/toolkit/airbag/airbag/src/tools/mac/dump_syms/Makefile.in,v  <--  Makefile.in
new revision: delete; previous revision: 1.1
done
Removing toolkit/airbag/airbag/src/common/Makefile.in;
/cvsroot/mozilla/toolkit/airbag/airbag/src/common/Makefile.in,v  <--  Makefile.in
new revision: delete; previous revision: 1.1
done
Status: RESOLVED → REOPENED
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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.