Closed Bug 1324093 Opened 7 years ago Closed 7 years ago

Too much inline code from MOZ_ASSERT, MOZ_RELEASE_ASSERT, MOZ_CRASH

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: away, Assigned: away)

Details

Attachments

(7 files, 5 obsolete files)

2.64 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.69 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.40 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.02 KB, patch
away
: review+
Details | Diff | Splinter Review
1.49 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.16 KB, patch
Details | Diff | Splinter Review
2.31 KB, patch
Details | Diff | Splinter Review
It's difficult to debug in disassembly mode when a function has lots of
these macros. Often an assert has to 'jne' over so much code that I
can't even see the jump target on same screenful of disassembly. And the
real work of the function gets lost in the noise of boilerplate code.

In theory a good compiler could banish the cold codepaths to a faraway
place, but MSVC doesn't understand MOZ_UNLIKELY, and local builds don't
get any help from PGO, so I still see a ton of code emitted in-line.

Here's the failure path of a MOZ_RELEASE_ASSERT on Win32:

> 11e6b51b 6a6c            push    6Ch
> 11e6b51d 68d0495312      push    offset xul!`string' (125349d0)
> 11e6b522 68b4275912      push    offset xul!`string' (125927b4)
> 11e6b527 68f8c4f011      push    offset xul!`string' (11f0c4f8)
> 11e6b52c 6a02            push    2
> 11e6b52e ff1520a0f011    call    dword ptr [xul!_imp____acrt_iob_func (11f0a020)]
> 11e6b534 83c404          add     esp,4
> 11e6b537 50              push    eax
> 11e6b538 e87ffe25fe      call    xul!fprintf (100cb3bc)
> 11e6b53d 6a02            push    2
> 11e6b53f ff1520a0f011    call    dword ptr [xul!_imp____acrt_iob_func (11f0a020)]
> 11e6b545 50              push    eax
> 11e6b546 ff1560a0f011    call    dword ptr [xul!_imp__fflush (11f0a060)]
> 11e6b54c a1a0a2f011      mov     eax,dword ptr [xul!_imp__gMozCrashReason (11f0a2a0)]
> 11e6b551 83c41c          add     esp,1Ch
> 11e6b554 c700cc275912    mov     dword ptr [eax],offset xul!`string' (125927cc)
> 11e6b55a cc              int     3
> 11e6b55b 6a03            push    3
> 11e6b55d c705000000006c000000 mov dword ptr ds:[0],6Ch
> 11e6b567 ff15d894f011    call    dword ptr [xul!_imp__GetCurrentProcess (11f094d8)]
> 11e6b56d 50              push    eax
> 11e6b56e ff155096f011    call    dword ptr [xul!_imp__TerminateProcess (11f09650)]

Observations:
(1) The TerminateProcess dance could be factored out, at a minimum.
    Though, I never figured out why we have that code at all, since the
    __debugbreak() is enough to bring down the process, and even if you
    catch it with a debugger and continue, the null deref will get you.
(2) MSVC has |#define stderr (__acrt_iob_func(2))| which leads to 
    repeated function calls. We could stash this into a local variable.
(3) MOZ_ReportAssertionFailure could be made never-inline, as long as
    we're careful to get the skip-frames right on the stack walking.
    (This would make #2 unnecessary)
(4) In debug builds, we probably don't care about crash reporter
    annotations.
(5) In non-debug builds, we probably shouldn't report MOZ_RELEASE_ASSERT
    to stderr, in line with the "100% safe in release builds" comment
    above MOZ_CRASH.

I expect we'd see a minor size improvement in release builds as a bonus.

I'm well aware of the need for crash reporting and assert stacks to
point to the actual failing code and not to some helper, as well as the
need to foil crash deduping ala bug 1119030. I intend to preserve the
existing behavior in these respects.
Drive-by cleanup.
Attachment #8819400 - Flags: review?(nfroyd)
Attachment #8819400 - Flags: review?(nfroyd) → review+
bsmedberg: Is it safe to assume that crash-stats watchers don't care much about debug builds?
Attachment #8819407 - Flags: review?(benjamin)
Not only does this trim the code, it also makes MOZ_RELEASE_ASSERT follow the advice of MOZ_CRASH earlier in the file:

 * If we're a DEBUG build and we crash at a MOZ_CRASH which provides an
 * explanation-string, we print the string to stderr.  Otherwise, we don't
 * print anything; this is because we want MOZ_CRASH to be 100% safe in release
 * builds, and it's hard to print to stderr safely when memory might have been
 * corrupted.

I placed the #ifdef DEBUG inside the definition of MOZ_Report{Crash,AssertionFailure} so that the scattered handful of external callers could get the same safety.

Waldo: I noticed that you were involved in several bugs in MOZ_ASSERT's past, so flagging you for extra feedback in case you have an opinion on this.
Attachment #8819412 - Flags: review?(nfroyd)
Attachment #8819412 - Flags: feedback?(jwalden+bmo)
You'd think that this would throw off the assertion stacks in nsTraceRefcnt::WalkTheStack. But as far as I can tell, it was already setting |skipFrames| too high!

On top of that, the function was getting out-of-lined in some instances already. It really should have been MOZ_ALWAYS_INLINE_EVEN_DEBUG.

(I'll need to test this carefully on other platforms)
Attachment #8819414 - Flags: review?(nfroyd)
Attachment #8819414 - Flags: feedback?(jwalden+bmo)
Attachment #8819414 - Attachment description: neverinline → Part 4: Make MOZ_Report{Crash,AssertionFailure} be MOZ_NEVER_INLINE.
Attachment #8819414 - Attachment is patch: true
Comment on attachment 8819412 [details] [diff] [review]
Part 3: Don't MOZ_ReportAssertionFailure in non-debug builds.

Review of attachment 8819412 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +154,5 @@
>  static MOZ_COLD MOZ_ALWAYS_INLINE void
>  MOZ_ReportAssertionFailure(const char* aStr, const char* aFilename, int aLine)
>    MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS
>  {
> +#ifdef DEBUG

Shoot, I just realized that this may collide with the patch where I make this MOZ_NEVER_INLINE, leading to a function call that does nothing. (I was taking a different approach in an earlier revision on my machine.) MSVC seems to elide the function call anyway, but I don't feel good depending on that for other compilers.

Maybe I should just switch on #ifdef DEBUG at the caller level, after all?
(In reply to David Major [:dmajor] from comment #2)
> Part 1: Move MOZ_REALLY_CRASH's null-deref and TerminateProcess into a
> never-inline function.

(In reply to David Major [:dmajor] from comment #5)
> Part 4: Make MOZ_Report{Crash,AssertionFailure} be MOZ_NEVER_INLINE.

Why mark these functions MOZ_NEVER_INLINE instead of moving them to an Assertions.cpp file? Won't MOZ_NEVER_INLINE emit redundant function definitions in every compilation unit?
The contents of the cpp file can only live in one place; in this case Assertions.cpp belongs to mozglue.dll, and I didn't want to bother with an exported symbol and potential linker-wrangling for this. The linker will fold together the identical function definitions in release builds.
Comment on attachment 8819403 [details] [diff] [review]
Part 1: Move MOZ_REALLY_CRASH's null-deref and TerminateProcess into a never-inline function.

Review of attachment 8819403 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +207,5 @@
>      * practically you need MSVC for debugging, and we only ship builds created
>      * by MSVC, so doing it this way reduces complexity.)
>      */
>  
> +static MOZ_COLD MOZ_NORETURN MOZ_NEVER_INLINE void MOZ_NoReturn(int aLine)

I think the only concern I have about this is that it's going to cause crashstats fits because the crash happens in a completely different function.  Can you file a pull request along the lines of:

https://github.com/mozilla/socorro/pull/3176

to get this function added to the skiplist before this patch series lands?  Maybe renaming it to MOZ_CrashAndDoNotReturn?
Attachment #8819403 - Flags: review?(nfroyd) → review+
> I think the only concern I have about this is that it's going to cause
> crashstats fits because the crash happens in a completely different
> function.  Can you file a pull request along the lines of:
> 
> https://github.com/mozilla/socorro/pull/3176
> 
> to get this function added to the skiplist before this patch series lands? 
> Maybe renaming it to MOZ_CrashAndDoNotReturn?

The thing that actually triggers breakpad is the __debugbreak(), outside of the function. I've kept that unchanged so that crash-stats (and developers running in a debugger) won't know the difference.

(As far as I can tell, that nullptr+TerminateProcess sequence is ancient and not relevant anymore. The null deref only kicks in if you use a debugger to continue past the break, and the TerminateProcess shouldn't be needed at all anymore. I'm tempted to just get rid of it, but after this change there will only be a single instance, so it's pretty much harmless.)
Comment on attachment 8819412 [details] [diff] [review]
Part 3: Don't MOZ_ReportAssertionFailure in non-debug builds.

Review of attachment 8819412 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +154,5 @@
>  static MOZ_COLD MOZ_ALWAYS_INLINE void
>  MOZ_ReportAssertionFailure(const char* aStr, const char* aFilename, int aLine)
>    MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS
>  {
> +#ifdef DEBUG

Yeah, I think switch on #ifdef DEBUG at callers.
Attachment #8819412 - Flags: feedback?(jwalden+bmo) → feedback-
Comment on attachment 8819414 [details] [diff] [review]
Part 4: Make MOZ_Report{Crash,AssertionFailure} be MOZ_NEVER_INLINE.

Review of attachment 8819414 [details] [diff] [review]:
-----------------------------------------------------------------

As long as this doesn't throw off assertion stacks, okay.
Attachment #8819414 - Flags: feedback?(jwalden+bmo) → feedback+
Comment on attachment 8819412 [details] [diff] [review]
Part 3: Don't MOZ_ReportAssertionFailure in non-debug builds.

Review of attachment 8819412 [details] [diff] [review]:
-----------------------------------------------------------------

Agree with Jeff here.  I think we *could* probably depend on the inlining heuristics, but better safe than sorry.
Attachment #8819412 - Flags: review?(nfroyd) → review-
Comment on attachment 8819414 [details] [diff] [review]
Part 4: Make MOZ_Report{Crash,AssertionFailure} be MOZ_NEVER_INLINE.

Review of attachment 8819414 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with some careful testing and willingness to be on call for any crash reporting breakage.
Attachment #8819414 - Flags: review?(nfroyd) → review+
Comment on attachment 8819407 [details] [diff] [review]
Part 2: Don't AnnotateMozCrashReason on debug builds.

I read through this and I'm a bit confused as to what this buys us. You're right that crash-stats itself probably doesn't care about this annotation, but I'm not so sure about automation. Does this reason string defeat the inlining done in the rest of the patch?
Flags: needinfo?(dmajor)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> I read through this and I'm a bit confused as to what this buys us. You're
> right that crash-stats itself probably doesn't care about this annotation,
> but I'm not so sure about automation. Does this reason string defeat the
> inlining done in the rest of the patch?

This patch is independent of the others. It's just intended to be an additional little savings in the amount of generated code in the disassembly view. (This bug was prompted by a moment at the workweek when my laptop's small screen was so full of assert gunk that I couldn't see any actual code.)

Good point about automation. I've checked out several architectures in treeherder and they don't seem to be using the reason string in their reporting, so I think we should be fine there.
Flags: needinfo?(dmajor)
Attachment #8819407 - Flags: review?(benjamin) → review+
(In reply to David Major [:dmajor] from comment #10)
> (As far as I can tell, that nullptr+TerminateProcess sequence is ancient and
> not relevant anymore. The null deref only kicks in if you use a debugger to
> continue past the break, and the TerminateProcess shouldn't be needed at all
> anymore. I'm tempted to just get rid of it, but after this change there will
> only be a single instance, so it's pretty much harmless.)

If they're otherwise unneeded, might be kind of nice to remove the null deref + TerminateProcess altogether so you could more easily continue execution past a MOZ_CRASH() during debugging? Probably a decision for another bug.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #17)
> If they're otherwise unneeded, might be kind of nice to remove the null
> deref + TerminateProcess altogether so you could more easily continue
> execution past a MOZ_CRASH() during debugging? Probably a decision for
> another bug.

I'm open to having my arm twisted to remove TerminateProcess. It's thoroughly unnecessary. I'm a bit scared of removing the null deref though. Given the nature of the things that we MOZ_CRASH for, I want to be super-extra sure that we kill the process even if there's some buggy injected code that somehow swallows a __debugbreak().
Is this more palatable?
Attachment #8819412 - Attachment is obsolete: true
Attachment #8823782 - Flags: review?(nfroyd)
Attachment #8823782 - Flags: feedback?(jwalden+bmo)
When Part 4 is combined with the revised Part 3, all platforms' compilers complain about unused MOZ_ReportAssertionFailure in some files. This will suppress those warnings.
Attachment #8823788 - Flags: review?(nfroyd)
Minor update to use MOZ_MAYBE_UNUSED.
Attachment #8819414 - Attachment is obsolete: true
Attachment #8823791 - Flags: review+
Missed a qref
Attachment #8823788 - Attachment is obsolete: true
Attachment #8823788 - Flags: review?(nfroyd)
Attachment #8823792 - Flags: review?(nfroyd)
Here's a different approach to part 1 that removes TerminateProcess altogether. If I'm really trying to go for compact disassembly, then it's shorter to do:
  mov dword ptr [0], __LINE__
than the previous Part 1's:
  mov rcx, __LINE__
  call MOZ_NoReturn
And it has the benefit of removing some dusty code. WDYT?
Attachment #8823796 - Flags: review?(nfroyd)
Comment on attachment 8823792 [details] [diff] [review]
Part 3.5: Add MOZ_MAYBE_UNUSED to mfbt/Attributes.h

Review of attachment 8823792 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Attributes.h
@@ +271,5 @@
> + */
> +#if defined(__GNUC__) || defined(__clang__)
> +#  define MOZ_MAYBE_UNUSED __attribute__ ((__unused__))
> +#elif defined(_MSC_VER)
> +#  define MOZ_MAYBE_UNUSED __pragma(warning(suppress:4505))

Nice, I didn't know MSVC had something for this.
Attachment #8823792 - Flags: review?(nfroyd) → review+
Comment on attachment 8823782 [details] [diff] [review]
Part 3: Don't MOZ_ReportAssertionFailure in non-debug builds.

Review of attachment 8823782 [details] [diff] [review]:
-----------------------------------------------------------------

I like this better.

::: mfbt/Assertions.h
@@ +353,5 @@
>  #  define MOZ_VALIDATE_ASSERT_CONDITION_TYPE(x)
>  #endif
>  
> +#if defined(DEBUG)
> +#  define MOZ_REPORT_ASSERTION_FAILURE(...) MOZ_ReportAssertionFailure(__VA_ARGS__)

I assume that you tagged MOZ_ReportAssertionFailure with MOZ_MAYBE_UNUSED?
Attachment #8823782 - Flags: review?(nfroyd) → review+
Comment on attachment 8823796 [details] [diff] [review]
Part 1 (alternate version): Remove the unreachable TerminateProcess in MOZ_REALLY_CRASH

Review of attachment 8823796 [details] [diff] [review]:
-----------------------------------------------------------------

On the one hand, I like this patch: less code, less clutter in the disassembly.  OTOH, I dislike this patch because it makes the Windows and non-Windows codepaths differ in style: we call abort() from the non-Windows codepaths, and expect the null deref to Just Work on the Windows one.  Though I suppose that anybody who can force the program to continue after the null deref could probably skip over the TerminateProcess call anyway?

Being conservative, I vote that we go with the previous version.
Attachment #8823796 - Flags: review?(nfroyd)
Attachment #8823782 - Flags: feedback?(jwalden+bmo) → feedback+
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/865a34953fb5
Part 0: Use MOZ_{BEGIN,END}_EXTERN_C in Assertions.h. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cc90db1908
Part 1: Move MOZ_REALLY_CRASH's null-deref and TerminateProcess into a never-inline function. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee273b613db
Part 2: Don't AnnotateMozCrashReason on debug builds. r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/b843a34fdc23
Part 3: Don't MOZ_ReportAssertionFailure in non-debug builds. r=froydnj f=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/630abb959e06
Part 3.5: Add MOZ_MAYBE_UNUSED to mfbt/Attributes.h. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c26d27a3635
Part 4: Make MOZ_Report{Crash,AssertionFailure} be MOZ_NEVER_INLINE. r=froydnj
I looked at a few taskcluster loaners, and as far as I can tell, this assert may pass or fail depending on the implementation of MOZ_RELEASE_ASSERT. I can't afford to spend any more time debugging this. I'm going to assume that it's a miscompile and propose that we just skip the assert on ASan builds.
Flags: needinfo?(dmajor)
Attachment #8827049 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8827049 [details] [diff] [review]
Part 2.5: Disable a miscompiled assertion in ASan builds

Review of attachment 8827049 [details] [diff] [review]:
-----------------------------------------------------------------

This assertion is used to assert that our optimizations are working well. It is used by a few test cases, and triggered deliberately with:

http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/self-test/assertRecoveredOnBailout-1.js

Are you sure it is being miss-compiled?
(In reply to Nicolas B. Pierron [:nbp] from comment #31)
> http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/self-test/
> assertRecoveredOnBailout-1.js
> 
> Are you sure it is being miss-compiled?

Did you patch-set changed the exit code returned in case of assertion failures?  In which case you might want to change the following code:

http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/js/src/tests/lib/jittests.py#415
(In reply to Nicolas B. Pierron [:nbp] from comment #32)
> (In reply to Nicolas B. Pierron [:nbp] from comment #31)
> > http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/self-test/
> > assertRecoveredOnBailout-1.js
> > 
> > Are you sure it is being miss-compiled?
> 
> Did you patch-set changed the exit code returned in case of assertion
> failures?  In which case you might want to change the following code:
> 
> http://searchfox.org/mozilla-central/rev/
> 0aed9484bd3e97206fd1949ee4a4992ef300a81f/js/src/tests/lib/jittests.py#415

Are you saying that the test actually wants the MOZ_RELEASE_ASSERT to fail?

In this case, given the stderr parsing in that jittest.py line, it seems I can't disable the error spew for ASan builds.
Attachment #8823782 - Attachment is obsolete: true
Attachment #8827049 - Attachment is obsolete: true
Attachment #8827049 - Flags: review?(nicolas.b.pierron)
Attachment #8827280 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8827280 [details] [diff] [review]
Part 3: Don't MOZ_ReportAssertionFailure in non-debug non-asan builds.

Review of attachment 8827280 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to David Major [:dmajor] from comment #33)
> Are you saying that the test actually wants the MOZ_RELEASE_ASSERT to fail?

Yes, this code path is only reachable by inlining a testing function.

This patch sounds good to me.

For your information, this patch will also solve part of the memory issue reported in Bug 1276912.
Attachment #8827280 - Flags: feedback?(nicolas.b.pierron) → feedback+
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd54089097b
Part 0: Use MOZ_{BEGIN,END}_EXTERN_C in Assertions.h. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/9939303eecf1
Part 1: Move MOZ_REALLY_CRASH's null-deref and TerminateProcess into a never-inline function. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/58b1e40375e6
Part 2: Don't AnnotateMozCrashReason on debug builds. r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/f82548e0df70
Part 3: Don't MOZ_ReportAssertionFailure in non-debug builds. r=froydnj f=Waldo f=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/d319245c16de
Part 3.5: Add MOZ_MAYBE_UNUSED to mfbt/Attributes.h. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e50de104a6
Part 4: Make MOZ_Report{Crash,AssertionFailure} be MOZ_NEVER_INLINE. r=froydnj
good news, this seemed to show an decrease (improvement) in the installer size:
== Change summary for alert #4825 (as of January 17 2017 21:11 UTC) ==

Improvements:

  7%  installer size summary osx-10-7 debug                    70111734.92 -> 64941884.67
  4%  installer size summary android-4-0-armv7-api15 debug     34587625.92 -> 33354180.58
  2%  installer size summary linux64 debug                     66578403.42 -> 64978646.25
  2%  installer size summary windows2012-64 debug              78673488.25 -> 77108099.67
  2%  installer size summary windows8-64 debug                 78751506.33 -> 77194146.33
  1%  installer size summary windows2012-32 debug              65949431.75 -> 65070774.75
  1%  installer size summary windowsxp debug                   66029779.33 -> 65153944.58
  1%  installer size summary linux32 debug                     65691093.58 -> 65101941.58
  0%  installer size summary windows2012-64 pgo                57441109.17 -> 57208311.25
  0%  installer size summary windows8-64 opt                   57404735.5 -> 57183518.67
  0%  installer size summary windows2012-64 opt                57344916.5 -> 57126334.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4825
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: