Closed Bug 383083 Opened 17 years ago Closed 17 years ago

post-process symbol files to add source file revision numbers

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(4 files, 1 obsolete file)

Breakpad issue 180 (linked in the URL) is about supporting an optional revision number on the FILE lines of symbol files, and printing them out in minidump_stackwalk.  Assuming that lands in breakpad, we should do some post-processing in our buildsymbols step to add the current revision numbers to the FILE lines.  This way, we can easily add Bonsai links in Socorro.
So, turns out we don't really need to change anything in Breakpad, since it will cheerfully pass along any data we stick after the filename as part of the filename.
Attached file make_symbol_store.py (obsolete) —
Ok, here's make_symbol_store.py, which is make_symbol_store.pl reimplemented in Python, but with a little extra functionality.  It can be used just like the Perl script, but it also has a few extra tricks:
- It can take directories instead of just filenames, and it will walk them and process any acceptable files.  The logic for this is split up by platform.  This means we can ditch some of the `find` goop in the Makefile.
- It will process the FILE lines in the dump_syms output, fixing the case of Win32 filenames when possible.  (The file must exist on the local disk)
- It will grab CVS root and revision number from CVS/{Root,Entries} if they exist, and munge the filename to add them in.  Filenames come out like:
cvs:cvs.mozilla.org/cvsroot:mozilla/browser/app/nsBrowserApp.cpp:1.36
- It supports a --srcdir argument, to get nice relative pathnames in the output, like the example immediately above.

I haven't tested this in cygwin (it's the same Python), but bsmedberg tells me that $(PYTHON) translates its arguments to windows pathnames.
(In reply to comment #2)
> cvs:cvs.mozilla.org/cvsroot:mozilla/browser/app/nsBrowserApp.cpp:1.36

Will cvs-mirror vs. cvs matter in any of the scripts that might use this data?
(In reply to comment #3)
> Will cvs-mirror vs. cvs matter in any of the scripts that might use this data?

You know, bsmedberg asked that same question.  Do any of our official builds use cvs-mirror?  Even so, I hope it will be trivial on the server side to setup a mapping for cvs.mo or cvs-mirror.mo to point to the same bonsai links.

Comment on attachment 267451 [details]
make_symbol_store.py

So at least fx-win32-tbox uses cygwin Python, which runs this script okay, except it doesn't actually handle the point of this bug, because the FILE lines in dump_syms output are native windows paths, and cygwin python can't handle those.

:-(
Well, this works great on msys, Linux, and OS X.
Depends on: 383499
Comment on attachment 267480 [details] [diff] [review]
integrated into buildsymbols [checked in]

Might as well get this in, since it will work on OS X and Linux, and it would be functionally equivalent on Windows.
Attachment #267480 - Flags: review?(benjamin)
Comment on attachment 267480 [details] [diff] [review]
integrated into buildsymbols [checked in]

>Index: toolkit/airbag/tools/symbolstore.py

>+# The Initial Developer of the Original Code is
>+# Ted Mielczarek <ted.mielczarek@gmail.com>

I believe that this should be "The Mozilla Foundation" for MoCo employees, and we can put our individual names below in the Contributors section.

>+# Utility functions
>+
>+def GetCVSRevision(file):

I think we might want to add logging to stderr when this fails, and GetCVSRoot below.
Attachment #267480 - Flags: review?(benjamin) → review+
Comment on attachment 267480 [details] [diff] [review]
integrated into buildsymbols [checked in]

Checked in.  As I mentioned, this won't actually fix this bug on Windows, since cygwin python won't handle the Win32 paths properly.
Depends on: 386847
Moved this patch over from bug 386212.  The patch there made getting the VCS info an option, and only enabled it on OS X.  This patch fixes the most commonly encountered warnings on Linux and re-enables it there.

I noticed while making this patch that symbolstore.py has DOS line endings.  :-(  I'll clean that up when I check this in if that's ok.
Attachment #271545 - Flags: review?(benjamin)
Attachment #271545 - Flags: review?(benjamin) → review+
Attachment #271545 - Attachment description: re-enable --vcs-info on linux → re-enable --vcs-info on linux [checked in]
Attachment #267480 - Attachment description: integrated into buildsymbols → integrated into buildsymbols [checked in]
Attachment #267451 - Attachment is obsolete: true
Now that the Firefox tinderbox is on the new refplatform using MSYS, I'd like to get it using --vcs-info without breaking the other cygwin tinderboxes.
Attachment #277566 - Flags: review?(mark)
Attachment #277566 - Flags: review?(mark) → review+
Checked in the last patch, so this bug is finally FIXED!
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Attachment #277566 - Attachment description: re-enable --vcs-info on win32/msys → re-enable --vcs-info on win32/msys [checked in]
Turns out this didn't work perfectly on the tinderbox because the srcdir has mixed case directories in its path, and my code didn't handle that properly.  This patch fixes that bug.
Attachment #277720 - Flags: review?(mark)
Comment on attachment 277720 [details] [diff] [review]
handle mixed-case srcdir on win32 [checked in]

Ted, is this in cross-platform code?  Most Unices don't have case-insensitive-preserving filesystems.

If we don't care, it should at least be noted as a comment.
It is in cross-platform code, but I'm not really worried about this doing the wrong thing on other platforms, since that would only happen if the file was in a path identical to the srcdir path except for case.  I can add a comment, or I could pull that out into a method and only use the case-insensitive code on Win32, whichever would make you feel better
Comment on attachment 277720 [details] [diff] [review]
handle mixed-case srcdir on win32 [checked in]

Put the comment in (and note that the \-to-/ change also only makes sense on Windows).  The default fs for the Mac is case-insensitive-preserving, too.  r=me.
Attachment #277720 - Flags: review?(mark) → review+
Comment on attachment 277720 [details] [diff] [review]
handle mixed-case srcdir on win32 [checked in]

Checked in with comment fixes, thanks.
Attachment #277720 - Attachment description: handle mixed-case srcdir on win32 → handle mixed-case srcdir on win32 [checked in]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.