751673 - make fix_stack_using_bpsyms work for Windows assertion stacks

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[(not currently active) Ted Mielczarek]

This patch makes fix_stack_using_bpsyms work for existing Windows assertion stacks. It's not fantastic, but it works.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: make fix_stack_using_bpsyms work for existing Windows assertion stacks

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: don't use export symbols in NS_DescribeCodeAddress

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.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: make fix_stack_using_bpsyms work for existing Windows assertion stacks

Updated version of the previous fix_stack_using_bpsyms patch that works on the output produced by the NS_StackWalk patch above.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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.)

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: fix Windows assertion stacks

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]

Comment 8

[(not currently active) Ted Mielczarek]

dbaron: if you don't want to review I can redirect.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 722896 [details] [diff] [review]
fix Windows assertion stacks

r=dbaron

Comment 11

[(not currently active) Ted Mielczarek]

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)

Comment 12

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b871dfb2186f

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b871dfb2186f