Closed Bug 751673 Opened 9 years ago Closed 8 years ago

make fix_stack_using_bpsyms work for Windows assertion stacks

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(1 file, 3 obsolete files)

This patch makes fix_stack_using_bpsyms work for existing Windows assertion stacks. It's not fantastic, but it works.
Jesse suggested that I fix this to only build the extra dict on Windows. Also the code in automation.py should be fixed to use this stack fixer on Windows.
Summary: make fix_stack_using_bpsyms work for existing Windows assertion stacks → make fix_stack_using_bpsyms work for Windows assertion stacks
This doesn't really work in practice. I have a couple of patches that make it work, but I haven't hooked it up to automation.py yet.
When we don't have the PDB files available NS_DescribeCodeAddress falls
back to export symbols, which makes it impossible to post-process the
result into the real function names. This change makes it not use export
symbols.

I realize as I'm typing this that this will make symbols from system
libraries not show up, so perhaps this isn't the perfect solution. Maybe
we should augment the existing output with the module-relative offset,
since that's what we really need to get back to the real function name.
Attachment #722848 - Flags: review?(dbaron)
Updated version of the previous fix_stack_using_bpsyms patch that works on the output produced by the NS_StackWalk patch above.
Attachment #620814 - Attachment is obsolete: true
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> I realize as I'm typing this that this will make symbols from system
> libraries not show up, so perhaps this isn't the perfect solution. Maybe
> we should augment the existing output with the module-relative offset,
> since that's what we really need to get back to the real function name.

That sounds pretty much like what the Linux version does, no?  It sounds like a good idea to me.

(That said, I'm also not sure I'm still a good reviewer for this code.)
Okay, slight rework. This just changes the output format of windows stack frames to look more like those on mac/linux. I folded the fix_stack_using_bpsyms patch into this one because it's much simpler now. For bonus points I made the tiny change to automation.py to hook this up on Windows. I haven't tested that specifically because I couldn't find an easy-to-reproduce assertion in Mochitest and I was too lazy to insert one.

Here's what the output looks like with local PDB symbols with this patch:
XREMain::XRE_main+0x00000257 [xul +0x0000000000152EA7] (d:\build\mozilla-central\toolkit\xre\nsapprunner.cpp, line 3952)
XRE_main+0x00000035 [xul +0x0000000000153615] (d:\build\mozilla-central\toolkit\xre\nsapprunner.cpp, line 4155)

and without local PDB symbols, without filtering through fix_stack:
CoverageAddAnnotation+0x00022DB5 [xul +0x0000000000152EA7]
CoverageAddAnnotation+0x00023523 [xul +0x0000000000153615]

and after filtering through fix_stack:
XREMain::XRE_main(int,char * * const,nsXREAppData const *) [toolkit/xre/nsAppRunner.cpp:3952]
XRE_main [toolkit/xre/nsAppRunner.cpp:4155]
Attachment #722896 - Flags: review?(dbaron)
Attachment #722848 - Attachment is obsolete: true
Attachment #722848 - Flags: review?(dbaron)
Attachment #722850 - Attachment is obsolete: true
dbaron: if you don't want to review I can redirect.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> For bonus points I made the tiny change to automation.py to hook this up on
> Windows. I haven't tested that specifically because I couldn't find an
> easy-to-reproduce assertion in Mochitest and I was too lazy to insert one.

http://mxr.mozilla.org/mozilla-central/search?string=expectAssertions%28
(pick one without a 0)
Comment on attachment 722896 [details] [diff] [review]
fix Windows assertion stacks

r=dbaron
Attachment #722896 - Flags: review?(dbaron) → review+
Pushed to try to sanity check, found a weird edgecase it broke on, so I fixed that and re-pushed and it looks good:
https://tbpl.mozilla.org/?tree=Try&rev=c1e3b0b981e1

Sample output, from https://tbpl.mozilla.org/php/getParsedLog.php?id=20547967&tree=Try&full=1:
19:13:29     INFO -  ###!!! ASSERTION: Adding a child where we already have a child? This may misbehave: 'Error', file e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/docshell/shistory/src/nsSHEntry.cpp, line 595
19:13:39     INFO -  nsDocShell::AddChildSHEntry(nsISHEntry *,nsISHEntry *,int,unsigned int,bool) [docshell/base/nsDocShell.cpp:3814]
19:13:39     INFO -  nsDocShell::DoAddChildSHEntry(nsISHEntry *,int,bool) [docshell/base/nsDocShell.cpp:3887]
19:13:39     INFO -  nsDocShell::AddToSessionHistory(nsIURI *,nsIChannel *,nsISupports *,bool,nsISHEntry * *) [docshell/base/nsDocShell.cpp:10720]
19:13:39     INFO -  nsDocShell::OnStateChange(nsIWebProgress *,nsIRequest *,unsigned int,tag_nsresult) [docshell/base/nsDocShell.cpp:6491]
19:13:39     INFO -  nsDocLoader::DoFireOnStateChange(nsIWebProgress * const,nsIRequest * const,int &,tag_nsresult) [uriloader/base/nsDocLoader.cpp:1293]
19:13:39     INFO -  nsDocLoader::FireOnStateChange(nsIWebProgress *,nsIRequest *,int,tag_nsresult) [uriloader/base/nsDocLoader.cpp:1227]
(stack continues all the way down)
https://hg.mozilla.org/mozilla-central/rev/b871dfb2186f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 575188
You need to log in before you can comment on or make changes to this bug.