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

Closed
laboger opened this issue Mar 31, 2021 · 3 comments
Closed

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

laboger opened this issue Mar 31, 2021 · 3 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@laboger
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 2, 2021
@dr2chase dr2chase added this to the Unplanned milestone Apr 2, 2021
@guodongli-google
Copy link

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.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 5, 2021
@laboger
Copy link
Contributor Author

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.

@aclements
Copy link
Member

There are a lot of problems with asmdecl right now, unfortunately. I'm going to close this as a duplicate of #17544.

With that information it could run the preprocessor or the equivalent on the source file before parsing it.

This is tricky because there's no separate preprocessor right now for vet to run. In fact, calling what asmdecl does "parsing" would be quite generous. It's really just a pile of ad hoc regexps. This is why we'd like to just integrate this into cmd/asm itself.

@golang golang locked and limited conversation to collaborators Oct 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants