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

Open
aclements opened this issue Oct 21, 2016 · 17 comments
Open

cmd/vet: move asm checks into cmd/asm #17544

aclements opened this issue Oct 21, 2016 · 17 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis)
Milestone
Unplanned

Comments

@aclements
Copy link
Member

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
Copy link
Member Author

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

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
Copy link
Member

minux commented Oct 25, 2016 via email

@aclements
Copy link
Member Author

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

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

@josharian
Copy link
Contributor

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
Copy link
Member Author

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

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
Copy link
Member Author

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 cmd/vet: expand preprocessor macros in asm cmd/vet: move asm checks into cmd/asm Jun 22, 2017
@rsc rsc modified the milestones: Go1.10, Go1.9 Jun 22, 2017
@aclements
Copy link
Member Author

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

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

2 similar comments
@gopherbot
Copy link

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

@gopherbot
Copy link

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
@aclements
cmd/asm: factor out line parsing from assembling
52b2220 
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>
@josharian josharian mentioned this issue May 8, 2019
Closed
5 tasks
@gopherbot
Copy link

Change https://golang.org/cl/259338 mentions this issue: runtime: correct signature of call16

gopherbot pushed a commit that referenced this issue Oct 5, 2020
@aclements
runtime: correct signature of call16
9dc65d7 
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see #17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Fixes #41795.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/259597 mentions this issue: [release-branch.go1.14] runtime: correct signature of call16

@gopherbot
Copy link

Change https://golang.org/cl/259598 mentions this issue: [release-branch.go1.15] runtime: correct signature of call16

gopherbot pushed a commit that referenced this issue Oct 14, 2020
@aclements @toothrot
[release-branch.go1.15] runtime: correct signature of call16
8b224e9 
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see #17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Updates #41795.
Fixes #41797.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/259598
gopherbot pushed a commit that referenced this issue Oct 14, 2020
@aclements @toothrot
[release-branch.go1.14] runtime: correct signature of call16
0bf9410 
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see #17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Updates #41795.
Fixes #41796.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/259597
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
@aclements @claucece
[release-branch.go1.15] runtime: correct signature of call16
8da88ee 
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see golang#17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Updates golang#41795.
Fixes golang#41797.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/259598
@aclements
Copy link
Member Author

With the addition of ABIInternal assembly functions in runtime, vet is increasingly getting confused by the use of preprocessor in the runtime assembly code.

It also doesn't know how to check the calling conventions for ABIInternal functions and I don't think we want to put more knowledge like that into vet (for example, it doesn't know about ABIInternal when computing argument frame sizes, it gets the stack offsets of stack-assigned arguments and results wrong, and it doesn't have any mechanism for checking argument-assigned arguments or results). The compiler, of course, knows all these things, so to me this argues strongly for moving these checks into cmd/asm with a back-channel from the compiler.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 26, 2021
This was referenced Oct 11, 2021
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis)
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@josharian @rsc @minux @timothy-king @aclements @gopherbot