It would be helpful if MOZ_CRASH_UNSAFE_PRINTF would do its crashing inline at the caller, so that CI failure logs can blame the right code.
Before this patch, MOZ_CRASH_UNSAFE_PRINTF calls MOZ_CrashPrintf, which does the printf work and crashes.
This patch pulls out the crashing piece at the end, so that MOZ_CrashPrintf only does the printf work, and returns the string to the caller, who will MOZ_Crash inline.
Differential Revision: https://phabricator.services.mozilla.com/D25329
The current rust panic hook keeps a string for the crash reporter, and
goes on calling the default rust panic hook, which prints out a crash
stack... when RUST_BOOTSTRAP is set *and* when that works. Notably, on
both mac and Windows, it only really works for local builds, but fails
for debug builds from automation, although on automation itself, we also
do stackwalk from crash minidumps, which alleviates the problem.
Artifact debug builds are affected, though.
More importantly, C++ calls to e.g. MOZ_CRASH have a similar but
different behavior, in that they dump a stack trace on debug builds, by
default (with exceptions, see below for one). The format of those stack
traces is understood by the various fix*stack*py scripts under
tools/rb/, that are used by the various test harnesses both on
automation and locally.
Additionally, the current rust panic hook, as it calls the default rust
panic hook, ends up calling abort() on non-Windows platforms, which ends
up being verbosely redirected to mozalloc_abort per
https://dxr.mozilla.org/mozilla-central/rev/237e4c0633fda8e227b2ab3ab57e417c980a2811/memory/mozalloc/mozalloc_abort.cpp#79
which then calls MOZ_CRASH. Theoretically, /that/ would also print a
stack trace, but doesn't because currently the stack trace printing code
lives in libxul, and MOZ_CRASH only calls it when compiled from
libxul-code, which mozalloc_abort is not part of.
With this change, we make the rust panic handler call back into
MOZ_CRASH directly. This has multiple advantages:
- This is more consistent cross-platforms (Windows is not special
anymore).
- This is more consistent between C++ and rust (stack traces all look
the same, and can all be post-processed by fix*stack*py if need be)
- This is more consistent in behavior, where debug builds will show
those stack traces without caring about environment variables.
- It demangles C++ symbols in rust-initiated stack traces (for some
reason that didn't happen with the rust panic handler)
A few downsides:
- the loss of demangling for some rust symbols.
- the loss of addresses in the stacks, although they're not entirely
useful
- extra empty lines.
The first should be fixable later one. The latter two are arguably
something that should be consistent across C++ and rust, and should be
changed if necessary, independently of this patch.
Depends on D11719
Depends on D11719
Differential Revision: https://phabricator.services.mozilla.com/D11720
Ideally, we'd want the function to stay in Assertions.cpp, but that's
only part of MFBT proper, and that doesn't have access to WalkTheStack
like MOZ_CRASH has from being in Assertion.h, when included from Gecko
code. Moving WalkTheStack to mozglue, putting it close together with
MozStackWalk would be prefered, but that causes problems linking MFBT
tests (which don't have access to mozglue), and other things.
Overall, this was too deep a rabbit hole, and moving MOZ_CrashOOL to
Assertions.h is much simpler. Since it's essentially the same as
MOZ_CRASH, except it allows non-literal strings, we can make it inlined,
and leave it to the compiler to drop the filename argument when it's not
used.
Differential Revision: https://phabricator.services.mozilla.com/D11718
The current rust panic hook keeps a string for the crash reporter, and
goes on calling the default rust panic hook, which prints out a crash
stack... when RUST_BOOTSTRAP is set *and* when that works. Notably, on
both mac and Windows, it only really works for local builds, but fails
for debug builds from automation, although on automation itself, we also
do stackwalk from crash minidumps, which alleviates the problem.
Artifact debug builds are affected, though.
More importantly, C++ calls to e.g. MOZ_CRASH have a similar but
different behavior, in that they dump a stack trace on debug builds, by
default (with exceptions, see below for one). The format of those stack
traces is understood by the various fix*stack*py scripts under
tools/rb/, that are used by the various test harnesses both on
automation and locally.
Additionally, the current rust panic hook, as it calls the default rust
panic hook, ends up calling abort() on non-Windows platforms, which ends
up being verbosely redirected to mozalloc_abort per
https://dxr.mozilla.org/mozilla-central/rev/237e4c0633fda8e227b2ab3ab57e417c980a2811/memory/mozalloc/mozalloc_abort.cpp#79
which then calls MOZ_CRASH. Theoretically, /that/ would also print a
stack trace, but doesn't because currently the stack trace printing code
lives in libxul, and MOZ_CRASH only calls it when compiled from
libxul-code, which mozalloc_abort is not part of.
With this change, we make the rust panic handler call back into
MOZ_CRASH directly. This has multiple advantages:
- This is more consistent cross-platforms (Windows is not special
anymore).
- This is more consistent between C++ and rust (stack traces all look
the same, and can all be post-processed by fix*stack*py if need be)
- This is more consistent in behavior, where debug builds will show
those stack traces without caring about environment variables.
- It demangles C++ symbols in rust-initiated stack traces (for some
reason that didn't happen with the rust panic handler)
A few downsides:
- the loss of demangling for some rust symbols.
- the loss of addresses in the stacks, although they're not entirely
useful
- extra empty lines.
The first should be fixable later one. The latter two are arguably
something that should be consistent across C++ and rust, and should be
changed if necessary, independently of this patch.
Depends on D11719
Differential Revision: https://phabricator.services.mozilla.com/D11720
Ideally, we'd want the function to stay in Assertions.cpp, but that's
only part of MFBT proper, and that doesn't have access to WalkTheStack
like MOZ_CRASH has from being in Assertion.h, when included from Gecko
code. Moving WalkTheStack to mozglue, putting it close together with
MozStackWalk would be prefered, but that causes problems linking MFBT
tests (which don't have access to mozglue), and other things.
Overall, this was too deep a rabbit hole, and moving MOZ_CrashOOL to
Assertions.h is much simpler. Since it's essentially the same as
MOZ_CRASH, except it allows non-literal strings, we can make it inlined,
and leave it to the compiler to drop the filename argument when it's not
used.
Differential Revision: https://phabricator.services.mozilla.com/D11718
Ideally, we'd want the function to stay in Assertions.cpp, but that's
only part of MFBT proper, and that doesn't have access to WalkTheStack
like MOZ_CRASH has from being in Assertion.h, when included from Gecko
code. Moving WalkTheStack to mozglue, putting it close together with
MozStackWalk would be prefered, but that causes problems linking MFBT
tests (which don't have access to mozglue), and other things.
Overall, this was too deep a rabbit hole, and moving MOZ_CrashOOL to
Assertions.h is much simpler. Since it's essentially the same as
MOZ_CRASH, except it allows non-literal strings, we can make it inlined,
and leave it to the compiler to drop the filename argument when it's not
used.
Differential Revision: https://phabricator.services.mozilla.com/D11718
In Bug 1393538 I renamed MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE to MOZ_UNUSED_ATTRIBUTE,
moved it out of it's #define depth, and used it in toolkit. I also orphaned a
comment.
This was wrong. MOZ_UNUSED_ATTRIBUTE was basically identical to MOZ_MAYBE_UNUSED
which exists in Attributes.h (because it is an attribute, not an assertion.)
Undo that wrong thing: restore MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE to the correct
place, have toolkit use the correct macro, and remove MOZ_UNUSED_ATTRIBUTE.
MozReview-Commit-ID: 5BWWsXgbm9i
If MOZ_CRASH_UNSAFE_PRINTF is only given a format string, it means
either arguments are missing, or MOZ_CRASH should be used instead.
Hint at that with a static_assert.
At the same time, remove the MOZ_STATIC_ASSERT_VALID_ARG_COUNT, which
doesn't actually work for more than 50 arguments(*), and which is now not
useful to detect 0 arguments.
(*) the build fails, but not directly thanks to the static_assert it
expands to.
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.
MozReview-Commit-ID: J2FZmi0pKro
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.
MozReview-Commit-ID: Kuxzn1v9Vfs
I left gMozCrashReason visible (but not meaningfully used) in all builds, in order to match the behavior of Assertions.cpp, and to avoid more #ifdef clutter in nsExceptionHandler.cpp.
MozReview-Commit-ID: smoFkddGzd
The C versus C++ distinction was only there so that Android could make sure it used the global ::abort. I didn't see the need to maintain the distinction for Windows. (Besides, with this change we're no longer doing textual inclusion of "TerminateProcess" in the macro, so people can't take over the name.)
Linux's abort sequence wasn't long enough to be troublesome, so I left it alone.
MozReview-Commit-ID: Ah5XtWpevGz
