Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702413006:
Enable storing last-dispatched exception per-thread. (Closed)
DescriptionEnable storing last-dispatched exception per-thread.
BUG=620813
Review-Url: https://codereview.chromium.org/2702413006
Cr-Commit-Position: refs/heads/master@{#459402}
Committed: https://chromium.googlesource.com/chromium/src/+/f42407803d0a7e928d2d6fda4e849d49d03b80de
Patch Set 1 #Patch Set 2 : store last exception in thread header #Patch Set 3 : some cleanup #
Total comments: 12
Patch Set 4 : harden exception recording #
Total comments: 26
Patch Set 5 : rebased #Patch Set 6 : removed user-data for exception tracking; keep it simple to start #
Total comments: 9
Patch Set 7 : addressed review comments by manzagop #
Total comments: 4
Patch Set 8 : added 'code' to recorded exception information #Patch Set 9 : fixed test (larger memory buffer) #
Messages
Total messages: 53 (35 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: This issue passed the CQ dry run.
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-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
I'll remove the crashpad changes from this before its submitted but wanted to leave an example of how it would be used.
siggi@chromium.org changed reviewers: + siggi@chromium.org
nice. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:846: const void* origin, is this the exception address? If so, maybe name it that. If not, we'd want to record the exception address somewhere, somehow. Also we want to explicitly record the exception code. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:849: ActivityTrackerMemoryAllocator* allocator) { Note that here you will be executing in the context of an exception, which may - as a case in point - occur in your own code, leading to reentrancy. EXTREME caution is warranted here, as if you goof, most likely there will be no record of this happening anywhere, as you've blown our chance to get a crash report from the field. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.h:970: return RecordExceptionImpl(::tracked_objects::GetProgramCounter(), origin); no - this the program counter will have to come from the exception record. There'll only be one or two places where you record this, and those places are incidental. This is however useful for ScopedFromHereAnnotation-style things, used to drill into e.g. frequently occurring tasks...
Some minor comments. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:595: // A memory location used to indicate if changes have been made to the stack update comment https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.h:763: PROCESS_UNHANDLED_EXCEPTION = 50, Perhaps this should be kept separate (e.g. a hit-exception key set to true), so we keep the information about the state when the exception occurred. The exception might also be recovered from.
https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:595: // A memory location used to indicate if changes have been made to the stack On 2017/02/24 19:06:14, manzagop wrote: > update comment Done. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:846: const void* origin, On 2017/02/24 18:37:41, Sigurður Ásgeirsson wrote: > is this the exception address? If so, maybe name it that. > If not, we'd want to record the exception address somewhere, somehow. > Also we want to explicitly record the exception code. Yes, it's for holding the address where the exception occurred. I've got 4+64 bits of information I can save in the activity. Anything beyond that has to be "user data" in key/value pairs. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:849: ActivityTrackerMemoryAllocator* allocator) { On 2017/02/24 18:37:41, Sigurður Ásgeirsson wrote: > Note that here you will be executing in the context of an exception, which may - > as a case in point - occur in your own code, leading to reentrancy. EXTREME > caution is warranted here, as if you goof, most likely there will be no record > of this happening anywhere, as you've blown our chance to get a crash report > from the field. There are two points that need hardening: 1) Creation of the thread-tracker object needs a lock but getting an existing one does not. I'll just get on the assumption that the tracker already exists. This will leave us blind during thread start-up, however. 2) The user-data object. That also requires a lock to create. The alternative is to create one in advance, per-thread, and then return it when needed. That seems bad. Perhaps I could keep one on reserve globally, extract it atomically if needed, and refill it later on during normal program operation. Two exceptions in a row could result in no user-data for the second, but that's probably the lesser concern. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.h:763: PROCESS_UNHANDLED_EXCEPTION = 50, On 2017/02/24 19:06:14, manzagop wrote: > Perhaps this should be kept separate (e.g. a hit-exception key set to true), so > we keep the information about the state when the exception occurred. The > exception might also be recovered from. Actually, I need to remove it because it writes to process_data which has a lock. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.h:970: return RecordExceptionImpl(::tracked_objects::GetProgramCounter(), origin); On 2017/02/24 18:37:41, Sigurður Ásgeirsson wrote: > no - this the program counter will have to come from the exception record. > There'll only be one or two places where you record this, and those places are > incidental. Every activity holds the PC of where it was created so this is just for consistency. The "origin" has the source of the exception.
Nice! Some questions. https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:849: ActivityTrackerMemoryAllocator* allocator) { On 2017/02/24 19:10:58, bcwhite wrote: > On 2017/02/24 18:37:41, Sigurður Ásgeirsson wrote: > > Note that here you will be executing in the context of an exception, which may > - > > as a case in point - occur in your own code, leading to reentrancy. EXTREME > > caution is warranted here, as if you goof, most likely there will be no record > > of this happening anywhere, as you've blown our chance to get a crash report > > from the field. > > There are two points that need hardening: > > 1) Creation of the thread-tracker object needs a lock but getting an existing > one does not. I'll just get on the assumption that the tracker already exists. > This will leave us blind during thread start-up, however. > > 2) The user-data object. That also requires a lock to create. The alternative > is to create one in advance, per-thread, and then return it when needed. That > seems bad. Perhaps I could keep one on reserve globally, extract it atomically > if needed, and refill it later on during normal program operation. Two > exceptions in a row could result in no user-data for the second, but that's > probably the lesser concern. IIRC the locks are to enable memory reuse. Another option is to just "leak" (give up on reuse) the allocator memory used in those cases. That said, it would be a problem if threads keep spinning up and hitting recoverable exceptions. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:60: // A pointer to an AcitivyUserData object that can used when handing exceptions. typo: AcitivyUserData, handing https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:344: void ActivityUserData::Reset() { DCHECK memory_/header_ or early exit. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:345: // Clear the memory in an atomic manner. ActivityUserData is either used in a thread affine way or behind a lock. Why can't you just memset? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:355: values_.clear(); Also reset memory_ and available_? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:534: void ActivityUserData::Initialize() { DCHECK memory_ or early return. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:543: ImportExistingData(); Worth skipping the import when just initialized? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:860: // Release user-data for an exception. Does this clear the exception data as soon as we pop an activity? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:879: header_->last_exception.user_data_ref = 0; This doesn't release the reference? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:890: // Re-use any existing user-data structure. How do the exception ref and data id get set? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:896: // Get the reserved "exception data" storage, if exists. IIUC only the first excepting thread can have data? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:1656: ThreadActivityTracker* tracker = GetTrackerForCurrentThread(); Comment on why we don't use GetOrCreate and why it's ok. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:1668: ActivityUserData* exception_data = new ActivityUserData(...); ...? https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.h:393: ActivityUserData(void* memory, size_t size); Mention memory points to zeroed out memory.
Change CL description to something like: Enable storing last-dispatched exception per-thread. Or some such. Didn't review.
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: 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_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 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Record unhandled exceptions in breadcrumbs. BUG=620813 ========== to ========== Enable storing last-dispatched exception per-thread. BUG=620813 ==========
https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/40001/base/debug/activity_tra... base/debug/activity_tracker.cc:849: ActivityTrackerMemoryAllocator* allocator) { On 2017/02/27 16:05:15, manzagop wrote: > On 2017/02/24 19:10:58, bcwhite wrote: > > On 2017/02/24 18:37:41, Sigurður Ásgeirsson wrote: > > > Note that here you will be executing in the context of an exception, which > may > > - > > > as a case in point - occur in your own code, leading to reentrancy. EXTREME > > > caution is warranted here, as if you goof, most likely there will be no > record > > > of this happening anywhere, as you've blown our chance to get a crash report > > > from the field. > > > > There are two points that need hardening: > > > > 1) Creation of the thread-tracker object needs a lock but getting an existing > > one does not. I'll just get on the assumption that the tracker already > exists. > > This will leave us blind during thread start-up, however. > > > > 2) The user-data object. That also requires a lock to create. The > alternative > > is to create one in advance, per-thread, and then return it when needed. That > > seems bad. Perhaps I could keep one on reserve globally, extract it > atomically > > if needed, and refill it later on during normal program operation. Two > > exceptions in a row could result in no user-data for the second, but that's > > probably the lesser concern. > > IIRC the locks are to enable memory reuse. Another option is to just "leak" > (give up on reuse) the allocator memory used in those cases. > > That said, it would be a problem if threads keep spinning up and hitting > recoverable exceptions. Correct on all counts. Better to avoid the leaks if possible. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:60: // A pointer to an AcitivyUserData object that can used when handing exceptions. On 2017/02/27 16:05:16, manzagop wrote: > typo: AcitivyUserData, handing Done. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:344: void ActivityUserData::Reset() { On 2017/02/27 16:05:16, manzagop wrote: > DCHECK memory_/header_ or early exit. Done. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:345: // Clear the memory in an atomic manner. On 2017/02/27 16:05:16, manzagop wrote: > ActivityUserData is either used in a thread affine way or behind a lock. Why > can't you just memset? It's written with thread affinity but another (analyzer) thread could also be reading it. Comment added. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:355: values_.clear(); On 2017/02/27 16:05:16, manzagop wrote: > Also reset memory_ and available_? Oops! Done. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:534: void ActivityUserData::Initialize() { On 2017/02/27 16:05:16, manzagop wrote: > DCHECK memory_ or early return. Done. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:543: ImportExistingData(); On 2017/02/27 16:05:16, manzagop wrote: > Worth skipping the import when just initialized? Not really since the first read will cause it to exit. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:860: // Release user-data for an exception. On 2017/02/27 16:05:16, manzagop wrote: > Does this clear the exception data as soon as we pop an activity? No. That is held completely independent of the activity stack, inside the thread "Header" structure. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:879: header_->last_exception.user_data_ref = 0; On 2017/02/27 16:05:16, manzagop wrote: > This doesn't release the reference? Yes. I hadn't finished the conversion from the first attempt when I had to leave that Friday but wanted to show the basic idea. Finishing it now. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:890: // Re-use any existing user-data structure. On 2017/02/27 16:05:16, manzagop wrote: > How do the exception ref and data id get set? The data_id is set when the user-data object was originally created, or below during Reset(). https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:896: // Get the reserved "exception data" storage, if exists. On 2017/02/27 16:05:15, manzagop wrote: > IIUC only the first excepting thread can have data? I'll refill the g_exception_data from time to time -- haven't worked out exactly when. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:1656: ThreadActivityTracker* tracker = GetTrackerForCurrentThread(); On 2017/02/27 16:05:16, manzagop wrote: > Comment on why we don't use GetOrCreate and why it's ok. Done. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.cc:1668: ActivityUserData* exception_data = new ActivityUserData(...); On 2017/02/27 16:05:16, manzagop wrote: > ...? "..." is a C++11 feature that automatically creates an object based on the developers wishes without requiring it to be be explicitly written out. Quite handy. https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/60001/base/debug/activity_tra... base/debug/activity_tracker.h:393: ActivityUserData(void* memory, size_t size); On 2017/02/27 16:05:16, manzagop wrote: > Mention memory points to zeroed out memory. Done.
lgtm Likely a good idea for Siggi to do another pass given the sensitive nature. https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:964: TimeDelta::FromInternalValue( Ticks to time might be worth a function. https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:677: void ExceptionActivity(const void* program_counter, nit: add a verb, eg [Store/Record/Save]ExceptionActivity? https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:983: // Record exception information for the current thread and return space where No space is returned? https://codereview.chromium.org/2702413006/diff/100001/third_party/crashpad/c... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2702413006/diff/100001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client_win.cc:118: // Store the event in persistent tracking data. Perhaps add a "// DO NOT SUBMIT" to be safe.
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/2702413006/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:964: TimeDelta::FromInternalValue( On 2017/03/20 14:29:55, manzagop wrote: > Ticks to time might be worth a function. Done. https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:677: void ExceptionActivity(const void* program_counter, On 2017/03/20 14:29:55, manzagop wrote: > nit: add a verb, eg [Store/Record/Save]ExceptionActivity? Done. https://codereview.chromium.org/2702413006/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:983: // Record exception information for the current thread and return space where On 2017/03/20 14:29:55, manzagop wrote: > No space is returned? Done. https://codereview.chromium.org/2702413006/diff/100001/third_party/crashpad/c... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2702413006/diff/100001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client_win.cc:118: // Store the event in persistent tracking data. On 2017/03/20 14:29:55, manzagop wrote: > Perhaps add a "// DO NOT SUBMIT" to be safe. Does that actually do anything to prevent it going through the CQ? I've added such in the past to keep me from uploading it (usually by having it extend past 80 characters).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2702413006/diff/100001/third_party/crashpad/c... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2702413006/diff/100001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client_win.cc:118: // Store the event in persistent tracking data. On 2017/03/21 12:25:06, bcwhite wrote: > On 2017/03/20 14:29:55, manzagop wrote: > > Perhaps add a "// DO NOT SUBMIT" to be safe. > > Does that actually do anything to prevent it going through the CQ? I've added > such in the past to keep me from uploading it (usually by having it extend past > 80 characters). So it does! chromium_presubmit fails. Good to know.
https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:989: return RecordExceptionImpl(::tracked_objects::GetProgramCounter(), origin); Sorry, but I don't understand how this relates to what we're trying to get done here. What we want to record is the following: 1. The exception code (DWORD). 2. The instruction pointer where the exception occurred. 3. The exception address (optionally) These come from various fields from EXCEPTION_POINTERS (https://msdn.microsoft.com/en-us/library/windows/desktop/ms679331(v=vs.85).aspx), notably ex_ptrs->ExceptionRecord.ExceptionCode ex_ptrs->ContextRecord.{Eip|Rip} ex_ptrs->ExceptionRecord.ExceptionAddress The address of *where we record the exception* is immaterial, so I don't know why ::tracked_objects::GetProgramCounters() enters the picture.
https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:989: return RecordExceptionImpl(::tracked_objects::GetProgramCounter(), origin); > What we want to record is the following: > 1. The exception code (DWORD). I can add that. > 2. The instruction pointer where the exception occurred. "origin". > 3. The exception address (optionally) I can add that, though it'll enlarge the "activity" structure for all activity types (since it's a union). > These come from various fields from EXCEPTION_POINTERS > (https://msdn.microsoft.com/en-us/library/windows/desktop/ms679331(v=vs.85).aspx), > notably > > ex_ptrs->ExceptionRecord.ExceptionCode > ex_ptrs->ContextRecord.{Eip|Rip} > ex_ptrs->ExceptionRecord.ExceptionAddress > > The address of *where we record the exception* is immaterial, so I don't know > why ::tracked_objects::GetProgramCounters() enters the picture. You asked that before. All activities record the PC of where they are added. This is just for consistency.
https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:989: return RecordExceptionImpl(::tracked_objects::GetProgramCounter(), origin); On 2017/03/21 14:40:40, bcwhite wrote: > > What we want to record is the following: > > 1. The exception code (DWORD). > > I can add that. > Cool. > > 2. The instruction pointer where the exception occurred. > > "origin". > Sounds good, might want to also document this parameter as such? > > 3. The exception address (optionally) > > I can add that, though it'll enlarge the "activity" structure for all activity > types (since it's a union). I'd leave it be then, as it's the least interesting bit of info. > > > > These come from various fields from EXCEPTION_POINTERS > > > (https://msdn.microsoft.com/en-us/library/windows/desktop/ms679331(v=vs.85).aspx), > > notably > > > > ex_ptrs->ExceptionRecord.ExceptionCode > > ex_ptrs->ContextRecord.{Eip|Rip} > > ex_ptrs->ExceptionRecord.ExceptionAddress > > > > The address of *where we record the exception* is immaterial, so I don't know > > why ::tracked_objects::GetProgramCounters() enters the picture. > > You asked that before. All activities record the PC of where they are added. > This is just for consistency. Ah, oki.
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...
I've reverted the example call in CrashPad. Now we just need a CL in the windows code that calls this. Another CL, though. https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2702413006/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:989: return RecordExceptionImpl(::tracked_objects::GetProgramCounter(), origin); On 2017/03/21 14:56:12, Sigurður Ásgeirsson wrote: > On 2017/03/21 14:40:40, bcwhite wrote: > > > What we want to record is the following: > > > 1. The exception code (DWORD). > > > > I can add that. > > > > Cool. Done.
lgtm as far as storing what we're after :)
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: 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 bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, siggi@chromium.org Link to the patchset: https://codereview.chromium.org/2702413006/#ps160001 (title: "fixed test (larger memory buffer)")
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": 160001, "attempt_start_ts": 1490359887006140,
"parent_rev": "567655f3f72b9ac9754c2fccfcacca4953d4c656", "commit_rev":
"f42407803d0a7e928d2d6fda4e849d49d03b80de"}
Message was sent while issue was closed.
Description was changed from ========== Enable storing last-dispatched exception per-thread. BUG=620813 ========== to ========== Enable storing last-dispatched exception per-thread. BUG=620813 Review-Url: https://codereview.chromium.org/2702413006 Cr-Commit-Position: refs/heads/master@{#459402} Committed: https://chromium.googlesource.com/chromium/src/+/f42407803d0a7e928d2d6fda4e84... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f42407803d0a7e928d2d6fda4e84... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
