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:
#3411

* code review improvements: remove unused main; clarify comment

Author: evanj

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: {},

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
 }
 

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(

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())
 }

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)
 	}

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",
+)

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

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

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

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)
+		}
+	}
+}