510920 - Sync to breakpad revision 350 for breakpad fix 248, 10.5+ SDK compat

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Josh Aas]

I'd like to pick up the fix for breakpad bug 248 which allows building against the Mac OS X 10.5+ SDKs. Without that fix people run into build failures quite often and need to go back and disable breakpad. We also need this if we are going to move to 10.5+ SDK builds on trunk and for 64-bit builds.

http://code.google.com/p/google-breakpad/issues/detail?id=248

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Josh Aas]

Attachment: fix v1.1

Includes my fix for the following breakpad bug:

http://code.google.com/p/google-breakpad/issues/detail?id=330

Comment 3

[(not currently active) Ted Mielczarek]

I did some fixup on this patch, but my Linux build is still failing because some of the upstream files don't compile with -pedantic. I think the faster path to success here will be to just take a revision prior to the linux client rewrite, so we get the 10.5 SDK fixes but don't have to deal with all this Linux stuff yet. I'll update us to the latest code at a later date and deal with the issues, since they may require some upstream fixes.

Josh: can you regenerate this patch from svn revision 350? That would be prior to the linux rewrite, but would pick up my fix for http://code.google.com/p/google-breakpad/issues/detail?id=323, which would be a nice bonus.

One side note, I forgot to mention that when syncing from upstream, I leave out the "Makefile" files from the linux/ dirs, since they'd confuse our build system. I stripped a few of those out of your patch before testing.

Comment 4

[(not currently active) Ted Mielczarek]

Updating summary to narrow scope.

Comment 5

[Josh Aas]

Attachment: fix v1.2

Breakpad revision 350 patch.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 398432 [details] [diff] [review]
fix v1.2

diff --git a/toolkit/crashreporter/google-breakpad/src/client/linux/handler/Makefile b/toolkit/crashreporter/google-breakpad/src/client/linux/handler/Makefile
new file mode 100644
diff --git a/toolkit/crashreporter/google-breakpad/src/client/solaris/handler/Makefile b/toolkit/crashreporter/google-breakpad/src/client/solaris/handler/Makefile
new file mode 100644
diff --git a/toolkit/crashreporter/google-breakpad/src/tools/linux/dump_syms/Makefile b/toolkit/crashreporter/google-breakpad/src/tools/linux/dump_syms/Makefile
new file mode 100644
diff --git a/toolkit/crashreporter/google-breakpad/src/tools/linux/symupload/Makefile b/toolkit/crashreporter/google-breakpad/src/tools/linux/symupload/Makefile
new file mode 100644
diff --git a/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/Makefile b/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/Makefile
new file mode 100644

Please leave these files out of the patch, they're likely to confuse our build system.

Looks fine otherwise, thanks!

Comment 7

[(not currently active) Ted Mielczarek]

This is the fix that we have locally that's not upstreamed:
http://hg.mozilla.org/mozilla-central/rev/cfef2762ecb3

Comment 8

[Josh Aas]

That fix appears to have been upstreamed, breakpad svn revision 313 so we pick it up when updating to revision 350.

Comment 9

[Josh Aas]

pushed to mozilla-central with ted's comments addressed

http://hg.mozilla.org/mozilla-central/rev/e8c01867056a

Comment 10

[Josh Aas]

backed out because of this:

exception_handler.cc:181: error: ‘memset’ was not declared in this scope

I assume we just need to include string.h in that file but why hasn't anyone else ran into this?

Comment 11

[Josh Aas]

Attachment: fix v1.3

This patch, the one I pushed to mozilla-central, compiled on the try server but failed due to the memset problem mentioned above on mozilla-central.

Comment 12

[(not currently active) Ted Mielczarek]

Could be the switch to gcc 4.3 that changed today, but I thought we were good to go with that.

Comment 13

[(not currently active) Ted Mielczarek]

I mean, I thought jimb's patch here (already upstreamed at rev 321):
http://code.google.com/p/google-breakpad/source/detail?r=321

Would have covered this.

Comment 14

[Josh Aas]

Attachment: fix v1.4

Includes Linux build fix.

Comment 15

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/382cb6699240

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Can someone look at this for 1.9.0 or are we not taking updates there?

Comment 17

[(not currently active) Ted Mielczarek]

I think it's way too much for 1.9.0, and I don't think I'd take it on any branch, honestly. Just use the 10.4 SDK.

Comment 21

[(not currently active) Ted Mielczarek]

We should just take the "fix compilation with gcc 4.4" bits on the branches. They're tiny and harmless and would stop all these bugs.

Comment 24

[Ian Neal]

Attachment: Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26][Checkin for 1.9.1.8: Comment 28]

Fixes for gcc 4.4

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 413951 [details] [diff] [review]
Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26][Checkin for 1.9.1.8: Comment 28]

That's fine, you'll need approval for this.

Drivers: this patch has essentially zero risk, and fixes compiling Breakpad code with GCC 4.4 (which is the default in newer distros).

Comment 26

[Ian Neal]

Comment on attachment 413951 [details] [diff] [review]
Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26][Checkin for 1.9.1.8: Comment 28]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0ab07692df85

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 413951 [details] [diff] [review]
Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26][Checkin for 1.9.1.8: Comment 28]

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 28

[Ian Neal]

Comment on attachment 413951 [details] [diff] [review]
Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26][Checkin for 1.9.1.8: Comment 28]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e91b9342677