Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16770006:
Implement basic stack traces on Android and reenable unit tests. (Closed)
|
Created:
7 years, 6 months ago by scherkus (not reviewing) Modified:
7 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, michaelbai Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement basic stack traces on Android and reenable unit tests.
Since we install stripped binaries in APKs, print out object file names and relocatable addresses that can be used on the host machine to complete symbolizing and demangling the stack trace using unstripped binaries.
In addition the implementation of StackTrace::PrintBacktrace() was changed to log the stack trace instead of sending SIGSTKFLT.
BUG=248775, 248784
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210132
Patch Set 1 #
Total comments: 9
Patch Set 2 : refactor symbolize #
Total comments: 15
Patch Set 3 : fixes #
Total comments: 10
Patch Set 4 : fixes #Patch Set 5 : use proc maps #Patch Set 6 : rebase #Patch Set 7 : #
Total comments: 11
Patch Set 8 : fixes #
Total comments: 2
Patch Set 9 : rebase #
Messages
Total messages: 25 (0 generated)
this is my take on getting basic stack traces on Android that we can feed into addr2line/c++filt on the host machine satorux: I'm really abusing google::Symbolize() and would love to get your input! digit: in case you have any words of wisdom w.r.t. stack traces https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:134: size = readlink(path, buf, sizeof(buf)); satorux: this is also kind of unnecessary since symbolize has the filename -- it would be nice to simply get that bit of information :) https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:146: google::InstallSymbolizeCallback(&OnSymbolizeCallback); satorux: is this intended usage? I couldn't find any existing usage in the Chromium code :\ https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:160: // TODO(scherkus): HACK - we blindly print contents of |buf| knowing that satorux: this is also really gross I want to use symbolize's /proc/self/map mapping code + symbolizing + demanging instead of duplicating the code from Android's bionic. Knowing that on Android we'll fail to symbolize most of the time due to using stripped binaries, my goal is to get the file name + relocatable address any ideas?
https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:101: void StackTrace::PrintBacktrace() const { PrintBacktrace() and OutputToStream() will behave very differently. Is it a good thing? In strack_trace_posix.cc, the two functions behave consistently. https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:134: size = readlink(path, buf, sizeof(buf)); On 2013/06/12 00:43:43, scherkus wrote: > satorux: this is also kind of unnecessary since symbolize has the filename -- it > would be nice to simply get that bit of information :) I think it's easy to change symbolize.cc to provide the file name to this callback. I think it's fine to make local modifications like that. https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:146: google::InstallSymbolizeCallback(&OnSymbolizeCallback); On 2013/06/12 00:43:43, scherkus wrote: > satorux: is this intended usage? I couldn't find any existing usage in the > Chromium code :\ IIRC, this awkward callback hook was introduced to decouple some dependency (we wanted to inject a debug info reader through this callback). This is not intended usage, but it should be ok to use this way. https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:160: // TODO(scherkus): HACK - we blindly print contents of |buf| knowing that On 2013/06/12 00:43:43, scherkus wrote: > satorux: this is also really gross > > I want to use symbolize's /proc/self/map mapping code + symbolizing + demanging > instead of duplicating the code from Android's bionic. Knowing that on Android > we'll fail to symbolize most of the time due to using stripped binaries, my goal > is to get the file name + relocatable address > > any ideas? If Symbolize() fails most of the time, using this function only to get extra bits of information (pc, relocation, fd) sounds rather awkward. Maybe: 1) Copy the code for reading /proc/self/maps from symbolize.cc. Copying code is not desirable in general, but it's not a big amount of code. 2) Refactor symbolize.cc so you can achieve your goal cleanly. This will make it hard to follow the upstream, but this may not be a big deal, as we don't seem to be following the upstream anyway. 3) Write your own minimal /proc/self/maps reader here, that suits your purpose perfectly. 4) Write a generic /proc/self/maps reader and make it a separate base library like base/debug/proc_maps_reader.h
PTAL https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:101: void StackTrace::PrintBacktrace() const { On 2013/06/12 05:43:42, satorux1 wrote: > PrintBacktrace() and OutputToStream() will behave very differently. Is it a good > thing? In strack_trace_posix.cc, the two functions behave consistently. I noticed this too -- I filed a bug and am trying to track down the owner https://codereview.chromium.org/16770006/diff/1/base/debug/stack_trace_androi... base/debug/stack_trace_android.cc:160: // TODO(scherkus): HACK - we blindly print contents of |buf| knowing that On 2013/06/12 05:43:42, satorux1 wrote: > On 2013/06/12 00:43:43, scherkus wrote: > > satorux: this is also really gross > > > > I want to use symbolize's /proc/self/map mapping code + symbolizing + > demanging > > instead of duplicating the code from Android's bionic. Knowing that on Android > > we'll fail to symbolize most of the time due to using stripped binaries, my > goal > > is to get the file name + relocatable address > > > > any ideas? > > If Symbolize() fails most of the time, using this function only to get extra > bits of information (pc, relocation, fd) sounds rather awkward. Maybe: > > 1) Copy the code for reading /proc/self/maps from symbolize.cc. Copying code is > not desirable in general, but it's not a big amount of code. > > 2) Refactor symbolize.cc so you can achieve your goal cleanly. This will make it > hard to follow the upstream, but this may not be a big deal, as we don't seem to > be following the upstream anyway. > > 3) Write your own minimal /proc/self/maps reader here, that suits your purpose > perfectly. > > 4) Write a generic /proc/self/maps reader and make it a separate base library > like base/debug/proc_maps_reader.h I went with (2) -- let me know if you think it's too much of a divergence https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... File base/third_party/symbolize/symbolize.cc (right): https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:593: char buf[256]; this additional buffer allocation is a bit crummy I was thinking we _could_ use |out| and |out_size| as a temporary buffer... but that would place some hidden requirements on the size of the buffer the caller provides
https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... base/debug/stack_trace_android.cc:132: uint64 start_address; Please move them to where these are used. I'd also suggest to initialize start_address with 0, to be extra defensive. it's more like a habit, though. https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... base/debug/stack_trace_android.cc:134: *os << "#" << std::dec << std::setfill('0') << std::setw(2) << i << " "; For formatting, I think base::StringPrintf() is more common than streams in Chrome code, and easier to read. If you plan to make this async-signal-safe, you should format this in a very low level fashion. stack_trace_posix.cc does thi using itoa_r(). That may be too much work to do in this first version. https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... File base/third_party/symbolize/symbolize.cc (right): https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:488: uint64_t pc0 = reinterpret_cast<uintptr_t>(pc); pc0 looks a bit cryptic. maybe: void *in_pc, ...) { uint64_t pc = reinterpret_cast<uintptr_t>(in_pc); https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:571: if (strlen(cursor) >= out_size) { maybe slightly easier to read: if (stlren(cursor) + 1 > out_size) { // +1 for '\0' https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:593: char buf[256]; On 2013/06/13 01:27:44, scherkus wrote: > this additional buffer allocation is a bit crummy > > I was thinking we _could_ use |out| and |out_size| as a temporary buffer... but > that would place some hidden requirements on the size of the buffer the caller > provides sounds reasonable. buf -> file_name? https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:599: int object_fd; object_fd = - 1; to be extra defensive. https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... File base/third_party/symbolize/symbolize.h (right): https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.h:132: // the specified pc. If found, copy the path name to "out" and parsed starting fpath name -> file name, to be consistent with the function name. please also mention that the file name includes the trailing '\0',
thanks for the quick review! https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... base/debug/stack_trace_android.cc:132: uint64 start_address; On 2013/06/13 01:58:10, satorux1 wrote: > Please move them to where these are used. > > I'd also suggest to initialize start_address with 0, to be extra defensive. it's > more like a habit, though. Done. https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... base/debug/stack_trace_android.cc:134: *os << "#" << std::dec << std::setfill('0') << std::setw(2) << i << " "; On 2013/06/13 01:58:10, satorux1 wrote: > For formatting, I think base::StringPrintf() is more common than streams in > Chrome code, and easier to read. > > If you plan to make this async-signal-safe, you should format this in a very low > level fashion. stack_trace_posix.cc does thi using itoa_r(). That may be too > much work to do in this first version. Yeah -- part of me wanted to make things async safe from the get-go, but that's a bit too much for V1. Also after I figure out what's going on with http://crbug.com/248775 and some other related work I have a feeling we could possibly merge this in w/ the posix code https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... File base/third_party/symbolize/symbolize.cc (right): https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:488: uint64_t pc0 = reinterpret_cast<uintptr_t>(pc); On 2013/06/13 01:58:10, satorux1 wrote: > pc0 looks a bit cryptic. maybe: > > void *in_pc, ...) { > uint64_t pc = reinterpret_cast<uintptr_t>(in_pc); I was following SymbolizeAndDemangle() but agree it's a bit cryptic. This file is also a bit inconsistent with respect to using void*, uintptr_t, and uint64_t for addresses but I wanted to keep the diff small. Let me know if want to see any other changes :) https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:571: if (strlen(cursor) >= out_size) { On 2013/06/13 01:58:10, satorux1 wrote: > maybe slightly easier to read: > > if (stlren(cursor) + 1 > out_size) { // +1 for '\0' Done + beefed up comment on next line https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.cc:599: int object_fd; On 2013/06/13 01:58:10, satorux1 wrote: > object_fd = - 1; > > to be extra defensive. Done. https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... File base/third_party/symbolize/symbolize.h (right): https://codereview.chromium.org/16770006/diff/8001/base/third_party/symbolize... base/third_party/symbolize/symbolize.h:132: // the specified pc. If found, copy the path name to "out" and parsed starting On 2013/06/13 01:58:10, satorux1 wrote: > fpath name -> file name, to be consistent with the function name. > > please also mention that the file name includes the trailing '\0', Done.
LGTM, but you need LGTM from base/OWNERS. willchan@ may be interested in this stuff. https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/8001/base/debug/stack_trace_and... base/debug/stack_trace_android.cc:134: *os << "#" << std::dec << std::setfill('0') << std::setw(2) << i << " "; On 2013/06/13 02:20:46, scherkus wrote: > On 2013/06/13 01:58:10, satorux1 wrote: > > For formatting, I think base::StringPrintf() is more common than streams in > > Chrome code, and easier to read. > > > > If you plan to make this async-signal-safe, you should format this in a very > low > > level fashion. stack_trace_posix.cc does thi using itoa_r(). That may be too > > much work to do in this first version. > > Yeah -- part of me wanted to make things async safe from the get-go, but that's > a bit too much for V1. > > Also after I figure out what's going on with http://crbug.com/248775 and some > other related work I have a feeling we could possibly merge this in w/ the posix > code For now, maybe add a TODO comment? https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:140: *os << base::StringPrintf("pc %08x %s", rel_pc, path); http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... %08x -> %08"PRIx64" who said printf was easier to read? :)
https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:102: // stderr. Instead it crashes the program http://crbug.com/248775 Small clarification here: outputting to stderr will only work if you run the program as a standalone executable. Otherwise, everything will be lost. For regular APKs (like most Chrome unittests on Android these day), you should really send this to the log (with __android_log_print, for example). An exception would be a unit tests that reads the stderr() output, but that would probably a bad idea anyway. It's possible to test for this condition by looking at the username with getpwnam(), if it is "root" or "shell", the program is being run from the shell and output can go to stderr. Otherwise, send it to the log. https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:111: kill(gettid(), SIGSTKFLT); Note: I know you didn't modify that part, but I'm not sure doing this is such a good idea. The implementation of the default signal handlers (setup by the linker) and debuggerd is bound to change between platform releases (i.e. certainly not stable). This code is likely to not work everywhere as expected, which could result in flaky tests. :-( https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:121: signal(SIGSTKFLT, sig_handler); Note: Oops, we should never use signal() to save/restore a signal handler. This code should use sigaction() instead to ensure none of the sa_flags are lost (SA_SIGINFO, more importantly). https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:140: *os << base::StringPrintf("pc %08x %s", rel_pc, path); On 2013/06/13 03:34:25, satorux1 wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... > > %08x -> %08"PRIx64" > nit: That would be very wrong, uintptr_t is always 32-bit on Android. Your proposal would make base::StringPrintf generate garbage unless rel_pc was explicitely cast to a int64_t. > who said printf was easier to read? :) > Compare to what exactly ? :-) https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:142: *os << "<unknown>"; I'd suggest printing the address value as well, just in case this makes diagnosing this kind of situation easier. Note that this will happen for renderer processes on JB 4.1 and higher, since they run in a additional sandboxed mode that prevent them from accessing /proc.
+willchan for base/ OWNERS + peanut gallery comments If you're curious what the output looks like, here's a sample mapped trace: #00 pc 000634c1 /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #01 pc 00206759 /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #02 pc 0020831d /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #03 pc 002083cd /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #04 pc 00208491 /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #05 pc 0020864b /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #06 pc 00206759 /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #07 pc 00206787 /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #08 pc 002895df /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #09 pc 0024958f /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #10 pc 0028f193 /data/app-lib/org.chromium.native_test-1/libbase_unittests.so #11 pc 0001dc4f /system/lib/libdvm.so #12 pc 0004ded1 /system/lib/libdvm.so #13 pc 00027063 /system/lib/libdvm.so #14 pc 0002b5ef /system/lib/libdvm.so #15 pc 000601e1 /system/lib/libdvm.so #16 pc 00067de1 /system/lib/libdvm.so #17 pc 00027063 /system/lib/libdvm.so #18 pc 0002b5ef /system/lib/libdvm.so #19 pc 0005ff23 /system/lib/libdvm.so #20 pc 00049b69 /system/lib/libdvm.so #21 pc 0004b5ff /system/lib/libandroid_runtime.so #22 pc 0004c291 /system/lib/libandroid_runtime.so #23 pc 0000105d /system/bin/app_process #24 pc 0000db4f /system/lib/libc.so Here's a sample unknown trace: #00 <unknown> 0x68c9e4c2 #01 <unknown> 0x68e4175a #02 <unknown> 0x68e4331e #03 <unknown> 0x68e433ce #04 <unknown> 0x68e43492 #05 <unknown> 0x68e4364c #06 <unknown> 0x68e4175a #07 <unknown> 0x68e41788 #08 <unknown> 0x68ec45e0 #09 <unknown> 0x68e84590 #10 <unknown> 0x68eca194 #11 <unknown> 0x40929c50 #12 <unknown> 0x40959ed2 #13 <unknown> 0x40933064 #14 <unknown> 0x409375f0 #15 <unknown> 0x4096c1e2 #16 <unknown> 0x40973de2 #17 <unknown> 0x40933064 #18 <unknown> 0x409375f0 #19 <unknown> 0x4096bf24 #20 <unknown> 0x40955b6a #21 <unknown> 0x4024c600 #22 <unknown> 0x4024d292 #23 <unknown> 0x400be05e #24 <unknown> 0x40165b50 Happy to tweak the formatting. The former presentation mirrors the Android debuggerd format ... but I'm not married to it :) https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:111: kill(gettid(), SIGSTKFLT); On 2013/06/13 08:17:25, digit wrote: > Note: I know you didn't modify that part, but I'm not sure doing this is such a > good idea. The implementation of the default signal handlers (setup by the > linker) and debuggerd is bound to change between platform releases (i.e. > certainly not stable). This code is likely to not work everywhere as expected, > which could result in flaky tests. :-( You know what? I'm going to rip out this code and put in much simpler logging of the stack trace. I've had coworkers complain they've been bitten by the current crashing behaviour a couple times (myself included!) https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:140: *os << base::StringPrintf("pc %08x %s", rel_pc, path); On 2013/06/13 08:17:25, digit wrote: > On 2013/06/13 03:34:25, satorux1 wrote: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... > > > > %08x -> %08"PRIx64" > > > nit: That would be very wrong, uintptr_t is always 32-bit on Android. Your > proposal would make base::StringPrintf generate garbage unless rel_pc was > explicitely cast to a int64_t. Open to ideas ... I tried using PRIxPTR but got silly compiler failures: ../../base/debug/stack_trace_android.cc:111:69: error: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'uintptr_t {aka unsigned int}' [-Werror=format] FWIW a similar warning would happen if we ever compiled with uintptr_t being 64-bit. https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:142: *os << "<unknown>"; On 2013/06/13 08:17:25, digit wrote: > I'd suggest printing the address value as well, just in case this makes > diagnosing this kind of situation easier. Note that this will happen for > renderer processes on JB 4.1 and higher, since they run in a additional > sandboxed mode that prevent them from accessing /proc. Done.
https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/15001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:140: *os << base::StringPrintf("pc %08x %s", rel_pc, path); On 2013/06/13 08:17:25, digit wrote: > On 2013/06/13 03:34:25, satorux1 wrote: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... > > > > %08x -> %08"PRIx64" > > > nit: That would be very wrong, uintptr_t is always 32-bit on Android. Your > proposal would make base::StringPrintf generate garbage unless rel_pc was > explicitely cast to a int64_t. Ah, I misunderstood that we were handling uint64 here. Using %08x sounds good if it's guaranteed to be always 32-bit. Adding a comment about it would be nice. > > > who said printf was easier to read? :) > > > Compare to what exactly ? :-) streams.
Sorry, I was in a conference today and will be busy there tomorrow too. I recommend talking to another reviewer. Otherwise I can look next week.
brettw: OWNERS for base/ and peanut gallery commentsas willchan is OOO
lgtm for final version. Thanks for fixing the issues. Not an owner though :)
pinging brettw@ for base/ OWNERS
base/third_party/symbolize/README.chromium is now out-of-date since it says the files were copied verbatim. Can we be upstreaming these changes? We don't want to have random forks of this kind of thing, especially when Google is the maintainer of the library in question.
On 2013/06/17 22:29:49, brettw wrote: > base/third_party/symbolize/README.chromium is now out-of-date since it says the > files were copied verbatim. > > Can we be upstreaming these changes? We don't want to have random forks of this > kind of thing, especially when Google is the maintainer of the library in > question. I'll take a stab at upstreaming. AFAIK satorux@ wrote this stuff originally and one of his coworkers (hamaji@) did most/all of the initial release, so they might be able to give some extra guidance here.
satorux, brettw: PTAL after a few offline convos, I decided against hacking up symbolize in favour of writing dedicated /proc/<pid>/maps parsing code: https://codereview.chromium.org/18178015/ the update also removes symbolize from the android build as its not being used
not LGTM (to avoid accidental commit—this change is obsolete in favor of https://codereview.chromium.org/18178015/, right?)
On 2013/07/02 18:48:48, Mark Mentovai wrote: > not LGTM > > (to avoid accidental commit—this change is obsolete in favor of > https://codereview.chromium.org/18178015/, right?) no, this CL uses the /proc/self/maps parsing code from https://codereview.chromium.org/18178015/ (see line 10 of base/debug/stack_trace_android.cc)
OK, will re-review once that lands, since you’ll need an OWNER anyway.
alrighty ... rebased on top of http://crrev.com/209855 mark: PTAL
https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:112: // Subtract by one as return address of function may be in the next Nit: “subtract one,” not “subtract by one.” https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:113: // function when a function is annotated as noreturn. This comment only tells half the story. If someone goes and disassembles things, it’s nice for the instruction pointers to line up with the disassembly. Will this code run only on ARM? If so, and thumb instructions are irrelevant, you can reliably subtract 4 to land on the actual instruction. But I think you can’t exclude the possibility of thumb, and even so, with Atom-based Androids, you can’t really infer anything about instruction length. https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:119: (iter->permissions & MappedMemoryRegion::READ) && Why check the protection bits? If the address is in the range, it’s a winner. Checking the bits just means that some bad actor can mess your stack trace up by calling mprotect. On the other hand, since you’re using this to figure out if you have a region that belongs to a file, maybe you want to check that the device (and inode?) numbers are zero and that there actually is a nonempty filename. https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:129: *os << base::StringPrintf("pc %08x %s", rel_pc, path); Maybe it’s just me, but this format seems kind of weird. I would expect something like “path + offset_into_path”. https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:131: *os << base::StringPrintf("<unknown> 0x%08x", address); kinda like the format you used here. But why’d you put a 0x on the address this time and not if the file was known?
PTAL https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:112: // Subtract by one as return address of function may be in the next On 2013/07/03 14:03:30, Mark Mentovai wrote: > Nit: “subtract one,” not “subtract by one.” Done. https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:113: // function when a function is annotated as noreturn. On 2013/07/03 14:03:30, Mark Mentovai wrote: > This comment only tells half the story. If someone goes and disassembles things, > it’s nice for the instruction pointers to line up with the disassembly. > > Will this code run only on ARM? If so, and thumb instructions are irrelevant, > you can reliably subtract 4 to land on the actual instruction. But I think you > can’t exclude the possibility of thumb, and even so, with Atom-based Androids, > you can’t really infer anything about instruction length. I fully admit that this is vestigial code from when I attempted to implement the android stack trace code using the posix code. This code + comment is lifted from stack_trace_posix.cc:136 I notice build/common.gypi will enable -mthumb when building arm_version==7, but I'm not aware of a #define I could check for. I'm also not sure we want to go down the path of assuming instruction lengths based on build configs :\ I'm not an expert in this area so let me know if you've got any ideas. Happy to experiment! https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:119: (iter->permissions & MappedMemoryRegion::READ) && On 2013/07/03 14:03:30, Mark Mentovai wrote: > Why check the protection bits? If the address is in the range, it’s a winner. > Checking the bits just means that some bad actor can mess your stack trace up by > calling mprotect. > > On the other hand, since you’re using this to figure out if you have a region > that belongs to a file, maybe you want to check that the device (and inode?) > numbers are zero and that there actually is a nonempty filename. do you mean non-zero inode/device? considering we're printing out the path I opted for checking the path https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:129: *os << base::StringPrintf("pc %08x %s", rel_pc, path); On 2013/07/03 14:03:30, Mark Mentovai wrote: > Maybe it’s just me, but this format seems kind of weird. I would expect > something like “path + offset_into_path”. Was attempting to emulate the Android stack dump style so these stacks could integrate with their tools, but that's no longer a requirement and I don't even like their stack trace style. Changed to the following style: #01 0x1234abcd /path/to/lib.so+0x0004012a #02 0x1234abc0 <unknown> Here's some sample output from running the StackTraceTest.DebugOutputToStream test: #00 0x692b0ed1 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x00065ed1 #01 0x6941bc01 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d0c01 #02 0x6941d7c5 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d27c5 #03 0x6941d875 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d2875 #04 0x6941d939 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d2939 #05 0x6941daf3 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d2af3 #06 0x6941bc01 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d0c01 #07 0x6941bc2f /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d0c2f #08 0x69421fe3 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001d6fe3 #09 0x69415321 /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001ca321 #10 0x69425f7f /data/app-lib/org.chromium.native_test-1/libbase_unittests.cr.so+0x001daf7f #11 0x40929c4f /system/lib/libdvm.so+0x0001dc4f #12 0x40959ed1 /system/lib/libdvm.so+0x0004ded1 #13 0x40933063 /system/lib/libdvm.so+0x00027063 #14 0x409375ef /system/lib/libdvm.so+0x0002b5ef #15 0x4096c1e1 /system/lib/libdvm.so+0x000601e1 #16 0x40973de1 /system/lib/libdvm.so+0x00067de1 #17 0x40933063 /system/lib/libdvm.so+0x00027063 #18 0x409375ef /system/lib/libdvm.so+0x0002b5ef #19 0x4096bf23 /system/lib/libdvm.so+0x0005ff23 #20 0x40955b69 /system/lib/libdvm.so+0x00049b69 #21 0x4024c5ff /system/lib/libandroid_runtime.so+0x0004b5ff #22 0x4024d291 /system/lib/libandroid_runtime.so+0x0004c291 #23 0x400be05d /system/bin/app_process+0x0000105d #24 0x40165b4f /system/lib/libc.so+0x0000db4f https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:131: *os << base::StringPrintf("<unknown> 0x%08x", address); On 2013/07/03 14:03:30, Mark Mentovai wrote: > kinda like the format you used here. > > But why’d you put a 0x on the address this time and not if the file was known? Done.
LGTM and have a nice long weekend! https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/51001/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:119: (iter->permissions & MappedMemoryRegion::READ) && scherkus wrote: > On 2013/07/03 14:03:30, Mark Mentovai wrote: > > Why check the protection bits? If the address is in the range, it’s a winner. > > Checking the bits just means that some bad actor can mess your stack trace up > by > > calling mprotect. > > > > On the other hand, since you’re using this to figure out if you have a region > > that belongs to a file, maybe you want to check that the device (and inode?) > > numbers are zero and that there actually is a nonempty filename. > > do you mean non-zero inode/device? > > considering we're printing out the path I opted for checking the path Oops, yeah, I meant nonzero. https://codereview.chromium.org/16770006/diff/53002/base/debug/stack_trace_an... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/16770006/diff/53002/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:112: uintptr_t address = reinterpret_cast<uintptr_t>(trace_[i]) - 1; OK, I guess the safe thing to do is leave this as 1. (x86 is the WORST, with its variable-length instructions.) https://codereview.chromium.org/16770006/diff/53002/base/debug/stack_trace_an... base/debug/stack_trace_android.cc:123: *os << base::StringPrintf("#%02d 0x%08x ", i, address); This format is awesome.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scherkus@chromium.org/16770006/63001
Message was sent while issue was closed.
Change committed as 210132 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
