Last Comment Bug 391361 - The integration of breakpad on Solaris
: The integration of breakpad on Solaris
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Breakpad Integration (show other bugs)
: unspecified
: x86 SunOS
: -- normal (vote)
: ---
Assigned To: Alfred Peng
:
: Ted Mielczarek [:ted.mielczarek]
Mentors:
Depends on: 391359 423674
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-08 06:32 PDT by Alfred Peng
Modified: 2008-03-20 00:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
broken patch (11.09 KB, patch)
2007-10-05 09:45 PDT, Alfred Peng
no flags Details | Diff | Splinter Review
full patch that breaks (19.36 KB, patch)
2007-10-14 00:27 PDT, Alfred Peng
no flags Details | Diff | Splinter Review
Patch v1 (25.34 KB, patch)
2007-11-01 21:00 PDT, Alfred Peng
no flags Details | Diff | Splinter Review
patch v2 (21.91 KB, patch)
2007-12-10 05:50 PST, Alfred Peng
ted: review-
Details | Diff | Splinter Review
The patch for breakpad (24.08 KB, patch)
2007-12-10 05:52 PST, Alfred Peng
no flags Details | Diff | Splinter Review
patch v3 (22.44 KB, patch)
2007-12-20 05:23 PST, Alfred Peng
ted: review+
Details | Diff | Splinter Review
patch v4 (24.23 KB, patch)
2008-03-18 12:23 PDT, Alfred Peng
ted: review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description Alfred Peng 2007-08-08 06:32:40 PDT
The Solaris port of breakpad client is available. The next step is to integrate this with Firefox itself.

luser, could you please give us a list of things to do next? Thanks.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2007-08-08 08:04:50 PDT
Ok, so once bug 391359 lands, you'll need to:

1) Add Makefile.ins in:
  src/common/solaris/
  src/client/solaris/handler/

2) Set DIRS to build the right set of code like this does for Linux:
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/Makefile.in#75

3) Make libxul link to the right set of libraries:
http://mxr.mozilla.org/mozilla/source/toolkit/xre/Makefile.in#211
also make the same changes in the test Makefile:
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/test/Makefile.in#88

4) Tweak the includes in nsCrashReporter.cpp:
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/nsExceptionHandler.cpp#55
You'll also want to verify that the XP_UNIX code paths in the rest of that file work properly.

I notice your Breakpad patch doesn't include a dump_syms tool or a crash report sender library.  Can you use the Linux ones, or is this something you still need to implement?  Either way:

5) Modify client makefile:
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/client/Makefile.in#82
6) Possibly add a crashreporter_solaris.cpp, unless the Linux client works, in which case you can just use that.

For dump_syms support:
7) Modify symbolstore.py to know about solaris:
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/tools/symbolstore.py#128
You may be able to just use the Dumper_Linux class in there, it looks for executable files or *.so files, and then runs file(1) on them and checks that the output starts with "ELF".
8) Add a Solaris case in mozilla/Makefile.in, might be able to combine it with the Linux case:
http://mxr.mozilla.org/mozilla/source/Makefile.in#152

9) Once everything is done and working, you may want to enable building it by default on Solaris:
http://mxr.mozilla.org/mozilla/source/configure.in#5316

I think that's a fairly exhaustive list, but you may find other things as you proceed.
Comment 2 Alfred Peng 2007-10-05 09:45:00 PDT
Created attachment 283722 [details] [diff] [review]
broken patch

luser, got the following error after applying the patch and start firefox:

ld.so.1: firefox-bin: fatal: /usr/lib/libcairo.so.2: Invalid argument
ld.so.1: firefox-bin: fatal: relocation error: file /usr/lib/libgtk-x11-2.0.so.0: symbol cairo_surface_destroy: referenced symbol not found
ld.so.1: firefox-bin: fatal: /usr/lib/libgdk-x11-2.0.so.0: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libgdk_pixbuf-2.0.so.0: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libgobject-2.0.so.0: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libglib-2.0.so.0: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libgthread-2.0.so.0: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libelf.so.1: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libXau.so.6: Invalid argument
ld.so.1: firefox-bin: fatal: /usr/lib/libXext.so.0: Invalid argument
ld.so.1: firefox-bin: fatal: relocation error: file ./dist/bin/libxul.so: symbol __1cH__rwstdPrwse_OutOfRange_: referenced symbol not found
ld.so.1: firefox-bin: fatal: ./dist/bin/libxul.so: Invalid argument
ld.so.1: firefox-bin: fatal: relocation error: file dist/bin/firefox-bin: symbol XRE_GetBinaryPath: referenced symbol not found

