|
|
DescriptionAdding PE TimeStamp to chrome trace to ease retrieving debug information
On windows, symbols servers can be queried to retrieve debug information.
The code identifier is used as a key to retrieve the exact executable.
Current chrome memory dump contains the loaded module and their loaded addresses.
example:
{"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"},
But to uniquely identify them and being able to retrieve debug information from
symbols servers, the TimeStamp present in PE header is required.
This patch is adding the "ts" (TimeStamp) field.
{"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"},
From these information one can query MS/Google symbols servers.
https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_
basename / code-ident /dll
The code-identifier is built from the timestamp AND the size of the executable
CC=chrisha@chromium.org, erikchen@chromium.org
R=primiano@chromium.org
BUG=719969
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2869073002
Cr-Commit-Position: refs/heads/master@{#475930}
Committed: https://chromium.googlesource.com/chromium/src/+/a81e171d89732c62f3e8d1846ea685baa87ecd6f
Patch Set 1 #
Total comments: 6
Patch Set 2 : primiano + cl format #
Total comments: 2
Patch Set 3 : nit #Patch Set 4 : fix components build #
Total comments: 2
Patch Set 5 : fix components build #Patch Set 6 : nit #Patch Set 7 : format #Patch Set 8 : nits #Patch Set 9 : nits #Patch Set 10 : dcheng comments #
Messages
Total messages: 66 (46 generated)
PTAL
I am sympathetic with the goal, if that is the best way to fetch symbols. just the current approach as-is has two disadvantages: 1) It grabs the PE metadata even if heap profiler is not enabled, just because we do detailed dumps 2) It keeps grabbing PE metadata on each DETAILED dump, over and over Can you: - capture the module timestamp only when the heap profiler is enabled (unless that thing has a trivial cost)? - if that get-timestamp call isn't trivial in terms of overhead, consider using a map to cache that. Just asking about perf because I always hear horror stories from brucedawson where apparently simple apis end up doing DCOM calls https://codereview.chromium.org/2869073002/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.h (right): https://codereview.chromium.org/2869073002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.h:35: uint64_t timestamp; since this is windows only can we call it win_module_timestamp or something like that? Otherwise this sounds like a "timestamp of the mmap" that "ourselves from the future" will find quite hard to understand. https://codereview.chromium.org/2869073002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2869073002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:25: #include "base/win/pe_image.h" this needs to be under #if defined(OS_WIN) right? https://codereview.chromium.org/2869073002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:280: base::win::PEImage pe_image(module_info.lpBaseOfDll); how long does this take? is it fine to keep doing this over and over? or should we cache this?
PTAnL This line: pe_image.GetNTHeaders()->FileHeader.TimeDateStamp; Should result in offset computation only: typedef struct _IMAGE_NT_HEADERS { DWORD Signature; IMAGE_FILE_HEADER FileHeader; IMAGE_OPTIONAL_HEADER OptionalHeader; } IMAGE_NT_HEADERS, *PIMAGE_NT_HEADERS; typedef struct _IMAGE_FILE_HEADER { WORD Machine; WORD NumberOfSections; DWORD TimeDateStamp; DWORD PointerToSymbolTable; DWORD NumberOfSymbols; WORD SizeOfOptionalHeader; WORD Characteristics; } IMAGE_FILE_HEADER, *PIMAGE_FILE_HEADER; see: https://msdn.microsoft.com/en-us/library/windows/desktop/ms680336(v=vs.85).aspx https://msdn.microsoft.com/en-us/library/windows/desktop/ms680313(v=vs.85).aspx https://codereview.chromium.org/2869073002/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps.h (right): https://codereview.chromium.org/2869073002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps.h:35: uint64_t timestamp; On 2017/05/09 16:34:01, Primiano Tucci wrote: > since this is windows only can we call it win_module_timestamp or something like > that? > Otherwise this sounds like a "timestamp of the mmap" that "ourselves from the > future" will find quite hard to understand. Agree that adding module_ will help understanding that it's not the timestamp of the mmap. I'm not a fan of win_ prefix. https://codereview.chromium.org/2869073002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2869073002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:25: #include "base/win/pe_image.h" On 2017/05/09 16:34:01, Primiano Tucci wrote: > this needs to be under #if defined(OS_WIN) right? right. https://codereview.chromium.org/2869073002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:280: base::win::PEImage pe_image(module_info.lpBaseOfDll); On 2017/05/09 16:34:01, Primiano Tucci wrote: > how long does this take? is it fine to keep doing this over and over? or should > we cache this? This is a trivial cost. Roughly, the PE image is already in memory and this is just computing the offset of a raw dword. TimeStamp = HMODULE[offset]
LGTM with 1 small comment addressed. https://codereview.chromium.org/2869073002/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2869073002/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:28: #include "base/win/pe_image.h" there is another OS_WIN section down on line 51, can you merge this with that?
fixed https://codereview.chromium.org/2869073002/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2869073002/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:28: #include "base/win/pe_image.h" On 2017/05/09 17:46:12, Primiano Tucci wrote: > there is another OS_WIN section down on line 51, can you merge this with that? Done.
landing...
The CQ bit was checked by etienneb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2869073002/#ps40001 (title: "nit")
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CL as-is is not working with the component build. is_clang = true is_asan = false is_debug = false is_component_build = true ninja: Entering directory `out\clang64' [1/1] Regenerating ninja files [29/27131] LINK(DLL) tracing.dll tracing.dll.lib tracing.dll.pdb FAILED: tracing.dll tracing.dll.lib tracing.dll.pdb error LNK2019: unresolved external symbol ... base::win::PEImage::RVAToAddr Unfortunately, the pe_image is only available in the static build now. https://cs.chromium.org/chromium/src/base/BUILD.gn?q=pe_image.cc&dr=C&l=1655 # This is the subset of files from base that should not be used with a dynamic # library. Note that this library cannot depend on base because base depends on # base_static. static_library("base_static") { sources = [ "base_switches.cc", "base_switches.h", "task_scheduler/switches.cc", "task_scheduler/switches.h", "win/pe_image.cc", "win/pe_image.h", ] We may need to change that.
The actual offset calculation is very trivial and there's no explicit need to pass through base::PEImage to do it. The types are all available directly in windows.h, and can be navigated in a couple lines of code. Maybe just a helper function that does the calculation directly? On Thu, May 11, 2017 at 10:52 AM <etienneb@chromium.org> wrote: > The CL as-is is not working with the component build. > > is_clang = true > is_asan = false > is_debug = false > is_component_build = true > > ninja: Entering directory `out\clang64' > [1/1] Regenerating ninja files > [29/27131] LINK(DLL) tracing.dll tracing.dll.lib tracing.dll.pdb > FAILED: tracing.dll tracing.dll.lib tracing.dll.pdb > > > error LNK2019: unresolved external symbol ... > base::win::PEImage::RVAToAddr > > > Unfortunately, the pe_image is only available in the static build now. > > > https://cs.chromium.org/chromium/src/base/BUILD.gn?q=pe_image.cc&dr=C&l=1655 > > # This is the subset of files from base that should not be used with a > dynamic > # library. Note that this library cannot depend on base because base > depends on > # base_static. > static_library("base_static") { > sources = [ > "base_switches.cc", > "base_switches.h", > "task_scheduler/switches.cc", > "task_scheduler/switches.h", > "win/pe_image.cc", > "win/pe_image.h", > ] > > > We may need to change that. > > > > https://codereview.chromium.org/2869073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
An alternative would be to call: GetTimestampForLoadedLibrary see https://msdn.microsoft.com/en-us/library/windows/desktop/ms680142(v=vs.85).aspx This implies a dependency to dbghelp and I'm afraid by the footing note: "All DbgHelp Functions, such as this one, are single threaded. Therefore, calls from more than one thread to this function will likely result in unexpected behavior or memory corruption. To avoid this, you must synchronize all concurrent calls from more than one thread to this function."
Description was changed from ========== Adding PE TimeStamp to chrome trace to ease retrieving debug information On windows, symbols servers can be queried to retrieve debug information. The code identifier is used as a key to retrieve the exact executable. Current chrome memory dump contains the loaded module and their loaded addresses. example: {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"}, But to uniquely identify them and being able to retrieve debug information from symbols servers, the TimeStamp present in PE header is required. This patch is adding the "ts" (TimeStamp) field. {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"}, From these information one can query MS/Google symbols servers. https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_ basename / code-ident /dll The code-identifier is built from the timestamp AND the size of the executable CC=chrisha@chromium.org, erikchen@chromium.org R=primiano@chromium.org BUG=719969 ========== to ========== Adding PE TimeStamp to chrome trace to ease retrieving debug information On windows, symbols servers can be queried to retrieve debug information. The code identifier is used as a key to retrieve the exact executable. Current chrome memory dump contains the loaded module and their loaded addresses. example: {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"}, But to uniquely identify them and being able to retrieve debug information from symbols servers, the TimeStamp present in PE header is required. This patch is adding the "ts" (TimeStamp) field. {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"}, From these information one can query MS/Google symbols servers. https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_ basename / code-ident /dll The code-identifier is built from the timestamp AND the size of the executable CC=chrisha@chromium.org, erikchen@chromium.org R=primiano@chromium.org BUG=719969 ==========
etienneb@chromium.org changed reviewers: + dcheng@chromium.org, jschuh@chromium.org
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2869073002/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2869073002/diff/60001/base/BUILD.gn#newcode1666 base/BUILD.gn:1666: deps = [ "//base/win:pe_image" ] The deps line looks a bit out of place below the sanitizer comment. Maybe put it above?
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by etienneb@chromium.org
The CQ bit was checked by etienneb@chromium.org to run a CQ dry run
The CQ bit was unchecked by etienneb@chromium.org
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by etienneb@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...
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Adding PE TimeStamp to chrome trace to ease retrieving debug information On windows, symbols servers can be queried to retrieve debug information. The code identifier is used as a key to retrieve the exact executable. Current chrome memory dump contains the loaded module and their loaded addresses. example: {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"}, But to uniquely identify them and being able to retrieve debug information from symbols servers, the TimeStamp present in PE header is required. This patch is adding the "ts" (TimeStamp) field. {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"}, From these information one can query MS/Google symbols servers. https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_ basename / code-ident /dll The code-identifier is built from the timestamp AND the size of the executable CC=chrisha@chromium.org, erikchen@chromium.org R=primiano@chromium.org BUG=719969 ========== to ========== Adding PE TimeStamp to chrome trace to ease retrieving debug information On windows, symbols servers can be queried to retrieve debug information. The code identifier is used as a key to retrieve the exact executable. Current chrome memory dump contains the loaded module and their loaded addresses. example: {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"}, But to uniquely identify them and being able to retrieve debug information from symbols servers, the TimeStamp present in PE header is required. This patch is adding the "ts" (TimeStamp) field. {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"}, From these information one can query MS/Google symbols servers. https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_ basename / code-ident /dll The code-identifier is built from the timestamp AND the size of the executable CC=chrisha@chromium.org, erikchen@chromium.org R=primiano@chromium.org BUG=719969 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by etienneb@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...
dcheng, PTAnL. I needed to change deps to public_deps to make it work. Is that fine? https://codereview.chromium.org/2869073002/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2869073002/diff/60001/base/BUILD.gn#newcode1666 base/BUILD.gn:1666: deps = [ "//base/win:pe_image" ] On 2017/05/11 20:31:20, dcheng wrote: > The deps line looks a bit out of place below the sanitizer comment. Maybe put it > above? Done.
The CQ bit was checked by etienneb@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jschuh@chromium.org changed reviewers: + scottmg@chromium.org
I'm still not entirely up to speed on GN. Scott, could you take a look and approve?
public_deps is OK. That just means that things that depend on base_static are allowed to include pe_image.h, which is how things were before. The other option would be to change other targets that currently depend on base_static to depend on both base_static and if (is_win) on base/win:pe_image. lgtm
lgtm
The CQ bit was checked by etienneb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2869073002/#ps180001 (title: "dcheng comments")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by etienneb@chromium.org
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": 180001, "attempt_start_ts": 1496243701225160, "parent_rev": "5dd8bd3e28d83ac7d744c6e00f7255cfde70d7f1", "commit_rev": "a81e171d89732c62f3e8d1846ea685baa87ecd6f"}
Message was sent while issue was closed.
Description was changed from ========== Adding PE TimeStamp to chrome trace to ease retrieving debug information On windows, symbols servers can be queried to retrieve debug information. The code identifier is used as a key to retrieve the exact executable. Current chrome memory dump contains the loaded module and their loaded addresses. example: {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"}, But to uniquely identify them and being able to retrieve debug information from symbols servers, the TimeStamp present in PE header is required. This patch is adding the "ts" (TimeStamp) field. {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"}, From these information one can query MS/Google symbols servers. https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_ basename / code-ident /dll The code-identifier is built from the timestamp AND the size of the executable CC=chrisha@chromium.org, erikchen@chromium.org R=primiano@chromium.org BUG=719969 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Adding PE TimeStamp to chrome trace to ease retrieving debug information On windows, symbols servers can be queried to retrieve debug information. The code identifier is used as a key to retrieve the exact executable. Current chrome memory dump contains the loaded module and their loaded addresses. example: {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\system32\\powrprof.dll","pf":0,"sa":"7ffdae7a0000","sz":"4b000"}, But to uniquely identify them and being able to retrieve debug information from symbols servers, the TimeStamp present in PE header is required. This patch is adding the "ts" (TimeStamp) field. {"bs":{"pc":"0","pd":"0","pss":"0","sc":"0","sd":"0","sw":"0"},"mf":"C:\\Windows\\SYSTEM32\\ntdll.dll","pf":0,"sa":"7ffdb2170000","sz":"1c1000","ts":"580ee321"}, From these information one can query MS/Google symbols servers. https://msdl.microsoft.com/download/symbols/ntdll.dll/580EE3211C1000/ntdll.dl_ basename / code-ident /dll The code-identifier is built from the timestamp AND the size of the executable CC=chrisha@chromium.org, erikchen@chromium.org R=primiano@chromium.org BUG=719969 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2869073002 Cr-Commit-Position: refs/heads/master@{#475930} Committed: https://chromium.googlesource.com/chromium/src/+/a81e171d89732c62f3e8d1846ea6... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a81e171d89732c62f3e8d1846ea6... |