From f3e4a1fc3b4930c72a6316611931ae277e96ecca Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 21 Jul 2023 11:19:06 -0700 Subject: [PATCH] Remove last remaining !go1.22 build tag The last remaining !go1.22 build is protecting the definition of pkg/sync.maptype, which is a copy of runtime.maptype. We need to ensure these definitions match so we can safely access the hasher field. At its core, this CL achieves this check by ensuring that unsafe.Offsetof(maptype{}.Hasher) matches the offset in the runtime version of the type. Several things happen along the way to achieve this: * As of May 2023, runtime.maptype is actually a type alias for internal/abi.MapType. checkoffset was failing to record the offsets because it skipped type aliases for no good reason. Simply removing the type alias check is sufficient to make type aliases work. (This part of the CL is technically unnecessary because this CL ultimately references internal/abi.MapType directly in anticipation of removal of the type alias. But there is no reason not to allow type aliases). * The checkconst / checkoffset regexp unintentionally does not allow / in package paths, even though the rest of the package supports /. Fix this. * checkconst was comparing the literal AST expression string against the runtime value (i.e., "unsafe.Offsetof(maptype{}.Hasher)" vs "72", which fails comparison. Switch to getting the resolved constant value from the type checker. * nogo/check.importer only loads package facts on direct import (stored in importer.cache). If a package is not directly imported ImportPackageFact will not find the facts. Typically packages need to ensure they directly depend on packages they want facts from (e.g., pkg/sync has a dummy import of runtime in runtime.go). This doesn't work for internal/abi because we cannot directly import an internal package. Work around this as a hack by unconditionally "importing" internal/abi when analyzing any package. With regard to the last point, not that the nogo/defs.bzl nogo integration only provides facts from the direct dependencies and the entire stdlib (since the stdlib is analyzed as one bundle). So this trick only works for a stdlib package. A bazel package indirect dependency would be missing facts altogether. PiperOrigin-RevId: 549999084 --- pkg/sync/BUILD | 2 + pkg/sync/runtime_go121_unsafe.go | 16 ++++++++ pkg/sync/runtime_not_go121_unsafe.go | 18 ++++++++ pkg/sync/runtime_unsafe.go | 18 +++----- tools/checkconst/checkconst.go | 61 +++++++++++++++++++++------- tools/nogo/check/check.go | 21 ++++++++++ 6 files changed, 109 insertions(+), 27 deletions(-) create mode 100644 pkg/sync/runtime_go121_unsafe.go create mode 100644 pkg/sync/runtime_not_go121_unsafe.go diff --git a/pkg/sync/BUILD b/pkg/sync/BUILD index c6d01d220..ec6e4c56b 100644 --- a/pkg/sync/BUILD +++ b/pkg/sync/BUILD @@ -24,6 +24,8 @@ go_library( "runtime.go", "runtime_amd64.go", "runtime_constants.go", + "runtime_go121_unsafe.go", + "runtime_not_go121_unsafe.go", "runtime_other.go", "runtime_spinning_amd64.s", "runtime_spinning_other.s", diff --git a/pkg/sync/runtime_go121_unsafe.go b/pkg/sync/runtime_go121_unsafe.go new file mode 100644 index 000000000..344b55663 --- /dev/null +++ b/pkg/sync/runtime_go121_unsafe.go @@ -0,0 +1,16 @@ +// Copyright 2023 The gVisor Authors. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.21 + +package sync + +import ( + "unsafe" +) + +// Use checkoffset to assert that maptype.hasher (the only field we use) has +// the correct offset. +const maptypeHasherOffset = unsafe.Offsetof(maptype{}.Hasher) // +checkoffset internal/abi MapType.Hasher diff --git a/pkg/sync/runtime_not_go121_unsafe.go b/pkg/sync/runtime_not_go121_unsafe.go new file mode 100644 index 000000000..4d7e8b9fb --- /dev/null +++ b/pkg/sync/runtime_not_go121_unsafe.go @@ -0,0 +1,18 @@ +// Copyright 2023 The gVisor Authors. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// runtime.maptype is moved to internal/abi.MapType in Go 1.21. +// +//go:build !go1.21 + +package sync + +import ( + "unsafe" +) + +// Use checkoffset to assert that maptype.hasher (the only field we use) has +// the correct offset. +const maptypeHasherOffset = unsafe.Offsetof(maptype{}.Hasher) // +checkoffset runtime maptype.hasher diff --git a/pkg/sync/runtime_unsafe.go b/pkg/sync/runtime_unsafe.go index 91cda67bb..a298bddbc 100644 --- a/pkg/sync/runtime_unsafe.go +++ b/pkg/sync/runtime_unsafe.go @@ -3,16 +3,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build go1.18 && !go1.22 -// +build go1.18,!go1.22 - -// //go:linkname directives type-checked by checklinkname. Any other -// non-linkname assumptions outside the Go 1 compatibility guarantee should -// have an accompanied vet check or version guard build tag. - -// Check type definitions and constants when updating Go version. -// -// TODO(b/165820485): add these checks to checklinkname. +// //go:linkname directives type-checked by checklinkname. +// Runtime type copies checked by checkoffset. package sync @@ -107,10 +99,10 @@ func MapKeyHasher(m any) func(unsafe.Pointer, uintptr) uintptr { panic(fmt.Sprintf("sync.MapKeyHasher: m is %v, not map", rtyp)) } mtyp := *(**maptype)(unsafe.Pointer(&m)) - return mtyp.hasher + return mtyp.Hasher } -// maptype is equivalent to the beginning of runtime.maptype. +// maptype is equivalent to the beginning of internal/abi.MapType. type maptype struct { size uintptr ptrdata uintptr @@ -126,7 +118,7 @@ type maptype struct { key unsafe.Pointer elem unsafe.Pointer bucket unsafe.Pointer - hasher func(unsafe.Pointer, uintptr) uintptr + Hasher func(unsafe.Pointer, uintptr) uintptr // more fields } diff --git a/tools/checkconst/checkconst.go b/tools/checkconst/checkconst.go index fe526da94..70c5a953d 100644 --- a/tools/checkconst/checkconst.go +++ b/tools/checkconst/checkconst.go @@ -21,10 +21,8 @@ package checkconst import ( - "bytes" "fmt" "go/ast" - "go/format" "go/token" "go/types" "io/ioutil" @@ -38,8 +36,8 @@ import ( var ( checkconstMagic = "\\+check(const|align|offset|size)" checkconstRegexp = regexp.MustCompile(checkconstMagic) - constRegexp = regexp.MustCompile("//\\s+" + checkconstMagic + "\\s+([A-Za-z0-9_\\.]+)\\s+([A-Za-z0-9_\\.]+)") - defineRegexp = regexp.MustCompile("#define\\s+[A-Za-z0-9_]+\\s+([A-Za-z0-9_]+\\s*\\+\\s*)*([x0-9]+)\\s+//\\s+" + checkconstMagic + "\\s+([A-Za-z0-9_\\.]+)\\s+([A-Za-z0-9_\\.]+)") + constRegexp = regexp.MustCompile("//\\s+" + checkconstMagic + "\\s+([A-Za-z0-9_\\./]+)\\s+([A-Za-z0-9_\\.]+)") + defineRegexp = regexp.MustCompile("#define\\s+[A-Za-z0-9_]+\\s+([A-Za-z0-9_]+\\s*\\+\\s*)*([x0-9]+)\\s+//\\s+" + checkconstMagic + "\\s+([A-Za-z0-9_\\./]+)\\s+([A-Za-z0-9_\\.]+)") ) // Analyzer defines the entrypoint. @@ -83,7 +81,7 @@ func (c *Constants) walkObject(pass *analysis.Pass, parents []string, obj types. // type parameter. If it is not an alias, then it must be // package-local. typ := x.Type() - if x.IsAlias() || typ == nil || typ.Underlying() == nil { + if typ == nil || typ.Underlying() == nil { break } if _, ok := typ.(*types.TypeParam); ok { @@ -151,19 +149,57 @@ func findPackage(pkg *types.Package, pkgName string) (*types.Package, error) { if pkgName == "." || pkgName == "" { return pkg, nil } + // Attempt to resolve with the full path. for _, importedPkg := range pkg.Imports() { if importedPkg.Path() == pkgName { return importedPkg, nil } } + // Attempt to resolve using the short name. for _, importedPkg := range pkg.Imports() { if importedPkg.Name() == pkgName { return importedPkg, nil } } - return nil, fmt.Errorf("unable to locate package %q", pkgName) + + // Attempt to resolve with the full path from transitive dependencies. + // + // This is needed for referencing internal/ packages which we cannot + // directly import, but can be reached indirectly (e.g., internal/abi + // is reachable from runtime). + // + // N.B. nogo/check.importer only loads facts on direct import, so + // ImportPackageFact may fail without an explicit import. See hack in + // nogo/check.Package. + visited := map[*types.Package]struct{}{} + var visit func(pkg *types.Package) *types.Package + visit = func(pkg *types.Package) *types.Package { + if _, ok := visited[pkg]; ok { + return nil + } + visited[pkg] = struct{}{} + + if pkg.Path() == pkgName { + return pkg + } + + for _, importedPkg := range pkg.Imports() { + if found := visit(importedPkg); found != nil { + return found + } + } + + return nil + } + for _, importedPkg := range pkg.Imports() { + if found := visit(importedPkg); found != nil { + return found, nil + } + } + + return nil, fmt.Errorf("unable to locate package %q (saw %v)", pkgName, visited) } // matchRegexp performs a regexp match with a sanity check. @@ -320,14 +356,11 @@ func checkConsts(pass *analysis.Pass) error { continue // Nothing was set. } // Format the expression. - var buf bytes.Buffer - for _, value := range vs.Values { - if err := format.Node(&buf, pass.Fset, value); err != nil { - pass.Reportf(value.Pos(), "unable to format expression: %v", err) - continue - } - if s := string(buf.Bytes()); s != expectedValue { - pass.Reportf(value.Pos(), "got value %q, wanted %q", s, expectedValue) + for _, valueExpr := range vs.Values { + val := pass.TypesInfo.Types[valueExpr].Value + s := fmt.Sprint(val) + if s != expectedValue { + pass.Reportf(valueExpr.Pos(), "got value %q, wanted %q", s, expectedValue) continue } } diff --git a/tools/nogo/check/check.go b/tools/nogo/check/check.go index b0c908dfe..a5a115c40 100644 --- a/tools/nogo/check/check.go +++ b/tools/nogo/check/check.go @@ -58,6 +58,19 @@ var ( releaseTagsErr error ) +// Hack! factFacts only provides facts loaded from directly imported packages +// for efficiency (see importer.cache). In general, if you need a fact from a +// package that isn't otherwise imported, the expectation is that you will add +// a dummy import/use of the desired package to ensure it is a dependency. +// +// Unfortunately, some packages need facts from internal packages. Since +// internal packages cannot be imported we explicitly import in this tool to +// ensure the facts are available to ImportPackageFact. +var internalPackages = []string{ + // Required by pkg/sync for internal/abi.MapType. + "internal/abi", +} + // shouldInclude indicates whether the file should be included. func shouldInclude(path string) (bool, error) { tagsOnce.Do(func() { @@ -635,6 +648,14 @@ func Package(path string, srcs []string) (FindingSet, facts.Serializer, error) { cache: make(map[string]*importerEntry), imports: make(map[string]*types.Package), } + + // See comment on internalPackages. + for _, pkg := range internalPackages { + if _, err := i.Import(pkg); err != nil { + return nil, nil, fmt.Errorf("error importing %s: %w", pkg, err) + } + } + _, findings, facts, err := i.checkPackage(path, srcs) if err != nil { return nil, nil, err