Skip to content

cmd/compile: MOV $0, r to XOR r, r rewrite is unsafe #12405

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.

Sign up for GitHub

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
Closed
josharian opened this issue Aug 30, 2015 · 4 comments
Closed

cmd/compile: MOV $0, r to XOR r, r rewrite is unsafe #12405

josharian opened this issue Aug 30, 2015 · 4 comments
Labels
FrozenDueToAge

Comments

@josharian
Copy link
Contributor

liblink rewrites MOV $0, r to XOR r, r. MOV does not impact flags, but XOR does. As a result, this rewrite is not generally safe. This was the source of a bug in the SSA branch (see CL 14042 and 14043). It could also impact hand-written assembly. It is an accident of the current backend codegen that it doesn't cause bugs now.

We should remove the liblink rewrite and generate XOR r, r directly if/when that is desired. For example, for the existing backend, this could become part of the peephole optimizer; we can update the existing handwritten assembly.

cc @bradfitz @tzneal @randall77
@rsc
Copy link
Contributor

rsc commented Oct 23, 2015

I am not convinced. "MOV" is a pseudo-operation in the Go assembler. It is NOT just one instruction. XOR is one possible implementation of that pseudo-operation. Therefore SSA should expect that MOV can affect flags.

@randall77
Copy link
Contributor

Yes, that was our fix, marking MOVx as clobbering flags. Even though most of the time it won't.
Meta-question - how do we know what is a pseudo-op and what isn't?

@rsc
Copy link
Contributor

rsc commented Oct 23, 2015

MOV is the main one. I know that's not terribly precise. Eventually we'll have a formal statement of all this that you could just look at.

@rsc rsc added this to the dev.ssa milestone Oct 23, 2015
@rsc
Copy link
Contributor

rsc commented Oct 23, 2015

Anyway, sounds like this got fixed.

@rsc rsc closed this as completed Oct 23, 2015
gopherbot pushed a commit that referenced this issue Jan 26, 2016
@randall77
[dev.ssa] cmd/compile: disable xor clearing when flags must be preserved
7b77394 
The x86 backend automatically rewrites MOV $0, AX to
XOR AX, AX.  That rewrite isn't ok when the flags register
is live across the MOV.  Keep track of which moves care
about preserving flags, then disable this rewrite for them.

On x86, Prog.Mark was being used to hold the length of the
instruction.  We already store that in Prog.Isize, so no
need to store it in Prog.Mark also.  This frees up Prog.Mark
to hold a bitmask on x86 just like all the other architectures.

Update #12405

Change-Id: Ibad8a8f41fc6222bec1e4904221887d3cc3ca029
Reviewed-on: https://go-review.googlesource.com/18861
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Oct 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@josharian @rsc @randall77 @gopherbot