A long standing issue is that MOZ_ASSERT and related don't print stack
traces in debug builds when they're directly or indirectly emitted from
non-libxul code. Moving WalkTheStack to mozglue alleviates the problem.
It's also not printing stack traces when emitted from C code (and for
some C third party libraries, we do redirect assert to MOZ_ASSERT),
which we solve by making the corresponding API available without C++
(which WalkTheStack being a static method of the nsTraceRefCnt class
didn't allow, or the use of a closure on Android).
This requires some adjustements to headers that indirectly assume that
Assertions.h includes ErrorList.h through nsError.h through nscore.h
through nsTraceRefcnt.h.
We also remove TestStackCrawl.cpp because it hasn't been built since
bug 158528, 19 years ago.
Differential Revision: https://phabricator.services.mozilla.com/D108913
Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's __attribute__((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.
Depends on D108344
Differential Revision: https://phabricator.services.mozilla.com/D108345
MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(), so MOZ_DIAGNOSTIC_ALWAYS_TRUE() will be redundant.
Differential Revision: https://phabricator.services.mozilla.com/D66922
--HG--
extra : moz-landing-system : lando
MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT() so it will also assert in Nightly and DevEdition release builds.
Differential Revision: https://phabricator.services.mozilla.com/D66921
--HG--
extra : moz-landing-system : lando
MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(), so MOZ_DIAGNOSTIC_ALWAYS_TRUE() will be redundant.
Differential Revision: https://phabricator.services.mozilla.com/D66922
--HG--
extra : moz-landing-system : lando
MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT() so it will also assert in Nightly and DevEdition release builds.
Differential Revision: https://phabricator.services.mozilla.com/D66921
--HG--
extra : moz-landing-system : lando
MOZ_ASSERT(false) will evaluate to a no-op in release builds, so there should be no overhead from using MOZ_ASSERT(false) to define MOZ_ALWAYS_TRUE/FALSE/OK/ERR in both debug and release.
Differential Revision: https://phabricator.services.mozilla.com/D66797
--HG--
extra : moz-landing-system : lando
This changeset is a simple find and replace of `MOZ_FALLTHROUGH` and `[[fallthrough]]`.
Unfortunately, the MOZ_FALLTHROUGH_ASSERT macro (to assert on case fallthrough in debug builds) is still necessary after switching from [[clang::fallthrough]] to [[fallthrough]] because:
* MOZ_ASSERT(false) followed by [[fallthrough]] triggers a -Wunreachable-code warning in DEBUG builds
* but MOZ_ASSERT(false) without [[fallthrough]] triggers a -Wimplicit-fallthrough warning in NDEBUG builds.
Differential Revision: https://phabricator.services.mozilla.com/D56440
--HG--
extra : moz-landing-system : lando
Defines an android-only version of nsTraceRefcnt::WalkTheStack that takes a function callback, which outputs the stack frame buffer to `__android_log_print`. Also uses `__wrap_dladdr` in MozDescribeCodeAddress, which outputs slightly more informative data for the stack trace (instead of instances of '???/??? [???]').
Differential Revision: https://phabricator.services.mozilla.com/D46868
--HG--
extra : moz-landing-system : lando
Defines an android-only version of nsTraceRefcnt::WalkTheStack that takes a function callback, which outputs the stack frame buffer to `__android_log_print`. Also uses `__wrap_dladdr` in MozDescribeCodeAddress, which outputs slightly more informative data for the stack trace (instead of instances of '???/??? [???]').
Differential Revision: https://phabricator.services.mozilla.com/D46868
--HG--
extra : moz-landing-system : lando
We want MOZ_Crash frames to stay out of Taskcluster logs even on debug builds. Perhaps you could say, _especially_ on debug builds.
Differential Revision: https://phabricator.services.mozilla.com/D26352
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : moz-landing-system : lando
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
--HG--
extra : rebase_source : d07156068c877bf57d400bc6a71e115b7f1aef31
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.
--HG--
extra : rebase_source : 355c37deb8b007e61939a4c657e411d110e7bbe7
In a couple places, MOZ_PASTE_PREFIX_AND_ARG_COUNT is used to only count
the number of arguments, we can now use MOZ_ARG_COUNT directly for that.
--HG--
extra : rebase_source : 1064e4cc231863dc4aff83ee6bc90d318b4be418
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.
--HG--
extra : rebase_source : 8f0fe7b352c89b5a3ec87f42ef5464c370c362ef
Mike Hommey
Chris Peterson
Sebastian Hengst
André Bargull
kriswright
Bogdan Tara
Nika Layzell
Andi-Bogdan Postelnicu
shindli
David Major
Jan Varga
Daniel Varga
Ehsan Akhgari
Sylvestre Ledru
Dorel Luca
Coroiu Cristina
twsmith
Tiberius Oros
Tom Ritter
Benjamin Smedberg