Any hint for this error? Do I miss something?
Comment 3 Ted Mielczarek [:ted.mielczarek] 2007-10-05 10:00:08 PDT
That's not anything I'm familiar with, sorry.
Comment 4 Alfred Peng 2007-10-14 00:27:38 PDT
Created attachment 284814 [details] [diff] [review]
full patch that breaks
Comment 5 Alfred Peng 2007-11-01 21:00:58 PDT
Created attachment 287046 [details] [diff] [review]
Patch v1

I'll take several weeks' leave. In case someone feels interested in having a try. This patch is the one that can work with Firefox trunk. It also includes the sub patch which I've already submitted an issue for breakpad.
Comment 6 Alfred Peng 2007-12-10 05:50:21 PST
Created attachment 292393 [details] [diff] [review]
patch v2

One limitation: In order to upload the correct symbols information to the server, we can't make the build in the topsrcdir directory. A different directory is fine. 
If we make the build in the topsrcdir, the generated dynamic libraries only contain the file name information, not the path information. This will bring some trouble when getting the Revision information in symbolstore.py.
Comment 7 Alfred Peng 2007-12-10 05:52:32 PST
Created attachment 292394 [details] [diff] [review]
The patch for breakpad

This patch is also necessary and has been posted to issue 223 of breakpad for review.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2007-12-19 13:55:46 PST
Comment on attachment 292393 [details] [diff] [review]
patch v2

>Index: Makefile.in
>===================================================================
> ifeq ($(OS_ARCH),Linux)
> MAKE_SYM_STORE_ARGS := --vcs-info
> DUMP_SYMS_BIN ?= $(DIST)/host/bin/dump_syms
> MAKE_SYM_STORE_PATH := $(DIST)/bin
> endif
>+ifeq ($(OS_ARCH),SunOS)
>+MAKE_SYM_STORE_ARGS := --vcs-info
>+DUMP_SYMS_BIN ?= $(DIST)/host/bin/dump_syms
>+MAKE_SYM_STORE_PATH := $(DIST)/bin
>+endif

We should probably just combine these two blocks if they're going to use the exact same options.  You can do this like:

ifeq (,$(filter-out Linux SunOS,$(OS_ARCH)))

>Index: toolkit/crashreporter/nsExceptionHandler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/crashreporter/nsExceptionHandler.cpp,v
>retrieving revision 1.30
>diff -u -p -8 -r1.30 nsExceptionHandler.cpp
>--- toolkit/crashreporter/nsExceptionHandler.cpp	12 Nov 2007 18:53:52 -0000	1.30
>+++ toolkit/crashreporter/nsExceptionHandler.cpp	10 Dec 2007 12:51:59 -0000
>@@ -52,37 +52,42 @@
> #include <sys/types.h>
> #include <unistd.h>
> #include "mac_utils.h"
> #elif defined(XP_LINUX)
> #include "client/linux/handler/exception_handler.h"
> #include <fcntl.h>
> #include <sys/types.h>
> #include <unistd.h>
>+#elif defined(XP_SOLARIS)
>+#include "client/solaris/handler/exception_handler.h"
>+#include <fcntl.h>
>+#include <sys/types.h>
>+#include <unistd.h>
> #else
> #error "Not yet implemented for this platform"
> #endif // defined(XP_WIN32)

Again, can you combine these blocks if they're exactly the same?  No point in having multiple copies.  Should just be:
#if defined(XP_LINUX) || defined(XP_SOLARIS)

 
>-#ifndef HAVE_CPP_2BYTE_WCHAR_T
>-#error "This code expects a 2 byte wchar_t.  You should --disable-crashreporter."
>-#endif
>-

I see that you moved this down into the WIN32 block.  I think you can just remove it altogether now.  It's outlived its usefulness.

