Skip to content

Commit 165de83

Browse files
Update go_library build rule to compile assembly files individually (#330)
Currently, when compiling a Go package with Assembly files, the `go_library` build rule invokes `go tool asm` once on all Assembly files in the package. However, this caused errors that look like the below when multiple files declare symbols with the same name. ``` asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:8: symbol LCDATA1 redeclared asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:242: symbol LCDATA2 redeclared asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:460: symbol LCDATA3 redeclared asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:691: symbol LCDATA4 redeclared asm: pkg/linux_amd64/github.com/apache/arrow-go/v18/internal/utils/min_max_sse4_amd64.s:847: symbol LCDATA5 redeclared asm: assembly failed ``` This is despite the `<>` suffix [1] after the offending symbol name, which should've been enough to make the symbol visible in that file only [2] from the perspective of `go tool asm`. When building this same package without Please (i.e. just with `go build`), we don't run into these errors - it looks like this is because the Go tool invokes `go tool asm` on each Assembly individually [3, 4]. So this PR updates the `go_library` build def's behaviour to more closely match the behaviour in the Go tool. I was able to reproduce these errors by adding the new unit tests in this PR. The errors were fixed by my updates to the `go_library` build def. [1] https://github.com/apache/arrow-go/blob/main/internal/utils/min_max_sse4_amd64.s#L8 [2] https://go.dev/doc/asm#:~:text=Adding%20%3C%3E%20to%20the%20name%2C%20as%20in%20foo%3C%3E(SB)%2C%20makes%20the%20name%20visible%20only%20in%20the%20current%20source%20file%2C%20like%20a%20top%2Dlevel%20static%20declaration%20in%20a%20C%20file [3] https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/work/gccgo.go;l=212 [4] https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/work/gc.go;l=397 Co-authored-by: Chris Novakovic <chris@chrisn.me.uk>
1 parent f6d219e commit 165de83

File tree

7 files changed

+45
-6
lines changed

7 files changed

+45
-6
lines changed

build_defs/go.build_defs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -424,9 +424,22 @@ def go_library(name:str, srcs:list, resources:list=[], asm_srcs:list=None, hdrs:
424424
'hdrs': [hdrs_rule],
425425
'goasm': [lib_rule + '|h'],
426426
},
427-
outs = [name + '.o'],
427+
outs = [name + "#" + (splitext(basename(a)))[0] + ".o" for a in asm_srcs],
428428
building_description = 'Assembling...',
429-
cmd = f'eval `"$TOOL" env` && mkdir include && mv $SRCS_GOASM include/go_asm.h && "$TOOL" tool asm -trimpath "$TMP_DIR" -I "$TMP_DIR"/include -I ${GOROOT}/pkg/include -D GOOS={CONFIG.OS} -D GOARCH_{CONFIG.ARCH} -p {package_path} -o "$OUT" $SRCS_ASM',
429+
cmd = " && ".join([
430+
'eval `"$TOOL" env`',
431+
'mkdir include',
432+
'mv $SRCS_GOASM include/go_asm.h',
433+
'for ASM_FILE in $SRCS_ASM; do "$TOOL" tool asm ' + " ".join([
434+
'-trimpath "$TMP_DIR"',
435+
'-I "$TMP_DIR"/include',
436+
'-I ${GOROOT}/pkg/include',
437+
f'-D GOOS={CONFIG.OS}',
438+
f'-D GOARCH_{CONFIG.ARCH}',
439+
f'-p {package_path}',
440+
f'''-o "{name}#$(basename $ASM_FILE | sed -e 's/\.[sS]$//').o"''',
441+
]) + ' $ASM_FILE; done',
442+
]),
430443
env= {
431444
"GOOS": CONFIG.OS,
432445
"GOARCH": CONFIG.ARCH,

test/asm/lib/BUILD

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@ subinclude("//build_defs:go")
33
go_library(
44
name = "asm",
55
srcs = ["asm.go"],
6-
asm_srcs = ["add.s"],
6+
asm_srcs = [
7+
"add.s",
8+
"subtract.s",
9+
],
710
deps = ["//test/asm/lib/golib"],
811
)
912

test/asm/lib/add.s

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
GLOBL COLLIDINGSYMBOL<>(SB), 8, $32
2+
13
// func add(x, y int64)
24
TEXT ·add(SB),$0-24
35
MOVQ x+0(FP), BX

test/asm/lib/asm.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,11 @@ func add(x, y int64) int64
1010
func Add() int {
1111
return int(add(golib.LHS, golib.RHS))
1212
}
13+
14+
// subtract is the forward declaration of the assembly implementation.
15+
func subtract(x, y int) int64
16+
17+
// Subtract subtracts two numbers using assembly.
18+
func Subtract() int {
19+
return int(subtract(golib.LHS, golib.RHS))
20+
}

test/asm/lib/asm_test.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,9 @@ import (
1111
)
1212

1313
func TestAssemblyAdd(t *testing.T) {
14-
assert.Equal(t, 15, asm.Add())
14+
assert.Equal(t, 14, asm.Add())
15+
}
16+
17+
func TestAssemblySubtract(t *testing.T) {
18+
assert.Equal(t, 6, asm.Subtract())
1519
}

test/asm/lib/golib/go_lib.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
package golib
22

3-
const LHS = 5
4-
const RHS = 10
3+
const LHS = 10
4+
const RHS = 4

test/asm/lib/subtract.s

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
GLOBL COLLIDINGSYMBOL<>(SB), 8, $32
2+
3+
// func subtract(x, y int64)
4+
TEXT ·subtract(SB),$0-24
5+
MOVQ x+0(FP), BX
6+
MOVQ y+8(FP), BP
7+
SUBQ BP, BX
8+
MOVQ BX, ret+16(FP)
9+
RET

0 commit comments

Comments
 (0)