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: improve error for missing method body #48780

Open
ianlancetaylor opened this issue Oct 4, 2021 · 20 comments
Open

cmd/link: improve error for missing method body #48780

ianlancetaylor opened this issue Oct 4, 2021 · 20 comments

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 4, 2021

The language permits omitting the method body. However, the error message for this could be clearer.

Test case (uses import "C" to force the compiler to assume that method bodies are defined elsewhere):

package main

import "C"

type S struct{}

func (S) M()

var Sink interface{}

func main() {
	Sink = S{}
}

Running go build foo.go produces

type.main.S: relocation target main.S.M not defined

This error message is accurate but conveys little information to someone unfamiliar with linker jargon like "relocation target." We should instead say "missing function body", though unfortunately it may be difficult to give a good file/line position for the declaration with the missing body.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 4, 2021

Good point. I guess one difficulty is that for an undefined symbol the linker doesn't know if it is supposed to be a function or not. Maybe we could make some guess, like assuming all ABIInternal references are functions (which will probably help this one), or name-based guessing.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 4, 2021

Or we could say "undefined function or variable".

@deepto98
Copy link

@deepto98 deepto98 commented Oct 4, 2021

@ianlancetaylor @cherrymui Hi, I'd like to pick this issue up? Can you please assign it, if it isn't being worked on already? Also could you point me to the relevant files for the error definitions?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 4, 2021

@deepto98 thanks. The error is emitted at https://cs.opensource.google/go/go/+/master:src/cmd/link/internal/ld/errors.go;l=63
Feel free to send a CL when you ready. (But note that I'm still unsure what the best solution is.)

@deepto98
Copy link

@deepto98 deepto98 commented Oct 5, 2021

@cherrymui @ianlancetaylor I'm trying to reproduce this issue, but I get a different error for the code snippet given:
image

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 5, 2021

@deepto98 Please include plain text messages as plain text, not as an image. Images are very hard to read. Thanks.

The error in the image appears to be from the external linker. Unfortunately I don't know that there is much that we can do about this when in external linking mode. But I also don't know why you didn't see the same error that I did. What operating system are you on? I am on amd64-linux.

@deepto98
Copy link

@deepto98 deepto98 commented Oct 5, 2021

Okay, sure, will keep this in mind about error messages. I'm also on linux (Ubuntu 20.04) and the go version I'm using is go1.12.2 gccgo (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 linux/amd64 . I added the code to a file (foo.go) and ran go build foo.go

@deepto98
Copy link

@deepto98 deepto98 commented Oct 5, 2021

@ianlancetaylor I wasn't aware about external and internal linking, and just read up on it. Do you know how I can change the config to switch to internal linking?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 5, 2021

This issue is not about gccgo. This is about the Go toolchain distributed from https://golang.org/dl/ , also known as "the gc toolchain".

@deepto98
Copy link

@deepto98 deepto98 commented Oct 5, 2021

@cherrymui okay, got it, my bad. I just installed go 1.16.8 and was able to reproduce the error you'd mentioned initially

@deepto98
Copy link

@deepto98 deepto98 commented Oct 5, 2021

@cherrymui Should I change the error message format to say type.main.S: undefined function or variable?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 5, 2021

I don't know. I think it is important to print the undefined symbol name, i.e. the main.S.M in the original post. But I don't know the exact wording.

@deepto98
Copy link

@deepto98 deepto98 commented Oct 5, 2021

okay

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 6, 2021

Does the linker have any way of knowing that an undefined symbol is a function? I can imagine various ways to determining that but I don't know if the linker currently does any of them.

@deepto98
Copy link

@deepto98 deepto98 commented Oct 6, 2021

okay, I'm looking into this

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 6, 2021

@ianlancetaylor I don't think the linker has a very definitive answer. I posted some thoughts in #48780 (comment)

@deepto98
Copy link

@deepto98 deepto98 commented Oct 6, 2021

@cherrymui @ianlancetaylor from what I found out for empty variable definitions, they're already handled in /go/src/go/parser/parser.go with messages missing variable type or initialization or missing constant value, so maybe we can assume that ABI reference symbols are functions, as @cherrymui had suggested?

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 6, 2021

We may have to consider references from assembly code.

@deepto98
Copy link

@deepto98 deepto98 commented Oct 6, 2021

@ianlancetaylor Okay, I'm looking into this. Can please you point me to some resources so I can test this with assembly?

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 6, 2021

An example of assembly code referring to a variable is runtime/asm_amd64.s. These lines assign to the variables _rt0_amd64_lib_argc and _rt0_amd64_lib_argv.

	MOVQ	DI, _rt0_amd64_lib_argc<>(SB)
	MOVQ	SI, _rt0_amd64_lib_argv<>(SB)

Those variables are defined in the same assembly file using:

DATA _rt0_amd64_lib_argc<>(SB)/8, $0
GLOBL _rt0_amd64_lib_argc<>(SB),NOPTR, $8
DATA _rt0_amd64_lib_argv<>(SB)/8, $0
GLOBL _rt0_amd64_lib_argv<>(SB),NOPTR, $8

If those DATA and GLOBL lines are removed, you will have assembly code that refers to undefined variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants