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

x/tools/go/ssa: no position information for <binop>= #26294

Closed
dominikh opened this issue Jul 9, 2018 · 3 comments
Closed

x/tools/go/ssa: no position information for <binop>= #26294

dominikh opened this issue Jul 9, 2018 · 3 comments

Comments

@dominikh
Copy link
Member

dominikh commented Jul 9, 2018

go/ssa doesn't store any position information for <binop>= operators (e.g. x &= 123).

IMO this is trivially fixable by passing a position to (*builder).assignOp, but I'm not sure if there's more to this that I've missed.

Demonstrating the issue:
https://play.golang.org/p/hcDCHopxmYr

fn1:
x & 0:int @ 0
println(t0) @ 51
return @ 0

fn2:
x & 0:int @ 84
println(t0) @ 97
return @ 0

Potential fix:

diff --git a/ssa/builder.go b/ssa/builder.go
index 111bc33..bfb7a2b 100644
--- a/ssa/builder.go
+++ b/ssa/builder.go
@@ -970,9 +970,9 @@ func (b *builder) setCall(fn *Function, e *ast.CallExpr, c *CallCommon) {
 }
 
 // assignOp emits to fn code to perform loc += incr or loc -= incr.
-func (b *builder) assignOp(fn *Function, loc lvalue, incr Value, op token.Token) {
+func (b *builder) assignOp(fn *Function, loc lvalue, incr Value, op token.Token, pos token.Pos) {
        oldv := loc.load(fn)
-       loc.store(fn, emitArith(fn, op, oldv, emitConv(fn, incr, oldv.Type()), loc.typ(), token.NoPos))
+       loc.store(fn, emitArith(fn, op, oldv, emitConv(fn, incr, oldv.Type()), loc.typ(), pos))
 }
 
 // localValueSpec emits to fn code to define all of the vars in the
@@ -1998,7 +1998,7 @@ start:
                        op = token.SUB
                }
                loc := b.addr(fn, s.X, false)
-               b.assignOp(fn, loc, NewConst(exact.MakeInt64(1), loc.typ()), op)
+               b.assignOp(fn, loc, NewConst(exact.MakeInt64(1), loc.typ()), op, s.Pos())
 
        case *ast.AssignStmt:
                switch s.Tok {
@@ -2007,7 +2007,7 @@ start:
 
                default: // +=, etc.
                        op := s.Tok + token.ADD - token.ADD_ASSIGN
-                       b.assignOp(fn, b.addr(fn, s.Lhs[0], false), b.expr(fn, s.Rhs[0]), op)
+                       b.assignOp(fn, b.addr(fn, s.Lhs[0], false), b.expr(fn, s.Rhs[0]), op, s.Pos())
                }
 
        case *ast.GoStmt:

/cc @alandonovan @griesemer does the proposed patch look sensible for a CL?

@dominikh dominikh added this to the Unreleased milestone Jul 9, 2018
@alandonovan
Copy link
Contributor

Thanks. I'm not sure how this went unnoticed for so long. The fix looks good, very straightforward. (Almost too easy... I recall struggling with the position information to ensure that it was possible to find "the" SSA instruction for a given source operator, which means you can't reuse that same position for implicit conversions, loads, etc. But those concerns don't seem to apply here.)

@griesemer
Copy link
Contributor

@dominikh What is the status of this? Is there a pending CL? Should this be assigned to somebody (Alan)?

@gopherbot
Copy link

Change https://golang.org/cl/128776 mentions this issue: go/ssa: emit position information for assignment operators

@golang golang locked and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants