Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710163002:
Implement basic support for Windows module emission. (Closed)
DescriptionImplement basic support for Windows module emission.
Native heap profiles require the load addresses of all modules so that program counters can be converted into symbols.
BUG=686208
Review-Url: https://codereview.chromium.org/2710163002
Cr-Commit-Position: refs/heads/master@{#452882}
Committed: https://chromium.googlesource.com/chromium/src/+/d43e0e0a70cbd8048886dd04e6b5554de28d520e
Patch Set 1 #
Total comments: 30
Patch Set 2 : Comments from wez. #
Total comments: 3
Patch Set 3 : Comments from etienne. #
Total comments: 9
Patch Set 4 : Respond to comments. #Patch Set 5 : Respond to comments. #Patch Set 6 : Respond to comments. #
Messages
Total messages: 29 (15 generated)
Description was changed from ========== Implement basic support for Windows Module emission. BUG= ========== to ========== Implement basic support for Windows module emission. BUG=686208 ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
erikchen@chromium.org changed reviewers: + wez@chromium.org
wez: Please review.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Very nice - thanks for the quick turn-around. :) https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:248: DWORD module_count = 1024; I would suggest including a brief comment to introduce each of the two blocks of work here, e.g: // Fetch handles to each of the modules loaded in this process. The process itself retains ownership of the returned handles. and // Query the base address for each module, and attach it to the dump. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:252: DWORD output_size; nit: Even though you're about to overwrite it, please initialize this. If you move this outside the loop then you can have requested_hmodule_bytes and result_hmodule_bytes, and only assign one to the other if (result > requested) - the rest of the code can just use result_hmodule_bytes. WDYT? (Or equally with counts, modulo some bit shifting ;) https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:254: module_count, &output_size); You're passing |module_count| here, where the API expects a size in bytes - I'm surprised this doesn't cause the code to tight-loop, actually! Note that this function is racey wrt the module table, as is the querying code below, so it's quite possible to have this, or the other calls, fail even though the process is in a good state, just because something got loaded or unloaded; might be worth noting that in a comment in case we start seeing flakiness. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:263: break; Can we re-arrange this to read as: while (true) { ... if (condition) break ... } It reads oddly to have break here, and continue in the conditional. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:265: for (unsigned int i = 0; i < module_count; ++i) { nit: Since |module_count|'s range is defined by DWORD, use a DWORD as the iterator. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:266: TCHAR module_name[MAX_PATH]; wchar_t https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:267: BOOL result = GetModuleFileName(modules[i], module_name, MAX_PATH); nit: You used :: prefix for EnumProcessModules(), so be consistent with this call too. Do we need the filename in any sort of "normalized" form, or is this primarily for pretty-printing purposes? https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:269: return false; This means failing to resolve any module's filename will prevent any mmap info being output; perhaps continue here, instead? https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:275: return false; As above, consider continue'ing here rather than failing. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:283: pmd->set_has_process_mmaps(); If you follow my suggestion to use continue's then you might want to make this conditional on there being >0 regions successfully queried. :) Is there any way to indicate that we know we failed to gather some regions? https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:603: defined(OS_WIN) nit: Are there any other platforms? Or is this no longer conditional? https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:290: TCHAR module_name[MAX_PATH]; wchar_t https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:295: bool found_components_unittests = false; For extra credit: Use GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, some_function_address) to get the handle to the module containing this test code (may or may not be the test exe) then grab a dump and check that it has a region containing the address, with the right module name. ;)
not for this CL: Let's move the platform-specific map fetchers into _win.cc etc files?
Description was changed from ========== Implement basic support for Windows module emission. BUG=686208 ========== to ========== Implement basic support for Windows module emission. Native heap profiles require the load addresses of all modules so that program counters can be converted into symbols. BUG=686208 ==========
wez: PTAL https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:248: DWORD module_count = 1024; On 2017/02/23 04:15:11, Wez wrote: > I would suggest including a brief comment to introduce each of the two blocks of > work here, e.g: > > // Fetch handles to each of the modules loaded in this process. The process > itself retains ownership of the returned handles. > > and > > // Query the base address for each module, and attach it to the dump. Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:252: DWORD output_size; On 2017/02/23 04:15:11, Wez wrote: > nit: Even though you're about to overwrite it, please initialize this. > > If you move this outside the loop then you can have requested_hmodule_bytes and > result_hmodule_bytes, and only assign one to the other if (result > requested) - > the rest of the code can just use result_hmodule_bytes. WDYT? > > (Or equally with counts, modulo some bit shifting ;) Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:254: module_count, &output_size); On 2017/02/23 04:15:11, Wez wrote: > You're passing |module_count| here, where the API expects a size in bytes - I'm > surprised this doesn't cause the code to tight-loop, actually! > > Note that this function is racey wrt the module table, as is the querying code > below, so it's quite possible to have this, or the other calls, fail even though > the process is in a good state, just because something got loaded or unloaded; > might be worth noting that in a comment in case we start seeing flakiness. fascinating, fixed. Also added a comment + buffer. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:263: break; On 2017/02/23 04:15:11, Wez wrote: > Can we re-arrange this to read as: > > while (true) { > ... > if (condition) > break > ... > } > > It reads oddly to have break here, and continue in the conditional. Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:265: for (unsigned int i = 0; i < module_count; ++i) { On 2017/02/23 04:15:11, Wez wrote: > nit: Since |module_count|'s range is defined by DWORD, use a DWORD as the > iterator. Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:266: TCHAR module_name[MAX_PATH]; On 2017/02/23 04:15:10, Wez wrote: > wchar_t Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:267: BOOL result = GetModuleFileName(modules[i], module_name, MAX_PATH); On 2017/02/23 04:15:10, Wez wrote: > nit: You used :: prefix for EnumProcessModules(), so be consistent with this > call too. > > Do we need the filename in any sort of "normalized" form, or is this primarily > for pretty-printing purposes? Switched to :: prefixes. The filename is necessary for converting PCs -> symbols, but the format/path isn't particularly important. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:269: return false; On 2017/02/23 04:15:11, Wez wrote: > This means failing to resolve any module's filename will prevent any mmap info > being output; perhaps continue here, instead? Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:275: return false; On 2017/02/23 04:15:11, Wez wrote: > As above, consider continue'ing here rather than failing. Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:283: pmd->set_has_process_mmaps(); On 2017/02/23 04:15:11, Wez wrote: > If you follow my suggestion to use continue's then you might want to make this > conditional on there being >0 regions successfully queried. :) Done > > Is there any way to indicate that we know we failed to gather some regions? No https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:603: defined(OS_WIN) On 2017/02/23 04:15:11, Wez wrote: > nit: Are there any other platforms? Or is this no longer conditional? Nope, fixed. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:290: TCHAR module_name[MAX_PATH]; On 2017/02/23 04:15:11, Wez wrote: > wchar_t Done. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:295: bool found_components_unittests = false; On 2017/02/23 04:15:11, Wez wrote: > For extra credit: > > Use GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, > some_function_address) to get the handle to the module containing this test code > (may or may not be the test exe) then grab a dump and check that it has a region > containing the address, with the right module name. ;) Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
etienneb@chromium.org changed reviewers: + etienneb@chromium.org
drive-by https://codereview.chromium.org/2710163002/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:261: result_module_count = output_size / sizeof(HMODULE); The way we were used to handle these racy windows Enumeration API is by always growing the size of the resulting output_size. That way, the while (true) { } must end one day (line:253). And, why not using: https://cs.chromium.org/chromium/src/base/win/win_util.cc?q=EnumProcessModule... bool GetLoadedModulesSnapshot(HANDLE process, std::vector<HMODULE>* snapshot) {
https://codereview.chromium.org/2710163002/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:261: result_module_count = output_size / sizeof(HMODULE); On 2017/02/23 21:11:36, etienneb wrote: > The way we were used to handle these racy windows Enumeration API is by always > growing the size of the resulting output_size. > > That way, the while (true) { } must end one day (line:253). Sorry, I don't understand what you mean. AFAICT, the code I wrote is almost identical to the code in win_util, but without a limit of 5 retries. > > And, why not using: > > https://cs.chromium.org/chromium/src/base/win/win_util.cc?q=EnumProcessModule... > > bool GetLoadedModulesSnapshot(HANDLE process, std::vector<HMODULE>* snapshot) > { Didn't realize it existed. Switched to it.
ajwong@chromium.org changed reviewers: + ajwong@chromium.org
https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:248: DWORD module_count = 1024; On 2017/02/23 04:15:11, Wez wrote: > I would suggest including a brief comment to introduce each of the two blocks of > work here, e.g: > > // Fetch handles to each of the modules loaded in this process. The process > itself retains ownership of the returned handles. > > and > > // Query the base address for each module, and attach it to the dump. Also, please comment on why 1024 makes sense as a magic number. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:51: #endif // defined(OS_MACOSX) #elif?
RS-LGTM on components/tracing/common/process_metrics_memory_dump_provider* to avoid timezone latency once you all are happy about the code, and assuming that the patch has this shape with per-os #ifdefs. ping-me if you change your mind and reshuffle other things or have any question about memory-infra. ship it without asking otherwise once ready.
lgtm https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:267: BOOL result = GetModuleFileName(modules[i], module_name, MAX_PATH); On 2017/02/23 21:00:14, erikchen wrote: > On 2017/02/23 04:15:10, Wez wrote: > > nit: You used :: prefix for EnumProcessModules(), so be consistent with this > > call too. > > > > Do we need the filename in any sort of "normalized" form, or is this primarily > > for pretty-printing purposes? > > Switched to :: prefixes. The filename is necessary for converting PCs -> > symbols, but the format/path isn't particularly important. Ah OK; I wondered if it might be important to get an absolute path, for that reason, for example. https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2710163002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:51: #endif // defined(OS_MACOSX) On 2017/02/23 22:05:44, awong wrote: > #elif? Or just rename the platform-specific module reading tests to TestPlatformRegionDump or something equally generic, so we can unconditionally friend that. https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:48: #endif nit: // defined(OS_WIN) here, since the block is getting long. https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:252: if (!result) nit: Do you need |result|, or can you just if (!GetLoadedModulesSnapshot()) (and similarly for the conditionals below)? https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:311: bool found_components_unittests = false; nit: Suggest renaming this to found_executable, since you called it executable_name, above.
some nits, lgtm https://codereview.chromium.org/2710163002/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:261: result_module_count = output_size / sizeof(HMODULE); > Didn't realize it existed. Switched to it. I'm fine with the switch. thx. https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:258: BOOL result = ::GetModuleFileName(modules[i], module_name, MAX_PATH); nit: I agree with wez@ on the previous message. You have a variable shadowed here (result) with a different type. This is valid, but not a good practice. Also, GetModuleFileName -> DWORD If the function succeeds, the return value is the length of the string that is copied to the buffer, in characters, GetModuleInformation -> BOOL
https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:258: BOOL result = ::GetModuleFileName(modules[i], module_name, MAX_PATH); I remember a corner case with the function GetModuleFileName. If you read the doc: https://msdn.microsoft.com/en-us/library/windows/desktop/ms683197(v=vs.85).aspx """ Return value [...] If the buffer is too small to hold the module name, the string is truncated to nSize characters including the terminating null character [...] """ This is why it's implemented that way: https://cs.chromium.org/chromium/src/content/common/sandbox_win.cc?dr=C&l=175 Which mean the mapped_filename can be truncated. region.mapped_file = base::SysWideToNativeMB(module_name); We should also note that MAX_PATH is not an hard limit, with unicode paths can be longer. see: https://msdn.microsoft.com/en-ca/library/windows/desktop/aa365247(v=vs.85).aspx read section: Maximum Path Length Limitation "The Windows API has many functions that also have Unicode versions to permit an extended-length path for a maximum total path length of 32,767 characters." These cases are rare and we can ignore them for now. I remember seeing malwares evading our detection system using these tricks.
https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:48: #endif On 2017/02/23 23:35:03, Wez wrote: > nit: // defined(OS_WIN) here, since the block is getting long. Done. https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:252: if (!result) On 2017/02/23 23:35:03, Wez wrote: > nit: Do you need |result|, or can you just if (!GetLoadedModulesSnapshot()) (and > similarly for the conditionals below)? Done. https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:258: BOOL result = ::GetModuleFileName(modules[i], module_name, MAX_PATH); On 2017/02/24 17:06:43, etienneb wrote: > I remember a corner case with the function GetModuleFileName. > > If you read the doc: > https://msdn.microsoft.com/en-us/library/windows/desktop/ms683197(v=vs.85).aspx > > """ > Return value > > [...] If the buffer is too small to hold the module name, the string is > truncated to nSize characters including the terminating null character [...] > """ > > This is why it's implemented that way: > https://cs.chromium.org/chromium/src/content/common/sandbox_win.cc?dr=C&l=175 > > Which mean the mapped_filename can be truncated. > region.mapped_file = base::SysWideToNativeMB(module_name); > > We should also note that MAX_PATH is not an hard limit, with unicode paths can > be longer. > see: > https://msdn.microsoft.com/en-ca/library/windows/desktop/aa365247(v=vs.85).aspx > read section: Maximum Path Length Limitation > "The Windows API has many functions that also have Unicode versions to > permit an extended-length path for a maximum total path length of 32,767 > characters." > > These cases are rare and we can ignore them for now. > I remember seeing malwares evading our detection system using these tricks. Noted. It looks like we mostly ignore this problem in our code base. https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2710163002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:311: bool found_components_unittests = false; On 2017/02/23 23:35:03, Wez wrote: > nit: Suggest renaming this to found_executable, since you called it > executable_name, above. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, etienneb@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2710163002/#ps100001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487957573466300,
"parent_rev": "25b6521ad2777278de6d1bf5cccf861e36b255d7", "commit_rev":
"d43e0e0a70cbd8048886dd04e6b5554de28d520e"}
Message was sent while issue was closed.
Description was changed from ========== Implement basic support for Windows module emission. Native heap profiles require the load addresses of all modules so that program counters can be converted into symbols. BUG=686208 ========== to ========== Implement basic support for Windows module emission. Native heap profiles require the load addresses of all modules so that program counters can be converted into symbols. BUG=686208 Review-Url: https://codereview.chromium.org/2710163002 Cr-Commit-Position: refs/heads/master@{#452882} Committed: https://chromium.googlesource.com/chromium/src/+/d43e0e0a70cbd8048886dd04e6b5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d43e0e0a70cbd8048886dd04e6b5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
