946877 - __declspec(noreturn) may be adversely affecting stackwalking of NS_ABORT_OOM (Windows)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

The Windows declaration of MOZ_REALLY_CRASH calls a MOZ_NoReturn function, which is __declspec(noreturn).

I am experiencing issues with MOZ_CRASH not walking the stack correctly in crash reports, and I suspect this noreturn declaration may be part of the problem. In particular I'm seeing this behavior with NS_ABORT_OOM:

many functions call NS_ABORT_OOM: the Windows LTO system "outlines" the three instructions:

027FBDC0  push        dword ptr [edi+4]  
027FBDC3  call        NS_ABORT_OOM (2955F9Eh)  
$LN14:
027FBDC8  int         3  

Each function which calls NS_ABORT_OOM jumps to this outlined hunk, which means we can't really tell which function actually made the call. I'm not sure whether this is a problem specific to NS_ABORT_OOM or whether it affects other functions like this...

It's possible that we can rectify this just for NS_ABORT_OOM by building nsDebugImpl.cpp without PGO/LTO optimizations... I'm going to try that first. That will have the effect of hiding the "noreturn" clause from the optimizer, so that it assumes a normal return pattern and probably won't perform this outlining.

I understand from blame that the reason we have __declspec(noreturn) is to suppress warnings/errors about functions which don't return/don't return a value. I'd also like to see what actually happens in practice if we remove the "noreturn" bits and force people to return dummy values.

Comment 1

[Benjamin Smedberg]

Attachment: bug946877-DebugImpl-nopgo

This is the patch racing with 945042.

Comment 2

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5118bd7f0954

removing the noreturn turns out to be really hard because of warnings, and I can't find a way to disable the side effects, so I'm going to hope this is sufficient and reopen later if it doesn't actually help.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5118bd7f0954

Comment 4

[Mike Hommey [:glandium]]

That's not going to work until this reaches aurora, because nsDebugImpl.cpp is a unified source.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> That's not going to work until this reaches aurora, because nsDebugImpl.cpp
> is a unified source.

Note bug 945042 will fix this, and i want to have it in the tree before aurora.

Comment 6

[Benjamin Smedberg]

Reopening to move this out of unified sources for now.

Comment 7

[Benjamin Smedberg]

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

Moving version flags to track this back to FF28 and hopefully to 27 as well, since NS_ABORT_OOM landed on 27.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Benjamin Smedberg]

Comment on attachment 8343247 [details] [diff] [review]
bug946877-DebugImpl-nopgo

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 938794 made better annotations for OOM crashes in our data structures (strings etc) but the stacks are less than great because of this bug
User impact if declined: Worse OOM crash analysis
Testing completed (on m-c, etc.): landed and appears to be helping in crash-stats
Risk to taking this patch (and alternatives if risky): very low, mainly performance but that's unlikely for this file
String or IDL/UUID changes made by this patch: none

There were two patches in this bug, the first landed for 28 but the second was for 29 only, so requesting approval for both branches to get everything in sync.

Comment 10

[(no longer active)]

Benjamin, out of curiosity, do you know yet if this worked successfully?

Comment 11

[Benjamin Smedberg]

Yes, based on this crash-stats query: https://crash-stats.mozilla.com/search/?release_channel=nightly&build_id=%3E20131211000000&signature=~NS_ABORT_OOM&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform

I loaded a sample of crashes and the calling frame appears to be correct in each case without folding.

Comment 12

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ba3867108cd
https://hg.mozilla.org/releases/mozilla-beta/rev/a05c70a52b9b (the unified-sources patch wasn't necessary on beta because unified XPCOM didn't land there)

Comment 13

[Matt Brubeck (:mbrubeck)]

It looks like this caused significant perf improvements in Windows PGO builds on a variety of Talos benchmarks (tpaint, a11y, svg, tart, tp5):
https://groups.google.com/d/topic/mozilla.dev.tree-management/ZkRWKK-2Gew/discussion

Comment 14

[(no longer active)]

(In reply to comment #13)
> It looks like this caused significant perf improvements in Windows PGO builds
> on a variety of Talos benchmarks (tpaint, a11y, svg, tart, tp5):
> https://groups.google.com/d/topic/mozilla.dev.tree-management/ZkRWKK-2Gew/discussion

That... seems bogus.  :-)