From 496909eb1dac7684268faf8208885c24dfd20b27 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Tue, 15 Aug 2023 15:30:45 -0400 Subject: [PATCH 1/2] 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 --- go/tools/builders/compilepkg.go | 12 ++++--- 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.go | 14 ++++++++ .../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.go | 14 ++++++++ .../asm_dep_conditional_cgo_test.go | 22 +++++++++++++ .../asm_dep_conditional_cgo/asm_dep_nocgo.go | 13 ++++++++ 17 files changed, 247 insertions(+), 7 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.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.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 4c7779c697..c0c203122f 100644 --- a/go/tools/builders/compilepkg.go +++ b/go/tools/builders/compilepkg.go @@ -273,7 +273,9 @@ func compileArchive( hSrcs[i] = src.filename } haveCgo := len(cgoSrcs)+len(cSrcs)+len(cxxSrcs)+len(objcSrcs)+len(objcxxSrcs) > 0 - packageUsesCgo := cgoEnabled && haveCgo + // NOTE: We build cgo packages with cgoEnabled both set to true and false. + // This uses build constraints to exclude files. + compilingWithCgo := cgoEnabled && haveCgo // Instrument source files for coverage. if coverMode != "" { @@ -330,7 +332,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 +357,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 +467,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 { @@ -483,7 +485,7 @@ func compileArchive( } // Compile the .s files if we are not a cgo package; cgo is assembled by cc above - if len(srcs.sSrcs) > 0 && !packageUsesCgo { + 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 fbb0f2ac4d..328caac95b 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 98e618a01e..2e03cd90cc 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 f0a3347493..188dec8238 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 9227ec0ea9..fbc0661b39 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 0000000000..ffd7b185ad --- /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 0000000000..97c5d1a36d --- /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 0000000000..0585bf75db --- /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 0000000000..9bef73d6fb --- /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.go b/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo.go new file mode 100644 index 0000000000..c4b2cca963 --- /dev/null +++ b/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo.go @@ -0,0 +1,14 @@ +package main + +import ( + "fmt" +) + +func main() { + result, err := callASM() + if err != nil { + fmt.Printf("callASM() returned err=%s\n", err.Error()) + } else { + fmt.Printf("callASM() == %d\n", result) + } +} 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 0000000000..b76dac2b77 --- /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 0000000000..ef4d0c0bfd --- /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 0000000000..6467ad3e73 --- /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 0000000000..531cc7dcb3 --- /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.go b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo.go new file mode 100644 index 0000000000..8a6894e28b --- /dev/null +++ b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo.go @@ -0,0 +1,14 @@ +package main + +import ( + "fmt" +) + +func main() { + result, err := callASMPackage() + if err != nil { + fmt.Printf("callASMPackage() returned err=%s\n", err.Error()) + } else { + fmt.Printf("callASMPackage() == %d\n", result) + } +} 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 0000000000..0f1f1521cb --- /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 0000000000..e8968dac18 --- /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 From 7829754de51b54eaec14346ccc87037ffb50be48 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Sat, 19 Aug 2023 13:01:29 -0400 Subject: [PATCH 2/2] code review improvements: remove unused main; clarify comment --- go/tools/builders/compilepkg.go | 12 ++++++++---- .../cgo/asm_conditional_cgo/asm_conditional_cgo.go | 14 -------------- .../asm_dep_conditional_cgo.go | 14 -------------- 3 files changed, 8 insertions(+), 32 deletions(-) delete mode 100644 tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo.go delete mode 100644 tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo.go diff --git a/go/tools/builders/compilepkg.go b/go/tools/builders/compilepkg.go index c0c203122f..b3a6d283a9 100644 --- a/go/tools/builders/compilepkg.go +++ b/go/tools/builders/compilepkg.go @@ -272,10 +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 - // NOTE: We build cgo packages with cgoEnabled both set to true and false. - // This uses build constraints to exclude files. - compilingWithCgo := 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 != "" { @@ -484,7 +487,8 @@ func compileArchive( return err } - // Compile the .s files if we are not a cgo package; cgo is assembled by cc above + // 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"): {}, diff --git a/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo.go b/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo.go deleted file mode 100644 index c4b2cca963..0000000000 --- a/tests/core/cgo/asm_conditional_cgo/asm_conditional_cgo.go +++ /dev/null @@ -1,14 +0,0 @@ -package main - -import ( - "fmt" -) - -func main() { - result, err := callASM() - if err != nil { - fmt.Printf("callASM() returned err=%s\n", err.Error()) - } else { - fmt.Printf("callASM() == %d\n", result) - } -} diff --git a/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo.go b/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo.go deleted file mode 100644 index 8a6894e28b..0000000000 --- a/tests/core/cgo/asm_dep_conditional_cgo/asm_dep_conditional_cgo.go +++ /dev/null @@ -1,14 +0,0 @@ -package main - -import ( - "fmt" -) - -func main() { - result, err := callASMPackage() - if err != nil { - fmt.Printf("callASMPackage() returned err=%s\n", err.Error()) - } else { - fmt.Printf("callASMPackage() == %d\n", result) - } -}