compilepkg: cgo assembly uses the C compiler (#3648)

* compilepkg: cgo assembly uses the C compiler

This changes compilepkg to use the C compiler for .S and .s files to
use the C compiler, like go build does. Previously it would use the
Go assembler, which is used for pure Go packages. This should help
fix issue: #3411

I have added a cgo test for this issue that is intended to work with
both arm64 and amd64 machines. I have only tested it on Linux amd64
and Darwin arm64. Without this change it fails to build with the
following output. This fails to parse the assembly file because it
uses the Go assembler.

ERROR: rules_go/tests/core/cgo/asm/BUILD.bazel:3:11: GoCompilePkg
tests/core/cgo/asm/asm.a failed: (Exit 1): builder failed: error
executing command (from target //tests/core/cgo/asm:asm)
bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder
compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src
tests/core/cgo/asm/cgoasm.go -src tests/core/cgo/asm/asm_amd64.S ...

tests/core/cgo/asm/asm_amd64.S:4: expected identifier, found "."
asm: assembly of tests/core/cgo/asm/asm_amd64.S failed
compilepkg: error running subcommand external/go_sdk/pkg/tool/linux_amd64/asm: exit status 1

* add build constraint: unix && (amd64 || arm64)

* run buildifier; make asm conditional

* assembly fixes for Mac OS X and Linux

* Change build constraint to (amd64 || arm64): Should work on Windows

The tests were failing on Windows because rules_go incorrectly
believed this was a "native" Go package. I think this should work on
Windows.

---------

Co-authored-by: Fabian Meumertzheim <[email protected]>

Author: evanj

go/tools/builders/compilepkg.go

@@ -273,6 +273,7 @@ func compileArchive(
 		hSrcs[i] = src.filename
 	}
 	haveCgo := len(cgoSrcs)+len(cSrcs)+len(cxxSrcs)+len(objcSrcs)+len(objcxxSrcs) > 0
+	packageUsesCgo := cgoEnabled && haveCgo
 
 	// Instrument source files for coverage.
 	if coverMode != "" {
@@ -329,12 +330,11 @@ func compileArchive(
 	// If we have cgo, generate separate C and go files, and compile the
 	// C files.
 	var objFiles []string
-	if cgoEnabled && haveCgo {
-		// TODO(#2006): Compile .s and .S files with cgo2, not the Go assembler.
-		// If cgo is not enabled or we don't have other cgo sources, don't
-		// compile .S files.
+	if packageUsesCgo {
+		// 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
-		srcDir, goSrcs, objFiles, err = cgo2(goenv, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, nil, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath)
+		srcDir, goSrcs, objFiles, err = cgo2(goenv, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath)
 		if err != nil {
 			return err
 		}
@@ -355,7 +355,7 @@ func compileArchive(
 	if err != nil {
 		return err
 	}
-	if cgoEnabled && len(cgoSrcs) != 0 {
+	if packageUsesCgo {
 		// cgo generated code imports some extra packages.
 		imports["runtime/cgo"] = nil
 		imports["syscall"] = nil
@@ -458,28 +458,32 @@ func compileArchive(
 		}()
 	}
 
-	// If there are assembly files, and this is go1.12+, generate symbol ABIs.
+	// If there are Go assembly files and this is go1.12+: generate symbol ABIs.
+	// This excludes Cgo packages: they use the C compiler for assembly.
 	asmHdrPath := ""
 	if len(srcs.sSrcs) > 0 {
 		asmHdrPath = filepath.Join(workDir, "go_asm.h")
 	}
-	symabisPath, err := buildSymabisFile(goenv, srcs.sSrcs, srcs.hSrcs, asmHdrPath)
-	if symabisPath != "" {
-		if !goenv.shouldPreserveWorkDir {
-			defer os.Remove(symabisPath)
+	var symabisPath string
+	if !packageUsesCgo {
+		symabisPath, err = buildSymabisFile(goenv, srcs.sSrcs, srcs.hSrcs, asmHdrPath)
+		if symabisPath != "" {
+			if !goenv.shouldPreserveWorkDir {
+				defer os.Remove(symabisPath)
+			}
+		}
+		if err != nil {
+			return err
 		}
-	}
-	if err != nil {
-		return err
 	}
 
 	// Compile the filtered .go files.
 	if err := compileGo(goenv, goSrcs, packagePath, importcfgPath, embedcfgPath, asmHdrPath, symabisPath, gcFlags, pgoprofile, outPath); err != nil {
 		return err
 	}
 
-	// Compile the .s files.
-	if len(srcs.sSrcs) > 0 {
+	// Compile the .s files if we are not a cgo package; cgo is assembled by cc above
+	if len(srcs.sSrcs) > 0 && !packageUsesCgo {
 		includeSet := map[string]struct{}{
 			filepath.Join(os.Getenv("GOROOT"), "pkg", "include"): {},
 			workDir: {},

tests/core/cgo/asm/BUILD.bazel

@@ -0,0 +1,20 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "asm",
+    srcs = [
+        "asm_amd64.S",
+        "asm_arm64.S",
+        "cgoasm.go",
+    ],
+    cgo = True,
+    importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/asm",
+)
+
+go_test(
+    name = "asm_test",
+    srcs = [
+        "cgoasm_test.go",
+    ],
+    embed = [":asm"],
+)

tests/core/cgo/asm/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/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/cgoasm.go

@@ -0,0 +1,12 @@
+//go:build amd64 || arm64
+
+package asm
+
+/*
+extern int example_asm_func();
+*/
+import "C"
+
+func callAssembly() int {
+	return int(C.example_asm_func())
+}

tests/core/cgo/asm/cgoasm_test.go

@@ -0,0 +1,23 @@
+//go:build amd64 || arm64
+
+package asm
+
+import (
+	"runtime"
+	"testing"
+)
+
+func TestNativeAssembly(t *testing.T) {
+	expectedGOARCH := map[string]int{
+		"amd64": 42,
+		"arm64": 44,
+	}
+	expected := expectedGOARCH[runtime.GOARCH]
+	if expected == 0 {
+		t.Fatalf("expected=0 for GOARCH=%s; missing value?", runtime.GOARCH)
+	}
+	actual := callAssembly()
+	if actual != expected {
+		t.Errorf("callAssembly()=%d; expected=%d", actual, expected)
+	}
+}