Closed Bug 510920 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: Crash Reporting, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- final-fixed
status1.9.1 --- .8-fixed

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Attached patch fix v1.0 (obsolete) — Splinter Review
Assignee: nobody → joshmoz
Blocks: 501436
Attached patch fix v1.1 (obsolete) — Splinter Review
Includes my fix for the following breakpad bug:

http://code.google.com/p/google-breakpad/issues/detail?id=330
Attachment #397043 - Attachment is obsolete: true
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.
Updating summary to narrow scope.
Summary: pull breakpad from upstream for breakpad fix 248, 10.5+ SDK compat → Sync to breakpad revision 350 for breakpad fix 248, 10.5+ SDK compat
Attached patch fix v1.2 (obsolete) — Splinter Review
Breakpad revision 350 patch.
Attachment #397116 - Attachment is obsolete: true
Attachment #398432 - Flags: review?(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!
Attachment #398432 - Flags: review?(ted.mielczarek) → review+
This is the fix that we have locally that's not upstreamed:
http://hg.mozilla.org/mozilla-central/rev/cfef2762ecb3
That fix appears to have been upstreamed, breakpad svn revision 313 so we pick it up when updating to revision 350.
pushed to mozilla-central with ted's comments addressed

http://hg.mozilla.org/mozilla-central/rev/e8c01867056a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
Attached patch fix v1.3 (obsolete) — Splinter Review
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.
Attachment #398432 - Attachment is obsolete: true
Could be the switch to gcc 4.3 that changed today, but I thought we were good to go with that.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v1.4Splinter Review
Includes Linux build fix.
Attachment #398569 - Attachment is obsolete: true
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/382cb6699240
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Can someone look at this for 1.9.0 or are we not taking updates there?
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.
Duplicate of this bug: 488111
Duplicate of this bug: 522881
Duplicate of this bug: 524969
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.
Duplicate of this bug: 530072
Duplicate of this bug: 530424
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).
Attachment #413951 - Flags: review?(ted.mielczarek)
Attachment #413951 - Flags: review+
Attachment #413951 - Flags: approval1.9.1.7?
Attachment #413951 - Flags: approval1.9.2?
Attachment #413951 - Flags: approval1.9.2? → approval1.9.2+
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
Attachment #413951 - Attachment description: Add cstdio to various files patch v0.1 → Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26]
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
Attachment #413951 - Flags: approval1.9.1.8? → approval1.9.1.8+
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
Attachment #413951 - Attachment description: Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26] → Add cstdio to various files patch v0.1 [Checkin for 1.9.2: Comment 26][Checkin for 1.9.1.8: Comment 28]
You need to log in before you can comment on or make changes to this bug.