From 2e821f66bb9fe1e16ea42743d30936248c1fa11a Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Sun, 20 Aug 2023 07:06:59 -0400 Subject: [PATCH] cgo packages with assembly: Support CGO_ENABLED=0 (#3661) * cgo packages with assembly: Support CGO_ENABLED=0 Go supports building cgo packages both with and without Cgo. It uses build constraints to exclude the appropriate files. When building a Cgo package with assembly files, we must exclude them. Previous to this change, rules_go would run the Go assembler on them. This meant that packages which had "conditional" imports of cgo libraries with assembly would fail when compiled with cgo disabled. Add tests for these two cases: * asm_conditional_cgo: A Go package that compiles both with and without Cgo. * asm_dep_conditional_cgo: A Go package that conditionally imports a package with Cgo. Fixes: https://github.com/bazelbuild/rules_go/issues/3411 * code review improvements: remove unused main; clarify comment --- go/tools/builders/compilepkg.go | 18 ++++++---- go/tools/builders/filter.go | 10 ++++++ tests/core/cgo/asm/BUILD.bazel | 1 + tests/core/cgo/asm/cgoasm.go | 2 +- tests/core/cgo/asm/cgoasm_test.go | 2 +- .../core/cgo/asm_conditional_cgo/BUILD.bazel | 33 +++++++++++++++++++ .../core/cgo/asm_conditional_cgo/asm_amd64.S | 22 +++++++++++++ .../core/cgo/asm_conditional_cgo/asm_arm64.S | 15 +++++++++ tests/core/cgo/asm_conditional_cgo/asm_cgo.go | 16 +++++++++ .../asm_conditional_cgo_test.go | 22 +++++++++++++ .../core/cgo/asm_conditional_cgo/asm_nocgo.go | 11 +++++++ .../cgo/asm_dep_conditional_cgo/BUILD.bazel | 30 +++++++++++++++++ .../asm_dep_conditional_cgo/asm_dep_cgo.go | 15 +++++++++ .../asm_dep_conditional_cgo_test.go | 22 +++++++++++++ .../asm_dep_conditional_cgo/asm_dep_nocgo.go | 13 ++++++++ 15 files changed, 224 insertions(+), 8 deletions(-) create mode 100644 tests/core/cgo/asm_conditional_cgo/BUILD.bazel create mode 100644 tests/core/cgo/asm_conditional_cgo/asm_amd64.S create mode 100644 tests/core/cgo/asm_conditional_cgo/asm_arm64.S create mode 100644 tests/core/cgo/asm_conditional_cgo/asm_cgo.go create mode 100644 tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo_test.go create mode 100644 tests/core/cgo/asm_conditional_cgo/asm_nocgo.go create mode 100644 tests/core/cgo/asm_dep_conditional_cgo/BUILD.bazel create mode 100644 tests/core/cgo/asm_dep_conditional_cgo/asm_dep_cgo.go create mode 100644 tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo_test.go create mode 100644 tests/core/cgo/asm_dep_conditional_cgo/asm_dep_nocgo.go diff --git a/go/tools/builders/compilepkg.go b/go/tools/builders/compilepkg.go index 4c7779c69..b3a6d283a 100644 --- a/go/tools/builders/compilepkg.go +++ b/go/tools/builders/compilepkg.go @@ -272,8 +272,13 @@ func compileArchive( for i, src := range srcs.hSrcs { hSrcs[i] = src.filename } + + // haveCgo is true if the package contains Cgo files. haveCgo := len(cgoSrcs)+len(cSrcs)+len(cxxSrcs)+len(objcSrcs)+len(objcxxSrcs) > 0 - packageUsesCgo := cgoEnabled && haveCgo + // compilingWithCgo is true if the package contains Cgo files AND Cgo is enabled. A package + // containing Cgo files can also be built with Cgo disabled, and will work if there are build + // constraints. + compilingWithCgo := haveCgo && cgoEnabled // Instrument source files for coverage. if coverMode != "" { @@ -330,7 +335,7 @@ func compileArchive( // If we have cgo, generate separate C and go files, and compile the // C files. var objFiles []string - if packageUsesCgo { + if compilingWithCgo { // If the package uses Cgo, compile .s and .S files with cgo2, not the Go assembler. // Otherwise: the .s/.S files will be compiled with the Go assembler later var srcDir string @@ -355,7 +360,7 @@ func compileArchive( if err != nil { return err } - if packageUsesCgo { + if compilingWithCgo { // cgo generated code imports some extra packages. imports["runtime/cgo"] = nil imports["syscall"] = nil @@ -465,7 +470,7 @@ func compileArchive( asmHdrPath = filepath.Join(workDir, "go_asm.h") } var symabisPath string - if !packageUsesCgo { + if !haveCgo { symabisPath, err = buildSymabisFile(goenv, srcs.sSrcs, srcs.hSrcs, asmHdrPath) if symabisPath != "" { if !goenv.shouldPreserveWorkDir { @@ -482,8 +487,9 @@ func compileArchive( return err } - // Compile the .s files if we are not a cgo package; cgo is assembled by cc above - if len(srcs.sSrcs) > 0 && !packageUsesCgo { + // Compile the .s files with Go's assembler, if this is not a cgo package. + // Cgo is assembled by cc above. + if len(srcs.sSrcs) > 0 && !haveCgo { includeSet := map[string]struct{}{ filepath.Join(os.Getenv("GOROOT"), "pkg", "include"): {}, workDir: {}, diff --git a/go/tools/builders/filter.go b/go/tools/builders/filter.go index fbb0f2ac4..328caac95 100644 --- a/go/tools/builders/filter.go +++ b/go/tools/builders/filter.go @@ -69,11 +69,15 @@ type archiveSrcs struct { // them by extension. func filterAndSplitFiles(fileNames []string) (archiveSrcs, error) { var res archiveSrcs + packageContainsCgo := false for _, s := range fileNames { src, err := readFileInfo(build.Default, s) if err != nil { return archiveSrcs{}, err } + if src.isCgo { + packageContainsCgo = true + } if !src.matched { continue } @@ -96,6 +100,12 @@ func filterAndSplitFiles(fileNames []string) (archiveSrcs, error) { } *srcs = append(*srcs, src) } + if packageContainsCgo && !build.Default.CgoEnabled { + // Cgo packages use the C compiler for asm files, rather than Go's assembler. + // This is a package with cgo files, but we are compiling with Cgo disabled: + // Remove the assembly files. + res.sSrcs = nil + } return res, nil } diff --git a/tests/core/cgo/asm/BUILD.bazel b/tests/core/cgo/asm/BUILD.bazel index 98e618a01..2e03cd90c 100644 --- a/tests/core/cgo/asm/BUILD.bazel +++ b/tests/core/cgo/asm/BUILD.bazel @@ -9,6 +9,7 @@ go_library( ], cgo = True, importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/asm", + visibility = ["//tests/core/cgo:__subpackages__"], ) go_test( diff --git a/tests/core/cgo/asm/cgoasm.go b/tests/core/cgo/asm/cgoasm.go index f0a334749..188dec823 100644 --- a/tests/core/cgo/asm/cgoasm.go +++ b/tests/core/cgo/asm/cgoasm.go @@ -7,6 +7,6 @@ extern int example_asm_func(); */ import "C" -func callAssembly() int { +func CallAssembly() int { return int(C.example_asm_func()) } diff --git a/tests/core/cgo/asm/cgoasm_test.go b/tests/core/cgo/asm/cgoasm_test.go index 9227ec0ea..fbc0661b3 100644 --- a/tests/core/cgo/asm/cgoasm_test.go +++ b/tests/core/cgo/asm/cgoasm_test.go @@ -16,7 +16,7 @@ func TestNativeAssembly(t *testing.T) { if expected == 0 { t.Fatalf("expected=0 for GOARCH=%s; missing value?", runtime.GOARCH) } - actual := callAssembly() + actual := CallAssembly() if actual != expected { t.Errorf("callAssembly()=%d; expected=%d", actual, expected) } diff --git a/tests/core/cgo/asm_conditional_cgo/BUILD.bazel b/tests/core/cgo/asm_conditional_cgo/BUILD.bazel new file mode 100644 index 000000000..ffd7b185a --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/BUILD.bazel @@ -0,0 +1,33 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "asm_conditional_cgo", + srcs = [ + "asm_amd64.S", + "asm_arm64.S", + "asm_cgo.go", + "asm_nocgo.go", + ], + cgo = True, + importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/asm_conditional_cgo", + deps = ["//tests/core/cgo/asm"], +) + +# this is a "native" target: it uses cgo and calls the assembly function as expected +go_test( + name = "asm_conditional_cgo_test", + srcs = [ + "asm_conditional_cgo_test.go", + ], + embed = [":asm_conditional_cgo"], +) + +# this is a CGO_ENABLED=0 target: it does not import the cgo target +go_test( + name = "asm_conditional_nocgo_test", + srcs = [ + "asm_conditional_cgo_test.go", + ], + embed = [":asm_conditional_cgo"], + pure = "on", +) diff --git a/tests/core/cgo/asm_conditional_cgo/asm_amd64.S b/tests/core/cgo/asm_conditional_cgo/asm_amd64.S new file mode 100644 index 000000000..97c5d1a36 --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/asm_amd64.S @@ -0,0 +1,22 @@ +/* +https://stackoverflow.com/questions/73435637/how-can-i-fix-usr-bin-ld-warning-trap-o-missing-note-gnu-stack-section-imp +*/ +#if defined(__ELF__) && defined(__GNUC__) +.section .note.GNU-stack,"",@progbits +#endif + +/* +define this symbol both with and without underscore. +It seems like Mac OS X wants the underscore, but Linux does not. +*/ +.global example_asm_func +.global _example_asm_func +.text +example_asm_func: +_example_asm_func: + mov $42, %rax + ret + +#if NOT_DEFINED +#error "should not fail" +#endif diff --git a/tests/core/cgo/asm_conditional_cgo/asm_arm64.S b/tests/core/cgo/asm_conditional_cgo/asm_arm64.S new file mode 100644 index 000000000..0585bf75d --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/asm_arm64.S @@ -0,0 +1,15 @@ +/* +define this symbol both with and without underscore. +It seems like Mac OS X wants the underscore, but Linux does not. +*/ +.global example_asm_func +.global _example_asm_func +.p2align 2 /* ld: warning: arm64 function not 4-byte aligned */ +example_asm_func: +_example_asm_func: + mov x0, #44 + ret + +#if NOT_DEFINED +#error "should not fail" +#endif diff --git a/tests/core/cgo/asm_conditional_cgo/asm_cgo.go b/tests/core/cgo/asm_conditional_cgo/asm_cgo.go new file mode 100644 index 000000000..9bef73d6f --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/asm_cgo.go @@ -0,0 +1,16 @@ +//go:build amd64 || arm64 + +// build constraints must match the constraints in the tests/core/cgo/asm package + +package main + +/* +extern int example_asm_func(); +*/ +import "C" + +func callASM() (int, error) { + return int(C.example_asm_func()), nil +} + +const builtWithCgo = true diff --git a/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo_test.go b/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo_test.go new file mode 100644 index 000000000..b76dac2b7 --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo_test.go @@ -0,0 +1,22 @@ +package main + +import "testing" + +func TestConditionalCgo(t *testing.T) { + // Uses build constraints to depend on assembly code when cgo is available, or a + // pure go version if it is not. This can be run both with and without cgo. E.g.: + // CGO_ENABLED=0 go test . && CGO_ENABLED=1 go test . + result, err := callASM() + if builtWithCgo { + if err != nil { + t.Errorf("builtWithCgo=%t; err must be nil, was %s", builtWithCgo, err.Error()) + } else if result <= 0 { + t.Errorf("builtWithCgo=%t; result must be > 0 was %d", builtWithCgo, result) + } + } else { + // does not support calling the cgo library + if !(result == 0 && err != nil) { + t.Errorf("builtWithCgo=%t; expected result=0, err != nil; result=%d, err=%#v", builtWithCgo, result, err) + } + } +} diff --git a/tests/core/cgo/asm_conditional_cgo/asm_nocgo.go b/tests/core/cgo/asm_conditional_cgo/asm_nocgo.go new file mode 100644 index 000000000..ef4d0c0bf --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/asm_nocgo.go @@ -0,0 +1,11 @@ +//go:build !(cgo && (amd64 || arm64)) + +package main + +import "errors" + +func callASM() (int, error) { + return 0, errors.New("cgo disabled and/or unsupported platform") +} + +const builtWithCgo = false diff --git a/tests/core/cgo/asm_dep_conditional_cgo/BUILD.bazel b/tests/core/cgo/asm_dep_conditional_cgo/BUILD.bazel new file mode 100644 index 000000000..6467ad3e7 --- /dev/null +++ b/tests/core/cgo/asm_dep_conditional_cgo/BUILD.bazel @@ -0,0 +1,30 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "asm_dep_conditional_cgo", + srcs = [ + "asm_dep_cgo.go", + "asm_dep_nocgo.go", + ], + importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/asm_dep_conditional_cgo", + deps = ["//tests/core/cgo/asm"], +) + +# this is a "native" target: it uses cgo and calls the assembly function as expected +go_test( + name = "asm_dep_conditional_cgo_test", + srcs = [ + "asm_dep_conditional_cgo_test.go", + ], + embed = [":asm_dep_conditional_cgo"], +) + +# this is a CGO_ENABLED=0 target: it does not import the cgo target +go_test( + name = "asm_dep_conditional_nocgo_test", + srcs = [ + "asm_dep_conditional_cgo_test.go", + ], + embed = [":asm_dep_conditional_cgo"], + pure = "on", +) diff --git a/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_cgo.go b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_cgo.go new file mode 100644 index 000000000..531cc7dcb --- /dev/null +++ b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_cgo.go @@ -0,0 +1,15 @@ +//go:build cgo && (amd64 || arm64) + +// build constraints must match the constraints in the tests/core/cgo/asm package + +package main + +import ( + "github.com/bazelbuild/rules_go/tests/core/cgo/asm" +) + +func callASMPackage() (int, error) { + return asm.CallAssembly(), nil +} + +const builtWithCgo = true diff --git a/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo_test.go b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo_test.go new file mode 100644 index 000000000..0f1f1521c --- /dev/null +++ b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo_test.go @@ -0,0 +1,22 @@ +package main + +import "testing" + +func TestConditionalCgo(t *testing.T) { + // Uses build constraints to depend on a native Cgo package when cgo is available, or a + // pure go version if it is not. This can be run both with and without cgo. E.g.: + // CGO_ENABLED=0 go test . && CGO_ENABLED=1 go test . + result, err := callASMPackage() + if builtWithCgo { + if err != nil { + t.Errorf("builtWithCgo=%t; err must be nil, was %s", builtWithCgo, err.Error()) + } else if result <= 0 { + t.Errorf("builtWithCgo=%t; result must be > 0 was %d", builtWithCgo, result) + } + } else { + // does not support calling the cgo library + if !(result == 0 && err != nil) { + t.Errorf("builtWithCgo=%t; expected result=0, err != nil; result=%d, err=%#v", builtWithCgo, result, err) + } + } +} diff --git a/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_nocgo.go b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_nocgo.go new file mode 100644 index 000000000..e8968dac1 --- /dev/null +++ b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_nocgo.go @@ -0,0 +1,13 @@ +//go:build !(cgo && (amd64 || arm64)) + +// build constraints must match the constraints in the tests/core/cgo/asm package + +package main + +import "errors" + +func callASMPackage() (int, error) { + return 0, errors.New("cgo disabled and/or unsupported platform") +} + +const builtWithCgo = false