Closed Bug 391361 Opened 17 years ago Closed 16 years ago

The integration of breakpad on Solaris

Categories

(Toolkit :: Crash Reporting, defect)

x86
SunOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: alfred.peng, Assigned: alfred.peng)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
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.
Depends on: 391359
Attached patch broken patch (obsolete) — Splinter Review
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?
That's not anything I'm familiar with, sorry.
Attached patch full patch that breaks (obsolete) — Splinter Review
Attachment #283722 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → alfred.peng
Attachment #284814 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #287046 - Attachment is obsolete: true
Attachment #292393 - Flags: review?(ted.mielczarek)
This patch is also necessary and has been posted to issue 223 of breakpad for review.
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!
Attachment #292393 - Flags: review?(ted.mielczarek) → review-
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.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #292393 - Attachment is obsolete: true
Attachment #294024 - Flags: review?(ted.mielczarek)
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.
Attachment #294024 - Flags: review?(ted.mielczarek) → review+
Attached patch patch v4Splinter Review
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.
Attachment #294024 - Attachment is obsolete: true
Attachment #310305 - Flags: review?(ted.mielczarek)
Depends on: 423674
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.
Attachment #310305 - Flags: review?(ted.mielczarek)
Attachment #310305 - Flags: review+
Attachment #310305 - Flags: approval1.9?
Comment on attachment 310305 [details] [diff] [review]
patch v4

a1.9=beltzner
Attachment #310305 - Flags: approval1.9? → approval1.9+
checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.