Fix Mac Breakpad host tools to build in Linux cross-compile

Fix Mac Breakpad host tools to build in Linux cross-compile

We're working on building our Firefox Mac builds as a Linux cross-compile
(https://bugzilla.mozilla.org/show_bug.cgi?id=921040) and we need symbol
dumping to work. This change ports the Mac dump_syms tool to build and work
on Linux. I've tested it and it produces identical output to running the
tool on Mac.

The bulk of the work here was converting src/common/mac/dump_syms.mm and
src/tools/mac/dump_syms/dump_syms_tool.mm from ObjC++ to C++ and removing
their use of Foundation classes in favor of standard C/C++.

This won't compile out-of-the-box on Linux, it requires some Mac system
headers that are not included in this patch. I have those tentatively in
a separate patch to land in Gecko
(http://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/5fb8da23c83c),
but I wasn't sure if you'd be interested in having them in the Breakpad tree.
We could almost certainly pare down the set of headers included there, I
didn't spend too much time trying to minimize them (we primarily just need
the Mach-O structs and a few associated bits).

I just realized that this patch is missing updating the XCode project files
(ugh). I'll fix that up in a bit.

R=mark@chromium.org
BUG=https://bugzilla.mozilla.org/show_bug.cgi?id=543111

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/6cee755e09223833137cefa589037c1448e44fa2

[Mark Mentovai, 2015-09-11 15:49:51]

I think we could take the headers in-tree in a location off to the side where
they won’t hurt anyone.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
File src/common/mac/arch_utilities.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:37: #ifndef CPU_TYPE_ARM
I don’t think we need backup definitions of CPU_TYPE_ARM or CPU_SUBTYPE_ARM_V7
anymore.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:116: #ifndef __APPLE__
Did this code come from anywhere else or did you write it yourself according to
the interface?

If it came from somewhere else, we may need to put it into third_party. In that
case, ignore the next comments in this file.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:129: const NXArchInfo kKnownArchitectures[] = {
Comment that Architecture and kKnownArchitectures[] must be kept in sync.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/byteswap.h
File src/common/mac/byteswap.h (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/byteswap.h#n...
src/common/mac/byteswap.h:39: #include <architecture/byte_order.h>
The top of this header says

 * Please note that the byte ordering functions in this file are deprecated.
 * A replacement API exists in libkern/OSByteOrder.h

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc
File src/common/mac/dump_syms.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc...
src/common/mac/dump_syms.cc:93: fprintf(stderr, "Object file does not exist:
%s\n",
Not necessarily—use strerror(errno) for the real error.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc...
src/common/mac/dump_syms.cc:104: // Filenames referring to bundles usually have
names of the form
This is totally hokey. It seems to break down if the .dSYM bundle is renamed.

If we’re not going to read the Info.plist (and I’m guessing we’re not, unless
you want to include CF for non-Mac), then it’s probably more reliable to just
look for the single file in Contents/Resources/DWARF. We can fail if more than
one file exists in there, and if there’s only one, it’s got to be the winner.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc...
src/common/mac/dump_syms.cc:147: read_ok = (fread(&contents_[0], st.st_size, 1,
f) == 1);
No read loop?

Generally it’s preferable to read until you hit EOF, instead of calling stat()
to figure out how much to read.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.h
File src/common/mac/dump_syms.h (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.h#...
src/common/mac/dump_syms.h:167: vector<uint8_t> contents_;
vector<> always initializes each element when it grows, which is undesirable
when reading a large file into it immediately afterwards. A scoped_ptr_malloc or
something else that doesn’t do initialization may be more appropriate.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/file_id.cc
File src/common/mac/file_id.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/file_id.cc#n...
src/common/mac/file_id.cc:48: strncpy(path_, path, sizeof(path_) - 1);
strlcpy() guarantees NUL-termination, but strncpy() doesn’t, so you need to
ensure that path_[sizeof(path_) - 1] = '\0'. But actually, there’s a better
alternative: snprintf does guarantee NUL-termination and is a more portable
replacement for strlcpy() than strncpy():

  sprintf(path_, sizeof(path_), "%s", path);

This recurs elsewhere in the change.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_id.cc
File src/common/mac/macho_id.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_id.cc#...
src/common/mac/macho_id.cc:36: extern "C" {  // necessary for Leopard
1. This was probably never true.
2. Nobody cares about this SDK anymore.

OK to remove the extern "C" wrapping.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_walker.cc
File src/common/mac/macho_walker.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_walker...
src/common/mac/macho_walker.cc:36: extern "C" {  // necessary for Leopard
Same.

https://codereview.chromium.org/1340543002/diff/1/src/tools/mac/dump_syms/dum...
File src/tools/mac/dump_syms/dump_syms_tool.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/tools/mac/dump_syms/dum...
src/tools/mac/dump_syms/dump_syms_tool.cc:119: string primary_file =
split_module ? options.dsymPath : options.srcPath;
const string&

[Ted Mielczarek, 2015-09-11 15:58:01]

Ok, I will pull the headers in in an updated patch. If I do that I can wire up
the autotools build to (optionally?) build the Mac dump_syms when building on
Linux, which would make this easier to test.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
File src/common/mac/arch_utilities.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:116: #ifndef __APPLE__
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> Did this code come from anywhere else or did you write it yourself according
to
> the interface?

I wrote it myself after I noticed the existing polyfills in this file. I wasn't
sure whether to try to merge my versions with the existing ones though.
Thoughts?

[Mark Mentovai, 2015-09-11 16:19:35]

Ted Mielczarek wrote:
>
https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
> src/common/mac/arch_utilities.cc:116: #ifndef __APPLE__
> On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> > Did this code come from anywhere else or did you write it yourself according
> to
> > the interface?
> 
> I wrote it myself after I noticed the existing polyfills in this file. I
wasn't
> sure whether to try to merge my versions with the existing ones though.
> Thoughts?

I don’t know how clean a merge would be. It might be good enough as-is.

[Ted Mielczarek, 2015-09-15 12:58:36]

I think I addressed all your review comments.

I added src/tools/mac/dump_syms to the autotools build system, so it builds as
part of configure && make now. I also added the macho_reader_unittest to that
build. I had to fix some things to get it to build, I suspect nobody has been
building it for a while...

I fixed the XCode projects, and dump_syms builds locally for me on my Mac.

I fixed the gyp files, but I'm not 100% sure on how to test that since I've
never done a gyp-based Breakpad build before.

I added the Mac SDK headers under src/third-party/mac-headers.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
File src/common/mac/arch_utilities.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:37: #ifndef CPU_TYPE_ARM
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> I don’t think we need backup definitions of CPU_TYPE_ARM or CPU_SUBTYPE_ARM_V7
> anymore.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:116: #ifndef __APPLE__
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> Did this code come from anywhere else or did you write it yourself according
to
> the interface?
> 
> If it came from somewhere else, we may need to put it into third_party. In
that
> case, ignore the next comments in this file.

Acknowledged.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/arch_utiliti...
src/common/mac/arch_utilities.cc:129: const NXArchInfo kKnownArchitectures[] = {
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> Comment that Architecture and kKnownArchitectures[] must be kept in sync.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/byteswap.h
File src/common/mac/byteswap.h (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/byteswap.h#n...
src/common/mac/byteswap.h:39: #include <architecture/byte_order.h>
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> The top of this header says
> 
>  * Please note that the byte ordering functions in this file are deprecated.
>  * A replacement API exists in libkern/OSByteOrder.h

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc
File src/common/mac/dump_syms.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc...
src/common/mac/dump_syms.cc:93: fprintf(stderr, "Object file does not exist:
%s\n",
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> Not necessarily—use strerror(errno) for the real error.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc...
src/common/mac/dump_syms.cc:104: // Filenames referring to bundles usually have
names of the form
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> This is totally hokey. It seems to break down if the .dSYM bundle is renamed.
> 
> If we’re not going to read the Info.plist (and I’m guessing we’re not, unless
> you want to include CF for non-Mac), then it’s probably more reliable to just
> look for the single file in Contents/Resources/DWARF. We can fail if more than
> one file exists in there, and if there’s only one, it’s got to be the winner.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.cc...
src/common/mac/dump_syms.cc:147: read_ok = (fread(&contents_[0], st.st_size, 1,
f) == 1);
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> No read loop?
> 
> Generally it’s preferable to read until you hit EOF, instead of calling stat()
> to figure out how much to read.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.h
File src/common/mac/dump_syms.h (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/dump_syms.h#...
src/common/mac/dump_syms.h:167: vector<uint8_t> contents_;
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> vector<> always initializes each element when it grows, which is undesirable
> when reading a large file into it immediately afterwards. A scoped_ptr_malloc
or
> something else that doesn’t do initialization may be more appropriate.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/file_id.cc
File src/common/mac/file_id.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/file_id.cc#n...
src/common/mac/file_id.cc:48: strncpy(path_, path, sizeof(path_) - 1);
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> strlcpy() guarantees NUL-termination, but strncpy() doesn’t, so you need to
> ensure that path_[sizeof(path_) - 1] = '\0'. But actually, there’s a better
> alternative: snprintf does guarantee NUL-termination and is a more portable
> replacement for strlcpy() than strncpy():
> 
>   sprintf(path_, sizeof(path_), "%s", path);
> 
> This recurs elsewhere in the change.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_id.cc
File src/common/mac/macho_id.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_id.cc#...
src/common/mac/macho_id.cc:36: extern "C" {  // necessary for Leopard
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> 1. This was probably never true.
> 2. Nobody cares about this SDK anymore.
> 
> OK to remove the extern "C" wrapping.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_walker.cc
File src/common/mac/macho_walker.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/common/mac/macho_walker...
src/common/mac/macho_walker.cc:36: extern "C" {  // necessary for Leopard
On 2015/09/11 15:49:50, Mark Mentovai - August is over wrote:
> Same.

Done.

https://codereview.chromium.org/1340543002/diff/1/src/tools/mac/dump_syms/dum...
File src/tools/mac/dump_syms/dump_syms_tool.cc (right):

https://codereview.chromium.org/1340543002/diff/1/src/tools/mac/dump_syms/dum...
src/tools/mac/dump_syms/dump_syms_tool.cc:119: string primary_file =
split_module ? options.dsymPath : options.srcPath;
On 2015/09/11 15:49:51, Mark Mentovai - August is over wrote:
> const string&

Done.

[Mark Mentovai, 2015-09-15 16:27:52]

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode572
Makefile.am:572: -I$(top_srcdir)/src/third_party/mac-headers \
You don’t want to do this on Mac, where you should just pick up the headers from
the system SDK.

https://codereview.chromium.org/1340543002/diff/20001/src/third_party/mac-hea...
File src/third_party/mac-headers/README (right):

https://codereview.chromium.org/1340543002/diff/20001/src/third_party/mac-hea...
src/third_party/mac-headers/README:1: These headers were copied from the Mac OS
X 10.7 SDK to enable building
mac_headers (underscore, like third_party)

[Ted Mielczarek, 2015-09-15 16:51:15]

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode572
Makefile.am:572: -I$(top_srcdir)/src/third_party/mac-headers \
On 2015/09/15 16:27:52, Mark Mentovai - August is over wrote:
> You don’t want to do this on Mac, where you should just pick up the headers
from
> the system SDK.

Humorously, we're not actually building this on Mac via autotools because this
is in an `if LINUX_HOST` block. I was going to change that but then looked at
the automake conditionals documentation and wept. If you'd like that to work I
can make it happen.

https://codereview.chromium.org/1340543002/diff/20001/src/third_party/mac-hea...
File src/third_party/mac-headers/README (right):

https://codereview.chromium.org/1340543002/diff/20001/src/third_party/mac-hea...
src/third_party/mac-headers/README:1: These headers were copied from the Mac OS
X 10.7 SDK to enable building
On 2015/09/15 16:27:52, Mark Mentovai - August is over wrote:
> mac_headers (underscore, like third_party)

Done.

[Mark Mentovai, 2015-09-15 18:24:23]

LGTM

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode572
Makefile.am:572: -I$(top_srcdir)/src/third_party/mac-headers \
On 2015/09/15 16:51:15, Ted Mielczarek wrote:
> On 2015/09/15 16:27:52, Mark Mentovai - August is over wrote:
> > You don’t want to do this on Mac, where you should just pick up the headers
> from
> > the system SDK.
> 
> Humorously, we're not actually building this on Mac via autotools because this
> is in an `if LINUX_HOST` block.

Oh, ha!

> I was going to change that but then looked at
> the automake conditionals documentation and wept. If you'd like that to work I
> can make it happen.

You can still build it on Mac by telling make you want to build that target,
though. Can’t you just wrap this line with the CXXFLAGS in the same kind of
LINUX_HOST ifdef as above?

https://codereview.chromium.org/1340543002/diff/40001/src/common/mac/dump_sym...
File src/common/mac/dump_syms.cc (right):

https://codereview.chromium.org/1340543002/diff/40001/src/common/mac/dump_sym...
src/common/mac/dump_syms.cc:94: const vector<string> list_directory(const
string& directory) {
The return value shouldn’t be const. It’s up to the caller to decide if it wants
to store the returned vector in a const variable.

[Mark Mentovai, 2015-09-15 18:24:58]

Update the CL description too.

[Ted Mielczarek, 2015-09-16 10:45:32]

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode572
Makefile.am:572: -I$(top_srcdir)/src/third_party/mac-headers \
On 2015/09/15 18:24:23, Mark Mentovai - August is over wrote:
> You can still build it on Mac by telling make you want to build that target,
> though. Can’t you just wrap this line with the CXXFLAGS in the same kind of
> LINUX_HOST ifdef as above?

This entire section is in an `if LINUX_HOST`, it starts here:
https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode445
and runs through:
https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode682

...so building the tool via autotools on Mac doesn't currently work. I could fix
that by adding a MAC_HOST variable, but automake doesn't support anything but
single variable checks in conditionals, so I'd also have to add a
LINUX_OR_MAC_HOST variable, which is when I stopped trying. If you think that's
something I should do I can certainly do it.

https://codereview.chromium.org/1340543002/diff/40001/src/common/mac/dump_sym...
File src/common/mac/dump_syms.cc (right):

https://codereview.chromium.org/1340543002/diff/40001/src/common/mac/dump_sym...
src/common/mac/dump_syms.cc:94: const vector<string> list_directory(const
string& directory) {
On 2015/09/15 18:24:23, Mark Mentovai - August is over wrote:
> The return value shouldn’t be const. It’s up to the caller to decide if it
wants
> to store the returned vector in a const variable.

Done.

[Ted Mielczarek, 2015-09-16 10:47:04]

Committed patchset #3 (id:40001) manually as
6cee755e09223833137cefa589037c1448e44fa2 (presubmit successful).

[Ted Mielczarek, 2015-09-16 10:48:55]

On 2015/09/15 18:24:58, Mark Mentovai - August is over wrote:
> Update the CL description too.

*sigh*, sorry, I'm new to this whole `git cl` workflow. I had updated my local
git changeset commit message but didn't realize that `git cl land` would replace
that with the description from the CL. :-/

[Mark Mentovai, 2015-09-16 14:06:24]

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode572
Makefile.am:572: -I$(top_srcdir)/src/third_party/mac-headers \
On 2015/09/16 10:45:32, Ted Mielczarek wrote:
> On 2015/09/15 18:24:23, Mark Mentovai - August is over wrote:
> > You can still build it on Mac by telling make you want to build that target,
> > though. Can’t you just wrap this line with the CXXFLAGS in the same kind of
> > LINUX_HOST ifdef as above?
> 
> This entire section is in an `if LINUX_HOST`, it starts here:
> https://codereview.chromium.org/1340543002/diff/20001/Makefile.am#newcode445

402, actually.

I think that’s what confused me. I saw “if LINUX_HOST” at 445 and assumed that
it wouldn’t be in a wider “if LINUX_HOST”.

[labath, 2015-11-17 16:24:38]

Hi,

this change breaks "make install" on linux. It complains about having two
executables named "dump_syms", as it tries to install them to the same place.

How would you feel about a change which puts the mac version into
"noinst_PROGRAMS" (alternatives being renaming it, installing to a different
folder, merging the two executables(?))?

I can do the patch if the direction is ok.

cheers,
pl

[Ted Mielczarek, 2015-11-17 20:52:57]

On 2015/11/17 16:24:38, labath wrote:
> Hi,
> 
> this change breaks "make install" on linux. It complains about having two
> executables named "dump_syms", as it tries to install them to the same place.
> 
> How would you feel about a change which puts the mac version into
> "noinst_PROGRAMS" (alternatives being renaming it, installing to a different
> folder, merging the two executables(?))?
> 
> I can do the patch if the direction is ok.

I did notice this and forgot to fix it. I'm OK with either putting it in
noinst_PROGRAMS or renaming it (we could call it dump_syms_mac).

[Ted Mielczarek, 2015-11-25 14:14:24]

On 2015/11/17 20:52:57, Ted Mielczarek wrote:
> I did notice this and forgot to fix it. I'm OK with either putting it in
> noinst_PROGRAMS or renaming it (we could call it dump_syms_mac).

I submitted a CL to rename the binary:
https://codereview.chromium.org/1474673004/

[labath, 2015-11-25 16:30:22]

On 2015/11/25 14:14:24, Ted Mielczarek wrote:
> On 2015/11/17 20:52:57, Ted Mielczarek wrote:
> > I did notice this and forgot to fix it. I'm OK with either putting it in
> > noinst_PROGRAMS or renaming it (we could call it dump_syms_mac).
> 
> I submitted a CL to rename the binary:
> https://codereview.chromium.org/1474673004/

Thanks a lot! :)

(I am sorry for the late response, I only noticed your comments now. I was
hoping I'd get notifications by email, but apparently commenting does not
auto-subscribe me here. :) )