Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18178015:
Implement /proc/self/maps/ parsing code. (Closed)
|
Created:
7 years, 5 months ago by scherkus (not reviewing) Modified:
7 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, digit1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement /proc/self/maps parsing code.
This will be used to implement stack traces on Android by printing relocatable addresses.
BUG=248784
R=mark@chromium.org, satorux@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209855
Patch Set 1 #
Total comments: 10
Patch Set 2 : added permissions and tests #Patch Set 3 : rebase #
Total comments: 3
Patch Set 4 : fix compile errors #Patch Set 5 : add link #
Total comments: 52
Patch Set 6 : fixes #
Total comments: 3
Patch Set 7 : eliminated copy and added test for 'S' #
Total comments: 6
Patch Set 8 : fixes #Patch Set 9 : rebase #
Total comments: 2
Messages
Total messages: 21 (0 generated)
satorux: can you review? As discussed offline and in https://codereview.chromium.org/16770006/ I've decided that a simple /proc/self/maps parser will be sufficient for my needs. I feel my symbolize changes aren't worth upstreaming as it's somewhat hacky :\
Part of me thinks this code should be written in async-signal-safe fashion but the other part of me thinks that would'be overkill and YAGNI. I think the latter part of me is bigger. :) https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.cc#newco... base/debug/proc_maps.cc:34: static const char kProcMapFormat[] = remove static? function-local 'static' is not guaranteed to be thread-safe with some compilers, IIRC. Maybe simpler to go without a constant: if (!sscanf("%" SCNxPTR "-%" SCNxPTR " %*s %zx %*x:%*x %*d %n", https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.cc#newco... base/debug/proc_maps.cc:35: "%" SCNxPTR "-%" SCNxPTR " %*s %zx %*x:%*x %*d %n"; didn't know about SCNxPTR. :) https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.h File base/debug/proc_maps.h (right): https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.h#newcode29 base/debug/proc_maps.h:29: FilePath path; maybe add the permission bits? that info would be useful. https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.h#newcode43 base/debug/proc_maps.h:43: BASE_EXPORT void ParseProcMaps(const std::string& input, Maybe return a boolean to report an error? https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps_unittest.cc File base/debug/proc_maps_unittest.cc (right): https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps_unittest... base/debug/proc_maps_unittest.cc:148: Add a test for broken input data?
PTAL https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.cc#newco... base/debug/proc_maps.cc:34: static const char kProcMapFormat[] = On 2013/07/01 03:59:56, satorux1 wrote: > remove static? function-local 'static' is not guaranteed to be thread-safe with > some compilers, IIRC. Maybe simpler to go without a constant: > > if (!sscanf("%" SCNxPTR "-%" SCNxPTR " %*s %zx %*x:%*x %*d %n", Done. https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.cc#newco... base/debug/proc_maps.cc:35: "%" SCNxPTR "-%" SCNxPTR " %*s %zx %*x:%*x %*d %n"; On 2013/07/01 03:59:56, satorux1 wrote: > didn't know about SCNxPTR. :) yeah I didn't either until I starting looking through inttypes.h however android's libc seems to disagree on what a uintptr_t is... will see if digit@ knows what to make of it https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.h File base/debug/proc_maps.h (right): https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.h#newcode29 base/debug/proc_maps.h:29: FilePath path; On 2013/07/01 03:59:56, satorux1 wrote: > maybe add the permission bits? that info would be useful. Done. I'll make my stack trace code check for r+x bits. https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps.h#newcode43 base/debug/proc_maps.h:43: BASE_EXPORT void ParseProcMaps(const std::string& input, On 2013/07/01 03:59:56, satorux1 wrote: > Maybe return a boolean to report an error? Done, but made this function an all-or-nothing function (i.e., all of input must be valid, whereas before it was more friendly to potentially garbage input -- personally I like stricter APIs) https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps_unittest.cc File base/debug/proc_maps_unittest.cc (right): https://codereview.chromium.org/18178015/diff/1/base/debug/proc_maps_unittest... base/debug/proc_maps_unittest.cc:148: On 2013/07/01 03:59:56, satorux1 wrote: > Add a test for broken input data? ah! good call also added tests for permissions
cc'ing digit@ for question about Bionic's stdint.h vs. inttypes.h https://codereview.chromium.org/18178015/diff/17001/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/17001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:14: #define SCNxPTR "x" digit: any ideas on Bionic stdint.h vs inttypes.h?
LGTM
mark / mmentovai: please review for base/ OWNERS
https://codereview.chromium.org/18178015/diff/17001/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/17001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:14: #define SCNxPTR "x" Looks like a real bug. I just filed https://code.google.com/p/android/issues/detail?id=57218 to track that in the NDK. The current lines above are a good work-around. Thanks.
jar: base/ OWNERS review (I think mark is OOO) https://codereview.chromium.org/18178015/diff/17001/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/17001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:14: #define SCNxPTR "x" On 2013/07/02 09:24:18, digit wrote: > Looks like a real bug. I just filed > https://code.google.com/p/android/issues/detail?id=57218 to track that in the > NDK. The current lines above are a good work-around. > > Thanks. Thanks for taking a look. Updated CL to include link to bug.
https://codereview.chromium.org/18178015/diff/41001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/18178015/diff/41001/base/base.gypi#newcode136 base/base.gypi:136: 'debug/proc_maps.cc', You need to name this “proc_maps_linux.cc” or something like that, or exclude it from the platforms that it’s nonsensical on. Since you want this for both linux and android, you’ll probably need an exclude (like “sources!”). The same goes for the unit test. The code itself might build just fine on other platforms, but it’s pointless and bloaty to build it elsewhere. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:22: namespace debug { No more nested namespaces in new code. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:26: std::string ReadProcMaps() { For the sake of performance, this would likely be better if it accepted a std::string* argument. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:44: // Ignore empty lines produced by splitting on '\n'. This should only be the last line, right? Since you wrote this parser pretty strictly, you might want to enforce that too. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:51: char permissions[4] = {'\0'}; Recommend [5] so that you always have a NUL-terminated string, in case someone ever wants to printf("%s", permissions) or something along those lines. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:52: size_t offset = 0; The kernel calls this an unsigned long long. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:55: int inode = 0; The kernel calls this a long. You can verify the other types used. fs/proc/task_mmu.c show_map_vma (or fs/proc/task_nommu.c nommu_vma_show). For example, start and end are longs to the kernel, but that should always be compatible with uintptr_t for the architectures we’re working with. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:72: region.start = start; What’s with the copies? If it’s because there are data width conversions, then it’s OK, but I don’t think there are. Why not declare the MemoryMappedRegion above the sscanf, and where possible, give sscanf pointers to the MemiryMappedRegion’s fields? https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:94: else if (permissions[3] != 's') // Shared memory. fs/proc/task_nommu.c nommu_vma_show permits 'S' too. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:97: region.path = FilePath(line + path_index); Since it might not be a path, maybe FilePath isn’t the right choice. Since you know what OS you’re on and what paths are, maybe you should just use a std::string. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h File base/debug/proc_maps.h (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:16: namespace debug { Aren’t we not doing second-level nested namespaces for new code now? https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:18: #if defined(OS_LINUX) || defined(OS_ANDROID) Get rid of this. Just rename the file with _linux or something that works for both Linux or Android, or if there’s nothing appropriate, you’ll have to write an exclude in the gyp file. You shouldn’t be #including this file on platforms where it doesn’t make sense. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:22: enum Permissions { The members of this enum are single permissions, so I think it’d be usual to name this enum “Permission” in the singular. Permission p = MappedMemoryRegion::READ makes more sense, right? https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:26: PRIVATE = 1 << 3, // If set, region is private otherwise it is shared. Missing comma? https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:44: // do so. What’s it return otherwise? Just the contents of /proc/self/maps? Say so. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... File base/debug/proc_maps_unittest.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:12: namespace debug { Ditto on the inner nested namespace. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:88: #if defined(ARCH_CPU_64_BITS) #elif makes it clear that this is the same test but for a different architecture. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:133: int permissions; You can call this a uint8 to match what it is in the struct. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:159: for (size_t i = 0; kTestCases[i].input != NULL; ++i) { Why not just use arraysize (like you did for the last two tests in this file) and get rid of the null entry? https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:164: ASSERT_TRUE(ParseProcMaps(kTestCases[i].input, ®ions)); On one hand, I think these should be EXPECT, because there’s no point in stopping the test loop just because one of these failed. On the other hand, I see how within a pass of the loop, each EXPECT/ASSERT builds on the one that came before it. Your call. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:172: ASSERT_TRUE(ParseProcMaps(ReadProcMaps(), ®ions)); I’d break this into two checked operations so that if it fails, you’ll know whether the read failed or the parse failed. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:178: if (regions[i].path.value().find("base_unittests") != std::string::npos) { There’s got to be a better way to figure out your executable’s basename than hardcoding it. You can readlink /proc/self/exe and take its basename, for example. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:205: ASSERT_FALSE(ParseProcMaps(kTestCases[i], ®ions)); This one should be EXPECT, and so should line 220. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:229: } // namespace base This is a really good and comprehensive test, nice job!
Mark looks like he jumped all over this when you asked... so this is just a nit to add to hist concerns. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:67: &inode, &path_index) < 7) { nit: I think you have 8 fields, rather than 7. Maybe you're guessing that if you get 7, you'll get the 8th (which consumes no data)... but it reads nicer with an 8 here.
thanks for the review! PTAL https://codereview.chromium.org/18178015/diff/41001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/18178015/diff/41001/base/base.gypi#newcode136 base/base.gypi:136: 'debug/proc_maps.cc', On 2013/07/02 19:36:57, Mark Mentovai wrote: > You need to name this “proc_maps_linux.cc” or something like that, or exclude it > from the platforms that it’s nonsensical on. Since you want this for both linux > and android, you’ll probably need an exclude (like “sources!”). > > The same goes for the unit test. > > The code itself might build just fine on other platforms, but it’s pointless and > bloaty to build it elsewhere. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc File base/debug/proc_maps.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:26: std::string ReadProcMaps() { On 2013/07/02 19:36:57, Mark Mentovai wrote: > For the sake of performance, this would likely be better if it accepted a > std::string* argument. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:44: // Ignore empty lines produced by splitting on '\n'. On 2013/07/02 19:36:57, Mark Mentovai wrote: > This should only be the last line, right? Since you wrote this parser pretty > strictly, you might want to enforce that too. Gah! I had that code written but ditched it, because it felt a bit silly to be so strict ... but you're right given how strict the rest of this parsing code is. Anyway, it's back w/ tests. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:51: char permissions[4] = {'\0'}; On 2013/07/02 19:36:57, Mark Mentovai wrote: > Recommend [5] so that you always have a NUL-terminated string, in case someone > ever wants to printf("%s", permissions) or something along those lines. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:52: size_t offset = 0; On 2013/07/02 19:36:57, Mark Mentovai wrote: > The kernel calls this an unsigned long long. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:55: int inode = 0; On 2013/07/02 19:36:57, Mark Mentovai wrote: > The kernel calls this a long. > > You can verify the other types used. fs/proc/task_mmu.c show_map_vma (or > fs/proc/task_nommu.c nommu_vma_show). For example, start and end are longs to > the kernel, but that should always be compatible with uintptr_t for the > architectures we’re working with. Done. Thanks for the tip! https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:67: &inode, &path_index) < 7) { On 2013/07/02 19:58:57, jar wrote: > nit: I think you have 8 fields, rather than 7. Maybe you're guessing that if > you get 7, you'll get the 8th (which consumes no data)... but it reads nicer > with an 8 here. > man 3 sscanf has an interesting note about %n: """ The C standard says: "Execution of a %n directive does not increment the assignment count returned at the completion of execution" but the Corrigendum seems to contradict this. Probably it is wise not to make any assumptions on the effect of %n conversions on the return value. """ I confirmed that on Linux and Android we return 7 when we successfully parse every field. Added a note to the comment. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:72: region.start = start; On 2013/07/02 19:36:57, Mark Mentovai wrote: > What’s with the copies? If it’s because there are data width conversions, then > it’s OK, but I don’t think there are. Why not declare the MemoryMappedRegion > above the sscanf, and where possible, give sscanf pointers to the > MemiryMappedRegion’s fields? Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:94: else if (permissions[3] != 's') // Shared memory. On 2013/07/02 19:36:57, Mark Mentovai wrote: > fs/proc/task_nommu.c nommu_vma_show permits 'S' too. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.cc#n... base/debug/proc_maps.cc:97: region.path = FilePath(line + path_index); On 2013/07/02 19:36:57, Mark Mentovai wrote: > Since it might not be a path, maybe FilePath isn’t the right choice. Since you > know what OS you’re on and what paths are, maybe you should just use a > std::string. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h File base/debug/proc_maps.h (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:16: namespace debug { On 2013/07/02 19:36:57, Mark Mentovai wrote: > Aren’t we not doing second-level nested namespaces for new code now? here's the most relevant discussion I could find: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/RVps606IB_g... they're not outright banned ... but rather discouraged for cases where they're not providing much value (e.g., net::url_request) personally I find this is an acceptable use of nested namespaces since it groups together a variety of debugging-related code that isn't typically used in production code for now I'd rather keep base/debug/ consistent + should we make the change tackle that in a future CL https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:18: #if defined(OS_LINUX) || defined(OS_ANDROID) On 2013/07/02 19:36:57, Mark Mentovai wrote: > Get rid of this. Just rename the file with _linux or something that works for > both Linux or Android, or if there’s nothing appropriate, you’ll have to write > an exclude in the gyp file. You shouldn’t be #including this file on platforms > where it doesn’t make sense. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:22: enum Permissions { On 2013/07/02 19:36:57, Mark Mentovai wrote: > The members of this enum are single permissions, so I think it’d be usual to > name this enum “Permission” in the singular. > > Permission p = MappedMemoryRegion::READ > > makes more sense, right? Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:26: PRIVATE = 1 << 3, // If set, region is private otherwise it is shared. On 2013/07/02 19:36:57, Mark Mentovai wrote: > Missing comma? Hrm. Not sure which comma is missing. I assume you mean between "private" and "otherwise" https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:44: // do so. On 2013/07/02 19:36:57, Mark Mentovai wrote: > What’s it return otherwise? Just the contents of /proc/self/maps? Say so. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... File base/debug/proc_maps_unittest.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:88: #if defined(ARCH_CPU_64_BITS) On 2013/07/02 19:36:57, Mark Mentovai wrote: > #elif makes it clear that this is the same test but for a different > architecture. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:133: int permissions; On 2013/07/02 19:36:57, Mark Mentovai wrote: > You can call this a uint8 to match what it is in the struct. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:159: for (size_t i = 0; kTestCases[i].input != NULL; ++i) { On 2013/07/02 19:36:57, Mark Mentovai wrote: > Why not just use arraysize (like you did for the last two tests in this file) > and get rid of the null entry? arraysize() doesn't work with anonymous struct types: error: template argument uses local type '<anonymous struct at ../../base/debug/proc_maps_linux_unittest.cc:140:10>' [-Werror,-Wlocal-type-template-args] options: 1) declare this struct in an anonymous namespace 2) wait for c++11 3) nothing https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:164: ASSERT_TRUE(ParseProcMaps(kTestCases[i].input, ®ions)); On 2013/07/02 19:36:57, Mark Mentovai wrote: > On one hand, I think these should be EXPECT, because there’s no point in > stopping the test loop just because one of these failed. On the other hand, I > see how within a pass of the loop, each EXPECT/ASSERT builds on the one that > came before it. Your call. agreed -- changed to EXPECT + added check to verify we won't crash when accessing regions[0] https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:172: ASSERT_TRUE(ParseProcMaps(ReadProcMaps(), ®ions)); On 2013/07/02 19:36:57, Mark Mentovai wrote: > I’d break this into two checked operations so that if it fails, you’ll know > whether the read failed or the parse failed. Done. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:178: if (regions[i].path.value().find("base_unittests") != std::string::npos) { On 2013/07/02 19:36:57, Mark Mentovai wrote: > There’s got to be a better way to figure out your executable’s basename than > hardcoding it. You can readlink /proc/self/exe and take its basename, for > example. hrmm... while this will work for linux it's trickier on android as base_unittests is linked as libbase_unittestslib.cr.so and invoked via JNI I reworked this to look for where our stack is mapped in memory. It's slightly more portable / less hardcoded, but I'm open to ideas :\ https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:205: ASSERT_FALSE(ParseProcMaps(kTestCases[i], ®ions)); On 2013/07/02 19:36:57, Mark Mentovai wrote: > This one should be EXPECT, and so should line 220. Done.
https://codereview.chromium.org/18178015/diff/53001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/18178015/diff/53001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:91: regions.push_back(region); hrmm.. I guess I could cut down on a string copy if I pushed then did the assign thoughts?
https://codereview.chromium.org/18178015/diff/53001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/18178015/diff/53001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:91: regions.push_back(region); On 2013/07/02 21:48:38, scherkus wrote: > hrmm.. I guess I could cut down on a string copy if I pushed then did the assign > > thoughts? went ahead and did it -- can't hurt!
LGTM https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h File base/debug/proc_maps.h (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps.h#ne... base/debug/proc_maps.h:26: PRIVATE = 1 << 3, // If set, region is private otherwise it is shared. scherkus wrote: > Hrm. Not sure which comma is missing. I assume you mean between "private" and > "otherwise" Yeah, that’s the one I meant. https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... File base/debug/proc_maps_unittest.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:159: for (size_t i = 0; kTestCases[i].input != NULL; ++i) { scherkus wrote: > arraysize() doesn't work with anonymous struct types: > > error: template argument uses local type '<anonymous struct at > ../../base/debug/proc_maps_linux_unittest.cc:140:10>' > [-Werror,-Wlocal-type-template-args] > > options: > 1) declare this struct in an anonymous namespace > 2) wait for c++11 > 3) nothing 4) ARRAYSIZE_UNSAFE Your call https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:178: if (regions[i].path.value().find("base_unittests") != std::string::npos) { scherkus wrote: > hrmm... while this will work for linux it's trickier on android as > base_unittests is linked as libbase_unittestslib.cr.so and invoked via JNI > > I reworked this to look for where our stack is mapped in memory. It's slightly > more portable / less hardcoded, but I'm open to ideas :\ Well, it would have been OK to use /proc/self/exe (if it exists on Android) even if it didn’t have base_unittests in its name. The main executable still should show up in /proc/self/maps. Maybe you even want to look for both /proc/self/exe and the stack. https://codereview.chromium.org/18178015/diff/53001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/18178015/diff/53001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:91: regions.push_back(region); scherkus wrote: > hrmm.. I guess I could cut down on a string copy if I pushed then did the assign > > thoughts? I’m cool with that. Give it a comment so the rationale is obvious. https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:15: #define SCNxPTR "x" You might want to #undef SCNxPTR before this, in case something else #includes the bum <inttypes.h> before reaching here. If you do that, you should move this OS_ANDROID block below all of the other #includes. https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:91: regions.back().path.assign(line + path_index); Comment “this weird thing saves a string copy?” https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux_unittest.cc (right): https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... base/debug/proc_maps_linux_unittest.cc:200: return; Instead of returning here, stay in the loop to make sure that the region doesn’t bogusly show up more times than it should.
https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... File base/debug/proc_maps_unittest.cc (right): https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:159: for (size_t i = 0; kTestCases[i].input != NULL; ++i) { On 2013/07/02 22:21:35, Mark Mentovai wrote: > scherkus wrote: > > arraysize() doesn't work with anonymous struct types: > > > > error: template argument uses local type '<anonymous struct at > > ../../base/debug/proc_maps_linux_unittest.cc:140:10>' > > [-Werror,-Wlocal-type-template-args] > > > > options: > > 1) declare this struct in an anonymous namespace > > 2) wait for c++11 > > 3) nothing > > 4) ARRAYSIZE_UNSAFE > > Your call The More You Know(tm) Done. (I'm an arraysize fan) https://codereview.chromium.org/18178015/diff/41001/base/debug/proc_maps_unit... base/debug/proc_maps_unittest.cc:178: if (regions[i].path.value().find("base_unittests") != std::string::npos) { On 2013/07/02 22:21:35, Mark Mentovai wrote: > scherkus wrote: > > hrmm... while this will work for linux it's trickier on android as > > base_unittests is linked as libbase_unittestslib.cr.so and invoked via JNI > > > > I reworked this to look for where our stack is mapped in memory. It's slightly > > more portable / less hardcoded, but I'm open to ideas :\ > > Well, it would have been OK to use /proc/self/exe (if it exists on Android) even > if it didn’t have base_unittests in its name. The main executable still should > show up in /proc/self/maps. > > Maybe you even want to look for both /proc/self/exe and the stack. Looks like PathService w/ FILE_EXE does a readlink() on /proc/self/exe Added test + confirmed working on Linux and Android. Score. https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:15: #define SCNxPTR "x" On 2013/07/02 22:21:36, Mark Mentovai wrote: > You might want to #undef SCNxPTR before this, in case something else #includes > the bum <inttypes.h> before reaching here. > > If you do that, you should move this OS_ANDROID block below all of the other > #includes. Done. https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... base/debug/proc_maps_linux.cc:91: regions.back().path.assign(line + path_index); On 2013/07/02 22:21:36, Mark Mentovai wrote: > Comment “this weird thing saves a string copy?” Done. https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... File base/debug/proc_maps_linux_unittest.cc (right): https://codereview.chromium.org/18178015/diff/62001/base/debug/proc_maps_linu... base/debug/proc_maps_linux_unittest.cc:200: return; On 2013/07/02 22:21:36, Mark Mentovai wrote: > Instead of returning here, stay in the loop to make sure that the region doesn’t > bogusly show up more times than it should. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scherkus@chromium.org/18178015/48002
Retried try job too often on win7_aura for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Message was sent while issue was closed.
Committed patchset #9 manually as r209855 (presubmit successful).
Message was sent while issue was closed.
LGTM-post-facto https://codereview.chromium.org/18178015/diff/48002/base/debug/proc_maps_linu... File base/debug/proc_maps_linux_unittest.cc (right): https://codereview.chromium.org/18178015/diff/48002/base/debug/proc_maps_linu... base/debug/proc_maps_linux_unittest.cc:200: found_exe = true; EXPECT_FALSE(found_exe) before this line.
Message was sent while issue was closed.
Oh, never mind, you’ll have multiple sections mapped.
Message was sent while issue was closed.
https://codereview.chromium.org/18178015/diff/48002/base/debug/proc_maps_linu... File base/debug/proc_maps_linux_unittest.cc (right): https://codereview.chromium.org/18178015/diff/48002/base/debug/proc_maps_linu... base/debug/proc_maps_linux_unittest.cc:200: found_exe = true; On 2013/07/03 13:26:36, Mark Mentovai wrote: > EXPECT_FALSE(found_exe) before this line. Addressed in https://codereview.chromium.org/18328027/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
