Skip to content

Commit

Permalink
log: fix call depth for go1.12 panics
Browse files Browse the repository at this point in the history
This PR fixes the following test failure on Linux:

```
$ make test IGNORE_GOVERS=1 PKG=./pkg/util/log
--- FAIL: TestCrashReportingPacket (0.20s)
    --- FAIL: TestCrashReportingPacket/#00 (0.00s)
        crash_reporting_packet_test.go:155: expected crash_reporting_packet_test.go:85: boom, got testing.go:865: boom
    --- FAIL: TestCrashReportingPacket/#1 (0.00s)
        crash_reporting_packet_test.go:155: expected crash_reporting_packet_test.go:93: baam, got testing.go:865: baam
FAIL
```

Interestingly this is not quite the same failure noted in #35792, maybe that's
MacOS specific?

Release note: None
  • Loading branch information
ajwerner committed Mar 26, 2019
1 parent 9fff28a commit 84bd549
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 14 deletions.
29 changes: 29 additions & 0 deletions pkg/util/log/crash_report_panic_depth_1_11.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2019 The Cockroach Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
// implied. See the License for the specific language governing
// permissions and limitations under the License.

// +build !go1.12

// This file exists to deal with the fact that the call depth during panics
// differs before and after go version 1.12. It holds the correct depth for use
// with go1.11 and prior and carries the appropriate build constraint.

package log

// The call stack here is usually:
// - ReportPanic
// - RecoverAndReport
// - panic.go
// - panic()
// so ReportPanic should pop four frames.
const depthForRecoverAndReportPanic = 4
28 changes: 28 additions & 0 deletions pkg/util/log/crash_report_panic_depth_1_12.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright 2019 The Cockroach Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
// implied. See the License for the specific language governing
// permissions and limitations under the License.

// +build go1.12

// This file exists to deal with the fact that the call depth during panics
// differs before and after go version 1.12. It holds the correct depth for use
// with go1.12 and later and carries the appropriate build constraint.

package log

// The call stack here is usually:
// - ReportPanic
// - RecoverAndReport
// - panic()
// so ReportPanic should pop three frames.
const depthForRecoverAndReportPanic = 3
16 changes: 2 additions & 14 deletions pkg/util/log/crash_reporting.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,7 @@ var (
// real stderr a panic has occurred.
func RecoverAndReportPanic(ctx context.Context, sv *settings.Values) {
if r := recover(); r != nil {
// The call stack here is usually:
// - ReportPanic
// - RecoverAndReport
// - panic.go
// - panic()
// so ReportPanic should pop four frames.
ReportPanic(ctx, sv, r, 4)
ReportPanic(ctx, sv, r, depthForRecoverAndReportPanic)
panic(r)
}
}
Expand All @@ -106,13 +100,7 @@ func RecoverAndReportPanic(ctx context.Context, sv *settings.Values) {
// does not re-panic in Release builds.
func RecoverAndReportNonfatalPanic(ctx context.Context, sv *settings.Values) {
if r := recover(); r != nil {
// The call stack here is usually:
// - ReportPanic
// - RecoverAndReport
// - panic.go
// - panic()
// so ReportPanic should pop four frames.
ReportPanic(ctx, sv, r, 4)
ReportPanic(ctx, sv, r, depthForRecoverAndReportPanic)
if !build.IsRelease() || PanicOnAssertions.Get(sv) {
panic(r)
}
Expand Down

0 comments on commit 84bd549

Please sign in to comment.