Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(20229)

Issue 212001: Android support for Linux client (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 6 months ago by Ted Mielczarek
Modified:
13 years, 6 months ago
Reviewers:
mochalatte, mwu
CC:
google-breakpad-dev_googlegroups.com
Visibility:
Public.

Description

This patch adds support for building the Linux client code with the Android NDK.
It turns out to be fairly straightforward. The NDK is missing quite a few
headers that we use, but most of the definitions are there, just in other
headers, so it's not a problem. There are only two that I actually needed to
provide skeletons for: link.h, for a definition of the ElfW macro from, and
ucontext.h, for ucontext_t. For the former I just re-implemented it. For the
latter, I found the code I needed in the V8 source. The rest of the patch is
some other fiddly things, plus some test changes (mostly changing the hardcoded
temp dir).

There's also some linux_syscall_support changes to go along with this, I'll
submit those elsewhere. I also have some tiny Google Test changes, I'll see
about upstreaming those (but they're only really necessary for building the unit
tests anyway). The tests build and mostly pass with this patch, it's still
failing my recently-added InstructionPointerMemory tests, although I haven't
sorted out why yet.

The stock NDK doesn't include an STL implementation, so I'm using a Crystax NDK
for compiling. Since the NDK is primarily focused on using Makefiles for
building, I'm using the following shell script to invoke Breakpad's configure:
http://people.mozilla.com/~tmielczarek/build-android.txt

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M configure View 1 chunk +1 line, -1 line 0 comments Download
M configure.ac View 1 chunk +1 line, -1 line 0 comments Download
A src/client/linux/android_link.h View 1 chunk +44 lines, -0 lines 0 comments Download
A src/client/linux/android_ucontext.h View 1 chunk +77 lines, -0 lines 0 comments Download
M src/client/linux/handler/exception_handler.cc View 3 chunks +8 lines, -2 lines 4 comments Download
M src/client/linux/handler/exception_handler.h View 1 chunk +4 lines, -0 lines 0 comments Download
M src/client/linux/handler/exception_handler_unittest.cc View 14 chunks +20 lines, -13 lines 3 comments Download
M src/client/linux/minidump_writer/line_reader_unittest.cc View 1 chunk +7 lines, -1 line 0 comments Download
M src/client/linux/minidump_writer/linux_dumper.cc View 3 chunks +10 lines, -3 lines 0 comments Download
M src/client/linux/minidump_writer/linux_dumper.h View 2 chunks +24 lines, -0 lines 0 comments Download
M src/client/linux/minidump_writer/linux_dumper_unittest.cc View 2 chunks +21 lines, -3 lines 0 comments Download
M src/client/linux/minidump_writer/linux_dumper_unittest_helper.cc View 1 chunk +3 lines, -1 line 0 comments Download
M src/client/linux/minidump_writer/minidump_writer.cc View 5 chunks +18 lines, -1 line 0 comments Download
M src/client/linux/minidump_writer/minidump_writer_unittest.cc View 2 chunks +7 lines, -1 line 0 comments Download
M src/common/linux/file_id.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 4
Ted Mielczarek
13 years, 6 months ago #1
Ted Mielczarek
LSS patch: http://codereview.chromium.org/3656004
13 years, 6 months ago #2
mwu
Looks good to me for the most part. Just a few little things I have ...
13 years, 6 months ago #3
Ted Mielczarek
13 years, 6 months ago #4
http://breakpad.appspot.com/212001/diff/1/6
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/212001/diff/1/6#newcode195
Line 195: if (sys_sigaltstack(&stack, NULL) == -1)
On 2010/10/19 19:39:34, mwu wrote:
> Hm, how does this work? I couldn't find a reference to a sys_sigaltstack

linux_syscall_support:
http://code.google.com/p/linux-syscall-support/
defines sys_* for essentially every syscall. It's used to avoid entering the
dynamic linker inside the exception handler. Here, I'm just using it because the
NDK headers are missing sigaltstack.

http://breakpad.appspot.com/212001/diff/1/6#newcode331
Line 331: sys_prctl(PR_SET_DUMPABLE, 1);
On 2010/10/19 19:39:34, mwu wrote:
> prctl appears to be defined in unistd.h and sys/prctl.h.

Yeah, but the signature is different, it takes 5 arguments. I didn't really want
to deal with that so I did this instead.

http://breakpad.appspot.com/212001/diff/1/8
File src/client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/212001/diff/1/8#newcode629
Line 629: ExceptionHandler handler(TEMPDIR, NULL, NULL, (void*) fds[1], true);
On 2010/10/19 19:39:34, mwu wrote:
> This used to be /tmp1. I guess that was a mistake unless this was
intentionally
> testing an invalid tmp dir.

Hm. Now that you mention that it might have been. The point of this test is to
verify that we can call a callback from the exception handler that signals the
parent process to actually write a minidump. I'll leave it as-is.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted