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