Closed Bug 543111 Opened 10 years ago Closed 4 years ago

Breakpad doesn't build when cross-compiling on Linux

Categories

(Toolkit :: Crash Reporting, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: bzbarsky, Assigned: ted)

References

Details

Attachments

(5 files, 6 obsolete files)

40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
glandium
: review+
Details
40 bytes, text/x-review-board-request
glandium
: review+
Details
40 bytes, text/x-review-board-request
glandium
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
Details
c++ -o host_functioninfo.o -c  -funsigned-char -I/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/../.. -I/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf -I. -I../../../../../../dist/include -I../../../../../../dist/include/nsprpub  -I/home/bzbarsky/mozilla/mac-test/obj-firefox/dist/include/nspr -I/home/bzbarsky/mozilla/mac-test/obj-firefox/dist/include/nss      -I/home/bzbarsky/mozilla/mac-test/obj-firefox/dist/include/nspr /home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc: In constructor ‘dwarf2reader::CULineInfoHandler::CULineInfoHandler(std::vector<dwarf2reader::SourceFileInfo, std::allocator<dwarf2reader::SourceFileInfo> >*, std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >*, dwarf2reader::LineMap*)’:
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:76: error: ‘ULLONG_MAX’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc: In member function ‘virtual void dwarf2reader::CULineInfoHandler::DefineFile(const std::string&, int32, uint32, uint64, uint64)’:
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:97: error: ‘ULLONG_MAX’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:107: error: ‘stderr’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:107: error: ‘fprintf’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc: In member function ‘virtual void dwarf2reader::CULineInfoHandler::AddLine(uint64, uint32, uint32, uint32)’:
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:121: error: ‘stderr’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:121: error: ‘fprintf’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc: In member function ‘virtual void dwarf2reader::CUFunctionInfoHandler::ProcessAttributeUnsigned(uint64, dwarf2reader::DwarfAttribute, dwarf2reader::DwarfForm, uint64)’:
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:183: error: ‘auto_ptr’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:183: error: expected primary-expression before ‘>’ token
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:185: error: ‘lireader’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:215: error: ‘stderr’ was not declared in this scope
/home/bzbarsky/mozilla/mac-test/mozilla/toolkit/crashreporter/google-breakpad/src/common/dwarf/functioninfo.cc:215: error: ‘fprintf’ was not declared in this scope
make[3]: *** [host_functioninfo.o] Error 1
So the key is that this is NOT running the toolwhip compiler.  It's running the host compiler.  Is that expected here?
So in the .i, this is what the part stdio.h is supposed to include looks like:

# 1 "/usr/include/stdio.h" 1 3 4
# 45 "/usr/include/stdio.h" 3 4
struct _IO_FILE;

typedef struct _IO_FILE FILE;

# 65 "/usr/include/stdio.h" 3 4
typedef struct _IO_FILE __FILE;

Thats it.  No obvious definition of stderr and the like.

Note that the file in question does NOT include stdio directly.  It seems to rely on stddef or someone including it.... which may well work differently on Linux vs windows or something.
Ah, and if I try to build that directory on Linux in one of my Linux trees, it fails to build in the same exact way.  

And the Makefile.in in toolkit/crashreporter/google-breakpad/src/common has:

ifeq ($(OS_ARCH),Darwin)
DIRS = dwarf
endif
Maybe that check should be HOST_OS_ARCH?
No, we do want to build a host dump_syms binary, otherwise dumping symbols wouldn't work. It's possible this file is just missing the right includes.
I did check; this file just fails on Linux period, per comment 3.  It should be including <stdio>, imho.
So try that? :)
OK, with some makefile hackery and the inclusion of limits.h (to do ULLONG_MAX), stdio.h (to do stderr and fprintf), and memory (for auto_ptr), things compile.  Patch coming up.
Attached patch Like so (obsolete) — Splinter Review
Attachment #424653 - Flags: review?(ted.mielczarek)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 424653 [details] [diff] [review]
Like so

Not great, but doesn't seem worth the hassle to fix it correctly at the moment. Also, that functioninfo.cc change will need to land upstream. (I'll try to remember to do that.)
Attachment #424653 - Flags: review?(ted.mielczarek) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/df90f0171ba7

Anything I can do to make sure the upstreaming is easier?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
If you give me a patch against Breakpad SVN I can just land it for you:
http://code.google.com/p/google-breakpad/source/checkout
And backed out:

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

It looks like the change to toolkit/crashreporter/google-breakpad/src/common/mac/Makefile.in makes things not compile on Mac when not cross-compiling (though I swear I try-servered this).

I tried doing:

