From a625ba1d0ae6edac8a8e1edc9dc442d6f0f7b49f Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 1 Mar 2023 18:30:16 -0800 Subject: [PATCH] Use build tags to conditionally enable vfs.checkInvariants. We don't want these checks in the production binary at all. Even though the hardware branch predictor will do away with any runtime costs of the if condition, the code under the "if checkInvariants {...}" could force escapes to the heap. It could also increase function complexity, making it no longer inlinable. And these invariant checks lie on hot code paths. PiperOrigin-RevId: 513398208 --- pkg/sentry/vfs/BUILD | 1 + pkg/sentry/vfs/debug.go | 7 +++++-- pkg/sentry/vfs/debug_testonly.go | 23 +++++++++++++++++++++++ tools/bazeldefs/tags.bzl | 5 +++-- 4 files changed, 32 insertions(+), 4 deletions(-) create mode 100644 pkg/sentry/vfs/debug_testonly.go diff --git a/pkg/sentry/vfs/BUILD b/pkg/sentry/vfs/BUILD index e17e9af41..4aa86996a 100644 --- a/pkg/sentry/vfs/BUILD +++ b/pkg/sentry/vfs/BUILD @@ -115,6 +115,7 @@ go_library( "anonfs.go", "context.go", "debug.go", + "debug_testonly.go", "dentry.go", "device.go", "epoll.go", diff --git a/pkg/sentry/vfs/debug.go b/pkg/sentry/vfs/debug.go index 0ed20f249..53cac1fd9 100644 --- a/pkg/sentry/vfs/debug.go +++ b/pkg/sentry/vfs/debug.go @@ -12,11 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. +//go:build !check_invariants +// +build !check_invariants + package vfs const ( // If checkInvariants is true, perform runtime checks for invariants - // expected by the vfs package. This is normally disabled since VFS is - // often a hot path. + // expected by the vfs package. This is disabled for non-test binaries since + // VFS is often a hot path. checkInvariants = false ) diff --git a/pkg/sentry/vfs/debug_testonly.go b/pkg/sentry/vfs/debug_testonly.go new file mode 100644 index 000000000..6c6158b43 --- /dev/null +++ b/pkg/sentry/vfs/debug_testonly.go @@ -0,0 +1,23 @@ +// Copyright 2019 The gVisor 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. + +//go:build check_invariants +// +build check_invariants + +package vfs + +const ( + // Set checkInvariants to true for tests. + checkInvariants = true +) diff --git a/tools/bazeldefs/tags.bzl b/tools/bazeldefs/tags.bzl index 6564c3b25..80dfc6f2c 100644 --- a/tools/bazeldefs/tags.bzl +++ b/tools/bazeldefs/tags.bzl @@ -49,12 +49,13 @@ generic = [ "_norace", "_unsafe", "_opts", + "_testonly", ] # State explosion? Sure. This is approximately: -# len(archs) * (1 + 2 * len(oses) * (1 + 2 * len(generic)) +# len(archs) * (1 + 2 * len(oses)) * (1 + 2 * len(generic)) # -# This evaluates to 495 at the time of writing. So it's a lot of different +# This evaluates to 663 at the time of writing. So it's a lot of different # combinations, but not so much that it will cause issues. We can probably add # quite a few more variants before this becomes a genuine problem. go_suffixes = explode(explode(archs, oses), generic)