Changes to get the breakpad client compiling and running on Android using

Changes to get the breakpad client compiling and running on Android using
ndk-7b.

[carlosvaldivia]

ping

[yfriedman]

Took a pass. It's mostly style-based as I'm just learning this code

https://breakpad.appspot.com/363001/diff/1/src/client/linux/android_link.h
File src/client/linux/android_link.h (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/android_link.h#ne...
src/client/linux/android_link.h:36: // TODO(zhenghao): exec_elf.h conflicts with
linux/elf.h.
If the new include works (for linux, too) then I don't think we need to keep the
commented out version.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h
File src/client/linux/log/log.h (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h#newcode2
src/client/linux/log/log.h:2: // Author: carlosvaldivia@google.com (Carlos
Valdivia)
Looks like these files don't specify "Author"

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h#newcode4
src/client/linux/log/log.h:4: #ifndef
CODE_UPSTREAM_GOOGLE_BREAKPAD_READ_ONLY_SRC_CLIENT_LINUX_LOG_LOG_H_
Guard is wrong

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h#newcode13
src/client/linux/log/log.h:13: } // namespace log
s/log/logger/

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:81: // XXX(carlosvaldivia)
remove
??

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1345: /*
What's going on here?

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.h:41: #ifdef __ANDROID__
Style elsewhere seems to be: #if defined(__ANDROID__). Also fix lower down (49)

https://breakpad.appspot.com/363001/diff/1/src/tools/linux/dump_syms/dump_sym...
File src/tools/linux/dump_syms/dump_syms.cc (right):

https://breakpad.appspot.com/363001/diff/1/src/tools/linux/dump_syms/dump_sym...
src/tools/linux/dump_syms/dump_syms.cc:55: // By default assume that the target
wordsize is the same as the host
Nit: move this just above the part of specifying "-w"

https://breakpad.appspot.com/363001/diff/1/src/tools/linux/dump_syms/dump_sym...
src/tools/linux/dump_syms/dump_syms.cc:87: /*
Please don't check in commented out code

[carlosvaldivia]

rfal

https://breakpad.appspot.com/363001/diff/1/src/client/linux/android_link.h
File src/client/linux/android_link.h (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/android_link.h#ne...
src/client/linux/android_link.h:36: // TODO(zhenghao): exec_elf.h conflicts with
linux/elf.h.
Let me look into this some more.

On 2012/03/22 20:47:43, yfriedman wrote:
> If the new include works (for linux, too) then I don't think we need to keep
the
> commented out version.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h
File src/client/linux/log/log.h (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h#newcode2
src/client/linux/log/log.h:2: // Author: carlosvaldivia@google.com (Carlos
Valdivia)
On 2012/03/22 20:47:43, yfriedman wrote:
> Looks like these files don't specify "Author"

Done.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h#newcode4
src/client/linux/log/log.h:4: #ifndef
CODE_UPSTREAM_GOOGLE_BREAKPAD_READ_ONLY_SRC_CLIENT_LINUX_LOG_LOG_H_
On 2012/03/22 20:47:43, yfriedman wrote:
> Guard is wrong

Done.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/log/log.h#newcode13
src/client/linux/log/log.h:13: } // namespace log
On 2012/03/22 20:47:43, yfriedman wrote:
> s/log/logger/

Done.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:81: // XXX(carlosvaldivia)
remove
On 2012/03/22 20:47:43, yfriedman wrote:
> ??

Done.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1345: /*
On 2012/03/22 20:47:43, yfriedman wrote:
> What's going on here?

I meant to delete that. There is a core2minidump utility tool that is supported
by a different tool. This isn't called from any breakpad_client code as far as I
can tell so I just moved it to the appropriate tool code. I'd really like for
mark to give his judgment though.

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.h:41: #ifdef __ANDROID__
On 2012/03/22 20:47:43, yfriedman wrote:
> Style elsewhere seems to be: #if defined(__ANDROID__). Also fix lower down
(49)

Done.

[yfriedman]

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/crash_generati...
File src/client/linux/crash_generation/crash_generation_client.cc (left):

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/crash_generati...
src/client/linux/crash_generation/crash_generation_client.cc:29: 
Revert this file

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.h:52: MappingInfo first;
Why do you need this instead of std::pair as below? There's lots of places in
chromium where std::pair is used and works fine with STLPort.

[carlosvaldivia]

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/crash_generati...
File src/client/linux/crash_generation/crash_generation_client.cc (left):

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/crash_generati...
src/client/linux/crash_generation/crash_generation_client.cc:29: 
On 2012/03/23 16:50:31, yfriedman wrote:
> Revert this file

Done.

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/4003/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.h:52: MappingInfo first;
So the stlport implementation in the ndk is wonky. It can't take basic types
like ints.

On 2012/03/23 16:50:31, yfriedman wrote:
> Why do you need this instead of std::pair as below? There's lots of places in
> chromium where std::pair is used and works fine with STLPort.

[yfriedman]

As discussed offline, please remove the .deps files

[carlosvaldivia]

Looks like the .dirstamp needs to be there, but the .deps stuff is now gone.

Still an outstanding issue I need to look at, but marks opinions on the
core2minidump would be much appreciated.

[yfriedman]

lgtm

This looks fine although it doesn't mean much coming from me :)

[carlosvaldivia]

Ok I have some more changes incomming to minidump_writer.cc/h. Basically I need
to expose the MinidumpWriter class. No deep design changes but core2minidump
won't compile on linux without it.

[carlosvaldivia]

Everything is good again. For some reason I cant seem to add my
adnroid-configure.sh file the the change. That said I've emailed it to mark.

[carlosvaldivia]

Everything is good again. For some reason I cant seem to add my
adnroid-configure.sh file the the change. That said I've emailed it to mark.

[Mark Mentovai]

This worked out to be much more simple than I expected.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.cc
File src/client/linux/log/log.cc (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.cc#ne...
src/client/linux/log/log.cc:1: // Copyright 2012 Google Inc. All Rights
Reserved.
I see an empty file called .dirstamp that I can’t comment on — because it’s
empty. What’s it for? Can it be removed?

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.cc#ne...
src/client/linux/log/log.cc:2: // Author: carlosvaldivia@google.com (Carlos
Valdivia)
Please use the proper Breakpad boilerplate.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.h
File src/client/linux/log/log.h (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.h#new...
src/client/linux/log/log.h:33: #include <cstddef>
No, use <stddef.h>. <cstddef> only brings in std::size_t. You want it without a
namespace.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.h#new...
src/client/linux/log/log.h:39: } // namespace logger
Style nit: two spaces between the last non-whitespace thing on a line (the })
and the // for an inline comment.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:50: #include <cctype>
No, use <ctype.h> for the same reason that I just mentioned.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.h:47: #if defined(__ANDROID__)
Is there any reason to make this Android-only?

[carlosvaldivia]

Thanks for taking a look. Sorry about the delay. I've been busy with the clank
merge for the last week.

Anyway. Here is the latest and greatest. I might also just include the
dump_symbols changes in this cl.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.cc
File src/client/linux/log/log.cc (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.cc#ne...
src/client/linux/log/log.cc:1: // Copyright 2012 Google Inc. All Rights
Reserved.
I think it is related to svn. I tried removing it earlier but it caused problems
(I could upload changes) so I added it back.

On 2012/03/27 18:56:12, Mark Mentovai wrote:
> I see an empty file called .dirstamp that I can’t comment on — because it’s
> empty. What’s it for? Can it be removed?

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.cc#ne...
src/client/linux/log/log.cc:2: // Author: carlosvaldivia@google.com (Carlos
Valdivia)
On 2012/03/27 18:56:12, Mark Mentovai wrote:
> Please use the proper Breakpad boilerplate.

Done.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.h
File src/client/linux/log/log.h (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.h#new...
src/client/linux/log/log.h:33: #include <cstddef>
On 2012/03/27 18:56:12, Mark Mentovai wrote:
> No, use <stddef.h>. <cstddef> only brings in std::size_t. You want it without
a
> namespace.

Done.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/log/log.h#new...
src/client/linux/log/log.h:39: } // namespace logger
On 2012/03/27 18:56:12, Mark Mentovai wrote:
> Style nit: two spaces between the last non-whitespace thing on a line (the })
> and the // for an inline comment.

Done.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:50: #include <cctype>
On 2012/03/27 18:56:12, Mark Mentovai wrote:
> No, use <ctype.h> for the same reason that I just mentioned.

Done.

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/10001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.h:47: #if defined(__ANDROID__)
Only that it is really ugly and I wanted to limit the risk. But I see the other
viewpoint and I'll change it.

On 2012/03/27 18:56:12, Mark Mentovai wrote:
> Is there any reason to make this Android-only?

[carlosvaldivia]

Good news. I discovered the --enable-m32 flag in configure.ac.

As a result it is nearly trivial to get dump_syms to work on android binaries.
Simply run:

> configure --enable-m32
> make clean
> make

And the output dump_syms binary will be able to dump symbols .

The only change I made is to get rid of some misleading symbols that find their
way into the android binaries.

[Mark Mentovai]

LG otherwise

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:60: #include <ctype.h>
<ctype.h> is included unconditionally here, and conditionally for __ANDROID__ on
line 50.

I suggest removing this one and removing the guards around the one on line 50
(and the blank line at line 52). These headers ought to be sorted anyway, and
<ctype.h> comes before everything else listed.

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.h:40: // The android ndk
STLPort can't handle incomplete types in generics.
This comment is obsolete and really only makes sense in a previous version of
this change. In this version, the comment makes no sense in relation to the rest
of the file. It explains an implementation choice but there’s no longer any
evidence of an alternative implementation.

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.h:47: struct MappingInfo;
This struct is defined in linux_dumper.h, which is included unconditionally in
linux_dumper.h.

[carlosvaldivia]

Done. How do I submit?

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:60: #include <ctype.h>
On 2012/04/02 13:30:59, Mark Mentovai wrote:
> <ctype.h> is included unconditionally here, and conditionally for __ANDROID__
on
> line 50.
> 
> I suggest removing this one and removing the guards around the one on line 50
> (and the blank line at line 52). These headers ought to be sorted anyway, and
> <ctype.h> comes before everything else listed.

Done.

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.h:40: // The android ndk
STLPort can't handle incomplete types in generics.
On 2012/04/02 13:30:59, Mark Mentovai wrote:
> This comment is obsolete and really only makes sense in a previous version of
> this change. In this version, the comment makes no sense in relation to the
rest
> of the file. It explains an implementation choice but there’s no longer any
> evidence of an alternative implementation.

Done.

https://breakpad.appspot.com/363001/diff/14001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.h:47: struct MappingInfo;
On 2012/04/02 13:30:59, Mark Mentovai wrote:
> This struct is defined in linux_dumper.h, which is included unconditionally in
> linux_dumper.h.

Done.

[Mark Mentovai]

LGTM

I fixed the things I mention below and checked this in at r945.

https://breakpad.appspot.com/363001/diff/17001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/363001/diff/17001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:49: #include <algorithm>
C system headers come before C++ system headers. This was correct where it was
placed before.

https://breakpad.appspot.com/363001/diff/17001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:76: #include
"google_breakpad/common/minidump_cpu_amd64.h"
These are gone now following r944, checked in earlier today.