-ifeq ($(HOST_OS_ARCH),Darwin)
+ifneq (,$(filter darwin,$(HOST_OS_ARCH)))

due to HOST_OS_ARCH not using the same value set as OS_ARCH, but that still didn't help.

Ted said he'll try to look at this sometime.
Assignee: bzbarsky → ted.mielczarek
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch The fix I tried (obsolete) — Splinter Review
I bet this got lost when I synced a new Breakpad.
Assignee: ted → nobody
Blocks: 921040
Ted: can you clarify where these tools are used? 

As we move to compiling darwin-on-linux (bug 921040), we're starting to draw the line on what can still must be done on mac hardware. ATM we're unsure if these tools are part of the shipped binary, or are used solely for preparation of the crash reporter symbol files. (For other reasons, we have not yet been able to build with crash reporter support included.)
Flags: needinfo?(ted)
These tools are only used on the build machines. They're invoked during the "buildsymbols" step. They are not shipped with the binary.

We are probably going to have to figure something out here because the alternative is copying every single object file+binary from the build to a Mac to run this process.
Flags: needinfo?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> We are probably going to have to figure something out here because the
> alternative is copying every single object file+binary from the build to a
> Mac to run this process.

Correct -- that is our plan for phase 1, as the entire crash symbol preparation step looks to be significant work to port. There are issues with the rest of this toolchain as well.
In the interest of serving our immediate goals, we can scope this bug to cover "should be possible to cross-compile Firefox for Mac on Linux without having to --disable-crashreporter". I think the simplest fix here will be to not compile the host sources/binaries in a linux->darwin cross. There might be some other build system fiddling required, but it shouldn't be anything substantial.
Blocks: 951758
Attached patch 543111-patch (obsolete) — Splinter Review
Conditionally enable building crashreporter elements when cross compiling for osx.
Attachment #8370092 - Flags: review?(ted)
(In reply to Joey Armstrong [:joey] from comment #21)
> Created attachment 8370092 [details] [diff] [review]
> 543111-patch
> 
> Conditionally enable building crashreporter elements when cross compiling
> for osx.

non-osx platform try job
https://tbpl.mozilla.org/?tree=Try&rev=31461ba01f9c

try job with darwin non-cross condional fix
https://tbpl.mozilla.org/?tree=Try&rev=9819cc8232fb
Comment on attachment 8370092 [details] [diff] [review]
543111-patch

Need to fix a conditional in breakpad/common.
Attachment #8370092 - Flags: review?(ted)
Comment on attachment 8370092 [details] [diff] [review]
543111-patch

Review of attachment 8370092 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/google-breakpad/src/common/mac/moz.build
@@ +19,5 @@
>  # The host lib is used for dump_syms, and the target lib for the
>  # crash reporter client.  Therefore, we don't need all the srcs in both.
> +if not CONFIG['MOZ_CRASHREPORTER']:
> +    pass
> +elif CONFIG['CROSS_COMPILE'] and CONFIG['OS_TARGET'] == 'Darwin':

This might be clearer if you added a new AC_SUBST, like BREAKPAD_BUILD_HOST_TOOLS, so you could just test on that.
Attached patch 543111-patch (obsolete) — Splinter Review
Same patch as before, conditionally build crashreporter.  Just do not default to only building common on winnt (should not have been left as default conditional).
Attachment #8370092 - Attachment is obsolete: true
Attachment #8370181 - Flags: review?(ted)
(In reply to Joey Armstrong [:joey] from comment #25)
> Created attachment 8370181 [details] [diff] [review]
> 543111-patch
> 
> Same patch as before, conditionally build crashreporter.  Just do not
> default to only building common on winnt (should not have been left as
> default conditional).

Oh and latest try run: https://tbpl.mozilla.org/?tree=Try&rev=10a6293bb05a
Attached patch 543111-patch (obsolete) — Splinter Review
Good thought, added BREAKPAD_BUILD_HOST_TOOLS.
Attachment #8370181 - Attachment is obsolete: true
Attachment #8370181 - Flags: review?(ted)
Attachment #8370238 - Flags: review?(ted)
(In reply to Joey Armstrong [:joey] from comment #27)
> Created attachment 8370238 [details] [diff] [review]
> 543111-patch
> 
> Good thought, added BREAKPAD_BUILD_HOST_TOOLS.

https://tbpl.mozilla.org/?tree=Try&rev=ea016dec096d
(In reply to Joey Armstrong [:joey] from comment #27)
> Created attachment 8370238 [details] [diff] [review]
> 543111-patch
> 
> Good thought, added BREAKPAD_BUILD_HOST_TOOLS.

ping on the code review
Comment on attachment 8370238 [details] [diff] [review]
543111-patch

Review of attachment 8370238 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with some rejiggering

::: configure.in
@@ +5931,5 @@
>      [MOZ_CRASHREPORTER=F # Force enable breakpad])
>  
> +if test "$OS_ARCH" != "$HOST_OS_ARCH"
> +       -a "$OS_ARCH" != "Darwin" \
> +       -a "$OS_ARCH" != "WINNT"; then

It would probably be nice to make this more general, but that can be a followup.

::: toolkit/crashreporter/google-breakpad/src/common/mac/moz.build
@@ +21,5 @@
> +if not CONFIG['MOZ_CRASHREPORTER']:
> +    pass
> +elif not CONFIG['BREAKPAD_BUILD_HOST_TOOLS']:
> +    pass
> +else:

You ought to be able to just make this:
if CONFIG['MOZ_CRASHREPORTER'] and CONFIG['BREAKPAD_BUILD_HOST_TOOLS']:

::: toolkit/crashreporter/google-breakpad/src/common/moz.build
@@ +50,5 @@
> +elif not CONFIG['BREAKPAD_BUILD_HOST_TOOLS']:
> +    pass
> +elif CONFIG['OS_TARGET'] == 'WINNT':
> +    pass
> +else:

Same here, just tack "and CONFIG['BREAKPAD_BUILD_HOST_TOOLS']" on to the existing conditional.
Attachment #8370238 - Flags: review?(ted) → review+
Assignee: nobody → joey
Attachment #424653 - Flags: review+
Assignee: joey → ted
I've got this working locally:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/1a6ea5478fd6

Most of the work was porting the dump_syms code from ObjC to C++ and replacing the use of Cocoa APIs with POSIX+STL. I'm going to try to upstream the Breakpad bits, but I have a working dump_syms on Linux that can dump Mach-O binaries.
Summary: breakpad doesn't build when cross-compiling with toolwhip → Breakpad doesn't build when cross-compiling on Linux
I submitted the Breakpad bits for upstream review:
https://codereview.chromium.org/1340543002

I split the rest of the patch into three parts:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/5fb8da23c83c - Imported Mac system headers into the tree (not sure if upstream wants this)
http://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/285e9fe6381d - Build fixes to our moz.build files etc
http://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/9beb9b816f42 - Enable crashreporter in mozconfig+configure
Depends on: 1205242
Related: I tested llvm-dsymutil from LLVM SVN r247440 and it works! Hooray!
bug 543111 - Fix Mac Breakpad host tools to build in Linux cross-compile. r=mento
bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium
Attachment #8667453 - Flags: review?(mh+mozilla)
bug 543111 - enable Breakpad for cross-mac builds. r?glandium
Attachment #8667454 - Flags: review?(mh+mozilla)
bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r?glandium
Attachment #8667455 - Flags: review?(mh+mozilla)
bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r?gps
Attachment #8667456 - Flags: review?(gps)
Attachment #8370238 - Attachment is obsolete: true
Attachment #426039 - Attachment is obsolete: true
Attachment #426037 - Attachment is obsolete: true
Attachment #424653 - Attachment is obsolete: true
Comment on attachment 8667456 [details]
MozReview Request: bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r=gps

https://reviewboard.mozilla.org/r/20757/#review18589
Attachment #8667456 - Flags: review?(gps) → review+
Comment on attachment 8667453 [details]
MozReview Request: bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium

https://reviewboard.mozilla.org/r/20751/#review18673

::: toolkit/crashreporter/google-breakpad/src/common/mac/moz.build:18
(Diff revision 1)
> +        '-I$(topsrcdir)/toolkit/crashreporter/google-breakpad/src/third_party/mac_headers/',

likewise

::: toolkit/crashreporter/google-breakpad/src/common/moz.build:41
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'Darwin' and CONFIG['HOST_OS_ARCH'] != 'Darwin':

use OS_ARCH instead of OS_TARGET.

::: toolkit/crashreporter/google-breakpad/src/common/moz.build:43
(Diff revision 1)
> +        '-I$(topsrcdir)/toolkit/crashreporter/google-breakpad/src/third_party/mac_headers/',

'-I%s...' % TOPSRCDIR

::: toolkit/crashreporter/google-breakpad/src/tools/mac/dump_syms/moz.build:29
(Diff revision 1)
> +        '-I$(topsrcdir)/toolkit/crashreporter/google-breakpad/src/third_party/mac_headers/',

likewise

::: tools/profiler/core/shim_mac_dump_syms.mm:16
(Diff revision 1)
> -  NSString* obj_file_ns = [NSString stringWithUTF8String:obj_file.c_str()];
> +  if (!ds.Read(obj_file))

shouldn't that be in part 1?
Attachment #8667453 - Flags: review?(mh+mozilla)
Comment on attachment 8667454 [details]
MozReview Request: bug 543111 - enable Breakpad for cross-mac builds. r=glandium

https://reviewboard.mozilla.org/r/20753/#review18675
Attachment #8667454 - Flags: review?(mh+mozilla) → review+
Attachment #8667455 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8667455 [details]
MozReview Request: bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r=glandium

https://reviewboard.mozilla.org/r/20755/#review18677
https://reviewboard.mozilla.org/r/20751/#review18673

> shouldn't that be in part 1?

I'm going to fold these two patches together, but I wanted to separate out the upstream patch from our local changes, since the upstream patch got review upstream.
https://reviewboard.mozilla.org/r/20751/#review18673

> use OS_ARCH instead of OS_TARGET.

I did s/OS_TARGET/OS_ARCH/ in this file just for sanity's sake.
A -p all Try push with the updated patches just for sanity:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fa65a0af2ee
Comment on attachment 8667452 [details]
MozReview Request: bug 543111 - Fix Mac Breakpad host tools to build in Linux cross-compile. r=mento

bug 543111 - Fix Mac Breakpad host tools to build in Linux cross-compile. r=mento
Attachment #8667453 - Flags: review?(mh+mozilla)
Comment on attachment 8667453 [details]
MozReview Request: bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium

bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium
Comment on attachment 8667454 [details]
MozReview Request: bug 543111 - enable Breakpad for cross-mac builds. r=glandium

bug 543111 - enable Breakpad for cross-mac builds. r=glandium
Attachment #8667454 - Attachment description: MozReview Request: bug 543111 - enable Breakpad for cross-mac builds. r?glandium → MozReview Request: bug 543111 - enable Breakpad for cross-mac builds. r=glandium
Comment on attachment 8667455 [details]
MozReview Request: bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r=glandium

bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r=glandium
Attachment #8667455 - Attachment description: MozReview Request: bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r?glandium → MozReview Request: bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r=glandium
Comment on attachment 8667456 [details]
MozReview Request: bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r=gps

bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r=gps
Attachment #8667456 - Attachment description: MozReview Request: bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r?gps → MozReview Request: bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r=gps
Comment on attachment 8667453 [details]
MozReview Request: bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium

bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium
Comment on attachment 8667454 [details]
MozReview Request: bug 543111 - enable Breakpad for cross-mac builds. r=glandium

bug 543111 - enable Breakpad for cross-mac builds. r=glandium
Comment on attachment 8667455 [details]
MozReview Request: bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r=glandium

bug 543111 - add support for alternate dsymutil paths to configure, add to cross-mozconfig. r=glandium
Comment on attachment 8667456 [details]
MozReview Request: bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r=gps

bug 543111 - fix symbolstore.py to work properly for cross-compiled mac builds on linux. r=gps
https://reviewboard.mozilla.org/r/20751/#review18673

> I did s/OS_TARGET/OS_ARCH/ in this file just for sanity's sake.

...except for the part where Android has OS_ARCH=Linux, so I changed those back.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #61)
> Once more, with feeling:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e1dd570bdf

That one is nice and green.
Comment on attachment 8667453 [details]
MozReview Request: bug 543111 - Mozilla build fixes to support building mac dump_syms on Linux r?glandium

https://reviewboard.mozilla.org/r/20751/#review18791
Attachment #8667453 - Flags: review?(mh+mozilla) → review+
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=15043928&repo=mozilla-inbound
Flags: needinfo?(ted)
*sigh*, this is because I renamed some source files from .mm -> .cc. I tried to work around that with the workaround glandium suggested last time I hit this problem:
https://hg.mozilla.org/integration/mozilla-inbound/diff/3b323ed3b20a/toolkit/crashreporter/google-breakpad/src/common/mac/Makefile.in
https://hg.mozilla.org/integration/mozilla-inbound/diff/3b323ed3b20a/toolkit/crashreporter/google-breakpad/src/tools/mac/dump_syms/Makefile.in

...but I guess that's not sufficient. (glandium has said that adding Makefile.in files doesn't necessarily work reliably)
Flags: needinfo?(ted)
Oh, I'm just an idiot. Those Makefile.ins had an extra '+' at the beginning of the line because apparently I copied them out of a diff. I fixed that and tested a dep build locally (built the parent changeset, then updated to the tip of my patchset and rebuilt) and it worked.
You need to log in before you can comment on or make changes to this bug.