From 161418a42dfedb47096d03544b48be0ca87e1469 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Wed, 9 Aug 2023 22:08:29 -0400 Subject: [PATCH 1/5] 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: https://github.com/bazelbuild/rules_go/issues/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 --- go/tools/builders/compilepkg.go | 36 +++++++++++++++++-------------- tests/core/cgo/asm/BUILD.bazel | 20 +++++++++++++++++ tests/core/cgo/asm/asm_amd64.S | 14 ++++++++++++ tests/core/cgo/asm/asm_arm64.S | 9 ++++++++ tests/core/cgo/asm/cgoasm.go | 10 +++++++++ tests/core/cgo/asm/cgoasm_test.go | 21 ++++++++++++++++++ 6 files changed, 94 insertions(+), 16 deletions(-) create mode 100644 tests/core/cgo/asm/BUILD.bazel create mode 100644 tests/core/cgo/asm/asm_amd64.S create mode 100644 tests/core/cgo/asm/asm_arm64.S create mode 100644 tests/core/cgo/asm/cgoasm.go create mode 100644 tests/core/cgo/asm/cgoasm_test.go diff --git a/go/tools/builders/compilepkg.go b/go/tools/builders/compilepkg.go index 9c6202bb5c..4c7779c697 100644 --- a/go/tools/builders/compilepkg.go +++ b/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,19 +458,23 @@ 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. @@ -478,8 +482,8 @@ func compileArchive( 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: {}, diff --git a/tests/core/cgo/asm/BUILD.bazel b/tests/core/cgo/asm/BUILD.bazel new file mode 100644 index 0000000000..1a93098cf3 --- /dev/null +++ b/tests/core/cgo/asm/BUILD.bazel @@ -0,0 +1,20 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test", "go_library") + +go_library( + name = "asm", + srcs = [ + "asm_amd64.S", + "asm_arm64.S", + "cgoasm.go", + ], + cgo = True, + importpath = "github.com/bazelbuild/rules_go/tests/cgo/asm" +) + +go_test( + name = "asm_test", + srcs = [ + "cgoasm_test.go", + ], + embed = [":asm"], +) diff --git a/tests/core/cgo/asm/asm_amd64.S b/tests/core/cgo/asm/asm_amd64.S new file mode 100644 index 0000000000..181139e178 --- /dev/null +++ b/tests/core/cgo/asm/asm_amd64.S @@ -0,0 +1,14 @@ +/* +https://stackoverflow.com/questions/73435637/how-can-i-fix-usr-bin-ld-warning-trap-o-missing-note-gnu-stack-section-imp +*/ +.section .note.GNU-stack,"",@progbits + +.global example_asm_func +.text +example_asm_func: + mov $42, %rax + ret + +#if NOT_DEFINED +#error "should not fail +#endif diff --git a/tests/core/cgo/asm/asm_arm64.S b/tests/core/cgo/asm/asm_arm64.S new file mode 100644 index 0000000000..d7725e35d7 --- /dev/null +++ b/tests/core/cgo/asm/asm_arm64.S @@ -0,0 +1,9 @@ +.global _example_asm_func +.p2align 2 /* ld: warning: arm64 function not 4-byte aligned */ +_example_asm_func: + mov x0, #44 + ret + +#if NOT_DEFINED +#error "should not fail +#endif diff --git a/tests/core/cgo/asm/cgoasm.go b/tests/core/cgo/asm/cgoasm.go new file mode 100644 index 0000000000..b5c1f84413 --- /dev/null +++ b/tests/core/cgo/asm/cgoasm.go @@ -0,0 +1,10 @@ +package asm + +/* +extern int example_asm_func(); +*/ +import "C" + +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 new file mode 100644 index 0000000000..e2c0d134c0 --- /dev/null +++ b/tests/core/cgo/asm/cgoasm_test.go @@ -0,0 +1,21 @@ +package asm + +import ( + "testing" + "runtime" +) + +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) + } +} From fedc4320b9f5a9f851d1bbd9fa837497f37425c9 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Sat, 12 Aug 2023 15:17:11 -0400 Subject: [PATCH 2/5] add build constraint: unix && (amd64 || arm64) --- tests/core/cgo/asm/cgoasm.go | 2 ++ tests/core/cgo/asm/cgoasm_test.go | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/core/cgo/asm/cgoasm.go b/tests/core/cgo/asm/cgoasm.go index b5c1f84413..227bcab375 100644 --- a/tests/core/cgo/asm/cgoasm.go +++ b/tests/core/cgo/asm/cgoasm.go @@ -1,3 +1,5 @@ +//go:build unix && (amd64 || arm64) + package asm /* diff --git a/tests/core/cgo/asm/cgoasm_test.go b/tests/core/cgo/asm/cgoasm_test.go index e2c0d134c0..d0d76c9dc9 100644 --- a/tests/core/cgo/asm/cgoasm_test.go +++ b/tests/core/cgo/asm/cgoasm_test.go @@ -1,8 +1,10 @@ +//go:build unix && (amd64 || arm64) + package asm import ( - "testing" "runtime" + "testing" ) func TestNativeAssembly(t *testing.T) { From c86b8b948d43f4c4437c8c1805247be25c5199c2 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Sat, 12 Aug 2023 16:24:19 -0400 Subject: [PATCH 3/5] run buildifier; make asm conditional --- tests/core/cgo/asm/BUILD.bazel | 4 ++-- tests/core/cgo/asm/asm_amd64.S | 4 +++- tests/core/cgo/asm/asm_arm64.S | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/core/cgo/asm/BUILD.bazel b/tests/core/cgo/asm/BUILD.bazel index 1a93098cf3..98e618a01e 100644 --- a/tests/core/cgo/asm/BUILD.bazel +++ b/tests/core/cgo/asm/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_test", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "asm", @@ -8,7 +8,7 @@ go_library( "cgoasm.go", ], cgo = True, - importpath = "github.com/bazelbuild/rules_go/tests/cgo/asm" + importpath = "github.com/bazelbuild/rules_go/tests/core/cgo/asm", ) go_test( diff --git a/tests/core/cgo/asm/asm_amd64.S b/tests/core/cgo/asm/asm_amd64.S index 181139e178..8b6b2503be 100644 --- a/tests/core/cgo/asm/asm_amd64.S +++ b/tests/core/cgo/asm/asm_amd64.S @@ -1,7 +1,9 @@ /* 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 .global example_asm_func .text @@ -10,5 +12,5 @@ example_asm_func: ret #if NOT_DEFINED -#error "should not fail +#error "should not fail" #endif diff --git a/tests/core/cgo/asm/asm_arm64.S b/tests/core/cgo/asm/asm_arm64.S index d7725e35d7..e7546f4c47 100644 --- a/tests/core/cgo/asm/asm_arm64.S +++ b/tests/core/cgo/asm/asm_arm64.S @@ -5,5 +5,5 @@ _example_asm_func: ret #if NOT_DEFINED -#error "should not fail +#error "should not fail" #endif From 4a8bd2a6c5a5712a8b6bf821ce8e5ac7fe1e7a04 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Sat, 12 Aug 2023 16:29:01 -0400 Subject: [PATCH 4/5] assembly fixes for Mac OS X and Linux --- tests/core/cgo/asm/asm_amd64.S | 6 ++++++ tests/core/cgo/asm/asm_arm64.S | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/tests/core/cgo/asm/asm_amd64.S b/tests/core/cgo/asm/asm_amd64.S index 8b6b2503be..97c5d1a36d 100644 --- a/tests/core/cgo/asm/asm_amd64.S +++ b/tests/core/cgo/asm/asm_amd64.S @@ -5,9 +5,15 @@ https://stackoverflow.com/questions/73435637/how-can-i-fix-usr-bin-ld-warning-tr .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 diff --git a/tests/core/cgo/asm/asm_arm64.S b/tests/core/cgo/asm/asm_arm64.S index e7546f4c47..0585bf75db 100644 --- a/tests/core/cgo/asm/asm_arm64.S +++ b/tests/core/cgo/asm/asm_arm64.S @@ -1,5 +1,11 @@ +/* +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 From 14d5fdc36b52ef8aa15fb5840d553d12b64f5913 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Mon, 14 Aug 2023 09:08:30 -0400 Subject: [PATCH 5/5] 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. --- tests/core/cgo/asm/cgoasm.go | 2 +- tests/core/cgo/asm/cgoasm_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/core/cgo/asm/cgoasm.go b/tests/core/cgo/asm/cgoasm.go index 227bcab375..f0a3347493 100644 --- a/tests/core/cgo/asm/cgoasm.go +++ b/tests/core/cgo/asm/cgoasm.go @@ -1,4 +1,4 @@ -//go:build unix && (amd64 || arm64) +//go:build amd64 || arm64 package asm diff --git a/tests/core/cgo/asm/cgoasm_test.go b/tests/core/cgo/asm/cgoasm_test.go index d0d76c9dc9..9227ec0ea9 100644 --- a/tests/core/cgo/asm/cgoasm_test.go +++ b/tests/core/cgo/asm/cgoasm_test.go @@ -1,4 +1,4 @@ -//go:build unix && (amd64 || arm64) +//go:build amd64 || arm64 package asm