Skip to content

Commit

Permalink
nogo: use original source files instead of coverage-instrumented (#3770)
Browse files Browse the repository at this point in the history
* nogo: use original source files for static analysis instead of coverage instrumented

* nogo: cleanup nogoSrcsOrigin

* nogo: use goSrcsNogo directly to run nogo static analysis

* nogo: add unit test to demonstrate how the issue is fixed

* nogo: update per review comments

* nogo: attempt to get test to pass on windows
  • Loading branch information
emmaxy authored Jan 4, 2024
1 parent cea5f9f commit 6236dd8
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 33 deletions.
1 change: 1 addition & 0 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ tasks:
- "-//tests/core/go_proto_library_importmap:importmap_test"
- "-//tests/core/go_test:data_test"
- "-//tests/core/go_test:pwd_test"
- "-//tests/core/nogo/coverage:coverage_cgo_test"
- "-//tests/core/nogo/coverage:coverage_test"
- "-//tests/core/nogo/coverage:gen_code_test"
- "-//tests/core/race:race_test" # fails on Windows due to upstream bug, see issue #2911
Expand Down
54 changes: 21 additions & 33 deletions go/tools/builders/compilepkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,15 +203,6 @@ func compileArchive(
}
defer cleanup()

// As part of compilation process, rules_go does generate and/or rewrite code
// based on the original source files. We should only run static analysis
// over original source files and not the generated source as end users have
// little control over the generated source.
//
// nogoSrcsOrigin maps generated/rewritten source files back to original source.
// If the original source path is an empty string, exclude generated source from nogo run.
nogoSrcsOrigin := make(map[string]string)

if len(srcs.goSrcs) == 0 {
// We need to run the compiler to create a valid archive, even if there's nothing in it.
// Otherwise, GoPack will complain if we try to add assembly or cgo objects.
Expand All @@ -237,7 +228,6 @@ func compileArchive(
pkg: "empty",
})

nogoSrcsOrigin[emptyGoFile.Name()] = ""
}
packageName := srcs.goSrcs[0].pkg
var goSrcs, cgoSrcs []string
Expand Down Expand Up @@ -280,8 +270,18 @@ func compileArchive(
// constraints.
compilingWithCgo := haveCgo && cgoEnabled

// When coverage is set, source files will be modified during instrumentation. We should only run static analysis
// over original source files and not the modified ones.
// goSrcsNogo and cgoSrcsNogo are copies of the original source files for nogo to run static analysis.
goSrcsNogo := goSrcs
cgoSrcsNogo := cgoSrcs

// Instrument source files for coverage.
if coverMode != "" {
// deep copy original source files for nogo static analysis, avoid being modified by coverage.
goSrcsNogo = append([]string{}, goSrcs...)
cgoSrcsNogo = append([]string{}, cgoSrcs...)

relCoverPath := make(map[string]string)
for _, s := range coverSrcs {
relCoverPath[abs(s)] = s
Expand Down Expand Up @@ -324,7 +324,6 @@ func compileArchive(

if i < len(goSrcs) {
goSrcs[i] = coverSrc
nogoSrcsOrigin[coverSrc] = origSrc
continue
}

Expand All @@ -343,6 +342,15 @@ func compileArchive(
if err != nil {
return err
}
if coverMode != "" && nogoPath != "" {
// Compile original source files, not coverage instrumented, for nogo
_, goSrcsNogo, _, err = cgo2(goenv, goSrcsNogo, cgoSrcsNogo, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath)
if err != nil {
return err
}
} else {
goSrcsNogo = goSrcs
}

gcFlags = append(gcFlags, createTrimPath(gcFlags, srcDir))
} else {
Expand Down Expand Up @@ -429,31 +437,11 @@ func compileArchive(
// Run nogo concurrently.
var nogoChan chan error
outFactsPath := filepath.Join(workDir, nogoFact)
nogoSrcs := make([]string, 0, len(goSrcs))
for _, goSrc := range goSrcs {
// If source is found in the origin map, that means it's likely to be a generated source file
// so feed the original source file to static analyzers instead of the generated one.
//
// If origin is empty, that means the generated source file is not based on a user-provided source file
// thus ignore that entry entirely.
if originSrc, ok := nogoSrcsOrigin[goSrc]; ok {
if originSrc != "" {
nogoSrcs = append(nogoSrcs, originSrc)
}
continue
}

// TODO(sluongng): most likely what remains here are CGO-generated source files as the result of calling cgo2()
// Need to determine whether we want to feed these CGO-generated files into static analyzers.
//
// Add unknown origin source files into the mix.
nogoSrcs = append(nogoSrcs, goSrc)
}
if nogoPath != "" && len(nogoSrcs) > 0 {
if nogoPath != "" && len(goSrcsNogo) > 0 {
ctx, cancel := context.WithCancel(context.Background())
nogoChan = make(chan error)
go func() {
nogoChan <- runNogo(ctx, workDir, nogoPath, nogoSrcs, deps, packagePath, importcfgPath, outFactsPath)
nogoChan <- runNogo(ctx, workDir, nogoPath, goSrcsNogo, deps, packagePath, importcfgPath, outFactsPath)
}()
defer func() {
if nogoChan != nil {
Expand Down
5 changes: 5 additions & 0 deletions tests/core/nogo/coverage/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
load("@io_bazel_rules_go//go/tools/bazel_testing:def.bzl", "go_bazel_test")

go_bazel_test(
name = "coverage_cgo_test",
srcs = ["coverage_cgo_test.go"],
)

go_bazel_test(
name = "coverage_test",
srcs = ["coverage_test.go"],
Expand Down
8 changes: 8 additions & 0 deletions tests/core/nogo/coverage/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,19 @@ nogo test with coverage
=======================

.. _nogo: /go/nogo.rst
.. _#3769: https://github.com/bazelbuild/rules_go/issues/3769
.. _#1940: https://github.com/bazelbuild/rules_go/issues/1940
.. _#2146: https://github.com/bazelbuild/rules_go/issues/2146

Tests to ensure that `nogo`_ works with coverage.

coverage_cgo_test
-------------
Checks that `nogo`_ works when both cgo and coverage are enabled. With coverage
instrumentation modifying source files, and cgo compilation changing files paths,
`nogo`_ should be running static analysis against original source files, instead
of modified ones. Verifies `#3769`_.

coverage_test
-------------
Checks that `nogo`_ works when coverage is enabled. All covered libraries gain
Expand Down
124 changes: 124 additions & 0 deletions tests/core/nogo/coverage/coverage_cgo_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
// Copyright 2019 The Bazel Authors. All rights reserved.
//
// 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 coverage_cgo_test

import (
"testing"

"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
)

func TestMain(m *testing.M) {
bazel_testing.TestMain(m, bazel_testing.Args{
Main: `
-- BUILD.bazel --
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test", "nogo")
go_library(
name = "foo_cgo",
cgo = True,
srcs = ["foo_cgo.go"],
importpath = "foo_cgo"
)
go_test(
name = "foo_cgo_test",
srcs = ["foo_cgo_test.go"],
embed = [":foo_cgo"]
)
nogo(
name = "nogo",
deps = ["//noinit"],
visibility = ["//visibility:public"],
)
-- foo_cgo.go --
package foo_cgo
import "C"
func FooCgo() string {
return "foo_cgo"
}
-- foo_cgo_test.go --
package foo_cgo
import "testing"
func TestFooCgo(t *testing.T) {
if actual, expected := FooCgo(), "foo_cgo"; actual != expected {
t.Errorf("FooCgo() should return foo_cgo")
}
}
-- noinit/BUILD.bazel --
load("@io_bazel_rules_go//go:def.bzl", "go_library")
go_library(
name = "noinit",
srcs = ["analyzer.go"],
importpath = "noinit",
visibility = ["//visibility:public"],
deps = [
"@org_golang_x_tools//go/analysis",
],
)
-- noinit/analyzer.go --
package noinit
import (
"fmt"
"go/ast"
"golang.org/x/tools/go/analysis"
)
const Name = "gochecknoinits"
var Analyzer = &analysis.Analyzer{
Name: Name,
Doc: "Checks that no init() functions are present in Go code",
Run: run,
}
func run(pass *analysis.Pass) (interface{}, error) {
for _, f := range pass.Files {
for _, decl := range f.Decls {
funcDecl, ok := decl.(*ast.FuncDecl)
if !ok {
continue
}
name := funcDecl.Name.Name
if name == "init" && funcDecl.Recv.NumFields() == 0 {
pass.Report(analysis.Diagnostic{
Pos: funcDecl.Pos(),
Message: fmt.Sprintf("don't use init function"),
Category: Name,
})
}
}
}
return nil, nil
}
`,
Nogo: `@//:nogo`,
})
}

func TestNogoWithCoverageAndCgo(t *testing.T) {
if out, err := bazel_testing.BazelOutput("coverage", "//:foo_cgo_test"); err != nil {
println(string(out))
t.Fatal(err)
}
}

0 comments on commit 6236dd8

Please sign in to comment.