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

Jump to bottom

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
Closed

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

nigeltao opened this issue Apr 14, 2016 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@nigeltao
Copy link
Contributor 

$ 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
Copy link
Contributor

/cc @ianlancetaylor @crawshaw @rsc

@bradfitz bradfitz added this to the Go1.7 milestone Apr 14, 2016
@rsc
Copy link
Contributor

rsc 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
Copy link
Contributor

$ 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
Copy link
Contributor Author

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.

@mwhudson
Copy link
Contributor

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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
5 participants
@bradfitz @rsc @mwhudson @nigeltao @gopherbot