Skip to content

Commit

Permalink
Revert "Remove NOTREACHED() dumping for official builds"
Browse files Browse the repository at this point in the history
This reverts commit a1b3c71.

Reason for revert: Perf regression, will try a smaller revert.

Original change's description:
> Remove NOTREACHED() dumping for official builds
>
> This is to be merged to M111. A follow-up change will enable
> `enable_log_error_not_reached` that will allow us to dump again (with
> more data), and we can then decide if that should follow M112 all the
> way to stable before we make this fatal, or if we need to strip the
> useful debug info from reporting before we hit stable.
>
> Bug: 851128
> Change-Id: Iec9bfe61ec8988d6e6017f7a64ec0401d0f541d1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200079
> Auto-Submit: Peter Boström <pbos@chromium.org>
> Commit-Queue: danakj <danakj@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1098455}

Bug: 851128, 1411319
Change-Id: I17ba4e8d3d30bd8098a228cfb030223d8cadbedb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4205307
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098802}
  • Loading branch information
pbos authored and Chromium LUCI CQ committed Jan 30, 2023
1 parent b9d1f59 commit 6da0aba
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 8 deletions.
6 changes: 6 additions & 0 deletions base/check.cc
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,12 @@ NotReachedError NotReachedError::NotReached(const char* file, int line) {
return NotReachedError(log_message);
}

void NotReachedError::TriggerNotReached() {
// TODO(pbos): Add back NotReachedError("", -1) here asap. This was removed to
// disable NOTREACHED() reports temporarily for M111 and should be added
// back once this change has merged to M111.
}

NotReachedError::~NotReachedError() = default;

void RawCheck(const char* message) {
Expand Down
4 changes: 4 additions & 0 deletions base/check.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ class BASE_EXPORT NotReachedError : public CheckError {
public:
static NotReachedError NotReached(const char* file, int line);

// Used to trigger a NOTREACHED() without providing file or line while also
// discarding log-stream arguments. See base/notreached.h.
NOMERGE NOINLINE NOT_TAIL_CALLED static void TriggerNotReached();

// TODO(crbug.com/851128): Mark [[noreturn]] once this is CHECK-fatal on all
// builds.
NOMERGE NOINLINE NOT_TAIL_CALLED ~NotReachedError();
Expand Down
3 changes: 3 additions & 0 deletions base/check_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,9 @@ TEST(CheckDeathTest, NotReached) {
// Expect LOG(ERROR) that looks like CHECK(false) with streamed params intact.
EXPECT_LOG_ERROR(__LINE__, NOTREACHED() << "foo",
"Check failed: false. foo\n");
#else
// Expect nothing.
EXPECT_NO_LOG(NOTREACHED() << "foo");
#endif
}

Expand Down
14 changes: 6 additions & 8 deletions base/notreached.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,19 @@ namespace logging {
// non-FATAL a crash report will be generated for the first NOTREACHED() that
// hits per process.
//
// If `enable_log_error_not_reached` is enabled in the build then NOTREACHED()
// builds will LOG(ERROR) and also upload a crash report without crashing in
// order to weed out prevalent NOTREACHED()s in the wild before always turning
// NOTREACHED()s FATAL.
// Outside DCHECK builds NOTREACHED() will LOG(ERROR) and also upload a crash
// report without crashing in order to weed out prevalent NOTREACHED()s in the
// wild before always turning NOTREACHED()s FATAL.
//
// Without either of the above this currently does nothing. In the future (TODO
// below) we should replace the #else branch here with base::ImmediateCrash() or
// equivalent (and make ~NotReachedError()) FATAL and [[noreturn]].
// TODO(crbug.com/851128): Turn NOTREACHED() FATAL and mark them [[noreturn]].
#if CHECK_WILL_STREAM() || BUILDFLAG(ENABLE_LOG_ERROR_NOT_REACHED)
#define NOTREACHED() \
CHECK_FUNCTION_IMPL( \
::logging::NotReachedError::NotReached(__FILE__, __LINE__), false)
#else
#define NOTREACHED() EAT_CHECK_STREAM_PARAMS()
#define NOTREACHED() \
(true) ? ::logging::NotReachedError::TriggerNotReached() \
: EAT_CHECK_STREAM_PARAMS()
#endif

// The NOTIMPLEMENTED() macro annotates codepaths which have not been
Expand Down

0 comments on commit 6da0aba

Please sign in to comment.