>Index: toolkit/crashreporter/client/Makefile.in
>===================================================================
>+ifeq ($(OS_ARCH),SunOS)
>+CPPSRCS += crashreporter_linux.cpp

Maybe we should consider renaming this to crashreporter_gtk.cpp?

>+LIBS += \
>+  $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/solaris/$(LIB_PREFIX)breakpad_solaris_common_s.$(LIB_SUFFIX) \
>+  $(NULL)
>+LOCAL_INCLUDES += -I$(srcdir)
>+OS_CXXFLAGS += $(MOZ_GTK2_CFLAGS)
>+OS_LIBS += $(MOZ_GTK2_LIBS) -lcurl

You shouldn't need to explicitly link libcurl anymore.

> ifeq ($(OS_ARCH),Linux)
> export:: $(srcdir)/../google-breakpad/src/common/linux/http_upload.cc
> 	$(INSTALL) $^ .
> endif
>+
>+ifeq ($(OS_ARCH),SunOS)
>+export:: $(srcdir)/../google-breakpad/src/common/linux/http_upload.cc
>+	$(INSTALL) $^ .
>+endif

Again, can we combine these two blocks?


>Index: toolkit/crashreporter/tools/symbolstore.py
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/crashreporter/tools/symbolstore.py,v
>retrieving revision 1.8
>diff -u -p -8 -r1.8 symbolstore.py
>--- toolkit/crashreporter/tools/symbolstore.py	11 Oct 2007 21:54:03 -0000	1.8
>+++ toolkit/crashreporter/tools/symbolstore.py	10 Dec 2007 12:51:59 -0000
>@@ -273,16 +273,17 @@ def GetVCSFilename(file, srcdir):
>     return file.replace("\\", "/")
> 
> def GetPlatformSpecificDumper(**kwargs):
>     """This function simply returns a instance of a subclass of Dumper
>     that is appropriate for the current platform."""
>     return {'win32': Dumper_Win32,
>             'cygwin': Dumper_Win32,
>             'linux2': Dumper_Linux,
>+            'sunos5': Dumper_Linux,
>             'darwin': Dumper_Mac}[sys.platform](**kwargs)
> 
> class Dumper:
>     """This class can dump symbols from a file with debug info, and
>     store the output in a directory structure that is valid for use as
>     a Breakpad symbol server.  Requires a path to a dump_syms binary--
>     |dump_syms| and a directory to store symbols in--|symbol_path|.
>     Optionally takes a list of processor architectures to process from
>@@ -314,19 +315,23 @@ class Dumper:
> 
>     # subclasses override this
>     def ShouldProcess(self, file):
>         return False
> 
>     def RunFileCommand(self, file):
>         """Utility function, returns the output of file(1)"""
>         try:
>-            # we use -L to read the targets of symlinks,
>-            # and -b to print just the content, not the filename
>-            return os.popen("file -Lb " + file).read()
>+            if sys.platform == "sunos5":
>+              output = os.popen("file " + file).read()
>+              return output.split('\t')[1];
>+            else:
>+              # we use -L to read the targets of symlinks,
>+              # and -b to print just the content, not the filename
>+              return os.popen("file -Lb " + file).read()
>         except:
>             return ""

I don't really like this change.  Maybe you could subclass Dumper_Linux to Dumper_Solaris, and override this method in there?  That's more in the spirit of how this class is used.

> 
>     # This is a no-op except on Win32
>     def FixFilenameCase(self, file):
>         return file
> 
>     def Process(self, file_or_dir):
>@@ -375,16 +380,20 @@ class Dumper:
>                         pass
>                     f = open(full_path, "w")
>                     f.write(module_line)
>                     # now process the rest of the output
>                     for line in cmd:
>                         if line.startswith("FILE"):
>                             # FILE index filename
>                             (x, index, filename) = line.split(None, 2)
>+                            if sys.platform == "sunos5":
>+                              filename = os.path.join(self.srcdir,
>+                                                      re.sub("^.*mozilla/", "",
>+                                                             filename));

This is a definite no-no, I don't want to hardcode "mozilla" in this script.  What exactly are you trying to do here?

>Index: toolkit/crashreporter/google-breakpad/src/client/solaris/handler/Makefile.in
>===================================================================
>+# The Original Code is Mozilla Breakpad integration
>+#
>+# The Initial Developer of the Original Code is
>+# Ted Mielczarek <ted.mielczarek@gmail.com>

I think you get to claim credit here, and all the other files you've added.  :-)

>Index: toolkit/crashreporter/google-breakpad/src/common/solaris/Makefile.in
>===================================================================
>+CXXFLAGS := $(filter-out -pedantic,$(CXXFLAGS))

This sucks, what doesn't build with -pedantic?

r- for a few issues, but overall pretty close!
Comment 9 Alfred Peng 2007-12-20 04:32:41 PST
luser, thanks for the review. See my comments below:

(In reply to comment #8)
> > #elif defined(XP_LINUX)
> > #include "client/linux/handler/exception_handler.h"
> > #include <fcntl.h>
> > #include <sys/types.h>
> > #include <unistd.h>
> >+#elif defined(XP_SOLARIS)
> >+#include "client/solaris/handler/exception_handler.h"
> >+#include <fcntl.h>
> >+#include <sys/types.h>
> >+#include <unistd.h>
> > #else
> > #error "Not yet implemented for this platform"
> > #endif // defined(XP_WIN32)
> 
> Again, can you combine these blocks if they're exactly the same?  No point in
> having multiple copies.  Should just be:
> #if defined(XP_LINUX) || defined(XP_SOLARIS)
> 
The header file exception_handler.h are in different path between Linux and Solaris. I think we should keep them separate.

> >Index: toolkit/crashreporter/client/Makefile.in
> >===================================================================
> >+ifeq ($(OS_ARCH),SunOS)
> >+CPPSRCS += crashreporter_linux.cpp
> 
> Maybe we should consider renaming this to crashreporter_gtk.cpp?
> 
So we need a cvs copy request for this to keep all the log?

> This is a definite no-no, I don't want to hardcode "mozilla" in this script. 
> What exactly are you trying to do here?
> 
Take libmozjs.so as an example, the filename is in the form of "../../../mozilla/js/src/js*.c" when the topsrcdir is "../mozilla". This line of code helps kill the leading "../../*" and we can get the VCS info from it. Anyway, I'll get rid of the hardcode in the updated patch.

> >Index: toolkit/crashreporter/google-breakpad/src/common/solaris/Makefile.in
> >===================================================================
> >+CXXFLAGS := $(filter-out -pedantic,$(CXXFLAGS))
> 
> This sucks, what doesn't build with -pedantic?
Then how about the Makefile in Linux directory? Do we also need to remove it?

Will update the patch later.
Comment 10 Alfred Peng 2007-12-20 05:23:04 PST
Created attachment 294024 [details] [diff] [review]
patch v3
Comment 11 Ted Mielczarek [:ted.mielczarek] 2007-12-21 12:38:46 PST
Comment on attachment 294024 [details] [diff] [review]
patch v3

This looks good.  Let's not worry about renaming crashreporter_linux.cpp right now though, we can do that later.  (Maybe in the mercurial repository where it's easier.  :-)  Also, I didn't remember that we had that -pedantic filter in the Linux Makefile.  If we can build without it, I'd like to get it removed.
Comment 12 Alfred Peng 2008-03-18 12:23:33 PDT
Created attachment 310305 [details] [diff] [review]
patch v4

Update the latest patch to include the followup fix for bug 404855. With this patch applied, crashreporter_linux.cpp has to be renamed to crashreporter_gtk.cpp.
Comment 13 Ted Mielczarek [:ted.mielczarek] 2008-03-18 18:38:38 PDT
Comment on attachment 310305 [details] [diff] [review]
patch v4

Ok, this looks good. Let's leave the renaming of crashreporter_linux.cpp out for now, we can do that very easily in mozilla2, and it's not very important for right now.

Drivers: this patch will enable breakpad support on Solaris, and it's pretty low-risk, basically the entire patch is just adding Solaris-only paths to makefiles and one or two source files.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-18 20:07:01 PDT
Comment on attachment 310305 [details] [diff] [review]
patch v4

a1.9=beltzner
Comment 15 Alfred Peng 2008-03-20 00:15:29 PDT
checked in

Note You need to log in before you can comment on or make changes to this bug.