Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/link: multiple function bodies accepted, one in a .go, one in a .s #15297

Closed
nigeltao opened this issue Apr 14, 2016 · 5 comments

Comments

Projects
None yet
5 participants
@nigeltao
Copy link
Contributor

commented Apr 14, 2016

$ pwd
/tmp/scratch
$ ls
main.go  x_amd64.s  x_other.go
$ cat main.go 
package main

func main() {
    println(foo())
}
$ cat x_amd64.s 
TEXT ·foo(SB), 7, $0
    MOVQ $43, ret+0(FP)
    RET

$ cat x_other.go 
package main

func foo() int { return 42 }

Note that foo has two bodies defined: one returns 42, the other returns 43. Surely this should be a compile or asm or link error, yet "go build" seems happy:

$ go build -x && ./scratch && rm ./scratch 
WORK=/tmp/go-build115513716
mkdir -p $WORK/_/tmp/scratch/_obj/
mkdir -p $WORK/_/tmp/scratch/_obj/exe/
cd /tmp/scratch
/home/nt/go/pkg/tool/linux_amd64/compile -o $WORK/_/tmp/scratch.a -trimpath $WORK -p main -buildid acfccd74ae478f8fecc64195dd5e90e7757cec11 -D _/tmp/scratch -I $WORK -pack -asmhdr $WORK/_/tmp/scratch/_obj/go_asm.h ./main.go ./x_other.go
/home/nt/go/pkg/tool/linux_amd64/asm -o $WORK/_/tmp/scratch/_obj/x_amd64.o -trimpath $WORK -I $WORK/_/tmp/scratch/_obj/ -I /home/nt/go/pkg/include -D GOOS_linux -D GOARCH_amd64 ./x_amd64.s
pack r $WORK/_/tmp/scratch.a $WORK/_/tmp/scratch/_obj/x_amd64.o # internal
cd .
/home/nt/go/pkg/tool/linux_amd64/link -o $WORK/_/tmp/scratch/_obj/exe/a.out -L $WORK -extld=gcc -buildmode=exe -buildid=acfccd74ae478f8fecc64195dd5e90e7757cec11 $WORK/_/tmp/scratch.a
mv $WORK/_/tmp/scratch/_obj/exe/a.out scratch
42

Changing the

func foo() int { return 42 }

to be

func foo() int

in x_other.go now has "go build" picking up the other definition:

$ vim x_other.go 
$ cat x_other.go 
package main

func foo() int
$ go build -x && ./scratch && rm ./scratch 
WORK=/tmp/go-build732255335
mkdir -p $WORK/_/tmp/scratch/_obj/
mkdir -p $WORK/_/tmp/scratch/_obj/exe/
cd /tmp/scratch
/home/nt/go/pkg/tool/linux_amd64/compile -o $WORK/_/tmp/scratch.a -trimpath $WORK -p main -buildid acfccd74ae478f8fecc64195dd5e90e7757cec11 -D _/tmp/scratch -I $WORK -pack -asmhdr $WORK/_/tmp/scratch/_obj/go_asm.h ./main.go ./x_other.go
/home/nt/go/pkg/tool/linux_amd64/asm -o $WORK/_/tmp/scratch/_obj/x_amd64.o -trimpath $WORK -I $WORK/_/tmp/scratch/_obj/ -I /home/nt/go/pkg/include -D GOOS_linux -D GOARCH_amd64 ./x_amd64.s
pack r $WORK/_/tmp/scratch.a $WORK/_/tmp/scratch/_obj/x_amd64.o # internal
cd .
/home/nt/go/pkg/tool/linux_amd64/link -o $WORK/_/tmp/scratch/_obj/exe/a.out -L $WORK -extld=gcc -buildmode=exe -buildid=acfccd74ae478f8fecc64195dd5e90e7757cec11 $WORK/_/tmp/scratch.a
mv $WORK/_/tmp/scratch/_obj/exe/a.out scratch
43

Still, something ain't right. Re-inserting that

{ return 42 }

into the x_other.go file gives:

$ vim x_other.go 
$ cat x_other.go 
package main

func foo() int { return 42 }
$ go vet
: x_amd64.s:1: [amd64] foo: function foo missing Go declaration
exit status 1
@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

@bradfitz bradfitz added this to the Go1.7 milestone Apr 14, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2016

It looks like the linker's check for duplicate symbols is being incorrected suppressed for function definitions. Still, nice that vet catches it.

Would be nice but not critical for Go 1.7.

@mwhudson

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2016

$ cat x_amd64.s
TEXT ·foo(SB), 7, $0

Why 7 here? That's NOSPLIT + DUPOK + NOPROF. It's probably still a bug that this fails, but the presence of DUPOK here makes it a bit less surprising :-)

MOVQ $43, ret+0(FP)
@nigeltao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2016

Duh, of course it's DUPOK. It's been so long since I've seen a deliberate DUPOK that I forgot it existed.

I'm closing this as WAI.

The 7 occurs in https://github.com/klauspost/compress/blob/master/snappy/asm_amd64.s which is probably a copy/paste of old standard library .s files. See https://groups.google.com/d/msg/golang-nuts/uJTYuruvnZM/6j4KXgkdcf0J for some history.

@nigeltao nigeltao closed this Apr 14, 2016

@mwhudson

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2016

Well I think dupok is only ok if both symbols are dupok? But that admittedly is not what the code currently does.

@golang golang locked and limited conversation to collaborators Apr 15, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.