From f735d050c4873b7497506b635b266ea304b3922f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 27 Oct 2016 17:02:48 -0400 Subject: [PATCH 1/9] Port the util library to Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this change, it is possible to build crashpad_util on Linux. I built with clang 3.8.1 and GCC 6.2.0. - For per-OS “exception code” metrics, Android and Linux are broken out distinctly. - Because Linux provides no standard UUID generator, base::RandBytes() is used to generate random UUIDs for the InitializeWithNew() form. - Multiple fixes for CloseMultipleNowOrOnExec(): - readdir_r() is deprecated in glibc 2.24. Use readdir() on Linux. - Linux does not have OPEN_MAX. Use the fs.nr_open sysctl (via /proc/sys) to determine the maximum (currently-configured) possible number of file descriptors per process. - Use the {CTL_KERN, KERN_MAXFILESPERPROC} sysctl on Mac to determine the maximum (currently-configured) possible number of file descriptors per process. This is an improvement over using OPEN_MAX, which is still consulted. - ThreadLogMessages’ use of DCHECK_EQ() needs an address-of operator on function pointers to avoid confusing GCC. One problem remains: - util/misc/pdb_structures.h produces -Wmultichar errors. -Wmultichar is enabled by default with GCC (but not clang). It is impossible to disable this warning with #pragma GCC diagnostic ignored. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431 This has not been tested beyond building the crashpad_util target. BUG=crashpad:30 Change-Id: I02e7a05da512ca312806d825b3fc9b2c5bf1a990 Reviewed-on: https://chromium-review.googlesource.com/404009 Reviewed-by: Robert Sesek --- util/misc/metrics.cc | 29 +++++------ util/misc/uuid.cc | 11 ++++ util/posix/close_multiple.cc | 84 +++++++++++++++++++++++++++--- util/thread/thread_log_messages.cc | 4 +- 4 files changed, 103 insertions(+), 25 deletions(-) diff --git a/util/misc/metrics.cc b/util/misc/metrics.cc index f5ab07b6..48be7ecd 100644 --- a/util/misc/metrics.cc +++ b/util/misc/metrics.cc @@ -18,6 +18,16 @@ #include "base/metrics/sparse_histogram.h" #include "build/build_config.h" +#if defined(OS_MACOSX) +#define METRICS_OS_NAME "Mac" +#elif defined(OS_WIN) +#define METRICS_OS_NAME "Win" +#elif defined(OS_ANDROID) +#define METRICS_OS_NAME "Android" +#elif defined(OS_LINUX) +#define METRICS_OS_NAME "Linux" +#endif + namespace crashpad { namespace { @@ -79,12 +89,7 @@ void Metrics::ExceptionCaptureResult(CaptureResult result) { // static void Metrics::ExceptionCode(uint32_t exception_code) { -#if defined(OS_WIN) - static const char kExceptionCodeString[] = "Crashpad.ExceptionCode.Win"; -#elif defined(OS_MACOSX) - static const char kExceptionCodeString[] = "Crashpad.ExceptionCode.Mac"; -#endif - UMA_HISTOGRAM_SPARSE_SLOWLY(kExceptionCodeString, + UMA_HISTOGRAM_SPARSE_SLOWLY("Crashpad.ExceptionCode." METRICS_OS_NAME, static_cast(exception_code)); } @@ -94,15 +99,9 @@ void Metrics::ExceptionEncountered() { } void Metrics::HandlerCrashed(uint32_t exception_code) { -#if defined(OS_WIN) - static const char kExceptionCodeString[] = - "Crashpad.HandlerCrash.ExceptionCode.Win"; -#elif defined(OS_MACOSX) - static const char kExceptionCodeString[] = - "Crashpad.HandlerCrash.ExceptionCode.Mac"; -#endif - UMA_HISTOGRAM_SPARSE_SLOWLY(kExceptionCodeString, - static_cast(exception_code)); + UMA_HISTOGRAM_SPARSE_SLOWLY( + "Crashpad.HandlerCrash.ExceptionCode." METRICS_OS_NAME, + static_cast(exception_code)); } } // namespace crashpad diff --git a/util/misc/uuid.cc b/util/misc/uuid.cc index 9ef55a65..21581cf3 100644 --- a/util/misc/uuid.cc +++ b/util/misc/uuid.cc @@ -23,6 +23,7 @@ #include #include "base/logging.h" +#include "base/rand_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "base/sys_byteorder.h" @@ -109,6 +110,16 @@ bool UUID::InitializeWithNew() { return false; } InitializeFromSystemUUID(&system_uuid); + return true; +#elif defined(OS_LINUX) + // Linux does not provide a UUID generator in a widely-available system + // library. uuid_generate() from libuuid is not available everywhere. + base::RandBytes(this, sizeof(*this)); + + // Set six bits per RFC 4122 §4.4 to identify this as a pseudo-random UUID. + data_3 = (4 << 12) | (data_3 & 0x0fff); // §4.1.3 + data_4[0] = 0x80 | (data_4[0] & 0x3f); // §4.1.1 + return true; #else #error Port. diff --git a/util/posix/close_multiple.cc b/util/posix/close_multiple.cc index fea7ca03..d46cd9db 100644 --- a/util/posix/close_multiple.cc +++ b/util/posix/close_multiple.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -25,12 +26,17 @@ #include #include +#include "base/files/scoped_file.h" #include "base/logging.h" #include "base/posix/eintr_wrapper.h" #include "build/build_config.h" #include "util/misc/implicit_cast.h" #include "util/numeric/safe_assignment.h" +#if defined(OS_MACOSX) +#include +#endif + // Everything in this file is expected to execute between fork() and exec(), // so everything called here must be acceptable in this context. However, // logging code that is not expected to execute under normal circumstances is @@ -100,10 +106,19 @@ bool CloseMultipleNowOrOnExecUsingFDDir(int fd, int preserve_fd) { return false; } - dirent entry; dirent* result; - int rv; - while ((rv = readdir_r(dir, &entry, &result)) == 0 && result != nullptr) { +#if defined(OS_LINUX) + // readdir_r() is deprecated as of glibc 2.24. See + // https://sourceware.org/bugzilla/show_bug.cgi?id=19056 and + // https://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit?id=0c52f6d623636a61eacd0f7b7a3bb942793a2a05. + const char kReaddirName[] = "readdir"; + while ((errno = 0, result = readdir(dir)) != nullptr) +#else + const char kReaddirName[] = "readdir_r"; + dirent entry; + while ((errno = readdir_r(dir, &entry, &result)) == 0 && result != nullptr) +#endif + { const char* entry_name = &(*result->d_name); if (strcmp(entry_name, ".") == 0 || strcmp(entry_name, "..") == 0) { continue; @@ -127,6 +142,11 @@ bool CloseMultipleNowOrOnExecUsingFDDir(int fd, int preserve_fd) { } } + if (errno != 0) { + PLOG(WARNING) << kReaddirName; + return false; + } + return true; } @@ -141,13 +161,61 @@ void CloseMultipleNowOrOnExec(int fd, int preserve_fd) { // system’s file descriptor limit. Check a few values and use the highest as // the limit, because these may be based on the file descriptor limit set by // setrlimit(), and higher-numbered file descriptors may have been opened - // prior to the limit being lowered. For Mac OS X, see 10.9.2 - // Libc-997.90.3/gen/FreeBSD/sysconf.c sysconf() and 10.9.4 - // xnu-2422.110.17/bsd/kern/kern_descrip.c getdtablesize(), which both return - // the current RLIMIT_NOFILE value, not the maximum possible file descriptor. - int max_fd = std::max(implicit_cast(sysconf(_SC_OPEN_MAX)), OPEN_MAX); + // prior to the limit being lowered. On both macOS and Linux glibc, both + // sysconf() and getdtablesize() return the current RLIMIT_NOFILE value, not + // the maximum possible file descriptor. For macOS, see 10.11.5 + // Libc-1082.50.1/gen/FreeBSD/sysconf.c sysconf() and 10.11.6 + // xnu-3248.60.10/bsd/kern/kern_descrip.c getdtablesize(). For Linux glibc, + // see glibc-2.24/sysdeps/posix/sysconf.c __sysconf() and + // glibc-2.24/sysdeps/posix/getdtsz.c __getdtablesize(). + int max_fd = implicit_cast(sysconf(_SC_OPEN_MAX)); max_fd = std::max(max_fd, getdtablesize()); +#if !defined(OS_LINUX) || defined(OPEN_MAX) + // Linux does not provide OPEN_MAX. See + // https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/linux/limits.h?id=77293034696e3e0b6c8b8fc1f96be091104b3d2b. + max_fd = std::max(max_fd, OPEN_MAX); +#endif + + // Consult a sysctl to determine the system-wide limit on the maximum number + // of open files per process. Note that it is possible to change this limit + // while the system is running, but it’s still a better upper bound than the + // current RLIMIT_NOFILE value. + +#if defined(OS_MACOSX) + // See 10.11.6 xnu-3248.60.10/bsd/kern/kern_resource.c maxfilesperproc, + // referenced by dosetrlimit(). + int oid[] = {CTL_KERN, KERN_MAXFILESPERPROC}; + int maxfilesperproc; + size_t maxfilesperproc_size = sizeof(maxfilesperproc); + if (sysctl(oid, + arraysize(oid), + &maxfilesperproc, + &maxfilesperproc_size, + nullptr, + 0) == 0) { + max_fd = std::max(max_fd, maxfilesperproc); + } else { + PLOG(WARNING) << "sysctl"; + } +#elif defined(OS_LINUX) + // See linux-4.4.27/fs/file.c sysctl_nr_open, referenced by kernel/sys.c + // do_prlimit() and kernel/sysctl.c fs_table. Inability to open this file is + // not considered an error, because /proc may not be available or usable. + { + base::ScopedFILE nr_open_file(fopen("/proc/sys/fs/nr_open", "r")); + if (nr_open_file.get() != nullptr) { + int nr_open; + if (fscanf(nr_open_file.get(), "%d\n", &nr_open) == 1 && + feof(nr_open_file.get())) { + max_fd = std::max(max_fd, nr_open); + } else { + LOG(WARNING) << "/proc/sys/fs/nr_open format error"; + } + } + } +#endif + for (int entry_fd = fd; entry_fd < max_fd; ++entry_fd) { if (entry_fd != preserve_fd) { CloseNowOrOnExec(entry_fd, true); diff --git a/util/thread/thread_log_messages.cc b/util/thread/thread_log_messages.cc index 7f67e458..dec111e7 100644 --- a/util/thread/thread_log_messages.cc +++ b/util/thread/thread_log_messages.cc @@ -46,14 +46,14 @@ class ThreadLogMessagesMaster { } ~ThreadLogMessagesMaster() { - DCHECK_EQ(logging::GetLogMessageHandler(), LogMessageHandler); + DCHECK_EQ(logging::GetLogMessageHandler(), &LogMessageHandler); logging::SetLogMessageHandler(nullptr); tls_.Free(); } void SetThreadMessageList(std::vector* message_list) { - DCHECK_EQ(logging::GetLogMessageHandler(), LogMessageHandler); + DCHECK_EQ(logging::GetLogMessageHandler(), &LogMessageHandler); DCHECK_NE(tls_.Get() != nullptr, message_list != nullptr); tls_.Set(message_list); } From e956a8252fc1c9ba79b2c4ff3f4d5baecdd8e390 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 31 Oct 2016 11:23:00 -0400 Subject: [PATCH 2/9] Port the util library to Android MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this change, it is possible to build crashpad_util for Android with clang. I built with NDK 13b (clang 3.8) at API 24 (current), API 21 (used by Chrome in 64-bit builds), and API 16 (used by Chrome in 32-bit builds). - In WeakFileHandleFileWriter::WriteIoVec(): Android does not expose the IOV_MAX macro, but its value can be obtained by calling sysconf(_SC_IOV_MAX). - In CloseMultipleNowOrOnExec(): API 21 removes getdtablesize(). Skip it, because it returned the same thing as sysconf(_SC_OPEN_MAX), which is already consulted. - Throughout: Various #ifdefs checking for OS_LINUX have been extended to also check for OS_ANDROID. In Chrome’s build_config.h (and thus mini_chromium’s), OS_LINUX is not defined when OS_ANDROID is. This has not been tested beyond building the crashpad_util target. BUG=crashpad:30 Change-Id: Ieb0bed736029d2d776c534e30e534f186e6fb663 Reviewed-on: https://chromium-review.googlesource.com/405267 Reviewed-by: Robert Sesek --- util/file/file_writer.cc | 14 ++++++++++++-- util/misc/uuid.cc | 2 +- util/posix/close_multiple.cc | 14 +++++++++++--- util/posix/drop_privileges.cc | 2 +- util/posix/symbolic_constants_posix.cc | 6 +++--- util/posix/symbolic_constants_posix_test.cc | 4 ++-- 6 files changed, 30 insertions(+), 12 deletions(-) diff --git a/util/file/file_writer.cc b/util/file/file_writer.cc index edbdb8d5..94f41f14 100644 --- a/util/file/file_writer.cc +++ b/util/file/file_writer.cc @@ -73,9 +73,19 @@ bool WeakFileHandleFileWriter::WriteIoVec(std::vector* iovecs) { iovec* iov = reinterpret_cast(&(*iovecs)[0]); size_t remaining_iovecs = iovecs->size(); +#if defined(OS_ANDROID) + // Android does not expose the IOV_MAX macro, but makes its value available + // via sysconf(). See Android 7.0.0 bionic/libc/bionic/sysconf.cpp sysconf(). + // Bionic defines IOV_MAX at bionic/libc/include/limits.h, but does not ship + // this file to the NDK as , substituting + // bionic/libc/include/bits/posix_limits.h. + const size_t kIovMax = sysconf(_SC_IOV_MAX); +#else + const size_t kIovMax = IOV_MAX; +#endif + while (size > 0) { - size_t writev_iovec_count = - std::min(remaining_iovecs, implicit_cast(IOV_MAX)); + size_t writev_iovec_count = std::min(remaining_iovecs, kIovMax); ssize_t written = HANDLE_EINTR(writev(file_handle_, iov, writev_iovec_count)); if (written < 0) { diff --git a/util/misc/uuid.cc b/util/misc/uuid.cc index 21581cf3..e4b4b31a 100644 --- a/util/misc/uuid.cc +++ b/util/misc/uuid.cc @@ -111,7 +111,7 @@ bool UUID::InitializeWithNew() { } InitializeFromSystemUUID(&system_uuid); return true; -#elif defined(OS_LINUX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) // Linux does not provide a UUID generator in a widely-available system // library. uuid_generate() from libuuid is not available everywhere. base::RandBytes(this, sizeof(*this)); diff --git a/util/posix/close_multiple.cc b/util/posix/close_multiple.cc index d46cd9db..f1d7773f 100644 --- a/util/posix/close_multiple.cc +++ b/util/posix/close_multiple.cc @@ -88,7 +88,7 @@ using ScopedDIR = std::unique_ptr; bool CloseMultipleNowOrOnExecUsingFDDir(int fd, int preserve_fd) { #if defined(OS_MACOSX) const char kFDDir[] = "/dev/fd"; -#elif defined(OS_LINUX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) const char kFDDir[] = "/proc/self/fd"; #endif @@ -167,9 +167,17 @@ void CloseMultipleNowOrOnExec(int fd, int preserve_fd) { // Libc-1082.50.1/gen/FreeBSD/sysconf.c sysconf() and 10.11.6 // xnu-3248.60.10/bsd/kern/kern_descrip.c getdtablesize(). For Linux glibc, // see glibc-2.24/sysdeps/posix/sysconf.c __sysconf() and - // glibc-2.24/sysdeps/posix/getdtsz.c __getdtablesize(). + // glibc-2.24/sysdeps/posix/getdtsz.c __getdtablesize(). For Android, see + // 7.0.0 bionic/libc/bionic/sysconf.cpp sysconf() and + // bionic/libc/bionic/ndk_cruft.cpp getdtablesize(). int max_fd = implicit_cast(sysconf(_SC_OPEN_MAX)); + +#if !defined(OS_ANDROID) + // getdtablesize() was removed effective Android 5.0.0 (API 21). Since it + // returns the same thing as the sysconf() above, just skip it. See + // https://android.googlesource.com/platform/bionic/+/462abab12b074c62c0999859e65d5a32ebb41951. max_fd = std::max(max_fd, getdtablesize()); +#endif #if !defined(OS_LINUX) || defined(OPEN_MAX) // Linux does not provide OPEN_MAX. See @@ -198,7 +206,7 @@ void CloseMultipleNowOrOnExec(int fd, int preserve_fd) { } else { PLOG(WARNING) << "sysctl"; } -#elif defined(OS_LINUX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) // See linux-4.4.27/fs/file.c sysctl_nr_open, referenced by kernel/sys.c // do_prlimit() and kernel/sysctl.c fs_table. Inability to open this file is // not considered an error, because /proc may not be available or usable. diff --git a/util/posix/drop_privileges.cc b/util/posix/drop_privileges.cc index 3fb0ab4a..5c809904 100644 --- a/util/posix/drop_privileges.cc +++ b/util/posix/drop_privileges.cc @@ -71,7 +71,7 @@ void DropPrivileges() { CHECK_EQ(setegid(egid), -1); } } -#elif defined(OS_LINUX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) PCHECK(setresgid(gid, gid, gid) == 0) << "setresgid"; PCHECK(setresuid(uid, uid, uid) == 0) << "setresuid"; diff --git a/util/posix/symbolic_constants_posix.cc b/util/posix/symbolic_constants_posix.cc index c952e82a..4ea2300b 100644 --- a/util/posix/symbolic_constants_posix.cc +++ b/util/posix/symbolic_constants_posix.cc @@ -14,9 +14,9 @@ #include "util/posix/symbolic_constants_posix.h" +#include #include #include -#include #include "base/macros.h" #include "base/strings/stringprintf.h" @@ -64,7 +64,7 @@ const char* kSignalNames[] = { "INFO", "USR1", "USR2", -#elif defined(OS_LINUX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) // sed -Ene 's/^#define[[:space:]]SIG([[:alnum:]]+)[[:space:]]+[[:digit:]]{1,2}([[:space:]]|$).*/ "\1",/p' // /usr/include/asm-generic/signal.h // and fix up by removing SIGIOT, SIGLOST, SIGUNUSED, and SIGRTMIN. @@ -101,7 +101,7 @@ const char* kSignalNames[] = { "SYS", #endif }; -#if defined(OS_LINUX) +#if defined(OS_LINUX) || defined(OS_ANDROID) // NSIG is 64 to account for real-time signals. static_assert(arraysize(kSignalNames) == 32, "kSignalNames length"); #else diff --git a/util/posix/symbolic_constants_posix_test.cc b/util/posix/symbolic_constants_posix_test.cc index 72e37bd4..6779a30d 100644 --- a/util/posix/symbolic_constants_posix_test.cc +++ b/util/posix/symbolic_constants_posix_test.cc @@ -65,7 +65,7 @@ const struct { #if defined(OS_MACOSX) {SIGEMT, "SIGEMT", "EMT"}, {SIGINFO, "SIGINFO", "INFO"}, -#elif defined(OS_LINUX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) {SIGPWR, "SIGPWR", "PWR"}, {SIGSTKFLT, "SIGSTKFLT", "STKFLT"}, #endif @@ -120,7 +120,7 @@ TEST(SymbolicConstantsPOSIX, SignalToString) { kSignalTestData[index].short_name); } -#if defined(OS_LINUX) +#if defined(OS_LINUX) || defined(OS_ANDROID) // NSIG is 64 to account for real-time signals. const int kSignalCount = 32; #else From d5a759c900aef7e23de4e8f57a777e57e042bfe4 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 31 Oct 2016 13:38:12 -0400 Subject: [PATCH 3/9] Update mini_chromium to 8e8d3cc9a245f1bf63296e97fb6ac1c90f6d86f5 9f129335dbe5 Add Android and ARM support to mini_chromium base 1d3e5ef89ad0 Link Linux/Android executables with -pie for position independence 8e8d3cc9a245 Add Android to filename_rules.gypi BUG=crashpad:30 Change-Id: Idace661e0ffa598f5c2a2a4af2d578355c101c56 Reviewed-on: https://chromium-review.googlesource.com/405567 Reviewed-by: Robert Sesek --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index 52333980..79677f35 100644 --- a/DEPS +++ b/DEPS @@ -25,7 +25,7 @@ deps = { '93cc6e2c23e4d5ebd179f388e67aa907d0dfd43d', 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '88e0a3e1a965c698d7ead8bcfc0cfb6aacdc3524', + '8e8d3cc9a245f1bf63296e97fb6ac1c90f6d86f5', 'buildtools': Var('chromium_git') + '/chromium/buildtools.git@' + 'f8fc76ea5ce4a60cda2fa5d7df3d4a62935b3113', From d1aafe78ea467cb722d3155f124c463aad3a5880 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 31 Oct 2016 14:58:30 -0400 Subject: [PATCH 4/9] Port the test library and crashpad_test_test to Linux/Android - Linux (but not Android) provides __fpurge() instead of fpurge(). - In multiprocess_exec_test_child, use getrlimit(RLIMIT_NOFILE) instead of max(sysconf(_SC_OPEN_MAX), OPEN_MAX, getdtablesize()). OPEN_MAX is not availble on Linux (but is in Android as a bogus value), and getdtablesize() is not available in Android since 5.0.0 (API 21). sysconf(_SC_OPEN_MAX) and getdtablesize() both return getrlimit(RLIMIT_NOFILE) on all relevant platforms. - Add a Linux/Android implementation of test::Paths::Executable(). - Respect TMPDIR for all POSIX platforms in ScopedTempDir::CreateTemporaryDirectory(). If TMPDIR is unset or empty, use /tmp, except on Android, where /tmp does not exist and /data/local/tmp is used instead. Also: - Fix the Mac and Windows implementations of test::Paths::Executable() to abort on fatal error, in line with the new Linux/Android version. BUG=crashpad:30 TEST=crashpad_test_test Change-Id: I98a50d8579b193c813ba79794be087649e94cc06 Reviewed-on: https://chromium-review.googlesource.com/405507 Reviewed-by: Robert Sesek --- test/multiprocess_exec_posix.cc | 12 +++++- test/multiprocess_exec_test_child.cc | 11 +++-- test/multiprocess_posix_test.cc | 2 +- test/paths_linux.cc | 62 ++++++++++++++++++++++++++++ test/paths_mac.cc | 5 ++- test/paths_win.cc | 6 ++- test/scoped_temp_dir_posix.cc | 24 +++++++++-- test/test.gyp | 8 ++++ 8 files changed, 119 insertions(+), 11 deletions(-) create mode 100644 test/paths_linux.cc diff --git a/test/multiprocess_exec_posix.cc b/test/multiprocess_exec_posix.cc index caf2b476..93363a01 100644 --- a/test/multiprocess_exec_posix.cc +++ b/test/multiprocess_exec_posix.cc @@ -25,6 +25,10 @@ #include "util/misc/scoped_forbid_return.h" #include "util/posix/close_multiple.h" +#if defined(OS_LINUX) +#include +#endif + namespace crashpad { namespace test { @@ -78,8 +82,14 @@ void MultiprocessExec::MultiprocessChild() { ASSERT_NE(read_handle, STDOUT_FILENO); ASSERT_EQ(STDIN_FILENO, fileno(stdin)); - int rv = fpurge(stdin); + int rv; + +#if defined(OS_LINUX) + __fpurge(stdin); +#else + rv = fpurge(stdin); ASSERT_EQ(0, rv) << ErrnoMessage("fpurge"); +#endif rv = HANDLE_EINTR(dup2(read_handle, STDIN_FILENO)); ASSERT_EQ(STDIN_FILENO, rv) << ErrnoMessage("dup2"); diff --git a/test/multiprocess_exec_test_child.cc b/test/multiprocess_exec_test_child.cc index c796e4a3..011a37d0 100644 --- a/test/multiprocess_exec_test_child.cc +++ b/test/multiprocess_exec_test_child.cc @@ -27,6 +27,7 @@ #endif #if defined(OS_POSIX) +#include #include #elif defined(OS_WIN) #include @@ -37,9 +38,13 @@ int main(int argc, char* argv[]) { // Make sure that there’s nothing open at any FD higher than 3. All FDs other // than stdin, stdout, and stderr should have been closed prior to or at // exec(). - int max_fd = std::max(static_cast(sysconf(_SC_OPEN_MAX)), OPEN_MAX); - max_fd = std::max(max_fd, getdtablesize()); - for (int fd = STDERR_FILENO + 1; fd < max_fd; ++fd) { + rlimit rlimit_nofile; + if (getrlimit(RLIMIT_NOFILE, &rlimit_nofile) != 0) { + abort(); + } + for (int fd = STDERR_FILENO + 1; + fd < static_cast(rlimit_nofile.rlim_cur); + ++fd) { if (close(fd) == 0 || errno != EBADF) { abort(); } diff --git a/test/multiprocess_posix_test.cc b/test/multiprocess_posix_test.cc index 191bdeff..be927111 100644 --- a/test/multiprocess_posix_test.cc +++ b/test/multiprocess_posix_test.cc @@ -14,8 +14,8 @@ #include "test/multiprocess.h" +#include #include -#include #include #include "base/macros.h" diff --git a/test/paths_linux.cc b/test/paths_linux.cc new file mode 100644 index 00000000..6e6da0e0 --- /dev/null +++ b/test/paths_linux.cc @@ -0,0 +1,62 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "test/paths.h" + +#include +#include + +#include +#include + +#include "base/logging.h" +#include "util/misc/implicit_cast.h" + +namespace crashpad { +namespace test { + +// static +base::FilePath Paths::Executable() { + // Linux does not provide a straightforward way to size the buffer before + // calling readlink(). Normally, the st_size field returned by lstat() could + // be used, but this is usually zero for things in /proc. + // + // The /proc filesystem does not provide any way to read “exe” links for + // pathnames longer than a page. See linux-4.4.27/fs/proc/base.c + // do_proc_readlink(), which allocates a single page to receive the path + // string. Coincidentally, the page size and PATH_MAX are normally the same + // value, although neither is strictly a limit on the length of a pathname. + // + // On Android, the smaller of the page size and PATH_MAX actually does serve + // as an effective limit on the length of an executable’s pathname. See + // Android 7.0.0 bionic/linker/linker.cpp get_executable_path(), which aborts + // via __libc_fatal() if the “exe” link can’t be read into a PATH_MAX-sized + // buffer. + std::string exe_path(std::max(implicit_cast(sysconf(_SC_PAGESIZE)), + implicit_cast(PATH_MAX)), + std::string::value_type()); + ssize_t exe_path_len = + readlink("/proc/self/exe", &exe_path[0], exe_path.size()); + if (exe_path_len < 0) { + PLOG(FATAL) << "readlink"; + } else if (static_cast(exe_path_len) >= exe_path.size()) { + LOG(FATAL) << "readlink"; + } + + exe_path.resize(exe_path_len); + return base::FilePath(exe_path); +} + +} // namespace test +} // namespace crashpad diff --git a/test/paths_mac.cc b/test/paths_mac.cc index f3dca57a..d16049e8 100644 --- a/test/paths_mac.cc +++ b/test/paths_mac.cc @@ -26,10 +26,11 @@ namespace test { base::FilePath Paths::Executable() { uint32_t executable_length = 0; _NSGetExecutablePath(nullptr, &executable_length); - DCHECK_GT(executable_length, 1u); + CHECK_GT(executable_length, 1u); + std::string executable_path(executable_length - 1, std::string::value_type()); int rv = _NSGetExecutablePath(&executable_path[0], &executable_length); - DCHECK_EQ(rv, 0); + CHECK_EQ(rv, 0); return base::FilePath(executable_path); } diff --git a/test/paths_win.cc b/test/paths_win.cc index 95d0264e..947acd5c 100644 --- a/test/paths_win.cc +++ b/test/paths_win.cc @@ -16,13 +16,17 @@ #include +#include "base/logging.h" + namespace crashpad { namespace test { // static base::FilePath Paths::Executable() { wchar_t executable_path[_MAX_PATH]; - GetModuleFileName(nullptr, executable_path, sizeof(executable_path)); + unsigned int len = + GetModuleFileName(nullptr, executable_path, arraysize(executable_path)); + PCHECK(len != 0 && len < arraysize(executable_path)) << "GetModuleFileName"; return base::FilePath(executable_path); } diff --git a/test/scoped_temp_dir_posix.cc b/test/scoped_temp_dir_posix.cc index 34db76f3..03681036 100644 --- a/test/scoped_temp_dir_posix.cc +++ b/test/scoped_temp_dir_posix.cc @@ -15,10 +15,12 @@ #include "test/scoped_temp_dir.h" #include +#include #include #include #include "base/logging.h" +#include "build/build_config.h" #include "gtest/gtest.h" #include "test/errors.h" @@ -33,9 +35,25 @@ void ScopedTempDir::Rename() { // static base::FilePath ScopedTempDir::CreateTemporaryDirectory() { - char dir_template[] = "/tmp/org.chromium.crashpad.test.XXXXXX"; - PCHECK(mkdtemp(dir_template)) << "mkdtemp " << dir_template; - return base::FilePath(dir_template); + char* tmpdir = getenv("TMPDIR"); + std::string dir; + if (tmpdir && tmpdir[0] != '\0') { + dir.assign(tmpdir); + } else { +#if defined(OS_ANDROID) + dir.assign("/data/local/tmp"); +#else + dir.assign("/tmp"); +#endif + } + + if (dir[dir.size() - 1] != '/') { + dir.append(1, '/'); + } + dir.append("org.chromium.crashpad.test.XXXXXX"); + + PCHECK(mkdtemp(&dir[0])) << "mkdtemp " << dir; + return base::FilePath(dir); } // static diff --git a/test/test.gyp b/test/test.gyp index e8c1709c..666a68a1 100644 --- a/test/test.gyp +++ b/test/test.gyp @@ -47,6 +47,7 @@ 'multiprocess_posix.cc', 'paths.cc', 'paths.h', + 'paths_linux.cc', 'paths_mac.cc', 'paths_win.cc', 'scoped_temp_dir.cc', @@ -76,6 +77,13 @@ }, }], ], + 'target_conditions': [ + ['OS=="android"', { + 'sources/': [ + ['include', '^paths_linux\\.cc$'], + ], + }], + ], }, ], } From b978b03fa188c3156dfe431f0b939e69d840f321 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 31 Oct 2016 15:48:24 -0400 Subject: [PATCH 5/9] Port most of crashpad_util_test to Linux/Android - In the ProcessInfo test, port the global argc/argv getter to Linux by reading /proc/self/cmdline. - Use format macros for 64-bit types. - Only #include on macOS. - #include instead of . In order to test on Linux/Android, the following changes to the crashpad_util_test target must be made until more porting is complete: - Remove the dependency on crashpad_client because that library has not been ported yet. - Remove process_info_test.cc because it depends on crashpad_client and there is no implementation of ProcessInfo for Linux yet. - Remove http_transport_test.cc because there is no HTTPTransport implementation for Linux or Android yet. - Remove checked_address_range_test.cc because checked_address_range.cc does not yet expose a cross-bit usable type for addresses and sizes on Linux. BUG=crashpad:30 TEST=crashpad_util_test Change-Id: Ic17cf26bdf19b3eff3915bb1acdaa701f28222cd Reviewed-on: https://chromium-review.googlesource.com/405647 Reviewed-by: Robert Sesek --- util/misc/clock_test.cc | 6 ++--- util/numeric/checked_address_range_test.cc | 6 ++--- util/posix/process_info.h | 2 +- util/posix/process_info_test.cc | 30 +++++++++++++++++++++ util/posix/symbolic_constants_posix_test.cc | 2 +- 5 files changed, 38 insertions(+), 8 deletions(-) diff --git a/util/misc/clock_test.cc b/util/misc/clock_test.cc index c6e2c02a..d7f9cd71 100644 --- a/util/misc/clock_test.cc +++ b/util/misc/clock_test.cc @@ -14,11 +14,11 @@ #include "util/misc/clock.h" -#include #include #include +#include "base/format_macros.h" #include "base/logging.h" #include "base/macros.h" #include "base/strings/stringprintf.h" @@ -85,8 +85,8 @@ TEST(Clock, SleepNanoseconds) { for (size_t index = 0; index < arraysize(kTestData); ++index) { const uint64_t nanoseconds = kTestData[index]; - SCOPED_TRACE( - base::StringPrintf("index %zu, nanoseconds %llu", index, nanoseconds)); + SCOPED_TRACE(base::StringPrintf( + "index %zu, nanoseconds %" PRIu64, index, nanoseconds)); TestSleepNanoseconds(nanoseconds); } diff --git a/util/numeric/checked_address_range_test.cc b/util/numeric/checked_address_range_test.cc index 6b7e4c43..403c000f 100644 --- a/util/numeric/checked_address_range_test.cc +++ b/util/numeric/checked_address_range_test.cc @@ -122,7 +122,7 @@ TEST(CheckedAddressRange, IsValid) { for (size_t index = 0; index < arraysize(kTestData); ++index) { const TestData& testcase = kTestData[index]; SCOPED_TRACE(base::StringPrintf("index %" PRIuS - ", base 0x%llx, size 0x%llx", + ", base 0x%" PRIx64 ", size 0x%" PRIx64, index, testcase.base, testcase.size)); @@ -173,7 +173,7 @@ TEST(CheckedAddressRange, ContainsValue) { for (size_t index = 0; index < arraysize(kTestData); ++index) { const TestData& testcase = kTestData[index]; SCOPED_TRACE(base::StringPrintf( - "index %" PRIuS ", value 0x%llx", index, testcase.value)); + "index %" PRIuS ", value 0x%" PRIx64, index, testcase.value)); EXPECT_EQ(testcase.expectation, parent_range_32.ContainsValue(testcase.value)); @@ -230,7 +230,7 @@ TEST(CheckedAddressRange, ContainsRange) { for (size_t index = 0; index < arraysize(kTestData); ++index) { const TestData& testcase = kTestData[index]; SCOPED_TRACE(base::StringPrintf("index %" PRIuS - ", base 0x%llx, size 0x%llx", + ", base 0x%" PRIx64 ", size 0x%" PRIx64, index, testcase.base, testcase.size)); diff --git a/util/posix/process_info.h b/util/posix/process_info.h index 5aeda25a..1aa23d8c 100644 --- a/util/posix/process_info.h +++ b/util/posix/process_info.h @@ -15,7 +15,6 @@ #ifndef CRASHPAD_UTIL_POSIX_PROCESS_INFO_H_ #define CRASHPAD_UTIL_POSIX_PROCESS_INFO_H_ -#include #include #include #include @@ -30,6 +29,7 @@ #if defined(OS_MACOSX) #include +#include #endif namespace crashpad { diff --git a/util/posix/process_info_test.cc b/util/posix/process_info_test.cc index dd0844be..7e28e5f4 100644 --- a/util/posix/process_info_test.cc +++ b/util/posix/process_info_test.cc @@ -15,12 +15,14 @@ #include "util/posix/process_info.h" #include +#include #include #include #include #include +#include "base/files/scoped_file.h" #include "build/build_config.h" #include "gtest/gtest.h" #include "test/errors.h" @@ -98,6 +100,34 @@ void TestSelfProcess(const ProcessInfo& process_info) { #if defined(OS_MACOSX) int expect_argc = *_NSGetArgc(); char** expect_argv = *_NSGetArgv(); +#elif defined(OS_LINUX) || defined(OS_ANDROID) + std::vector expect_arg_vector; + { + base::ScopedFILE cmdline(fopen("/proc/self/cmdline", "r")); + ASSERT_NE(nullptr, cmdline.get()) << ErrnoMessage("fopen"); + + int expect_arg_char; + std::string expect_arg_string; + while ((expect_arg_char = fgetc(cmdline.get())) != EOF) { + if (expect_arg_char != '\0') { + expect_arg_string.append(1, expect_arg_char); + } else { + expect_arg_vector.push_back(expect_arg_string); + expect_arg_string.clear(); + } + } + ASSERT_EQ(0, ferror(cmdline.get())) << ErrnoMessage("fgetc"); + ASSERT_TRUE(expect_arg_string.empty()); + } + + std::vector expect_argv_storage; + for (const std::string& expect_arg_string : expect_arg_vector) { + expect_argv_storage.push_back(expect_arg_string.c_str()); + } + + int expect_argc = expect_argv_storage.size(); + const char* const* expect_argv = + !expect_argv_storage.empty() ? &expect_argv_storage[0] : nullptr; #else #error Obtain expect_argc and expect_argv correctly on your system. #endif diff --git a/util/posix/symbolic_constants_posix_test.cc b/util/posix/symbolic_constants_posix_test.cc index 6779a30d..ddbe336e 100644 --- a/util/posix/symbolic_constants_posix_test.cc +++ b/util/posix/symbolic_constants_posix_test.cc @@ -14,7 +14,7 @@ #include "util/posix/symbolic_constants_posix.h" -#include +#include #include #include "base/macros.h" From c2814e25191212f492fe439ac096d9193a0d683a Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Mon, 31 Oct 2016 14:16:12 -0700 Subject: [PATCH 6/9] Don't throttle explicitly requested uploads R=mark@chromium.org BUG=chromium:660955 Change-Id: Ia31846fe3487a52f4cad34859e23a7192ca4065e Reviewed-on: https://chromium-review.googlesource.com/405533 Reviewed-by: Mark Mentovai --- handler/crash_report_upload_thread.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 23dda482..5bd1fbf9 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -207,9 +207,12 @@ void CrashReportUploadThread::ProcessPendingReport( // hour, and retire reports that would exceed this limit or for which the // upload fails on the first attempt. // + // If upload was requested explicitly (i.e. by user action), we do not + // throttle the upload. + // // TODO(mark): Provide a proper rate-limiting strategy and allow for failed // upload attempts to be retried. - if (rate_limit_) { + if (!report.upload_explicitly_requested && rate_limit_) { time_t last_upload_attempt_time; if (settings->GetLastUploadAttemptTime(&last_upload_attempt_time)) { time_t now = time(nullptr); From fd751f4708cc6f7d95ff21f9f300403b7158573b Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 31 Oct 2016 18:04:21 -0400 Subject: [PATCH 7/9] Correct StringToUnsignedInt[64]() StringToUnsignedInt[64]Traits::Convert() was returning in its failure (negative input) case without touching *end. Its caller relies on *end to detect failure. Change-Id: I636f95471cd499434743e73f0e5e0b60c0871795 Reviewed-on: https://chromium-review.googlesource.com/405468 Reviewed-by: Robert Sesek --- util/stdlib/string_number_conversion.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/util/stdlib/string_number_conversion.cc b/util/stdlib/string_number_conversion.cc index 373428b9..28f6ea11 100644 --- a/util/stdlib/string_number_conversion.cc +++ b/util/stdlib/string_number_conversion.cc @@ -103,6 +103,7 @@ struct StringToUnsignedIntTraits : public StringToUnsignedIntegerTraits { static LongType Convert(const char* str, char** end, int base) { if (str[0] == '-') { + *end = const_cast(str); return 0; } return strtoul(str, end, base); @@ -113,6 +114,7 @@ struct StringToUnsignedInt64Traits : public StringToUnsignedIntegerTraits { static LongType Convert(const char* str, char** end, int base) { if (str[0] == '-') { + *end = const_cast(str); return 0; } return strtoull(str, end, base); From e7bd798af43800089fb25a9d911f28cd388021e5 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 1 Nov 2016 14:06:13 -0400 Subject: [PATCH 8/9] Update build/test and status documentation to reflect Android BUG=crashpad:30 Change-Id: I0170e95e43146f6a2af6b6753c5197794bd83817 Reviewed-on: https://chromium-review.googlesource.com/406307 Reviewed-by: Robert Sesek --- doc/developing.ad | 107 ++++++++++++++++++++++++++++++++++++++++++++++ doc/status.ad | 2 +- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/doc/developing.ad b/doc/developing.ad index 9174f6b1..952ed7cb 100644 --- a/doc/developing.ad +++ b/doc/developing.ad @@ -104,6 +104,80 @@ Ninja is part of the https://dev.chromium.org/developers/how-tos/depottools[depot_tools]. There’s no need to install it separately. +=== Android + +Crashpad’s Android port is in its early stages. This build relies on +cross-compilation. It’s possible to develop Crashpad for Android on any platform +that the https://developer.android.com/ndk/[Android NDK (Native Development +Kit)] runs on. + +If it’s not already present on your system, +https://developer.android.com/ndk/downloads/[download the NDK package for your +system] and expand it to a suitable location. These instructions assume that +it’s been expanded to `~/android-ndk-r13`. + +To build Crashpad, portions of the NDK must be reassembled into a +https://developer.android.com/ndk/guides/standalone_toolchain.html[standalone +toolchain]. This is a repackaged subset of the NDK suitable for cross-compiling +for a single Android architecture (such as `arm`, `arm64`, `x86`, and `x86_64`) +targeting a specific +https://source.android.com/source/build-numbers.html[Android API level]. The +standalone toolchain only needs to be built from the NDK one time for each set +of options desired. To build a standalone toolchain targeting 64-bit ARM and API +level 21 (Android 5.0 “Lollipop”), run: + +[subs="verbatim,quotes"] +---- +$ *cd ~* +$ *python android-ndk-r13/build/tools/make_standalone_toolchain.py \ + --arch=arm64 --api=21 --install-dir=android-ndk-r13_arm64_api21* +---- + +Note that Chrome uses Android API level 21 for 64-bit platforms and 16 for +32-bit platforms. See Chrome’s +https://chromium.googlesource.com/chromium/src/+/master/build/config/android/config.gni[`build/config/android/config.gni`] +which sets `_android_api_level` and `_android64_api_level`. + +To configure a Crashpad build for Android using this standalone toolchain, +set several environment variables directing the build to the standalone +toolchain, along with GYP options to identify an Android build. This must be +done after any `gclient sync`, or instead of any `gclient runhooks` operation. +The environment variables only need to be set for this `gyp_crashpad.py` +invocation, and need not be permanent. + +[subs="verbatim,quotes"] +---- +$ *cd \~/crashpad/crashpad* +$ *CC_target=\~/android-ndk-r13_arm64_api21/bin/clang \ + CXX_target=\~/android-ndk-r13_arm64_api21/bin/clang++ \ + AR_target=\~/android-ndk-r13_arm64_api21/bin/aarch64-linux-android-ar \ + NM_target=\~/android-ndk-r13_arm64_api21/bin/aarch64-linux-android-nm \ + READELF_target=~/android-ndk-r13_arm64_api21/bin/aarch64-linux-android-readelf \ + python build/gyp_crashpad.py \ + -DOS=android -Dtarget_arch=arm64 -Dclang=1 \ + --generator-output=out_android_arm64_api21 -f ninja-android* +---- + +Target “triplets” to use for `ar`, `nm`, and `readelf` are: + +[width="40%",cols="1,3",frame="topbot"] +|=== +|`arm` |`arm-linux-androideabi` +|`arm64` |`aarch64-linux-android` +|`x86` |`i686-linux-android` +|`x86_64` |`x86_64-linux-android` +|=== + +The port is incomplete, but targets known to be working include `crashpad_util`, +`crashpad_test`, and `crashpad_test_test`. This list will grow over time. To +build, direct `ninja` to the specific `out` directory chosen by +`--generator-output` above. + +[subs="verbatim,quotes"] +---- +$ *ninja -C out_android_arm64_api21/out/Debug crashpad_test_test* +---- + == Testing Crashpad uses https://github.com/google/googletest/[Google Test] as its @@ -130,6 +204,39 @@ $ *cd ~/crashpad/crashpad* $ *python build/run_tests.py Debug* ---- +=== Android + +To test on Android, use +https://developer.android.com/studio/command-line/adb.html[ADB (Android Debug +Bridge)] to `adb push` test executables and test data to a device or emulator, +then use `adb shell` to get a shell to run the test executables from. ADB is +part of the https://developer.android.com/sdk/[Android SDK]. Note that it is +sufficient to install just the command-line tools. The entire Android Studio IDE +is not necessary to obtain ADB. + +This example runs `crashpad_test_test` on a device. This test executable has a +run-time dependency on a second executable and a test data file, which are also +transferred to the device prior to running the test. + +[subs="verbatim,quotes"] +---- +$ *cd ~/crashpad/crashpad* +$ *adb push out_android_arm64_api21/out/Debug/crashpad_test_test /data/local/tmp/* +[100%] /data/local/tmp/crashpad_test_test +$ *adb push \ + out_android_arm64_api21/out/Debug/crashpad_test_test_multiprocess_exec_test_child \ + /data/local/tmp/* +[100%] /data/local/tmp/crashpad_test_test_multiprocess_exec_test_child +$ *adb shell mkdir -p /data/local/tmp/crashpad_test_data_root/test* +$ *adb push test/paths_test_data_root.txt \ + /data/local/tmp/crashpad_test_data_root/test/* +[100%] /data/local/tmp/crashpad_test_data_root/test/paths_test_data_root.txt +$ *adb shell* +device:/ $ *cd /data/local/tmp* +device:/data/local/tmp $ *CRASHPAD_TEST_DATA_ROOT=crashpad_test_data_root \ + ./crashpad_test_test* +---- + == Contributing Crashpad’s contribution process is very similar to diff --git a/doc/status.ad b/doc/status.ad index 90982140..6d45dd45 100644 --- a/doc/status.ad +++ b/doc/status.ad @@ -31,7 +31,7 @@ https://chromium.googlesource.com/chromium/src/\+/cfa5b01bb1d06bf96967bd37e21a44 Initial work on a Crashpad client for https://crashpad.chromium.org/bug/30[Android] has begun. This is currently in -the design phase. +the early implementation phase. == Future From e616638c9d87ca854877ba563dbbca40a5ddc8a6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 1 Nov 2016 14:55:40 -0400 Subject: [PATCH 9/9] Replace Rietveld with Gerrit in the developer documentation Also, update a few links for good measure. Change-Id: I47113a4f324e4ad6ba02aa46bae821eefd4d98ea Reviewed-on: https://chromium-review.googlesource.com/406347 Reviewed-by: Robert Sesek --- doc/developing.ad | 57 +++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/doc/developing.ad b/doc/developing.ad index 952ed7cb..858c4008 100644 --- a/doc/developing.ad +++ b/doc/developing.ad @@ -38,7 +38,7 @@ present in the `$PATH` environment variable: * Chromium’s https://dev.chromium.org/developers/how-tos/depottools[depot_tools]. - * http://git-scm.com/[Git]. This is provided by Xcode on Mac OS X and by + * https://git-scm.com/[Git]. This is provided by Xcode on Mac OS X and by depot_tools on Windows. * https://www.python.org/[Python]. This is provided by the operating system on Mac OS X, and by depot_tools on Windows. @@ -69,8 +69,8 @@ $ *cd ~/crashpad* $ *fetch crashpad* ---- -`fetch crashpad` performs the initial `gclient sync`, establishing a -fully-functional local checkout. +`fetch crashpad` performs the initial `git clone` and `gclient sync`, +establishing a fully-functional local checkout. === Subsequent Checkouts @@ -84,10 +84,10 @@ $ *gclient sync* == Building Crashpad uses https://gyp.gsrc.io/[GYP] to generate -https://martine.github.io/ninja/[Ninja] build files. The build is described by -`.gyp` files throughout the Crashpad source code tree. The -`build/gyp_crashpad.py` script runs GYP properly for Crashpad, and is also -called when you run `fetch crashpad`, `gclient sync`, or `gclient runhooks`. +https://ninja-build.org/[Ninja] build files. The build is described by `.gyp` +files throughout the Crashpad source code tree. The `build/gyp_crashpad.py` +script runs GYP properly for Crashpad, and is also called when you run `fetch +crashpad`, `gclient sync`, or `gclient runhooks`. The Ninja build files and build output are in the `out` directory. Both debug- and release-mode configurations are available. The examples below show the debug @@ -246,9 +246,9 @@ process]. === Code Review A code review must be conducted for every change to Crashpad’s source code. Code -review is conducted on https://codereview.chromium.org/[Chromium’s Rietveld] -system, and all code reviews must be sent to an appropriate reviewer, with a Cc -sent to +review is conducted on https://chromium-review.googlesource.com/[Chromium’s +Gerrit] system, and all code reviews must be sent to an appropriate reviewer, +with a Cc sent to https://groups.google.com/a/chromium.org/group/crashpad-dev[crashpad-dev]. The `codereview.settings` file specifies this environment to `git-cl`. @@ -266,14 +266,18 @@ $ *git commit* $ *git cl upload* ---- -Uploading a patch to Rietveld does not automatically request a review. You must -select a reviewer and mail your request to them (with a Cc to crashpad-dev) from -the Rietveld issue page after running `git cl upload`. If you have lost track of -the issue page, `git cl issue` will remind you of its URL. Alternatively, you -can request review when uploading to Rietveld by using `git cl upload ---send-mail` +The https://polygerrit.appspot.com/[PolyGerrit interface] to Gerrit, undergoing +active development, is recommended. To switch from the classic GWT-based Gerrit +UI to PolyGerrit, click the PolyGerrit link in a Gerrit review page’s footer. -Git branches maintain their association with Rietveld issues, so if you need to +Uploading a patch to Gerrit does not automatically request a review. You must +select a reviewer on the Gerrit review page after running `git cl upload`. This +action notifies your reviewer of the code review request. If you have lost track +of the review page, `git cl issue` will remind you of its URL. Alternatively, +you can request review when uploading to Gerrit by using `git cl upload +--send-mail`. + +Git branches maintain their association with Gerrit reviews, so if you need to make changes based on review feedback, you can do so on the correct Git branch, committing your changes locally with `git commit`. You can then upload a new patch set with `git cl upload` and let your reviewer know you’ve addressed the @@ -281,8 +285,8 @@ feedback. === Landing Changes -After code review is complete and “LGTM” (“looks good to me”) has been received -from all reviewers, project members can commit the patch themselves: +After code review is complete and “Code-Review: +1”) has been received from all +reviewers, project members can commit the patch themselves: [subs="verbatim,quotes"] ---- @@ -291,19 +295,14 @@ $ *git checkout work_branch* $ *git cl land* ---- +Alternatively, patches can be committed by clicking the “Submit” button in the +Gerrit UI. + Crashpad does not currently have a https://dev.chromium.org/developers/testing/commit-queue[commit queue], so -contributors that are not project members will have to ask a project member to +contributors who are not project members will have to ask a project member to commit the patch for them. Project members can commit changes on behalf of -external contributors by patching the change into a local branch and landing it: - -[subs="verbatim,quotes"] ----- -$ *cd ~/crashpad/crashpad* -$ *git checkout -b for_external_contributor origin/master* -$ *git cl patch 12345678* _# 12345678 is the Rietveld issue number_ -$ *git cl land -c \'External Contributor '* ----- +external contributors by clicking the “Submit” button in the Gerrit UI. === External Contributions