From a8d6cc40727783b24ef40bc02860f8beeb518748 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 27 Jan 2023 07:40:17 -0800 Subject: [PATCH] Move nmspinning address lookup to dynamic facts render This removes the need to check the offset every release. I've also removed the negative build tag from runtime_amd64.go, which less obviously correct. In theory, we should check that this package's use of nmspinning is still valid in each release, but there is no way to automate that and I don't realistically seeing checking happening beyond verifying tests work. Additionally, the existing use is already suspect. :) The good news is the misuse is likely to cause scheduling issues, not memory corruption. PiperOrigin-RevId: 505114683 --- pkg/sync/BUILD | 27 +++++++++++++++++-- pkg/sync/runtime.go | 22 +++++++++++++++ pkg/sync/runtime_amd64.go | 3 +-- ...ntime_amd64.s => runtime_spinning_amd64.s} | 8 +++--- pkg/sync/runtime_spinning_other.s | 18 +++++++++++++ 5 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 pkg/sync/runtime.go rename pkg/sync/{runtime_amd64.s => runtime_spinning_amd64.s} (76%) create mode 100644 pkg/sync/runtime_spinning_other.s diff --git a/pkg/sync/BUILD b/pkg/sync/BUILD index 78d50f8d9..ead9fc6ae 100644 --- a/pkg/sync/BUILD +++ b/pkg/sync/BUILD @@ -1,10 +1,33 @@ -load("//tools:defs.bzl", "go_library", "go_test") +load("//tools:defs.bzl", "arch_genrule", "go_library", "go_test", "select_arch") +load("//tools/nogo:defs.bzl", "nogo_facts") package( default_visibility = ["//:sandbox"], licenses = ["notice"], ) +nogo_facts( + name = "runtime_spinning_impl", + srcs = ["runtime.go"], + output = "runtime_spinning_impl.s", + template = select_arch( + amd64 = "runtime_spinning_amd64.s", + arm64 = "runtime_spinning_other.s", + ), +) + +# For arm64 (or any !amd64), this will generate runtime_spinning_impl_arm64.s, +# which is a copy of the (empty) runtime_spinning_other.s. +# +# On the go branch, only amd64 and arm64 will have have files, other arches +# won't select any of these files. That is fine because the contents only +# matter for amd64 anyway. +arch_genrule( + name = "runtime_spinning_impl_arch", + src = ":runtime_spinning_impl", + template = "runtime_spinning_impl_%s.s", +) + go_library( name = "sync", srcs = [ @@ -21,12 +44,12 @@ go_library( "race_arm64.s", "race_unsafe.go", "runtime_amd64.go", - "runtime_amd64.s", "runtime_other.go", "runtime_unsafe.go", "rwmutex_unsafe.go", "seqcount.go", "sync.go", + ":runtime_spinning_impl_arch", ], marshal = False, stateify = False, diff --git a/pkg/sync/runtime.go b/pkg/sync/runtime.go new file mode 100644 index 000000000..e4604e83e --- /dev/null +++ b/pkg/sync/runtime.go @@ -0,0 +1,22 @@ +// Copyright 2023 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. + +package sync + +import ( + "runtime" +) + +// Dummy reference for facts. +const _ = runtime.Compiler diff --git a/pkg/sync/runtime_amd64.go b/pkg/sync/runtime_amd64.go index a9d883e4c..dad10bfef 100644 --- a/pkg/sync/runtime_amd64.go +++ b/pkg/sync/runtime_amd64.go @@ -3,8 +3,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build amd64 && go1.8 && !go1.21 && !goexperiment.staticlockranking -// +build amd64,go1.8,!go1.21,!goexperiment.staticlockranking +//go:build amd64 package sync diff --git a/pkg/sync/runtime_amd64.s b/pkg/sync/runtime_spinning_amd64.s similarity index 76% rename from pkg/sync/runtime_amd64.s rename to pkg/sync/runtime_spinning_amd64.s index 252dda1bb..545f9dae6 100644 --- a/pkg/sync/runtime_amd64.s +++ b/pkg/sync/runtime_spinning_amd64.s @@ -12,14 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -//go:build amd64 && go1.14 && !go1.21 && !goexperiment.staticlockranking -// +build amd64,go1.14,!go1.21,!goexperiment.staticlockranking +//go:build amd64 #include "textflag.h" +#define NMSPINNING_OFFSET {{ .import.runtime.schedt.nmspinning.Offset }} + TEXT ·addrOfSpinning(SB),NOSPLIT,$0-8 - // The offset specified here is the nmspinning value in sched. LEAQ runtime·sched(SB), AX - ADDQ $92, AX + ADDQ $NMSPINNING_OFFSET, AX MOVQ AX, ret+0(FP) RET diff --git a/pkg/sync/runtime_spinning_other.s b/pkg/sync/runtime_spinning_other.s new file mode 100644 index 000000000..85501e54c --- /dev/null +++ b/pkg/sync/runtime_spinning_other.s @@ -0,0 +1,18 @@ +// Copyright 2023 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 !amd64 + +// This file is intentionally left blank. Other arches don't use +// addrOfSpinning, but we still need an input to the nogo temlate rule.