Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1346453004:
NativeStackSampler implementation for Mac. (Closed)
DescriptionNativeStackSampler implementation for Mac.
Stack sampling on Mac implemented for StackSamplingProfiler to provide
Chrome stack trace data from Mac users to UMA.
BUG=531673
Patch Set 1 #Patch Set 2 : Adjusted comments. #
Total comments: 46
Patch Set 3 : Addressed Rob's comments. #
Total comments: 8
Patch Set 4 : #
Total comments: 23
Patch Set 5 : Small changes and kUnknownModuleIndex on dladdr failure. #Patch Set 6 : wittman's comments. #
Total comments: 29
Patch Set 7 : adress comments. #
Total comments: 34
Patch Set 8 : #
Total comments: 2
Patch Set 9 : rsesek's comments #
Total comments: 7
Patch Set 10 : Binding libraries and nits. #
Messages
Total messages: 32 (9 generated)
sydli@google.com changed reviewers: + erikchen@chromium.org, robliao@chromium.org
+erikchen and +robliao for review.
A lot of fun stuff here! https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:23: Nit: Remove extra line. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:63: bool SafeJump(const StackFrame* src, StackFrame* dst) { Can src be a const StackFrame& ? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:64: vm_size_t bytesCopied = 0; ignored_bytes_copied https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:70: // Functions related to retrieving Mach-O Identifer ---------------------------- Remove trailing -'s https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:81: memcpy(&hdr, module_addr, sizeof(hdr)); Safety: Where do we get the assurance that module_addr is at least sizeof(fat_header)? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:92: memcpy(&arch, (const char*)module_addr + offset, sizeof(arch)); Would casting module_addr+offset to a fat_arch[] work? At minimum, use reintrepret_cast<const char*>() from here and forwards. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:114: if (hdr.cputype != CPU_TYPE_X86_64) return hdr.cputype == CPU_TYPE_X86_64; https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:119: // Fills |id| with UUID of x86_64 Mach-O binary loaded at |module_addr|. Nit: {the} UUID of {the} x86_64 https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:120: // |offset| is the offset in bytes into |module_addr| where the x86_64 header It might be instructive to do some ASCII Art towards the top, something like + <-- module_addr | | <-- + Offset, where the headers start. . . . https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:128: return false; When a conditional spans multiple lines, use braces. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:137: load_command cmd; Move cmd outside the loop to avoid constructing this hdr.ncmds times. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:138: memcpy(&cmd, (const char*)module_addr + offset, sizeof(cmd)); Could represent module_addr + offset as a temp value so that you don't have to copy it again below. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:147: memcpy(id, &uuid_cmd, sizeof(uuid_cmd)); Safety: This copy probably should be no longer than sizeof(id). Might be useful to have a static assert that sizeof(uuid_command) == sizeof(id) https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:187: // Returns number of frames in stack, unless there's a corrupt ebp, in which corrupt ebp or missing ebp or both? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:192: int max_stack_size, Maybe instruction_pointers_size to make it obvious that this describes the size of the instruction_pointers array? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:193: const void* instruction_pointers[]) { Should this be const uintptr_t? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:209: return i; When a conditional spans multiple lines, add braces around the single statement. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:224: auto loc = module_to_index->find(inf.dli_fbase); What is loc? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:231: loc = module_to_index->insert( Linebreak after the -> and reflow. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:289: const int max_stack_size = 64; MAX_STACK_SIZE. Consider moving this to the top of the namespace.
Patchset #3 (id:40001) has been deleted
sydli@google.com changed reviewers: + wittman@chromium.org
Rob's comments. +wittman for review. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:23: On 2015/09/14 21:58:17, robliao wrote: > Nit: Remove extra line. Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:63: bool SafeJump(const StackFrame* src, StackFrame* dst) { On 2015/09/14 21:58:17, robliao wrote: > Can src be a const StackFrame& ? Don't think so; I pass in frame.prev (which is a StackFrame*) as src, which I don't want to dereference until vm_read_overwrite. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:64: vm_size_t bytesCopied = 0; On 2015/09/14 21:58:16, robliao wrote: > ignored_bytes_copied Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:70: // Functions related to retrieving Mach-O Identifer ---------------------------- On 2015/09/14 21:58:17, robliao wrote: > Remove trailing -'s Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:81: memcpy(&hdr, module_addr, sizeof(hdr)); On 2015/09/14 21:58:17, robliao wrote: > Safety: Where do we get the assurance that module_addr is at least > sizeof(fat_header)? This is an assumption we make since |module_addr| is supposedly where a module is loaded into memory. Unsure what would be the best way to check this besides checking the results of dladdr (where |module_addr| comes from); any recommendations? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:92: memcpy(&arch, (const char*)module_addr + offset, sizeof(arch)); On 2015/09/14 21:58:17, robliao wrote: > Would casting module_addr+offset to a fat_arch[] work? > At minimum, use reintrepret_cast<const char*>() from here and forwards. module_addr has to be a pointer of a byte-wide type so we can offset exactly |offset| bytes. Afterwards we can cast to fat_arch[] if needed. What is the standard pointer type to use for arithmetic? https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:114: if (hdr.cputype != CPU_TYPE_X86_64) On 2015/09/14 21:58:16, robliao wrote: > return hdr.cputype == CPU_TYPE_X86_64; Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:119: // Fills |id| with UUID of x86_64 Mach-O binary loaded at |module_addr|. On 2015/09/14 21:58:17, robliao wrote: > Nit: {the} UUID of {the} x86_64 Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:120: // |offset| is the offset in bytes into |module_addr| where the x86_64 header On 2015/09/14 21:58:17, robliao wrote: > It might be instructive to do some ASCII Art towards the top, something like > > + <-- module_addr > | > | <-- + Offset, where the headers start. > . > . > . This is only accurate for FAT Mach-O headers. Updated to comment that offset is only relevant if binary is FAT. In this case, should I diagram both layouts? I linked the relevant spec as well. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:128: return false; On 2015/09/14 21:58:17, robliao wrote: > When a conditional spans multiple lines, use braces. Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:137: load_command cmd; On 2015/09/14 21:58:17, robliao wrote: > Move cmd outside the loop to avoid constructing this hdr.ncmds times. Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:138: memcpy(&cmd, (const char*)module_addr + offset, sizeof(cmd)); On 2015/09/14 21:58:16, robliao wrote: > Could represent module_addr + offset as a temp value so that you don't have to > copy it again below. Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:147: memcpy(id, &uuid_cmd, sizeof(uuid_cmd)); On 2015/09/14 21:58:17, robliao wrote: > Safety: This copy probably should be no longer than sizeof(id). > Might be useful to have a static assert that sizeof(uuid_command) == sizeof(id) Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:187: // Returns number of frames in stack, unless there's a corrupt ebp, in which On 2015/09/14 21:58:17, robliao wrote: > corrupt ebp or missing ebp or both? A corrupt base pointer implies that it is missing (unless something is horribly wrong). Updated comment. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:192: int max_stack_size, On 2015/09/14 21:58:17, robliao wrote: > Maybe instruction_pointers_size to make it obvious that this describes the size > of the instruction_pointers array? Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:193: const void* instruction_pointers[]) { On 2015/09/14 21:58:17, robliao wrote: > Should this be const uintptr_t? Done. Changed where appropriate. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:209: return i; On 2015/09/14 21:58:17, robliao wrote: > When a conditional spans multiple lines, add braces around the single statement. Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:224: auto loc = module_to_index->find(inf.dli_fbase); On 2015/09/14 21:58:16, robliao wrote: > What is loc? Pair result from module -> index map. Updated name to module_index. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:231: loc = module_to_index->insert( On 2015/09/14 21:58:17, robliao wrote: > Linebreak after the -> and reflow. Done. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:289: const int max_stack_size = 64; On 2015/09/14 21:58:17, robliao wrote: > MAX_STACK_SIZE. Consider moving this to the top of the namespace. Done. Made this a static const in NativeStackSamplerMac.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:81: memcpy(&hdr, module_addr, sizeof(hdr)); On 2015/09/14 23:00:13, sydli wrote: > On 2015/09/14 21:58:17, robliao wrote: > > Safety: Where do we get the assurance that module_addr is at least > > sizeof(fat_header)? > > This is an assumption we make since |module_addr| is supposedly where a module > is loaded into memory. Unsure what would be the best way to check this besides > checking the results of dladdr (where |module_addr| comes from); any > recommendations? The sections don't appear to have size info, so I guess we'll have to stick with this. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:92: memcpy(&arch, (const char*)module_addr + offset, sizeof(arch)); On 2015/09/14 23:00:13, sydli wrote: > On 2015/09/14 21:58:17, robliao wrote: > > Would casting module_addr+offset to a fat_arch[] work? > > At minimum, use reintrepret_cast<const char*>() from here and forwards. > > module_addr has to be a pointer of a byte-wide type so we can offset exactly > |offset| bytes. Afterwards we can cast to fat_arch[] if needed. What is the > standard pointer type to use for arithmetic? const char* is likely what you want since you're operating off of the assumption that you're working with bytes here. You could also use uint8_t* https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:120: // |offset| is the offset in bytes into |module_addr| where the x86_64 header On 2015/09/14 23:00:13, sydli wrote: > On 2015/09/14 21:58:17, robliao wrote: > > It might be instructive to do some ASCII Art towards the top, something like > > > > + <-- module_addr > > | > > | <-- + Offset, where the headers start. > > . > > . > > . > > This is only accurate for FAT Mach-O headers. Updated to comment that offset is > only relevant if binary is FAT. In this case, should I diagram both layouts? I > linked the relevant spec as well. The link is good, but the diagram is more helpful in case the link goes bad. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:128: return false; On 2015/09/14 23:00:13, sydli wrote: > On 2015/09/14 21:58:17, robliao wrote: > > When a conditional spans multiple lines, use braces. > > Done. I assume this is the one you forgot? https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:150: // STATIC assert that length (uuid_cmd.uuid == id) Remove comment since we can see the assertion below. https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:152: memcpy(id, &uuid_cmd.uuid, sizeof(uuid_cmd)); Ah, so this was a bug before? ;-) The comment still stands. Given that the destination is id, you'll want to use sizeof(id). https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:270: static const int kMaxStackSize = 64; Constants go before the constructor. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:298: uintptr_t instruction_pointers[kMaxStackSize] = {0}; Oh yeah, and {} should work here.
On 2015/09/14 23:28:06, robliao wrote: > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > File base/profiler/native_stack_sampler_mac.cc (right): > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:81: memcpy(&hdr, module_addr, > sizeof(hdr)); > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > Safety: Where do we get the assurance that module_addr is at least > > > sizeof(fat_header)? > > > > This is an assumption we make since |module_addr| is supposedly where a module > > is loaded into memory. Unsure what would be the best way to check this besides > > checking the results of dladdr (where |module_addr| comes from); any > > recommendations? > > The sections don't appear to have size info, so I guess we'll have to stick with > this. > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:92: memcpy(&arch, (const > char*)module_addr + offset, sizeof(arch)); > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > Would casting module_addr+offset to a fat_arch[] work? > > > At minimum, use reintrepret_cast<const char*>() from here and forwards. > > > > module_addr has to be a pointer of a byte-wide type so we can offset exactly > > |offset| bytes. Afterwards we can cast to fat_arch[] if needed. What is the > > standard pointer type to use for arithmetic? > > const char* is likely what you want since you're operating off of the assumption > that you're working with bytes here. > > You could also use uint8_t* > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:120: // |offset| is the offset in > bytes into |module_addr| where the x86_64 header > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > It might be instructive to do some ASCII Art towards the top, something like > > > > > > + <-- module_addr > > > | > > > | <-- + Offset, where the headers start. > > > . > > > . > > > . > > > > This is only accurate for FAT Mach-O headers. Updated to comment that offset > is > > only relevant if binary is FAT. In this case, should I diagram both layouts? I > > linked the relevant spec as well. > > The link is good, but the diagram is more helpful in case the link goes bad. > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:128: return false; > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > When a conditional spans multiple lines, use braces. > > > > Done. > > I assume this is the one you forgot? > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > File base/profiler/native_stack_sampler_mac.cc (right): > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:150: // STATIC assert that length > (uuid_cmd.uuid == id) > Remove comment since we can see the assertion below. > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:152: memcpy(id, &uuid_cmd.uuid, > sizeof(uuid_cmd)); > Ah, so this was a bug before? ;-) > > The comment still stands. Given that the destination is id, you'll want to use > sizeof(id). > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:270: static const int kMaxStackSize = > 64; > Constants go before the constructor. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:298: uintptr_t > instruction_pointers[kMaxStackSize] = {0}; > Oh yeah, and {} should work here.
On 2015/09/14 23:28:06, robliao wrote: > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > File base/profiler/native_stack_sampler_mac.cc (right): > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:81: memcpy(&hdr, module_addr, > sizeof(hdr)); > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > Safety: Where do we get the assurance that module_addr is at least > > > sizeof(fat_header)? > > > > This is an assumption we make since |module_addr| is supposedly where a module > > is loaded into memory. Unsure what would be the best way to check this besides > > checking the results of dladdr (where |module_addr| comes from); any > > recommendations? > > The sections don't appear to have size info, so I guess we'll have to stick with > this. > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:92: memcpy(&arch, (const > char*)module_addr + offset, sizeof(arch)); > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > Would casting module_addr+offset to a fat_arch[] work? > > > At minimum, use reintrepret_cast<const char*>() from here and forwards. > > > > module_addr has to be a pointer of a byte-wide type so we can offset exactly > > |offset| bytes. Afterwards we can cast to fat_arch[] if needed. What is the > > standard pointer type to use for arithmetic? > > const char* is likely what you want since you're operating off of the assumption > that you're working with bytes here. > > You could also use uint8_t* > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:120: // |offset| is the offset in > bytes into |module_addr| where the x86_64 header > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > It might be instructive to do some ASCII Art towards the top, something like > > > > > > + <-- module_addr > > > | > > > | <-- + Offset, where the headers start. > > > . > > > . > > > . > > > > This is only accurate for FAT Mach-O headers. Updated to comment that offset > is > > only relevant if binary is FAT. In this case, should I diagram both layouts? I > > linked the relevant spec as well. > > The link is good, but the diagram is more helpful in case the link goes bad. > > https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:128: return false; > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > When a conditional spans multiple lines, use braces. > > > > Done. > > I assume this is the one you forgot? > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > File base/profiler/native_stack_sampler_mac.cc (right): > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:150: // STATIC assert that length > (uuid_cmd.uuid == id) > Remove comment since we can see the assertion below. > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:152: memcpy(id, &uuid_cmd.uuid, > sizeof(uuid_cmd)); > Ah, so this was a bug before? ;-) > > The comment still stands. Given that the destination is id, you'll want to use > sizeof(id). > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:270: static const int kMaxStackSize = > 64; > Constants go before the constructor. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... > > https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... > base/profiler/native_stack_sampler_mac.cc:298: uintptr_t > instruction_pointers[kMaxStackSize] = {0}; > Oh yeah, and {} should work here.
sydli@google.com changed reviewers: - mimmyk7@gmail.com
Comments and removed spammer as reviewer. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:120: // |offset| is the offset in bytes into |module_addr| where the x86_64 header On 2015/09/14 23:28:05, robliao wrote: > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > It might be instructive to do some ASCII Art towards the top, something like > > > > > > + <-- module_addr > > > | > > > | <-- + Offset, where the headers start. > > > . > > > . > > > . > > > > This is only accurate for FAT Mach-O headers. Updated to comment that offset > is > > only relevant if binary is FAT. In this case, should I diagram both layouts? I > > linked the relevant spec as well. > > The link is good, but the diagram is more helpful in case the link goes bad. Ok. https://codereview.chromium.org/1346453004/diff/20001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:128: return false; On 2015/09/14 23:28:05, robliao wrote: > On 2015/09/14 23:00:13, sydli wrote: > > On 2015/09/14 21:58:17, robliao wrote: > > > When a conditional spans multiple lines, use braces. > > > > Done. > > I assume this is the one you forgot? Yep! Added it in new patchset. https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:150: // STATIC assert that length (uuid_cmd.uuid == id) On 2015/09/14 23:28:06, robliao wrote: > Remove comment since we can see the assertion below. My bad- done. https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:152: memcpy(id, &uuid_cmd.uuid, sizeof(uuid_cmd)); On 2015/09/14 23:28:06, robliao wrote: > Ah, so this was a bug before? ;-) > > The comment still stands. Given that the destination is id, you'll want to use > sizeof(id). Since parameter arrays are passed as pointers, the array length information is lost and sizeof doesn't work. Made UUID length in bytes a constant in anonymous namespace. https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:270: static const int kMaxStackSize = 64; On 2015/09/14 23:28:05, robliao wrote: > Constants go before the constructor. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... Done. https://codereview.chromium.org/1346453004/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:298: uintptr_t instruction_pointers[kMaxStackSize] = {0}; On 2015/09/14 23:28:05, robliao wrote: > Oh yeah, and {} should work here. Done.
https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:13: #include "base/logging.h" unused? https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:14: #include "base/md5.h" unused? https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:17: #include "base/time/time.h" unused? https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:63: return vm_read_overwrite(mach_task_self(), (vm_address_t)src, use C++-style casts https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:113: return (hdr.cputype == CPU_TYPE_X86_64); nit: no parens https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:166: if (!IsX64Header(module_addr) && shouldn't this be || ? https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:184: bool GetThreadState(thread_act_t target_thread, ThreadContext* state) { can we call this GetThreadContext? https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:199: int instruction_pointers_size, can we call this max_stack_size, as in the Windows code? https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:252: std::map<void*, size_t> module_index; This needs to be a member variable, as in the Windows code. Otherwise the same module will be added to |modules| each time it appears in a sample. Also, use const void* for the key to be consistent with the rest of the code that deals with memory addresses. In general, can you mimic the names and logic of corresponding functions in the Windows code, where it makes sense to do so? Limiting variation to only where necessary makes it easier to understand both pieces of code in conjunction. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:271: static const int kMaxStackSize = 64; this can be a local variable in RecordStackSample() (constants should be declared as local as possible to the code that uses them) https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:287: : thread_handle_(static_cast<mach_port_t>(thread_handle)) {} current_modules_(nullptr) https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:316: #if defined(OS_MACOSX) && !defined(OS_NACL) you can remove the "defined(OS_MACOSX)", and exclusion from the NACL compile is better dealt with in the build files https://codereview.chromium.org/1346453004/diff/90005/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1346453004/diff/90005/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:20: #if defined(_WIN64) || defined(OS_MACOSX) Update comment.
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:13: #include "base/logging.h" On 2015/09/15 00:34:54, Mike Wittman wrote: > unused? Removed all of these. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:63: return vm_read_overwrite(mach_task_self(), (vm_address_t)src, On 2015/09/15 00:34:54, Mike Wittman wrote: > use C++-style casts Done. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:113: return (hdr.cputype == CPU_TYPE_X86_64); On 2015/09/15 00:34:54, Mike Wittman wrote: > nit: no parens Done. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:166: if (!IsX64Header(module_addr) && On 2015/09/15 00:34:54, Mike Wittman wrote: > shouldn't this be || ? No, if it's not an x86_64-exclusive module it could be a module that supports multiple archs, so it searches for the appropriate offset into the module. If it can't find the offset either, then returns false. Added comment to indicate this. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:184: bool GetThreadState(thread_act_t target_thread, ThreadContext* state) { On 2015/09/15 00:34:54, Mike Wittman wrote: > can we call this GetThreadContext? Done. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:199: int instruction_pointers_size, On 2015/09/15 00:34:54, Mike Wittman wrote: > can we call this max_stack_size, as in the Windows code? Changed back to max_stack_size for consistency with Windows profiler. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:252: std::map<void*, size_t> module_index; On 2015/09/15 00:34:54, Mike Wittman wrote: > This needs to be a member variable, as in the Windows code. Otherwise the same > module will be added to |modules| each time it appears in a sample. > > Also, use const void* for the key to be consistent with the rest of the code > that deals with memory addresses. > > In general, can you mimic the names and logic of corresponding functions in the > Windows code, where it makes sense to do so? Limiting variation to only where > necessary makes it easier to understand both pieces of code in conjunction. Sure. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:271: static const int kMaxStackSize = 64; On 2015/09/15 00:34:54, Mike Wittman wrote: > this can be a local variable in RecordStackSample() > > (constants should be declared as local as possible to the code that uses them) Done. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:287: : thread_handle_(static_cast<mach_port_t>(thread_handle)) {} On 2015/09/15 00:34:54, Mike Wittman wrote: > current_modules_(nullptr) Done. https://codereview.chromium.org/1346453004/diff/90005/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:316: #if defined(OS_MACOSX) && !defined(OS_NACL) On 2015/09/15 00:34:54, Mike Wittman wrote: > you can remove the "defined(OS_MACOSX)", and exclusion from the NACL compile is > better dealt with in the build files Done.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Why didn't we continue this review on https://codereview.chromium.org/1342453003/ ? https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:60: bool SafeJump(uintptr_t src, StackFrame* dst) { How is this a "jump"? This seems more like ReadStackFrame to me. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:80: // x86_64 Mach-O FAT64 Mach-O I see that another reviewer asked for it, but I'm not sure this ASCII art is useful to an accustomed reader. I think it'd just be better to refer readers to the ABI File Format reference as you've done above. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:95: memcpy(&hdr, module_addr, sizeof(hdr)); Do you actually need to copy data in these routines when you're just performing a search? For example, this could probably be: auto header = reinterpret_cast<const fat_header*>(module_address); if (header->magic != FAT_CIGAM || header->magic != FAT_MAGIC) return 0; auto fat_arches = reinterpret_cast<const fat_arch*>(module_address + sizeof(*header)); for (uint32_t i = 0; i < OSSwapBigToHostInt32(header->nfat_arch); ++i) { const fat_arch& arch = fat_arches[i]; if (OSSwapBigToHostInt32(arch.cputype) == CPU_TYPE_X86_64) return OSSwapBigToHostInt32(arch.offset); } https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:227: // 1) We tried to jump off the stack. Avoid using "we" and other personal pronouns in comments. What does "jump off the stack" mean? https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:228: // 2) We jumped to a lower address. "The next recovered rsp is not lower than the previously recovered one, indicating the stack is not growing down." https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:230: uintptr_t old_esp = frame.prev; Since this is x86_64, this isn't esp, it's rsp. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:267: // Mach handle for thread being profiled. Weak or strong right reference, here? https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:330: realpath(inf.dli_fname, filepath); realpath will still touch disk. Is this necessary? https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:356: PlatformThreadId thread_id) { #if defined(__i386__) NOTIMPLEMENTED() << "This class only has support for x86_64 sampling. return nullptr; #else … #endif
https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:20: static const int kUUIDLengthBytes = 16; static is unnecessary when declaring in an unnamed namespace https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:258: const uintptr_t instruction_pointer); input arguments should appear before output arguments https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:265: StackSamplingProfiler::Sample* sample); |sample| can go before |modules|, to be consistent with the Windows code and the previous function https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:316: void NativeStackSamplerMac::AddModule( This function feels like it's doing too many things. Is there a reason we can't use the same functional split as in the Windows code: CopyToSample, GetModuleIndex, GetModuleForHandle (maybe GetModuleForInstructionPointer here)? https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:348: for (int i = 0; i < stack_depth; i++) { nit: no braces https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:356: PlatformThreadId thread_id) { On 2015/09/15 13:28:19, Robert Sesek wrote: > #if defined(__i386__) > NOTIMPLEMENTED() << "This class only has support for x86_64 sampling. > return nullptr; > #else > … > #endif Just returning the null pointer is sufficient. The code that uses the NativeStackSampler interprets that as being unsupported.
Patchset #7 (id:160001) has been deleted
I've been using https://codereview.chromium.org/1342453003/ as a test CL; sorry for the confusion! I addressed your comments here though. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:20: static const int kUUIDLengthBytes = 16; On 2015/09/15 16:00:15, Mike Wittman wrote: > static is unnecessary when declaring in an unnamed namespace Done. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:60: bool SafeJump(uintptr_t src, StackFrame* dst) { On 2015/09/15 13:28:19, Robert Sesek wrote: > How is this a "jump"? This seems more like ReadStackFrame to me. Changed function name + comments. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:80: // x86_64 Mach-O FAT64 Mach-O On 2015/09/15 13:28:19, Robert Sesek wrote: > I see that another reviewer asked for it, but I'm not sure this ASCII art is > useful to an accustomed reader. I think it'd just be better to refer readers to > the ABI File Format reference as you've done above. Done. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:95: memcpy(&hdr, module_addr, sizeof(hdr)); On 2015/09/15 13:28:19, Robert Sesek wrote: > Do you actually need to copy data in these routines when you're just performing > a search? For example, this could probably be: > > auto header = reinterpret_cast<const fat_header*>(module_address); > if (header->magic != FAT_CIGAM || header->magic != FAT_MAGIC) > return 0; > > auto fat_arches = reinterpret_cast<const fat_arch*>(module_address + > sizeof(*header)); > for (uint32_t i = 0; i < OSSwapBigToHostInt32(header->nfat_arch); ++i) { > const fat_arch& arch = fat_arches[i]; > if (OSSwapBigToHostInt32(arch.cputype) == CPU_TYPE_X86_64) > return OSSwapBigToHostInt32(arch.offset); > } No-- not with the OSSwap procedure. Changed code to read addresses and use OSSwap when endianness does not match. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:227: // 1) We tried to jump off the stack. On 2015/09/15 13:28:19, Robert Sesek wrote: > Avoid using "we" and other personal pronouns in comments. What does "jump off > the stack" mean? SafeReadFrame should only allow us to read from a valid frame ptr. Changed wording to indicate this. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:228: // 2) We jumped to a lower address. On 2015/09/15 13:28:19, Robert Sesek wrote: > "The next recovered rsp is not lower than the previously recovered one, > indicating the stack is not growing down." Done. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:230: uintptr_t old_esp = frame.prev; On 2015/09/15 13:28:19, Robert Sesek wrote: > Since this is x86_64, this isn't esp, it's rsp. Oops-- my bad. Updated name. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:258: const uintptr_t instruction_pointer); On 2015/09/15 16:00:15, Mike Wittman wrote: > input arguments should appear before output arguments Done. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:265: StackSamplingProfiler::Sample* sample); On 2015/09/15 16:00:15, Mike Wittman wrote: > |sample| can go before |modules|, to be consistent with the Windows code and the > previous function Done. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:267: // Mach handle for thread being profiled. On 2015/09/15 13:28:19, Robert Sesek wrote: > Weak or strong right reference, here? Weak. updated comment. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:316: void NativeStackSamplerMac::AddModule( On 2015/09/15 16:00:15, Mike Wittman wrote: > This function feels like it's doing too many things. Is there a reason we can't > use the same functional split as in the Windows code: CopyToSample, > GetModuleIndex, GetModuleForHandle (maybe GetModuleForInstructionPointer here)? Split the function into CopyToSample and GetmoduleIndex. GetModuleFor* is strange since retrieving the module key into profile_module_index_ is done simultaneously with retrieving the module itself (using dladdr). https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:330: realpath(inf.dli_fname, filepath); On 2015/09/15 13:28:19, Robert Sesek wrote: > realpath will still touch disk. Is this necessary? Suppose not; this just means ./ or ../ won't necessarily be resolved in inf.dli_fname. Removed and updated tests to reflect change. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:348: for (int i = 0; i < stack_depth; i++) { On 2015/09/15 16:00:15, Mike Wittman wrote: > nit: no braces Done. https://codereview.chromium.org/1346453004/diff/140001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:356: PlatformThreadId thread_id) { On 2015/09/15 16:00:15, Mike Wittman wrote: > On 2015/09/15 13:28:19, Robert Sesek wrote: > > #if defined(__i386__) > > NOTIMPLEMENTED() << "This class only has support for x86_64 sampling. > > return nullptr; > > #else > > … > > #endif > > Just returning the null pointer is sufficient. The code that uses the > NativeStackSampler interprets that as being unsupported. Done.
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:325: #else nit: #endif here https://codereview.chromium.org/1346453004/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:265: EXPECT_EQ(executable_path, FilePath(filepath)); EXPECT_EQ(executable_path, MakeAbsoluteFilePath(filepath)); (from base/files/file_util.h)
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:325: #else On 2015/09/15 20:16:49, Mike Wittman wrote: > nit: #endif here Done. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:265: EXPECT_EQ(executable_path, FilePath(filepath)); On 2015/09/15 20:16:49, Mike Wittman wrote: > EXPECT_EQ(executable_path, MakeAbsoluteFilePath(filepath)); > > (from base/files/file_util.h) Done.
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:5: #include <dlfcn.h> #include <libkern/OSByteOrder.h> https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:20: const int kUUIDLengthBytes = 16; You use sizeof(uuid_t) instead of declaring your own constant (also include <uuid/uuid.h>). https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:53: uintptr_t prev; naming: base_pointer ? See the styleguide's remarks on naming: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin.... The preference is for description and precision, rather than the conservation of horizontal space. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:77: // These functions were cannibalized from Mach-O Identifier procedures found in Is this comment still true? https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:88: const fat_header* hdr = reinterpret_cast<const fat_header*>(module_addr); naming: header https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:96: for (uint32_t i = 0; i < SwapIfBig32(hdr->nfat_arch, swap); ++i) { The fat header is *always* in big endian. So OSSwapBigToHostInt32 is all that is necessary. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:107: const mach_header_64* hdr = naming: header https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:111: bool swap = hdr->magic == MH_CIGAM_64; This is named |swap| in functions but |big| in the prototype. I'd reconcile that, or just inline it: uint32_t cputype = header->magic == MH_CIGAM_64 ? OSSwapInt32(header->cputype) : header->cputype; return cputype == CPU_TYPE_X86_64; https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:112: return SwapIfBig32((*hdr).cputype, swap) == CPU_TYPE_X86_64; header->cputype https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:121: const mach_header_64* hdr = reinterpret_cast<const mach_header_64*>( naming: header https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:134: static_assert(sizeof(uuid_cmd->uuid) == kUUIDLengthBytes, Using sizeof(uuid_t) would then mean removing this static_assert. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:153: !(offset = GetMach64HeaderOffset(module_addr))) nit: needs braces since it is multi-line https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:228: size_t GetModuleIndex(const uintptr_t instruction_pointer, Doesn't need |const| since it's a scalar.
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:228: size_t GetModuleIndex(const uintptr_t instruction_pointer, On 2015/09/15 20:53:00, Robert Sesek wrote: > Doesn't need |const| since it's a scalar. This is probably okay here since the intent is to not change it within the implementation of GetModuleIndex. It's a benefit only for the implementation, not for the caller.
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:20: const int kUUIDLengthBytes = 16; On 2015/09/15 20:53:00, Robert Sesek wrote: > You use sizeof(uuid_t) instead of declaring your own constant (also include > <uuid/uuid.h>). You *can* use
one comment, otherwise NativeStackSamplerMac and tests lgtm https://codereview.chromium.org/1346453004/diff/200001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1346453004/diff/200001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:5: #include <stdlib.h> we no longer need this
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:5: #include <dlfcn.h> On 2015/09/15 20:53:00, Robert Sesek wrote: > #include <libkern/OSByteOrder.h> Done. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:20: const int kUUIDLengthBytes = 16; On 2015/09/15 21:11:37, Robert Sesek wrote: > On 2015/09/15 20:53:00, Robert Sesek wrote: > > You use sizeof(uuid_t) instead of declaring your own constant (also include > > <uuid/uuid.h>). > > You *can* use That's helpful. Thanks! https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:53: uintptr_t prev; On 2015/09/15 20:53:00, Robert Sesek wrote: > naming: base_pointer ? > > See the styleguide's remarks on naming: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin.... > The preference is for description and precision, rather than the conservation of > horizontal space. Done. Expanded the other abbreviations in naming. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:77: // These functions were cannibalized from Mach-O Identifier procedures found in On 2015/09/15 20:53:00, Robert Sesek wrote: > Is this comment still true? Guess not! Updated comment. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:88: const fat_header* hdr = reinterpret_cast<const fat_header*>(module_addr); On 2015/09/15 20:53:00, Robert Sesek wrote: > naming: header Done for all instances. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:96: for (uint32_t i = 0; i < SwapIfBig32(hdr->nfat_arch, swap); ++i) { On 2015/09/15 20:53:00, Robert Sesek wrote: > The fat header is *always* in big endian. So OSSwapBigToHostInt32 is all that is > necessary. Ok. Is there no such guarantee for other types of headers? https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:107: const mach_header_64* hdr = On 2015/09/15 20:53:00, Robert Sesek wrote: > naming: header Done. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:111: bool swap = hdr->magic == MH_CIGAM_64; On 2015/09/15 20:53:00, Robert Sesek wrote: > This is named |swap| in functions but |big| in the prototype. I'd > reconcile that, or just inline it: > > uint32_t cputype = header->magic == MH_CIGAM_64 ? OSSwapInt32(header->cputype) > : header->cputype; > return cputype == CPU_TYPE_X86_64; Kept function (since it's useful in GetX64UUIDAt), reconciled names. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:112: return SwapIfBig32((*hdr).cputype, swap) == CPU_TYPE_X86_64; On 2015/09/15 20:53:00, Robert Sesek wrote: > header->cputype Done. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:121: const mach_header_64* hdr = reinterpret_cast<const mach_header_64*>( On 2015/09/15 20:53:00, Robert Sesek wrote: > naming: header Done. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:134: static_assert(sizeof(uuid_cmd->uuid) == kUUIDLengthBytes, On 2015/09/15 20:53:00, Robert Sesek wrote: > Using sizeof(uuid_t) would then mean removing this static_assert. Done. https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:153: !(offset = GetMach64HeaderOffset(module_addr))) On 2015/09/15 20:53:00, Robert Sesek wrote: > nit: needs braces since it is multi-line Done. https://codereview.chromium.org/1346453004/diff/200001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1346453004/diff/200001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:5: #include <stdlib.h> On 2015/09/15 21:21:02, Mike Wittman wrote: > we no longer need this Done.
lgtm https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:134: static_assert(sizeof(uuid_cmd->uuid) == kUUIDLengthBytes, On 2015/09/15 21:49:43, sydli wrote: > On 2015/09/15 20:53:00, Robert Sesek wrote: > > Using sizeof(uuid_t) would then mean removing this static_assert. > > Done. I might be looking at the wrong header, but it appears that uuid_command is just uint8_t uuid[16]; Having the static assert would still be useful here. https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:90: for (uint32_t i = 0; i < OSSwapBigToHostInt32(header->nfat_arch); ++i) { Cache the result of OSSwapBigToHostInt32(header->nfat_arch) https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:119: bool swap = header->magic == MH_CIGAM_64; Nit: Group statement with the next set of statements. (e.g. linebreak above and remove linebreak below.) https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:266: const int max_stack_size = 64; Optional: It would be nice to unify this value somehow.
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:134: static_assert(sizeof(uuid_cmd->uuid) == kUUIDLengthBytes, On 2015/09/16 01:41:45, robliao wrote: > On 2015/09/15 21:49:43, sydli wrote: > > On 2015/09/15 20:53:00, Robert Sesek wrote: > > > Using sizeof(uuid_t) would then mean removing this static_assert. > > > > Done. > > I might be looking at the wrong header, but it appears that uuid_command is just > uint8_t uuid[16]; Having the static assert would still be useful here. Fair point. Keeping the static_assert would be good to do. https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:269: thread_suspend(thread_handle_); In the email thread, Mark mentioned that this could deadlock if the thread about to be suspended is holding a lock in dyld. He gave some ideas for handling that you may want to consider.
Patchset #10 (id:240001) has been deleted
https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:134: static_assert(sizeof(uuid_cmd->uuid) == kUUIDLengthBytes, On 2015/09/17 22:06:56, Robert Sesek wrote: > On 2015/09/16 01:41:45, robliao wrote: > > On 2015/09/15 21:49:43, sydli wrote: > > > On 2015/09/15 20:53:00, Robert Sesek wrote: > > > > Using sizeof(uuid_t) would then mean removing this static_assert. > > > > > > Done. > > > > I might be looking at the wrong header, but it appears that uuid_command is > just > > uint8_t uuid[16]; Having the static assert would still be useful here. > > Fair point. Keeping the static_assert would be good to do. Ah-- makes sense. Put it back. https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:90: for (uint32_t i = 0; i < OSSwapBigToHostInt32(header->nfat_arch); ++i) { On 2015/09/16 01:41:45, robliao wrote: > Cache the result of OSSwapBigToHostInt32(header->nfat_arch) Done. https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:119: bool swap = header->magic == MH_CIGAM_64; On 2015/09/16 01:41:45, robliao wrote: > Nit: Group statement with the next set of statements. (e.g. linebreak above and > remove linebreak below.) Done. https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... base/profiler/native_stack_sampler_mac.cc:269: thread_suspend(thread_handle_); On 2015/09/17 22:06:56, Robert Sesek wrote: > In the email thread, Mark mentioned that this could deadlock if the thread about > to be suspended is holding a lock in dyld. He gave some ideas for handling that > you may want to consider. Now forcibly binds library used during stack walk. Not sure about the best way to find the kernel library pathname, though; assuming it's always in /usr/lib/system feels very wrong. Is there a better way?
Message was sent while issue was closed.
On 2015/09/18 20:29:21, sydli wrote: > https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... > File base/profiler/native_stack_sampler_mac.cc (right): > > https://codereview.chromium.org/1346453004/diff/180001/base/profiler/native_s... > base/profiler/native_stack_sampler_mac.cc:134: > static_assert(sizeof(uuid_cmd->uuid) == kUUIDLengthBytes, > On 2015/09/17 22:06:56, Robert Sesek wrote: > > On 2015/09/16 01:41:45, robliao wrote: > > > On 2015/09/15 21:49:43, sydli wrote: > > > > On 2015/09/15 20:53:00, Robert Sesek wrote: > > > > > Using sizeof(uuid_t) would then mean removing this static_assert. > > > > > > > > Done. > > > > > > I might be looking at the wrong header, but it appears that uuid_command is > > just > > > uint8_t uuid[16]; Having the static assert would still be useful here. > > > > Fair point. Keeping the static_assert would be good to do. > > Ah-- makes sense. Put it back. > > https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... > File base/profiler/native_stack_sampler_mac.cc (right): > > https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... > base/profiler/native_stack_sampler_mac.cc:90: for (uint32_t i = 0; i < > OSSwapBigToHostInt32(header->nfat_arch); ++i) { > On 2015/09/16 01:41:45, robliao wrote: > > Cache the result of OSSwapBigToHostInt32(header->nfat_arch) > > Done. > > https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... > base/profiler/native_stack_sampler_mac.cc:119: bool swap = header->magic == > MH_CIGAM_64; > On 2015/09/16 01:41:45, robliao wrote: > > Nit: Group statement with the next set of statements. (e.g. linebreak above > and > > remove linebreak below.) > > Done. > > https://codereview.chromium.org/1346453004/diff/220001/base/profiler/native_s... > base/profiler/native_stack_sampler_mac.cc:269: thread_suspend(thread_handle_); > On 2015/09/17 22:06:56, Robert Sesek wrote: > > In the email thread, Mark mentioned that this could deadlock if the thread > about > > to be suspended is holding a lock in dyld. He gave some ideas for handling > that > > you may want to consider. > > Now forcibly binds library used during stack walk. Not sure about the best way > to find the kernel library pathname, though; assuming it's always in > /usr/lib/system feels very wrong. Is there a better way? Closing this review for now as it's now reasonably stale and will need updating by sydli@ before any submission. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
