diff options
Diffstat (limited to 'tools')
73 files changed, 3504 insertions, 2244 deletions
diff --git a/tools/BUILD b/tools/BUILD index da83877b1..faf310676 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -5,5 +5,7 @@ package(licenses = ["notice"]) bzl_library( name = "defs_bzl", srcs = ["defs.bzl"], - visibility = ["//visibility:private"], + visibility = [ + "//:sandbox", + ], ) diff --git a/tools/bazel.mk b/tools/bazel.mk index 3e27af7d1..88431ce66 100644 --- a/tools/bazel.mk +++ b/tools/bazel.mk @@ -14,11 +14,16 @@ # See the License for the specific language governing permissions and # limitations under the License. +# Make hacks. +EMPTY := +SPACE := $(EMPTY) $(EMPTY) + # See base Makefile. SHELL=/bin/bash -o pipefail BRANCH_NAME := $(shell (git branch --show-current 2>/dev/null || \ git rev-parse --abbrev-ref HEAD 2>/dev/null) | \ xargs -n 1 basename 2>/dev/null) +BUILD_ROOTS := bazel-bin/ bazel-out/ # Bazel container configuration (see below). USER ?= gvisor @@ -31,6 +36,7 @@ DOCKER_PRIVILEGED ?= --privileged BAZEL_CACHE := $(shell readlink -m ~/.cache/bazel/) GCLOUD_CONFIG := $(shell readlink -m ~/.config/gcloud/) DOCKER_SOCKET := /var/run/docker.sock +DOCKER_CONFIG := /etc/docker/daemon.json # Bazel flags. BAZEL := bazel $(STARTUP_OPTIONS) @@ -56,6 +62,9 @@ endif # Add docker passthrough options. ifneq ($(DOCKER_PRIVILEGED),) FULL_DOCKER_RUN_OPTIONS += -v "$(DOCKER_SOCKET):$(DOCKER_SOCKET)" +# TODO(gvisor.dev/issue/1624): Remove docker config volume. This is required +# temporarily for checking VFS1 vs VFS2 by some tests. +FULL_DOCKER_RUN_OPTIONS += -v "$(DOCKER_CONFIG):$(DOCKER_CONFIG)" FULL_DOCKER_RUN_OPTIONS += $(DOCKER_PRIVILEGED) FULL_DOCKER_EXEC_OPTIONS += $(DOCKER_PRIVILEGED) DOCKER_GROUP := $(shell stat -c '%g' $(DOCKER_SOCKET)) @@ -127,7 +136,7 @@ bazel-server-start: bazel-image ## Starts the bazel server. --workdir "$(CURDIR)" \ $(FULL_DOCKER_RUN_OPTIONS) \ $(BUILDER_IMAGE) \ - sh -c "tail -f --pid=\$$($(BAZEL) info server_pid)" + sh -c "tail -f --pid=\$$($(BAZEL) info server_pid) /dev/null" .PHONY: bazel-server-start bazel-shutdown: ## Shuts down a running bazel server. @@ -147,9 +156,12 @@ build_cmd = docker exec $(FULL_DOCKER_EXEC_OPTIONS) $(DOCKER_NAME) sh -o pipefai build_paths = $(build_cmd) 2>&1 \ | tee /proc/self/fd/2 \ - | grep -E "^ bazel-bin/" \ - | tr -d '\r' \ + | grep -A1 -E '^Target' \ + | grep -E '^ ($(subst $(SPACE),|,$(BUILD_ROOTS)))' \ + | sed "s/ /\n/g" \ + | strings -n 10 \ | awk '{$$1=$$1};1' \ + | xargs -n 1 -I {} readlink -f "{}" \ | xargs -n 1 -I {} sh -c "$(1)" build: bazel-server diff --git a/tools/bazeldefs/BUILD b/tools/bazeldefs/BUILD index 8d4356119..d043caf06 100644 --- a/tools/bazeldefs/BUILD +++ b/tools/bazeldefs/BUILD @@ -26,43 +26,6 @@ rbe_platform( remote_execution_properties = """ properties: { name: "container-image" - value:"docker://gcr.io/cloud-marketplace/google/rbe-ubuntu16-04@sha256:93f7e127196b9b653d39830c50f8b05d49ef6fd8739a9b5b8ab16e1df5399e50" - } - properties: { - name: "dockerAddCapabilities" - value: "SYS_ADMIN" - } - properties: { - name: "dockerPrivileged" - value: "true" - } - """, -) - -rbe_toolchain( - name = "cc-toolchain-clang-x86_64-default", - exec_compatible_with = [], - tags = [ - "manual", - ], - target_compatible_with = [], - toolchain = "@bazel_toolchains//configs/ubuntu16_04_clang/10.0.0/bazel_2.0.0/cc:cc-compiler-k8", - toolchain_type = "@bazel_tools//tools/cpp:toolchain_type", -) - -# Updated versions of the above, compatible with bazel3. -rbe_platform( - name = "rbe_ubuntu1604_bazel3", - constraint_values = [ - "@bazel_tools//platforms:x86_64", - "@bazel_tools//platforms:linux", - "@bazel_tools//tools/cpp:clang", - "@bazel_toolchains_bazel3//constraints:xenial", - "@bazel_toolchains_bazel3//constraints/sanitizers:support_msan", - ], - remote_execution_properties = """ - properties: { - name: "container-image" value:"docker://gcr.io/cloud-marketplace/google/rbe-ubuntu16-04@sha256:b516a2d69537cb40a7c6a7d92d0008abb29fba8725243772bdaf2c83f1be2272" } properties: { @@ -77,13 +40,13 @@ rbe_platform( ) rbe_toolchain( - name = "cc-toolchain-clang-x86_64-default_bazel3", + name = "cc-toolchain-clang-x86_64-default", exec_compatible_with = [], tags = [ "manual", ], target_compatible_with = [], - toolchain = "@bazel_toolchains_bazel3//configs/ubuntu16_04_clang/11.0.0/bazel_3.1.0/cc:cc-compiler-k8", + toolchain = "@bazel_toolchains//configs/ubuntu16_04_clang/11.0.0/bazel_3.1.0/cc:cc-compiler-k8", toolchain_type = "@bazel_tools//tools/cpp:toolchain_type", ) diff --git a/tools/bazeldefs/cc.bzl b/tools/bazeldefs/cc.bzl new file mode 100644 index 000000000..7f41a0142 --- /dev/null +++ b/tools/bazeldefs/cc.bzl @@ -0,0 +1,43 @@ +"""C++ rules.""" + +load("@bazel_tools//tools/cpp:cc_flags_supplier.bzl", _cc_flags_supplier = "cc_flags_supplier") +load("@rules_cc//cc:defs.bzl", _cc_binary = "cc_binary", _cc_library = "cc_library", _cc_proto_library = "cc_proto_library", _cc_test = "cc_test") +load("@com_github_grpc_grpc//bazel:cc_grpc_library.bzl", _cc_grpc_library = "cc_grpc_library") + +cc_library = _cc_library +cc_flags_supplier = _cc_flags_supplier +cc_proto_library = _cc_proto_library +cc_test = _cc_test +cc_toolchain = "@bazel_tools//tools/cpp:current_cc_toolchain" +gtest = "@com_google_googletest//:gtest" +gbenchmark = "@com_google_benchmark//:benchmark" +grpcpp = "@com_github_grpc_grpc//:grpc++" +vdso_linker_option = "-fuse-ld=gold " + +def cc_grpc_library(name, **kwargs): + _cc_grpc_library(name = name, grpc_only = True, **kwargs) + +def cc_binary(name, static = False, **kwargs): + """Run cc_binary. + + Args: + name: name of the target. + static: make a static binary if True + **kwargs: the rest of the args. + """ + if static: + # How to statically link a c++ program that uses threads, like for gRPC: + # https://gcc.gnu.org/legacy-ml/gcc-help/2010-05/msg00029.html + if "linkopts" not in kwargs: + kwargs["linkopts"] = [] + kwargs["linkopts"] += [ + "-static", + "-lstdc++", + "-Wl,--whole-archive", + "-lpthread", + "-Wl,--no-whole-archive", + ] + _cc_binary( + name = name, + **kwargs + ) diff --git a/tools/bazeldefs/defs.bzl b/tools/bazeldefs/defs.bzl index db7f379b8..ba186aace 100644 --- a/tools/bazeldefs/defs.bzl +++ b/tools/bazeldefs/defs.bzl @@ -1,35 +1,13 @@ -"""Bazel implementations of standard rules.""" +"""Meta and miscellaneous rules.""" -load("@bazel_gazelle//:def.bzl", _gazelle = "gazelle") load("@bazel_skylib//rules:build_test.bzl", _build_test = "build_test") load("@bazel_skylib//:bzl_library.bzl", _bzl_library = "bzl_library") -load("@bazel_tools//tools/cpp:cc_flags_supplier.bzl", _cc_flags_supplier = "cc_flags_supplier") -load("@io_bazel_rules_go//go:def.bzl", "GoLibrary", _go_binary = "go_binary", _go_context = "go_context", _go_embed_data = "go_embed_data", _go_library = "go_library", _go_path = "go_path", _go_test = "go_test") -load("@io_bazel_rules_go//proto:def.bzl", _go_grpc_library = "go_grpc_library", _go_proto_library = "go_proto_library") -load("@rules_cc//cc:defs.bzl", _cc_binary = "cc_binary", _cc_library = "cc_library", _cc_proto_library = "cc_proto_library", _cc_test = "cc_test") -load("@rules_pkg//:pkg.bzl", _pkg_deb = "pkg_deb", _pkg_tar = "pkg_tar") -load("@com_github_grpc_grpc//bazel:cc_grpc_library.bzl", _cc_grpc_library = "cc_grpc_library") build_test = _build_test bzl_library = _bzl_library -cc_library = _cc_library -cc_flags_supplier = _cc_flags_supplier -cc_proto_library = _cc_proto_library -cc_test = _cc_test -cc_toolchain = "@bazel_tools//tools/cpp:current_cc_toolchain" -gazelle = _gazelle -go_embed_data = _go_embed_data -go_path = _go_path -gtest = "@com_google_googletest//:gtest" -grpcpp = "@com_github_grpc_grpc//:grpc++" -gbenchmark = "@com_google_benchmark//:benchmark" loopback = "//tools/bazeldefs:loopback" -pkg_deb = _pkg_deb -pkg_tar = _pkg_tar -py_binary = native.py_binary rbe_platform = native.platform rbe_toolchain = native.toolchain -vdso_linker_option = "-fuse-ld=gold " def short_path(path): return path @@ -40,128 +18,6 @@ def proto_library(name, has_services = None, **kwargs): **kwargs ) -def cc_grpc_library(name, **kwargs): - _cc_grpc_library(name = name, grpc_only = True, **kwargs) - -def _go_proto_or_grpc_library(go_library_func, name, **kwargs): - deps = [ - dep.replace("_proto", "_go_proto") - for dep in (kwargs.pop("deps", []) or []) - ] - go_library_func( - name = name + "_go_proto", - importpath = "gvisor.dev/gvisor/" + native.package_name() + "/" + name + "_go_proto", - proto = ":" + name + "_proto", - deps = deps, - **kwargs - ) - -def go_proto_library(name, **kwargs): - _go_proto_or_grpc_library(_go_proto_library, name, **kwargs) - -def go_grpc_and_proto_libraries(name, **kwargs): - _go_proto_or_grpc_library(_go_grpc_library, name, **kwargs) - -def cc_binary(name, static = False, **kwargs): - """Run cc_binary. - - Args: - name: name of the target. - static: make a static binary if True - **kwargs: the rest of the args. - """ - if static: - # How to statically link a c++ program that uses threads, like for gRPC: - # https://gcc.gnu.org/legacy-ml/gcc-help/2010-05/msg00029.html - if "linkopts" not in kwargs: - kwargs["linkopts"] = [] - kwargs["linkopts"] += [ - "-static", - "-lstdc++", - "-Wl,--whole-archive", - "-lpthread", - "-Wl,--no-whole-archive", - ] - _cc_binary( - name = name, - **kwargs - ) - -def go_binary(name, static = False, pure = False, **kwargs): - """Build a go binary. - - Args: - name: name of the target. - static: build a static binary. - pure: build without cgo. - **kwargs: rest of the arguments are passed to _go_binary. - """ - if static: - kwargs["static"] = "on" - if pure: - kwargs["pure"] = "on" - _go_binary( - name = name, - **kwargs - ) - -def go_importpath(target): - """Returns the importpath for the target.""" - return target[GoLibrary].importpath - -def go_library(name, **kwargs): - _go_library( - name = name, - importpath = "gvisor.dev/gvisor/" + native.package_name(), - **kwargs - ) - -def go_test(name, pure = False, library = None, **kwargs): - """Build a go test. - - Args: - name: name of the output binary. - pure: should it be built without cgo. - library: the library to embed. - **kwargs: rest of the arguments to pass to _go_test. - """ - if pure: - kwargs["pure"] = "on" - if library: - kwargs["embed"] = [library] - _go_test( - name = name, - **kwargs - ) - -def go_rule(rule, implementation, **kwargs): - """Wraps a rule definition with Go attributes. - - Args: - rule: rule function (typically rule or aspect). - implementation: implementation function. - **kwargs: other arguments to pass to rule. - - Returns: - The result of invoking the rule. - """ - attrs = kwargs.pop("attrs", []) - attrs["_go_context_data"] = attr.label(default = "@io_bazel_rules_go//:go_context_data") - attrs["_stdlib"] = attr.label(default = "@io_bazel_rules_go//:stdlib") - toolchains = kwargs.get("toolchains", []) + ["@io_bazel_rules_go//go:toolchain"] - return rule(implementation, attrs = attrs, toolchains = toolchains, **kwargs) - -def go_context(ctx): - go_ctx = _go_context(ctx) - return struct( - go = go_ctx.go, - env = go_ctx.env, - runfiles = depset([go_ctx.go] + go_ctx.sdk.tools + go_ctx.stdlib.libs), - goos = go_ctx.sdk.goos, - goarch = go_ctx.sdk.goarch, - tags = go_ctx.tags, - ) - def select_arch(amd64 = "amd64", arm64 = "arm64", default = None, **kwargs): values = { "@bazel_tools//src/conditions:linux_x86_64": amd64, @@ -179,3 +35,6 @@ def default_installer(): def default_net_util(): return [] # Nothing needed. + +def coreutil(): + return [] # Nothing needed. diff --git a/tools/bazeldefs/go.bzl b/tools/bazeldefs/go.bzl new file mode 100644 index 000000000..d388346a5 --- /dev/null +++ b/tools/bazeldefs/go.bzl @@ -0,0 +1,142 @@ +"""Go rules.""" + +load("@bazel_gazelle//:def.bzl", _gazelle = "gazelle") +load("@io_bazel_rules_go//go:def.bzl", "GoLibrary", _go_binary = "go_binary", _go_context = "go_context", _go_embed_data = "go_embed_data", _go_library = "go_library", _go_path = "go_path", _go_test = "go_test") +load("@io_bazel_rules_go//proto:def.bzl", _go_grpc_library = "go_grpc_library", _go_proto_library = "go_proto_library") +load("//tools/bazeldefs:defs.bzl", "select_arch", "select_system") + +gazelle = _gazelle +go_embed_data = _go_embed_data +go_path = _go_path + +def _go_proto_or_grpc_library(go_library_func, name, **kwargs): + deps = [ + dep.replace("_proto", "_go_proto") + for dep in (kwargs.pop("deps", []) or []) + ] + go_library_func( + name = name + "_go_proto", + importpath = "gvisor.dev/gvisor/" + native.package_name() + "/" + name + "_go_proto", + proto = ":" + name + "_proto", + deps = deps, + **kwargs + ) + +def go_proto_library(name, **kwargs): + _go_proto_or_grpc_library(_go_proto_library, name, **kwargs) + +def go_grpc_and_proto_libraries(name, **kwargs): + _go_proto_or_grpc_library(_go_grpc_library, name, **kwargs) + +def go_binary(name, static = False, pure = False, x_defs = None, **kwargs): + """Build a go binary. + + Args: + name: name of the target. + static: build a static binary. + pure: build without cgo. + x_defs: additional definitions. + **kwargs: rest of the arguments are passed to _go_binary. + """ + if static: + kwargs["static"] = "on" + if pure: + kwargs["pure"] = "on" + _go_binary( + name = name, + x_defs = x_defs, + **kwargs + ) + +def go_importpath(target): + """Returns the importpath for the target.""" + return target[GoLibrary].importpath + +def go_library(name, **kwargs): + _go_library( + name = name, + importpath = "gvisor.dev/gvisor/" + native.package_name(), + **kwargs + ) + +def go_test(name, pure = False, library = None, **kwargs): + """Build a go test. + + Args: + name: name of the output binary. + pure: should it be built without cgo. + library: the library to embed. + **kwargs: rest of the arguments to pass to _go_test. + """ + if pure: + kwargs["pure"] = "on" + if library: + kwargs["embed"] = [library] + _go_test( + name = name, + **kwargs + ) + +def go_rule(rule, implementation, **kwargs): + """Wraps a rule definition with Go attributes. + + Args: + rule: rule function (typically rule or aspect). + implementation: implementation function. + **kwargs: other arguments to pass to rule. + + Returns: + The result of invoking the rule. + """ + attrs = kwargs.pop("attrs", dict()) + attrs["_go_context_data"] = attr.label(default = "@io_bazel_rules_go//:go_context_data") + attrs["_stdlib"] = attr.label(default = "@io_bazel_rules_go//:stdlib") + toolchains = kwargs.get("toolchains", []) + ["@io_bazel_rules_go//go:toolchain"] + return rule(implementation, attrs = attrs, toolchains = toolchains, **kwargs) + +def go_test_library(target): + if hasattr(target.attr, "embed") and len(target.attr.embed) > 0: + return target.attr.embed[0] + return None + +def go_context(ctx, goos = None, goarch = None, std = False): + """Extracts a standard Go context struct. + + Args: + ctx: the starlark context (required). + goos: the GOOS value. + goarch: the GOARCH value. + std: ignored. + + Returns: + A context Go struct with pointers to Go toolchain components. + """ + + # We don't change anything for the standard library analysis. All Go files + # are available in all instances. Note that this includes the standard + # library sources, which are analyzed by nogo. + go_ctx = _go_context(ctx) + if goos == None: + goos = go_ctx.sdk.goos + elif goos != go_ctx.sdk.goos: + fail("Internal GOOS (%s) doesn't match GoSdk GOOS (%s)." % (goos, go_ctx.sdk.goos)) + if goarch == None: + goarch = go_ctx.sdk.goarch + elif goarch != go_ctx.sdk.goarch: + fail("Internal GOARCH (%s) doesn't match GoSdk GOARCH (%s)." % (goarch, go_ctx.sdk.goarch)) + return struct( + go = go_ctx.go, + env = go_ctx.env, + nogo_args = [], + stdlib_srcs = go_ctx.sdk.srcs, + runfiles = depset([go_ctx.go] + go_ctx.sdk.srcs + go_ctx.sdk.tools + go_ctx.stdlib.libs), + goos = go_ctx.sdk.goos, + goarch = go_ctx.sdk.goarch, + tags = go_ctx.tags, + ) + +def select_goarch(): + return select_arch(arm64 = "arm64", amd64 = "amd64") + +def select_goos(): + return select_system(linux = "linux") diff --git a/tools/bazeldefs/pkg.bzl b/tools/bazeldefs/pkg.bzl new file mode 100644 index 000000000..56317d93f --- /dev/null +++ b/tools/bazeldefs/pkg.bzl @@ -0,0 +1,6 @@ +"""Packaging rules.""" + +load("@rules_pkg//:pkg.bzl", _pkg_deb = "pkg_deb", _pkg_tar = "pkg_tar") + +pkg_deb = _pkg_deb +pkg_tar = _pkg_tar diff --git a/tools/bigquery/BUILD b/tools/bigquery/BUILD index 5748fb390..1cea9e1c9 100644 --- a/tools/bigquery/BUILD +++ b/tools/bigquery/BUILD @@ -6,5 +6,11 @@ go_library( name = "bigquery", testonly = 1, srcs = ["bigquery.go"], - deps = ["@com_google_cloud_go_bigquery//:go_default_library"], + visibility = [ + "//:sandbox", + ], + deps = [ + "@com_google_cloud_go_bigquery//:go_default_library", + "@org_golang_google_api//option:go_default_library", + ], ) diff --git a/tools/bigquery/bigquery.go b/tools/bigquery/bigquery.go index 56f0dc5c9..34b270cc0 100644 --- a/tools/bigquery/bigquery.go +++ b/tools/bigquery/bigquery.go @@ -25,16 +25,33 @@ import ( "time" bq "cloud.google.com/go/bigquery" + "google.golang.org/api/option" ) -// Benchmark is the top level structure of recorded benchmark data. BigQuery +// Suite is the top level structure for a benchmark run. BigQuery // will infer the schema from this. +type Suite struct { + Name string `bq:"name"` + Conditions []*Condition `bq:"conditions"` + Benchmarks []*Benchmark `bq:"benchmarks"` + Official bool `bq:"official"` + Timestamp time.Time `bq:"timestamp"` +} + +// Benchmark represents an individual benchmark in a suite. type Benchmark struct { - Name string `bq:"name"` - Timestamp time.Time `bq:"timestamp"` - Official bool `bq:"official"` - Metric []*Metric `bq:"metric"` - Metadata *Metadata `bq:"metadata"` + Name string `bq:"name"` + Condition []*Condition `bq:"condition"` + Metric []*Metric `bq:"metric"` +} + +// Condition represents qualifiers for the benchmark or suite. For example: +// Get_Pid/1/real_time would have Benchmark Name "Get_Pid" with "1" +// and "real_time" parameters as conditions. Suite conditions include +// information such as the CL number and platform name. +type Condition struct { + Name string `bq:"name"` + Value string `bq:"value"` } // Metric holds the actual metric data and unit information for this benchmark. @@ -44,19 +61,9 @@ type Metric struct { Sample float64 `bq:"sample"` } -// Metadata about this benchmark. -type Metadata struct { - CL string `bq:"changelist"` - IterationID string `bq:"iteration_id"` - PendingCL string `bq:"pending_cl"` - Workflow string `bq:"workflow"` - Platform string `bq:"platform"` - Gofer string `bq:"gofer"` -} - // InitBigQuery initializes a BigQuery dataset/table in the project. If the dataset/table already exists, it is not duplicated. -func InitBigQuery(ctx context.Context, projectID, datasetID, tableID string) error { - client, err := bq.NewClient(ctx, projectID) +func InitBigQuery(ctx context.Context, projectID, datasetID, tableID string, opts []option.ClientOption) error { + client, err := bq.NewClient(ctx, projectID, opts...) if err != nil { return fmt.Errorf("failed to initialize client on project %s: %v", projectID, err) } @@ -68,7 +75,7 @@ func InitBigQuery(ctx context.Context, projectID, datasetID, tableID string) err } table := dataset.Table(tableID) - schema, err := bq.InferSchema(Benchmark{}) + schema, err := bq.InferSchema(Suite{}) if err != nil { return fmt.Errorf("failed to infer schema: %v", err) } @@ -79,6 +86,14 @@ func InitBigQuery(ctx context.Context, projectID, datasetID, tableID string) err return nil } +// AddCondition adds a condition to an existing Benchmark. +func (bm *Benchmark) AddCondition(name, value string) { + bm.Condition = append(bm.Condition, &Condition{ + Name: name, + Value: value, + }) +} + // AddMetric adds a metric to an existing Benchmark. func (bm *Benchmark) AddMetric(metricName, unit string, sample float64) { m := &Metric{ @@ -90,26 +105,34 @@ func (bm *Benchmark) AddMetric(metricName, unit string, sample float64) { } // NewBenchmark initializes a new benchmark. -func NewBenchmark(name string, official bool) *Benchmark { +func NewBenchmark(name string, iters int, official bool) *Benchmark { return &Benchmark{ - Name: name, - Timestamp: time.Now().UTC(), - Official: official, - Metric: make([]*Metric, 0), + Name: name, + Metric: make([]*Metric, 0), + } +} + +// NewSuite initializes a new Suite. +func NewSuite(name string) *Suite { + return &Suite{ + Name: name, + Timestamp: time.Now().UTC(), + Benchmarks: make([]*Benchmark, 0), + Conditions: make([]*Condition, 0), } } // SendBenchmarks sends the slice of benchmarks to the BigQuery dataset/table. -func SendBenchmarks(ctx context.Context, benchmarks []*Benchmark, projectID, datasetID, tableID string) error { - client, err := bq.NewClient(ctx, projectID) +func SendBenchmarks(ctx context.Context, suite *Suite, projectID, datasetID, tableID string, opts []option.ClientOption) error { + client, err := bq.NewClient(ctx, projectID, opts...) if err != nil { - return fmt.Errorf("Failed to initialize client on project: %s: %v", projectID, err) + return fmt.Errorf("failed to initialize client on project: %s: %v", projectID, err) } defer client.Close() uploader := client.Dataset(datasetID).Table(tableID).Uploader() - if err = uploader.Put(ctx, benchmarks); err != nil { - return fmt.Errorf("failed to upload benchmarks to proejct %s, table %s.%s: %v", projectID, datasetID, tableID, err) + if err = uploader.Put(ctx, suite); err != nil { + return fmt.Errorf("failed to upload benchmarks %s to project %s, table %s.%s: %v", suite.Name, projectID, datasetID, tableID, err) } return nil diff --git a/tools/checkescape/BUILD b/tools/checkescape/BUILD index b8c3ddf44..8956be621 100644 --- a/tools/checkescape/BUILD +++ b/tools/checkescape/BUILD @@ -8,7 +8,6 @@ go_library( nogo = False, visibility = ["//tools/nogo:__subpackages__"], deps = [ - "//tools/nogo/data", "@org_golang_x_tools//go/analysis:go_tool_library", "@org_golang_x_tools//go/analysis/passes/buildssa:go_tool_library", "@org_golang_x_tools//go/ssa:go_tool_library", diff --git a/tools/checkescape/checkescape.go b/tools/checkescape/checkescape.go index f8def4823..e5a7e23c7 100644 --- a/tools/checkescape/checkescape.go +++ b/tools/checkescape/checkescape.go @@ -61,20 +61,21 @@ package checkescape import ( "bufio" "bytes" + "flag" "fmt" "go/ast" "go/token" "go/types" "io" + "log" "os" + "os/exec" "path/filepath" - "strconv" "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/buildssa" "golang.org/x/tools/go/ssa" - "gvisor.dev/gvisor/tools/nogo/data" ) const ( @@ -91,81 +92,20 @@ const ( exempt = "// escapes" ) -// escapingBuiltins are builtins known to escape. -// -// These are lowered at an earlier stage of compilation to explicit function -// calls, but are not available for recursive analysis. -var escapingBuiltins = []string{ - "append", - "makemap", - "newobject", - "mallocgc", -} - -// Analyzer defines the entrypoint. -var Analyzer = &analysis.Analyzer{ - Name: "checkescape", - Doc: "surfaces recursive escape analysis results", - Run: run, - Requires: []*analysis.Analyzer{buildssa.Analyzer}, - FactTypes: []analysis.Fact{(*packageEscapeFacts)(nil)}, -} - -// packageEscapeFacts is the set of all functions in a package, and whether or -// not they recursively pass escape analysis. -// -// All the type names for receivers are encoded in the full key. The key -// represents the fully qualified package and type name used at link time. -type packageEscapeFacts struct { - Funcs map[string][]Escape -} - -// AFact implements analysis.Fact.AFact. -func (*packageEscapeFacts) AFact() {} - -// CallSite is a single call site. -// -// These can be chained. -type CallSite struct { - LocalPos token.Pos - Resolved LinePosition -} - -// Escape is a single escape instance. -type Escape struct { - Reason EscapeReason - Detail string - Chain []CallSite -} - -// LinePosition is a low-resolution token.Position. -// -// This is used to match against possible exemptions placed in the source. -type LinePosition struct { - Filename string - Line int -} +var ( + // Binary is the binary under analysis. + // + // See Reader, below. + binary = flag.String("binary", "", "binary under analysis") -// String implements fmt.Stringer.String. -func (e *LinePosition) String() string { - return fmt.Sprintf("%s:%d", e.Filename, e.Line) -} + // Reader is the input stream. + // + // This may be set instead of Binary. + Reader io.Reader -// String implements fmt.Stringer.String. -// -// Note that this string will contain new lines. -func (e *Escape) String() string { - var b bytes.Buffer - fmt.Fprintf(&b, "%s", e.Reason.String()) - for i, cs := range e.Chain { - if i == len(e.Chain)-1 { - fmt.Fprintf(&b, "\n @ %s → %s", cs.Resolved.String(), e.Detail) - } else { - fmt.Fprintf(&b, "\n + %s", cs.Resolved.String()) - } - } - return b.String() -} + // objdumpTool is the tool used to dump a binary. + objdumpTool = flag.String("objdump_tool", "", "tool used to dump a binary") +) // EscapeReason is an escape reason. // @@ -173,12 +113,12 @@ func (e *Escape) String() string { type EscapeReason int const ( - interfaceInvoke EscapeReason = iota - unknownPackage - allocation + allocation EscapeReason = iota builtin + interfaceInvoke dynamicCall stackSplit + unknownPackage reasonCount // Count for below. ) @@ -189,17 +129,17 @@ const ( func (e EscapeReason) String() string { switch e { case interfaceInvoke: - return "interface: function invocation via interface" + return "interface: call to potentially allocating function" case unknownPackage: return "unknown: no package information available" case allocation: - return "heap: call to runtime heap allocation" + return "heap: explicit allocation" case builtin: - return "builtin: call to runtime builtin" + return "builtin: call to potentially allocating builtin" case dynamicCall: - return "dynamic: call via dynamic function" + return "dynamic: call to potentially allocating function" case stackSplit: - return "stack: stack split on function entry" + return "stack: possible split on function entry" default: panic(fmt.Sprintf("unknown reason: %d", e)) } @@ -228,52 +168,289 @@ var escapeTypes = func() map[string]EscapeReason { return result }() -// EscapeCount counts escapes. +// escapingBuiltins are builtins known to escape. +// +// These are lowered at an earlier stage of compilation to explicit function +// calls, but are not available for recursive analysis. +var escapingBuiltins = []string{ + "append", + "makemap", + "newobject", + "mallocgc", +} + +// packageEscapeFacts is the set of all functions in a package, and whether or +// not they recursively pass escape analysis. +// +// All the type names for receivers are encoded in the full key. The key +// represents the fully qualified package and type name used at link time. // -// It is used to avoid accumulating too many escapes for the same reason, for -// the same function. We limit each class to 3 instances (arbitrarily). -type EscapeCount struct { - byReason [reasonCount]uint32 +// Note that each Escapes object is a summary. Local findings may be reported +// using more detailed information. +type packageEscapeFacts struct { + Funcs map[string]Escapes +} + +// AFact implements analysis.Fact.AFact. +func (*packageEscapeFacts) AFact() {} + +// Analyzer includes specific results. +var Analyzer = &analysis.Analyzer{ + Name: "checkescape", + Doc: "escape analysis checks based on +checkescape annotations", + Run: runSelectEscapes, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, + FactTypes: []analysis.Fact{(*packageEscapeFacts)(nil)}, +} + +// EscapeAnalyzer includes all local escape results. +var EscapeAnalyzer = &analysis.Analyzer{ + Name: "checkescape", + Doc: "complete local escape analysis results (requires Analyzer facts)", + Run: runAllEscapes, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, } -// maxRecordsPerReason is the number of explicit records. +// LinePosition is a low-resolution token.Position. // -// See EscapeCount (and usage), and Record implementation. -const maxRecordsPerReason = 5 - -// Record records the reason or returns false if it should not be added. -func (ec *EscapeCount) Record(reason EscapeReason) bool { - ec.byReason[reason]++ - if ec.byReason[reason] > maxRecordsPerReason { - return false +// This is used to match against possible exemptions placed in the source. +type LinePosition struct { + Filename string + Line int +} + +// String implements fmt.Stringer.String. +func (e LinePosition) String() string { + return fmt.Sprintf("%s:%d", e.Filename, e.Line) +} + +// Simplified returns the simplified name. +func (e LinePosition) Simplified() string { + return fmt.Sprintf("%s:%d", filepath.Base(e.Filename), e.Line) +} + +// CallSite is a single call site. +// +// These can be chained. +type CallSite struct { + LocalPos token.Pos + Resolved LinePosition +} + +// IsValid indicates whether the CallSite is valid or not. +func (cs *CallSite) IsValid() bool { + return cs.LocalPos.IsValid() +} + +// Escapes is a collection of escapes. +// +// We record at most one escape for each reason, but record the number of +// escapes that were omitted. +// +// This object should be used to summarize all escapes for a single line (local +// analysis) or a single function (package facts). +// +// All fields are exported for gob. +type Escapes struct { + CallSites [reasonCount][]CallSite + Details [reasonCount]string + Omitted [reasonCount]int +} + +// add is called by Add and Merge. +func (es *Escapes) add(r EscapeReason, detail string, omitted int, callSites ...CallSite) { + if es.CallSites[r] != nil { + // We will either be replacing the current escape or dropping + // the added one. Either way, we increment omitted by the + // appropriate amount. + es.Omitted[r]++ + // If the callSites in the other is only a single element, then + // we will universally favor this. This provides the cleanest + // set of escapes to summarize, and more importantly: if there + if len(es.CallSites) == 1 || len(callSites) != 1 { + return + } + } + es.Details[r] = detail + es.CallSites[r] = callSites + es.Omitted[r] += omitted +} + +// Add adds a single escape. +func (es *Escapes) Add(r EscapeReason, detail string, callSites ...CallSite) { + es.add(r, detail, 0, callSites...) +} + +// IsEmpty returns true iff this Escapes is empty. +func (es *Escapes) IsEmpty() bool { + for _, cs := range es.CallSites { + if cs != nil { + return false + } } return true } +// Filter filters out all escapes except those matches the given reasons. +// +// If local is set, then non-local escapes will also be filtered. +func (es *Escapes) Filter(reasons []EscapeReason, local bool) { +FilterReasons: + for r := EscapeReason(0); r < reasonCount; r++ { + for i := 0; i < len(reasons); i++ { + if r == reasons[i] { + continue FilterReasons + } + } + // Zap this reason. + es.CallSites[r] = nil + es.Details[r] = "" + es.Omitted[r] = 0 + } + if !local { + return + } + for r := EscapeReason(0); r < reasonCount; r++ { + // Is does meet our local requirement? + if len(es.CallSites[r]) > 1 { + es.CallSites[r] = nil + es.Details[r] = "" + es.Omitted[r] = 0 + } + } +} + +// MergeWithCall merges these escapes with another. +// +// If callSite is nil, no call is added. +func (es *Escapes) MergeWithCall(other Escapes, callSite CallSite) { + for r := EscapeReason(0); r < reasonCount; r++ { + if other.CallSites[r] != nil { + // Construct our new call chain. + newCallSites := other.CallSites[r] + if callSite.IsValid() { + newCallSites = append([]CallSite{callSite}, newCallSites...) + } + // Add (potentially replacing) the underlying escape. + es.add(r, other.Details[r], other.Omitted[r], newCallSites...) + } + } +} + +// Reportf will call Reportf for each class of escapes. +func (es *Escapes) Reportf(pass *analysis.Pass) { + var b bytes.Buffer // Reused for all escapes. + for r := EscapeReason(0); r < reasonCount; r++ { + if es.CallSites[r] == nil { + continue + } + b.Reset() + fmt.Fprintf(&b, "%s ", r.String()) + if es.Omitted[r] > 0 { + fmt.Fprintf(&b, "(%d omitted) ", es.Omitted[r]) + } + for _, cs := range es.CallSites[r][1:] { + fmt.Fprintf(&b, "→ %s ", cs.Resolved.String()) + } + fmt.Fprintf(&b, "→ %s", es.Details[r]) + pass.Reportf(es.CallSites[r][0].LocalPos, b.String()) + } +} + +// MergeAll merges a sequence of escapes. +func MergeAll(others []Escapes) (es Escapes) { + for _, other := range others { + es.MergeWithCall(other, CallSite{}) + } + return +} + // loadObjdump reads the objdump output. // // This records if there is a call any function for every source line. It is // used only to remove false positives for escape analysis. The call will be // elided if escape analysis is able to put the object on the heap exclusively. -func loadObjdump() (map[LinePosition]string, error) { - f, err := os.Open(data.Objdump) +// +// Note that the map uses <basename.go>:<line> because that is all that is +// provided in the objdump format. Since this is all local, it is sufficient. +func loadObjdump() (map[string][]string, error) { + var ( + args []string + stdin io.Reader + ) + if *binary != "" { + args = append(args, *binary) + } else if Reader != nil { + stdin = Reader + } else { + // We have no input stream or binary. + return nil, fmt.Errorf("no binary or reader provided") + } + + // Construct our command. + cmd := exec.Command(*objdumpTool, args...) + cmd.Stdin = stdin + cmd.Stderr = os.Stderr + out, err := cmd.StdoutPipe() if err != nil { return nil, err } - defer f.Close() + if err := cmd.Start(); err != nil { + return nil, err + } + + // Identify calls by address or name. Note that this is also + // constructed dynamically below, as we encounted the addresses. + // This is because some of the functions (duffzero) may have + // jump targets in the middle of the function itself. + funcsAllowed := map[string]struct{}{ + "runtime.duffzero": struct{}{}, + "runtime.duffcopy": struct{}{}, + "runtime.racefuncenter": struct{}{}, + "runtime.gcWriteBarrier": struct{}{}, + "runtime.retpolineAX": struct{}{}, + "runtime.retpolineBP": struct{}{}, + "runtime.retpolineBX": struct{}{}, + "runtime.retpolineCX": struct{}{}, + "runtime.retpolineDI": struct{}{}, + "runtime.retpolineDX": struct{}{}, + "runtime.retpolineR10": struct{}{}, + "runtime.retpolineR11": struct{}{}, + "runtime.retpolineR12": struct{}{}, + "runtime.retpolineR13": struct{}{}, + "runtime.retpolineR14": struct{}{}, + "runtime.retpolineR15": struct{}{}, + "runtime.retpolineR8": struct{}{}, + "runtime.retpolineR9": struct{}{}, + "runtime.retpolineSI": struct{}{}, + "runtime.stackcheck": struct{}{}, + "runtime.settls": struct{}{}, + } + addrsAllowed := make(map[string]struct{}) // Build the map. - m := make(map[LinePosition]string) - r := bufio.NewReader(f) - var ( - lastField string - lastPos LinePosition - ) + nextFunc := "" // For funcsAllowed. + m := make(map[string][]string) + r := bufio.NewReader(out) +NextLine: for { line, err := r.ReadString('\n') if err != nil && err != io.EOF { return nil, err } + fields := strings.Fields(line) + + // Is this an "allowed" function definition? + if len(fields) >= 2 && fields[0] == "TEXT" { + nextFunc = strings.TrimSuffix(fields[1], "(SB)") + if _, ok := funcsAllowed[nextFunc]; !ok { + nextFunc = "" // Don't record addresses. + } + } + if nextFunc != "" && len(fields) > 2 { + // Save the given address (in hex form, as it appears). + addrsAllowed[fields[1]] = struct{}{} + } // We recognize lines corresponding to actual code (not the // symbol name or other metadata) and annotate them if they @@ -283,53 +460,70 @@ func loadObjdump() (map[LinePosition]string, error) { // // Lines look like this (including the first space): // gohacks_unsafe.go:33 0xa39 488b442408 MOVQ 0x8(SP), AX - if len(line) > 0 && line[0] == ' ' { - fields := strings.Fields(line) + if len(fields) >= 5 && line[0] == ' ' { if !strings.Contains(fields[3], "CALL") { continue } + site := fields[0] + target := strings.TrimSuffix(fields[4], "(SB)") - // Ignore strings containing duffzero, which is just - // used by stack allocations for types that are large - // enough to warrant Duff's device. - if strings.Contains(line, "runtime.duffzero") { + // Ignore strings containing allowed functions. + if _, ok := funcsAllowed[target]; ok { continue } - - // Ignore the racefuncenter call, which is used for - // race builds. This does not escape. - if strings.Contains(line, "runtime.racefuncenter") { + if _, ok := addrsAllowed[target]; ok { continue } - - // Calculate the filename and line. Note that per the - // example above, the filename is not a fully qualified - // base, just the basename (what we require). - if fields[0] != lastField { - parts := strings.SplitN(fields[0], ":", 2) - lineNum, err := strconv.ParseInt(parts[1], 10, 64) - if err != nil { - return nil, err - } - lastPos = LinePosition{ - Filename: parts[0], - Line: int(lineNum), + if len(fields) > 5 { + // This may be a future relocation. Some + // objdump versions describe this differently. + // If it contains any of the functions allowed + // above as a string, we let it go. + softTarget := strings.Join(fields[5:], " ") + for name := range funcsAllowed { + if strings.Contains(softTarget, name) { + continue NextLine + } } - lastField = fields[0] - } - if _, ok := m[lastPos]; ok { - continue // Already marked. } - // Save the actual call for the detail. - m[lastPos] = strings.Join(fields[3:], " ") + // Does this exist already? + existing, ok := m[site] + if !ok { + existing = make([]string, 0, 1) + } + for _, other := range existing { + if target == other { + continue NextLine + } + } + existing = append(existing, target) + m[site] = existing // Update. } if err == io.EOF { break } } - return m, nil + // Zap any accidental false positives. + final := make(map[string][]string) + for site, calls := range m { + filteredCalls := make([]string, 0, len(calls)) + for _, call := range calls { + if _, ok := addrsAllowed[call]; ok { + continue // Omit this call. + } + filteredCalls = append(filteredCalls, call) + } + final[site] = filteredCalls + } + + // Wait for the dump to finish. + if err := cmd.Wait(); err != nil { + return nil, err + } + + return final, nil } // poser is a type that implements Pos. @@ -337,65 +531,156 @@ type poser interface { Pos() token.Pos } +// runSelectEscapes runs with only select escapes. +func runSelectEscapes(pass *analysis.Pass) (interface{}, error) { + return run(pass, false) +} + +// runAllEscapes runs with all escapes included. +func runAllEscapes(pass *analysis.Pass) (interface{}, error) { + return run(pass, true) +} + +// findReasons extracts reasons from the function. +func findReasons(pass *analysis.Pass, fdecl *ast.FuncDecl) ([]EscapeReason, bool, map[EscapeReason]bool) { + // Is there a comment? + if fdecl.Doc == nil { + return nil, false, nil + } + var ( + reasons []EscapeReason + local bool + testReasons = make(map[EscapeReason]bool) // reason -> local? + ) + // Scan all lines. + found := false + for _, c := range fdecl.Doc.List { + // Does the comment contain a +checkescape line? + if !strings.HasPrefix(c.Text, magic) && !strings.HasPrefix(c.Text, testMagic) { + continue + } + if c.Text == magic { + // Default: hard reasons, local only. + reasons = hardReasons + local = true + } else if strings.HasPrefix(c.Text, magicParams) { + // Extract specific reasons. + types := strings.Split(c.Text[len(magicParams):], ",") + found = true // For below. + for i := 0; i < len(types); i++ { + if types[i] == "local" { + // Limit search to local escapes. + local = true + } else if types[i] == "all" { + // Append all reasons. + reasons = append(reasons, allReasons...) + } else if types[i] == "hard" { + // Append all hard reasons. + reasons = append(reasons, hardReasons...) + } else { + r, ok := escapeTypes[types[i]] + if !ok { + // This is not a valid escape reason. + pass.Reportf(fdecl.Pos(), "unknown reason: %v", types[i]) + continue + } + reasons = append(reasons, r) + } + } + } else if strings.HasPrefix(c.Text, testMagic) { + types := strings.Split(c.Text[len(testMagic):], ",") + local := false + for i := 0; i < len(types); i++ { + if types[i] == "local" { + local = true + } else { + r, ok := escapeTypes[types[i]] + if !ok { + // This is not a valid escape reason. + pass.Reportf(fdecl.Pos(), "unknown reason: %v", types[i]) + continue + } + if v, ok := testReasons[r]; ok && v { + // Already registered as local. + continue + } + testReasons[r] = local + } + } + } + } + if len(reasons) == 0 && found { + // A magic annotation was provided, but no reasons. + pass.Reportf(fdecl.Pos(), "no reasons provided") + } + return reasons, local, testReasons +} + // run performs the analysis. -func run(pass *analysis.Pass) (interface{}, error) { +func run(pass *analysis.Pass, localEscapes bool) (interface{}, error) { calls, err := loadObjdump() if err != nil { - return nil, err - } - pef := packageEscapeFacts{ - Funcs: make(map[string][]Escape), + // Note that if this analysis fails, then we don't actually + // fail the analyzer itself. We simply report every possible + // escape. In most cases this will work just fine. + log.Printf("WARNING: unable to load objdump: %v", err) } + allEscapes := make(map[string][]Escapes) + mergedEscapes := make(map[string]Escapes) linePosition := func(inst, parent poser) LinePosition { p := pass.Fset.Position(inst.Pos()) if (p.Filename == "" || p.Line == 0) && parent != nil { p = pass.Fset.Position(parent.Pos()) } return LinePosition{ - Filename: filepath.Base(p.Filename), + Filename: p.Filename, Line: p.Line, } } - hasCall := func(inst poser) (string, bool) { - p := linePosition(inst, nil) - s, ok := calls[p] - return s, ok - } callSite := func(inst ssa.Instruction) CallSite { return CallSite{ LocalPos: inst.Pos(), Resolved: linePosition(inst, inst.Parent()), } } - escapes := func(reason EscapeReason, detail string, inst ssa.Instruction, ec *EscapeCount) []Escape { - if !ec.Record(reason) { - return nil // Skip. + hasCall := func(inst poser) (string, bool) { + p := linePosition(inst, nil) + if calls == nil { + // See above: we don't have access to the binary + // itself, so need to include every possible call. + return "(possible)", true } - es := Escape{ - Reason: reason, - Detail: detail, - Chain: []CallSite{callSite(inst)}, + s, ok := calls[p.Simplified()] + if !ok { + return "", false } - return []Escape{es} + // Join all calls together. + return strings.Join(s, " or "), true } - resolve := func(sub []Escape, inst ssa.Instruction, ec *EscapeCount) (es []Escape) { - for _, e := range sub { - if !ec.Record(e.Reason) { - continue // Skip. + state := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) + + // Build the exception list. + exemptions := make(map[LinePosition]string) + for _, f := range pass.Files { + for _, cg := range f.Comments { + for _, c := range cg.List { + p := pass.Fset.Position(c.Slash) + if strings.HasPrefix(strings.ToLower(c.Text), exempt) { + exemptions[LinePosition{ + Filename: p.Filename, + Line: p.Line, + }] = c.Text[len(exempt):] + } } - es = append(es, Escape{ - Reason: e.Reason, - Detail: e.Detail, - Chain: append([]CallSite{callSite(inst)}, e.Chain...), - }) } - return es } - state := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) - var loadFunc func(*ssa.Function) []Escape // Used below. - - analyzeInstruction := func(inst ssa.Instruction, ec *EscapeCount) []Escape { + var loadFunc func(*ssa.Function) Escapes // Used below. + analyzeInstruction := func(inst ssa.Instruction) (es Escapes) { + cs := callSite(inst) + if _, ok := exemptions[cs.Resolved]; ok { + return // No escape. + } switch x := inst.(type) { case *ssa.Call: if x.Call.IsInvoke() { @@ -404,19 +689,15 @@ func run(pass *analysis.Pass) (interface{}, error) { // not, since we don't know the underlying // type. call, _ := hasCall(inst) - return escapes(interfaceInvoke, call, inst, ec) + es.Add(interfaceInvoke, call, cs) + return } switch x := x.Call.Value.(type) { case *ssa.Function: if x.Pkg == nil { // Can't resolve the package. - return escapes(unknownPackage, "no package", inst, ec) - } - - // Atomic functions are instrinics. We can - // assume that they don't escape. - if x.Pkg.Pkg.Name() == "atomic" { - return nil + es.Add(unknownPackage, "no package", cs) + return } // Is this a local function? If yes, call the @@ -424,7 +705,8 @@ func run(pass *analysis.Pass) (interface{}, error) { // local escapes are the escapes found in the // local function. if x.Pkg.Pkg == pass.Pkg { - return resolve(loadFunc(x), inst, ec) + es.MergeWithCall(loadFunc(x), cs) + return } // Recursively collect information from @@ -433,22 +715,26 @@ func run(pass *analysis.Pass) (interface{}, error) { if !pass.ImportPackageFact(x.Pkg.Pkg, &imp) { // Unable to import the dependency; we must // declare these as escaping. - return escapes(unknownPackage, "no analysis", inst, ec) + es.Add(unknownPackage, "no analysis", cs) + return } // The escapes of this instruction are the // escapes of the called function directly. - return resolve(imp.Funcs[x.RelString(x.Pkg.Pkg)], inst, ec) + // Note that this may record many escapes. + es.MergeWithCall(imp.Funcs[x.RelString(x.Pkg.Pkg)], cs) + return case *ssa.Builtin: // Ignore elided escapes. if _, has := hasCall(inst); !has { - return nil + return } // Check if the builtin is escaping. for _, name := range escapingBuiltins { if x.Name() == name { - return escapes(builtin, name, inst, ec) + es.Add(builtin, name, cs) + return } } default: @@ -457,82 +743,87 @@ func run(pass *analysis.Pass) (interface{}, error) { // dispatches. We cannot actually look up what // this refers to using static analysis alone. call, _ := hasCall(inst) - return escapes(dynamicCall, call, inst, ec) + es.Add(dynamicCall, call, cs) } case *ssa.Alloc: // Ignore non-heap allocations. if !x.Heap { - return nil + return } // Ignore elided escapes. call, has := hasCall(inst) if !has { - return nil + return } // This is a real heap allocation. - return escapes(allocation, call, inst, ec) + es.Add(allocation, call, cs) case *ssa.MakeMap: - return escapes(builtin, "makemap", inst, ec) + es.Add(builtin, "makemap", cs) case *ssa.MakeSlice: - return escapes(builtin, "makeslice", inst, ec) + es.Add(builtin, "makeslice", cs) case *ssa.MakeClosure: - return escapes(builtin, "makeclosure", inst, ec) + es.Add(builtin, "makeclosure", cs) case *ssa.MakeChan: - return escapes(builtin, "makechan", inst, ec) + es.Add(builtin, "makechan", cs) } - return nil // No escapes. + return } - var analyzeBasicBlock func(*ssa.BasicBlock, *EscapeCount) []Escape // Recursive. - analyzeBasicBlock = func(block *ssa.BasicBlock, ec *EscapeCount) (rval []Escape) { + var analyzeBasicBlock func(*ssa.BasicBlock) []Escapes // Recursive. + analyzeBasicBlock = func(block *ssa.BasicBlock) (rval []Escapes) { for _, inst := range block.Instrs { - rval = append(rval, analyzeInstruction(inst, ec)...) + if es := analyzeInstruction(inst); !es.IsEmpty() { + rval = append(rval, es) + } } - return rval // N.B. may be empty. + return } - loadFunc = func(fn *ssa.Function) []Escape { + loadFunc = func(fn *ssa.Function) Escapes { // Is this already available? name := fn.RelString(pass.Pkg) - if es, ok := pef.Funcs[name]; ok { + if es, ok := mergedEscapes[name]; ok { return es } // In the case of a true cycle, we assume that the current - // function itself has no escapes until the rest of the - // analysis is complete. This will trip the above in the case - // of a cycle of any kind. - pef.Funcs[name] = nil + // function itself has no escapes. + // + // When evaluating the function again, the proper escapes will + // be filled in here. + allEscapes[name] = nil + mergedEscapes[name] = Escapes{} // Perform the basic analysis. - var ( - es []Escape - ec EscapeCount - ) + var es []Escapes if fn.Recover != nil { - es = append(es, analyzeBasicBlock(fn.Recover, &ec)...) + es = append(es, analyzeBasicBlock(fn.Recover)...) } for _, block := range fn.Blocks { - es = append(es, analyzeBasicBlock(block, &ec)...) + es = append(es, analyzeBasicBlock(block)...) } // Check for a stack split. if call, has := hasCall(fn); has { - es = append(es, Escape{ - Reason: stackSplit, - Detail: call, - Chain: []CallSite{CallSite{ - LocalPos: fn.Pos(), - Resolved: linePosition(fn, fn.Parent()), - }}, + var ss Escapes + ss.Add(stackSplit, call, CallSite{ + LocalPos: fn.Pos(), + Resolved: linePosition(fn, fn.Parent()), }) + es = append(es, ss) } // Save the result and return. - pef.Funcs[name] = es - return es + // + // Note that we merge the result when saving to the facts. It + // doesn't really matter the specific escapes, as long as we + // have recorded all the appropriate classes of escapes. + summary := MergeAll(es) + allEscapes[name] = es + mergedEscapes[name] = summary + return summary } // Complete all local functions. @@ -540,173 +831,76 @@ func run(pass *analysis.Pass) (interface{}, error) { loadFunc(fn) } - // Build the exception list. - exemptions := make(map[LinePosition]string) - for _, f := range pass.Files { - for _, cg := range f.Comments { - for _, c := range cg.List { - p := pass.Fset.Position(c.Slash) - if strings.HasPrefix(strings.ToLower(c.Text), exempt) { - exemptions[LinePosition{ - Filename: filepath.Base(p.Filename), - Line: p.Line, - }] = c.Text[len(exempt):] - } - } - } + if !localEscapes { + // Export all findings for future packages. We only do this in + // non-local escapes mode, and expect to run this analysis + // after the SelectAnalysis. + pass.ExportPackageFact(&packageEscapeFacts{ + Funcs: mergedEscapes, + }) } - // Delete everything matching the excemtions. - // - // This has the implication that exceptions are applied recursively, - // since this now modified set is what will be saved. - for name, escapes := range pef.Funcs { - var newEscapes []Escape - for _, escape := range escapes { - isExempt := false - for line, _ := range exemptions { - // Note that an exemption applies if it is - // marked as an exemption anywhere in the call - // chain. It need not be marked as escapes in - // the function itself, nor in the top-level - // caller. - for _, callSite := range escape.Chain { - if callSite.Resolved == line { - isExempt = true - break - } - } - if isExempt { - break - } - } - if !isExempt { - // Record this escape; not an exception. - newEscapes = append(newEscapes, escape) - } - } - pef.Funcs[name] = newEscapes // Update. - } - - // Export all findings for future packages. - pass.ExportPackageFact(&pef) - // Scan all functions for violations. for _, f := range pass.Files { // Scan all declarations. for _, decl := range f.Decls { - fdecl, ok := decl.(*ast.FuncDecl) // Function declaration? + fdecl, ok := decl.(*ast.FuncDecl) if !ok { continue } - // Is there a comment? - if fdecl.Doc == nil { - continue - } var ( reasons []EscapeReason - found bool local bool - testReasons = make(map[EscapeReason]bool) // reason -> local? + testReasons map[EscapeReason]bool ) - // Does the comment contain a +checkescape line? - for _, c := range fdecl.Doc.List { - if !strings.HasPrefix(c.Text, magic) && !strings.HasPrefix(c.Text, testMagic) { - continue - } - if c.Text == magic { - // Default: hard reasons, local only. - reasons = hardReasons - local = true - } else if strings.HasPrefix(c.Text, magicParams) { - // Extract specific reasons. - types := strings.Split(c.Text[len(magicParams):], ",") - found = true // For below. - for i := 0; i < len(types); i++ { - if types[i] == "local" { - // Limit search to local escapes. - local = true - } else if types[i] == "all" { - // Append all reasons. - reasons = append(reasons, allReasons...) - } else if types[i] == "hard" { - // Append all hard reasons. - reasons = append(reasons, hardReasons...) - } else { - r, ok := escapeTypes[types[i]] - if !ok { - // This is not a valid escape reason. - pass.Reportf(fdecl.Pos(), "unknown reason: %v", types[i]) - continue - } - reasons = append(reasons, r) - } - } - } else if strings.HasPrefix(c.Text, testMagic) { - types := strings.Split(c.Text[len(testMagic):], ",") - local := false - for i := 0; i < len(types); i++ { - if types[i] == "local" { - local = true - } else { - r, ok := escapeTypes[types[i]] - if !ok { - // This is not a valid escape reason. - pass.Reportf(fdecl.Pos(), "unknown reason: %v", types[i]) - continue - } - if v, ok := testReasons[r]; ok && v { - // Already registered as local. - continue - } - testReasons[r] = local - } - } - } - } - if len(reasons) == 0 && found { - // A magic annotation was provided, but no reasons. - pass.Reportf(fdecl.Pos(), "no reasons provided") - continue + if localEscapes { + // Find all hard escapes. + reasons = hardReasons + } else { + // Find all declared reasons. + reasons, local, testReasons = findReasons(pass, fdecl) } // Scan for matches. fn := pass.TypesInfo.Defs[fdecl.Name].(*types.Func) - name := state.Pkg.Prog.FuncValue(fn).RelString(pass.Pkg) - es, ok := pef.Funcs[name] - if !ok { + fv := state.Pkg.Prog.FuncValue(fn) + if fv == nil { + continue + } + name := fv.RelString(pass.Pkg) + all, allOk := allEscapes[name] + merged, mergedOk := mergedEscapes[name] + if !allOk || !mergedOk { pass.Reportf(fdecl.Pos(), "internal error: function %s not found.", name) continue } - for _, e := range es { - for _, r := range reasons { - // Is does meet our local requirement? - if local && len(e.Chain) > 1 { - continue - } - // Does this match the reason? Emit - // with a full stack trace that - // explains why this violates our - // constraints. - if e.Reason == r { - pass.Reportf(e.Chain[0].LocalPos, "%s", e.String()) - } - } + + // Filter reasons and report. + // + // For the findings, we use all escapes. + for _, es := range all { + es.Filter(reasons, local) + es.Reportf(pass) } // Scan for test (required) matches. + // + // For tests we need only the merged escapes. testReasonsFound := make(map[EscapeReason]bool) - for _, e := range es { + for r := EscapeReason(0); r < reasonCount; r++ { + if merged.CallSites[r] == nil { + continue + } // Is this local? - local, ok := testReasons[e.Reason] - wantLocal := len(e.Chain) == 1 - testReasonsFound[e.Reason] = wantLocal + wantLocal, ok := testReasons[r] + isLocal := len(merged.CallSites[r]) == 1 + testReasonsFound[r] = isLocal if !ok { continue } - if local == wantLocal { - delete(testReasons, e.Reason) + if isLocal == wantLocal { + delete(testReasons, r) } } for reason, local := range testReasons { @@ -714,10 +908,8 @@ func run(pass *analysis.Pass) (interface{}, error) { pass.Reportf(fdecl.Pos(), fmt.Sprintf("testescapes not found: reason=%s, local=%t", reason, local)) } if len(testReasons) > 0 { - // Dump all reasons found to help in debugging. - for _, e := range es { - pass.Reportf(e.Chain[0].LocalPos, "escape found: %s", e.String()) - } + // Report for debugging. + merged.Reportf(pass) } } } diff --git a/tools/checkescape/test1/test1.go b/tools/checkescape/test1/test1.go index 68d3f72cc..27991649f 100644 --- a/tools/checkescape/test1/test1.go +++ b/tools/checkescape/test1/test1.go @@ -17,7 +17,6 @@ package test1 import ( "fmt" - "reflect" ) // Interface is a generic interface. @@ -163,20 +162,6 @@ func dynamicRec(f func()) { Dynamic(f) } -// +mustescape:local,unknown -//go:noinline -//go:nosplit -func Unknown() { - _ = reflect.TypeOf((*Type)(nil)) // Does not actually escape. -} - -// +mustescape:unknown -//go:noinline -//go:nosplit -func unknownRec() { - Unknown() -} - //go:noinline //go:nosplit func internalFunc() { @@ -190,6 +175,7 @@ func Split() { // +mustescape:stack //go:noinline +//go:nosplit func splitRec() { Split() } diff --git a/tools/checkescape/test2/test2.go b/tools/checkescape/test2/test2.go index 7fce3e3be..067d5a1f4 100644 --- a/tools/checkescape/test2/test2.go +++ b/tools/checkescape/test2/test2.go @@ -81,14 +81,9 @@ func dynamicCrossPkg(f func()) { test1.Dynamic(f) } -// +mustescape:unknown -//go:noinline -func unknownCrossPkg() { - test1.Unknown() -} - // +mustescape:stack //go:noinline +//go:nosplit func splitCrosssPkt() { test1.Split() } diff --git a/tools/defs.bzl b/tools/defs.bzl index e71a26cf4..d75e40863 100644 --- a/tools/defs.bzl +++ b/tools/defs.bzl @@ -7,55 +7,90 @@ change for Google-internal and bazel-compatible rules. load("//tools/go_stateify:defs.bzl", "go_stateify") load("//tools/go_marshal:defs.bzl", "go_marshal", "marshal_deps", "marshal_test_deps") -load("//tools/bazeldefs:defs.bzl", _build_test = "build_test", _bzl_library = "bzl_library", _cc_binary = "cc_binary", _cc_flags_supplier = "cc_flags_supplier", _cc_grpc_library = "cc_grpc_library", _cc_library = "cc_library", _cc_proto_library = "cc_proto_library", _cc_test = "cc_test", _cc_toolchain = "cc_toolchain", _default_installer = "default_installer", _default_net_util = "default_net_util", _gazelle = "gazelle", _gbenchmark = "gbenchmark", _go_binary = "go_binary", _go_embed_data = "go_embed_data", _go_grpc_and_proto_libraries = "go_grpc_and_proto_libraries", _go_library = "go_library", _go_path = "go_path", _go_proto_library = "go_proto_library", _go_test = "go_test", _grpcpp = "grpcpp", _gtest = "gtest", _loopback = "loopback", _pkg_deb = "pkg_deb", _pkg_tar = "pkg_tar", _proto_library = "proto_library", _py_binary = "py_binary", _rbe_platform = "rbe_platform", _rbe_toolchain = "rbe_toolchain", _select_arch = "select_arch", _select_system = "select_system", _short_path = "short_path", _vdso_linker_option = "vdso_linker_option") +load("//tools/nogo:defs.bzl", "nogo_test") +load("//tools/bazeldefs:defs.bzl", _build_test = "build_test", _bzl_library = "bzl_library", _coreutil = "coreutil", _default_installer = "default_installer", _default_net_util = "default_net_util", _loopback = "loopback", _proto_library = "proto_library", _rbe_platform = "rbe_platform", _rbe_toolchain = "rbe_toolchain", _select_arch = "select_arch", _select_system = "select_system", _short_path = "short_path") +load("//tools/bazeldefs:cc.bzl", _cc_binary = "cc_binary", _cc_flags_supplier = "cc_flags_supplier", _cc_grpc_library = "cc_grpc_library", _cc_library = "cc_library", _cc_proto_library = "cc_proto_library", _cc_test = "cc_test", _cc_toolchain = "cc_toolchain", _gbenchmark = "gbenchmark", _grpcpp = "grpcpp", _gtest = "gtest", _vdso_linker_option = "vdso_linker_option") +load("//tools/bazeldefs:go.bzl", _gazelle = "gazelle", _go_binary = "go_binary", _go_embed_data = "go_embed_data", _go_grpc_and_proto_libraries = "go_grpc_and_proto_libraries", _go_library = "go_library", _go_path = "go_path", _go_proto_library = "go_proto_library", _go_test = "go_test", _select_goarch = "select_goarch", _select_goos = "select_goos") +load("//tools/bazeldefs:pkg.bzl", _pkg_deb = "pkg_deb", _pkg_tar = "pkg_tar") load("//tools/bazeldefs:platforms.bzl", _default_platform = "default_platform", _platforms = "platforms") load("//tools/bazeldefs:tags.bzl", "go_suffixes") -load("//tools/nogo:defs.bzl", "nogo_test") -# Delegate directly. +# Core rules. build_test = _build_test bzl_library = _bzl_library +default_installer = _default_installer +default_net_util = _default_net_util +loopback = _loopback +select_arch = _select_arch +select_system = _select_system +short_path = _short_path +rbe_platform = _rbe_platform +rbe_toolchain = _rbe_toolchain +coreutil = _coreutil + +# C++ rules. cc_binary = _cc_binary cc_flags_supplier = _cc_flags_supplier cc_grpc_library = _cc_grpc_library cc_library = _cc_library cc_test = _cc_test cc_toolchain = _cc_toolchain -default_installer = _default_installer -default_net_util = _default_net_util gbenchmark = _gbenchmark +gtest = _gtest +grpcpp = _grpcpp +vdso_linker_option = _vdso_linker_option + +# Go rules. gazelle = _gazelle go_embed_data = _go_embed_data go_path = _go_path -go_test = _go_test -gtest = _gtest -grpcpp = _grpcpp -loopback = _loopback +select_goos = _select_goos +select_goarch = _select_goarch + +# Packaging rules. pkg_deb = _pkg_deb pkg_tar = _pkg_tar -py_binary = _py_binary -select_arch = _select_arch -select_system = _select_system -short_path = _short_path -rbe_platform = _rbe_platform -rbe_toolchain = _rbe_toolchain -vdso_linker_option = _vdso_linker_option # Platform options. default_platform = _default_platform platforms = _platforms -def go_binary(name, **kwargs): +def go_binary(name, nogo = True, pure = False, static = False, x_defs = None, **kwargs): """Wraps the standard go_binary. Args: name: the rule name. + nogo: enable nogo analysis. + pure: build a pure Go (no CGo) binary. + static: build a static binary. + x_defs: additional linker definitions. **kwargs: standard go_binary arguments. """ _go_binary( name = name, + pure = pure, + static = static, + x_defs = x_defs, **kwargs ) + if nogo: + # Note that the nogo rule applies only for go_library and go_test + # targets, therefore we construct a library from the binary sources. + # This is done because the binary may not be in a form that objdump + # supports (i.e. a pure Go binary). + _go_library( + name = name + "_nogo_library", + srcs = kwargs.get("srcs", []), + deps = kwargs.get("deps", []), + testonly = 1, + ) + nogo_test( + name = name + "_nogo", + config = "//:nogo_config", + srcs = kwargs.get("srcs", []), + deps = [":" + name + "_nogo_library"], + tags = ["nogo"], + ) def calculate_sets(srcs): """Calculates special Go sets for templates. @@ -119,6 +154,7 @@ def go_library(name, srcs, deps = [], imports = [], stateify = True, marshal = F stateify: whether statify is enabled (default: true). marshal: whether marshal is enabled (default: false). marshal_debug: whether the gomarshal tools emits debugging output (default: false). + nogo: enable nogo analysis. **kwargs: standard go_library arguments. """ all_srcs = srcs @@ -184,7 +220,10 @@ def go_library(name, srcs, deps = [], imports = [], stateify = True, marshal = F if nogo: nogo_test( name = name + "_nogo", + config = "//:nogo_config", + srcs = all_srcs, deps = [":" + name], + tags = ["nogo"], ) if marshal: @@ -196,12 +235,36 @@ def go_library(name, srcs, deps = [], imports = [], stateify = True, marshal = F for (suffix, _) in marshal_sets.items(): _go_test( name = name + suffix + "_abi_autogen_test", - srcs = [name + suffix + "_abi_autogen_test.go"], + srcs = [ + name + suffix + "_abi_autogen_test.go", + name + suffix + "_abi_autogen_unconditional_test.go", + ], library = ":" + name, deps = marshal_test_deps, **kwargs ) +def go_test(name, nogo = True, **kwargs): + """Wraps the standard go_test. + + Args: + name: the rule name. + nogo: enable nogo analysis. + **kwargs: standard go_test arguments. + """ + _go_test( + name = name, + **kwargs + ) + if nogo: + nogo_test( + name = name + "_nogo", + config = "//:nogo_config", + srcs = kwargs.get("srcs", []), + deps = [":" + name], + tags = ["nogo"], + ) + def proto_library(name, srcs, deps = None, has_services = 0, **kwargs): """Wraps the standard proto_library. diff --git a/tools/github/BUILD b/tools/github/BUILD new file mode 100644 index 000000000..aad088d13 --- /dev/null +++ b/tools/github/BUILD @@ -0,0 +1,15 @@ +load("//tools:defs.bzl", "go_binary") + +package(licenses = ["notice"]) + +go_binary( + name = "github", + srcs = ["main.go"], + nogo = False, + deps = [ + "//tools/github/nogo", + "//tools/github/reviver", + "@com_github_google_go_github_v28//github:go_default_library", + "@org_golang_x_oauth2//:go_default_library", + ], +) diff --git a/tools/github/main.go b/tools/github/main.go new file mode 100644 index 000000000..681003eef --- /dev/null +++ b/tools/github/main.go @@ -0,0 +1,186 @@ +// 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. + +// Binary github is the entry point for GitHub utilities. +package main + +import ( + "context" + "flag" + "fmt" + "io/ioutil" + "log" + "os" + "os/exec" + "strings" + + "github.com/google/go-github/github" + "golang.org/x/oauth2" + "gvisor.dev/gvisor/tools/github/nogo" + "gvisor.dev/gvisor/tools/github/reviver" +) + +var ( + owner string + repo string + tokenFile string + paths stringList + commit string + dryRun bool +) + +type stringList []string + +func (s *stringList) String() string { + return strings.Join(*s, ",") +} + +func (s *stringList) Set(value string) error { + *s = append(*s, value) + return nil +} + +// Keep the options simple for now. Supports only a single path and repo. +func init() { + flag.StringVar(&owner, "owner", "", "GitHub project org/owner (required, except nogo dry-run)") + flag.StringVar(&repo, "repo", "", "GitHub repo (required, except nogo dry-run)") + flag.StringVar(&tokenFile, "oauth-token-file", "", "file containing the GitHub token (or GITHUB_TOKEN is set)") + flag.Var(&paths, "path", "path(s) to scan (required for revive and nogo)") + flag.StringVar(&commit, "commit", "", "commit to associated (required for nogo, except dry-run)") + flag.BoolVar(&dryRun, "dry-run", false, "just print changes to be made") +} + +func filterPaths(paths []string) (existing []string) { + for _, path := range paths { + if _, err := os.Stat(path); err != nil { + log.Printf("WARNING: skipping %v: %v", path, err) + continue + } + existing = append(existing, path) + } + return +} + +func main() { + // Set defaults from the environment. + repository := os.Getenv("GITHUB_REPOSITORY") + if parts := strings.SplitN(repository, "/", 2); len(parts) == 2 { + owner = parts[0] + repo = parts[1] + } + + // Parse flags. + flag.Usage = func() { + fmt.Fprintf(flag.CommandLine.Output(), "usage: %s [options] <command>\n", os.Args[0]) + fmt.Fprintf(flag.CommandLine.Output(), "commands: revive, nogo\n") + flag.PrintDefaults() + } + flag.Parse() + args := flag.Args() + if len(args) != 1 { + fmt.Fprintf(flag.CommandLine.Output(), "extra arguments: %s\n", strings.Join(args[1:], ", ")) + flag.Usage() + os.Exit(1) + } + + // Check for mandatory parameters. + command := args[0] + if len(owner) == 0 && (command != "nogo" || !dryRun) { + fmt.Fprintln(flag.CommandLine.Output(), "missing --owner option.") + flag.Usage() + os.Exit(1) + } + if len(repo) == 0 && (command != "nogo" || !dryRun) { + fmt.Fprintln(flag.CommandLine.Output(), "missing --repo option.") + flag.Usage() + os.Exit(1) + } + filteredPaths := filterPaths(paths) + if len(filteredPaths) == 0 { + fmt.Fprintln(flag.CommandLine.Output(), "no valid --path options provided.") + flag.Usage() + os.Exit(1) + } + + // The access token may be passed as a file so it doesn't show up in + // command line arguments. It also may be provided through the + // environment to faciliate use through GitHub's CI system. + token := os.Getenv("GITHUB_TOKEN") + if len(tokenFile) != 0 { + bytes, err := ioutil.ReadFile(tokenFile) + if err != nil { + fmt.Println(err.Error()) + os.Exit(1) + } + token = string(bytes) + } + var client *github.Client + if len(token) == 0 { + // Client is unauthenticated. + client = github.NewClient(nil) + } else { + // Using the above token. + ts := oauth2.StaticTokenSource( + &oauth2.Token{AccessToken: token}, + ) + tc := oauth2.NewClient(context.Background(), ts) + client = github.NewClient(tc) + } + + switch command { + case "revive": + // Load existing GitHub bugs. + bugger, err := reviver.NewGitHubBugger(client, owner, repo, dryRun) + if err != nil { + fmt.Fprintf(os.Stderr, "Error getting github issues: %v\n", err) + os.Exit(1) + } + // Scan the provided path. + rev := reviver.New(filteredPaths, []reviver.Bugger{bugger}) + if errs := rev.Run(); len(errs) > 0 { + fmt.Fprintf(os.Stderr, "Encountered %d errors:\n", len(errs)) + for _, err := range errs { + fmt.Fprintf(os.Stderr, "\t%v\n", err) + } + os.Exit(1) + } + case "nogo": + // Did we get a commit? Try to extract one. + if len(commit) == 0 && !dryRun { + cmd := exec.Command("git", "rev-parse", "HEAD") + revBytes, err := cmd.Output() + if err != nil { + fmt.Fprintf(flag.CommandLine.Output(), "missing --commit option, unable to infer: %v\n", err) + flag.Usage() + os.Exit(1) + } + commit = strings.TrimSpace(string(revBytes)) + } + // Scan all findings. + poster := nogo.NewFindingsPoster(client, owner, repo, commit, dryRun) + if err := poster.Walk(filteredPaths); err != nil { + fmt.Fprintln(os.Stderr, "Error finding nogo findings:", err) + os.Exit(1) + } + // Post to GitHub. + if err := poster.Post(); err != nil { + fmt.Fprintln(os.Stderr, "Error posting nogo findings:", err) + } + default: + // Not a known command. + fmt.Fprintf(flag.CommandLine.Output(), "unknown command: %s\n", command) + flag.Usage() + os.Exit(1) + } +} diff --git a/tools/github/nogo/BUILD b/tools/github/nogo/BUILD new file mode 100644 index 000000000..19b7eec4d --- /dev/null +++ b/tools/github/nogo/BUILD @@ -0,0 +1,16 @@ +load("//tools:defs.bzl", "go_library") + +package(licenses = ["notice"]) + +go_library( + name = "nogo", + srcs = ["nogo.go"], + nogo = False, + visibility = [ + "//tools/github:__subpackages__", + ], + deps = [ + "//tools/nogo", + "@com_github_google_go_github_v28//github:go_default_library", + ], +) diff --git a/tools/github/nogo/nogo.go b/tools/github/nogo/nogo.go new file mode 100644 index 000000000..27ab1b8eb --- /dev/null +++ b/tools/github/nogo/nogo.go @@ -0,0 +1,132 @@ +// 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. + +// Package nogo provides nogo-related utilities. +package nogo + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "github.com/google/go-github/github" + "gvisor.dev/gvisor/tools/nogo" +) + +// FindingsPoster is a simple wrapper around the GitHub api. +type FindingsPoster struct { + owner string + repo string + commit string + dryRun bool + startTime time.Time + + findings map[nogo.Finding]struct{} + client *github.Client +} + +// NewFindingsPoster returns a object that can post findings. +func NewFindingsPoster(client *github.Client, owner, repo, commit string, dryRun bool) *FindingsPoster { + return &FindingsPoster{ + owner: owner, + repo: repo, + commit: commit, + dryRun: dryRun, + startTime: time.Now(), + findings: make(map[nogo.Finding]struct{}), + client: client, + } +} + +// Walk walks the given path tree for findings files. +func (p *FindingsPoster) Walk(paths []string) error { + for _, path := range paths { + if err := filepath.Walk(path, func(filename string, info os.FileInfo, err error) error { + if err != nil { + return err + } + // Skip any directories or files not ending in .findings. + if !strings.HasSuffix(filename, ".findings") || info.IsDir() { + return nil + } + findings, err := nogo.ExtractFindingsFromFile(filename) + if err != nil { + return err + } + // Add all findings to the list. We use a map to ensure + // that each finding is unique. + for _, finding := range findings { + p.findings[finding] = struct{}{} + } + return nil + }); err != nil { + return err + } + } + return nil +} + +// Post posts all results to the GitHub API as a check run. +func (p *FindingsPoster) Post() error { + // Just show results? + if p.dryRun { + for finding, _ := range p.findings { + // Pretty print, so that this is useful for debugging. + fmt.Printf("%s: (%s+%d) %s\n", finding.Category, finding.Position.Filename, finding.Position.Line, finding.Message) + } + return nil + } + + // Construct the message. + title := "nogo" + count := len(p.findings) + status := "completed" + conclusion := "success" + if count > 0 { + conclusion = "failure" // Contains errors. + } + summary := fmt.Sprintf("%d findings.", count) + opts := github.CreateCheckRunOptions{ + Name: title, + HeadSHA: p.commit, + Status: &status, + Conclusion: &conclusion, + StartedAt: &github.Timestamp{p.startTime}, + CompletedAt: &github.Timestamp{time.Now()}, + Output: &github.CheckRunOutput{ + Title: &title, + Summary: &summary, + AnnotationsCount: &count, + }, + } + annotationLevel := "failure" // Always. + for finding, _ := range p.findings { + title := string(finding.Category) + opts.Output.Annotations = append(opts.Output.Annotations, &github.CheckRunAnnotation{ + Path: &finding.Position.Filename, + StartLine: &finding.Position.Line, + EndLine: &finding.Position.Line, + Message: &finding.Message, + Title: &title, + AnnotationLevel: &annotationLevel, + }) + } + + // Post to GitHub. + _, _, err := p.client.Checks.CreateCheckRun(context.Background(), p.owner, p.repo, opts) + return err +} diff --git a/tools/github/reviver/BUILD b/tools/github/reviver/BUILD new file mode 100644 index 000000000..7d78480a7 --- /dev/null +++ b/tools/github/reviver/BUILD @@ -0,0 +1,27 @@ +load("//tools:defs.bzl", "go_library", "go_test") + +package(licenses = ["notice"]) + +go_library( + name = "reviver", + srcs = [ + "github.go", + "reviver.go", + ], + nogo = False, + visibility = [ + "//tools/github:__subpackages__", + ], + deps = ["@com_github_google_go_github_v28//github:go_default_library"], +) + +go_test( + name = "reviver_test", + size = "small", + srcs = [ + "github_test.go", + "reviver_test.go", + ], + library = ":reviver", + nogo = False, +) diff --git a/tools/issue_reviver/github/github.go b/tools/github/reviver/github.go index 8ffd7e606..c4b624f2a 100644 --- a/tools/issue_reviver/github/github.go +++ b/tools/github/reviver/github.go @@ -12,8 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package github implements reviver.Bugger interface on top of Github issues. -package github +package reviver import ( "context" @@ -23,12 +22,10 @@ import ( "time" "github.com/google/go-github/github" - "golang.org/x/oauth2" - "gvisor.dev/gvisor/tools/issue_reviver/reviver" ) -// Bugger implements reviver.Bugger interface for github issues. -type Bugger struct { +// GitHubBugger implements Bugger interface for github issues. +type GitHubBugger struct { owner string repo string dryRun bool @@ -37,36 +34,25 @@ type Bugger struct { issues map[int]*github.Issue } -// NewBugger creates a new Bugger. -func NewBugger(token, owner, repo string, dryRun bool) (*Bugger, error) { - b := &Bugger{ +// NewGitHubBugger creates a new GitHubBugger. +func NewGitHubBugger(client *github.Client, owner, repo string, dryRun bool) (*GitHubBugger, error) { + b := &GitHubBugger{ owner: owner, repo: repo, dryRun: dryRun, issues: map[int]*github.Issue{}, + client: client, } - if err := b.load(token); err != nil { + if err := b.load(); err != nil { return nil, err } return b, nil } -func (b *Bugger) load(token string) error { - ctx := context.Background() - if len(token) == 0 { - fmt.Print("No OAUTH token provided, using unauthenticated account.\n") - b.client = github.NewClient(nil) - } else { - ts := oauth2.StaticTokenSource( - &oauth2.Token{AccessToken: token}, - ) - tc := oauth2.NewClient(ctx, ts) - b.client = github.NewClient(tc) - } - +func (b *GitHubBugger) load() error { err := processAllPages(func(listOpts github.ListOptions) (*github.Response, error) { opts := &github.IssueListByRepoOptions{State: "open", ListOptions: listOpts} - tmps, resp, err := b.client.Issues.ListByRepo(ctx, b.owner, b.repo, opts) + tmps, resp, err := b.client.Issues.ListByRepo(context.Background(), b.owner, b.repo, opts) if err != nil { return resp, err } @@ -83,8 +69,8 @@ func (b *Bugger) load(token string) error { return nil } -// Activate implements reviver.Bugger. -func (b *Bugger) Activate(todo *reviver.Todo) (bool, error) { +// Activate implements Bugger.Activate. +func (b *GitHubBugger) Activate(todo *Todo) (bool, error) { id, err := parseIssueNo(todo.Issue) if err != nil { return true, err @@ -135,13 +121,24 @@ func (b *Bugger) Activate(todo *reviver.Todo) (bool, error) { return true, nil } +var issuePrefixes = []string{ + "gvisor.dev/issue/", + "gvisor.dev/issues/", +} + // parseIssueNo parses the issue number out of the issue url. +// +// 0 is returned if url does not correspond to an issue. func parseIssueNo(url string) (int, error) { - const prefix = "gvisor.dev/issue/" - // First check if I can handle the TODO. - idStr := strings.TrimPrefix(url, prefix) - if len(url) == len(idStr) { + var idStr string + for _, p := range issuePrefixes { + if str := strings.TrimPrefix(url, p); len(str) < len(url) { + idStr = str + break + } + } + if len(idStr) == 0 { return 0, nil } diff --git a/tools/issue_reviver/github/github_test.go b/tools/github/reviver/github_test.go index a78b230ef..5df7e3624 100644 --- a/tools/issue_reviver/github/github_test.go +++ b/tools/github/reviver/github_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package github +package reviver import ( "testing" diff --git a/tools/issue_reviver/reviver/reviver.go b/tools/github/reviver/reviver.go index 2af7f0d59..2af7f0d59 100644 --- a/tools/issue_reviver/reviver/reviver.go +++ b/tools/github/reviver/reviver.go diff --git a/tools/issue_reviver/reviver/reviver_test.go b/tools/github/reviver/reviver_test.go index a9fb1f9f1..851306c9d 100644 --- a/tools/issue_reviver/reviver/reviver_test.go +++ b/tools/github/reviver/reviver_test.go @@ -33,6 +33,15 @@ func TestProcessLine(t *testing.T) { }, }, { + line: "// TODO(foobar.com/issues/123): comment, bla. blabla.", + want: &Todo{ + Issue: "foobar.com/issues/123", + Locations: []Location{ + {Comment: "comment, bla. blabla."}, + }, + }, + }, + { line: "// FIXME(b/123): internal bug", want: &Todo{ Issue: "b/123", diff --git a/tools/go_generics/defs.bzl b/tools/go_generics/defs.bzl index 33329cf28..ad97208a8 100644 --- a/tools/go_generics/defs.bzl +++ b/tools/go_generics/defs.bzl @@ -1,25 +1,32 @@ -"""Generics support via go_generics.""" +"""Generics support via go_generics. + +A Go template is similar to a go library, except that it has certain types that +can be replaced before usage. For example, one could define a templatized List +struct, whose elements are of type T, then instantiate that template for +T=segment, where "segment" is the concrete type. +""" TemplateInfo = provider( + "Information about a go_generics template.", fields = { + "unsafe": "whether the template requires unsafe code", "types": "required types", "opt_types": "optional types", "consts": "required consts", "opt_consts": "optional consts", "deps": "package dependencies", - "file": "merged template", + "template": "merged template source file", }, ) def _go_template_impl(ctx): srcs = ctx.files.srcs - output = ctx.outputs.out - - args = ["-o=%s" % output.path] + [f.path for f in srcs] + template = ctx.actions.declare_file(ctx.label.name + "_template.go") + args = ["-o=%s" % template.path] + [f.path for f in srcs] ctx.actions.run( inputs = srcs, - outputs = [output], + outputs = [template], mnemonic = "GoGenericsTemplate", progress_message = "Building Go template %s" % ctx.label, arguments = args, @@ -32,74 +39,48 @@ def _go_template_impl(ctx): consts = ctx.attr.consts, opt_consts = ctx.attr.opt_consts, deps = ctx.attr.deps, - file = output, + template = template, )] -""" -Generates a Go template from a set of Go files. - -A Go template is similar to a go library, except that it has certain types that -can be replaced before usage. For example, one could define a templatized List -struct, whose elements are of type T, then instantiate that template for -T=segment, where "segment" is the concrete type. - -Args: - name: the name of the template. - srcs: the list of source files that comprise the template. - types: the list of generic types in the template that are required to be specified. - opt_types: the list of generic types in the template that can but aren't required to be specified. - consts: the list of constants in the template that are required to be specified. - opt_consts: the list of constants in the template that can but aren't required to be specified. - deps: the list of dependencies. -""" go_template = rule( implementation = _go_template_impl, attrs = { - "srcs": attr.label_list(mandatory = True, allow_files = True), - "deps": attr.label_list(allow_files = True, cfg = "target"), - "types": attr.string_list(), - "opt_types": attr.string_list(), - "consts": attr.string_list(), - "opt_consts": attr.string_list(), + "srcs": attr.label_list(doc = "the list of source files that comprise the template", mandatory = True, allow_files = True), + "deps": attr.label_list(doc = "the standard dependency list", allow_files = True, cfg = "target"), + "types": attr.string_list(doc = "the list of generic types in the template that are required to be specified"), + "opt_types": attr.string_list(doc = "the list of generic types in the template that can but aren't required to be specified"), + "consts": attr.string_list(doc = "the list of constants in the template that are required to be specified"), + "opt_consts": attr.string_list(doc = "the list of constants in the template that can but aren't required to be specified"), "_tool": attr.label(executable = True, cfg = "host", default = Label("//tools/go_generics/go_merge")), }, - outputs = { - "out": "%{name}_template.go", - }, -) - -TemplateInstanceInfo = provider( - fields = { - "srcs": "source files", - }, ) def _go_template_instance_impl(ctx): - template = ctx.attr.template[TemplateInfo] + info = ctx.attr.template[TemplateInfo] output = ctx.outputs.out # Check that all required types are defined. - for t in template.types: + for t in info.types: if t not in ctx.attr.types: fail("Missing value for type %s in %s" % (t, ctx.attr.template.label)) # Check that all defined types are expected by the template. for t in ctx.attr.types: - if (t not in template.types) and (t not in template.opt_types): + if (t not in info.types) and (t not in info.opt_types): fail("Type %s it not a parameter to %s" % (t, ctx.attr.template.label)) # Check that all required consts are defined. - for t in template.consts: + for t in info.consts: if t not in ctx.attr.consts: fail("Missing value for constant %s in %s" % (t, ctx.attr.template.label)) # Check that all defined consts are expected by the template. for t in ctx.attr.consts: - if (t not in template.consts) and (t not in template.opt_consts): + if (t not in info.consts) and (t not in info.opt_consts): fail("Const %s it not a parameter to %s" % (t, ctx.attr.template.label)) # Build the argument list. - args = ["-i=%s" % template.file.path, "-o=%s" % output.path] + args = ["-i=%s" % info.template.path, "-o=%s" % output.path] if ctx.attr.package: args.append("-p=%s" % ctx.attr.package) @@ -117,7 +98,7 @@ def _go_template_instance_impl(ctx): args.append("-anon") ctx.actions.run( - inputs = [template.file], + inputs = [info.template], outputs = [output], mnemonic = "GoGenericsInstance", progress_message = "Building Go template instance %s" % ctx.label, @@ -125,35 +106,22 @@ def _go_template_instance_impl(ctx): executable = ctx.executable._tool, ) - return [TemplateInstanceInfo( - srcs = [output], + return [DefaultInfo( + files = depset([output]), )] -""" -Instantiates a Go template by replacing all generic types with concrete ones. - -Args: - name: the name of the template instance. - template: the label of the template to be instatiated. - prefix: a prefix to be added to globals in the template. - suffix: a suffix to be added to global in the template. - types: the map from generic type names to concrete ones. - consts: the map from constant names to their values. - imports: the map from imports used in types/consts to their import paths. - package: the name of the package the instantiated template will be compiled into. -""" go_template_instance = rule( implementation = _go_template_instance_impl, attrs = { - "template": attr.label(mandatory = True), - "prefix": attr.string(), - "suffix": attr.string(), - "types": attr.string_dict(), - "consts": attr.string_dict(), - "imports": attr.string_dict(), - "anon": attr.bool(mandatory = False, default = False), - "package": attr.string(mandatory = False), - "out": attr.output(mandatory = True), + "template": attr.label(doc = "the label of the template to be instantiated", mandatory = True), + "prefix": attr.string(doc = "a prefix to be added to globals in the template"), + "suffix": attr.string(doc = "a suffix to be added to globals in the template"), + "types": attr.string_dict(doc = "the map from generic type names to concrete ones"), + "consts": attr.string_dict(doc = "the map from constant names to their values"), + "imports": attr.string_dict(doc = "the map from imports used in types/consts to their import paths"), + "anon": attr.bool(doc = "whether anoymous fields should be processed", mandatory = False, default = False), + "package": attr.string(doc = "the package for the generated source file", mandatory = False), + "out": attr.output(doc = "output file", mandatory = True), "_tool": attr.label(executable = True, cfg = "host", default = Label("//tools/go_generics")), }, ) diff --git a/tools/go_generics/go_merge/main.go b/tools/go_generics/go_merge/main.go index f6a331123..e0345500f 100644 --- a/tools/go_generics/go_merge/main.go +++ b/tools/go_generics/go_merge/main.go @@ -77,6 +77,7 @@ func main() { // Create a new declaration slice with all imports at the top, merging any // redundant imports. imports := make(map[string]*ast.ImportSpec) + var importNames []string // Keep imports in the original order to get deterministic output. var anonImports []*ast.ImportSpec for _, d := range f.Decls { if g, ok := d.(*ast.GenDecl); ok && g.Tok == token.IMPORT { @@ -98,6 +99,7 @@ func main() { } } else { imports[n] = i + importNames = append(importNames, n) } } } @@ -112,8 +114,8 @@ func main() { Lparen: token.NoPos + 1, Specs: make([]ast.Spec, 0, l), } - for _, i := range imports { - d.Specs = append(d.Specs, i) + for _, i := range importNames { + d.Specs = append(d.Specs, imports[i]) } for _, i := range anonImports { d.Specs = append(d.Specs, i) diff --git a/tools/go_generics/imports.go b/tools/go_generics/imports.go index 148dc7216..90d3aa1e0 100644 --- a/tools/go_generics/imports.go +++ b/tools/go_generics/imports.go @@ -21,6 +21,7 @@ import ( "go/format" "go/parser" "go/token" + "sort" "strconv" "gvisor.dev/gvisor/tools/go_generics/globals" @@ -132,10 +133,17 @@ func updateImports(maps []mapValue, imports mapValue) (ast.Decl, error) { if len(importsUsed) == 0 { return nil, nil } + var names []string + for n := range importsUsed { + names = append(names, n) + } + // Sort the new imports for deterministic build outputs. + sort.Strings(names) // Create spec array for each new import. specs := make([]ast.Spec, 0, len(importsUsed)) - for _, i := range importsUsed { + for _, n := range names { + i := importsUsed[n] specs = append(specs, &ast.ImportSpec{ Name: &ast.Ident{Name: i.newName}, Path: &ast.BasicLit{Value: i.path}, diff --git a/tools/go_marshal/README.md b/tools/go_marshal/README.md index 68d759083..d8045c295 100644 --- a/tools/go_marshal/README.md +++ b/tools/go_marshal/README.md @@ -3,18 +3,19 @@ This package implements the go_marshal utility. # Overview `go_marshal` is a code generation utility similar to `go_stateify` for -automatically generating code to marshal go data structures to memory. +marshalling go data structures to and from memory. `go_marshal` attempts to improve on `binary.Write` and the sentry's -`binary.Marshal` by moving the go runtime reflection necessary to marshal a -struct to compile-time. +`binary.Marshal` by moving the expensive use of reflection from runtime to +compile-time. `go_marshal` automatically generates implementations for `marshal.Marshallable` -and `safemem.{Reader,Writer}`. Data structures that require custom serialization -will have manual implementations for these interfaces. +interface. Data structures that require custom serialization can be accomodated +through a manual implementation this interface. Data structures can be flagged for code generation by adding a struct-level -comment `// +marshal`. +comment `// +marshal`. For additional details and options, see the documentation +for the `marshal.Marshallable` interface. # Usage @@ -74,7 +75,7 @@ intended for ABI structs, which have these additional restrictions: dependent native pointer size. - Fields must either be a primitive integer type (`byte`, - `[u]int{8,16,32,64}`), or of a type that implements abi.Marshallable. + `[u]int{8,16,32,64}`), or of a type that implements `marshal.Marshallable`. - `int` and `uint` fields are not allowed. Use an explicitly-sized numeric type. @@ -112,3 +113,18 @@ The following are some guidelines for modifying the `go_marshal` tool: - No runtime reflection in the code generated for the marshallable interface. The entire point of the tool is to avoid runtime reflection. The generated tests may use reflection. + +## Debugging + +To enable debugging output from the go-marshal tool, use one of the following +options, depending on how go-marshal is being invoked: + +- Pass `--define gomarshal=verbose` to the bazel command. Note that this can + generate a lot of output depending on what's being compiled, as this will + enable debugging for all packages built by the command. + +- Set `marshal_debug = True` on the top-level `go_library` BUILD rule. + +- Set `debug = True` on the `go_marshal` BUILD rule. + +- Pass `-debug` to the go-marshal tool invocation. diff --git a/tools/go_marshal/defs.bzl b/tools/go_marshal/defs.bzl index 323e33882..f44f83eab 100644 --- a/tools/go_marshal/defs.bzl +++ b/tools/go_marshal/defs.bzl @@ -4,11 +4,13 @@ def _go_marshal_impl(ctx): """Execute the go_marshal tool.""" output = ctx.outputs.lib output_test = ctx.outputs.test + output_test_unconditional = ctx.outputs.test_unconditional # Run the marshal command. args = ["-output=%s" % output.path] - args += ["-pkg=%s" % ctx.attr.package] - args += ["-output_test=%s" % output_test.path] + args.append("-pkg=%s" % ctx.attr.package) + args.append("-output_test=%s" % output_test.path) + args.append("-output_test_unconditional=%s" % output_test_unconditional.path) if ctx.attr.debug: args += ["-debug"] @@ -18,7 +20,7 @@ def _go_marshal_impl(ctx): args += [f.path for f in src.files.to_list()] ctx.actions.run( inputs = ctx.files.srcs, - outputs = [output, output_test], + outputs = [output, output_test, output_test_unconditional], mnemonic = "GoMarshal", progress_message = "go_marshal: %s" % ctx.label, arguments = args, @@ -48,6 +50,7 @@ go_marshal = rule( outputs = { "lib": "%{name}_unsafe.go", "test": "%{name}_test.go", + "test_unconditional": "%{name}_unconditional_test.go", }, ) @@ -56,7 +59,7 @@ marshal_deps = [ "//pkg/gohacks", "//pkg/safecopy", "//pkg/usermem", - "//tools/go_marshal/marshal", + "//pkg/marshal", ] # marshal_test_deps are required by test targets. diff --git a/tools/go_marshal/gomarshal/generator.go b/tools/go_marshal/gomarshal/generator.go index 19bcd4e6a..4a53d25be 100644 --- a/tools/go_marshal/gomarshal/generator.go +++ b/tools/go_marshal/gomarshal/generator.go @@ -38,8 +38,8 @@ import ( // All recievers are single letters, so we don't allow import aliases to be a // single letter. var badIdents = []string{ - "addr", "blk", "buf", "dst", "dsts", "count", "err", "hdr", "idx", "inner", - "length", "limit", "ptr", "size", "src", "srcs", "task", "val", + "addr", "blk", "buf", "cc", "dst", "dsts", "count", "err", "hdr", "idx", + "inner", "length", "limit", "ptr", "size", "src", "srcs", "val", // All single-letter identifiers. } @@ -68,6 +68,8 @@ type Generator struct { output *os.File // Output file to write generated tests. outputTest *os.File + // Output file to write unconditionally generated tests. + outputTestUC *os.File // Package name for the generated file. pkg string // Set of extra packages to import in the generated file. @@ -75,21 +77,26 @@ type Generator struct { } // NewGenerator creates a new code Generator. -func NewGenerator(srcs []string, out, outTest, pkg string, imports []string) (*Generator, error) { +func NewGenerator(srcs []string, out, outTest, outTestUnconditional, pkg string, imports []string) (*Generator, error) { f, err := os.OpenFile(out, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) if err != nil { - return nil, fmt.Errorf("Couldn't open output file %q: %v", out, err) + return nil, fmt.Errorf("couldn't open output file %q: %w", out, err) } fTest, err := os.OpenFile(outTest, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) if err != nil { - return nil, fmt.Errorf("Couldn't open test output file %q: %v", out, err) + return nil, fmt.Errorf("couldn't open test output file %q: %w", out, err) + } + fTestUC, err := os.OpenFile(outTestUnconditional, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) + if err != nil { + return nil, fmt.Errorf("couldn't open unconditional test output file %q: %w", out, err) } g := Generator{ - inputs: srcs, - output: f, - outputTest: fTest, - pkg: pkg, - imports: newImportTable(), + inputs: srcs, + output: f, + outputTest: fTest, + outputTestUC: fTestUC, + pkg: pkg, + imports: newImportTable(), } for _, i := range imports { // All imports on the extra imports list are unconditionally marked as @@ -107,7 +114,7 @@ func NewGenerator(srcs []string, out, outTest, pkg string, imports []string) (*G g.imports.add("gvisor.dev/gvisor/pkg/gohacks") g.imports.add("gvisor.dev/gvisor/pkg/safecopy") g.imports.add("gvisor.dev/gvisor/pkg/usermem") - g.imports.add("gvisor.dev/gvisor/tools/go_marshal/marshal") + g.imports.add("gvisor.dev/gvisor/pkg/marshal") return &g, nil } @@ -174,7 +181,7 @@ func (g *Generator) parse() ([]*ast.File, []*token.FileSet, error) { f, err := parser.ParseFile(fset, path, nil, parser.ParseComments) if err != nil { // Not a valid input file? - return nil, nil, fmt.Errorf("Input %q can't be parsed: %v", path, err) + return nil, nil, fmt.Errorf("input %q can't be parsed: %w", path, err) } if debugEnabled() { @@ -454,6 +461,46 @@ func (g *Generator) Run() error { // source file. func (g *Generator) writeTests(ts []*testGenerator) error { var b sourceBuffer + + // Write the unconditional test file. This file is always compiled, + // regardless of what build tags were specified on the original input + // files. We use this file to guarantee we never end up with an empty test + // file, as that causes the build to fail with "no tests/benchmarks/examples + // found". + // + // There's no easy way to determine ahead of time if we'll end up with an + // empty build file since build constraints can arbitrarily cause some of + // the original types to be not defined. We also have no way to tell bazel + // to omit the entire test suite since the output files are already defined + // before go-marshal is called. + b.emit("// Automatically generated marshal tests. See tools/go_marshal.\n\n") + b.emit("package %s\n\n", g.pkg) + b.emit("func Example() {\n") + b.inIndent(func() { + b.emit("// This example is intentionally empty, and ensures this package contains at\n") + b.emit("// least one testable entity. go-marshal is forced to emit a test package if the\n") + b.emit("// input package is marked marshallable, but emitting no testable entities \n") + b.emit("// results in a build failure.\n") + }) + b.emit("}\n") + if err := b.write(g.outputTestUC); err != nil { + return err + } + + // Now generate the real test file that contains the real types we + // processed. These need to be conditionally compiled according to the build + // tags, as the original types may not be defined under all build + // configurations. + + b.reset() + b.emit("// Automatically generated marshal tests. See tools/go_marshal.\n\n") + + // Emit build tags. + if t := tags.Aggregate(g.inputs); len(t) > 0 { + b.emit(strings.Join(t.Lines(), "\n")) + b.emit("\n\n") + } + b.emit("package %s\n\n", g.pkg) if err := b.write(g.outputTest); err != nil { return err @@ -470,26 +517,6 @@ func (g *Generator) writeTests(ts []*testGenerator) error { } // Write test functions. - - // If we didn't generate any Marshallable implementations, we can't just - // emit an empty test file, since that causes the build to fail with "no - // tests/benchmarks/examples found". Unfortunately we can't signal bazel to - // omit the entire package since the outputs are already defined before - // go-marshal is called. If we'd otherwise emit an empty test suite, emit an - // empty example instead. - if len(ts) == 0 { - b.reset() - b.emit("func Example() {\n") - b.inIndent(func() { - b.emit("// This example is intentionally empty to ensure this file contains at least\n") - b.emit("// one testable entity. go-marshal is forced to emit a test file if a package\n") - b.emit("// is marked marshallable, but emitting a test file with no entities results\n") - b.emit("// in a build failure.\n") - }) - b.emit("}\n") - return b.write(g.outputTest) - } - for _, t := range ts { if err := t.write(g.outputTest); err != nil { return err diff --git a/tools/go_marshal/gomarshal/generator_interfaces.go b/tools/go_marshal/gomarshal/generator_interfaces.go index e3c3dac63..36447b86b 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces.go +++ b/tools/go_marshal/gomarshal/generator_interfaces.go @@ -43,8 +43,8 @@ type interfaceGenerator struct { // of t's interfaces. ms map[string]struct{} - // as records embedded fields in t that are potentially not packed. The key - // is the accessor for the field. + // as records fields in t that are potentially not packed. The key is the + // accessor for the field. as map[string]struct{} } @@ -224,7 +224,7 @@ func (g *interfaceGenerator) emitNoEscapeSliceDataPointer(srcPtr, dstVar string) func (g *interfaceGenerator) emitKeepAlive(ptrVar string) { g.emit("// Since we bypassed the compiler's escape analysis, indicate that %s\n", ptrVar) g.emit("// must live until the use above.\n") - g.emit("runtime.KeepAlive(%s)\n", ptrVar) + g.emit("runtime.KeepAlive(%s) // escapes: replaced by intrinsic.\n", ptrVar) } func (g *interfaceGenerator) expandBinaryExpr(b *strings.Builder, e *ast.BinaryExpr) { diff --git a/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go b/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go index 72ef03a22..7525b52da 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go @@ -102,11 +102,11 @@ func (g *interfaceGenerator) emitMarshallableForArrayNewtype(n *ast.Ident, a *as g.emit("// CopyOutN implements marshal.Marshallable.CopyOutN.\n") g.emit("//go:nosplit\n") - g.emit("func (%s *%s) CopyOutN(task marshal.Task, addr usermem.Addr, limit int) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyOutN(cc marshal.CopyContext, addr usermem.Addr, limit int) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := task.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") + g.emit("length, err := cc.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") g.emitKeepAlive(g.r) g.emit("return length, err\n") }) @@ -114,19 +114,19 @@ func (g *interfaceGenerator) emitMarshallableForArrayNewtype(n *ast.Ident, a *as g.emit("// CopyOut implements marshal.Marshallable.CopyOut.\n") g.emit("//go:nosplit\n") - g.emit("func (%s *%s) CopyOut(task marshal.Task, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyOut(cc marshal.CopyContext, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("return %s.CopyOutN(task, addr, %s.SizeBytes())\n", g.r, g.r) + g.emit("return %s.CopyOutN(cc, addr, %s.SizeBytes())\n", g.r, g.r) }) g.emit("}\n\n") g.emit("// CopyIn implements marshal.Marshallable.CopyIn.\n") g.emit("//go:nosplit\n") - g.emit("func (%s *%s) CopyIn(task marshal.Task, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyIn(cc marshal.CopyContext, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := task.CopyInBytes(addr, buf) // escapes: okay.\n") + g.emit("length, err := cc.CopyInBytes(addr, buf) // escapes: okay.\n") g.emitKeepAlive(g.r) g.emit("return length, err\n") }) diff --git a/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go b/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go index 39f654ea8..7edaf666c 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go @@ -154,11 +154,11 @@ func (g *interfaceGenerator) emitMarshallableForPrimitiveNewtype(nt *ast.Ident) g.emit("// CopyOutN implements marshal.Marshallable.CopyOutN.\n") g.emit("//go:nosplit\n") - g.emit("func (%s *%s) CopyOutN(task marshal.Task, addr usermem.Addr, limit int) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyOutN(cc marshal.CopyContext, addr usermem.Addr, limit int) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := task.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") + g.emit("length, err := cc.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") g.emitKeepAlive(g.r) g.emit("return length, err\n") }) @@ -166,19 +166,19 @@ func (g *interfaceGenerator) emitMarshallableForPrimitiveNewtype(nt *ast.Ident) g.emit("// CopyOut implements marshal.Marshallable.CopyOut.\n") g.emit("//go:nosplit\n") - g.emit("func (%s *%s) CopyOut(task marshal.Task, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyOut(cc marshal.CopyContext, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("return %s.CopyOutN(task, addr, %s.SizeBytes())\n", g.r, g.r) + g.emit("return %s.CopyOutN(cc, addr, %s.SizeBytes())\n", g.r, g.r) }) g.emit("}\n\n") g.emit("// CopyIn implements marshal.Marshallable.CopyIn.\n") g.emit("//go:nosplit\n") - g.emit("func (%s *%s) CopyIn(task marshal.Task, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyIn(cc marshal.CopyContext, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := task.CopyInBytes(addr, buf) // escapes: okay.\n") + g.emit("length, err := cc.CopyInBytes(addr, buf) // escapes: okay.\n") g.emitKeepAlive(g.r) g.emit("return length, err\n") }) @@ -211,7 +211,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Id g.emit("// Copy%sIn copies in a slice of %s objects from the task's memory.\n", slice.ident, eltType) g.emit("//go:nosplit\n") - g.emit("func Copy%sIn(task marshal.Task, addr usermem.Addr, dst []%s) (int, error) {\n", slice.ident, eltType) + g.emit("func Copy%sIn(cc marshal.CopyContext, addr usermem.Addr, dst []%s) (int, error) {\n", slice.ident, eltType) g.inIndent(func() { g.emit("count := len(dst)\n") g.emit("if count == 0 {\n") @@ -223,7 +223,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Id g.emitCastSliceToByteSlice("&dst", "buf", "size * count") - g.emit("length, err := task.CopyInBytes(addr, buf) // escapes: okay.\n") + g.emit("length, err := cc.CopyInBytes(addr, buf) // escapes: okay.\n") g.emitKeepAlive("dst") g.emit("return length, err\n") }) @@ -231,7 +231,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Id g.emit("// Copy%sOut copies a slice of %s objects to the task's memory.\n", slice.ident, eltType) g.emit("//go:nosplit\n") - g.emit("func Copy%sOut(task marshal.Task, addr usermem.Addr, src []%s) (int, error) {\n", slice.ident, eltType) + g.emit("func Copy%sOut(cc marshal.CopyContext, addr usermem.Addr, src []%s) (int, error) {\n", slice.ident, eltType) g.inIndent(func() { g.emit("count := len(src)\n") g.emit("if count == 0 {\n") @@ -243,7 +243,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Id g.emitCastSliceToByteSlice("&src", "buf", "size * count") - g.emit("length, err := task.CopyOutBytes(addr, buf) // escapes: okay.\n") + g.emit("length, err := cc.CopyOutBytes(addr, buf) // escapes: okay.\n") g.emitKeepAlive("src") g.emit("return length, err\n") }) diff --git a/tools/go_marshal/gomarshal/generator_interfaces_struct.go b/tools/go_marshal/gomarshal/generator_interfaces_struct.go index 4b9cea08a..fe76d3785 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_struct.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_struct.go @@ -20,6 +20,7 @@ package gomarshal import ( "fmt" "go/ast" + "sort" "strings" ) @@ -40,6 +41,8 @@ func (g *interfaceGenerator) areFieldsPackedExpression() (string, bool) { for accessor, _ := range g.as { cs = append(cs, fmt.Sprintf("%s.Packed()", accessor)) } + // Sort expressions for determinstic build outputs. + sort.Strings(cs) return strings.Join(cs, " && "), true } @@ -48,12 +51,6 @@ func (g *interfaceGenerator) areFieldsPackedExpression() (string, bool) { // later. func (g *interfaceGenerator) validateStruct(ts *ast.TypeSpec, st *ast.StructType) { forEachStructField(st, func(f *ast.Field) { - if len(f.Names) == 0 { - g.abortAt(f.Pos(), "Cannot marshal structs with embedded fields, give the field a name; use '_' for anonymous fields such as padding fields") - } - }) - - forEachStructField(st, func(f *ast.Field) { fieldDispatcher{ primitive: func(_, t *ast.Ident) { g.validatePrimitiveNewtype(t) @@ -98,7 +95,7 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { var dynamicSizeTerms []string forEachStructField(st, fieldDispatcher{ - primitive: func(n, t *ast.Ident) { + primitive: func(_, t *ast.Ident) { if size, dynamic := g.scalarSize(t); !dynamic { primitiveSize += size } else { @@ -106,13 +103,13 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { dynamicSizeTerms = append(dynamicSizeTerms, fmt.Sprintf("(*%s)(nil).SizeBytes()", t.Name)) } }, - selector: func(n, tX, tSel *ast.Ident) { + selector: func(_, tX, tSel *ast.Ident) { tName := fmt.Sprintf("%s.%s", tX.Name, tSel.Name) g.recordUsedImport(tX.Name) g.recordUsedMarshallable(tName) dynamicSizeTerms = append(dynamicSizeTerms, fmt.Sprintf("(*%s)(nil).SizeBytes()", tName)) }, - array: func(n *ast.Ident, a *ast.ArrayType, t *ast.Ident) { + array: func(_ *ast.Ident, a *ast.ArrayType, t *ast.Ident) { lenExpr := g.arrayLenExpr(a) if size, dynamic := g.scalarSize(t); !dynamic { dynamicSizeTerms = append(dynamicSizeTerms, fmt.Sprintf("%d*%s", size, lenExpr)) @@ -323,13 +320,13 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("//go:nosplit\n") g.recordUsedImport("marshal") g.recordUsedImport("usermem") - g.emit("func (%s *%s) CopyOutN(task marshal.Task, addr usermem.Addr, limit int) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyOutN(cc marshal.CopyContext, addr usermem.Addr, limit int) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { fallback := func() { g.emit("// Type %s doesn't have a packed layout in memory, fall back to MarshalBytes.\n", g.typeName()) - g.emit("buf := task.CopyScratchBuffer(%s.SizeBytes()) // escapes: okay.\n", g.r) + g.emit("buf := cc.CopyScratchBuffer(%s.SizeBytes()) // escapes: okay.\n", g.r) g.emit("%s.MarshalBytes(buf) // escapes: fallback.\n", g.r) - g.emit("return task.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") + g.emit("return cc.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") } if thisPacked { g.recordUsedImport("reflect") @@ -343,7 +340,7 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { // Fast serialization. g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := task.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") + g.emit("length, err := cc.CopyOutBytes(addr, buf[:limit]) // escapes: okay.\n") g.emitKeepAlive(g.r) g.emit("return length, err\n") } else { @@ -356,9 +353,9 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("//go:nosplit\n") g.recordUsedImport("marshal") g.recordUsedImport("usermem") - g.emit("func (%s *%s) CopyOut(task marshal.Task, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyOut(cc marshal.CopyContext, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("return %s.CopyOutN(task, addr, %s.SizeBytes())\n", g.r, g.r) + g.emit("return %s.CopyOutN(cc, addr, %s.SizeBytes())\n", g.r, g.r) }) g.emit("}\n\n") @@ -366,12 +363,12 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("//go:nosplit\n") g.recordUsedImport("marshal") g.recordUsedImport("usermem") - g.emit("func (%s *%s) CopyIn(task marshal.Task, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) CopyIn(cc marshal.CopyContext, addr usermem.Addr) (int, error) {\n", g.r, g.typeName()) g.inIndent(func() { fallback := func() { g.emit("// Type %s doesn't have a packed layout in memory, fall back to UnmarshalBytes.\n", g.typeName()) - g.emit("buf := task.CopyScratchBuffer(%s.SizeBytes()) // escapes: okay.\n", g.r) - g.emit("length, err := task.CopyInBytes(addr, buf) // escapes: okay.\n") + g.emit("buf := cc.CopyScratchBuffer(%s.SizeBytes()) // escapes: okay.\n", g.r) + g.emit("length, err := cc.CopyInBytes(addr, buf) // escapes: okay.\n") g.emit("// Unmarshal unconditionally. If we had a short copy-in, this results in a\n") g.emit("// partially unmarshalled struct.\n") g.emit("%s.UnmarshalBytes(buf) // escapes: fallback.\n", g.r) @@ -389,7 +386,7 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { // Fast deserialization. g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := task.CopyInBytes(addr, buf) // escapes: okay.\n") + g.emit("length, err := cc.CopyInBytes(addr, buf) // escapes: okay.\n") g.emitKeepAlive(g.r) g.emit("return length, err\n") } else { @@ -400,13 +397,13 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("// WriteTo implements io.WriterTo.WriteTo.\n") g.recordUsedImport("io") - g.emit("func (%s *%s) WriteTo(w io.Writer) (int64, error) {\n", g.r, g.typeName()) + g.emit("func (%s *%s) WriteTo(writer io.Writer) (int64, error) {\n", g.r, g.typeName()) g.inIndent(func() { fallback := func() { g.emit("// Type %s doesn't have a packed layout in memory, fall back to MarshalBytes.\n", g.typeName()) g.emit("buf := make([]byte, %s.SizeBytes())\n", g.r) g.emit("%s.MarshalBytes(buf)\n", g.r) - g.emit("length, err := w.Write(buf)\n") + g.emit("length, err := writer.Write(buf)\n") g.emit("return int64(length), err\n") } if thisPacked { @@ -421,7 +418,7 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { // Fast serialization. g.emitCastToByteSlice(g.r, "buf", fmt.Sprintf("%s.SizeBytes()", g.r)) - g.emit("length, err := w.Write(buf)\n") + g.emit("length, err := writer.Write(buf)\n") g.emitKeepAlive(g.r) g.emit("return int64(length), err\n") } else { @@ -442,7 +439,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, g.recordUsedImport("usermem") g.emit("// Copy%sIn copies in a slice of %s objects from the task's memory.\n", slice.ident, g.typeName()) - g.emit("func Copy%sIn(task marshal.Task, addr usermem.Addr, dst []%s) (int, error) {\n", slice.ident, g.typeName()) + g.emit("func Copy%sIn(cc marshal.CopyContext, addr usermem.Addr, dst []%s) (int, error) {\n", slice.ident, g.typeName()) g.inIndent(func() { g.emit("count := len(dst)\n") g.emit("if count == 0 {\n") @@ -454,8 +451,8 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, fallback := func() { g.emit("// Type %s doesn't have a packed layout in memory, fall back to UnmarshalBytes.\n", g.typeName()) - g.emit("buf := task.CopyScratchBuffer(size * count)\n") - g.emit("length, err := task.CopyInBytes(addr, buf)\n\n") + g.emit("buf := cc.CopyScratchBuffer(size * count)\n") + g.emit("length, err := cc.CopyInBytes(addr, buf)\n\n") g.emit("// Unmarshal as much as possible, even on error. First handle full objects.\n") g.emit("limit := length/size\n") @@ -489,7 +486,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, // Fast deserialization. g.emitCastSliceToByteSlice("&dst", "buf", "size * count") - g.emit("length, err := task.CopyInBytes(addr, buf)\n") + g.emit("length, err := cc.CopyInBytes(addr, buf)\n") g.emitKeepAlive("dst") g.emit("return length, err\n") } else { @@ -499,7 +496,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, g.emit("}\n\n") g.emit("// Copy%sOut copies a slice of %s objects to the task's memory.\n", slice.ident, g.typeName()) - g.emit("func Copy%sOut(task marshal.Task, addr usermem.Addr, src []%s) (int, error) {\n", slice.ident, g.typeName()) + g.emit("func Copy%sOut(cc marshal.CopyContext, addr usermem.Addr, src []%s) (int, error) {\n", slice.ident, g.typeName()) g.inIndent(func() { g.emit("count := len(src)\n") g.emit("if count == 0 {\n") @@ -511,13 +508,13 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, fallback := func() { g.emit("// Type %s doesn't have a packed layout in memory, fall back to MarshalBytes.\n", g.typeName()) - g.emit("buf := task.CopyScratchBuffer(size * count)\n") + g.emit("buf := cc.CopyScratchBuffer(size * count)\n") g.emit("for idx := 0; idx < count; idx++ {\n") g.inIndent(func() { g.emit("src[idx].MarshalBytes(buf[size*idx:size*(idx+1)])\n") }) g.emit("}\n") - g.emit("return task.CopyOutBytes(addr, buf)\n") + g.emit("return cc.CopyOutBytes(addr, buf)\n") } if thisPacked { g.recordUsedImport("reflect") @@ -531,7 +528,7 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, // Fast serialization. g.emitCastSliceToByteSlice("&src", "buf", "size * count") - g.emit("length, err := task.CopyOutBytes(addr, buf)\n") + g.emit("length, err := cc.CopyOutBytes(addr, buf)\n") g.emitKeepAlive("src") g.emit("return length, err\n") } else { diff --git a/tools/go_marshal/gomarshal/util.go b/tools/go_marshal/gomarshal/util.go index d94314302..6a42691cd 100644 --- a/tools/go_marshal/gomarshal/util.go +++ b/tools/go_marshal/gomarshal/util.go @@ -79,7 +79,7 @@ type fieldDispatcher struct { } // Precondition: All dispatch callbacks that will be invoked must be -// provided. Embedded fields are not allowed, len(f.Names) >= 1. +// provided. func (fd fieldDispatcher) dispatch(f *ast.Field) { // Each field declaration may actually be multiple declarations of the same // type. For example, consider: @@ -88,12 +88,24 @@ func (fd fieldDispatcher) dispatch(f *ast.Field) { // x, y, z int // } // - // We invoke the call-backs once per such instance. Embedded fields are not - // allowed, and results in a panic. + // We invoke the call-backs once per such instance. + + // Handle embedded fields. Embedded fields have no names, but can be + // referenced by the type name. if len(f.Names) < 1 { - panic("Precondition not met: attempted to dispatch on embedded field") + switch v := f.Type.(type) { + case *ast.Ident: + fd.primitive(v, v) + case *ast.SelectorExpr: + fd.selector(v.Sel, v.X.(*ast.Ident), v.Sel) + default: + // Note: Arrays can't be embedded, which is handled here. + panic(fmt.Sprintf("Attempted to dispatch on embedded field of unsupported kind: %#v", f.Type)) + } + return } + // Non-embedded field. for _, name := range f.Names { switch v := f.Type.(type) { case *ast.Ident: diff --git a/tools/go_marshal/main.go b/tools/go_marshal/main.go index f74be5c29..6e4a3e8c4 100644 --- a/tools/go_marshal/main.go +++ b/tools/go_marshal/main.go @@ -31,10 +31,11 @@ import ( ) var ( - pkg = flag.String("pkg", "", "output package") - output = flag.String("output", "", "output file") - outputTest = flag.String("output_test", "", "output file for tests") - imports = flag.String("imports", "", "comma-separated list of extra packages to import in generated code") + pkg = flag.String("pkg", "", "output package") + output = flag.String("output", "", "output file") + outputTest = flag.String("output_test", "", "output file for tests") + outputTestUnconditional = flag.String("output_test_unconditional", "", "output file for unconditional tests") + imports = flag.String("imports", "", "comma-separated list of extra packages to import in generated code") ) func main() { @@ -61,7 +62,7 @@ func main() { // as an import. extraImports = strings.Split(*imports, ",") } - g, err := gomarshal.NewGenerator(flag.Args(), *output, *outputTest, *pkg, extraImports) + g, err := gomarshal.NewGenerator(flag.Args(), *output, *outputTest, *outputTestUnconditional, *pkg, extraImports) if err != nil { panic(err) } diff --git a/tools/go_marshal/marshal/BUILD b/tools/go_marshal/marshal/BUILD deleted file mode 100644 index 4aec98218..000000000 --- a/tools/go_marshal/marshal/BUILD +++ /dev/null @@ -1,17 +0,0 @@ -load("//tools:defs.bzl", "go_library") - -licenses(["notice"]) - -go_library( - name = "marshal", - srcs = [ - "marshal.go", - "marshal_impl_util.go", - ], - visibility = [ - "//:sandbox", - ], - deps = [ - "//pkg/usermem", - ], -) diff --git a/tools/go_marshal/marshal/marshal.go b/tools/go_marshal/marshal/marshal.go deleted file mode 100644 index 85b196f08..000000000 --- a/tools/go_marshal/marshal/marshal.go +++ /dev/null @@ -1,183 +0,0 @@ -// 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. - -// Package marshal defines the Marshallable interface for -// serialize/deserializing go data structures to/from memory, according to the -// Linux ABI. -// -// Implementations of this interface are typically automatically generated by -// tools/go_marshal. See the go_marshal README for details. -package marshal - -import ( - "io" - - "gvisor.dev/gvisor/pkg/usermem" -) - -// Task provides a subset of kernel.Task, used in marshalling. We don't import -// the kernel package directly to avoid circular dependency. -type Task interface { - // CopyScratchBuffer provides a task goroutine-local scratch buffer. See - // kernel.CopyScratchBuffer. - CopyScratchBuffer(size int) []byte - - // CopyOutBytes writes the contents of b to the task's memory. See - // kernel.CopyOutBytes. - CopyOutBytes(addr usermem.Addr, b []byte) (int, error) - - // CopyInBytes reads the contents of the task's memory to b. See - // kernel.CopyInBytes. - CopyInBytes(addr usermem.Addr, b []byte) (int, error) -} - -// Marshallable represents operations on a type that can be marshalled to and -// from memory. -// -// go-marshal automatically generates implementations for this interface for -// types marked as '+marshal'. -type Marshallable interface { - io.WriterTo - - // SizeBytes is the size of the memory representation of a type in - // marshalled form. - // - // SizeBytes must handle a nil receiver. Practically, this means SizeBytes - // cannot deference any fields on the object implementing it (but will - // likely make use of the type of these fields). - SizeBytes() int - - // MarshalBytes serializes a copy of a type to dst. - // Precondition: dst must be at least SizeBytes() in length. - MarshalBytes(dst []byte) - - // UnmarshalBytes deserializes a type from src. - // Precondition: src must be at least SizeBytes() in length. - UnmarshalBytes(src []byte) - - // Packed returns true if the marshalled size of the type is the same as the - // size it occupies in memory. This happens when the type has no fields - // starting at unaligned addresses (should always be true by default for ABI - // structs, verified by automatically generated tests when using - // go_marshal), and has no fields marked `marshal:"unaligned"`. - // - // Packed must return the same result for all possible values of the type - // implementing it. Violating this constraint implies the type doesn't have - // a static memory layout, and will lead to memory corruption. - // Go-marshal-generated code reuses the result of Packed for multiple values - // of the same type. - Packed() bool - - // MarshalUnsafe serializes a type by bulk copying its in-memory - // representation to the dst buffer. This is only safe to do when the type - // has no implicit padding, see Marshallable.Packed. When Packed would - // return false, MarshalUnsafe should fall back to the safer but slower - // MarshalBytes. - // Precondition: dst must be at least SizeBytes() in length. - MarshalUnsafe(dst []byte) - - // UnmarshalUnsafe deserializes a type by directly copying to the underlying - // memory allocated for the object by the runtime. - // - // This allows much faster unmarshalling of types which have no implicit - // padding, see Marshallable.Packed. When Packed would return false, - // UnmarshalUnsafe should fall back to the safer but slower unmarshal - // mechanism implemented in UnmarshalBytes. - // Precondition: src must be at least SizeBytes() in length. - UnmarshalUnsafe(src []byte) - - // CopyIn deserializes a Marshallable type from a task's memory. This may - // only be called from a task goroutine. This is more efficient than calling - // UnmarshalUnsafe on Marshallable.Packed types, as the type being - // marshalled does not escape. The implementation should avoid creating - // extra copies in memory by directly deserializing to the object's - // underlying memory. - // - // If the copy-in from the task memory is only partially successful, CopyIn - // should still attempt to deserialize as much data as possible. See comment - // for UnmarshalBytes. - CopyIn(task Task, addr usermem.Addr) (int, error) - - // CopyOut serializes a Marshallable type to a task's memory. This may only - // be called from a task goroutine. This is more efficient than calling - // MarshalUnsafe on Marshallable.Packed types, as the type being serialized - // does not escape. The implementation should avoid creating extra copies in - // memory by directly serializing from the object's underlying memory. - // - // The copy-out to the task memory may be partially successful, in which - // case CopyOut returns how much data was serialized. See comment for - // MarshalBytes for implications. - CopyOut(task Task, addr usermem.Addr) (int, error) - - // CopyOutN is like CopyOut, but explicitly requests a partial - // copy-out. Note that this may yield unexpected results for non-packed - // types and the caller may only want to allow this for packed types. See - // comment on MarshalBytes. - // - // The limit must be less than or equal to SizeBytes(). - CopyOutN(task Task, addr usermem.Addr, limit int) (int, error) -} - -// go-marshal generates additional functions for a type based on additional -// clauses to the +marshal directive. They are documented below. -// -// Slice API -// ========= -// -// Adding a "slice" clause to the +marshal directive for structs or newtypes on -// primitives like this: -// -// // +marshal slice:FooSlice -// type Foo struct { ... } -// -// Generates four additional functions for marshalling slices of Foos like this: -// -// // MarshalUnsafeFooSlice is like Foo.MarshalUnsafe, buf for a []Foo. It -// // might be more efficient that repeatedly calling Foo.MarshalUnsafe -// // over a []Foo in a loop if the type is Packed. -// // Preconditions: dst must be at least len(src)*Foo.SizeBytes() in length. -// func MarshalUnsafeFooSlice(src []Foo, dst []byte) (int, error) { ... } -// -// // UnmarshalUnsafeFooSlice is like Foo.UnmarshalUnsafe, buf for a []Foo. It -// // might be more efficient that repeatedly calling Foo.UnmarshalUnsafe -// // over a []Foo in a loop if the type is Packed. -// // Preconditions: src must be at least len(dst)*Foo.SizeBytes() in length. -// func UnmarshalUnsafeFooSlice(dst []Foo, src []byte) (int, error) { ... } -// -// // CopyFooSliceIn copies in a slice of Foo objects from the task's memory. -// func CopyFooSliceIn(task marshal.Task, addr usermem.Addr, dst []Foo) (int, error) { ... } -// -// // CopyFooSliceIn copies out a slice of Foo objects to the task's memory. -// func CopyFooSliceOut(task marshal.Task, addr usermem.Addr, src []Foo) (int, error) { ... } -// -// The name of the functions are of the format "Copy%sIn" and "Copy%sOut", where -// %s is the first argument to the slice clause. This directive is not supported -// for newtypes on arrays. -// -// The slice clause also takes an optional second argument, which must be the -// value "inner": -// -// // +marshal slice:Int32Slice:inner -// type Int32 int32 -// -// This is only valid on newtypes on primitives, and causes the generated -// functions to accept slices of the inner type instead: -// -// func CopyInt32SliceIn(task marshal.Task, addr usermem.Addr, dst []int32) (int, error) { ... } -// -// Without "inner", they would instead be: -// -// func CopyInt32SliceIn(task marshal.Task, addr usermem.Addr, dst []Int32) (int, error) { ... } -// -// This may help avoid a cast depending on how the generated functions are used. diff --git a/tools/go_marshal/marshal/marshal_impl_util.go b/tools/go_marshal/marshal/marshal_impl_util.go deleted file mode 100644 index 89c7d3575..000000000 --- a/tools/go_marshal/marshal/marshal_impl_util.go +++ /dev/null @@ -1,78 +0,0 @@ -// Copyright 2020 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 marshal - -import ( - "io" - - "gvisor.dev/gvisor/pkg/usermem" -) - -// StubMarshallable implements the Marshallable interface. -// StubMarshallable is a convenient embeddable type for satisfying the -// marshallable interface, but provides no actual implementation. It is -// useful when the marshallable interface needs to be implemented manually, -// but the caller doesn't require the full marshallable interface. -type StubMarshallable struct{} - -// WriteTo implements Marshallable.WriteTo. -func (StubMarshallable) WriteTo(w io.Writer) (n int64, err error) { - panic("Please implement your own WriteTo function") -} - -// SizeBytes implements Marshallable.SizeBytes. -func (StubMarshallable) SizeBytes() int { - panic("Please implement your own SizeBytes function") -} - -// MarshalBytes implements Marshallable.MarshalBytes. -func (StubMarshallable) MarshalBytes(dst []byte) { - panic("Please implement your own MarshalBytes function") -} - -// UnmarshalBytes implements Marshallable.UnmarshalBytes. -func (StubMarshallable) UnmarshalBytes(src []byte) { - panic("Please implement your own UnMarshalBytes function") -} - -// Packed implements Marshallable.Packed. -func (StubMarshallable) Packed() bool { - panic("Please implement your own Packed function") -} - -// MarshalUnsafe implements Marshallable.MarshalUnsafe. -func (StubMarshallable) MarshalUnsafe(dst []byte) { - panic("Please implement your own MarshalUnsafe function") -} - -// UnmarshalUnsafe implements Marshallable.UnmarshalUnsafe. -func (StubMarshallable) UnmarshalUnsafe(src []byte) { - panic("Please implement your own UnmarshalUnsafe function") -} - -// CopyIn implements Marshallable.CopyIn. -func (StubMarshallable) CopyIn(task Task, addr usermem.Addr) (int, error) { - panic("Please implement your own CopyIn function") -} - -// CopyOut implements Marshallable.CopyOut. -func (StubMarshallable) CopyOut(task Task, addr usermem.Addr) (int, error) { - panic("Please implement your own CopyOut function") -} - -// CopyOutN implements Marshallable.CopyOutN. -func (StubMarshallable) CopyOutN(task Task, addr usermem.Addr, limit int) (int, error) { - panic("Please implement your own CopyOutN function") -} diff --git a/tools/go_marshal/primitive/BUILD b/tools/go_marshal/primitive/BUILD deleted file mode 100644 index cc08ba63a..000000000 --- a/tools/go_marshal/primitive/BUILD +++ /dev/null @@ -1,18 +0,0 @@ -load("//tools:defs.bzl", "go_library") - -licenses(["notice"]) - -go_library( - name = "primitive", - srcs = [ - "primitive.go", - ], - marshal = True, - visibility = [ - "//:sandbox", - ], - deps = [ - "//pkg/usermem", - "//tools/go_marshal/marshal", - ], -) diff --git a/tools/go_marshal/primitive/primitive.go b/tools/go_marshal/primitive/primitive.go deleted file mode 100644 index d93edda8b..000000000 --- a/tools/go_marshal/primitive/primitive.go +++ /dev/null @@ -1,247 +0,0 @@ -// Copyright 2020 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 primitive defines marshal.Marshallable implementations for primitive -// types. -package primitive - -import ( - "io" - - "gvisor.dev/gvisor/pkg/usermem" - "gvisor.dev/gvisor/tools/go_marshal/marshal" -) - -// Int8 is a marshal.Marshallable implementation for int8. -// -// +marshal slice:Int8Slice:inner -type Int8 int8 - -// Uint8 is a marshal.Marshallable implementation for uint8. -// -// +marshal slice:Uint8Slice:inner -type Uint8 uint8 - -// Int16 is a marshal.Marshallable implementation for int16. -// -// +marshal slice:Int16Slice:inner -type Int16 int16 - -// Uint16 is a marshal.Marshallable implementation for uint16. -// -// +marshal slice:Uint16Slice:inner -type Uint16 uint16 - -// Int32 is a marshal.Marshallable implementation for int32. -// -// +marshal slice:Int32Slice:inner -type Int32 int32 - -// Uint32 is a marshal.Marshallable implementation for uint32. -// -// +marshal slice:Uint32Slice:inner -type Uint32 uint32 - -// Int64 is a marshal.Marshallable implementation for int64. -// -// +marshal slice:Int64Slice:inner -type Int64 int64 - -// Uint64 is a marshal.Marshallable implementation for uint64. -// -// +marshal slice:Uint64Slice:inner -type Uint64 uint64 - -// ByteSlice is a marshal.Marshallable implementation for []byte. -// This is a convenience wrapper around a dynamically sized type, and can't be -// embedded in other marshallable types because it breaks assumptions made by -// go-marshal internals. It violates the "no dynamically-sized types" -// constraint of the go-marshal library. -type ByteSlice []byte - -// SizeBytes implements marshal.Marshallable.SizeBytes. -func (b *ByteSlice) SizeBytes() int { - return len(*b) -} - -// MarshalBytes implements marshal.Marshallable.MarshalBytes. -func (b *ByteSlice) MarshalBytes(dst []byte) { - copy(dst, *b) -} - -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (b *ByteSlice) UnmarshalBytes(src []byte) { - copy(*b, src) -} - -// Packed implements marshal.Marshallable.Packed. -func (b *ByteSlice) Packed() bool { - return false -} - -// MarshalUnsafe implements marshal.Marshallable.MarshalUnsafe. -func (b *ByteSlice) MarshalUnsafe(dst []byte) { - b.MarshalBytes(dst) -} - -// UnmarshalUnsafe implements marshal.Marshallable.UnmarshalUnsafe. -func (b *ByteSlice) UnmarshalUnsafe(src []byte) { - b.UnmarshalBytes(src) -} - -// CopyIn implements marshal.Marshallable.CopyIn. -func (b *ByteSlice) CopyIn(task marshal.Task, addr usermem.Addr) (int, error) { - return task.CopyInBytes(addr, *b) -} - -// CopyOut implements marshal.Marshallable.CopyOut. -func (b *ByteSlice) CopyOut(task marshal.Task, addr usermem.Addr) (int, error) { - return task.CopyOutBytes(addr, *b) -} - -// CopyOutN implements marshal.Marshallable.CopyOutN. -func (b *ByteSlice) CopyOutN(task marshal.Task, addr usermem.Addr, limit int) (int, error) { - return task.CopyOutBytes(addr, (*b)[:limit]) -} - -// WriteTo implements io.WriterTo.WriteTo. -func (b *ByteSlice) WriteTo(w io.Writer) (int64, error) { - n, err := w.Write(*b) - return int64(n), err -} - -var _ marshal.Marshallable = (*ByteSlice)(nil) - -// Below, we define some convenience functions for marshalling primitive types -// using the newtypes above, without requiring superfluous casts. - -// 16-bit integers - -// CopyInt16In is a convenient wrapper for copying in an int16 from the task's -// memory. -func CopyInt16In(task marshal.Task, addr usermem.Addr, dst *int16) (int, error) { - var buf Int16 - n, err := buf.CopyIn(task, addr) - if err != nil { - return n, err - } - *dst = int16(buf) - return n, nil -} - -// CopyInt16Out is a convenient wrapper for copying out an int16 to the task's -// memory. -func CopyInt16Out(task marshal.Task, addr usermem.Addr, src int16) (int, error) { - srcP := Int16(src) - return srcP.CopyOut(task, addr) -} - -// CopyUint16In is a convenient wrapper for copying in a uint16 from the task's -// memory. -func CopyUint16In(task marshal.Task, addr usermem.Addr, dst *uint16) (int, error) { - var buf Uint16 - n, err := buf.CopyIn(task, addr) - if err != nil { - return n, err - } - *dst = uint16(buf) - return n, nil -} - -// CopyUint16Out is a convenient wrapper for copying out a uint16 to the task's -// memory. -func CopyUint16Out(task marshal.Task, addr usermem.Addr, src uint16) (int, error) { - srcP := Uint16(src) - return srcP.CopyOut(task, addr) -} - -// 32-bit integers - -// CopyInt32In is a convenient wrapper for copying in an int32 from the task's -// memory. -func CopyInt32In(task marshal.Task, addr usermem.Addr, dst *int32) (int, error) { - var buf Int32 - n, err := buf.CopyIn(task, addr) - if err != nil { - return n, err - } - *dst = int32(buf) - return n, nil -} - -// CopyInt32Out is a convenient wrapper for copying out an int32 to the task's -// memory. -func CopyInt32Out(task marshal.Task, addr usermem.Addr, src int32) (int, error) { - srcP := Int32(src) - return srcP.CopyOut(task, addr) -} - -// CopyUint32In is a convenient wrapper for copying in a uint32 from the task's -// memory. -func CopyUint32In(task marshal.Task, addr usermem.Addr, dst *uint32) (int, error) { - var buf Uint32 - n, err := buf.CopyIn(task, addr) - if err != nil { - return n, err - } - *dst = uint32(buf) - return n, nil -} - -// CopyUint32Out is a convenient wrapper for copying out a uint32 to the task's -// memory. -func CopyUint32Out(task marshal.Task, addr usermem.Addr, src uint32) (int, error) { - srcP := Uint32(src) - return srcP.CopyOut(task, addr) -} - -// 64-bit integers - -// CopyInt64In is a convenient wrapper for copying in an int64 from the task's -// memory. -func CopyInt64In(task marshal.Task, addr usermem.Addr, dst *int64) (int, error) { - var buf Int64 - n, err := buf.CopyIn(task, addr) - if err != nil { - return n, err - } - *dst = int64(buf) - return n, nil -} - -// CopyInt64Out is a convenient wrapper for copying out an int64 to the task's -// memory. -func CopyInt64Out(task marshal.Task, addr usermem.Addr, src int64) (int, error) { - srcP := Int64(src) - return srcP.CopyOut(task, addr) -} - -// CopyUint64In is a convenient wrapper for copying in a uint64 from the task's -// memory. -func CopyUint64In(task marshal.Task, addr usermem.Addr, dst *uint64) (int, error) { - var buf Uint64 - n, err := buf.CopyIn(task, addr) - if err != nil { - return n, err - } - *dst = uint64(buf) - return n, nil -} - -// CopyUint64Out is a convenient wrapper for copying out a uint64 to the task's -// memory. -func CopyUint64Out(task marshal.Task, addr usermem.Addr, src uint64) (int, error) { - srcP := Uint64(src) - return srcP.CopyOut(task, addr) -} diff --git a/tools/go_marshal/test/BUILD b/tools/go_marshal/test/BUILD index 3d989823a..4b27773c2 100644 --- a/tools/go_marshal/test/BUILD +++ b/tools/go_marshal/test/BUILD @@ -35,10 +35,10 @@ go_test( srcs = ["marshal_test.go"], deps = [ ":test", + "//pkg/marshal", "//pkg/syserror", "//pkg/usermem", "//tools/go_marshal/analysis", - "//tools/go_marshal/marshal", "@com_github_google_go_cmp//cmp:go_default_library", ], ) diff --git a/tools/go_marshal/test/escape/BUILD b/tools/go_marshal/test/escape/BUILD index f74e6ffae..2981ef196 100644 --- a/tools/go_marshal/test/escape/BUILD +++ b/tools/go_marshal/test/escape/BUILD @@ -7,8 +7,8 @@ go_library( testonly = 1, srcs = ["escape.go"], deps = [ + "//pkg/marshal", "//pkg/usermem", - "//tools/go_marshal/marshal", "//tools/go_marshal/test", ], ) diff --git a/tools/go_marshal/test/escape/escape.go b/tools/go_marshal/test/escape/escape.go index 6a46ddbf8..7f62b0a2b 100644 --- a/tools/go_marshal/test/escape/escape.go +++ b/tools/go_marshal/test/escape/escape.go @@ -15,34 +15,34 @@ package escape import ( + "gvisor.dev/gvisor/pkg/marshal" "gvisor.dev/gvisor/pkg/usermem" - "gvisor.dev/gvisor/tools/go_marshal/marshal" "gvisor.dev/gvisor/tools/go_marshal/test" ) -// dummyTask implements marshal.Task. -type dummyTask struct { +// dummyCopyContext implements marshal.CopyContext. +type dummyCopyContext struct { } -func (*dummyTask) CopyScratchBuffer(size int) []byte { +func (*dummyCopyContext) CopyScratchBuffer(size int) []byte { return make([]byte, size) } -func (*dummyTask) CopyOutBytes(addr usermem.Addr, b []byte) (int, error) { +func (*dummyCopyContext) CopyOutBytes(addr usermem.Addr, b []byte) (int, error) { return len(b), nil } -func (*dummyTask) CopyInBytes(addr usermem.Addr, b []byte) (int, error) { +func (*dummyCopyContext) CopyInBytes(addr usermem.Addr, b []byte) (int, error) { return len(b), nil } -func (t *dummyTask) MarshalBytes(addr usermem.Addr, marshallable marshal.Marshallable) { +func (t *dummyCopyContext) MarshalBytes(addr usermem.Addr, marshallable marshal.Marshallable) { buf := t.CopyScratchBuffer(marshallable.SizeBytes()) marshallable.MarshalBytes(buf) t.CopyOutBytes(addr, buf) } -func (t *dummyTask) MarshalUnsafe(addr usermem.Addr, marshallable marshal.Marshallable) { +func (t *dummyCopyContext) MarshalUnsafe(addr usermem.Addr, marshallable marshal.Marshallable) { buf := t.CopyScratchBuffer(marshallable.SizeBytes()) marshallable.MarshalUnsafe(buf) t.CopyOutBytes(addr, buf) @@ -50,21 +50,22 @@ func (t *dummyTask) MarshalUnsafe(addr usermem.Addr, marshallable marshal.Marsha // +checkescape:all //go:nosplit -func doCopyIn(t *dummyTask) { +func doCopyIn(t *dummyCopyContext) { var stat test.Stat stat.CopyIn(t, usermem.Addr(0xf000ba12)) } // +checkescape:all //go:nosplit -func doCopyOut(t *dummyTask) { +func doCopyOut(t *dummyCopyContext) { var stat test.Stat stat.CopyOut(t, usermem.Addr(0xf000ba12)) } // +mustescape:builtin // +mustescape:stack -func doMarshalBytesDirect(t *dummyTask) { +//go:nosplit +func doMarshalBytesDirect(t *dummyCopyContext) { var stat test.Stat buf := t.CopyScratchBuffer(stat.SizeBytes()) stat.MarshalBytes(buf) @@ -73,7 +74,8 @@ func doMarshalBytesDirect(t *dummyTask) { // +mustescape:builtin // +mustescape:stack -func doMarshalUnsafeDirect(t *dummyTask) { +//go:nosplit +func doMarshalUnsafeDirect(t *dummyCopyContext) { var stat test.Stat buf := t.CopyScratchBuffer(stat.SizeBytes()) stat.MarshalUnsafe(buf) @@ -82,14 +84,16 @@ func doMarshalUnsafeDirect(t *dummyTask) { // +mustescape:local,heap // +mustescape:stack -func doMarshalBytesViaMarshallable(t *dummyTask) { +//go:nosplit +func doMarshalBytesViaMarshallable(t *dummyCopyContext) { var stat test.Stat t.MarshalBytes(usermem.Addr(0xf000ba12), &stat) } // +mustescape:local,heap // +mustescape:stack -func doMarshalUnsafeViaMarshallable(t *dummyTask) { +//go:nosplit +func doMarshalUnsafeViaMarshallable(t *dummyCopyContext) { var stat test.Stat t.MarshalUnsafe(usermem.Addr(0xf000ba12), &stat) } diff --git a/tools/go_marshal/test/marshal_test.go b/tools/go_marshal/test/marshal_test.go index 16829ee45..a00f9a684 100644 --- a/tools/go_marshal/test/marshal_test.go +++ b/tools/go_marshal/test/marshal_test.go @@ -27,22 +27,22 @@ import ( "unsafe" "github.com/google/go-cmp/cmp" + "gvisor.dev/gvisor/pkg/marshal" "gvisor.dev/gvisor/pkg/syserror" "gvisor.dev/gvisor/pkg/usermem" "gvisor.dev/gvisor/tools/go_marshal/analysis" - "gvisor.dev/gvisor/tools/go_marshal/marshal" "gvisor.dev/gvisor/tools/go_marshal/test" ) var simulatedErr error = syserror.EFAULT -// mockTask implements marshal.Task. -type mockTask struct { +// mockCopyContext implements marshal.CopyContext. +type mockCopyContext struct { taskMem usermem.BytesIO } // populate fills the task memory with the contents of val. -func (t *mockTask) populate(val interface{}) { +func (t *mockCopyContext) populate(val interface{}) { var buf bytes.Buffer // Use binary.Write so we aren't testing go-marshal against its own // potentially buggy implementation. @@ -52,7 +52,7 @@ func (t *mockTask) populate(val interface{}) { t.taskMem.Bytes = buf.Bytes() } -func (t *mockTask) setLimit(n int) { +func (t *mockCopyContext) setLimit(n int) { if len(t.taskMem.Bytes) < n { grown := make([]byte, n) copy(grown, t.taskMem.Bytes) @@ -62,22 +62,22 @@ func (t *mockTask) setLimit(n int) { t.taskMem.Bytes = t.taskMem.Bytes[:n] } -// CopyScratchBuffer implements marshal.Task.CopyScratchBuffer. -func (t *mockTask) CopyScratchBuffer(size int) []byte { +// CopyScratchBuffer implements marshal.CopyContext.CopyScratchBuffer. +func (t *mockCopyContext) CopyScratchBuffer(size int) []byte { return make([]byte, size) } -// CopyOutBytes implements marshal.Task.CopyOutBytes. The implementation +// CopyOutBytes implements marshal.CopyContext.CopyOutBytes. The implementation // completely ignores the target address and stores a copy of b in its // internally buffer, overriding any previous contents. -func (t *mockTask) CopyOutBytes(_ usermem.Addr, b []byte) (int, error) { +func (t *mockCopyContext) CopyOutBytes(_ usermem.Addr, b []byte) (int, error) { return t.taskMem.CopyOut(nil, 0, b, usermem.IOOpts{}) } -// CopyInBytes implements marshal.Task.CopyInBytes. The implementation +// CopyInBytes implements marshal.CopyContext.CopyInBytes. The implementation // completely ignores the source address and always fills b from the begining of // its internal buffer. -func (t *mockTask) CopyInBytes(_ usermem.Addr, b []byte) (int, error) { +func (t *mockCopyContext) CopyInBytes(_ usermem.Addr, b []byte) (int, error) { return t.taskMem.CopyIn(nil, 0, b, usermem.IOOpts{}) } @@ -171,11 +171,11 @@ func compareMemory(t *testing.T, expected, actual []byte, n int) { // dst. The task signals an error at limit bytes during copy-in, which should // result in a truncated unmarshalling. func limitedCopyIn(t *testing.T, src, dst marshal.Marshallable, limit int) { - var task mockTask - task.populate(src) - task.setLimit(limit) + var cc mockCopyContext + cc.populate(src) + cc.setLimit(limit) - n, err := dst.CopyIn(&task, usermem.Addr(0)) + n, err := dst.CopyIn(&cc, usermem.Addr(0)) if n != limit { t.Errorf("CopyIn copied unexpected number of bytes, expected %d, got %d", limit, n) } @@ -202,10 +202,10 @@ func limitedCopyIn(t *testing.T, src, dst marshal.Marshallable, limit int) { // limitedCopyOut marshals src to task memory. The task signals an error at // limit bytes during copy-out, which should result in a truncated marshalling. func limitedCopyOut(t *testing.T, src marshal.Marshallable, limit int) { - var task mockTask - task.setLimit(limit) + var cc mockCopyContext + cc.setLimit(limit) - n, err := src.CopyOut(&task, usermem.Addr(0)) + n, err := src.CopyOut(&cc, usermem.Addr(0)) if n != limit { t.Errorf("CopyOut copied unexpected number of bytes, expected %d, got %d", limit, n) } @@ -215,7 +215,7 @@ func limitedCopyOut(t *testing.T, src marshal.Marshallable, limit int) { expectedMem := unsafeMemory(src) defer runtime.KeepAlive(src) - actualMem := task.taskMem.Bytes + actualMem := cc.taskMem.Bytes compareMemory(t, expectedMem, actualMem, n) } @@ -223,10 +223,10 @@ func limitedCopyOut(t *testing.T, src marshal.Marshallable, limit int) { // copyOutN marshals src to task memory, requesting the marshalling to be // limited to limit bytes. func copyOutN(t *testing.T, src marshal.Marshallable, limit int) { - var task mockTask - task.setLimit(limit) + var cc mockCopyContext + cc.setLimit(limit) - n, err := src.CopyOutN(&task, usermem.Addr(0), limit) + n, err := src.CopyOutN(&cc, usermem.Addr(0), limit) if err != nil { t.Errorf("CopyOut returned unexpected error: %v", err) } @@ -236,7 +236,7 @@ func copyOutN(t *testing.T, src marshal.Marshallable, limit int) { expectedMem := unsafeMemory(src) defer runtime.KeepAlive(src) - actualMem := task.taskMem.Bytes + actualMem := cc.taskMem.Bytes t.Logf("Expected: %v + %v\n", expectedMem[:n], expectedMem[n:]) t.Logf("Actual : %v + %v\n", actualMem[:n], actualMem[n:]) @@ -303,20 +303,20 @@ func TestLimitedMarshalling(t *testing.T) { func TestLimitedSliceMarshalling(t *testing.T) { types := []struct { arrayPtrType reflect.Type - copySliceIn func(task marshal.Task, addr usermem.Addr, dstSlice interface{}) (int, error) - copySliceOut func(task marshal.Task, addr usermem.Addr, srcSlice interface{}) (int, error) + copySliceIn func(cc marshal.CopyContext, addr usermem.Addr, dstSlice interface{}) (int, error) + copySliceOut func(cc marshal.CopyContext, addr usermem.Addr, srcSlice interface{}) (int, error) unsafeMemory func(arrPtr interface{}) []byte }{ // Packed types. { reflect.TypeOf((*[20]test.Stat)(nil)), - func(task marshal.Task, addr usermem.Addr, dst interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, dst interface{}) (int, error) { slice := dst.(*[20]test.Stat)[:] - return test.CopyStatSliceIn(task, addr, slice) + return test.CopyStatSliceIn(cc, addr, slice) }, - func(task marshal.Task, addr usermem.Addr, src interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, src interface{}) (int, error) { slice := src.(*[20]test.Stat)[:] - return test.CopyStatSliceOut(task, addr, slice) + return test.CopyStatSliceOut(cc, addr, slice) }, func(a interface{}) []byte { slice := a.(*[20]test.Stat)[:] @@ -325,13 +325,13 @@ func TestLimitedSliceMarshalling(t *testing.T) { }, { reflect.TypeOf((*[1]test.Stat)(nil)), - func(task marshal.Task, addr usermem.Addr, dst interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, dst interface{}) (int, error) { slice := dst.(*[1]test.Stat)[:] - return test.CopyStatSliceIn(task, addr, slice) + return test.CopyStatSliceIn(cc, addr, slice) }, - func(task marshal.Task, addr usermem.Addr, src interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, src interface{}) (int, error) { slice := src.(*[1]test.Stat)[:] - return test.CopyStatSliceOut(task, addr, slice) + return test.CopyStatSliceOut(cc, addr, slice) }, func(a interface{}) []byte { slice := a.(*[1]test.Stat)[:] @@ -340,13 +340,13 @@ func TestLimitedSliceMarshalling(t *testing.T) { }, { reflect.TypeOf((*[5]test.SignalSetAlias)(nil)), - func(task marshal.Task, addr usermem.Addr, dst interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, dst interface{}) (int, error) { slice := dst.(*[5]test.SignalSetAlias)[:] - return test.CopySignalSetAliasSliceIn(task, addr, slice) + return test.CopySignalSetAliasSliceIn(cc, addr, slice) }, - func(task marshal.Task, addr usermem.Addr, src interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, src interface{}) (int, error) { slice := src.(*[5]test.SignalSetAlias)[:] - return test.CopySignalSetAliasSliceOut(task, addr, slice) + return test.CopySignalSetAliasSliceOut(cc, addr, slice) }, func(a interface{}) []byte { slice := a.(*[5]test.SignalSetAlias)[:] @@ -356,13 +356,13 @@ func TestLimitedSliceMarshalling(t *testing.T) { // Non-packed types. { reflect.TypeOf((*[20]test.Type1)(nil)), - func(task marshal.Task, addr usermem.Addr, dst interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, dst interface{}) (int, error) { slice := dst.(*[20]test.Type1)[:] - return test.CopyType1SliceIn(task, addr, slice) + return test.CopyType1SliceIn(cc, addr, slice) }, - func(task marshal.Task, addr usermem.Addr, src interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, src interface{}) (int, error) { slice := src.(*[20]test.Type1)[:] - return test.CopyType1SliceOut(task, addr, slice) + return test.CopyType1SliceOut(cc, addr, slice) }, func(a interface{}) []byte { slice := a.(*[20]test.Type1)[:] @@ -371,13 +371,13 @@ func TestLimitedSliceMarshalling(t *testing.T) { }, { reflect.TypeOf((*[1]test.Type1)(nil)), - func(task marshal.Task, addr usermem.Addr, dst interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, dst interface{}) (int, error) { slice := dst.(*[1]test.Type1)[:] - return test.CopyType1SliceIn(task, addr, slice) + return test.CopyType1SliceIn(cc, addr, slice) }, - func(task marshal.Task, addr usermem.Addr, src interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, src interface{}) (int, error) { slice := src.(*[1]test.Type1)[:] - return test.CopyType1SliceOut(task, addr, slice) + return test.CopyType1SliceOut(cc, addr, slice) }, func(a interface{}) []byte { slice := a.(*[1]test.Type1)[:] @@ -386,13 +386,13 @@ func TestLimitedSliceMarshalling(t *testing.T) { }, { reflect.TypeOf((*[7]test.Type8)(nil)), - func(task marshal.Task, addr usermem.Addr, dst interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, dst interface{}) (int, error) { slice := dst.(*[7]test.Type8)[:] - return test.CopyType8SliceIn(task, addr, slice) + return test.CopyType8SliceIn(cc, addr, slice) }, - func(task marshal.Task, addr usermem.Addr, src interface{}) (int, error) { + func(cc marshal.CopyContext, addr usermem.Addr, src interface{}) (int, error) { slice := src.(*[7]test.Type8)[:] - return test.CopyType8SliceOut(task, addr, slice) + return test.CopyType8SliceOut(cc, addr, slice) }, func(a interface{}) []byte { slice := a.(*[7]test.Type8)[:] @@ -439,11 +439,11 @@ func TestLimitedSliceMarshalling(t *testing.T) { limit += elem.SizeBytes() / 2 analysis.RandomizeValue(expected) - var task mockTask - task.populate(expected) - task.setLimit(limit) + var cc mockCopyContext + cc.populate(expected) + cc.setLimit(limit) - n, err := tt.copySliceIn(&task, usermem.Addr(0), actual) + n, err := tt.copySliceIn(&cc, usermem.Addr(0), actual) if n != limit { t.Errorf("CopyIn copied unexpected number of bytes, expected %d, got %d", limit, n) } @@ -493,11 +493,11 @@ func TestLimitedSliceMarshalling(t *testing.T) { limit += elem.SizeBytes() / 2 analysis.RandomizeValue(expected) - var task mockTask - task.populate(expected) - task.setLimit(limit) + var cc mockCopyContext + cc.populate(expected) + cc.setLimit(limit) - n, err := tt.copySliceOut(&task, usermem.Addr(0), expected) + n, err := tt.copySliceOut(&cc, usermem.Addr(0), expected) if n != limit { t.Errorf("CopyIn copied unexpected number of bytes, expected %d, got %d", limit, n) } @@ -507,7 +507,7 @@ func TestLimitedSliceMarshalling(t *testing.T) { expectedMem := tt.unsafeMemory(expected) defer runtime.KeepAlive(expected) - actualMem := task.taskMem.Bytes + actualMem := cc.taskMem.Bytes compareMemory(t, expectedMem, actualMem, n) }) diff --git a/tools/go_marshal/test/test.go b/tools/go_marshal/test/test.go index f75ca1b7f..d9e9f341b 100644 --- a/tools/go_marshal/test/test.go +++ b/tools/go_marshal/test/test.go @@ -174,3 +174,27 @@ type Type9 struct { x int64 y [sizeA]int32 } + +// Type10Embed is a test data type which is be embedded into another type. +// +// +marshal +type Type10Embed struct { + x int64 +} + +// Type10 is a test data type which contains an embedded struct. +// +// +marshal +type Type10 struct { + Type10Embed + y int64 +} + +// Type11 is a test data type which contains an embedded struct from an external +// package. +// +// +marshal +type Type11 struct { + ex.External + y int64 +} diff --git a/tools/go_stateify/main.go b/tools/go_stateify/main.go index 4f6ed208a..e1de12e25 100644 --- a/tools/go_stateify/main.go +++ b/tools/go_stateify/main.go @@ -39,7 +39,7 @@ var ( ) // resolveTypeName returns a qualified type name. -func resolveTypeName(name string, typ ast.Expr) (field string, qualified string) { +func resolveTypeName(typ ast.Expr) (field string, qualified string) { for done := false; !done; { // Resolve star expressions. switch rs := typ.(type) { @@ -69,11 +69,7 @@ func resolveTypeName(name string, typ ast.Expr) (field string, qualified string) } // Figure out actual type name. - ident, ok := typ.(*ast.Ident) - if !ok { - panic(fmt.Sprintf("type not supported: %s (involves anonymous types?)", name)) - } - field = ident.Name + field = typ.(*ast.Ident).Name qualified = qualified + field return } @@ -119,7 +115,7 @@ func scanFields(ss *ast.StructType, prefix string, fn scanFunctions) { } else { // Anonymous types can't be embedded, so we don't need // to worry about providing a useful name here. - name, _ = resolveTypeName("", field.Type) + name, _ = resolveTypeName(field.Type) } // Skip _ fields. @@ -214,9 +210,6 @@ func main() { emitRegister := func(name string) { initCalls = append(initCalls, fmt.Sprintf("%sRegister((*%s)(nil))", statePrefix, name)) } - emitZeroCheck := func(name string) { - fmt.Fprintf(outputFile, " if !%sIsZeroValue(&x.%s) { %sFailf(\"%s is %%#v, expected zero\", &x.%s) }\n", statePrefix, name, statePrefix, name, name) - } // Automated warning. fmt.Fprint(outputFile, "// automatically generated by stateify.\n\n") @@ -265,52 +258,39 @@ func main() { } type method struct { - receiver string - name string + typeName string + methodName string } - // Search for and add all methods with a pointer receiver and no other - // arguments to a set. We support auto-detecting the existence of - // several different methods with this signature. - simpleMethods := map[method]struct{}{} + // Search for and add all method to a set. We auto-detecting several + // different methods (and insert them if we don't find them, in order + // to ensure that expectations match reality). + // + // While we do this, figure out the right receiver name. If there are + // multiple distinct receivers, then we will just pick the last one. + simpleMethods := make(map[method]struct{}) + receiverNames := make(map[string]string) for _, f := range files { - // Go over all functions. for _, decl := range f.Decls { d, ok := decl.(*ast.FuncDecl) if !ok { continue } - if d.Name == nil || d.Recv == nil || d.Type == nil { + if d.Recv == nil || len(d.Recv.List) != 1 { // Not a named method. continue } - if len(d.Recv.List) != 1 { - // Wrong number of receivers? - continue - } - if d.Type.Params != nil && len(d.Type.Params.List) != 0 { - // Has argument(s). - continue - } - if d.Type.Results != nil && len(d.Type.Results.List) != 0 { - // Has return(s). - continue - } - pt, ok := d.Recv.List[0].Type.(*ast.StarExpr) - if !ok { - // Not a pointer receiver. - continue + // Save the method and the receiver. + name, _ := resolveTypeName(d.Recv.List[0].Type) + simpleMethods[method{ + typeName: name, + methodName: d.Name.Name, + }] = struct{}{} + if len(d.Recv.List[0].Names) > 0 { + receiverNames[name] = d.Recv.List[0].Names[0].Name } - - t, ok := pt.X.(*ast.Ident) - if !ok { - // This shouldn't happen with valid Go. - continue - } - - simpleMethods[method{t.Name, d.Name.Name}] = struct{}{} } } @@ -349,6 +329,11 @@ func main() { for _, gs := range d.Specs { ts := gs.(*ast.TypeSpec) + recv, ok := receiverNames[ts.Name.Name] + if !ok { + // Maybe no methods were defined? + recv = strings.ToLower(ts.Name.Name[:1]) + } switch x := ts.Type.(type) { case *ast.StructType: maybeEmitImports() @@ -365,29 +350,32 @@ func main() { emitField(name) } emitLoadValue := func(name, typName string) { - fmt.Fprintf(outputFile, " m.LoadValue(%d, new(%s), func(y interface{}) { x.load%s(y.(%s)) })\n", fields[name], typName, camelCased(name), typName) + fmt.Fprintf(outputFile, " stateSourceObject.LoadValue(%d, new(%s), func(y interface{}) { %s.load%s(y.(%s)) })\n", fields[name], typName, recv, camelCased(name), typName) } emitLoad := func(name string) { - fmt.Fprintf(outputFile, " m.Load(%d, &x.%s)\n", fields[name], name) + fmt.Fprintf(outputFile, " stateSourceObject.Load(%d, &%s.%s)\n", fields[name], recv, name) } emitLoadWait := func(name string) { - fmt.Fprintf(outputFile, " m.LoadWait(%d, &x.%s)\n", fields[name], name) + fmt.Fprintf(outputFile, " stateSourceObject.LoadWait(%d, &%s.%s)\n", fields[name], recv, name) } emitSaveValue := func(name, typName string) { - fmt.Fprintf(outputFile, " var %s %s = x.save%s()\n", name, typName, camelCased(name)) - fmt.Fprintf(outputFile, " m.SaveValue(%d, %s)\n", fields[name], name) + fmt.Fprintf(outputFile, " var %sValue %s = %s.save%s()\n", name, typName, recv, camelCased(name)) + fmt.Fprintf(outputFile, " stateSinkObject.SaveValue(%d, %sValue)\n", fields[name], name) } emitSave := func(name string) { - fmt.Fprintf(outputFile, " m.Save(%d, &x.%s)\n", fields[name], name) + fmt.Fprintf(outputFile, " stateSinkObject.Save(%d, &%s.%s)\n", fields[name], recv, name) + } + emitZeroCheck := func(name string) { + fmt.Fprintf(outputFile, " if !%sIsZeroValue(&%s.%s) { %sFailf(\"%s is %%#v, expected zero\", &%s.%s) }\n", statePrefix, recv, name, statePrefix, name, recv, name) } // Generate the type name method. - fmt.Fprintf(outputFile, "func (x *%s) StateTypeName() string {\n", ts.Name.Name) + fmt.Fprintf(outputFile, "func (%s *%s) StateTypeName() string {\n", recv, ts.Name.Name) fmt.Fprintf(outputFile, " return \"%s.%s\"\n", *fullPkg, ts.Name.Name) fmt.Fprintf(outputFile, "}\n\n") // Generate the fields method. - fmt.Fprintf(outputFile, "func (x *%s) StateFields() []string {\n", ts.Name.Name) + fmt.Fprintf(outputFile, "func (%s *%s) StateFields() []string {\n", recv, ts.Name.Name) fmt.Fprintf(outputFile, " return []string{\n") scanFields(x, "", scanFunctions{ normal: emitField, @@ -401,8 +389,11 @@ func main() { // the code from compiling if a custom beforeSave was defined in a // file not provided to this binary and prevents inherited methods // from being called multiple times by overriding them. - if _, ok := simpleMethods[method{ts.Name.Name, "beforeSave"}]; !ok && generateSaverLoader { - fmt.Fprintf(outputFile, "func (x *%s) beforeSave() {}\n\n", ts.Name.Name) + if _, ok := simpleMethods[method{ + typeName: ts.Name.Name, + methodName: "beforeSave", + }]; !ok && generateSaverLoader { + fmt.Fprintf(outputFile, "func (%s *%s) beforeSave() {}\n\n", recv, ts.Name.Name) } // Generate the save method. @@ -412,8 +403,8 @@ func main() { // on this specific behavior, but the ability to specify slots // allows a manual implementation to be order-dependent. if generateSaverLoader { - fmt.Fprintf(outputFile, "func (x *%s) StateSave(m %sSink) {\n", ts.Name.Name, statePrefix) - fmt.Fprintf(outputFile, " x.beforeSave()\n") + fmt.Fprintf(outputFile, "func (%s *%s) StateSave(stateSinkObject %sSink) {\n", recv, ts.Name.Name, statePrefix) + fmt.Fprintf(outputFile, " %s.beforeSave()\n", recv) scanFields(x, "", scanFunctions{zerovalue: emitZeroCheck}) scanFields(x, "", scanFunctions{value: emitSaveValue}) scanFields(x, "", scanFunctions{normal: emitSave, wait: emitSave}) @@ -422,16 +413,19 @@ func main() { // Define afterLoad if a definition was not found. We do this for // the same reason that we do it for beforeSave. - _, hasAfterLoad := simpleMethods[method{ts.Name.Name, "afterLoad"}] + _, hasAfterLoad := simpleMethods[method{ + typeName: ts.Name.Name, + methodName: "afterLoad", + }] if !hasAfterLoad && generateSaverLoader { - fmt.Fprintf(outputFile, "func (x *%s) afterLoad() {}\n\n", ts.Name.Name) + fmt.Fprintf(outputFile, "func (%s *%s) afterLoad() {}\n\n", recv, ts.Name.Name) } // Generate the load method. // // N.B. See the comment above for the save method. if generateSaverLoader { - fmt.Fprintf(outputFile, "func (x *%s) StateLoad(m %sSource) {\n", ts.Name.Name, statePrefix) + fmt.Fprintf(outputFile, "func (%s *%s) StateLoad(stateSourceObject %sSource) {\n", recv, ts.Name.Name, statePrefix) scanFields(x, "", scanFunctions{normal: emitLoad, wait: emitLoadWait}) scanFields(x, "", scanFunctions{value: emitLoadValue}) if hasAfterLoad { @@ -439,7 +433,7 @@ func main() { // AfterLoad is called, the object encodes a dependency on // referred objects (i.e. fields). This means that afterLoad // will not be called until the other afterLoads are called. - fmt.Fprintf(outputFile, " m.AfterLoad(x.afterLoad)\n") + fmt.Fprintf(outputFile, " stateSourceObject.AfterLoad(%s.afterLoad)\n", recv) } fmt.Fprintf(outputFile, "}\n\n") } @@ -451,10 +445,10 @@ func main() { maybeEmitImports() // Generate the info methods. - fmt.Fprintf(outputFile, "func (x *%s) StateTypeName() string {\n", ts.Name.Name) + fmt.Fprintf(outputFile, "func (%s *%s) StateTypeName() string {\n", recv, ts.Name.Name) fmt.Fprintf(outputFile, " return \"%s.%s\"\n", *fullPkg, ts.Name.Name) fmt.Fprintf(outputFile, "}\n\n") - fmt.Fprintf(outputFile, "func (x *%s) StateFields() []string {\n", ts.Name.Name) + fmt.Fprintf(outputFile, "func (%s *%s) StateFields() []string {\n", recv, ts.Name.Name) fmt.Fprintf(outputFile, " return nil\n") fmt.Fprintf(outputFile, "}\n\n") diff --git a/tools/issue_reviver/BUILD b/tools/issue_reviver/BUILD deleted file mode 100644 index 4ef1a3124..000000000 --- a/tools/issue_reviver/BUILD +++ /dev/null @@ -1,12 +0,0 @@ -load("//tools:defs.bzl", "go_binary") - -package(licenses = ["notice"]) - -go_binary( - name = "issue_reviver", - srcs = ["main.go"], - deps = [ - "//tools/issue_reviver/github", - "//tools/issue_reviver/reviver", - ], -) diff --git a/tools/issue_reviver/github/BUILD b/tools/issue_reviver/github/BUILD deleted file mode 100644 index 0eabc2835..000000000 --- a/tools/issue_reviver/github/BUILD +++ /dev/null @@ -1,24 +0,0 @@ -load("//tools:defs.bzl", "go_library", "go_test") - -package(licenses = ["notice"]) - -go_library( - name = "github", - srcs = ["github.go"], - nogo = False, - visibility = [ - "//tools/issue_reviver:__subpackages__", - ], - deps = [ - "//tools/issue_reviver/reviver", - "@com_github_google_go_github_v28//github:go_default_library", - "@org_golang_x_oauth2//:go_default_library", - ], -) - -go_test( - name = "github_test", - size = "small", - srcs = ["github_test.go"], - library = ":github", -) diff --git a/tools/issue_reviver/main.go b/tools/issue_reviver/main.go deleted file mode 100644 index 47c796b8a..000000000 --- a/tools/issue_reviver/main.go +++ /dev/null @@ -1,100 +0,0 @@ -// 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. - -// Package main is the entry point for issue_reviver. -package main - -import ( - "flag" - "fmt" - "io/ioutil" - "os" - "strings" - - "gvisor.dev/gvisor/tools/issue_reviver/github" - "gvisor.dev/gvisor/tools/issue_reviver/reviver" -) - -var ( - owner string - repo string - tokenFile string - path string - dryRun bool -) - -// Keep the options simple for now. Supports only a single path and repo. -func init() { - flag.StringVar(&owner, "owner", "", "Github project org/owner to look for issues") - flag.StringVar(&repo, "repo", "", "Github repo to look for issues") - flag.StringVar(&tokenFile, "oauth-token-file", "", "Path to file containing the OAUTH token to be used as credential to github") - flag.StringVar(&path, "path", ".", "Path to scan for TODOs") - flag.BoolVar(&dryRun, "dry-run", false, "If set to true, no changes are made to issues") -} - -func main() { - // Set defaults from the environment. - repository := os.Getenv("GITHUB_REPOSITORY") - if parts := strings.SplitN(repository, "/", 2); len(parts) == 2 { - owner = parts[0] - repo = parts[1] - } - - // Parse flags. - flag.Parse() - - // Check for mandatory parameters. - if len(owner) == 0 { - fmt.Println("missing --owner option.") - flag.Usage() - os.Exit(1) - } - if len(repo) == 0 { - fmt.Println("missing --repo option.") - flag.Usage() - os.Exit(1) - } - if len(path) == 0 { - fmt.Println("missing --path option.") - flag.Usage() - os.Exit(1) - } - - // The access token may be passed as a file so it doesn't show up in - // command line arguments. It also may be provided through the - // environment to faciliate use through GitHub's CI system. - token := os.Getenv("GITHUB_TOKEN") - if len(tokenFile) != 0 { - bytes, err := ioutil.ReadFile(tokenFile) - if err != nil { - fmt.Println(err.Error()) - os.Exit(1) - } - token = string(bytes) - } - - bugger, err := github.NewBugger(token, owner, repo, dryRun) - if err != nil { - fmt.Fprintln(os.Stderr, "Error getting github issues:", err) - os.Exit(1) - } - rev := reviver.New([]string{path}, []reviver.Bugger{bugger}) - if errs := rev.Run(); len(errs) > 0 { - fmt.Fprintf(os.Stderr, "Encountered %d errors:\n", len(errs)) - for _, err := range errs { - fmt.Fprintf(os.Stderr, "\t%v\n", err) - } - os.Exit(1) - } -} diff --git a/tools/issue_reviver/reviver/BUILD b/tools/issue_reviver/reviver/BUILD deleted file mode 100644 index d262932bd..000000000 --- a/tools/issue_reviver/reviver/BUILD +++ /dev/null @@ -1,18 +0,0 @@ -load("//tools:defs.bzl", "go_library", "go_test") - -package(licenses = ["notice"]) - -go_library( - name = "reviver", - srcs = ["reviver.go"], - visibility = [ - "//tools/issue_reviver:__subpackages__", - ], -) - -go_test( - name = "reviver_test", - size = "small", - srcs = ["reviver_test.go"], - library = ":reviver", -) diff --git a/tools/make_apt.sh b/tools/make_apt.sh index 3fb1066e5..13c5edd76 100755 --- a/tools/make_apt.sh +++ b/tools/make_apt.sh @@ -54,18 +54,22 @@ declare -r release="${root}/dists/${suite}" mkdir -p "${release}" # Create a temporary keyring, and ensure it is cleaned up. +# Using separate homedir allows us to install apt repositories multiple times +# using the same key. This is a limitation in GnuPG pre-2.1. declare -r keyring=$(mktemp /tmp/keyringXXXXXX.gpg) +declare -r homedir=$(mktemp -d /tmp/homedirXXXXXX) +declare -r gpg_opts=("--no-default-keyring" "--secret-keyring" "${keyring}" "--homedir" "${homedir}") cleanup() { - rm -f "${keyring}" + rm -rf "${keyring}" "${homedir}" } trap cleanup EXIT # We attempt the import twice because the first one will fail if the public key # is not found. This isn't actually a failure for us, because we don't require -# the public (this may be stored separately). The second import will succeed +# the public key (this may be stored separately). The second import will succeed # because, in reality, the first import succeeded and it's a no-op. -gpg --no-default-keyring --keyring "${keyring}" --import "${private_key}" || \ - gpg --no-default-keyring --keyring "${keyring}" --import "${private_key}" +gpg "${gpg_opts[@]}" --import "${private_key}" || \ + gpg "${gpg_opts[@]}" --import "${private_key}" # Copy the packages into the root. for pkg in "$@"; do @@ -100,7 +104,8 @@ for pkg in "$@"; do cp -a "${pkg}" "${target}" chmod 0644 "${target}" if [[ "${ext}" == "deb" ]]; then - dpkg-sig -g "--no-default-keyring --keyring ${keyring}" --sign builder "${target}" + # We use [*] here to expand the gpg_opts array into a single shell-word. + dpkg-sig -g "${gpg_opts[*]}" --sign builder "${target}" fi done @@ -135,5 +140,5 @@ rm "${release}"/apt.conf # Sign the release. declare -r digest_opts=("--digest-algo" "SHA512" "--cert-digest-algo" "SHA512") (cd "${release}" && rm -f Release.gpg InRelease) -(cd "${release}" && gpg --no-default-keyring --keyring "${keyring}" --clearsign "${digest_opts[@]}" -o InRelease Release) -(cd "${release}" && gpg --no-default-keyring --keyring "${keyring}" -abs "${digest_opts[@]}" -o Release.gpg Release) +(cd "${release}" && gpg "${gpg_opts[@]}" --clearsign "${digest_opts[@]}" -o InRelease Release) +(cd "${release}" && gpg "${gpg_opts[@]}" -abs "${digest_opts[@]}" -o Release.gpg Release) diff --git a/tools/nogo/BUILD b/tools/nogo/BUILD index e1bfb9a2c..12b8b597c 100644 --- a/tools/nogo/BUILD +++ b/tools/nogo/BUILD @@ -1,22 +1,41 @@ -load("//tools:defs.bzl", "bzl_library", "go_library") +load("//tools:defs.bzl", "bzl_library", "go_library", "select_goarch", "select_goos") +load("//tools/nogo:defs.bzl", "nogo_objdump_tool", "nogo_stdlib", "nogo_target") package(licenses = ["notice"]) +nogo_target( + name = "target", + goarch = select_goarch(), + goos = select_goos(), + visibility = ["//visibility:public"], +) + +nogo_objdump_tool( + name = "objdump_tool", + visibility = ["//visibility:public"], +) + +nogo_stdlib( + name = "stdlib", + visibility = ["//visibility:public"], +) + go_library( name = "nogo", srcs = [ + "analyzers.go", "build.go", "config.go", - "matchers.go", + "findings.go", "nogo.go", - "register.go", ], nogo = False, visibility = ["//:sandbox"], deps = [ "//tools/checkescape", "//tools/checkunsafe", - "//tools/nogo/data", + "@co_honnef_go_tools//staticcheck:go_default_library", + "@co_honnef_go_tools//stylecheck:go_default_library", "@org_golang_x_tools//go/analysis:go_tool_library", "@org_golang_x_tools//go/analysis/internal/facts:go_tool_library", "@org_golang_x_tools//go/analysis/passes/asmdecl:go_tool_library", diff --git a/tools/nogo/analyzers.go b/tools/nogo/analyzers.go new file mode 100644 index 000000000..b919bc2f8 --- /dev/null +++ b/tools/nogo/analyzers.go @@ -0,0 +1,131 @@ +// 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. + +package nogo + +import ( + "encoding/gob" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/asmdecl" + "golang.org/x/tools/go/analysis/passes/assign" + "golang.org/x/tools/go/analysis/passes/atomic" + "golang.org/x/tools/go/analysis/passes/bools" + "golang.org/x/tools/go/analysis/passes/buildtag" + "golang.org/x/tools/go/analysis/passes/cgocall" + "golang.org/x/tools/go/analysis/passes/composite" + "golang.org/x/tools/go/analysis/passes/copylock" + "golang.org/x/tools/go/analysis/passes/errorsas" + "golang.org/x/tools/go/analysis/passes/httpresponse" + "golang.org/x/tools/go/analysis/passes/loopclosure" + "golang.org/x/tools/go/analysis/passes/lostcancel" + "golang.org/x/tools/go/analysis/passes/nilfunc" + "golang.org/x/tools/go/analysis/passes/nilness" + "golang.org/x/tools/go/analysis/passes/printf" + "golang.org/x/tools/go/analysis/passes/shadow" + "golang.org/x/tools/go/analysis/passes/shift" + "golang.org/x/tools/go/analysis/passes/stdmethods" + "golang.org/x/tools/go/analysis/passes/stringintconv" + "golang.org/x/tools/go/analysis/passes/structtag" + "golang.org/x/tools/go/analysis/passes/tests" + "golang.org/x/tools/go/analysis/passes/unmarshal" + "golang.org/x/tools/go/analysis/passes/unreachable" + "golang.org/x/tools/go/analysis/passes/unsafeptr" + "golang.org/x/tools/go/analysis/passes/unusedresult" + "honnef.co/go/tools/staticcheck" + "honnef.co/go/tools/stylecheck" + + "gvisor.dev/gvisor/tools/checkescape" + "gvisor.dev/gvisor/tools/checkunsafe" +) + +// AllAnalyzers is a list of all available analyzers. +var AllAnalyzers = []*analysis.Analyzer{ + asmdecl.Analyzer, + assign.Analyzer, + atomic.Analyzer, + bools.Analyzer, + buildtag.Analyzer, + cgocall.Analyzer, + composite.Analyzer, + copylock.Analyzer, + errorsas.Analyzer, + httpresponse.Analyzer, + loopclosure.Analyzer, + lostcancel.Analyzer, + nilfunc.Analyzer, + nilness.Analyzer, + printf.Analyzer, + shift.Analyzer, + stdmethods.Analyzer, + stringintconv.Analyzer, + shadow.Analyzer, + structtag.Analyzer, + tests.Analyzer, + unmarshal.Analyzer, + unreachable.Analyzer, + unsafeptr.Analyzer, + unusedresult.Analyzer, + checkescape.Analyzer, + checkunsafe.Analyzer, +} + +// EscapeAnalyzers is a list of escape-related analyzers. +var EscapeAnalyzers = []*analysis.Analyzer{ + checkescape.EscapeAnalyzer, +} + +func register(all []*analysis.Analyzer) { + // Register all fact types. + // + // N.B. This needs to be done recursively, because there may be + // analyzers in the Requires list that do not appear explicitly above. + registered := make(map[*analysis.Analyzer]struct{}) + var registerOne func(*analysis.Analyzer) + registerOne = func(a *analysis.Analyzer) { + if _, ok := registered[a]; ok { + return + } + + // Register dependencies. + for _, da := range a.Requires { + registerOne(da) + } + + // Register local facts. + for _, f := range a.FactTypes { + gob.Register(f) + } + + registered[a] = struct{}{} // Done. + } + for _, a := range all { + registerOne(a) + } +} + +func init() { + // Add all staticcheck analyzers. + for _, a := range staticcheck.Analyzers { + AllAnalyzers = append(AllAnalyzers, a) + } + // Add all stylecheck analyzers. + for _, a := range stylecheck.Analyzers { + AllAnalyzers = append(AllAnalyzers, a) + } + + // Register lists. + register(AllAnalyzers) + register(EscapeAnalyzers) +} diff --git a/tools/nogo/build.go b/tools/nogo/build.go index 433d13738..d173cff1f 100644 --- a/tools/nogo/build.go +++ b/tools/nogo/build.go @@ -20,21 +20,11 @@ import ( "os" ) -var ( - // internalPrefix is the internal path prefix. Note that this is not - // special, as paths should be passed relative to the repository root - // and should not have any special prefix applied. - internalPrefix = fmt.Sprintf("^") - - // externalPrefix is external workspace packages. - externalPrefix = "^external/" -) - // findStdPkg needs to find the bundled standard library packages. -func (i *importer) findStdPkg(path string) (io.ReadCloser, error) { +func findStdPkg(GOOS, GOARCH, path string) (io.ReadCloser, error) { if path == "C" { // Cgo builds cannot be analyzed. Skip. return nil, ErrSkip } - return os.Open(fmt.Sprintf("external/go_sdk/pkg/%s_%s/%s.a", i.GOOS, i.GOARCH, path)) + return os.Open(fmt.Sprintf("external/go_sdk/pkg/%s_%s/%s.a", GOOS, GOARCH, path)) } diff --git a/tools/nogo/check/BUILD b/tools/nogo/check/BUILD index e2d76cd5c..e18483a18 100644 --- a/tools/nogo/check/BUILD +++ b/tools/nogo/check/BUILD @@ -2,11 +2,10 @@ load("//tools:defs.bzl", "go_binary") package(licenses = ["notice"]) -# Note that the check binary must be public, since an aspect may be applied -# across lots of different rules in different repositories. go_binary( name = "check", srcs = ["main.go"], + nogo = False, visibility = ["//visibility:public"], deps = ["//tools/nogo"], ) diff --git a/tools/nogo/check/main.go b/tools/nogo/check/main.go index 3828edf3a..69bdfe502 100644 --- a/tools/nogo/check/main.go +++ b/tools/nogo/check/main.go @@ -16,9 +16,99 @@ package main import ( + "encoding/json" + "flag" + "fmt" + "io/ioutil" + "log" + "os" + "gvisor.dev/gvisor/tools/nogo" ) +var ( + packageFile = flag.String("package", "", "package configuration file (in JSON format)") + stdlibFile = flag.String("stdlib", "", "stdlib configuration file (in JSON format)") + findingsOutput = flag.String("findings", "", "output file (or stdout, if not specified)") + factsOutput = flag.String("facts", "", "output file for facts (optional)") + escapesOutput = flag.String("escapes", "", "output file for escapes (optional)") +) + +func loadConfig(file string, config interface{}) interface{} { + // Load the configuration. + f, err := os.Open(file) + if err != nil { + log.Fatalf("unable to open configuration %q: %v", file, err) + } + defer f.Close() + dec := json.NewDecoder(f) + dec.DisallowUnknownFields() + if err := dec.Decode(config); err != nil { + log.Fatalf("unable to decode configuration: %v", err) + } + return config +} + func main() { - nogo.Main() + // Parse all flags. + flag.Parse() + + var ( + findings []nogo.Finding + factData []byte + err error + ) + + // Check & load the configuration. + if *packageFile != "" && *stdlibFile != "" { + log.Fatalf("unable to perform stdlib and package analysis; provide only one!") + } + + // Run the configuration. + if *stdlibFile != "" { + // Perform basic analysis. + c := loadConfig(*stdlibFile, new(nogo.StdlibConfig)).(*nogo.StdlibConfig) + findings, factData, err = nogo.CheckStdlib(c, nogo.AllAnalyzers) + + } else if *packageFile != "" { + // Perform basic analysis. + c := loadConfig(*packageFile, new(nogo.PackageConfig)).(*nogo.PackageConfig) + findings, factData, err = nogo.CheckPackage(c, nogo.AllAnalyzers, nil) + + // Do we need to do escape analysis? + if *escapesOutput != "" { + escapes, _, err := nogo.CheckPackage(c, nogo.EscapeAnalyzers, nil) + if err != nil { + log.Fatalf("error performing escape analysis: %v", err) + } + if err := nogo.WriteFindingsToFile(escapes, *escapesOutput); err != nil { + log.Fatalf("error writing escapes to %q: %v", *escapesOutput, err) + } + } + } else { + log.Fatalf("please provide at least one of package or stdlib!") + } + + // Check that analysis was successful. + if err != nil { + log.Fatalf("error performing analysis: %v", err) + } + + // Save facts. + if *factsOutput != "" { + if err := ioutil.WriteFile(*factsOutput, factData, 0644); err != nil { + log.Fatalf("error saving findings to %q: %v", *factsOutput, err) + } + } + + // Write all findings. + if *findingsOutput != "" { + if err := nogo.WriteFindingsToFile(findings, *findingsOutput); err != nil { + log.Fatalf("error writing findings to %q: %v", *findingsOutput, err) + } + } else { + for _, finding := range findings { + fmt.Fprintf(os.Stdout, "%s\n", finding.String()) + } + } } diff --git a/tools/nogo/config.go b/tools/nogo/config.go index 6958fca69..2fea5b3e1 100644 --- a/tools/nogo/config.go +++ b/tools/nogo/config.go @@ -15,102 +15,247 @@ package nogo import ( - "golang.org/x/tools/go/analysis" - "golang.org/x/tools/go/analysis/passes/asmdecl" - "golang.org/x/tools/go/analysis/passes/assign" - "golang.org/x/tools/go/analysis/passes/atomic" - "golang.org/x/tools/go/analysis/passes/bools" - "golang.org/x/tools/go/analysis/passes/buildtag" - "golang.org/x/tools/go/analysis/passes/cgocall" - "golang.org/x/tools/go/analysis/passes/composite" - "golang.org/x/tools/go/analysis/passes/copylock" - "golang.org/x/tools/go/analysis/passes/errorsas" - "golang.org/x/tools/go/analysis/passes/httpresponse" - "golang.org/x/tools/go/analysis/passes/loopclosure" - "golang.org/x/tools/go/analysis/passes/lostcancel" - "golang.org/x/tools/go/analysis/passes/nilfunc" - "golang.org/x/tools/go/analysis/passes/nilness" - "golang.org/x/tools/go/analysis/passes/printf" - "golang.org/x/tools/go/analysis/passes/shadow" - "golang.org/x/tools/go/analysis/passes/shift" - "golang.org/x/tools/go/analysis/passes/stdmethods" - "golang.org/x/tools/go/analysis/passes/stringintconv" - "golang.org/x/tools/go/analysis/passes/structtag" - "golang.org/x/tools/go/analysis/passes/tests" - "golang.org/x/tools/go/analysis/passes/unmarshal" - "golang.org/x/tools/go/analysis/passes/unreachable" - "golang.org/x/tools/go/analysis/passes/unsafeptr" - "golang.org/x/tools/go/analysis/passes/unusedresult" - - "gvisor.dev/gvisor/tools/checkescape" - "gvisor.dev/gvisor/tools/checkunsafe" + "fmt" + "regexp" ) -var analyzerConfig = map[*analysis.Analyzer]matcher{ - // Standard analyzers. - asmdecl.Analyzer: alwaysMatches(), - assign.Analyzer: externalExcluded( - ".*gazelle/walk/walk.go", // False positive. - ), - atomic.Analyzer: alwaysMatches(), - bools.Analyzer: alwaysMatches(), - buildtag.Analyzer: alwaysMatches(), - cgocall.Analyzer: alwaysMatches(), - composite.Analyzer: and( - disableMatches(), // Disabled for now. - resultExcluded{ - "Object_", - "Range{", - }, - ), - copylock.Analyzer: internalMatches(), // Common external issues (e.g. protos). - errorsas.Analyzer: alwaysMatches(), - httpresponse.Analyzer: alwaysMatches(), - loopclosure.Analyzer: alwaysMatches(), - lostcancel.Analyzer: internalMatches(), // Common external issues. - nilfunc.Analyzer: alwaysMatches(), - nilness.Analyzer: and( - internalMatches(), // Common "tautological checks". - internalExcluded( - "pkg/sentry/platform/kvm/kvm_test.go", // Intentional. - "tools/bigquery/bigquery.go", // False positive. - ), - ), - printf.Analyzer: alwaysMatches(), - shift.Analyzer: alwaysMatches(), - stdmethods.Analyzer: internalMatches(), // Common external issues (e.g. methods named "Write"). - stringintconv.Analyzer: and( - internalExcluded(), - externalExcluded( - ".*protobuf/.*.go", // Bad conversions. - ".*flate/huffman_bit_writer.go", // Bad conversion. - ), - ), - shadow.Analyzer: disableMatches(), // Disabled for now. - structtag.Analyzer: internalMatches(), // External not subject to rules. - tests.Analyzer: alwaysMatches(), - unmarshal.Analyzer: alwaysMatches(), - unreachable.Analyzer: internalMatches(), - unsafeptr.Analyzer: and( - internalMatches(), - internalExcluded( - ".*_test.go", // Exclude tests. - "pkg/flipcall/.*_unsafe.go", // Special case. - "pkg/gohacks/gohacks_unsafe.go", // Special case. - "pkg/sentry/fs/fsutil/host_file_mapper_unsafe.go", // Special case. - "pkg/sentry/platform/kvm/bluepill_unsafe.go", // Special case. - "pkg/sentry/platform/kvm/machine_unsafe.go", // Special case. - "pkg/sentry/platform/ring0/pagetables/allocator_unsafe.go", // Special case. - "pkg/sentry/platform/safecopy/safecopy_unsafe.go", // Special case. - "pkg/sentry/vfs/mount_unsafe.go", // Special case. - "pkg/sentry/platform/systrap/stub_unsafe.go", // Special case. - "pkg/sentry/platform/systrap/switchto_google_unsafe.go", // Special case. - "pkg/sentry/platform/systrap/sysmsg_thread_unsafe.go", // Special case. - ), - ), - unusedresult.Analyzer: alwaysMatches(), - - // Internal analyzers: external packages not subject. - checkescape.Analyzer: internalMatches(), - checkunsafe.Analyzer: internalMatches(), +// GroupName is a named group. +type GroupName string + +// AnalyzerName is a named analyzer. +type AnalyzerName string + +// Group represents a named collection of files. +type Group struct { + // Name is the short name for the group. + Name GroupName `yaml:"name"` + + // Regex matches all full paths in the group. + Regex string `yaml:"regex"` + regex *regexp.Regexp `yaml:"-"` + + // Default determines the default group behavior. + // + // If Default is true, all Analyzers are enabled for this + // group. Otherwise, Analyzers must be individually enabled + // by specifying a (possible empty) ItemConfig for the group + // in the AnalyzerConfig. + Default bool `yaml:"default"` +} + +func (g *Group) compile() error { + r, err := regexp.Compile(g.Regex) + if err != nil { + return err + } + g.regex = r + return nil +} + +// ItemConfig is an (Analyzer,Group) configuration. +type ItemConfig struct { + // Exclude are analyzer exclusions. + // + // Exclude is a list of regular expressions. If the corresponding + // Analyzer emits a Finding for which Finding.Position.String() + // matches a regular expression in Exclude, the finding will not + // be reported. + Exclude []string `yaml:"exclude,omitempty"` + exclude []*regexp.Regexp `yaml:"-"` + + // Suppress are analyzer suppressions. + // + // Suppress is a list of regular expressions. If the corresponding + // Analyzer emits a Finding for which Finding.Message matches a regular + // expression in Suppress, the finding will not be reported. + Suppress []string `yaml:"suppress,omitempty"` + suppress []*regexp.Regexp `yaml:"-"` +} + +func compileRegexps(ss []string, rs *[]*regexp.Regexp) error { + *rs = make([]*regexp.Regexp, 0, len(ss)) + for _, s := range ss { + r, err := regexp.Compile(s) + if err != nil { + return err + } + *rs = append(*rs, r) + } + return nil +} + +func (i *ItemConfig) compile() error { + if i == nil { + // This may be nil if nothing is included in the + // item configuration. That's fine, there's nothing + // to compile and nothing to exclude & suppress. + return nil + } + if err := compileRegexps(i.Exclude, &i.exclude); err != nil { + return fmt.Errorf("in exclude: %w", err) + } + if err := compileRegexps(i.Suppress, &i.suppress); err != nil { + return fmt.Errorf("in suppress: %w", err) + } + return nil +} + +func (i *ItemConfig) merge(other *ItemConfig) { + i.Exclude = append(i.Exclude, other.Exclude...) + i.Suppress = append(i.Suppress, other.Suppress...) +} + +func (i *ItemConfig) shouldReport(fullPos, msg string) bool { + if i == nil { + // See above. + return true + } + for _, r := range i.exclude { + if r.MatchString(fullPos) { + return false + } + } + for _, r := range i.suppress { + if r.MatchString(msg) { + return false + } + } + return true +} + +// AnalyzerConfig is the configuration for a single analyzers. +// +// This map is keyed by individual Group names, to allow for different +// configurations depending on what Group the file belongs to. +type AnalyzerConfig map[GroupName]*ItemConfig + +func (a AnalyzerConfig) compile() error { + for name, gc := range a { + if err := gc.compile(); err != nil { + return fmt.Errorf("invalid group %q: %v", name, err) + } + } + return nil +} + +func (a AnalyzerConfig) merge(other AnalyzerConfig) { + // Merge all the groups. + for name, gc := range other { + old, ok := a[name] + if !ok || old == nil { + a[name] = gc // Not configured in a. + continue + } + old.merge(gc) + } +} + +func (a AnalyzerConfig) shouldReport(groupConfig *Group, fullPos, msg string) bool { + gc, ok := a[groupConfig.Name] + if !ok { + return groupConfig.Default + } + + // Note that if a section appears for a particular group + // for a particular analyzer, then it will now be enabled, + // and the group default no longer applies. + return gc.shouldReport(fullPos, msg) +} + +// Config is a nogo configuration. +type Config struct { + // Prefixes defines a set of regular expressions that + // are standard "prefixes", so that files can be grouped + // and specific rules applied to individual groups. + Groups []Group `yaml:"groups"` + + // Global is the global analyzer config. + Global AnalyzerConfig `yaml:"global"` + + // Analyzers are individual analyzer configurations. The + // key for each analyzer is the name of the analyzer. The + // value is either a boolean (enable/disable), or a map to + // the groups above. + Analyzers map[AnalyzerName]AnalyzerConfig `yaml:"analyzers"` +} + +// Merge merges two configurations. +func (c *Config) Merge(other *Config) { + // Merge all groups. + for _, g := range other.Groups { + // Is there a matching group? If yes, we just delete + // it. This will preserve the order provided in the + // overriding file, even if it differs. + for i := 0; i < len(c.Groups); i++ { + if g.Name == c.Groups[i].Name { + copy(c.Groups[i:], c.Groups[i+1:]) + c.Groups = c.Groups[:len(c.Groups)-1] + break + } + } + c.Groups = append(c.Groups, g) + } + + // Merge global configurations. + c.Global.merge(other.Global) + + // Merge all analyzer configurations. + for name, ac := range other.Analyzers { + old, ok := c.Analyzers[name] + if !ok { + c.Analyzers[name] = ac // No analyzer in original config. + continue + } + old.merge(ac) + } +} + +// Compile compiles a configuration to make it useable. +func (c *Config) Compile() error { + for i := 0; i < len(c.Groups); i++ { + if err := c.Groups[i].compile(); err != nil { + return fmt.Errorf("invalid group %q: %w", c.Groups[i].Name, err) + } + } + if err := c.Global.compile(); err != nil { + return fmt.Errorf("invalid global: %w", err) + } + for name, ac := range c.Analyzers { + if err := ac.compile(); err != nil { + return fmt.Errorf("invalid analyzer %q: %w", name, err) + } + } + return nil +} + +// ShouldReport returns true iff the finding should match the Config. +func (c *Config) ShouldReport(finding Finding) bool { + fullPos := finding.Position.String() + + // Find the matching group. + var groupConfig *Group + for i := 0; i < len(c.Groups); i++ { + if c.Groups[i].regex.MatchString(fullPos) { + groupConfig = &c.Groups[i] + break + } + } + + // If there is no group matching this path, then + // we default to accept the finding. + if groupConfig == nil { + return true + } + + // Suppress via global rule? + if !c.Global.shouldReport(groupConfig, fullPos, finding.Message) { + return false + } + + // Try the analyzer config. + ac, ok := c.Analyzers[finding.Category] + if !ok { + return groupConfig.Default + } + return ac.shouldReport(groupConfig, fullPos, finding.Message) } diff --git a/tools/nogo/data/BUILD b/tools/nogo/data/BUILD deleted file mode 100644 index b7564cc44..000000000 --- a/tools/nogo/data/BUILD +++ /dev/null @@ -1,10 +0,0 @@ -load("//tools:defs.bzl", "go_library") - -package(licenses = ["notice"]) - -go_library( - name = "data", - srcs = ["data.go"], - nogo = False, - visibility = ["//tools:__subpackages__"], -) diff --git a/tools/nogo/data/data.go b/tools/nogo/data/data.go deleted file mode 100644 index eb84d0d27..000000000 --- a/tools/nogo/data/data.go +++ /dev/null @@ -1,21 +0,0 @@ -// 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. - -// Package data contains shared data for nogo analysis. -// -// This is used to break a dependency cycle. -package data - -// Objdump is the dumped binary under analysis. -var Objdump string diff --git a/tools/nogo/defs.bzl b/tools/nogo/defs.bzl index d399079c5..b3d297308 100644 --- a/tools/nogo/defs.bzl +++ b/tools/nogo/defs.bzl @@ -1,6 +1,168 @@ """Nogo rules.""" -load("//tools/bazeldefs:defs.bzl", "go_context", "go_importpath", "go_rule") +load("//tools/bazeldefs:go.bzl", "go_context", "go_importpath", "go_rule", "go_test_library") + +NogoConfigInfo = provider( + "information about a nogo configuration", + fields = { + "srcs": "the collection of configuration files", + }, +) + +def _nogo_config_impl(ctx): + return [NogoConfigInfo( + srcs = ctx.files.srcs, + )] + +nogo_config = rule( + implementation = _nogo_config_impl, + attrs = { + "srcs": attr.label_list( + doc = "a list of yaml files (schema defined by tool/nogo/config.go).", + allow_files = True, + ), + }, +) + +NogoTargetInfo = provider( + "information about the Go target", + fields = { + "goarch": "the build architecture (GOARCH)", + "goos": "the build OS target (GOOS)", + }, +) + +def _nogo_target_impl(ctx): + return [NogoTargetInfo( + goarch = ctx.attr.goarch, + goos = ctx.attr.goos, + )] + +nogo_target = go_rule( + rule, + implementation = _nogo_target_impl, + attrs = { + "goarch": attr.string( + doc = "the Go build architecture (propagated to other rules).", + mandatory = True, + ), + "goos": attr.string( + doc = "the Go OS target (propagated to other rules).", + mandatory = True, + ), + }, +) + +def _nogo_objdump_tool_impl(ctx): + # Construct the magic dump command. + # + # Note that in some cases, the input is being fed into the tool via stdin. + # Unfortunately, the Go objdump tool expects to see a seekable file [1], so + # we need the tool to handle this case by creating a temporary file. + # + # [1] https://github.com/golang/go/issues/41051 + nogo_target_info = ctx.attr._nogo_target[NogoTargetInfo] + go_ctx = go_context(ctx, goos = nogo_target_info.goos, goarch = nogo_target_info.goarch) + env_prefix = " ".join(["%s=%s" % (key, value) for (key, value) in go_ctx.env.items()]) + dumper = ctx.actions.declare_file(ctx.label.name) + ctx.actions.write(dumper, "\n".join([ + "#!/bin/bash", + "set -euo pipefail", + "if [[ $# -eq 0 ]]; then", + " T=$(mktemp -u -t libXXXXXX.a)", + " cat /dev/stdin > ${T}", + "else", + " T=$1;", + "fi", + "%s %s tool objdump ${T}" % ( + env_prefix, + go_ctx.go.path, + ), + "if [[ $# -eq 0 ]]; then", + " rm -rf ${T}", + "fi", + "", + ]), is_executable = True) + + # Include the full runfiles. + return [DefaultInfo( + runfiles = ctx.runfiles(files = go_ctx.runfiles.to_list()), + executable = dumper, + )] + +nogo_objdump_tool = go_rule( + rule, + implementation = _nogo_objdump_tool_impl, + attrs = { + "_nogo_target": attr.label( + default = "//tools/nogo:target", + cfg = "target", + ), + }, +) + +# NogoStdlibInfo is the set of standard library facts. +NogoStdlibInfo = provider( + "information for nogo analysis (standard library facts)", + fields = { + "facts": "serialized standard library facts", + "raw_findings": "raw package findings (if relevant)", + }, +) + +def _nogo_stdlib_impl(ctx): + # Build the standard library facts. + nogo_target_info = ctx.attr._nogo_target[NogoTargetInfo] + go_ctx = go_context(ctx, goos = nogo_target_info.goos, goarch = nogo_target_info.goarch) + facts = ctx.actions.declare_file(ctx.label.name + ".facts") + raw_findings = ctx.actions.declare_file(ctx.label.name + ".raw_findings") + config = struct( + Srcs = [f.path for f in go_ctx.stdlib_srcs], + GOOS = go_ctx.goos, + GOARCH = go_ctx.goarch, + Tags = go_ctx.tags, + ) + config_file = ctx.actions.declare_file(ctx.label.name + ".cfg") + ctx.actions.write(config_file, config.to_json()) + ctx.actions.run( + inputs = [config_file] + go_ctx.stdlib_srcs, + outputs = [facts, raw_findings], + tools = depset(go_ctx.runfiles.to_list() + ctx.files._nogo_objdump_tool), + executable = ctx.files._nogo_check[0], + mnemonic = "NogoStandardLibraryAnalysis", + progress_message = "Analyzing Go Standard Library", + arguments = go_ctx.nogo_args + [ + "-objdump_tool=%s" % ctx.files._nogo_objdump_tool[0].path, + "-stdlib=%s" % config_file.path, + "-findings=%s" % raw_findings.path, + "-facts=%s" % facts.path, + ], + ) + + # Return the stdlib facts as output. + return [NogoStdlibInfo( + facts = facts, + raw_findings = raw_findings, + )] + +nogo_stdlib = go_rule( + rule, + implementation = _nogo_stdlib_impl, + attrs = { + "_nogo_check": attr.label( + default = "//tools/nogo/check:check", + cfg = "host", + ), + "_nogo_objdump_tool": attr.label( + default = "//tools/nogo:objdump_tool", + cfg = "host", + ), + "_nogo_target": attr.label( + default = "//tools/nogo:target", + cfg = "target", + ), + }, +) # NogoInfo is the serialized set of package facts for a nogo analysis. # @@ -8,10 +170,15 @@ load("//tools/bazeldefs:defs.bzl", "go_context", "go_importpath", "go_rule") # with the source files as input. Note however, that the individual nogo rules # are simply stubs that enter into the shadow dependency tree (the "aspect"). NogoInfo = provider( + "information for nogo analysis", fields = { "facts": "serialized package facts", + "raw_findings": "raw package findings (if relevant)", + "escapes": "escape-only findings (if relevant)", "importpath": "package import path", "binaries": "package binary files", + "srcs": "srcs (for go_test support)", + "deps": "deps (for go_test support)", }, ) @@ -21,17 +188,26 @@ def _nogo_aspect_impl(target, ctx): # All work is done in the shadow properties for go rules. For a proto # library, we simply skip the analysis portion but still need to return a # valid NogoInfo to reference the generated binary. - if ctx.rule.kind == "go_library": + if ctx.rule.kind in ("go_library", "go_tool_library", "go_binary", "go_test"): srcs = ctx.rule.files.srcs - elif ctx.rule.kind == "go_proto_library" or ctx.rule.kind == "go_wrap_cc": + deps = ctx.rule.attr.deps + elif ctx.rule.kind in ("go_proto_library", "go_wrap_cc"): srcs = [] + deps = ctx.rule.attr.deps else: return [NogoInfo()] - go_ctx = go_context(ctx) - - # Construct the Go environment from the go_ctx.env dictionary. - env_prefix = " ".join(["%s=%s" % (key, value) for (key, value) in go_ctx.env.items()]) + # If we're using the "library" attribute, then we need to aggregate the + # original library sources and dependencies into this target to perform + # proper type analysis. + if ctx.rule.kind == "go_test": + library = go_test_library(ctx.rule) + if library != None: + info = library[NogoInfo] + if hasattr(info, "srcs"): + srcs = srcs + info.srcs + if hasattr(info, "deps"): + deps = deps + info.deps # Start with all target files and srcs as input. inputs = target.files.to_list() + srcs @@ -41,50 +217,31 @@ def _nogo_aspect_impl(target, ctx): # to cleanly allow us redirect stdout to the actual output file. Perhaps # I'm missing something here, but the intermediate script does work. binaries = target.files.to_list() - disasm_file = ctx.actions.declare_file(target.label.name + ".out") - dumper = ctx.actions.declare_file("%s-dumper" % ctx.label.name) - ctx.actions.write(dumper, "\n".join([ - "#!/bin/bash", - "%s %s tool objdump %s > %s\n" % ( - env_prefix, - go_ctx.go.path, - [f.path for f in binaries if f.path.endswith(".a")][0], - disasm_file.path, - ), - ]), is_executable = True) - ctx.actions.run( - inputs = binaries, - outputs = [disasm_file], - tools = go_ctx.runfiles, - mnemonic = "GoObjdump", - progress_message = "Objdump %s" % target.label, - executable = dumper, - ) - inputs.append(disasm_file) + objfiles = [f for f in binaries if f.path.endswith(".a")] + if len(objfiles) > 0: + # Prefer the .a files for go_library targets. + target_objfile = objfiles[0] + else: + # Use the raw binary for go_binary and go_test targets. + target_objfile = binaries[0] + inputs.append(target_objfile) # Extract the importpath for this package. - importpath = go_importpath(target) - - # The nogo tool requires a configfile serialized in JSON format to do its - # work. This must line up with the nogo.Config fields. - facts = ctx.actions.declare_file(target.label.name + ".facts") - config = struct( - ImportPath = importpath, - GoFiles = [src.path for src in srcs if src.path.endswith(".go")], - NonGoFiles = [src.path for src in srcs if not src.path.endswith(".go")], - # Google's internal build system needs a bit more help to find std. - StdZip = go_ctx.std_zip.short_path if hasattr(go_ctx, "std_zip") else "", - GOOS = go_ctx.goos, - GOARCH = go_ctx.goarch, - Tags = go_ctx.tags, - FactMap = {}, # Constructed below. - ImportMap = {}, # Constructed below. - FactOutput = facts.path, - Objdump = disasm_file.path, - ) + if ctx.rule.kind == "go_test": + # If this is a test, then it will not be imported by anything else. + # We can safely set the importapth to just "test". Note that this + # is necessary if the library also imports the core library (in + # addition to including the sources directly), which happens in + # some complex cases (seccomp_victim). + importpath = "test" + else: + importpath = go_importpath(target) # Collect all info from shadow dependencies. - for dep in ctx.rule.attr.deps: + fact_map = dict() + import_map = dict() + all_raw_findings = [] + for dep in deps: # There will be no file attribute set for all transitive dependencies # that are not go_library or go_binary rules, such as a proto rules. # This is handled by the ctx.rule.kind check above. @@ -98,44 +255,101 @@ def _nogo_aspect_impl(target, ctx): x_files = [f.path for f in info.binaries if f.path.endswith(".x")] if not len(x_files): x_files = [f.path for f in info.binaries if f.path.endswith(".a")] - config.ImportMap[info.importpath] = x_files[0] - config.FactMap[info.importpath] = info.facts.path + import_map[info.importpath] = x_files[0] + fact_map[info.importpath] = info.facts.path + + # Collect all findings; duplicates are resolved at the end. + all_raw_findings.extend(info.raw_findings) # Ensure the above are available as inputs. inputs.append(info.facts) inputs += info.binaries - # Write the configuration and run the tool. + # Add the standard library facts. + stdlib_info = ctx.attr._nogo_stdlib[NogoStdlibInfo] + stdlib_facts = stdlib_info.facts + inputs.append(stdlib_facts) + + # The nogo tool operates on a configuration serialized in JSON format. + nogo_target_info = ctx.attr._nogo_target[NogoTargetInfo] + go_ctx = go_context(ctx, goos = nogo_target_info.goos, goarch = nogo_target_info.goarch) + facts = ctx.actions.declare_file(target.label.name + ".facts") + raw_findings = ctx.actions.declare_file(target.label.name + ".raw_findings") + escapes = ctx.actions.declare_file(target.label.name + ".escapes") + config = struct( + ImportPath = importpath, + GoFiles = [src.path for src in srcs if src.path.endswith(".go")], + NonGoFiles = [src.path for src in srcs if not src.path.endswith(".go")], + GOOS = go_ctx.goos, + GOARCH = go_ctx.goarch, + Tags = go_ctx.tags, + FactMap = fact_map, + ImportMap = import_map, + StdlibFacts = stdlib_facts.path, + ) config_file = ctx.actions.declare_file(target.label.name + ".cfg") ctx.actions.write(config_file, config.to_json()) inputs.append(config_file) - - # Run the nogo tool itself. ctx.actions.run( inputs = inputs, - outputs = [facts], - tools = go_ctx.runfiles, - executable = ctx.files._nogo[0], - mnemonic = "GoStaticAnalysis", + outputs = [facts, raw_findings, escapes], + tools = depset(go_ctx.runfiles.to_list() + ctx.files._nogo_objdump_tool), + executable = ctx.files._nogo_check[0], + mnemonic = "NogoAnalysis", progress_message = "Analyzing %s" % target.label, - arguments = ["-config=%s" % config_file.path], + arguments = go_ctx.nogo_args + [ + "-binary=%s" % target_objfile.path, + "-objdump_tool=%s" % ctx.files._nogo_objdump_tool[0].path, + "-package=%s" % config_file.path, + "-findings=%s" % raw_findings.path, + "-facts=%s" % facts.path, + "-escapes=%s" % escapes.path, + ], ) + # Flatten all findings from all dependencies. + # + # This is done because all the filtering must be done at the + # top-level nogo_test to dynamically apply a configuration. + # This does not actually add any additional work here, but + # will simply propagate the full list of files. + all_raw_findings = [stdlib_info.raw_findings] + depset(all_raw_findings).to_list() + [raw_findings] + # Return the package facts as output. return [NogoInfo( facts = facts, + raw_findings = all_raw_findings, + escapes = escapes, importpath = importpath, binaries = binaries, + srcs = srcs, + deps = deps, )] nogo_aspect = go_rule( aspect, implementation = _nogo_aspect_impl, - attr_aspects = ["deps"], + attr_aspects = [ + "deps", + "library", + "embed", + ], attrs = { - "_nogo": attr.label( + "_nogo_check": attr.label( default = "//tools/nogo/check:check", - allow_single_file = True, + cfg = "host", + ), + "_nogo_stdlib": attr.label( + default = "//tools/nogo:stdlib", + cfg = "host", + ), + "_nogo_objdump_tool": attr.label( + default = "//tools/nogo:objdump_tool", + cfg = "host", + ), + "_nogo_target": attr.label( + default = "//tools/nogo:target", + cfg = "target", ), }, ) @@ -143,34 +357,72 @@ nogo_aspect = go_rule( def _nogo_test_impl(ctx): """Check nogo findings.""" - # Build a runner that checks for the existence of the facts file. Note that - # the actual build will fail in the case of a broken analysis. We things - # this way so that any test applied is effectively pushed down to all - # upstream dependencies through the aspect. - inputs = [] - runner = ctx.actions.declare_file("%s-executer" % ctx.label.name) - runner_content = ["#!/bin/bash"] - for dep in ctx.attr.deps: - info = dep[NogoInfo] - inputs.append(info.facts) + # Ensure there's a single dependency. + if len(ctx.attr.deps) != 1: + fail("nogo_test requires exactly one dep.") + raw_findings = ctx.attr.deps[0][NogoInfo].raw_findings + escapes = ctx.attr.deps[0][NogoInfo].escapes + + # Build a step that applies the configuration. + config_srcs = ctx.attr.config[NogoConfigInfo].srcs + findings = ctx.actions.declare_file(ctx.label.name + ".findings") + ctx.actions.run( + inputs = raw_findings + ctx.files.srcs + config_srcs, + outputs = [findings], + tools = depset(ctx.files._filter), + executable = ctx.files._filter[0], + mnemonic = "GoStaticAnalysis", + progress_message = "Generating %s" % ctx.label, + arguments = ["-input=%s" % f.path for f in raw_findings] + + ["-config=%s" % f.path for f in config_srcs] + + ["-output=%s" % findings.path], + ) - # Draw a sweet unicode checkmark with the package name (in green). - runner_content.append("echo -e \"\\033[0;32m\\xE2\\x9C\\x94\\033[0;31m\\033[0m %s\"" % info.importpath) - runner_content.append("exit 0\n") + # Build a runner that checks the filtered facts. + # + # Note that this calls the filter binary without any configuration, so all + # findings will be included. But this is expected, since we've already + # filtered out everything that should not be included. + runner = ctx.actions.declare_file(ctx.label.name) + runner_content = [ + "#!/bin/bash", + "exec %s -input=%s" % (ctx.files._filter[0].short_path, findings.short_path), + "", + ] ctx.actions.write(runner, "\n".join(runner_content), is_executable = True) + return [DefaultInfo( - runfiles = ctx.runfiles(files = inputs), + # The runner just executes the filter again, on the + # newly generated filtered findings. We still need + # the filter tool as part of our runfiles, however. + runfiles = ctx.runfiles(files = ctx.files._filter + [findings]), executable = runner, + ), OutputGroupInfo( + # Propagate the filtered filters, for consumption by + # build tooling. Note that the build tooling typically + # pays attention to the mnemoic above, so this must be + # what is expected by the tooling. + nogo_findings = depset([findings]), + # Expose all escape analysis findings (see above). + nogo_escapes = depset([escapes]), )] -_nogo_test = rule( +nogo_test = rule( implementation = _nogo_test_impl, attrs = { - "deps": attr.label_list(aspects = [nogo_aspect]), + "config": attr.label( + mandatory = True, + doc = "A rule of kind nogo_config.", + ), + "deps": attr.label_list( + aspects = [nogo_aspect], + doc = "Exactly one Go dependency to be analyzed.", + ), + "srcs": attr.label_list( + allow_files = True, + doc = "Relevant src files. This is ignored except to make the nogo_test directly affected by the files.", + ), + "_filter": attr.label(default = "//tools/nogo/filter:filter"), }, test = True, ) - -def nogo_test(**kwargs): - tags = kwargs.pop("tags", []) + ["nogo"] - _nogo_test(tags = tags, **kwargs) diff --git a/tools/nogo/filter/BUILD b/tools/nogo/filter/BUILD new file mode 100644 index 000000000..e56a783e2 --- /dev/null +++ b/tools/nogo/filter/BUILD @@ -0,0 +1,14 @@ +load("//tools:defs.bzl", "go_binary") + +package(licenses = ["notice"]) + +go_binary( + name = "filter", + srcs = ["main.go"], + nogo = False, + visibility = ["//visibility:public"], + deps = [ + "//tools/nogo", + "@in_gopkg_yaml_v2//:go_default_library", + ], +) diff --git a/tools/nogo/filter/main.go b/tools/nogo/filter/main.go new file mode 100644 index 000000000..9cf41b3b0 --- /dev/null +++ b/tools/nogo/filter/main.go @@ -0,0 +1,131 @@ +// 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. + +// Binary check is the nogo entrypoint. +package main + +import ( + "flag" + "fmt" + "io/ioutil" + "log" + "os" + "strings" + + yaml "gopkg.in/yaml.v2" + "gvisor.dev/gvisor/tools/nogo" +) + +type stringList []string + +func (s *stringList) String() string { + return strings.Join(*s, ",") +} + +func (s *stringList) Set(value string) error { + *s = append(*s, value) + return nil +} + +var ( + inputFiles stringList + configFiles stringList + outputFile string + showConfig bool +) + +func init() { + flag.Var(&inputFiles, "input", "findings input files") + flag.StringVar(&outputFile, "output", "", "findings output file") + flag.Var(&configFiles, "config", "findings configuration files") + flag.BoolVar(&showConfig, "show-config", false, "dump configuration only") +} + +func main() { + flag.Parse() + + // Load all available findings. + var findings []nogo.Finding + for _, filename := range inputFiles { + inputFindings, err := nogo.ExtractFindingsFromFile(filename) + if err != nil { + log.Fatalf("unable to extract findings from %s: %v", filename, err) + } + findings = append(findings, inputFindings...) + } + + // Open and merge all configuations. + config := &nogo.Config{ + Global: make(nogo.AnalyzerConfig), + Analyzers: make(map[nogo.AnalyzerName]nogo.AnalyzerConfig), + } + for _, filename := range configFiles { + content, err := ioutil.ReadFile(filename) + if err != nil { + log.Fatalf("unable to read %s: %v", filename, err) + } + var newConfig nogo.Config // For current file. + if err := yaml.Unmarshal(content, &newConfig); err != nil { + log.Fatalf("unable to decode %s: %v", filename, err) + } + config.Merge(&newConfig) + if showConfig { + bytes, err := yaml.Marshal(&newConfig) + if err != nil { + log.Fatalf("error marshalling config: %v", err) + } + mergedBytes, err := yaml.Marshal(config) + if err != nil { + log.Fatalf("error marshalling config: %v", err) + } + fmt.Fprintf(os.Stdout, "Loaded configuration from %s:\n%s\n", filename, string(bytes)) + fmt.Fprintf(os.Stdout, "Merged configuration:\n%s\n", string(mergedBytes)) + } + } + if err := config.Compile(); err != nil { + log.Fatalf("error compiling config: %v", err) + } + if showConfig { + os.Exit(0) + } + + // Filter the findings (and aggregate by group). + filteredFindings := make([]nogo.Finding, 0, len(findings)) + for _, finding := range findings { + if ok := config.ShouldReport(finding); ok { + filteredFindings = append(filteredFindings, finding) + } + } + + // Write the output (if required). + // + // If the outputFile is specified, then we exit here. Otherwise, + // we continue to write to stdout and treat like a test. + if outputFile != "" { + if err := nogo.WriteFindingsToFile(filteredFindings, outputFile); err != nil { + log.Fatalf("unable to write findings: %v", err) + } + return + } + + // Treat the run as a test. + if len(filteredFindings) == 0 { + fmt.Fprintf(os.Stdout, "PASS\n") + os.Exit(0) + } + for _, finding := range filteredFindings { + fmt.Fprintf(os.Stdout, "%s\n", finding.String()) + } + os.Exit(1) +} diff --git a/tools/nogo/findings.go b/tools/nogo/findings.go new file mode 100644 index 000000000..5bd850269 --- /dev/null +++ b/tools/nogo/findings.go @@ -0,0 +1,63 @@ +// 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. + +package nogo + +import ( + "encoding/json" + "fmt" + "go/token" + "io/ioutil" +) + +// Finding is a single finding. +type Finding struct { + Category AnalyzerName + Position token.Position + Message string +} + +// String implements fmt.Stringer.String. +func (f *Finding) String() string { + return fmt.Sprintf("%s: %s: %s", f.Category, f.Position.String(), f.Message) +} + +// WriteFindingsToFile writes findings to a file. +func WriteFindingsToFile(findings []Finding, filename string) error { + content, err := WriteFindingsToBytes(findings) + if err != nil { + return err + } + return ioutil.WriteFile(filename, content, 0644) +} + +// WriteFindingsToBytes serializes findings as bytes. +func WriteFindingsToBytes(findings []Finding) ([]byte, error) { + return json.Marshal(findings) +} + +// ExtractFindingsFromFile loads findings from a file. +func ExtractFindingsFromFile(filename string) ([]Finding, error) { + content, err := ioutil.ReadFile(filename) + if err != nil { + return nil, err + } + return ExtractFindingsFromBytes(content) +} + +// ExtractFindingsFromBytes loads findings from bytes. +func ExtractFindingsFromBytes(content []byte) (findings []Finding, err error) { + err = json.Unmarshal(content, &findings) + return findings, err +} diff --git a/tools/nogo/io_bazel_rules_go-visibility.patch b/tools/nogo/io_bazel_rules_go-visibility.patch deleted file mode 100644 index 6b64b2e85..000000000 --- a/tools/nogo/io_bazel_rules_go-visibility.patch +++ /dev/null @@ -1,25 +0,0 @@ -diff --git a/third_party/org_golang_x_tools-extras.patch b/third_party/org_golang_x_tools-extras.patch -index 133fbccc..5f0d9a47 100644 ---- a/third_party/org_golang_x_tools-extras.patch -+++ b/third_party/org_golang_x_tools-extras.patch -@@ -32,7 +32,7 @@ diff -urN c/go/analysis/internal/facts/BUILD.bazel d/go/analysis/internal/facts/ - - go_library( - name = "go_default_library", --@@ -14,6 +14,23 @@ -+@@ -14,6 +14,20 @@ - ], - ) - -@@ -43,10 +43,7 @@ diff -urN c/go/analysis/internal/facts/BUILD.bazel d/go/analysis/internal/facts/ - + "imports.go", - + ], - + importpath = "golang.org/x/tools/go/analysis/internal/facts", --+ visibility = [ --+ "//go/analysis:__subpackages__", --+ "@io_bazel_rules_go//go/tools/builders:__pkg__", --+ ], -++ visibility = ["//visibility:public"], - + deps = [ - + "//go/analysis:go_tool_library", - + "//go/types/objectpath:go_tool_library", diff --git a/tools/nogo/matchers.go b/tools/nogo/matchers.go deleted file mode 100644 index 57a250501..000000000 --- a/tools/nogo/matchers.go +++ /dev/null @@ -1,143 +0,0 @@ -// 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. - -package nogo - -import ( - "go/token" - "path/filepath" - "regexp" - "strings" - - "golang.org/x/tools/go/analysis" -) - -type matcher interface { - ShouldReport(d analysis.Diagnostic, fs *token.FileSet) bool -} - -// pathRegexps filters explicit paths. -type pathRegexps struct { - expr []*regexp.Regexp - - // include, if true, indicates that paths matching any regexp in expr - // match. - // - // If false, paths matching no regexps in expr match. - include bool -} - -// buildRegexps builds a list of regular expressions. -// -// This will panic on error. -func buildRegexps(prefix string, args ...string) []*regexp.Regexp { - result := make([]*regexp.Regexp, 0, len(args)) - for _, arg := range args { - result = append(result, regexp.MustCompile(filepath.Join(prefix, arg))) - } - return result -} - -// ShouldReport implements matcher.ShouldReport. -func (p *pathRegexps) ShouldReport(d analysis.Diagnostic, fs *token.FileSet) bool { - fullPos := fs.Position(d.Pos).String() - for _, path := range p.expr { - if path.MatchString(fullPos) { - return p.include - } - } - return !p.include -} - -// internalExcluded excludes specific internal paths. -func internalExcluded(paths ...string) *pathRegexps { - return &pathRegexps{ - expr: buildRegexps(internalPrefix, paths...), - include: false, - } -} - -// excludedExcluded excludes specific external paths. -func externalExcluded(paths ...string) *pathRegexps { - return &pathRegexps{ - expr: buildRegexps(externalPrefix, paths...), - include: false, - } -} - -// internalMatches returns a path matcher for internal packages. -func internalMatches() *pathRegexps { - return &pathRegexps{ - expr: buildRegexps(internalPrefix, ".*"), - include: true, - } -} - -// resultExcluded excludes explicit message contents. -type resultExcluded []string - -// ShouldReport implements matcher.ShouldReport. -func (r resultExcluded) ShouldReport(d analysis.Diagnostic, _ *token.FileSet) bool { - for _, str := range r { - if strings.Contains(d.Message, str) { - return false - } - } - return true // Not excluded. -} - -// andMatcher is a composite matcher. -type andMatcher struct { - first matcher - second matcher -} - -// ShouldReport implements matcher.ShouldReport. -func (a *andMatcher) ShouldReport(d analysis.Diagnostic, fs *token.FileSet) bool { - return a.first.ShouldReport(d, fs) && a.second.ShouldReport(d, fs) -} - -// and is a syntactic convension for andMatcher. -func and(first matcher, second matcher) *andMatcher { - return &andMatcher{ - first: first, - second: second, - } -} - -// anyMatcher matches everything. -type anyMatcher struct{} - -// ShouldReport implements matcher.ShouldReport. -func (anyMatcher) ShouldReport(analysis.Diagnostic, *token.FileSet) bool { - return true -} - -// alwaysMatches returns an anyMatcher instance. -func alwaysMatches() anyMatcher { - return anyMatcher{} -} - -// neverMatcher will never match. -type neverMatcher struct{} - -// ShouldReport implements matcher.ShouldReport. -func (neverMatcher) ShouldReport(analysis.Diagnostic, *token.FileSet) bool { - return false -} - -// disableMatches returns a neverMatcher instance. -func disableMatches() neverMatcher { - return neverMatcher{} -} diff --git a/tools/nogo/nogo.go b/tools/nogo/nogo.go index ea1e97076..779d4d6d8 100644 --- a/tools/nogo/nogo.go +++ b/tools/nogo/nogo.go @@ -21,7 +21,6 @@ package nogo import ( "encoding/json" "errors" - "flag" "fmt" "go/ast" "go/build" @@ -32,51 +31,89 @@ import ( "io/ioutil" "log" "os" + "path" "path/filepath" "reflect" + "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/internal/facts" "golang.org/x/tools/go/gcexportdata" - "gvisor.dev/gvisor/tools/nogo/data" + + // Special case: flags live here and change overall behavior. + "gvisor.dev/gvisor/tools/checkescape" ) -// pkgConfig is serialized as the configuration. +// StdlibConfig is serialized as the configuration. // -// This contains everything required for the analysis. -type pkgConfig struct { - ImportPath string - GoFiles []string - NonGoFiles []string - Tags []string - GOOS string - GOARCH string - ImportMap map[string]string - FactMap map[string]string - FactOutput string - Objdump string - StdZip string +// This contains everything required for stdlib analysis. +type StdlibConfig struct { + Srcs []string + GOOS string + GOARCH string + Tags []string } -// loadFacts finds and loads facts per FactMap. -func (c *pkgConfig) loadFacts(path string) ([]byte, error) { - realPath, ok := c.FactMap[path] - if !ok { - return nil, nil // No facts available. - } +// PackageConfig is serialized as the configuration. +// +// This contains everything required for single package analysis. +type PackageConfig struct { + ImportPath string + GoFiles []string + NonGoFiles []string + Tags []string + GOOS string + GOARCH string + ImportMap map[string]string + FactMap map[string]string + StdlibFacts string +} - // Read the files file. - data, err := ioutil.ReadFile(realPath) - if err != nil { - return nil, err +// loader is a fact-loader function. +type loader func(string) ([]byte, error) + +// saver is a fact-saver function. +type saver func([]byte) error + +// factLoader returns a function that loads facts. +// +// This resolves all standard library facts and imported package facts up +// front. The returned loader function will never return an error, only +// empty facts. +// +// This is done because all stdlib data is stored together, and we don't want +// to load this data many times over. +func (c *PackageConfig) factLoader() (loader, error) { + allFacts := make(map[string][]byte) + if c.StdlibFacts != "" { + data, err := ioutil.ReadFile(c.StdlibFacts) + if err != nil { + return nil, fmt.Errorf("error loading stdlib facts from %q: %w", c.StdlibFacts, err) + } + var stdlibFacts map[string][]byte + if err := json.Unmarshal(data, &stdlibFacts); err != nil { + return nil, fmt.Errorf("error loading stdlib facts: %w", err) + } + for pkg, data := range stdlibFacts { + allFacts[pkg] = data + } } - return data, nil + for pkg, file := range c.FactMap { + data, err := ioutil.ReadFile(file) + if err != nil { + return nil, fmt.Errorf("error loading %q: %w", file, err) + } + allFacts[pkg] = data + } + return func(path string) ([]byte, error) { + return allFacts[path], nil + }, nil } // shouldInclude indicates whether the file should be included. // // NOTE: This does only basic parsing of tags. -func (c *pkgConfig) shouldInclude(path string) (bool, error) { +func (c *PackageConfig) shouldInclude(path string) (bool, error) { ctx := build.Default ctx.GOOS = c.GOOS ctx.GOARCH = c.GOARCH @@ -90,10 +127,11 @@ func (c *pkgConfig) shouldInclude(path string) (bool, error) { // files, and the facts. Note that this importer implementation will always // pass when a given package is not available. type importer struct { - pkgConfig - fset *token.FileSet - cache map[string]*types.Package - lastErr error + *PackageConfig + fset *token.FileSet + cache map[string]*types.Package + lastErr error + callback func(string) error } // Import implements types.Importer.Import. @@ -104,6 +142,17 @@ func (i *importer) Import(path string) (*types.Package, error) { // analyzers are specifically looking for this. return types.Unsafe, nil } + + // Call the internal callback. This is used to resolve loading order + // for the standard library. See checkStdlib. + if i.callback != nil { + if err := i.callback(path); err != nil { + i.lastErr = err + return nil, err + } + } + + // Actually load the data. realPath, ok := i.ImportMap[path] var ( rc io.ReadCloser @@ -112,7 +161,7 @@ func (i *importer) Import(path string) (*types.Package, error) { if !ok { // Not found in the import path. Attempt to find the package // via the standard library. - rc, err = i.findStdPkg(path) + rc, err = findStdPkg(i.GOOS, i.GOARCH, path) } else { // Open the file. rc, err = os.Open(realPath) @@ -135,7 +184,165 @@ func (i *importer) Import(path string) (*types.Package, error) { // ErrSkip indicates the package should be skipped. var ErrSkip = errors.New("skipped") -// checkPackage runs all analyzers. +// CheckStdlib checks the standard library. +// +// This constructs a synthetic package configuration for each library in the +// standard library sources, and call CheckPackage repeatedly. +// +// Note that not all parts of the source are expected to build. We skip obvious +// test files, and cmd files, which should not be dependencies. +func CheckStdlib(config *StdlibConfig, analyzers []*analysis.Analyzer) (allFindings []Finding, facts []byte, err error) { + if len(config.Srcs) == 0 { + return nil, nil, nil + } + + // Ensure all paths are normalized. + for i := 0; i < len(config.Srcs); i++ { + config.Srcs[i] = path.Clean(config.Srcs[i]) + } + + // Calculate the root source directory. This is always a directory + // named 'src', of which we simply take the first we find. This is a + // bit fragile, but works for all currently known Go source + // configurations. + // + // Note that there may be extra files outside of the root source + // directory; we simply ignore those. + rootSrcPrefix := "" + for _, file := range config.Srcs { + const src = "/src/" + i := strings.Index(file, src) + if i == -1 { + // Superfluous file. + continue + } + + // Index of first character after /src/. + i += len(src) + rootSrcPrefix = file[:i] + break + } + + // Aggregate all files by directory. + packages := make(map[string]*PackageConfig) + for _, file := range config.Srcs { + if !strings.HasPrefix(file, rootSrcPrefix) { + // Superflouous file. + continue + } + + d := path.Dir(file) + if len(rootSrcPrefix) >= len(d) { + continue // Not a file. + } + pkg := d[len(rootSrcPrefix):] + // Skip cmd packages and obvious test files: see above. + if strings.HasPrefix(pkg, "cmd/") || strings.HasSuffix(file, "_test.go") { + continue + } + c, ok := packages[pkg] + if !ok { + c = &PackageConfig{ + ImportPath: pkg, + GOOS: config.GOOS, + GOARCH: config.GOARCH, + Tags: config.Tags, + } + packages[pkg] = c + } + // Add the files appropriately. Note that they will be further + // filtered by architecture and build tags below, so this need + // not be done immediately. + if strings.HasSuffix(file, ".go") { + c.GoFiles = append(c.GoFiles, file) + } else { + c.NonGoFiles = append(c.NonGoFiles, file) + } + } + + // Closure to check a single package. + stdlibFacts := make(map[string][]byte) + stdlibErrs := make(map[string]error) + var checkOne func(pkg string) error // Recursive. + checkOne = func(pkg string) error { + // Is this already done? + if _, ok := stdlibFacts[pkg]; ok { + return nil + } + // Did this fail previously? + if _, ok := stdlibErrs[pkg]; ok { + return nil + } + + // Lookup the configuration. + config, ok := packages[pkg] + if !ok { + return nil // Not known. + } + + // Find the binary package, and provide to objdump. + rc, err := findStdPkg(config.GOOS, config.GOARCH, pkg) + if err != nil { + // If there's no binary for this package, it is likely + // not built with the distribution. That's fine, we can + // just skip analysis. + stdlibErrs[pkg] = err + return nil + } + + // Provide the input. + oldReader := checkescape.Reader + checkescape.Reader = rc // For analysis. + defer func() { + rc.Close() + checkescape.Reader = oldReader // Restore. + }() + + // Run the analysis. + findings, factData, err := CheckPackage(config, analyzers, checkOne) + if err != nil { + // If we can't analyze a package from the standard library, + // then we skip it. It will simply not have any findings. + stdlibErrs[pkg] = err + return nil + } + stdlibFacts[pkg] = factData + allFindings = append(allFindings, findings...) + return nil + } + + // Check all packages. + // + // Note that this may call checkOne recursively, so it's not guaranteed + // to evaluate in the order provided here. We do ensure however, that + // all packages are evaluated. + for pkg := range packages { + if err := checkOne(pkg); err != nil { + return nil, nil, err + } + } + + // Sanity check. + if len(stdlibFacts) == 0 { + return nil, nil, fmt.Errorf("no stdlib facts found: misconfiguration?") + } + + // Write out all findings. + factData, err := json.Marshal(stdlibFacts) + if err != nil { + return nil, nil, fmt.Errorf("error saving stdlib facts: %w", err) + } + + // Write out all errors. + for pkg, err := range stdlibErrs { + log.Printf("WARNING: error while processing %v: %v", pkg, err) + } + + // Return all findings. + return allFindings, factData, nil +} + +// CheckPackage runs all given analyzers. // // The implementation was adapted from [1], which was in turn adpated from [2]. // This returns a list of matching analysis issues, or an error if the analysis @@ -143,11 +350,12 @@ var ErrSkip = errors.New("skipped") // // [1] bazelbuid/rules_go/tools/builders/nogo_main.go // [2] golang.org/x/tools/go/checker/internal/checker -func checkPackage(config pkgConfig) ([]string, error) { +func CheckPackage(config *PackageConfig, analyzers []*analysis.Analyzer, importCallback func(string) error) (findings []Finding, factData []byte, err error) { imp := &importer{ - pkgConfig: config, - fset: token.NewFileSet(), - cache: make(map[string]*types.Package), + PackageConfig: config, + fset: token.NewFileSet(), + cache: make(map[string]*types.Package), + callback: importCallback, } // Load all source files. @@ -155,14 +363,14 @@ func checkPackage(config pkgConfig) ([]string, error) { for _, file := range config.GoFiles { include, err := config.shouldInclude(file) if err != nil { - return nil, fmt.Errorf("error evaluating file %q: %v", file, err) + return nil, nil, fmt.Errorf("error evaluating file %q: %v", file, err) } if !include { continue } s, err := parser.ParseFile(imp.fset, file, nil, parser.ParseComments) if err != nil { - return nil, fmt.Errorf("error parsing file %q: %v", file, err) + return nil, nil, fmt.Errorf("error parsing file %q: %v", file, err) } syntax = append(syntax, s) } @@ -180,22 +388,22 @@ func checkPackage(config pkgConfig) ([]string, error) { } types, err := typeConfig.Check(config.ImportPath, imp.fset, syntax, typesInfo) if err != nil && imp.lastErr != ErrSkip { - return nil, fmt.Errorf("error checking types: %w", err) + return nil, nil, fmt.Errorf("error checking types: %w", err) } // Load all package facts. - facts, err := facts.Decode(types, config.loadFacts) + loader, err := config.factLoader() if err != nil { - return nil, fmt.Errorf("error decoding facts: %w", err) + return nil, nil, fmt.Errorf("error loading facts: %w", err) + } + facts, err := facts.Decode(types, loader) + if err != nil { + return nil, nil, fmt.Errorf("error decoding facts: %w", err) } - - // Set the binary global for use. - data.Objdump = config.Objdump // Register fact types and establish dependencies between analyzers. // The visit closure will execute recursively, and populate results // will all required analysis results. - diagnostics := make(map[*analysis.Analyzer][]analysis.Diagnostic) results := make(map[*analysis.Analyzer]interface{}) var visit func(*analysis.Analyzer) error // For recursion. visit = func(a *analysis.Analyzer) error { @@ -210,27 +418,25 @@ func checkPackage(config pkgConfig) ([]string, error) { } } - // Prepare the matcher. - m := analyzerConfig[a] - report := func(d analysis.Diagnostic) { - if m.ShouldReport(d, imp.fset) { - diagnostics[a] = append(diagnostics[a], d) - } - } - // Run the analysis. factFilter := make(map[reflect.Type]bool) for _, f := range a.FactTypes { factFilter[reflect.TypeOf(f)] = true } p := &analysis.Pass{ - Analyzer: a, - Fset: imp.fset, - Files: syntax, - Pkg: types, - TypesInfo: typesInfo, - ResultOf: results, // All results. - Report: report, + Analyzer: a, + Fset: imp.fset, + Files: syntax, + Pkg: types, + TypesInfo: typesInfo, + ResultOf: results, // All results. + Report: func(d analysis.Diagnostic) { + findings = append(findings, Finding{ + Category: AnalyzerName(a.Name), + Position: imp.fset.Position(d.Pos), + Message: d.Message, + }) + }, ImportPackageFact: facts.ImportPackageFact, ExportPackageFact: facts.ExportPackageFact, ImportObjectFact: facts.ImportObjectFact, @@ -252,75 +458,16 @@ func checkPackage(config pkgConfig) ([]string, error) { return nil // Success. } - // Visit all analysis recursively. - for a, _ := range analyzerConfig { + // Visit all analyzers recursively. + for _, a := range analyzers { if imp.lastErr == ErrSkip { continue // No local analysis. } if err := visit(a); err != nil { - return nil, err // Already has context. - } - } - - // Write the output file. - if config.FactOutput != "" { - factData := facts.Encode() - if err := ioutil.WriteFile(config.FactOutput, factData, 0644); err != nil { - return nil, fmt.Errorf("error: unable to open facts output %q: %v", config.FactOutput, err) - } - } - - // Convert all diagnostics to strings. - findings := make([]string, 0, len(diagnostics)) - for a, ds := range diagnostics { - for _, d := range ds { - // Include the anlyzer name for debugability and configuration. - findings = append(findings, fmt.Sprintf("%s: %s: %s", a.Name, imp.fset.Position(d.Pos), d.Message)) + return nil, nil, err // Already has context. } } // Return all findings. - return findings, nil -} - -var ( - configFile = flag.String("config", "", "configuration file (in JSON format)") -) - -// Main is the entrypoint; it should be called directly from main. -// -// N.B. This package registers it's own flags. -func Main() { - // Parse all flags. - flag.Parse() - - // Load the configuration. - f, err := os.Open(*configFile) - if err != nil { - log.Fatalf("unable to open configuration %q: %v", *configFile, err) - } - defer f.Close() - config := new(pkgConfig) - dec := json.NewDecoder(f) - dec.DisallowUnknownFields() - if err := dec.Decode(config); err != nil { - log.Fatalf("unable to decode configuration: %v", err) - } - - // Process the package. - findings, err := checkPackage(*config) - if err != nil { - log.Fatalf("error checking package: %v", err) - } - - // No findings? - if len(findings) == 0 { - os.Exit(0) - } - - // Print findings and exit with non-zero code. - for _, finding := range findings { - fmt.Fprintf(os.Stdout, "%s\n", finding) - } - os.Exit(1) + return findings, facts.Encode(), nil } diff --git a/tools/nogo/register.go b/tools/nogo/register.go deleted file mode 100644 index 62b499661..000000000 --- a/tools/nogo/register.go +++ /dev/null @@ -1,64 +0,0 @@ -// 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. - -package nogo - -import ( - "encoding/gob" - "log" - - "golang.org/x/tools/go/analysis" -) - -// analyzers returns all configured analyzers. -func analyzers() (all []*analysis.Analyzer) { - for a, _ := range analyzerConfig { - all = append(all, a) - } - return all -} - -func init() { - // Validate basic configuration. - if err := analysis.Validate(analyzers()); err != nil { - log.Fatalf("unable to validate analyzer: %v", err) - } - - // Register all fact types. - // - // N.B. This needs to be done recursively, because there may be - // analyzers in the Requires list that do not appear explicitly above. - registered := make(map[*analysis.Analyzer]struct{}) - var register func(*analysis.Analyzer) - register = func(a *analysis.Analyzer) { - if _, ok := registered[a]; ok { - return - } - - // Regsiter dependencies. - for _, da := range a.Requires { - register(da) - } - - // Register local facts. - for _, f := range a.FactTypes { - gob.Register(f) - } - - registered[a] = struct{}{} // Done. - } - for _, a := range analyzers() { - register(a) - } -} diff --git a/tools/parsers/BUILD b/tools/parsers/BUILD new file mode 100644 index 000000000..dab954e25 --- /dev/null +++ b/tools/parsers/BUILD @@ -0,0 +1,41 @@ +load("//tools:defs.bzl", "go_binary", "go_library", "go_test") + +package(licenses = ["notice"]) + +go_test( + name = "parsers_test", + size = "small", + srcs = ["go_parser_test.go"], + library = ":parsers", + nogo = False, + deps = [ + "//tools/bigquery", + "@com_github_google_go_cmp//cmp:go_default_library", + ], +) + +go_library( + name = "parsers", + testonly = 1, + srcs = [ + "go_parser.go", + ], + nogo = False, + visibility = ["//:sandbox"], + deps = [ + "//test/benchmarks/tools", + "//tools/bigquery", + ], +) + +go_binary( + name = "parser", + testonly = 1, + srcs = ["parser_main.go"], + nogo = False, + deps = [ + ":parsers", + "//runsc/flag", + "//tools/bigquery", + ], +) diff --git a/tools/parsers/go_parser.go b/tools/parsers/go_parser.go new file mode 100644 index 000000000..df4875e6a --- /dev/null +++ b/tools/parsers/go_parser.go @@ -0,0 +1,150 @@ +// Copyright 2020 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 parsers holds parsers to parse Benchmark test output. +// +// Parsers parse Benchmark test output and place it in BigQuery +// structs for sending to BigQuery databases. +package parsers + +import ( + "fmt" + "strconv" + "strings" + + "gvisor.dev/gvisor/test/benchmarks/tools" + "gvisor.dev/gvisor/tools/bigquery" +) + +// ParseOutput expects golang benchmark output and returns a struct formatted +// for BigQuery. +func ParseOutput(output string, name string, official bool) (*bigquery.Suite, error) { + suite := bigquery.NewSuite(name) + lines := strings.Split(output, "\n") + for _, line := range lines { + bm, err := parseLine(line, official) + if err != nil { + return nil, fmt.Errorf("failed to parse line '%s': %v", line, err) + } + if bm != nil { + suite.Benchmarks = append(suite.Benchmarks, bm) + } + } + return suite, nil +} + +// parseLine handles parsing a benchmark line into a bigquery.Benchmark. +// +// Example: "BenchmarkRuby/server_threads.1-6 1 1397875880 ns/op 140 requests_per_second.QPS" +// +// This function will return the following benchmark: +// *bigquery.Benchmark{ +// Name: BenchmarkRuby +// []*bigquery.Condition{ +// {Name: GOMAXPROCS, 6} +// {Name: server_threads, 1} +// } +// []*bigquery.Metric{ +// {Name: ns/op, Unit: ns/op, Sample: 1397875880} +// {Name: requests_per_second, Unit: QPS, Sample: 140 } +// } +//} +func parseLine(line string, official bool) (*bigquery.Benchmark, error) { + fields := strings.Fields(line) + + // Check if this line is a Benchmark line. Otherwise ignore the line. + if len(fields) < 2 || !strings.HasPrefix(fields[0], "Benchmark") { + return nil, nil + } + + iters, err := strconv.Atoi(fields[1]) + if err != nil { + return nil, fmt.Errorf("expecting number of runs, got %s: %v", fields[1], err) + } + + name, params, err := parseNameParams(fields[0]) + if err != nil { + return nil, fmt.Errorf("parse name/params: %v", err) + } + + bm := bigquery.NewBenchmark(name, iters, official) + for _, p := range params { + bm.AddCondition(p.Name, p.Value) + } + + for i := 1; i < len(fields)/2; i++ { + value := fields[2*i] + metric := fields[2*i+1] + if err := makeMetric(bm, value, metric); err != nil { + return nil, fmt.Errorf("makeMetric on metric %q value: %s: %v", metric, value, err) + } + } + return bm, nil +} + +// parseNameParams parses the Name, GOMAXPROCS, and Params from the test. +// Field here should be of the format TESTNAME/PARAMS-GOMAXPROCS. +// Parameters will be separated by a "/" with individual params being +// "name.value". +func parseNameParams(field string) (string, []*tools.Parameter, error) { + var params []*tools.Parameter + // Remove GOMAXPROCS from end. + maxIndex := strings.LastIndex(field, "-") + if maxIndex < 0 { + return "", nil, fmt.Errorf("GOMAXPROCS not found: %s", field) + } + maxProcs := field[maxIndex+1:] + params = append(params, &tools.Parameter{ + Name: "GOMAXPROCS", + Value: maxProcs, + }) + + remainder := field[0:maxIndex] + index := strings.Index(remainder, "/") + if index == -1 { + return remainder, params, nil + } + + name := remainder[0:index] + p := remainder[index+1:] + + ps, err := tools.NameToParameters(p) + if err != nil { + return "", nil, fmt.Errorf("NameToParameters %s: %v", field, err) + } + params = append(params, ps...) + return name, params, nil +} + +// makeMetric parses metrics and adds them to the passed Benchmark. +func makeMetric(bm *bigquery.Benchmark, value, metric string) error { + switch metric { + // Ignore most output from golang benchmarks. + case "MB/s", "B/op", "allocs/op": + return nil + case "ns/op": + val, err := strconv.ParseFloat(value, 64) + if err != nil { + return fmt.Errorf("ParseFloat %s: %v", value, err) + } + bm.AddMetric(metric /*metric name*/, metric /*unit*/, val /*sample*/) + default: + m, err := tools.ParseCustomMetric(value, metric) + if err != nil { + return fmt.Errorf("ParseCustomMetric %s: %v ", metric, err) + } + bm.AddMetric(m.Name, m.Unit, m.Sample) + } + return nil +} diff --git a/tools/parsers/go_parser_test.go b/tools/parsers/go_parser_test.go new file mode 100644 index 000000000..0aa1152a2 --- /dev/null +++ b/tools/parsers/go_parser_test.go @@ -0,0 +1,169 @@ +// Copyright 2020 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 parsers + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "gvisor.dev/gvisor/tools/bigquery" +) + +func TestParseLine(t *testing.T) { + testCases := []struct { + name string + data string + want *bigquery.Benchmark + }{ + { + name: "Iperf", + data: "BenchmarkIperf/Upload-6 1 11094914892 ns/op 4751711232 bandwidth.bytes_per_second", + want: &bigquery.Benchmark{ + Name: "BenchmarkIperf", + Condition: []*bigquery.Condition{ + { + Name: "GOMAXPROCS", + Value: "6", + }, + { + Name: "Upload", + Value: "Upload", + }, + }, + Metric: []*bigquery.Metric{ + { + Name: "ns/op", + Unit: "ns/op", + Sample: 11094914892.0, + }, + { + Name: "bandwidth", + Unit: "bytes_per_second", + Sample: 4751711232.0, + }, + }, + }, + }, + { + name: "Ruby", + data: "BenchmarkRuby/server_threads.1-6 1 1397875880 ns/op 0.00710 average_latency.s 140 requests_per_second.QPS", + want: &bigquery.Benchmark{ + Name: "BenchmarkRuby", + Condition: []*bigquery.Condition{ + { + Name: "GOMAXPROCS", + Value: "6", + }, + { + Name: "server_threads", + Value: "1", + }, + }, + Metric: []*bigquery.Metric{ + { + Name: "ns/op", + Unit: "ns/op", + Sample: 1397875880.0, + }, + { + Name: "average_latency", + Unit: "s", + Sample: 0.00710, + }, + { + Name: "requests_per_second", + Unit: "QPS", + Sample: 140.0, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseLine(tc.data, false) + if err != nil { + t.Fatalf("parseLine failed with: %v", err) + } + + if !cmp.Equal(tc.want, got, nil) { + for _, c := range got.Condition { + t.Logf("Cond: %+v", c) + } + for _, m := range got.Metric { + t.Logf("Metric: %+v", m) + } + t.Fatalf("Compare failed want: %+v got: %+v", tc.want, got) + } + }) + + } +} + +func TestParseOutput(t *testing.T) { + testCases := []struct { + name string + data string + numBenchmarks int + numMetrics int + numConditions int + }{ + { + name: "Startup", + data: ` + BenchmarkStartupEmpty + BenchmarkStartupEmpty-6 2 766377884 ns/op 1 allocs/op + BenchmarkStartupNode + BenchmarkStartupNode-6 1 1752158409 ns/op 1 allocs/op + `, + numBenchmarks: 2, + numMetrics: 1, + numConditions: 1, + }, + { + name: "Ruby", + data: `BenchmarkRuby +BenchmarkRuby/server_threads.1 +BenchmarkRuby/server_threads.1-6 1 1397875880 ns/op 0.00710 average_latency.s 140 requests_per_second.QPS +BenchmarkRuby/server_threads.5 +BenchmarkRuby/server_threads.5-6 1 1416003331 ns/op 0.00950 average_latency.s 465 requests_per_second.QPS`, + numBenchmarks: 2, + numMetrics: 3, + numConditions: 2, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + suite, err := ParseOutput(tc.data, "", false) + if err != nil { + t.Fatalf("parseOutput failed: %v", err) + } else if len(suite.Benchmarks) != tc.numBenchmarks { + t.Fatalf("NumBenchmarks failed want: %d got: %d %+v", tc.numBenchmarks, len(suite.Benchmarks), suite.Benchmarks) + } + + for _, bm := range suite.Benchmarks { + if len(bm.Metric) != tc.numMetrics { + t.Fatalf("NumMetrics failed want: %d got: %d %+v", tc.numMetrics, len(bm.Metric), bm.Metric) + } + + if len(bm.Condition) != tc.numConditions { + t.Fatalf("NumConditions failed want: %d got: %d %+v", tc.numConditions, len(bm.Condition), bm.Condition) + } + } + }) + } +} diff --git a/tools/parsers/parser_main.go b/tools/parsers/parser_main.go new file mode 100644 index 000000000..6c6182464 --- /dev/null +++ b/tools/parsers/parser_main.go @@ -0,0 +1,129 @@ +// Copyright 2020 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. + +// Binary parser parses Benchmark data from golang benchmarks, +// puts it into a Schema for BigQuery, and sends it to BigQuery. +// parser will also initialize a table with the Benchmarks BigQuery schema. +package main + +import ( + "context" + "fmt" + "io/ioutil" + "os" + + "gvisor.dev/gvisor/runsc/flag" + bq "gvisor.dev/gvisor/tools/bigquery" + "gvisor.dev/gvisor/tools/parsers" +) + +const ( + initString = "init" + initDescription = "initializes a new table with benchmarks schema" + parseString = "parse" + parseDescription = "parses given benchmarks file and sends it to BigQuery table." +) + +var ( + // The init command will create a new dataset/table in the given project and initialize + // the table with the schema in //tools/bigquery/bigquery.go. If the table/dataset exists + // or has been initialized, init has no effect and successfully returns. + initCmd = flag.NewFlagSet(initString, flag.ContinueOnError) + initProject = initCmd.String("project", "", "GCP project to send benchmarks.") + initDataset = initCmd.String("dataset", "", "dataset to send benchmarks data.") + initTable = initCmd.String("table", "", "table to send benchmarks data.") + + // The parse command parses benchmark data in `file` and sends it to the + // requested table. + parseCmd = flag.NewFlagSet(parseString, flag.ContinueOnError) + file = parseCmd.String("file", "", "file to parse for benchmarks") + name = parseCmd.String("suite_name", "", "name of the benchmark suite") + clNumber = parseCmd.String("cl", "", "changelist number of this run") + gitCommit = parseCmd.String("git_commit", "", "git commit sha for this run") + parseProject = parseCmd.String("project", "", "GCP project to send benchmarks.") + parseDataset = parseCmd.String("dataset", "", "dataset to send benchmarks data.") + parseTable = parseCmd.String("table", "", "table to send benchmarks data.") + official = parseCmd.Bool("official", false, "mark input data as official.") +) + +// initBenchmarks initializes a dataset/table in a BigQuery project. +func initBenchmarks(ctx context.Context) error { + return bq.InitBigQuery(ctx, *initProject, *initDataset, *initTable, nil) +} + +// parseBenchmarks parses the given file into the BigQuery schema, +// adds some custom data for the commit, and sends the data to BigQuery. +func parseBenchmarks(ctx context.Context) error { + data, err := ioutil.ReadFile(*file) + if err != nil { + return fmt.Errorf("failed to read file: %v", err) + } + suite, err := parsers.ParseOutput(string(data), *name, *official) + if err != nil { + return fmt.Errorf("failed parse data: %v", err) + } + extraConditions := []*bq.Condition{ + { + Name: "change_list", + Value: *clNumber, + }, + { + Name: "commit", + Value: *gitCommit, + }, + } + + suite.Conditions = append(suite.Conditions, extraConditions...) + return bq.SendBenchmarks(ctx, suite, *parseProject, *parseDataset, *parseTable, nil) +} + +func main() { + ctx := context.Background() + switch { + // the "init" command + case len(os.Args) >= 2 && os.Args[1] == initString: + if err := initCmd.Parse(os.Args[2:]); err != nil { + fmt.Fprintf(os.Stderr, "failed parse flags: %v", err) + os.Exit(1) + } + if err := initBenchmarks(ctx); err != nil { + failure := "failed to initialize project: %s dataset: %s table: %s: %v" + fmt.Fprintf(os.Stderr, failure, *parseProject, *parseDataset, *parseTable, err) + os.Exit(1) + } + // the "parse" command. + case len(os.Args) >= 2 && os.Args[1] == parseString: + if err := parseCmd.Parse(os.Args[2:]); err != nil { + fmt.Fprintf(os.Stderr, "failed parse flags: %v", err) + os.Exit(1) + } + if err := parseBenchmarks(ctx); err != nil { + fmt.Fprintf(os.Stderr, "failed parse benchmarks: %v", err) + os.Exit(1) + } + default: + printUsage() + } +} + +// printUsage prints the top level usage string. +func printUsage() { + usage := `Usage: parser <command> <flags> ... + +Available commands: + %s %s + %s %s +` + fmt.Fprintf(os.Stderr, usage, initCmd.Name(), initDescription, parseCmd.Name(), parseDescription) +} diff --git a/tools/rules_go.patch b/tools/rules_go.patch new file mode 100644 index 000000000..5e1e87084 --- /dev/null +++ b/tools/rules_go.patch @@ -0,0 +1,14 @@ +diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl +index 17516ad7..76b6c68c 100644 +--- a/go/private/rules/test.bzl ++++ b/go/private/rules/test.bzl +@@ -121,9 +121,6 @@ def _go_test_impl(ctx): + ) + + test_gc_linkopts = gc_linkopts(ctx) +- if not go.mode.debug: +- # Disable symbol table and DWARF generation for test binaries. +- test_gc_linkopts.extend(["-s", "-w"]) + + # Now compile the test binary itself + test_library = GoLibrary( diff --git a/tools/tag_release.sh b/tools/tag_release.sh index b0bab74b4..50378065e 100755 --- a/tools/tag_release.sh +++ b/tools/tag_release.sh @@ -43,7 +43,7 @@ fi closest_commit() { while read line; do - if [[ "$line" =~ "commit " ]]; then + if [[ "$line" =~ ^"commit " ]]; then current_commit="${line#commit }" continue elif [[ "$line" =~ "PiperOrigin-RevId: " ]]; then @@ -57,7 +57,9 @@ closest_commit() { # Is the passed identifier a sha commit? if ! git show "${target_commit}" &> /dev/null; then # Extract the commit given a piper ID. - declare -r commit="$(git log | closest_commit "${target_commit}")" + commit="$(set +o pipefail; \ + git log --first-parent | closest_commit "${target_commit}")" + declare -r commit else declare -r commit="${target_commit}" fi |