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: move asm checks into cmd/asm #17544

Open
aclements opened this Issue Oct 21, 2016 · 13 comments

Comments

Projects
None yet
5 participants
@aclements
Member

aclements commented Oct 21, 2016

Currently cmd/vet does not expand preprocessor macros in assembly code. As a result, it missed several issues in the runtime (a73d68e) and x/crypto (golang/crypto@5953a47, golang/crypto@e67f5ec, golang/crypto@3c0d69f). It would be nice if it could preprocess the assembly code before vetting it.

@aclements

This comment has been minimized.

Member

aclements commented Oct 21, 2016

/cc @josharian, since you have the perhaps dubious distinction of having the most recent commits to cmd/vet. :)

Probably the way to do this would be to add a flag to the cmd/asm that causes it to just run the lexer and dump the post-preprocessed output back out.

@josharian

This comment has been minimized.

Contributor

josharian commented Oct 23, 2016

A dubious distinction indeed. :)

This seems like a good idea. It'd also solve another annoyance with the current asmdecl check, which is poor handling of comments. The preprocessing would remove them.

I'd have to investigate implementation. Running cmd/asm with a flag would be slow and flags upset Rob. It might be better to just share an internal package with cmd/asm, or extract a standalone preprocessor.

@josharian josharian added this to the Go1.9 milestone Oct 23, 2016

@josharian josharian self-assigned this Oct 23, 2016

@minux

This comment has been minimized.

Member

minux commented Oct 25, 2016

@aclements

This comment has been minimized.

Member

aclements commented Oct 25, 2016

It might be better to just share an internal package with cmd/asm, or extract a standalone preprocessor.

I would be fine with that. Would vet's asm checks then work on the token stream instead of the text? That would require some rewriting, but may ultimately be a better approach.

@josharian

This comment has been minimized.

Contributor

josharian commented Oct 25, 2016

Yes, probably. The current code is a highly functional but rather unreadable pile of regexps and loops.

@josharian

This comment has been minimized.

Contributor

josharian commented Feb 20, 2017

I have a tentative plan here, but I'd like to run it by @robpike before I start putting together CLs. Proposal:

  1. Make package cmd/asm/internal/lex available to cmd/vet. Do this by moving cmd/asm/internal/* to cmd/internal/asm/*, or just moving cmd/asm/internal/lex to (say) cmd/internal/asmlex.
  2. Teach the lexer that textflag.h lives in GOROOT/runtime. Currently, cmd/go copies GOROOT/runtime/textflag.h into packages as part of building them. Instead of teaching cmd/vet to do this as well, and also clean them up again afterwards, teach the lexer to recognize textflag.h and treat it specially by reading it from GOROOT/runtime if not present in the current directory.
  3. Have asmdecl recreate the preprocessed input using the token stream, tracking line numbers carefully. The rest of the asmdecl code remains unchanged; it remains a pile of regexps. This reduces rewrite risk. After this step, this issue could be considered closed.
  4. Optional: Have asmdecl work off the token stream directly. Eliminates a bunch of regular expressions. Should be more efficient, but there are no functional changes, so could reasonable be delayed for later or skipped entirely. My inclination is to skip it for now, but I'll do it if that's the only way to get approval for steps 1-3.

Rob, what do you think?

@aclements

This comment has been minimized.

Member

aclements commented Feb 20, 2017

@rsc and I were chatting a bit about this and he suggested considering making the assembler just do the checks that vet does now.

@josharian

This comment has been minimized.

Contributor

josharian commented Feb 20, 2017

That's appealing. I see a few hitches:

  1. I don't think the assembler currently has the go function prototypes used to check the offset names. And those prototypes can occur anywhere, so the assembler (or cmd/go) would have to parse an entire package to do those checks. That's relatively expensive, and a significant scope increase.

  2. There are a few assembly routines that knowingly and correctly violate the asmdecl rules, such as reflect.makeFuncStub, reflect.methodValueCall, and runtime.morestack, which do unusual things with the stack. We'd need to either hard-code a whitelist of these or make a way to mark an assembly routine //go:no-vet or some such.

  3. It might be easier to get there incrementally via vet (as laid out above), because the rewrite would involve no functional changes, reuse existing tests, be fuzz-testable, etc. It could then be migrated into cmd/asm, assuming we had a plan for (1) and (2) above.

Thoughts?

@aclements

This comment has been minimized.

Member

aclements commented Feb 20, 2017

As for 1, I think the idea was to emit these from the compiler along with go_asm.h. The build system already has to run the compiler before the assembler to generate go_asm.h. We've been talking about doing something similar as a possible part of registerizing the calling convention and dealing with existing assembly code.

@rsc rsc changed the title from cmd/vet: expand preprocessor macros in asm to cmd/vet: move asm checks into cmd/asm Jun 22, 2017

@rsc rsc modified the milestones: Go1.10, Go1.9 Jun 22, 2017

@aclements

This comment has been minimized.

Member

aclements commented Oct 27, 2017

I just ran across a few more issues with vet's assembly checks. It doesn't understand hex in the frame size, so it parses the frame size as 0. And it doesn't understand expressions in offsets, so, e.g., (2*8)(SP) is simply ignored.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Nov 2, 2018

Change https://golang.org/cl/147097 mentions this issue: cmd/asm: factor out line parsing from assembling

2 similar comments
@gopherbot

This comment has been minimized.

gopherbot commented Nov 2, 2018

Change https://golang.org/cl/147097 mentions this issue: cmd/asm: factor out line parsing from assembling

@gopherbot

This comment has been minimized.

gopherbot commented Nov 2, 2018

Change https://golang.org/cl/147097 mentions this issue: cmd/asm: factor out line parsing from assembling

gopherbot pushed a commit that referenced this issue Nov 12, 2018

cmd/asm: factor out line parsing from assembling
Currently cmd/asm's Parser.line both consumes a line of assembly from
the lexer and assembles it. This CL separates these two steps so that
the line parser can be reused for purposes other than generating a
Prog stream.

For #27539.
Updates #17544.

Change-Id: I452c9a2112fbcc1c94bf909efc0d1fcc71014812
Reviewed-on: https://go-review.googlesource.com/c/147097
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment