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/vet: asmdecl doesn't recognize #ifdefs in asm file #45318

Open
laboger opened this issue Mar 31, 2021 · 2 comments
Open

cmd/vet: asmdecl doesn't recognize #ifdefs in asm file #45318

laboger opened this issue Mar 31, 2021 · 2 comments

Comments

@laboger
Copy link
Contributor

@laboger laboger commented Mar 31, 2021

What version of Go are you using (go version)?

$ go version
tip

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
fails on linux-ppc64

What did you do?

Tried to build and test https://go-review.googlesource.com/c/go/+/306369. The build fails on ppc64 with a vet error because there is a function declaration in sys_linux_ppc64x.s inside an #ifdef GOARCH_ppc64le but vet's asmdecl considers it as existing on ppc64. It fails because the Go declaration for the function is built on ppc64le but not ppc64. This is displayed in the first Trybot failure on this CL; I tried to workaround the problem but got a different error the second time.

What did you expect to see?

Correct build and test.

What did you see instead?

runtime
./sys_linux_ppc64x.s:340:1: [ppc64] callCgoSigaction: function callCgoSigaction missing Go declaration

@dr2chase dr2chase added this to the Unplanned milestone Apr 2, 2021
@guodongli-google
Copy link

@guodongli-google guodongli-google commented Apr 5, 2021

The asmdecl checker infers the architecture from file name or the +build line, and assumes all the functions in the same file uses this architecture. So defining a function for ppc64le in a ppc64 file may not work:

#ifdef GOARCH_ppc64le
...
#endif

If we don't want to create a separate file "src/runtime/sys_linux_ppc64le.s" to avoid duplications, then we may need to change this checker's assumption, and examine the intent architecture for each function.

@laboger
Copy link
Contributor Author

@laboger laboger commented Apr 6, 2021

I think I have a workaround for this. Just create different stubs files for ppc64le and ppc64 and aix and put the Go prototype where it is needed by vet. Personally I think the tools should adapt to the code, and not make the code adapt to the tool. I see lines in several places saying "needed for vet."

Without knowing too much about how vet works, it must know what the target goos goarch is because it has decided to parse some files but not others since it has decided what prototype to remember for use when matching with the function in the asm file. With that information it could run the preprocessor or the equivalent on the source file before parsing it. The assembler must need to do that. Just a thought.

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
4 participants