Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566983009:
Support storing information about what modules are loaded in the process. (Closed)
DescriptionSupport storing information about what modules are loaded in the process.
BUG=620813
Review-Url: https://codereview.chromium.org/2566983009
Cr-Commit-Position: refs/heads/master@{#446382}
Committed: https://chromium.googlesource.com/chromium/src/+/de229b494a475f4ac9e36383241b27716e806c51
Patch Set 1 #
Total comments: 21
Patch Set 2 : refactored to support analysis extraction #Patch Set 3 : addressed review comments by manzagop; fixed build problems #Patch Set 4 : rebased #
Total comments: 2
Patch Set 5 : addressed review comments by manzagop #Patch Set 6 : rebased #Patch Set 7 : updated with PMA changes and improved atomicity #
Total comments: 2
Patch Set 8 : new ModuleInfo fields #Patch Set 9 : rebased #
Total comments: 14
Patch Set 10 : fixed some problems with new data structure #
Total comments: 2
Patch Set 11 : rebased #
Total comments: 3
Patch Set 12 : changed GUID to generic 'identifier'; noted it and others as 'opaque' with no defined meaning #
Messages
Total messages: 119 (96 generated)
The CQ bit was checked by bcwhite@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
Here's the storage part for you to take a look at. I'll add the retrieval part soon. Actually setting the module information is going to be another CL -- and not so easy.
The CQ bit was checked by bcwhite@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Nice! Main comment is about the possibility of modules being loaded more than once at the same time. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.cc File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:63: // State of a module as stared in persistent memory. typo: stared -> stored https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:66: static constexpr size_t kExpectedInstanceSize = 24; nit: I like having units in variable names (kExpectedInstanceSizeBytes). Personal preference, up to you. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:66: static constexpr size_t kExpectedInstanceSize = 24; Unused? https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:76: bool EncodeFrom(const GlobalActivityTracker::ModuleInfo& info, size_t size); Why do you need the size? Ah it's the size budget. Maybe rename (max_size or ?) and possibly add a comment. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:82: // Deterimes the required memory size for the encoded storage. typo: Deterimes https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:87: sizeof(ModuleInfoStore) - sizeof(((ModuleInfoStore*)0)->pickle); Size of on the null pointer smells funny to me. Would offsetof(ModuleInfoStore, pickle) work? https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:1166: // to create a new record to accomodate a possibly longer length. typo: accommodate https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:1168: return; return outside the if(store)? https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:838: std::map<const std::string, PersistentMemoryAllocator::Reference> modules_; I believe it's possible to have the same module loaded at multiple offsets. We might need the key to be something like pair<base_address, string>.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
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 bcwhite@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 bcwhite@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.cc File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:63: // State of a module as stared in persistent memory. On 2016/12/14 20:08:53, manzagop wrote: > typo: stared -> stored Done. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:66: static constexpr size_t kExpectedInstanceSize = 24; On 2016/12/14 20:08:53, manzagop wrote: > nit: I like having units in variable names (kExpectedInstanceSizeBytes). > Personal preference, up to you. The constant is required (and used) by the PersistentMemoryAllocator. The name is fixed. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:76: bool EncodeFrom(const GlobalActivityTracker::ModuleInfo& info, size_t size); On 2016/12/14 20:08:53, manzagop wrote: > Why do you need the size? Ah it's the size budget. Maybe rename (max_size or ?) > and possibly add a comment. Done. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:82: // Deterimes the required memory size for the encoded storage. On 2016/12/14 20:08:53, manzagop wrote: > typo: Deterimes Done. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:87: sizeof(ModuleInfoStore) - sizeof(((ModuleInfoStore*)0)->pickle); On 2016/12/14 20:08:53, manzagop wrote: > Size of on the null pointer smells funny to me. Would offsetof(ModuleInfoStore, > pickle) work? Done. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:1166: // to create a new record to accomodate a possibly longer length. On 2016/12/14 20:08:53, manzagop wrote: > typo: accommodate Done. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:1168: return; On 2016/12/14 20:08:53, manzagop wrote: > return outside the if(store)? If it fails to load it, a new record will be created below. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:838: std::map<const std::string, PersistentMemoryAllocator::Reference> modules_; On 2016/12/14 20:08:54, manzagop wrote: > I believe it's possible to have the same module loaded at multiple offsets. We > might need the key to be something like pair<base_address, string>. That would be annoying. How about we wait to see if it's a real problem before addressing it?
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_...)
The CQ bit was checked by bcwhite@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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
lgtm https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.cc File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:1168: return; On 2016/12/14 21:59:05, bcwhite wrote: > On 2016/12/14 20:08:53, manzagop wrote: > > return outside the if(store)? > > If it fails to load it, a new record will be created below. But then the map insertion will fail as the key already exists? https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:838: std::map<const std::string, PersistentMemoryAllocator::Reference> modules_; On 2016/12/14 21:59:05, bcwhite wrote: > On 2016/12/14 20:08:54, manzagop wrote: > > I believe it's possible to have the same module loaded at multiple offsets. We > > might need the key to be something like pair<base_address, string>. > > That would be annoying. How about we wait to see if it's a real problem before > addressing it? Totally. :( IIRC Siggi said this is to be expected so minimally put a comment to highlight this case. Shouldn't affect chrome modules though. https://codereview.chromium.org/2566983009/diff/140001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/140001/base/debug/activity_tr... base/debug/activity_tracker.h:791: // Expected size for 32/64-bit check. Mention this is PMA requirement?
The CQ bit was checked by bcwhite@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 implementation by some compilers of std::atomic makes it impossible to hold them in PersistentMemory with the current interface. I have an update to the PMA that provides more advanced "object" support. I'll have to wait for that CL. https://codereview.chromium.org/2578323002/ https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.cc File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.cc:1168: return; On 2016/12/16 16:09:31, manzagop wrote: > On 2016/12/14 21:59:05, bcwhite wrote: > > On 2016/12/14 20:08:53, manzagop wrote: > > > return outside the if(store)? > > > > If it fails to load it, a new record will be created below. > > But then the map insertion will fail as the key already exists? Good point. Always return. https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:838: std::map<const std::string, PersistentMemoryAllocator::Reference> modules_; On 2016/12/16 16:09:31, manzagop wrote: > On 2016/12/14 21:59:05, bcwhite wrote: > > On 2016/12/14 20:08:54, manzagop wrote: > > > I believe it's possible to have the same module loaded at multiple offsets. > We > > > might need the key to be something like pair<base_address, string>. > > > > That would be annoying. How about we wait to see if it's a real problem > before > > addressing it? > > Totally. :( IIRC Siggi said this is to be expected so minimally put a comment to > highlight this case. Shouldn't affect chrome modules though. Done. If it comes to it, I'll probably support multiple addresses in the ModuleInfoRecord. https://codereview.chromium.org/2566983009/diff/140001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/140001/base/debug/activity_tr... base/debug/activity_tracker.h:791: // Expected size for 32/64-bit check. On 2016/12/16 16:09:31, manzagop wrote: > Mention this is PMA requirement? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by bcwhite@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by bcwhite@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by bcwhite@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by bcwhite@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: 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 bcwhite@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...
Patchset #8 (id:240001) has been deleted
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by bcwhite@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...
Patchset #7 (id:260001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #7 (id:280001) has been deleted
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 bcwhite@chromium.org to run a CQ dry run
Patchset #7 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PMA changes are in. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm up to the question about what the module info provider code will feed into this. https://codereview.chromium.org/2566983009/diff/320001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2566983009/diff/320001/base/debug/activity_tr... base/debug/activity_tracker.cc:1009: sizer.AddString(info.debug_identifier); Siggi was suggesting we do as little work for storing and store the most compact possible. In that line, we may want to store whatever the calling code provides us with. Do you know if it provides the identifiers, or the underlying info? For the code identifier, it's the size and timestamp. For the debug identifier, a guid and an age. https://cs.chromium.org/chromium/src/base/profiler/native_stack_sampler_win.c...
https://codereview.chromium.org/2566983009/diff/320001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2566983009/diff/320001/base/debug/activity_tr... base/debug/activity_tracker.cc:1009: sizer.AddString(info.debug_identifier); On 2017/01/13 23:01:02, manzagop wrote: > Siggi was suggesting we do as little work for storing and store the most compact > possible. In that line, we may want to store whatever the calling code provides > us with. Do you know if it provides the identifiers, or the underlying info? According to my brief conversation with Chris, it provides nothing but a handle to the module from which code can be written to gather whatever information is desired. These fields reflect what would be given to the proto during upload. I can change it to anything you wish and have the reporting code translate it or I can leave it as is and have the code that loads this information do whatever translation is necessary. Or the proto could be changed plus either of the above. :-)
On 2017/01/14 01:23:24, bcwhite wrote: > https://codereview.chromium.org/2566983009/diff/320001/base/debug/activity_tr... > File base/debug/activity_tracker.cc (right): > > https://codereview.chromium.org/2566983009/diff/320001/base/debug/activity_tr... > base/debug/activity_tracker.cc:1009: sizer.AddString(info.debug_identifier); > On 2017/01/13 23:01:02, manzagop wrote: > > Siggi was suggesting we do as little work for storing and store the most > compact > > possible. In that line, we may want to store whatever the calling code > provides > > us with. Do you know if it provides the identifiers, or the underlying info? > > According to my brief conversation with Chris, it provides nothing but a handle > to the module from which code can be written to gather whatever information is > desired. > > These fields reflect what would be given to the proto during upload. I can > change it to anything you wish and have the reporting code translate it or I can > leave it as is and have the code that loads this information do whatever > translation is necessary. Or the proto could be changed plus either of the > above. :-) Cool. I've sent an email to discuss.
The CQ bit was checked by bcwhite@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...
Fields updated. PTAL.
The CQ bit was checked by bcwhite@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.
Almost there! Some naming suggestions and my mistake on guid size. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:663: size_t size = 0; Size should probably go below with things that cannot change. If it does change, likely identifier, guid, etc are no longer right either. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:664: int64_t timestamp = 0; Would loader_timestamp lift confusion wrt below timestamp? https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:668: uint32_t identifier; Is this the module's timestamp which we'll combine with the size to get the identifier? https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:669: uint32_t age; Wdyt of debug_age, debug_guid? https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint64_t guid; I said 8 bytes and I was wrong. It's 16 bytes: https://msdn.microsoft.com/en-us/library/system.guid(v=vs.110).aspx
The CQ bit was checked by bcwhite@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...
https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:663: size_t size = 0; On 2017/01/23 20:33:29, manzagop wrote: > Size should probably go below with things that cannot change. If it does change, > likely identifier, guid, etc are no longer right either. Done. I was thinking of it like mapped memory where it's possible to map only a portion of it. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:664: int64_t timestamp = 0; On 2017/01/23 20:33:29, manzagop wrote: > Would loader_timestamp lift confusion wrt below timestamp? Done. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:668: uint32_t identifier; On 2017/01/23 20:33:29, manzagop wrote: > Is this the module's timestamp which we'll combine with the size to get the > identifier? Oh, sorry. I misread the comments in your email. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:669: uint32_t age; On 2017/01/23 20:33:29, manzagop wrote: > Wdyt of debug_age, debug_guid? Would those be directly tied to that of the main module? Easy enough to add, now or in the future, if you'll have the data source to populate them. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint64_t guid; On 2017/01/23 20:33:29, manzagop wrote: > I said 8 bytes and I was wrong. It's 16 bytes: > https://msdn.microsoft.com/en-us/library/system.guid(v=vs.110).aspx Done. Though "guid" sounds like a windows-specific thing. Perhaps "identifier" would be better?
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...)
lgtm https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:669: uint32_t age; On 2017/01/24 13:45:53, bcwhite wrote: > On 2017/01/23 20:33:29, manzagop wrote: > > Wdyt of debug_age, debug_guid? > > Would those be directly tied to that of the main module? Easy enough to add, > now or in the future, if you'll have the data source to populate them. Oh I meant that age and guid are properties of the module's debug file. No need to add anything. https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint64_t guid; On 2017/01/24 13:45:53, bcwhite wrote: > On 2017/01/23 20:33:29, manzagop wrote: > > I said 8 bytes and I was wrong. It's 16 bytes: > > https://msdn.microsoft.com/en-us/library/system.guid(v=vs.110).aspx > > Done. Though "guid" sounds like a windows-specific thing. Perhaps "identifier" > would be better? You're right other platforms likely don't have a guid but I don't know what they have. Identifier would be confusing though because the debug identifier is a function of (age, guid). Ignore for now since we're windows only for now? https://codereview.chromium.org/2566983009/diff/380001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/380001/base/debug/activity_tr... base/debug/activity_tracker.h:820: uint8_t guid[16]; // GUID of the module. For age/guid: "X of the module's debug file."
https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint64_t guid; On 2017/01/24 14:42:46, manzagop wrote: > On 2017/01/24 13:45:53, bcwhite wrote: > > On 2017/01/23 20:33:29, manzagop wrote: > > > I said 8 bytes and I was wrong. It's 16 bytes: > > > https://msdn.microsoft.com/en-us/library/system.guid(v=vs.110).aspx > > > > Done. Though "guid" sounds like a windows-specific thing. Perhaps > "identifier" > > would be better? > > You're right other platforms likely don't have a guid but I don't know what they > have. Identifier would be confusing though because the debug identifier is a > function of (age, guid). Ignore for now since we're windows only for now? Perhaps it should just be a std::string then since the length is platform specific. You can still save raw bytes in a std::string. I don't think we have to be concerned about naming ambiguities. Windows can have a "win_id" when you build it. To this class, it's just an opaque identifier.
manzagop@chromium.org changed reviewers: + scottmg@chromium.org
+Scott Let's ask someone who knows more about modules on other platforms. Scott: Brian is implementing logging of module information to breadcrumbs. Our approach is to log the raw info without processing (ie age+guid instead of formatting to debug identifier). We think we're good with what we have for windows but are wondering how it works on other platforms so the logging mechanism works there as well. E.g., where does the debug identifier come from - is there a guid? Do you know? Thanks!
The CQ bit was checked by bcwhite@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...
On 2017/01/24 15:02:53, manzagop wrote: > +Scott > > Let's ask someone who knows more about modules on other platforms. > > Scott: Brian is implementing logging of module information to breadcrumbs. Our > approach is to log the raw info without processing (ie age+guid instead of > formatting to debug identifier). We think we're good with what we have for > windows but are wondering how it works on other platforms so the logging > mechanism works there as well. E.g., where does the debug identifier come from - > is there a guid? Do you know? > > Thanks! Mac Crashpad pulls a UUID out of the Mach-O file: https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/m.... There's no age, so it sets the ago to 0 in the minidump. I'm not sure if there's similar things for Android or Linux though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2566983009/diff/400001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/400001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint8_t guid[16]; I'm looking to do the (guid+age) -> debug id conversion and it looks like it would be simpler if we could use the struct from here (only the data, not the functions): https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/... The formatting is a printf based on that structure, ie "%08X%04X%04X%02X%02X%02X%02X%02X%02X%02X%02X" of the subparts.
https://codereview.chromium.org/2566983009/diff/400001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/400001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint8_t guid[16]; On 2017/01/24 22:45:55, manzagop wrote: > I'm looking to do the (guid+age) -> debug id conversion and it looks like it > would be simpler if we could use the struct from here (only the data, not the > functions): > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/... > > The formatting is a printf based on that structure, ie > "%08X%04X%04X%02X%02X%02X%02X%02X%02X%02X%02X" of the subparts. > > That would be far too OS-specific for this class. I've changed it just to "identifier" and noted in the comment that is an "opaque" value meaning that the caller is free to use in whatever way it wishes. Your best bet is just to cast the 16-byte array into whatever structure you want. MyGuid* info_guid = reinterpret_cast<MyGuid*>(info->identifier);
The CQ bit was checked by bcwhite@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 bcwhite@chromium.org to run a CQ dry run
Patchset #12 (id:420001) has been deleted
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.
lgtm https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/360001/base/debug/activity_tr... base/debug/activity_tracker.h:669: uint32_t age; On 2017/01/24 14:42:46, manzagop wrote: > On 2017/01/24 13:45:53, bcwhite wrote: > > On 2017/01/23 20:33:29, manzagop wrote: > > > Wdyt of debug_age, debug_guid? > > > > Would those be directly tied to that of the main module? Easy enough to add, > > now or in the future, if you'll have the data source to populate them. > > Oh I meant that age and guid are properties of the module's debug file. No need > to add anything. But prefixing with debug_ might lift some ambiguity. https://codereview.chromium.org/2566983009/diff/380001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/380001/base/debug/activity_tr... base/debug/activity_tracker.h:820: uint8_t guid[16]; // GUID of the module. On 2017/01/24 14:42:46, manzagop wrote: > For age/guid: "X of the module's debug file." In case you missed this one. https://codereview.chromium.org/2566983009/diff/400001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2566983009/diff/400001/base/debug/activity_tr... base/debug/activity_tracker.h:670: uint8_t guid[16]; On 2017/01/25 01:13:23, bcwhite wrote: > On 2017/01/24 22:45:55, manzagop wrote: > > I'm looking to do the (guid+age) -> debug id conversion and it looks like it > > would be simpler if we could use the struct from here (only the data, not the > > functions): > > > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/... > > > > The formatting is a printf based on that structure, ie > > "%08X%04X%04X%02X%02X%02X%02X%02X%02X%02X%02X" of the subparts. > > > > > > That would be far too OS-specific for this class. I've changed it just to > "identifier" and noted in the comment that is an "opaque" value meaning that the > caller is free to use in whatever way it wishes. > > Your best bet is just to cast the 16-byte array into whatever structure you > want. > > MyGuid* info_guid = reinterpret_cast<MyGuid*>(info->identifier); Hm, that UUID structure seems to be used across crashpad's platforms. https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/snapshot/m...
The CQ bit was checked by bcwhite@chromium.org
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 bcwhite@chromium.org
The CQ bit was checked by bcwhite@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": 440001, "attempt_start_ts": 1485450267022520,
"parent_rev": "c85ce45fdc0e047ccf05e01f50dba845fe4af888", "commit_rev":
"de229b494a475f4ac9e36383241b27716e806c51"}
Message was sent while issue was closed.
Description was changed from ========== Support storing information about what modules are loaded in the process. BUG=620813 ========== to ========== Support storing information about what modules are loaded in the process. BUG=620813 Review-Url: https://codereview.chromium.org/2566983009 Cr-Commit-Position: refs/heads/master@{#446382} Committed: https://chromium.googlesource.com/chromium/src/+/de229b494a475f4ac9e36383241b... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:440001) as https://chromium.googlesource.com/chromium/src/+/de229b494a475f4ac9e36383241b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
